Bug 183028 - pushState and replaceState no longer works in local file
Summary: pushState and replaceState no longer works in local file
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: History (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified iOS 11
: P1 Major
Assignee: Danyao Wang
URL:
Keywords: InRadar
Depends on: 178565
Blocks:
  Show dependency treegraph
 
Reported: 2018-02-21 22:24 PST by Danyao Wang
Modified: 2018-03-12 10:53 PDT (History)
7 users (show)

See Also:


Attachments
replace_state.html (258 bytes, text/html)
2018-02-21 22:24 PST, Danyao Wang
no flags Details
WIP (2.87 KB, patch)
2018-03-05 12:02 PST, Danyao Wang
no flags Details | Formatted Diff | Diff
Patch (12.52 KB, patch)
2018-03-09 15:22 PST, Danyao Wang
no flags Details | Formatted Diff | Diff
Patch for landing (13.05 KB, patch)
2018-03-12 07:44 PDT, Danyao Wang
no flags Details | Formatted Diff | Diff
Patch for landing (12.71 KB, patch)
2018-03-12 08:09 PDT, Danyao Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Danyao Wang 2018-02-21 22:24:05 PST
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
Comment 1 Frédéric Wang (:fredw) 2018-02-22 02:22:54 PST
cc'ing Brent as he is the author of https://trac.webkit.org/changeset/224609
Comment 2 Radar WebKit Bug Importer 2018-02-22 08:02:46 PST
<rdar://problem/37787656>
Comment 3 Frédéric Wang (:fredw) 2018-02-22 14:23:02 PST
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);
 }
Comment 4 Danyao Wang 2018-02-22 14:45:56 PST
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?
Comment 5 Brent Fulgham 2018-02-27 08:46:26 PST
(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.
Comment 6 Danyao Wang 2018-03-05 12:02:21 PST
Created attachment 335014 [details]
WIP
Comment 7 Danyao Wang 2018-03-05 12:03:54 PST
Brent - what do you think about this patch? It's based on https://codereview.chromium.org/1632513002/ and https://codereview.chromium.org/2060093002/.
Comment 8 Frédéric Wang (:fredw) 2018-03-07 03:04:32 PST
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.
Comment 9 Brent Fulgham 2018-03-07 10:01:25 PST
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
Comment 10 Brent Fulgham 2018-03-07 10:02:22 PST
(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!
Comment 11 Danyao Wang 2018-03-09 15:22:41 PST
Created attachment 335473 [details]
Patch
Comment 12 Danyao Wang 2018-03-09 16:27:52 PST
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.
Comment 13 Brent Fulgham 2018-03-09 17:34:16 PST
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.
Comment 14 Danyao Wang 2018-03-12 07:44:45 PDT
Created attachment 335594 [details]
Patch for landing
Comment 15 Danyao Wang 2018-03-12 08:09:44 PDT
Created attachment 335595 [details]
Patch for landing
Comment 16 Danyao Wang 2018-03-12 08:11:32 PDT
(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.
Comment 17 WebKit Commit Bot 2018-03-12 10:53:45 PDT
Comment on attachment 335595 [details]
Patch for landing

Clearing flags on attachment: 335595

Committed r229540: <https://trac.webkit.org/changeset/229540>
Comment 18 WebKit Commit Bot 2018-03-12 10:53:46 PDT
All reviewed patches have been landed.  Closing bug.