Skip to content

Conversation

@lpinca
Copy link
Member

@lpinca lpinca commented Jan 19, 2019

Known defects:

  • The options object it rebuilt after every redirect. This could be optimized to only do it once and update only the options that need to be changed after every redirect.
  • There might be issues if an agent is used and the protocol is changed from HTTPS to HTTP or vice versa since HTTPS and HTTP use different agents.

Fixes #812

@piranna
Copy link

piranna commented Jan 20, 2019

  • There might be issues if an agent is used and the protocol is changed from HTTPS to HTTP or vice versa since HTTPS and HTTP use different agents.

I would throw an error if changuing from HTTPS to HTTP to notify about security issues by default, and maybe add an option to disable it.

@lpinca
Copy link
Member Author

lpinca commented Jan 20, 2019

We can but afaik none of the most popular HTTP libs (request, got, node-fetch, axios, etc.) do it. Accepting redirects has a lot of security implications, like open redirect attacks, or passing authorization headers when the hostname changes.

It should be enabled only when the server is trusted.

@piranna
Copy link

piranna commented Jan 20, 2019

It should be enabled only when the server is trusted.

So do you think just lefting redirections as an opt-in is just enough?

@lpinca
Copy link
Member Author

lpinca commented Jan 20, 2019

I don't know if it is enough, but it is the reason why I chose to not follow redirects by default.

@lpinca lpinca merged commit 161f303 into master Mar 6, 2019
@lpinca lpinca deleted the follow/redirects branch March 6, 2019 06:47
@piranna
Copy link

piranna commented Mar 6, 2019

Great! Thank you :-)

: url.resolve(address, location);

abortHandshake(this, req, `Unexpected server response: ${res.statusCode}`);
initAsClient(websocket, addr, protocols, options);
Copy link

Choose a reason for hiding this comment

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

I think the redirect address should be emitted in some way to allow tracking.

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.

4 participants