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

Consider dropping unforgeable from window.location #5571

Closed
jonathanKingston opened this issue May 21, 2020 · 7 comments
Closed

Consider dropping unforgeable from window.location #5571

jonathanKingston opened this issue May 21, 2020 · 7 comments

Comments

@jonathanKingston
Copy link

jonathanKingston commented May 21, 2020

JavaScript designed to intervene in webpage location changes is not fully effective without being able to override window.location. As I understand it these scripts are not always able to sandbox the code that could potentially be malicious.

This ability was also considered as part of Trusted Types URL traps where the policy owner would be able to prevent or rewrite a URL redirect of this nature. @koto may be able to speak further of the work here, he suggested to me the reason this was dropped wasn't related to this issue and purely related to the difficulty in authoring such policy.

As I understand it the [unforgeable] flag on the property has existed to keep plugins trusting the validity of the location property. However with these going away in the near-ish future I would like to discuss dropping this flag in the future as it sounded like the impact in terms of web compat would be minimal. I'm also open to discussing other ways in which JavaScript could intercept the setting of window.location properties. Perhaps also it's less of an issue that I only want to implement a setter rather than modify the getter of window.location?

@domenic
Copy link
Member

domenic commented May 21, 2020

This is unlikely to be web-compatible. See, for example, https://bugs.chromium.org/p/chromium/issues/detail?id=1057795, wherein attempting to add a non-[LegacyUnforgeable] member to window.location broke sites. Changing the ones that have existed this way for decades seems really unlikely to work.

@jonathanKingston
Copy link
Author

Yeah I think if it can be something that is only observable (or better yet not at all) when a script has modified the property that would be more ideal.

The other direction I was thinking was an event handler for non user activated top level navigations that is able to prevent navigation. Again this could be gated on when a script has added the handler, so shouldn't impact performance of pages without it.

@domenic
Copy link
Member

domenic commented May 21, 2020

Some sort of navigate event? I think that has been floated a few times in the past; maybe @jakearchibald knows where.

@jonathanKingston
Copy link
Author

Yeah and it also seems there was also discussion by @slightlyoff here: https://twitter.com/slightlylate/status/1051199115466399744 I'm not sure if it went any further than this.

Essentially it seems safe to not send a warning dialog when the user hasn't interacted with the browser. Besides the attacker can also manipulate history breaking back/forward buttons for the user without a warning.

@annevk
Copy link
Member

annevk commented May 22, 2020

See #5562.

@annevk
Copy link
Member

annevk commented Jan 6, 2021

I can see this not being web-compatible for members of the Location class, but for window.location I'm less sure. What kind of code would end up breaking?

@jonathanKingston
Copy link
Author

I can see this not being web-compatible for members of the Location class, but for window.location I'm less sure. What kind of code would end up breaking?

I've had this issue open as a tab since you asked this, I'm not entirely sure the answer here. I think it's probably safe to close this out instead of working towards the WICG new repo: https://github.com/WICG/app-history which might address the issue I cared about which was cleanly controlling history.

The weirdness of window.location will likely have to stay unless you think otherwise, in which case feel free to reopen.

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

No branches or pull requests

3 participants