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

Add ability to follow redirects #1490

Merged
merged 1 commit into from Mar 6, 2019
Merged

Add ability to follow redirects #1490

merged 1 commit into from Mar 6, 2019

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 :-)


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.

None yet

3 participants