Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Newly created browsing contexts will almost always reject history.pushState/replaceState #6836

Closed
rakina opened this issue Jul 8, 2021 · 19 comments · Fixed by #7044
Closed
Labels
interop Implementations are not interoperable with each other topic: history

Comments

@rakina
Copy link
Member

rakina commented Jul 8, 2021

Discovered while working on web-platform-tests/wpt#28541.
When a new browsing context is created, it will load the initial about:blank document with URL set to about:blank, but its origin will be set to the creator's origin. So this means the URL and the origin can be quite different (unless the opener's origin is about:blank).

When the document URL and the origin differs, apparently history.pushState() or history.replaceState() will always result in a SecurityError. This is because of the checks that compare the target URL against both the document URL and the origin. If the document URL and origin differ, one of the checks will always fail. Example test cases that fails in browsers:

// pushState to the opener URL. Results in a SecurityError on Chrome, Firefox, Safari.
let newFrame = document.createElement("iframe");
document.body.appendChild(newFrame);
newFrame.contentWindow.history.pushState("foo", "", location.href);
console.log(newFrame.contentWindow.history.state);
// pushState to the document URL (about:blank). Results in a SecurityError on Chrome, Firefox.
let newFrame = document.createElement("iframe");
document.body.appendChild(newFrame);
newFrame.contentWindow.history.pushState("foo", "", "about:blank");
console.log(newFrame.contentWindow.history.state);
// pushState without changing the URL. Results in a SecurityError on Chrome.
let newFrame = document.createElement("iframe");
document.body.appendChild(newFrame);
newFrame.contentWindow.history.pushState("foo", "");
console.log(newFrame.contentWindow.history.state);

I wonder if this is intended? Maybe we can drop the comparison against the document URL, and just compare the origin? Or maybe we should special case cases where the document URL & origin differs? I'm worried that this case in general is not handled well in other parts of the spec.

cc @domenic

@domenic
Copy link
Member

domenic commented Jul 8, 2021

Ah, interesting.

So there are three checks that are naturally possible:

  1. Current spec: check newURL against the document's URL, requiring that they differ only in path, query, or fragment components.

  2. Same-origin URL: check newURL's origin for same-originness against the document's URL's origin.

  3. Same-origin document: check newURL's origin for same-originness against the document's origin. (Different from (2) because about:blank documents often get their creator's origin.)

Some examples:

  • An about:blank iframe being rewritten to about:blank#foo would pass (1) but not (2) or (3). (It fails 2 since the origin of about:blank as a URL is an opaque origin.)

  • Rewriting a page at https://example.com/qux to https://user:[email protected]/qux would pass (2) and (3) but not (1).

  • An about:blank iframe being rewritten to its parent's URL would pass (3) but not (2) or (1).

  • A page at blob:https://whatwg.org/d0360e2f-caee-469f-9a2f-87d5b0456f6f being rewritten to blob:asdf would pass (1) but not (2) or (3).

In a vacuum, absent compat concerns, adding something like (2) or (3) (probably (3)) might be reasonable.

Chrome code has an origin check, but it's rather complicated with several exceptions. It mentions https://bugs.chromium.org/p/chromium/issues/detail?id=528681 as a compat concern. So if we did do something it probably couldn't be as simple as (2) or (3).

WebKit code looks similar to Chrome but easier for me to read at least. I'm not sure if they're equivalent.

Firefox code seems to be attempting to do (1) but because it implements it using URL origin check + username/password check, it mixes in a bit of (2), which impacts the about:blank cases where the URL's origin is opaque.

Getting interop/spec/tests here would be good. I'm unsure what direction is best to go in though.

@domenic domenic added interop Implementations are not interoperable with each other topic: history labels Jul 8, 2021
@rakina
Copy link
Member Author

rakina commented Jul 9, 2021

Thanks for investigating! Looks like about:blank -> about:blank/about:blank#foo is already allowed per spec (it differs only in fragment), it's just the implementations aren't following it.

Looking at the Chrome bug about unique origins changing the fragment through replaceState and the comment in the spec mentioning spoofing, I think we need to keep allowing all URLs that are "same-url-excluding-fragments with the document URL" so that opaque origins (and other special origins that's not really in the spec) can continue changing the fragment through the API. But does that mean the origin of the document should change too? Maybe not since opaque origins don't depend on the exact URL?

I think we can either keep the "same URL except for query/path/fragment" path compare the URL's origin against the document's origin like we do now, or change it to use the "origin of the document if we navigate to this URL from this frame", not the URL's origin which is purely based off the URL. I guess we can get this by calling "determine the origin" with the current browsing context, target URL, current document's sandbox flags, and current document's origin?
This will allow:

  • about:blank/about:srcdoc -> opener/parent URL (maybe nice?)
  • URL -> about:blank/about:srcdoc (maybe weird)

Anyways this is sounding more like an implementation bug now, so I'm fine not changing anything in the spec and I'll just file relevant impl bugs!

@domenic
Copy link
Member

domenic commented Jul 12, 2021

Fixing implementations sounds great if you're willing to do so for Chrome/file bugs for others! I worry a bit that this would be relaxing a security check, but I can't think of any actual problem that would be caused by moving to behavior (1), so I look forward to your attempt.

@domenic domenic added the agenda+ To be discussed at a triage meeting label Aug 11, 2021
@domenic
Copy link
Member

domenic commented Aug 11, 2021

@rakina, can you work to confirm that updating to match the spec here is something Chrome is interested in doing, if possible? Maybe get confirmation from security folks that it should be OK?

@smaug---- @cdumez for the same question for Gecko/WebKit.

@rakina
Copy link
Member Author

rakina commented Aug 16, 2021

I've filed a bug for Chrome, with a simpler proposal to just do something like this:

if (URL is the same except for fragment and query) {
   // Allows documents with opaque origin (file:, data:, about:, sandboxed document, etc) to change fragment and query.
   can_change = true;
} else {
  // Allows all documents to change to a same origin URL, possibly differing in path, fragment and query.
  can_change = (requested URL is same origin as current origin) && (URL is the same except for fragment, query, and path);
}

I think this is an easy change for the current implementations, compared to my previous proposal of having a way to determine "origin of the document if we navigate to this URL from this frame" (but reading WebKit's code, this might be closer to what they're doing through the documentSecurityOrigin.canRequest(fullURL) check?).

Curious what @smaug---- and @cdumez thinks :)

@domenic
Copy link
Member

domenic commented Aug 16, 2021

So to be clear, your proposal is something a bit more strict than the current spec, which is just

can_change = (URL is the same except for fragment, query, and path)

In particular, the current spec allows changes such as about:blank -> about:foo, or data:text/html,foo -> data:bar, or file:///foo -> file:///bar. But you are proposing to disallow that.

Also, I am pretty sure your proposal is equivalent to the following:

if (document's origin is opaque) {
  can_change = (URL is the same except for query or fragment);
} else {
  can_change = (URL is the same except for path or query or fragment);
}

which to me at least is a bit clearer.

@rakina
Copy link
Member Author

rakina commented Aug 17, 2021

So to be clear, your proposal is something a bit more strict than the current spec

I think the current spec is a bit different due to step 6.5: it doesn't allow path/query changes when the new URL origin is different than the current document origin (so this disallows about:blank -> about:foo etc) to prevent sandboxed documents to change the path/query. (It seems like Chrome & WebKit allows query changes though)

I think your proposal is a bit different in cases where the document's origin is not opaque but the target URL creates an opaque origin, e.g. current document is about:blank that inherits the parent's/opener's origin, and the target URL is something like about:foo. My proposal would reject this, but yours would allow it (since the document's origin is not opaque). Not sure if that's better or not.

Maybe this is equivalent to my proposal, though I'm not 100% confident on my origin knowledge:

if (document origin is opaque OR target URL's origin is opaque) {
  // allows only fragment/query changes on whose origin aren't directly from the URL (opaque/inherited origins)
  can_change = (URL is the same except for query or fragment);
} else {
  can_change = (URL is the same except for path or query or fragment);
}

@domenic
Copy link
Member

domenic commented Aug 17, 2021

I think the current spec is a bit different due to step 6.5

Oh, wow, I totally missed that since the beginning of this thread. Oh dear. A lot of what I've said has been based on a misunderstanding.

So for example, the current spec disallows changing from about:blank to about:blank#foo. You correctly point that out in your OP but I just missed it. OK. So in terms of #6836 (comment) the current spec actually does both (1) and (3).

So the main issue here is that the spec is not as lenient as browsers in some cases. Here is how Chrome compares to the spec:

if (document_origin->IsOpaque() || document_origin->IsLocal()) {
  // This check is not in the spec
  can_change = EqualIgnoringQueryAndFragment(url, document_url);
} else if (!EqualIgnoringPathQueryAndFragment(url, document_url)) {
  // This check is step 6.4
  can_change = false;
} else if (requested_origin->IsOpaque() ||
           !requested_origin->IsSameOriginWith(document_origin)) {
  // This check is the same as step 6.5
  can_change = false;
}

(I am pretty sure the requested_origin->IsOpaque() check is redundant, because an opaque origin derived from a URL will never be same-origin with a document origin.)

So I think we have three options:

I don't have a strong preference between the last two. Maybe it's time to start writing a bunch of test cases? That way we could figure out what cases are allowed everywhere, and what cases are disallowed everywhere, without having to do a bunch of tricky reasoning and code inspection.

@domenic
Copy link
Member

domenic commented Aug 18, 2021

Since this also impacts app history, I am happy to spend some of my time on writing the test cases.

@rakina
Copy link
Member Author

rakina commented Aug 18, 2021

Having tests would be great! I think Safari might be more lenient than Chrome (judging from the about:blank tests) and it would be interesting to see what else is allowed and whether that makes sense for the spec.
(BTW I think the part you marked as the same as 6.5 in Chrome is not really the same too as it's missing the "is this only different in fragment?" bit)

@domenic
Copy link
Member

domenic commented Aug 18, 2021

(BTW I think the part you marked as the same as 6.5 in Chrome is not really the same too as it's missing the "is this only different in fragment?" bit)

Yes, I noticed this today too :(

domenic added a commit to web-platform-tests/wpt that referenced this issue Aug 25, 2021
domenic added a commit to web-platform-tests/wpt that referenced this issue Aug 25, 2021
@domenic
Copy link
Member

domenic commented Aug 25, 2021

I wrote some tests, with the tests checking against the current spec. (I'm pretty sure... it took a few rounds for me to figure it out.) Let me know if you can think of other interesting cases I missed!

Here are the departures between the spec and implementations:

  • Firefox throws the wrong type of exception quite often
  • Chrome and Firefox disallow (but the spec allows):
    • about:blank origin-inheriting document to about:blank
    • about:blank origin-inheriting document to about:blank#newhash
    • srcdoc origin-inheriting document to about:srcdoc
    • srcdoc origin-inheriting document to about:srcdoc#newhash
  • Firefox disallows (but the spec allows):
    • blob:(real blob URL) to itself
    • blob(real blob URL) to blob:(same blob URL)#newhash
    • blob(real blob URL) to blob:(same blob URL)?newsearch
    • blob(real blob URL) to blob:(origin of the blob URL)/newpath
  • Firefox, Chrome, and Safari allow (but the spec disallows):
    • sandboxed document to a version of itself with a different query
  • Chrome and Safari allow (but the spec disallows):
    • data: URL document to a version of itself with a different query
  • Firefox allows (but the spec disallows):
    • sandboxed document to a version of itself with a different path

An interesting thing to note here is the inconsistency of Chrome with regard to how safe it considers query- and hash-only changes:

  • For about:blank to about:blank?newsearch or about:blank#newhash, Chrome disallows
  • For data:(data URL) to data:(data URL)?newsearch or data:(data URL)?newquery, Chrome allows

My recommendation is to not special-case opaque origins or local schemes. That is, just modify the current spec to allow query differences if the origin is different. This is equivalent to @rakina's proposal in #6836 (comment) I believe. Then:

  • Firefox would need to disallow sandboxed documents rewriting their paths
  • Firefox would need to allow more blob: situations
  • Firefox would need to allow data: URL documents rewriting their queries
  • Chrome and Firefox would need to allow changing the query or fragment in about:blank cases and about:srcdoc cases
  • Safari would need to allow changing the query in about:blank cases and about:srcdoc cases
  • This probably has some impact on file: URLs, but it's not really interoperable or testable since their origin is not specified.

@rakina
Copy link
Member Author

rakina commented Aug 26, 2021

Thanks for the WPTs! I think changing the implementation in Chrome to match the recommendation should be easy, except for the javascript: URL cases, as I believe we don't update the URL of the document on javascript: URL "navigations" (we clone the old URL instead)

BTW I noticed the WPTs don't test file:// URLs - is it possible to test those as well in WPTs?

@domenic
Copy link
Member

domenic commented Aug 26, 2021

I believe we don't update the URL of the document on javascript: URL "navigations"

Oh, oops. That is what the spec does as well. So those tests are just invalid. I will update them and the above.

Sadly file:// URLs are not testable on WPT :(. Last I checked there wasn't much interest in making them testable either since they are so non-interoperable and we don't really have a spec for them.

domenic added a commit that referenced this issue Aug 26, 2021
The actual normative change here is to allow the query component to change in the cross-origin case, not just the fragment component. However, while we're here, we pull out the algorithm into a standalone definition, and rephrase it to be clearer with fewer nested negative conditions.

This better aligns with browsers, exactly matching WebKit and being only slightly more lenient than Chromium and Gecko. See the full analysis at #6836 (comment).

This also improves the domintro for pushState()/replaceState() to be a bit more in line with modern style, and to explain the error condition.

Closes #6836.
domenic added a commit to web-platform-tests/wpt that referenced this issue Aug 27, 2021
domenic added a commit that referenced this issue Sep 1, 2021
This allows the query component to change in the cross-origin case, not just the fragment component.

This better aligns with browsers, although not exactly with any particular browser, since browsers seem to special-case opaque origins. See the full analysis at #6836 (comment).

Closes #6836.
@domenic
Copy link
Member

domenic commented Sep 2, 2021

We discussed this today and @annevk had a counterproposal, which would roughly be like this:

  • If the target URL's scheme is "http" or "https", allow differences in path, query, or fragment
  • If the target URL's scheme is "file", allow differences in query or fragment
  • Otherwise, allow differences in fragment.

(I believe there is no origin check in @annevk's version.)

This is substantially stricter than Firefox, Chrome, and Safari in most cases, so there might be some compat risk. But, it is all in weird cases like blob: and data:, so maybe it is fine.

This is also more lenient than Chrome and Safari in the particular case of allowing sandboxed iframes to have their paths rewritten. So there might be some security concerns there.

@rakina, what do you think of @annevk's proposal? I still somewhat prefer #6992 as it seems like a nice simple rule; in particular I find varying on scheme somewhat aesthetically displeasing compared to just doing an origin check. But maybe it'd be easier to get interop if we went with @annevk's preference.

@past past removed the agenda+ To be discussed at a triage meeting label Sep 3, 2021
@annevk
Copy link
Member

annevk commented Sep 3, 2021

To be clear, the reason I favor varying on scheme is because where the actual authority sits with a URL depends on it. For HTTP(S) URLs it's scheme/host/port. For file URLs it's scheme/host/port/path. For data and blob URLs it's scheme/path/query (they never have a host/port). For about:blank it's arguably scheme/path, so we could group it with file if we wanted to I suppose. Other schemes you cannot really navigate to.

@rakina
Copy link
Member Author

rakina commented Sep 7, 2021

Being more lenient with sandboxed frames sounds a bit concerning, but I guess security checks for things are already based on origin instead of URL so maybe it's OK? A sandboxed main frame can also now affect the path part in the browser omnibox. Maybe that's not so bad since we already allow it to change the query and fragment anyways.. Do we know why we have the special concern about sandboxed frames to begin with?

Other than that, I think it sounds reasonable.

@annevk
Copy link
Member

annevk commented Sep 7, 2021

It seems the change was made in e5ace39. I cannot find any rationale for this change in https://lists.w3.org/Archives/Public/public-whatwg-archive/2010Jan/. I suppose the theoretical issue is that a /sandbox could pretend it's /, but users are not expected to make trust decisions based on the path so that seems flawed.

@rakina
Copy link
Member Author

rakina commented Sep 8, 2021

Thanks for looking that up! OK then, @annevk's proposal sounds good to me.

domenic added a commit that referenced this issue Sep 8, 2021
domenic added a commit that referenced this issue Sep 8, 2021
domenic added a commit to web-platform-tests/wpt that referenced this issue Sep 9, 2021
domenic added a commit that referenced this issue Sep 9, 2021
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Sep 22, 2021
…ing, a=testonly

Automatic update from web-platform-tests
Tests for history.pushState() URL rewriting

See whatwg/html#6836. Follows whatwg/html#7044.
--

wpt-commits: 130d57f9a1d6f4f51bb9bd81444978efad7016ce
wpt-pr: 30182
aosmond pushed a commit to aosmond/gecko that referenced this issue Sep 24, 2021
…ing, a=testonly

Automatic update from web-platform-tests
Tests for history.pushState() URL rewriting

See whatwg/html#6836. Follows whatwg/html#7044.
--

wpt-commits: 130d57f9a1d6f4f51bb9bd81444978efad7016ce
wpt-pr: 30182
Gabisampaio pushed a commit to Gabisampaio/wpt that referenced this issue Nov 18, 2021
mfreed7 pushed a commit to mfreed7/html that referenced this issue Jun 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interop Implementations are not interoperable with each other topic: history
4 participants