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

Set User-Agent header to window.navigator.userAgent #1237

Closed
wants to merge 8,998 commits into from

Conversation

shawwn
Copy link
Contributor

@shawwn shawwn commented Jun 26, 2019

Closes #1230

See JakeChampion/fetch#614 (comment)

Browsers will typically disallow you from changing the User-Agent string. It's a security restriction. There is no way around it; you'll have to use a different header name, e.g. X-Custom-user-agent, and configure the server to read it.

This PR forces the User-Agent header of any window.fetch call to be window.navigator.userAgent.

(This behavior seems identical to what Chrome enforces.)

Avaer Kazmer and others added 30 commits June 6, 2019 09:53
…adme

Remove dead --experimental-worker references from README.md
@avaer
Copy link
Member

avaer commented Jun 26, 2019

I think this is fine, but is there a plan to cover the rest of the cases here?

It seems that currently this PR adds disparity by fixing the one call point only.

@shawwn
Copy link
Contributor Author

shawwn commented Jun 26, 2019

Done.

@shawwn shawwn force-pushed the 2019-06-26/fetch-user-agent branch from 5b27f89 to 58ff970 Compare June 26, 2019 15:25
@shawwn
Copy link
Contributor Author

shawwn commented Jun 26, 2019

I updated this PR with the latest changes from master.

It seems to cover all the cases you outlined. If you feel it's sufficient, then it's ready to merge.

src/Window.js Outdated
@@ -102,6 +102,7 @@ const {
GlobalContext.id = id;
GlobalContext.args = args;
GlobalContext.version = version;
GlobalContext.userAgent = `Mozilla/5.0 (OS) AppleWebKit/999.0 (KHTML, like Gecko) Chrome/999.0.0.0 Safari/999.0 Exokit/${GlobalContext.version}`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't add anything new to GlobalContext unless there is no other way, Ideally, GlobalContext is deleted entirely.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can fetch pull from the global navigator instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any suggestions? The only alternative I can think of is to move this line into src/fetch.js. But then it will still depend on GlobalContext.version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be a method on window-fetch that sets the interception user agent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I've updated the code. src/fetch.js now pulls from Navigator.userAgent, which is a static get userAgent() { .... } property.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, the navigator should be a global in all cases, where the userAgent can be accessed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, but I don't think it needs to be a static. We could just access the navigator instance without adding any new data shapes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The navigator instance is per-window, and there can be multiple windows. As far as I know, there isn't a concept of "the global window value", so src/fetch.js can't really call window.navigator.userAgent unless the user passes in window, which would complicate the interface.

I think exokit wanted a constant, un-configurable User-Agent string, if I remember our conversation correctly from a few months ago when I proposed a configurable User-Agent string. If so, then it seems to make sense for it to be either attached to GlobalContext or a static getter on Navigator.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know, there isn't a concept of "the global window value"

There is, in the specs. For our purposes that is simply global, as set up by node. I think this should work for all of the worker cases, as well as window.

const GlobalContext = require('./GlobalContext');
const fetch = require('window-fetch');

Object.assign(module.exports, fetch);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't the usual way we export in the rest of the codebase. It's usually

module.exports.fetch = fetch;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/fetch.js needs to export everything that window-fetch exports, including Header, Request, Response, and Blob. This accomplishes that.

The idea is that src/fetch.js is a drop-in replacement for window-fetch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's the case, maybe it's best this is put upstream into window-fetch as an interception API?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mm, I would say that overriding the User-Agent header and forcing it to be a specific value is a browser-specific concern. It might be nice if window-fetch provided a way to do that, but the tradeoff is that the API becomes more complicated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's cleaner than requiring a hack to collect and re-export properties from the module.

window-fetch is designed to implement the needs of a "browser", and setting a User-Agent is one of the spec requirements of that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Malicious modules are a valid concern, but a malicious module could do much worse than setting a User-Agent -- and it's not clear that that itself would be a problem regardless.

The API wouldn't necessarily need to be forceTheUserAgentStringToBe -- it could be options on a constructor on the import. This would be roughly equivalent to fetch.js, except owned by window-fetch instead of Exokit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine -- Exokit allows user code to spoof.

Chrome does not: https://developer.chrome.com/multidevice/user-agent

You can reconfigure the user-agent string via extensions (https://chrome.google.com/webstore/detail/user-agent-switcher-for-c/djflhoibgkdhkhhcedjiklpkjnoahfmg?hl=en-US) but it's switched globally; there's no such thing as overriding it on a per-request basis.

This is important to prevent userland JS from spoofing other devices. Google Analytics also relies on standard User-Agent values, so allowing users to override it can break GA.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those concerns are outside the scope of Exokit. Exokit is not a browser.

Copy link
Contributor Author

@shawwn shawwn Jun 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. In that case, feel free to close this PR. I'll merge it into sweetiekit-dom which is what we bundle with SweetieKit.

That will fix our GA issue #1230

I was hoping to maintain parity with exokit's DOM implementation, but it sounds like it might take some time before the necessary functionality / fixes can be integrated into window-fetch and published. The window-fetch extensions also introduce risks due to the dependency on mutable global state.

It might be a better idea for us to switch over to the standard https://github.com/bitinn/node-fetch module. There are significant upstream differences, like support for stream.pipe.
image

Copy link
Member

@avaer avaer Jun 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds like it might take some time before the necessary functionality / fixes can be integrated into window-fetch and published.

Which timing does this refer to? It looks like most of that work is done in this PR.

The window-fetch extensions also introduce risks due to the dependency on mutable global state.

What are the risks?

There are significant upstream differences, like support for stream.pipe.

Indeed, window-fetch should pull from upstream.

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

Successfully merging this pull request may close these issues.

exokit DOM doesn't support google analytics
3 participants