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

Fix issue #160, remove auth header on redirect #432

Closed
wants to merge 3 commits into from
Closed

Fix issue #160, remove auth header on redirect #432

wants to merge 3 commits into from

Conversation

EchoAbstract
Copy link

This fix removes the authorization header when the host
that's being redirected to is different than the host
that's redirecting, if the hosts are the same then the
auth header remains the same.

Tests included.

This is try 2, I don't think the first pull request sent successfully.

This fix removes the authorization header when the host
that's being redirected to is different than the host
that's redirecting, if the hosts are the same then the
auth header remains the same.

Tests included.
@@ -674,12 +678,20 @@ Request.prototype.start = function () {
if (response.statusCode != 401) {
delete self.body
delete self._form

// Check for auth header and remove if the host doesn't match, issue #160
if (self.headers && url.parse(redirectTo).host !== self.originalHost){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it necessary to keep the "orginalHost" around? can't we just check the current host against the redirect url host before we do the forward and remove the authorization.

@EchoAbstract
Copy link
Author

Is there another variable that I can use to get the current host? I tried using the host header, but that's cleared prior to redirection. It's very possible I missed the variable that's used to store the current host. Let me know if there's a better way to do this.

Per comment by @mikeal removed the references to
originalHost.

Since this is just hostname (not host + port) updated the unit
tests to use localhost ip instead of hostname, also removed
the extra server that's not required.
@@ -558,7 +558,10 @@ Request.prototype.start = function () {
return
}

if (self.setHost) delete self.headers.host
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this style change shouldn't be in this pull req.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was changed for the first commit to add a second statement and accidentally left during the second. Fixing now.

@mikeal
Copy link
Member

mikeal commented Mar 1, 2013

this doesn't merge cleanly, please reopen when it does.

@mikeal mikeal closed this Mar 1, 2013
@EchoAbstract EchoAbstract deleted the 160-redirect-remove-basic-auth branch March 1, 2013 20:55
@EchoAbstract EchoAbstract restored the 160-redirect-remove-basic-auth branch March 1, 2013 20:56
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

Successfully merging this pull request may close these issues.

None yet

2 participants