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

Copy the headers from the last request on redirect #24

Merged
merged 4 commits into from
Oct 13, 2017

Conversation

gummiboll
Copy link
Contributor

Apparently headers does not survive a redirect in net/http (golang/go#4800) so here's a fix for that. Probably not the most elegant way of doing it though

@asciimoo
Copy link
Member

@gummiboll great, useful addition. Authentication header should be excluded to prevent credential leakage (at least in case the redirect points to a different domain).

@gummiboll
Copy link
Contributor Author

I do get what you mean but as I was thinking about how I would best implement this I started thinking about it; with AllowedDomains already in effect, is this needed?

Come to think about it, this PR might override AllowedDomains. Will look into it later!

@asciimoo
Copy link
Member

@gummiboll this redirect handler function could be a private member of the Collector, so it could have access to the AllowedDomains attribute and it could check if the redirect target is in the AllowedDomains. What do you think?

@gummiboll
Copy link
Contributor Author

Good idea! How about this?

@asciimoo
Copy link
Member

@gummiboll awesome! Could you please exclude Authentication header when the redirection points to a different domain?

@gummiboll
Copy link
Contributor Author

Yes, here you go. :)

Copy link
Member

@asciimoo asciimoo left a comment

Choose a reason for hiding this comment

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

Great, thank you.

@asciimoo asciimoo merged commit b45e8de into gocolly:master Oct 13, 2017
@gummiboll gummiboll deleted the copyheadersonredirect branch October 13, 2017 13:02
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.

2 participants