Storage Access API: Cap the number of times an iframe document can request access

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[John Wilander]

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.

[John Wilander]

<rdar://problem/51857195>

[John Wilander]

Created attachment 372576 [details]
Patch

[Brent Fulgham]

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?

[John Wilander]

(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.

[Brent Fulgham]

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.

[John Wilander]

(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!

[WebKit Commit Bot]

Comment on attachment 372576 [details]
Patch

Clearing flags on attachment: 372576

Committed r246647: <https://trac.webkit.org/changeset/246647>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.