Remove authorization header on redirect to different host #2
Conversation
src/index.js
Outdated
| // Remove authorization if changing hostnames (but not if just | ||
| // changing ports or protocols). This matches the behavior of request: | ||
| // https://github.com/request/request/blob/b12a6245/lib/redirect.js#L134-L138 | ||
| if (url.parse(request.url).hostname !== url.parse(res.headers.location).hostname) { |
There was a problem hiding this comment.
I think we need to emulate request's behavior a little more closely here: res.headers.location can be /foo or foo.com/x, which would not have a hostname property when put through url.parse().
It might be nicer to have code that's more explicit about detecting these different cases, instead of using request's concise but not-very-explicit implementation of it.
This patch looks otherwise fine, and I'm mostly fine with the tests as they are, modulo adding tests for Location: /foo and Location: foo.com and such
There was a problem hiding this comment.
Correction!
The Location header accepts a URI-Reference. That is, either a relative ref, or a fully-qualified URI (with ports). www.foo.com is invalid. I kinda wonder if anyone sends it that way, though.
There was a problem hiding this comment.
Correction again: It allows protocol-relative URIs (that is, //foo.com/bar). So that still needs to be compensated for. Heh.
There was a problem hiding this comment.
I think I solved the issues with parsing different types of urls and also added a couple more test cases. I wasn't sure about URIs like foo.com cause it seems those are not valid but maybe I'm misreading. Lemme know if there is anymore changes I should make.
|
Thanks!! I'll try to make this changes ASAP |
Matches the behavior of request: https://github.com/request/request/blob/b12a6245/lib/redirect.js#L134-L138
…ost (#2) Fixes: #1 Fixes: https://github.com/zkat/pacote/issues/87 Matches the behavior of request: https://github.com/request/request/blob/b12a6245/lib/redirect.js#L134-L138
|
Awesome!! |
It allows us to benefit from some security fix such as npm/node-fetch-npm#2
It allows us to benefit from some security fix such as npm/node-fetch-npm#2
It allows us to benefit from some security fix such as npm/node-fetch-npm#2
…ost (#2) Fixes: #1 Fixes: https://github.com/zkat/pacote/issues/87 Matches the behavior of request: https://github.com/request/request/blob/b12a6245/lib/redirect.js#L134-L138
…ost (#2) Fixes: #1 Fixes: https://github.com/zkat/pacote/issues/87 Matches the behavior of request: https://github.com/request/request/blob/b12a6245/lib/redirect.js#L134-L138
…ost (#2) Fixes: #1 Fixes: https://github.com/zkat/pacote/issues/87 Matches the behavior of request: https://github.com/request/request/blob/b12a6245/lib/redirect.js#L134-L138
Matches the behavior of request:
https://github.com/request/request/blob/b12a6245/lib/redirect.js#L134-L138
Fixes #1
A few cases are missing from the tests but I wasn't sure if I should create a new server just for that and I dunno if I should test all redirect status codes for this case.
I wasn't sure if this had to be handled in that piece of code you pointed to, cause that only seems to cover POST requests and from the
requestcode it seems that they do it for all methods but HEAD