Bug 199074 - Storage Access API: Cap the number of times an iframe document can request access
Summary: Storage Access API: Cap the number of times an iframe document can request ac...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-20 11:32 PDT by John Wilander
Modified: 2019-06-20 13:04 PDT (History)
8 users (show)

See Also:


Attachments
Patch (4.20 KB, patch)
2019-06-20 11:46 PDT, John Wilander
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Wilander 2019-06-20 11:32:01 PDT
We hoped that iframes that request storage access would count the number of times the user has been asked and not repeat the request over and over. However, we're seeing pretty aggressive use of the API and users are complaining. Therefore, we should implement a cap on how many times an iframed document can ask if it is explicitly denied access by the user. This is a first measure. If we see continued aggressive use of the API, we'll have to consider more drastic measures.
Comment 1 John Wilander 2019-06-20 11:32:28 PDT
<rdar://problem/51857195>
Comment 2 John Wilander 2019-06-20 11:46:57 PDT
Created attachment 372576 [details]
Patch
Comment 3 Brent Fulgham 2019-06-20 12:00:45 PDT
Comment on attachment 372576 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372576&action=review

> Source/WebCore/dom/DocumentStorageAccess.h:69
> +    void setWasExplicitlyDeniedFrameSpecificStorageAccess() { ++m_numberOfTimesExplicitlyDeniedFrameSpecificStorageAccess; };

This seems to increment count for all frames. So, if a page embedded a frame from 'social.com', a second from 'video.com', and a third from 'music.com', the 'music.com' site might get blocked even though each frame had only asked for permission once.

Is that a problem?

I'm trying to remember if storage access is gated on the user interacting with the frame. Are we hitting these multiple requests when just loading the page?

Would it be better to perform the counting at the {document, frame} pair, instead of just all access requests of any kind incrementing a single top-level document counter?
Comment 4 John Wilander 2019-06-20 12:05:22 PDT
(In reply to Brent Fulgham from comment #3)
> Comment on attachment 372576 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=372576&action=review
> 
> > Source/WebCore/dom/DocumentStorageAccess.h:69
> > +    void setWasExplicitlyDeniedFrameSpecificStorageAccess() { ++m_numberOfTimesExplicitlyDeniedFrameSpecificStorageAccess; };
> 
> This seems to increment count for all frames. So, if a page embedded a frame
> from 'social.com', a second from 'video.com', and a third from 'music.com',
> the 'music.com' site might get blocked even though each frame had only asked
> for permission once.

I don't think that's the case. The DocumentStorageAccess class supplements Document and has a reference to the document it belongs to. So the count should be for this document in the iframe. I tried to express this scope in the bug title and explanation.

> Is that a problem?
> 
> I'm trying to remember if storage access is gated on the user interacting
> with the frame. Are we hitting these multiple requests when just loading the
> page?

The user has to interact with the iframe. The problem is that these iframes just keep asking if the user says "Don't Allow." Being able to prompt a second time makes sense since the user might not be happy with the result of choosing "Don't Allow." But after two explicit rejections, the iframe should back off.

> Would it be better to perform the counting at the {document, frame} pair,
> instead of just all access requests of any kind incrementing a single
> top-level document counter?

See above.
Comment 5 Brent Fulgham 2019-06-20 12:08:21 PDT
Comment on attachment 372576 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372576&action=review

r=me

>>> Source/WebCore/dom/DocumentStorageAccess.h:69
>>> +    void setWasExplicitlyDeniedFrameSpecificStorageAccess() { ++m_numberOfTimesExplicitlyDeniedFrameSpecificStorageAccess; };
>> 
>> This seems to increment count for all frames. So, if a page embedded a frame from 'social.com', a second from 'video.com', and a third from 'music.com', the 'music.com' site might get blocked even though each frame had only asked for permission once.
>> 
>> Is that a problem?
>> 
>> I'm trying to remember if storage access is gated on the user interacting with the frame. Are we hitting these multiple requests when just loading the page?
>> 
>> Would it be better to perform the counting at the {document, frame} pair, instead of just all access requests of any kind incrementing a single top-level document counter?
> 
> I don't think that's the case. The DocumentStorageAccess class supplements Document and has a reference to the document it belongs to. So the count should be for this document in the iframe. I tried to express this scope in the bug title and explanation.

Ah! Understood. The Document here is the frame's document, not the overall top-level document.

This is a good change. r=me.
Comment 6 John Wilander 2019-06-20 12:10:12 PDT
(In reply to Brent Fulgham from comment #5)
> Comment on attachment 372576 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=372576&action=review
> 
> r=me
> 
> >>> Source/WebCore/dom/DocumentStorageAccess.h:69
> >>> +    void setWasExplicitlyDeniedFrameSpecificStorageAccess() { ++m_numberOfTimesExplicitlyDeniedFrameSpecificStorageAccess; };
> >> 
> >> This seems to increment count for all frames. So, if a page embedded a frame from 'social.com', a second from 'video.com', and a third from 'music.com', the 'music.com' site might get blocked even though each frame had only asked for permission once.
> >> 
> >> Is that a problem?
> >> 
> >> I'm trying to remember if storage access is gated on the user interacting with the frame. Are we hitting these multiple requests when just loading the page?
> >> 
> >> Would it be better to perform the counting at the {document, frame} pair, instead of just all access requests of any kind incrementing a single top-level document counter?
> > 
> > I don't think that's the case. The DocumentStorageAccess class supplements Document and has a reference to the document it belongs to. So the count should be for this document in the iframe. I tried to express this scope in the bug title and explanation.
> 
> Ah! Understood. The Document here is the frame's document, not the overall
> top-level document.
> 
> This is a good change. r=me.

I just tested to make sure, and it is scoped to the iframed document.

Thanks for the review!
Comment 7 WebKit Commit Bot 2019-06-20 13:04:04 PDT
Comment on attachment 372576 [details]
Patch

Clearing flags on attachment: 372576

Committed r246647: <https://trac.webkit.org/changeset/246647>
Comment 8 WebKit Commit Bot 2019-06-20 13:04:05 PDT
All reviewed patches have been landed.  Closing bug.