199074 2019-06-20 11:32:01 -0700 Storage Access API: Cap the number of times an iframe document can request access 2019-06-20 13:04:05 -0700 1 1 1 Unclassified WebKit WebCore Misc. WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 wilander wilander bfulgham cdumez commit-queue dbates esprehn+autocc ews-watchlist kangil.han webkit-bug-importer oldest_to_newest 1546589 0 wilander 2019-06-20 11:32:01 -0700 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. 1546590 1 wilander 2019-06-20 11:32:28 -0700 <rdar://problem/51857195> 1546597 2 372576 wilander 2019-06-20 11:46:57 -0700 Created attachment 372576 Patch 1546605 3 372576 bfulgham 2019-06-20 12:00:45 -0700 Comment on attachment 372576 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? 1546607 4 wilander 2019-06-20 12:05:22 -0700 (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. 1546608 5 372576 bfulgham 2019-06-20 12:08:21 -0700 Comment on attachment 372576 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. 1546609 6 wilander 2019-06-20 12:10:12 -0700 (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! 1546622 7 372576 commit-queue 2019-06-20 13:04:04 -0700 Comment on attachment 372576 Patch Clearing flags on attachment: 372576 Committed r246647: <https://trac.webkit.org/changeset/246647> 1546623 8 commit-queue 2019-06-20 13:04:05 -0700 All reviewed patches have been landed. Closing bug. 372576 2019-06-20 11:46:57 -0700 2019-06-20 13:04:04 -0700 Patch bug-199074-20190620114656.patch text/plain 4303 wilander U3VidmVyc2lvbiBSZXZpc2lvbjogMjQ2NjQwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZjU3YTE3ZDA0ZDRiZmMy ZGQ5ODA4NTE2OWRhNjk3NzBiZjgyMDQ5Yi4uMTA0ZGE2ZDM1ZmVhYTI4YzAzOGJiZmMyZTExNDIz YmMxMWZiYmY1ZiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDMwIEBACisyMDE5LTA2LTIwICBKb2hu IFdpbGFuZGVyICA8d2lsYW5kZXJAYXBwbGUuY29tPgorCisgICAgICAgIFN0b3JhZ2UgQWNjZXNz IEFQSTogQ2FwIHRoZSBudW1iZXIgb2YgdGltZXMgYW4gaWZyYW1lIGRvY3VtZW50IGNhbiByZXF1 ZXN0IGFjY2VzcworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/ aWQ9MTk5MDc0CisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS81MTg1NzE5NT4KKworICAgICAgICBS ZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBUZXN0ZWQgbWFudWFsbHkuCisK KyAgICAgICAgVGhpcyBjaGFuZ2UganVzdCBhZGRzIGEgY291bnRlciB0byB0aGUgbnVtYmVyIG9m IHRpbWVzIHRoZSB1c2VyIGV4cGxpY2l0bHkKKyAgICAgICAgZGVuaWVzIHN0b3JhZ2UgYWNjZXNz IGFuZCByZXR1cm5zIGVhcmx5IGlmIHRoZSBjb3VudGVyIGhhcyByZWFjaGVkIHRoZSBsaW1pdAor ICAgICAgICBvZiAyLgorCisgICAgICAgIFdlIGhvcGVkIHRoYXQgaWZyYW1lcyB0aGF0IHJlcXVl c3Qgc3RvcmFnZSBhY2Nlc3Mgd291bGQgY291bnQgdGhlIG51bWJlcgorICAgICAgICBvZiB0aW1l cyB0aGUgdXNlciBoYXMgYmVlbiBhc2tlZCBhbmQgbm90IHJlcGVhdCB0aGUgcmVxdWVzdCBvdmVy IGFuZCBvdmVyLgorICAgICAgICBIb3dldmVyLCB3ZSdyZSBzZWVpbmcgcHJldHR5IGFnZ3Jlc3Np dmUgdXNlIG9mIHRoZSBBUEkgYW5kIHVzZXJzIGFyZQorICAgICAgICBjb21wbGFpbmluZy4gVGhl cmVmb3JlLCB3ZSBuZWVkIGEgY2FwIG9uIGhvdyBtYW55IHRpbWVzIGFuIGlmcmFtZWQKKyAgICAg ICAgZG9jdW1lbnQgY2FuIGFzayBpZiBpdCBpcyBleHBsaWNpdGx5IGRlbmllZCBhY2Nlc3MgYnkg dGhlIHVzZXIuCisKKyAgICAgICAgVGhpcyBpcyBhIGZpcnN0IG1lYXN1cmUuIElmIHdlIHNlZSBj b250aW51ZWQgYWdncmVzc2l2ZSB1c2Ugb2YgdGhlIEFQSSwKKyAgICAgICAgd2UnbGwgaGF2ZSB0 byBjb25zaWRlciBtb3JlIGRyYXN0aWMgbWVhc3VyZXMuCisKKyAgICAgICAgKiBkb20vRG9jdW1l bnRTdG9yYWdlQWNjZXNzLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkRvY3VtZW50U3RvcmFnZUFj Y2Vzczo6cmVxdWVzdFN0b3JhZ2VBY2Nlc3MpOgorICAgICAgICAqIGRvbS9Eb2N1bWVudFN0b3Jh Z2VBY2Nlc3MuaDoKKwogMjAxOS0wNi0yMCAgU2FhbSBCYXJhdGkgIDxzYmFyYXRpQGFwcGxlLmNv bT4KIAogICAgICAgICBVbnJldmlld2VkLiBTcGVjdWxhdGl2ZSBidWlsZCBmaXggZm9yIHdhdGNo T1MgYWZ0ZXIgcjI0NjYzMS4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2RvbS9Eb2N1bWVu dFN0b3JhZ2VBY2Nlc3MuY3BwIGIvU291cmNlL1dlYkNvcmUvZG9tL0RvY3VtZW50U3RvcmFnZUFj Y2Vzcy5jcHAKaW5kZXggNGEzM2FhOTBmOWQyZmU0Nzc4MmJlYmQ4YmUzNTc4NGJjMDZlNmQ4NS4u ODFlZmIxMGQ1ZTc2ZWRiNmY2ZDM4MzUzMDA1MWQyOWMzODhlYWZkYiAxMDA2NDQKLS0tIGEvU291 cmNlL1dlYkNvcmUvZG9tL0RvY3VtZW50U3RvcmFnZUFjY2Vzcy5jcHAKKysrIGIvU291cmNlL1dl YkNvcmUvZG9tL0RvY3VtZW50U3RvcmFnZUFjY2Vzcy5jcHAKQEAgLTEyOCw3ICsxMjgsNyBAQCB2 b2lkIERvY3VtZW50U3RvcmFnZUFjY2Vzczo6cmVxdWVzdFN0b3JhZ2VBY2Nlc3MoUmVmPERlZmVy cmVkUHJvbWlzZT4mJiBwcm9taXNlKQogICAgICAgICByZXR1cm47CiAgICAgfQogICAgIAotICAg IGlmICghbV9kb2N1bWVudC5mcmFtZSgpIHx8IG1fZG9jdW1lbnQuc2VjdXJpdHlPcmlnaW4oKS5p c1VuaXF1ZSgpKSB7CisgICAgaWYgKCFtX2RvY3VtZW50LmZyYW1lKCkgfHwgbV9kb2N1bWVudC5z ZWN1cml0eU9yaWdpbigpLmlzVW5pcXVlKCkgfHwgIWlzQWxsb3dlZFRvUmVxdWVzdEZyYW1lU3Bl Y2lmaWNTdG9yYWdlQWNjZXNzKCkpIHsKICAgICAgICAgcHJvbWlzZS0+cmVqZWN0KCk7CiAgICAg ICAgIHJldHVybjsKICAgICB9CkBAIC0xOTIsOCArMTkyLDExIEBAIHZvaWQgRG9jdW1lbnRTdG9y YWdlQWNjZXNzOjpyZXF1ZXN0U3RvcmFnZUFjY2VzcyhSZWY8RGVmZXJyZWRQcm9taXNlPiYmIHBy b21pc2UpCiAgICAgICAgIGlmICh3YXNHcmFudGVkID09IFN0b3JhZ2VBY2Nlc3NXYXNHcmFudGVk OjpZZXMpIHsKICAgICAgICAgICAgIGRvY3VtZW50LT5zZXRIYXNGcmFtZVNwZWNpZmljU3RvcmFn ZUFjY2Vzcyh0cnVlKTsKICAgICAgICAgICAgIHByb21pc2UtPnJlc29sdmUoKTsKLSAgICAgICAg fSBlbHNlCisgICAgICAgIH0gZWxzZSB7CisgICAgICAgICAgICBpZiAocHJvbXB0V2FzU2hvd24g PT0gU3RvcmFnZUFjY2Vzc1Byb21wdFdhc1Nob3duOjpZZXMpCisgICAgICAgICAgICAgICAgZG9j dW1lbnQtPnNldFdhc0V4cGxpY2l0bHlEZW5pZWRGcmFtZVNwZWNpZmljU3RvcmFnZUFjY2Vzcygp OwogICAgICAgICAgICAgcHJvbWlzZS0+cmVqZWN0KCk7CisgICAgICAgIH0KIAogICAgICAgICBp ZiAoc2hvdWxkUHJlc2VydmVVc2VyR2VzdHVyZSkgewogICAgICAgICAgICAgTWljcm90YXNrUXVl dWU6Om1haW5UaHJlYWRRdWV1ZSgpLmFwcGVuZChzdGQ6Om1ha2VfdW5pcXVlPFZvaWRNaWNyb3Rh c2s+KFtkb2N1bWVudFJlZmVyZW5jZSA9IFdURk1vdmUoZG9jdW1lbnRSZWZlcmVuY2UpXSAoKSB7 CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9kb20vRG9jdW1lbnRTdG9yYWdlQWNjZXNzLmgg Yi9Tb3VyY2UvV2ViQ29yZS9kb20vRG9jdW1lbnRTdG9yYWdlQWNjZXNzLmgKaW5kZXggNmE2Zjhj MTlkMzhhYWMzMzBjZWY1YWQ0ZDMzODc1YWU1MzE1YTQ1Yy4uY2U3ZjMxMDBiYmVlYTk4Nzk1YmYz YzI3NmY4YzNlMDZmZTA0YmQ5YiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvZG9tL0RvY3Vt ZW50U3RvcmFnZUFjY2Vzcy5oCisrKyBiL1NvdXJjZS9XZWJDb3JlL2RvbS9Eb2N1bWVudFN0b3Jh Z2VBY2Nlc3MuaApAQCAtNDYsNiArNDYsOCBAQCBlbnVtIGNsYXNzIFN0b3JhZ2VBY2Nlc3NQcm9t cHRXYXNTaG93biA6IGJvb2wgewogICAgIFllcwogfTsKIAorY29uc3QgdW5zaWduZWQgbWF4TnVt YmVyT2ZUaW1lc0V4cGxpY2l0bHlEZW5pZWRGcmFtZVNwZWNpZmljU3RvcmFnZUFjY2VzcyA9IDI7 CisKIGNsYXNzIERvY3VtZW50U3RvcmFnZUFjY2VzcyBmaW5hbCA6IHB1YmxpYyBTdXBwbGVtZW50 PERvY3VtZW50PiwgcHVibGljIENhbk1ha2VXZWFrUHRyPERvY3VtZW50U3RvcmFnZUFjY2Vzcz4g ewogICAgIFdURl9NQUtFX0ZBU1RfQUxMT0NBVEVEOwogcHVibGljOgpAQCAtNjQsMTIgKzY2LDE2 IEBAIHByaXZhdGU6CiAgICAgc3RhdGljIGNvbnN0IGNoYXIqIHN1cHBsZW1lbnROYW1lKCk7CiAg ICAgYm9vbCBoYXNGcmFtZVNwZWNpZmljU3RvcmFnZUFjY2VzcygpIGNvbnN0OwogICAgIHZvaWQg c2V0SGFzRnJhbWVTcGVjaWZpY1N0b3JhZ2VBY2Nlc3MoYm9vbCk7CisgICAgdm9pZCBzZXRXYXNF eHBsaWNpdGx5RGVuaWVkRnJhbWVTcGVjaWZpY1N0b3JhZ2VBY2Nlc3MoKSB7ICsrbV9udW1iZXJP ZlRpbWVzRXhwbGljaXRseURlbmllZEZyYW1lU3BlY2lmaWNTdG9yYWdlQWNjZXNzOyB9OworICAg IGJvb2wgaXNBbGxvd2VkVG9SZXF1ZXN0RnJhbWVTcGVjaWZpY1N0b3JhZ2VBY2Nlc3MoKSB7IHJl dHVybiBtX251bWJlck9mVGltZXNFeHBsaWNpdGx5RGVuaWVkRnJhbWVTcGVjaWZpY1N0b3JhZ2VB Y2Nlc3MgPCBtYXhOdW1iZXJPZlRpbWVzRXhwbGljaXRseURlbmllZEZyYW1lU3BlY2lmaWNTdG9y YWdlQWNjZXNzOyB9OwogICAgIHZvaWQgZW5hYmxlVGVtcG9yYXJ5VGltZVVzZXJHZXN0dXJlKCk7 CiAgICAgdm9pZCBjb25zdW1lVGVtcG9yYXJ5VGltZVVzZXJHZXN0dXJlKCk7CiAKICAgICBzdGQ6 OnVuaXF1ZV9wdHI8VXNlckdlc3R1cmVJbmRpY2F0b3I+IG1fdGVtcG9yYXJ5VXNlckdlc3R1cmU7 CiAgICAgCiAgICAgRG9jdW1lbnQmIG1fZG9jdW1lbnQ7CisKKyAgICB1aW50OF90IG1fbnVtYmVy T2ZUaW1lc0V4cGxpY2l0bHlEZW5pZWRGcmFtZVNwZWNpZmljU3RvcmFnZUFjY2VzcyA9IDA7CiB9 OwogCiB9IC8vIG5hbWVzcGFjZSBXZWJDb3JlCg==