Bug 155609

Summary: Local file restrictions are also restricting session storage
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebKit Misc.Assignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, andersca, ap, beidson, bfulgham, commit-queue, dbates, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 155185    
Bug Blocks: 164592    
Attachments:
Description Flags
Patch aestes: review+, dbates: commit-queue-

Brent Fulgham
Reported 2016-03-17 16:30:49 PDT
In Bug 155185, I inadvertently blocked access to sessionStorage for local files. This should still be permitted.
Attachments
Patch (4.65 KB, patch)
2016-03-17 16:58 PDT, Brent Fulgham 		aestes: review+
dbates: commit-queue-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2016-03-17 16:41:47 PDT
<rdar://problem/25229461>
Brent Fulgham
Comment 2 2016-03-17 16:58:18 PDT
Created attachment 274341 [details] Patch
Daniel Bates
Comment 3 2016-03-17 22:58:29 PDT
Comment on attachment 274341 [details] 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.
Daniel Bates
Comment 4 2016-03-17 23:00:28 PDT
(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.
Daniel Bates
Comment 5 2016-03-17 23:10:58 PDT
Comment on attachment 274341 [details] 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.
Brent Fulgham
Comment 6 2016-03-18 12:02:18 PDT
Comment on attachment 274341 [details] 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.
Brent Fulgham
Comment 7 2016-03-18 12:46:40 PDT
Committed r198439: <http://trac.webkit.org/changeset/198439>
Brent Fulgham
Comment 8 2016-11-10 10:59:10 PST
Note: This fix was rolled out in Bug 160169 (https://trac.webkit.org/changeset/203695).
Brent Fulgham
Comment 9 2016-11-10 10:59:42 PST
Change was rolled back in under r208550 <https://trac.webkit.org/changeset/208550>.
Darin Adler
Comment 10 2016-11-10 12:34:40 PST
Comment on attachment 274341 [details] 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.
Note You need to log in before you can comment on or make changes to this bug.