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

use fully-resolved URI with custom scheme for HTTP Referer header, or a different header name entirely #930

Closed
isaacs opened this issue Feb 22, 2020 · 2 comments

Comments

@isaacs
Copy link
Contributor

isaacs commented Feb 22, 2020

Background

The fact that the npm client sends a referer header with a redacted copy of the npm command is cute, and not exactly a violation of the HTTP specification, but a weird use of it, which has caused some recent disruptions and pushback.

The reason for choosing Referer in the first place is that it is very reliably left intact by proxies. And, semantically, the resource represented by the command sent to npm is the "thing" that triggered the request. However:

  1. Losing some data is not so bad. We already use npm-session and a few pacote headers, and rely on them in our registry data analysis. Some of them undoubtably get dropped by over eager proxies more frequently than Referer would, but it's fine.
  2. Using something other than a fully qualified URI is treated by some heuristics as an indication of malicious requests. While this is arguably inappropriate, it certainly does exist.
  3. Originally this was a way for us to help determine the frequency of install vs update vs install with a given argument. Well, in practice, some 99% or more of all requests are just Referer: install, so it doesn't actually provide much value in practice in aggregate. However, it can provide some insight in certain debugging scenarios, so it's not completely without merit.

Proposed Change

  • Remove the referer header from all npm CLI requests.
  • Add a npm-command header to registry requests, with the current value of the Referer header.

Alternatively, don't send an npm-command header, since it's always install anyway. (And when it isn't, there's usually a way to infer what's going on from the HTTP verb and route.)

Example

No user-facing changes, except that referer: in a typical npm request will be replaced with npm-command:.

How

Current Behaviour

npm sends a referer: header.

Desired Behaviour

npm does not send a referer: header.

Implementation

  • Unless a referer header is explicitly supplied in the headers option to make-fetch-happen, do not send a referer header.
  • Remove the --refer config flag in the cli where it calculates and redacts based on the argv.
  • Remove the refer field in npm.flatOptions.
@isaacs
Copy link
Contributor Author

isaacs commented Feb 22, 2020

More on this... shouldn't the Referer of a tarball be the packument URL if we fetched that first? Or file:///path/to/package-lock.json if that's where we got it from? (And thus ignored, because you don't send Referer headers for file URIs.)

Definitely best to just omit it.

isaacs added a commit to npm/npm-registry-fetch that referenced this issue Feb 24, 2020
Re: npm/cli#930

BREAKING CHANGE: Removes the 'opts.refer' option and the HTTP Referer
header (unless explicitly added to the 'headers' option, of course).
isaacs added a commit to npm/npm-registry-fetch that referenced this issue Feb 24, 2020
Re: npm/cli#930

BREAKING CHANGE: Removes the 'opts.refer' option and the HTTP Referer
header (unless explicitly added to the 'headers' option, of course).

PR-URL: #25
Credit: @isaacs
Close: #25
Reviewed-by: @mikemimik
@isaacs
Copy link
Contributor Author

isaacs commented Mar 1, 2020

This will be fixed as we move deps to the new versions of npm-registry-fetch and libraries using it. I don't think we have to track this separately.

@isaacs isaacs closed this as completed Mar 1, 2020
isaacs added a commit that referenced this issue Mar 10, 2020
isaacs added a commit that referenced this issue Mar 17, 2020
isaacs added a commit that referenced this issue May 8, 2020
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

No branches or pull requests

1 participant