Refused to set unsafe header "user-agent"

[aaroncraig10e]

This isn't technically an issue with this library, however it should be noted that neither Chrome nor Safari currently allow setting the "user-agent" header, both throw Refused to set unsafe header "user-agent" errors.

It might be responsible to detect whether or not the browser allows for setting this header to avoid spurious errors getting thrown.

I'm happy to open a PR if there's a chance it will go in. We'd certainly like that, as currently we're getting this error on every page load.

[f2prateek]

@aaroncraig10e thanks for reporting. what's the end behaviour - does the request get blocked, or is it only that the user agent doesn't get set and a warning/error is shown in the console?

Depending on what the implementation looks like, I think we'd be open to detecting this case and handling it appropriately. This seems preferable if it's not too challenging.

Alternatively, we could have an API to disable sending the user agent.

[aaroncraig10e]

It looks like it makes the request without overriding the user agent string, ie I see (in the Network tab, so the request is successfully made):

:authority:api.segment.io
:method:POST
:path:/v1/batch
:scheme:https
accept:application/json, text/plain, */*
accept-encoding:gzip, deflate, br
accept-language:en-US,en;q=0.9,it;q=0.8
authorization:Basic [redacted]
cache-control:no-cache
content-length:401
content-type:application/json;charset=UTF-8
origin:http://localhost:3000
pragma:no-cache
referer:http://localhost:3000/settings/users
user-agent:Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.94 Safari/537.36

So, it looks like the only problem is the spurious error (and not setting the header value, of course).

I'm thinking the implementation would be to add a try...catch around the setRequestHeader call, and if an error is thrown and the key is 'user-agent', emit a warning instead of the error?

If try/catch doesn't work on a low-level call like that (the error may get emitted by the browser anyway), then I suppose detecting for Firefox and filtering everything else (with a warning?) is the next best approach.

Hopefully this issue will disappear when all the vendors catch up to the spec.

Happy to hear feedback on the implementation, and I'll try to get a PR together today or tomorrow.

[f2prateek]

cc @vadimdemedes @stephenmathieson what do you think? try-catch feels like it could hide other unintended bugs.

Maybe we could make user agent an option that defaults to our current user-agent - and then browser users could set it to "" to skip setting the user agent.

[stephenmathieson]

Interesting. I didn't know that Chrome/Safari did this!

I think a try/catch around setting the user agent would be fine, but I'm not 100% sure Axios would allow us to do this. If the library supports it, then 👍 from me =]

[aaroncraig10e]

@f2prateek sorry, should have specified -- I'd detect both the error message and the header key, and rethrow any error that isn't this specific problem.

[vadimdemedes]

try-catch feels like it could hide other unintended bugs.

We shouldn't do it, because it will hide bugs, which is not something we'd want. Instead, we should check if analytics-node is being run in a browser env (typeof window !== 'undefined') and see if we need to set user agent header. If this is a crucial information that can't be omitted, we should consider sending it some other way, maybe via query string or req body.

[aaroncraig10e]

I've opened an issue with Axios -- if they are open to a PR, then a fix upstream will be more useful to everyone.

Leaving the link here and will update as appropriate:
axios/axios#1231

[f2prateek]

Looks like we haven't heard back from axios yet, so tracking internally in JIRA https://segment.atlassian.net/browse/LIB-177.

I do think we want to send this as a header for the ALB logs (we already send it in the body but that's only available in logs once it gets accepted by the API). Sending writeKeys in paths could be an option but will require multiple changes from our API side (tracking-api, api-forwarder, api at least). Not sure if it's worth it to accommodate just this single library.

[ghost]

For now we're going to target v3.1.0 of the library to get around this problem. If you want to track version usage in the header, perhaps you can use a custom header like X-Segment-Version?

[stephenmathieson]

I think a custom header could work, but I'm not sure the logs from the ALB will contain the header.

@f2prateek since this is preventing users from using analytics-node and nothing (that I'm aware of) is relying on the user-agent being set (yet), should we remove it for now?

[f2prateek]

Sorry thought I already replied here. Correct, the issue is that custom headers don't show up in ALB logs.

I also just made a ticket to remove this https://segment.atlassian.net/browse/LIB-184.

Also I don't think we plan on actively using this header so it's expected that nothing is using it now (and even in the future). If we were to find an issue in the node library, then we'd be using this header to check for impact in the ALB logs.

[stephenmathieson]

Right, but my question isn't what the header is for, but if it's more important than users being able to use analytics-node. Currently you're unable to send events using the library in Chrome or Safari. Is this more or less important than the possibility to check logs for impact?

[f2prateek]

Currently you're unable to send events using the library in Chrome or Safari.

Going off what was reported in #139 (comment). It sounded like a warning is surfaced is surfaced (in Chrome and Safari) but the events themselves aren't being dropped. Though I guess for some folks the warning being surfaced can be enough of a "blocker".

My initial thinking here is that is a bit of of an edge case (given this is primarily a server side library u like a.js) and that we can suggest the following options for the users using analytics-node on Chrome and Safari:

• use 3.1.1 to get rid of the warnings
• use 3.2.0 with the warnings

And of course we'll get this fixed as well so Chrome/Safari users can use the latest version of analytics-node. So we're tracking it internally at https://segment.atlassian.net/browse/LIB-184. Don't have an ETA yet, but will post here when there's an update.

[aaroncraig10e]

I can confirm that in our setup, the events do get sent even with the warning.

The behavior does not break anything else going on in the app -- it's just the error is a bit of noise that we'd like to get rid of rather than learn to ignore.

Regarding the comment that this is primarily a server side library, I'm wondering if we're using the wrong library in the browser? What is the recommended library for integrating a React app into Segment.io?

[f2prateek]

Typically we recommend using analytics.js https://segment.com/docs/sources/#website. It has a fair amount of more client side specific features. e.g. grabbing browser context information and loading client side destinations. Another one we're currently working on is persistent queueing.

There's definitely nothing wrong with using analytics-node in the browser, it just won't have the same set of browser specific features as a.js. Depending on your use case, that may be an acceptable trade-off!

[stephenmathieson]

Ah, sorry. I read this as a literal throw new Error(...):

both throw Refused to set unsafe header "user-agent" errors

I agree, if it's just a warning, I don't think we need to revert the UA code quite yet. It would be nice to remove the warning at some point soon tho