Added hook to address 400 error returned during redirect

[Js-Brecht]

During tarball resolution, some npm registries will redirect to the actual file location. In my case, using Azure Artifact feeds, that redirect will return a 400 error if authentication headers are present. See #316.

Since got follows redirects automatically, I thought it would be best to hook into their process to update headers. Since I can't be certain that redirects made by all npm registries will have the same requirements for no Authorization: header, this will wait until a 400 error is returned, after a redirect. Then the request will be retried, and the authentication header removed.

Because this is forcing request retries, it has the potential to slow down the package fetching. So, I also added a redirect cache. It makes the assumption that once the original host returns a redirect, and that redirect returns 400, all redirects from that host will return the same. Thereafter, any redirect by that host will follow the same rules (i.e. remove the authentication headers). The other option would have been to cache by the redirected host, but that would not have been as effective, because (in my case), those redirected URLs were always different.

For example, pkgs.dev.azure.com would return 1yovsblobprodeus2184.blob.core.windows.net for one package, and puxvsblobprodeus216.blob.core.windows.net for another. This means the cache would be ineffectual, and every single request made to pkgs.dev.azure.com would need to be retried a minimum of one time. Therefore, the host cached will be pkgs.dev.azure.com.

Since these calls are made async, there might be one or two packages that have to retry their request once, because the cache hasn't yet been populated with that host. However, once it has been populated, the original request does not fail, because the beforeRedirect() hook is processed correctly. This was apparent during debugging, where sometimes both the afterResponse() hook with a 400 error and the beforeRedirect() was hit for two packages; and sometimes afterResponse() was only hit once, and every request afterwards would only hit the beforeRedirect(), and return successfully.

One thing I noticed is that got does not play nicely with the current Body interface. It is unable to infer the type of request, because the Body interface contains a null option, and also because of the dynamic key index. It looks like it would take more work here, and/or on the got side to make that work. So I just went with the established convention of forcing typescript ignore the type errors. Which is where all the //@ts-ignore's came from 😆. Honestly, I hate doing that, but I also didn't want to exceed the scope of this issue.

[arcanis]

Thanks for that! 😀

I'll look at the code later this week end, but in the meantime I just wanted to say that I think we should remove the authorization header for all redirects. It seems to match the behavior of the request package (cf npm/node-fetch-npm#1), and it avoids having to bother with a cache.

[Js-Brecht]

I will change it later. That will simplify it a lot.

[Js-Brecht]

Nevermind, I just did a quick hack. Removed the afterResponse() hook, and the redirect caching.

[arcanis]

Awesome, thanks 😃

[arcanis]

I just noticed that the Authorization header is meant to be removed only when the hostname changes (which makes sense as it would otherwise leak the token, but not when staying on the same hostname). Would you mind doing a followup PR to add this check to the hook?

Ref: https://github.com/npm/node-fetch-npm/pull/2/files#diff-1fdf421c05c1140f6d71444ea2b27638R90-R92

[arcanis]

Errata, I've submitted #334 to fix this 🙂