155609 2016-03-17 16:30:49 -0700 Local file restrictions are also restricting session storage 2016-11-10 12:34:40 -0800 1 1 1 Unclassified WebKit WebKit Misc. WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 155185 164592 1 bfulgham bfulgham aestes andersca ap beidson bfulgham commit-queue dbates simon.fraser webkit-bug-importer oldest_to_newest 1175909 0 bfulgham 2016-03-17 16:30:49 -0700 In Bug 155185, I inadvertently blocked access to sessionStorage for local files. This should still be permitted. 1175911 1 webkit-bug-importer 2016-03-17 16:41:47 -0700 <rdar://problem/25229461> 1175917 2 274341 bfulgham 2016-03-17 16:58:18 -0700 Created attachment 274341 Patch 1176042 3 274341 dbates 2016-03-17 22:58:29 -0700 Comment on attachment 274341 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274341&action=review > Source/WebCore/ChangeLog:13 > + Use of 'sesssionStorage' is governed by SecurityOrigin with third party access > + set to 'ShouldAllowFromThirdParty::AlwaysAllowFromThirdParty'. We should not > + reject local files for this combination of arguments. The bug description should come before the the list of included tests. > LayoutTests/storage/domstorage/sessionstorage/blocked-file-access-expected.txt:3 > +Test that we are permitted access to sessionStorage from a file URL if unversal access is turned off. Nit: unversal => universal > LayoutTests/storage/domstorage/sessionstorage/blocked-file-access.html:14 > +Test that we are permitted access to sessionStorage from a file URL if unversal access is turned off. Ditto. > LayoutTests/storage/domstorage/sessionstorage/resources/blocked-example.html:16 > + if (window.testRunner) > + testRunner.notifyDone(); This test does not make use of waitUntilDone() so this is not needed. 1176045 4 dbates 2016-03-17 23:00:28 -0700 (In reply to comment #3) > > LayoutTests/storage/domstorage/sessionstorage/resources/blocked-example.html:16 > > + if (window.testRunner) > > + testRunner.notifyDone(); > > This test does not make use of waitUntilDone() so this is not needed. Disregard this remark. 1176048 5 274341 dbates 2016-03-17 23:10:58 -0700 Comment on attachment 274341 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274341&action=review > LayoutTests/storage/domstorage/sessionstorage/blocked-file-access.html:1 > +<html> I do not see the need for this test to use quirks mode. We should opt into standards mode by adding <!DOCTYPE html>. > LayoutTests/storage/domstorage/sessionstorage/blocked-file-access.html:12 > +<iframe src="resources/blocked-example.html"></iframe> On another note, is is necessary to embed other page in an iframe? I mean, could we incorporate the content of file resources/blocked-example.html directly into this file? > LayoutTests/storage/domstorage/sessionstorage/resources/blocked-example.html:1 > +<html> I do not see the need for this test to use quirks mode. We should opt into standards mode by adding <!DOCTYPE html>. > LayoutTests/storage/domstorage/sessionstorage/resources/blocked-example.html:10 > + if (window.sessionStorage) { > + console.log("PASS: window.sessionStorage WAS accessible"); > + } Nit: We tend to follow the WebKit Code Style Guidelines for JavaScriptCore and omit braces for if-bocks whose body is a single line. This is OK as-is. I do not see the need to emphasize the word "was" by writing it in all capitals. > LayoutTests/storage/domstorage/sessionstorage/resources/blocked-example.html:11 > + } catch(e) { Nit: Missing space before '('. > LayoutTests/storage/domstorage/sessionstorage/resources/blocked-example.html:12 > + console.log("FAIL: window.sessionStorage was NOT accessible"); This is OK as-is. I do not see the need to emphasize the word "not" by writing it in all capitals. >> LayoutTests/storage/domstorage/sessionstorage/resources/blocked-example.html:16 >> + testRunner.notifyDone(); > > This test does not make use of waitUntilDone() so this is not needed. Never mind. 1176259 6 274341 bfulgham 2016-03-18 12:02:18 -0700 Comment on attachment 274341 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274341&action=review >> Source/WebCore/ChangeLog:13 >> + reject local files for this combination of arguments. > > The bug description should come before the the list of included tests. Done. >> LayoutTests/storage/domstorage/sessionstorage/blocked-file-access-expected.txt:3 >> +Test that we are permitted access to sessionStorage from a file URL if unversal access is turned off. > > Nit: unversal => universal Ugh! Fixed. >> LayoutTests/storage/domstorage/sessionstorage/blocked-file-access.html:1 >> +<html> > > I do not see the need for this test to use quirks mode. We should opt into standards mode by adding <!DOCTYPE html>. OK. >> LayoutTests/storage/domstorage/sessionstorage/blocked-file-access.html:12 >> +<iframe src="resources/blocked-example.html"></iframe> > > On another note, is is necessary to embed other page in an iframe? I mean, could we incorporate the content of file resources/blocked-example.html directly into this file? At the time this file is loaded, WKTR is already in a "allow universal file access" mode, and I found that turning it off using "setAllowUniversalAccessFromFileURLs" did not take effect during the load. Perhaps this is a bug in our test infrastructure that we could address someday, but for now the easiest approach was to set the flag, then invoke a new file load. >> LayoutTests/storage/domstorage/sessionstorage/blocked-file-access.html:14 >> +Test that we are permitted access to sessionStorage from a file URL if unversal access is turned off. > > Ditto. Fixed. >> LayoutTests/storage/domstorage/sessionstorage/resources/blocked-example.html:1 >> +<html> > > I do not see the need for this test to use quirks mode. We should opt into standards mode by adding <!DOCTYPE html>. Agreed. >> LayoutTests/storage/domstorage/sessionstorage/resources/blocked-example.html:10 >> + } > > Nit: We tend to follow the WebKit Code Style Guidelines for JavaScriptCore and omit braces for if-bocks whose body is a single line. > > This is OK as-is. I do not see the need to emphasize the word "was" by writing it in all capitals. OK. >> LayoutTests/storage/domstorage/sessionstorage/resources/blocked-example.html:11 >> + } catch(e) { > > Nit: Missing space before '('. Whoops! Fixed. >> LayoutTests/storage/domstorage/sessionstorage/resources/blocked-example.html:12 >> + console.log("FAIL: window.sessionStorage was NOT accessible"); > > This is OK as-is. I do not see the need to emphasize the word "not" by writing it in all capitals. Fixed. 1176274 7 bfulgham 2016-03-18 12:46:40 -0700 Committed r198439: <http://trac.webkit.org/changeset/198439> 1249859 8 bfulgham 2016-11-10 10:59:10 -0800 Note: This fix was rolled out in Bug 160169 (https://trac.webkit.org/changeset/203695). 1249862 9 bfulgham 2016-11-10 10:59:42 -0800 Change was rolled back in under r208550 <https://trac.webkit.org/changeset/208550>. 1249916 10 274341 darin 2016-11-10 12:34:40 -0800 Comment on attachment 274341 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274341&action=review > Source/WebCore/page/SecurityOrigin.cpp:379 > - if (isLocal() && !m_universalAccess) > + if (isLocal() && !m_universalAccess && shouldAllowFromThirdParty != AlwaysAllowFromThirdParty) > return false; A cleaner way to do this would be to move this check down *after* the shouldAllowFromThirdParty check below. The only downside is that this would change behavior for a case which should never happen, when !topOrigin is true. I suppose that makes the cleaner way slightly higher risk, so I suppose it’s safer to leave it this way. But otherwise, I feel it makes the logic a bit clearer by not repeating a check twice. 274341 2016-03-17 16:58:18 -0700 2016-03-17 22:58:29 -0700 Patch bug-155609-20160317165817.patch text/plain 4758 bfulgham SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE5ODM1OSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIxIEBACisyMDE2LTAzLTE3ICBCcmVudCBG dWxnaGFtICA8YmZ1bGdoYW1AYXBwbGUuY29tPgorCisgICAgICAgIExvY2FsIGZpbGUgcmVzdHJp Y3Rpb25zIHNob3VsZCBub3QgYmxvY2sgc2Vzc2lvblN0b3JhZ2UgYWNjZXNzCisgICAgICAgIGh0 dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNTU2MDkKKyAgICAgICAgPHJk YXI6Ly9wcm9ibGVtLzI1MjI5NDYxPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgIFRlc3Q6IHN0b3JhZ2UvZG9tc3RvcmFnZS9zZXNzaW9uc3RvcmFnZS9i bG9ja2VkLWZpbGUtYWNjZXNzLmh0bWwKKworICAgICAgICBVc2Ugb2YgJ3Nlc3NzaW9uU3RvcmFn ZScgaXMgZ292ZXJuZWQgYnkgU2VjdXJpdHlPcmlnaW4gd2l0aCB0aGlyZCBwYXJ0eSBhY2Nlc3MK KyAgICAgICAgc2V0IHRvICdTaG91bGRBbGxvd0Zyb21UaGlyZFBhcnR5OjpBbHdheXNBbGxvd0Zy b21UaGlyZFBhcnR5Jy4gV2Ugc2hvdWxkIG5vdAorICAgICAgICByZWplY3QgbG9jYWwgZmlsZXMg Zm9yIHRoaXMgY29tYmluYXRpb24gb2YgYXJndW1lbnRzLgorCisgICAgICAgICogcGFnZS9TZWN1 cml0eU9yaWdpbi5jcHA6CisgICAgICAgIChXZWJDb3JlOjpTZWN1cml0eU9yaWdpbjo6Y2FuQWNj ZXNzU3RvcmFnZSk6IEZvciB0aGUgY2FzZSBvZiBzZXNzaW9uU3RvcmFnZSwKKyAgICAgICAgYWxs b3cgbG9jYWwgZmlsZSBhY2Nlc3MuCisKIDIwMTYtMDMtMTcgIEJyYWR5IEVpZHNvbiAgPGJlaWRz b25AYXBwbGUuY29tPgogCiAgICAgICAgIERvbid0IHRyeSB0byByZXN0b3JlIGRlbGV0ZWQgTWVt b3J5SW5kZXhlcyBpZiB0aGVpciBvd25pbmcgb2JqZWN0IHN0b3JlIGlzIG5vdCByZXN0b3JlZC4K SW5kZXg6IFNvdXJjZS9XZWJDb3JlL3BhZ2UvU2VjdXJpdHlPcmlnaW4uY3BwCj09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0K LS0tIFNvdXJjZS9XZWJDb3JlL3BhZ2UvU2VjdXJpdHlPcmlnaW4uY3BwCShyZXZpc2lvbiAxOTgz NDQpCisrKyBTb3VyY2UvV2ViQ29yZS9wYWdlL1NlY3VyaXR5T3JpZ2luLmNwcAkod29ya2luZyBj b3B5KQpAQCAtMzc1LDcgKzM3NSw3IEBAIGJvb2wgU2VjdXJpdHlPcmlnaW46OmNhbkFjY2Vzc1N0 b3JhZ2UoY28KICAgICBpZiAobV9zdG9yYWdlQmxvY2tpbmdQb2xpY3kgPT0gQmxvY2tBbGxTdG9y YWdlKQogICAgICAgICByZXR1cm4gZmFsc2U7CiAKLSAgICBpZiAoaXNMb2NhbCgpICYmICFtX3Vu aXZlcnNhbEFjY2VzcykKKyAgICBpZiAoaXNMb2NhbCgpICYmICFtX3VuaXZlcnNhbEFjY2VzcyAm JiBzaG91bGRBbGxvd0Zyb21UaGlyZFBhcnR5ICE9IEFsd2F5c0FsbG93RnJvbVRoaXJkUGFydHkp CiAgICAgICAgIHJldHVybiBmYWxzZTsKIAogICAgIC8vIEZJWE1FOiBUaGlzIGNoZWNrIHNob3Vs ZCBiZSByZXBsYWNlZCB3aXRoIGFuIEFTU0VSVCBvbmNlIHdlIGNhbiBndWFyYW50ZWUgdGhhdCB0 b3BPcmlnaW4gaXMgbm90IG51bGwuCkluZGV4OiBMYXlvdXRUZXN0cy9DaGFuZ2VMb2cKPT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PQotLS0gTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCShyZXZpc2lvbiAxOTgzNDYpCisrKyBMYXlv dXRUZXN0cy9DaGFuZ2VMb2cJKHdvcmtpbmcgY29weSkKQEAgLTEsMyArMSwxNSBAQAorMjAxNi0w My0xNyAgQnJlbnQgRnVsZ2hhbSAgPGJmdWxnaGFtQGFwcGxlLmNvbT4KKworICAgICAgICBMb2Nh bCBmaWxlIHJlc3RyaWN0aW9ucyBzaG91bGQgbm90IGJsb2NrIHNlc3Npb25TdG9yYWdlIGFjY2Vz cworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTU1NjA5 CisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS8yNTIyOTQ2MT4KKworICAgICAgICBSZXZpZXdlZCBi eSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIHN0b3JhZ2UvZG9tc3RvcmFnZS9zZXNzaW9u c3RvcmFnZS9ibG9ja2VkLWZpbGUtYWNjZXNzLWV4cGVjdGVkLnR4dDogQWRkZWQuCisgICAgICAg ICogc3RvcmFnZS9kb21zdG9yYWdlL3Nlc3Npb25zdG9yYWdlL2Jsb2NrZWQtZmlsZS1hY2Nlc3Mu aHRtbDogQWRkZWQuCisgICAgICAgICogc3RvcmFnZS9kb21zdG9yYWdlL3Nlc3Npb25zdG9yYWdl L3Jlc291cmNlcy9ibG9ja2VkLWV4YW1wbGUuaHRtbDogQWRkZWQuCisKIDIwMTYtMDMtMTcgIFJ5 YW4gSGFkZGFkICA8cnlhbmhhZGRhZEBhcHBsZS5jb20+CiAKICAgICAgICAgTWFya2luZyBodHRw L3Rlc3RzL3NlY3VyaXR5L2Fib3V0Qmxhbmsvd2luZG93LW9wZW4tc2VsZi1hYm91dC1ibGFuay5o dG1sIGFzIGZsYWt5IG9uIGlvcy1zaW0tZGVidWcKSW5kZXg6IExheW91dFRlc3RzL3N0b3JhZ2Uv ZG9tc3RvcmFnZS9zZXNzaW9uc3RvcmFnZS9ibG9ja2VkLWZpbGUtYWNjZXNzLWV4cGVjdGVkLnR4 dAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09Ci0tLSBMYXlvdXRUZXN0cy9zdG9yYWdlL2RvbXN0b3JhZ2Uvc2Vzc2lvbnN0 b3JhZ2UvYmxvY2tlZC1maWxlLWFjY2Vzcy1leHBlY3RlZC50eHQJKHJldmlzaW9uIDApCisrKyBM YXlvdXRUZXN0cy9zdG9yYWdlL2RvbXN0b3JhZ2Uvc2Vzc2lvbnN0b3JhZ2UvYmxvY2tlZC1maWxl LWFjY2Vzcy1leHBlY3RlZC50eHQJKHdvcmtpbmcgY29weSkKQEAgLTAsMCArMSwzIEBACitDT05T T0xFIE1FU1NBR0U6IGxpbmUgOTogUEFTUzogd2luZG93LnNlc3Npb25TdG9yYWdlIFdBUyBhY2Nl c3NpYmxlCisKK1Rlc3QgdGhhdCB3ZSBhcmUgcGVybWl0dGVkIGFjY2VzcyB0byBzZXNzaW9uU3Rv cmFnZSBmcm9tIGEgZmlsZSBVUkwgaWYgdW52ZXJzYWwgYWNjZXNzIGlzIHR1cm5lZCBvZmYuCklu ZGV4OiBMYXlvdXRUZXN0cy9zdG9yYWdlL2RvbXN0b3JhZ2Uvc2Vzc2lvbnN0b3JhZ2UvYmxvY2tl ZC1maWxlLWFjY2Vzcy5odG1sCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIExheW91dFRlc3RzL3N0b3JhZ2UvZG9t c3RvcmFnZS9zZXNzaW9uc3RvcmFnZS9ibG9ja2VkLWZpbGUtYWNjZXNzLmh0bWwJKHJldmlzaW9u IDApCisrKyBMYXlvdXRUZXN0cy9zdG9yYWdlL2RvbXN0b3JhZ2Uvc2Vzc2lvbnN0b3JhZ2UvYmxv Y2tlZC1maWxlLWFjY2Vzcy5odG1sCSh3b3JraW5nIGNvcHkpCkBAIC0wLDAgKzEsMTYgQEAKKzxo dG1sPgorPGhlYWQ+Cis8c2NyaXB0PgoraWYgKHdpbmRvdy50ZXN0UnVubmVyKSB7CisgICAgdGVz dFJ1bm5lci5zZXRBbGxvd1VuaXZlcnNhbEFjY2Vzc0Zyb21GaWxlVVJMcyhmYWxzZSk7CisgICAg dGVzdFJ1bm5lci5kdW1wQXNUZXh0KCk7CisgICAgdGVzdFJ1bm5lci53YWl0VW50aWxEb25lKCk7 Cit9Cis8L3NjcmlwdD4KKzwvaGVhZD4KKzxib2R5PgorPGlmcmFtZSBzcmM9InJlc291cmNlcy9i bG9ja2VkLWV4YW1wbGUuaHRtbCI+PC9pZnJhbWU+Cis8ZGl2IGlkPSJyZXN1bHRzIj48L2Rpdj4K K1Rlc3QgdGhhdCB3ZSBhcmUgcGVybWl0dGVkIGFjY2VzcyB0byBzZXNzaW9uU3RvcmFnZSBmcm9t IGEgZmlsZSBVUkwgaWYgdW52ZXJzYWwgYWNjZXNzIGlzIHR1cm5lZCBvZmYuCis8L2JvZHk+Cis8 L2h0bWw+CkluZGV4OiBMYXlvdXRUZXN0cy9zdG9yYWdlL2RvbXN0b3JhZ2Uvc2Vzc2lvbnN0b3Jh Z2UvcmVzb3VyY2VzL2Jsb2NrZWQtZXhhbXBsZS5odG1sCj09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIExheW91dFRl c3RzL3N0b3JhZ2UvZG9tc3RvcmFnZS9zZXNzaW9uc3RvcmFnZS9yZXNvdXJjZXMvYmxvY2tlZC1l eGFtcGxlLmh0bWwJKHJldmlzaW9uIDApCisrKyBMYXlvdXRUZXN0cy9zdG9yYWdlL2RvbXN0b3Jh Z2Uvc2Vzc2lvbnN0b3JhZ2UvcmVzb3VyY2VzL2Jsb2NrZWQtZXhhbXBsZS5odG1sCSh3b3JraW5n IGNvcHkpCkBAIC0wLDAgKzEsMjIgQEAKKzxodG1sPgorPGhlYWQ+Cis8c2NyaXB0IHNyYz0icmVz b3VyY2VzL2NsZWFyU2Vzc2lvblN0b3JhZ2UuanMiPjwvc2NyaXB0PgorPHNjcmlwdD4KK2Z1bmN0 aW9uIHJ1blRlc3QoKQoreworICAgIHRyeSB7CisgICAgICAgIGlmICh3aW5kb3cuc2Vzc2lvblN0 b3JhZ2UpIHsKKyAgICAgICAgICAgIGNvbnNvbGUubG9nKCJQQVNTOiB3aW5kb3cuc2Vzc2lvblN0 b3JhZ2UgV0FTIGFjY2Vzc2libGUiKTsKKyAgICAgICAgfQorICAgIH0gY2F0Y2goZSkgeworICAg ICAgICBjb25zb2xlLmxvZygiRkFJTDogd2luZG93LnNlc3Npb25TdG9yYWdlIHdhcyBOT1QgYWNj ZXNzaWJsZSIpOworICAgICAgICBjb25zb2xlLmxvZygiRXhjZXB0aW9uOiAiICsgZS5tZXNzYWdl KTsKKyAgICB9CisgICAgaWYgKHdpbmRvdy50ZXN0UnVubmVyKQorICAgICAgICB0ZXN0UnVubmVy Lm5vdGlmeURvbmUoKTsKK30KKzwvc2NyaXB0PgorPC9oZWFkPgorPGJvZHkgb25sb2FkPSJydW5U ZXN0KCk7Ij4KKzwvYm9keT4KKzwvaHRtbD4K