-
-
Notifications
You must be signed in to change notification settings - Fork 38
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
Pass custom request headers when following redirects #91
Conversation
Thank you for this PR, I would love to get this feature in! 👍 However, there are a number of HTTP request headers that probably should not be forwarded, for example request/request@210b326#diff-ccc0734f65dd7a299409ff07d35be095R1255. Did you have a chance to look into this? |
@clue done! I've moved redirect request creation logic into a separate method, so Looking forward to your feedback 😎 |
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.
Thank you for the very quick update! The changes LGTM for the most part, but I've added a minor remark below 👍
src/Io/Transaction.php
Outdated
->withoutHeader('Content-Type') | ||
->withoutHeader('Content-Length'); | ||
|
||
if($location->getHost() !== $originalHost) { |
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.
It's my understanding this should probably verify the whole authority, no? (For example when redirecting from https:// to http:// (not sure about the other way around) or using another port etc.)
Also, this block could use a small comment (why), plus minor CS fix 👍
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.
We remove authorization only on changing hostname (not if just changing ports or protocols). I've added a description comment and fixed CS.
I've added some comments explaining the logic, and fixed CS issue 😎 |
->withoutHeader('Content-Length'); | ||
|
||
// Remove authorization if changing hostnames (but not if just changing ports or protocols). | ||
if ($location->getHost() !== $originalHost) { |
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.
See my above comment, do you have a specific use case in mind where it makes sense to keep this if the URI components change, but the host stays the same? It's still my understanding that this should check the URI authority instead, but perhaps I'm missing a relevant use case?
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.
Actually my assumptions about this behavior are based on other implementations:
- Don't forward authorization header across redirects to different hosts request/request#1184
- Composer should not send authorization header when following redirect composer/composer#6716
It looks like request
and composer
only check for host difference. Or I'm missing some-thing?
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.
Thank you for looking this up, no objections in this case 👍
This PR fixes #88