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

Reject non-HTTP schemes in StreamHandler #2989

Merged
merged 2 commits into from Mar 13, 2022
Merged

Conversation

TimWolla
Copy link
Contributor

@TimWolla TimWolla commented Feb 24, 2022

I recommend not to squash this PR when merging, as the commits try to tell a story.

Detailed reasoning can be found in the commit message of each of the both commits.

TimWolla added 2 commits Feb 24, 2022
If an URI that does not use the HTTP stream wrapper is passed to the
StreamHandler then the magic `$http_response_header` variable will not be
filled, thus remaining `null`.

This ultimately results in a `TypeError`, because `null` is passed to
HeaderProcessor::parseHeaders(), which expects an `array`.
Non-HTTP schemes are effectively not supported, because the HTTP response
headers will only be filled for the `http` and `https` stream wrappers. Also
Guzzle is an HTTP client after all.

Reject non-HTTP schemes early on to improve error messages and to prevent
possible exploits using odd stream wrappers in case an non-fully-trusted URL is
passed to Guzzle.
@Nyholm
Copy link
Member

Nyholm commented Mar 13, 2022

What if you want to fetch a file on an FTP server. Then you cannot use the stream handler, right?

@GrahamCampbell
Copy link
Member

GrahamCampbell commented Mar 13, 2022

Maybe something to consider for Guzzle 8. I don't feel like we can merge this on Guzzle 7.

@GrahamCampbell GrahamCampbell added the lifecycle/keep-open Issues that should not be closed label Mar 13, 2022
@TimWolla
Copy link
Contributor Author

TimWolla commented Mar 13, 2022

What if you want to fetch a file on an FTP server. Then you cannot use the stream handler, right?

Right, but you already cannot do this, because FTP is not HTTP and thus no HTTP headers will be set. In fact FTP is how I originally found out about this issue: https://www.woltlab.com/community/thread/294643-jeden-tag-selben-fehler-im-log/ (in German).

So this does not break anything that wasn't broken previously. It just improves the error message.

@TimWolla
Copy link
Contributor Author

TimWolla commented Mar 13, 2022

Alternatively just take the first commit for now, I've intentionally split this into two separate commits. The first one fixes the underlying issue (and that should be easy to verify).

@GrahamCampbell GrahamCampbell added this to the 8.0.0 milestone Mar 13, 2022
@GrahamCampbell GrahamCampbell removed the lifecycle/keep-open Issues that should not be closed label Mar 13, 2022
@GrahamCampbell GrahamCampbell removed this from the 8.0.0 milestone Mar 13, 2022
Nyholm
Nyholm approved these changes Mar 13, 2022
Copy link
Member

@Nyholm Nyholm left a comment

Thank you.

I do need to squash the commits though. I like a nice history like this:
Screenshot 2022-03-13 at 17 19 33

@Nyholm Nyholm merged commit be834db into guzzle:master Mar 13, 2022
23 checks passed
@TimWolla TimWolla deleted the stream-non-http branch Mar 13, 2022
@TimWolla
Copy link
Contributor Author

TimWolla commented Mar 13, 2022

I do need to squash the commits though. I like a nice history like this:

That's where I disagree. For my / our projects I rather prefer separate small commits (unless they are useless stuff like “fix typo”). But you're the maintainers and need to work with it, so I won't complain 😃

@Nyholm
Copy link
Member

Nyholm commented Mar 13, 2022

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