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

location.reload() should preserve the initiator of the navigation #4852

Closed
anforowicz opened this issue Aug 19, 2019 · 6 comments · Fixed by #6315
Closed

location.reload() should preserve the initiator of the navigation #4852

anforowicz opened this issue Aug 19, 2019 · 6 comments · Fixed by #6315

Comments

@anforowicz
Copy link

Current spec asks location.reload() to clobber initiator

https://html.spec.whatwg.org/multipage/history.html#dom-location-reload currently says "The source browsing context must be the browsing context being navigated"

https://html.spec.whatwg.org/multipage/browsing-the-web.html#source-browsing-context says "Navigation always involves source browsing context, which is the browsing context which was responsible for starting the navigation".

Problems caused by current spec / behavior

This behavior is responsible for the following security-related problems:

  • https://crbug.com/699271#c8 - Referrer is not preserved (instead new, security-sensitive URL is used as a Referrer and forwarded to an attacker)
  • https://crbug.com/968529#18 - SameSite cookies are unexpectedly missing when an error page reloads a failed navigation (no security problem other than hindering adoption of SameSite cookies)

Proposal

I propose that location.reload() should preserve the initiator (just like reloading via browser UI and just like history.back()).

@anforowicz
Copy link
Author

@anforowicz anforowicz changed the title location.reload() doesn't preserve the initiator of the navigation location.reload() should preserve the initiator of the navigation Aug 19, 2019
@annevk
Copy link
Member

annevk commented Aug 20, 2019

It seems this also affects the Refresh pragma directive and header. It makes sense that we reuse the original "source browsing context" (which should really be "source document navigation-relevant state") for that.

@annevk
Copy link
Member

annevk commented Aug 20, 2019

cc @bzbarsky

@bzbarsky
Copy link
Contributor

bzbarsky commented Aug 29, 2019

Fwiw, I'm pretty sure that whether reload via browser UI preserves the initiator is very browser-dependent, and history.back uses the browsing context of the document whose window's history back was called on as the source browsing context, not the original source browsing context.

So if we think that the "source browsing context" concept really needs to be propagated through history, we need changes to more than just reload, no?

As far as Refresh, I think that's quite different: a Refresh attached to document X is going to navigate to a URI under the control of document X, which may have nothing to do with document Y that initially loaded document X. So I don't think it makes sense to preserve any information about Y for that load, unless I'm missing something.

I am also pretty sure that in Gecko session history stores all sorts of state that can get used if there's a load triggered from history, via either reload or back/forward and that referrer is one of those pieces of state: see https://searchfox.org/mozilla-central/rev/7088fc958db5935eba24b413b1f16d6ab7bd13ea/docshell/shistory/nsISHEntry.idl#75-76. It's pretty important that this information be stored in the session history, not regotten from some browsing context that by now may have absolutely nothing to do with the original load that created the session history entry. As in, we do need the "source document navigation-relevant state" bit and the session history needs to store that.

@anforowicz
Copy link
Author

/CC @mikewest, @arturjanc, @csreis

Thank you @bzbarsky for pointing out the current spec and browser situation:

Fwiw, I'm pretty sure that whether reload via browser UI preserves the initiator is very browser-dependent, and history.back uses the browsing context of the document whose window's history back was called on as the source browsing context, not the original source browsing context.

After reading the above I think that maybe I messed up by landing https://chromium-review.googlesource.com/c/chromium/src/+/1662738 without consulting the specs :-(. After this CL Chrome behaves in the following way:

  • browser-UI-initiated reload preserves the initiator
  • both browser-UI-initiated and renderer-initiated (e.g. via history.back()) history navigations preserve the initiator
  • WPT tests have been added for Sec-Fetch-Site (I am not sure how else the initiator can be web-observable; I guess SameSite cookies are one other option; any other options for WPT tests that might be valuable to browsers that do not implement Sec-Fetch-... headers?)

For completeness, let me note that the initiator is not yet preserved by Chrome:

I see now that the behavior I've landed might be incompatible with the specs:

FWIW, I still think that preserving the initiator is desirable for Sec-Fetch-Site and SameSite behavior during history navigations and during reload.

@bzbarsky
Copy link
Contributor

Right, so to be clear I don't know that browsers follow the spec that much in general around the initiator bit (e.g. Gecko has no such concept at all!) and it's not clear that the initiator and source browsing context bits in the spec are correct. More, it's pretty clear they are not correct in general, because they go through the browsing context to get at a document, which may not be the document "responsible" for the navigation to start with in some cases....

Gecko does serialize and deserialize referrer information in its session history across session restore. I'm not sure what bits affect the SameSite cookie behavior, so I don't know offhand how those behave in Gecko.

I think we're in violent agreement that spec changes are needed here. ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

4 participants