pushState and replaceState no longer works in local file

• Component: History
• Status: RESOLVED FIXED

[Danyao Wang]

Created attachment 334441 [details]
replace_state.html

pushState and replaceState when called in file:// scheme fail in Safari Tech Review and iOS 11.3 with SecurityError: Blocked attempt to use history.replaceState() to change session history URL from file:///tmp/replace_state.html to file:///tmp/replace_state.html?replaced. Paths and fragments must match for a sandboxed document.

How to reproduce: load the attached file using file:// scheme.

This is a regression compared to Safari 11. I believe it's a result of this change: https://github.com/WebKit/webkit/commit/f7e07f31530f7c4df8bbf3a60697518428acd8da#diff-c870f5a610f9e6d344005bf329ccc405

Chrome on iOS depends on this behavior to restore session history into WKWebView after app is backgrounded.
There is also web compatibility concern, see http://crbug.com/528681. Blink ended up shipping a patch that considers two file:// scheme URLs same origin if they only differ in query and fragment: https://codereview.chromium.org/1632513002

[Frédéric Wang (:fredw)]

cc'ing Brent as he is the author of https://trac.webkit.org/changeset/224609

[Radar WebKit Bug Importer]

<rdar://problem/37787656>

[Frédéric Wang (:fredw)]

I confirm that reverting Brent's change makes Danyao's test work as expected.

diff --git a/Source/WebCore/page/SecurityOrigin.cpp b/Source/WebCore/page/SecurityOrigin.cpp
index e41af3763e5..d90e3ba3e79 100644
--- a/Source/WebCore/page/SecurityOrigin.cpp
+++ b/Source/WebCore/page/SecurityOrigin.cpp
@@ -283,7 +283,10 @@ bool SecurityOrigin::passesFileCheck(const SecurityOrigin& other) const
 {
     ASSERT(isLocal() && other.isLocal());
 
-    return !m_enforcesFilePathSeparation && !other.m_enforcesFilePathSeparation;
+    if (!m_enforcesFilePathSeparation && !other.m_enforcesFilePathSeparation)
+        return true;
+
+    return (m_filePath == other.m_filePath);
 }

[Danyao Wang]

Thanks Fred!

SecurityOrigin::passesFileCheck() is also used in SecurityOrigin::canAccess(). I'm not sure if reverting Brent's patch will have other undesired effect. So I'm also OK if we just patch History::stateObjectAdded() like Blink did.

bfulgham@, dbates@: Do you have any preference?

[Brent Fulgham]

(In reply to Danyao Wang from comment #4)
> Thanks Fred!
> 
> SecurityOrigin::passesFileCheck() is also used in
> SecurityOrigin::canAccess(). I'm not sure if reverting Brent's patch will
> have other undesired effect. So I'm also OK if we just patch
> History::stateObjectAdded() like Blink did.
> 
> bfulgham@, dbates@: Do you have any preference?

Please do not roll this change out without coordinating with me first. We made the original change as part of a security fix and need to make sure this request doesn't reintroduce a possible exploit.

[Danyao Wang]

Created attachment 335014 [details]
WIP

[Danyao Wang]

Brent - what do you think about this patch? It's based on https://codereview.chromium.org/1632513002/ and https://codereview.chromium.org/2060093002/.

[Frédéric Wang (:fredw)]

Comment on attachment 335014 [details]
WIP

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

> Source/WebCore/page/History.cpp:58
> +}

Why does that need to in the default namespace? Can you make this function static? If it's only used in one place, maybe it should be moved before History::stateObjectAdded?

> Source/WebCore/page/History.cpp:202
> +    const SecurityOrigin& documentOrigin = m_frame->document()->securityOrigin();

Are you able to use auto here?

> Source/WebCore/page/History.cpp:208
> +        && !allowSandboxException)

I would put !allowSandboxException first so that lazy evaluation of the expression is faster when allowSandboxException == true.

[Brent Fulgham]

Comment on attachment 335014 [details]
WIP

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

I think the overall approach here is good, but the new equality predicate needs to be added to the URL class so it can be used elsewhere if needed.

I'm also worried that we will inadvertently break this again without some form of test case. Can you repurpose some of the test cases in 'https://codereview.chromium.org/1632513002/patch/20001/30006'? Perhaps adding them to 'Tools/TestWebKitAPI/Tests/WebCore/URL.cpp'.

For these reasons, r- for now.

> Source/WebCore/page/History.cpp:52
> +        for (unsigned i = 0; i < pathEnd; i++) {

WebKit style uses "++i"

>> Source/WebCore/page/History.cpp:58
>> +}
> 
> Why does that need to in the default namespace? Can you make this function static? If it's only used in one place, maybe it should be moved before History::stateObjectAdded?

No, this should be added to URL, just like 'equalIgnoringFragmentIdentifier':

WTF/URL.h:
    WEBCORE_EXPORT friend bool equalIgnoringFragmentIdentifier(const URL&, const URL&);

>> Source/WebCore/page/History.cpp:202
>> +    const SecurityOrigin& documentOrigin = m_frame->document()->securityOrigin();
> 
> Are you able to use auto here?

I'd suggest using ' const auto& documentSecurityOrigin'

> Source/WebCore/page/History.cpp:203
> +    // We allow sandboxed documents, `data:`/`file:` URLs, etc. to use 'pushState'/'replaceState' to modify the URL query and fragments.

The quotes here are all over the place. Just please use plain-old ASCII single-quotes: '

>> Source/WebCore/page/History.cpp:208
>> +        && !allowSandboxException)
> 
> I would put !allowSandboxException first so that lazy evaluation of the expression is faster when allowSandboxException == true.

+1

[Brent Fulgham]

(In reply to Danyao Wang from comment #7)
> Brent - what do you think about this patch? It's based on
> https://codereview.chromium.org/1632513002/ and
> https://codereview.chromium.org/2060093002/.

Also, you should make some kind of test case based on 'replace_state.html' to help us avoid breaking this again.

Thanks!

[Danyao Wang]

Created attachment 335473 [details]
Patch

[Danyao Wang]

Thanks for the review. PTAL.

(In reply to Brent Fulgham from comment #9)
> Comment on attachment 335014 [details]
> WIP
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335014&action=review
> 
> I think the overall approach here is good, but the new equality predicate
> needs to be added to the URL class so it can be used elsewhere if needed.
> 
> I'm also worried that we will inadvertently break this again without some
> form of test case. Can you repurpose some of the test cases in
> 'https://codereview.chromium.org/1632513002/patch/20001/30006'? Perhaps
> adding them to 'Tools/TestWebKitAPI/Tests/WebCore/URL.cpp'.

Done.

I added LayoutTests/http/tests/navigation/pushstate-at-unique-origin-denied.php, which essentially tests the behavior in replace_state.html but for unique origin. This covers the IsUnique() part of |allowSandboxException|.

Is there a way to create a LayoutTest that is always as file:// scheme in run-webkit-tests? Then we can test the IsLocal() part of |allowSandboxException|.

I tried to create a pushstate-at-local-origin-denied.html test. But for some reason the file:// scheme is not detected inside testharness.js test when running using run-webkit-tests. It insists that document.URL is simply "pushstate-at-local-origin-denied.html", when I expected "file://<WebKitPath>/LayoutTests/security/pushstate-at-local-origin-denied.html"...

> 
> For these reasons, r- for now.
> 
> > Source/WebCore/page/History.cpp:52
> > +        for (unsigned i = 0; i < pathEnd; i++) {
> 
> WebKit style uses "++i"

Done.

> 
> >> Source/WebCore/page/History.cpp:58
> >> +}
> > 
> > Why does that need to in the default namespace? Can you make this function static? If it's only used in one place, maybe it should be moved before History::stateObjectAdded?
> 
> No, this should be added to URL, just like 'equalIgnoringFragmentIdentifier':
> 
> WTF/URL.h:
>     WEBCORE_EXPORT friend bool equalIgnoringFragmentIdentifier(const URL&,
> const URL&);

Done. I'm not sure what the 'friend' modifier is for. So omitted it for now.

> 
> >> Source/WebCore/page/History.cpp:202
> >> +    const SecurityOrigin& documentOrigin = m_frame->document()->securityOrigin();
> > 
> > Are you able to use auto here?
> 
> I'd suggest using ' const auto& documentSecurityOrigin'

Done.

> 
> > Source/WebCore/page/History.cpp:203
> > +    // We allow sandboxed documents, `data:`/`file:` URLs, etc. to use 'pushState'/'replaceState' to modify the URL query and fragments.
> 
> The quotes here are all over the place. Just please use plain-old ASCII
> single-quotes: '

Done.

> 
> >> Source/WebCore/page/History.cpp:208
> >> +        && !allowSandboxException)
> > 
> > I would put !allowSandboxException first so that lazy evaluation of the expression is faster when allowSandboxException == true.
> 
> +1

Done.

[Brent Fulgham]

Comment on attachment 335473 [details]
Patch

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

I think this looks correct -- I would like to see a simple test that file and http are not allowed to load each-other but otherwise this looks fine.

> Source/WebCore/platform/URL.h:171
> +    WEBCORE_EXPORT bool equalIgnoringQueryAndFragment(const URL&, const URL&);

This needs to be a friend declaration. However, the bots are green, so perhaps we don't need this declaration at all anymore?

> Tools/TestWebKitAPI/Tests/WebCore/URL.cpp:251
> +        {"file:///path/to/other/file.html", "file:///path/to/file.html", false},

Please add a failure case that compares a file URL and an HTTP URL.

[Danyao Wang]

Created attachment 335594 [details]
Patch for landing

[Danyao Wang]

Created attachment 335595 [details]
Patch for landing

[Danyao Wang]

(In reply to Brent Fulgham from comment #13)
> Comment on attachment 335473 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335473&action=review
> 
> I think this looks correct -- I would like to see a simple test that file
> and http are not allowed to load each-other but otherwise this looks fine.

Removed unnecessary "friend" declaration. The new equalIgnoringQueryAndFragment() only uses public methods of URL so it doesn't need this friend declaration.

> 
> > Source/WebCore/platform/URL.h:171
> > +    WEBCORE_EXPORT bool equalIgnoringQueryAndFragment(const URL&, const URL&);
> 
> This needs to be a friend declaration. However, the bots are green, so
> perhaps we don't need this declaration at all anymore?
> 
> > Tools/TestWebKitAPI/Tests/WebCore/URL.cpp:251
> > +        {"file:///path/to/other/file.html", "file:///path/to/file.html", false},
> 
> Please add a failure case that compares a file URL and an HTTP URL.

Added failure cases.

[WebKit Commit Bot]

Comment on attachment 335595 [details]
Patch for landing

Clearing flags on attachment: 335595

Committed r229540: <https://trac.webkit.org/changeset/229540>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.