-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Don't forward authorization header across redirects to different hosts #1184
Conversation
After this change, the following request to AWS works fine (code based on #450 (comment)): var http = require('http')
, child_process = require('child_process')
, request = require('./index')
var s = http.createServer(function(req, res) {
res.writeHead(301, {
location : 'http://aws-lightbox.s3.amazonaws.com/source/jquery.fancybox.css'
})
res.end()
}).listen(5678)
var requests = 0
request('http://foo:bar@localhost:5678/', function(err, res, body) {
console.log('Request', res.statusCode)
if (++requests == 2) s.close()
})
child_process.exec('curl -L -w "%{http_code}" -o /dev/null http://foo:bar@localhost:5678/', function(err, res) {
console.log('Curl', res)
if (++requests == 2) s.close()
}) |
@@ -1252,6 +1255,12 @@ Request.prototype.onRequestResponse = function (response) { | |||
self.removeHeader('host') | |||
self.removeHeader('content-type') | |||
self.removeHeader('content-length') | |||
if (self.uri.hostname !== self.originalHost.split(':')[0]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, if we redirect from abc.com:8080
to abc.com:1234
we preserve auth headers?
Seems like they should be dropped whenever the self.uri.host
changes, even if the change is only the port.
+1. If the readme mentions what we do on redirect, perhaps add a mention there about auth headers being dropped? Additional question: What happens if we redirect BACK to that original host? Ie, the request chain goes:
Should we send the |
looks like curl doesn't send it back if the third is the same as the first. (whoa.) var http = require('http')
, count = 0
;
http.createServer(function(req, res) {
console.log(req.headers);
if (count == 0) {
res.statusCode = 301;
res.setHeader('location', 'http://127.0.0.1:3000');
} else if (count == 1) {
res.statusCode = 301;
res.setHeader('location', 'http://localhost:3000');
} else
res.write('done!\n');
res.end();
count = count < 2 ? count + 1 : 0;
}).listen(3000); $ curl -X GET -L http://foo:bar@localhost:3000 the server logs: { authorization: 'Basic dG9tOmJ1Y2hvaw==',
'user-agent': 'curl/7.30.0',
host: 'localhost:3000',
accept: '*/*' }
{ 'user-agent': 'curl/7.30.0',
host: '127.0.0.1:3000',
accept: '*/*' }
{ 'user-agent': 'curl/7.30.0',
host: 'localhost:3000',
accept: '*/*' } |
We probably should, but that's a much more extensive change. (I bet |
I don't see a relevant place to note this in the readme, others feel free to suggest something though. |
Nope, { 'user-agent': 'Wget/1.14 (linux-gnu)',
accept: '*/*',
authorization: 'Basic Zm9vOmJhcg==',
host: 'localhost:3000',
connection: 'Keep-Alive' }
{ 'user-agent': 'Wget/1.14 (linux-gnu)',
accept: '*/*',
host: '127.0.0.1:3000',
connection: 'Keep-Alive' }
{ 'user-agent': 'Wget/1.14 (linux-gnu)',
accept: '*/*',
host: 'localhost:3000',
connection: 'Keep-Alive' } |
Ok, sounds good to me then. Ship it! We still do the green button on this repo, right? |
Yep. Thanks for reviewing. |
Don't forward authorization header across redirects to different hosts
As of v2.46.0 (because of request#1184) this crashes when a redirect occurs. We should also preserve the case of the header name if the user specifies 'Host', because lots of (broken) servers expect the header name to be 'Host' and not 'host'.
…nd contenct-length headers, body and _form removal check to mantain issue request#1184 behavior.
When redirecting to a different host, we should blow away any authorization header. This has caused issues with AWS, with npm install from private GitHub repos (ping @isaacs), and other servers.
Also see prior discussion at #160, #450, #860, #1058.