Fix #78719: http wrapper silently ignores long Location headers#6676
Closed
cmb69 wants to merge 1 commit intophp:PHP-7.4from
Closed
Fix #78719: http wrapper silently ignores long Location headers#6676cmb69 wants to merge 1 commit intophp:PHP-7.4from
cmb69 wants to merge 1 commit intophp:PHP-7.4from
Conversation
Currently, the HTTP wrapper reads and processes header lines of at most 1024 bytes (including the trailing line break). This appears to be overly restrictive, particularly with regard to Location headers, which may contain long URIs. There is no standard regarding the maximum length of an URI, but assuming about 2000 bytes appears to be prudent. We therefore increase the size of the buffer to 2048. That still does not properly cater to header lines which are even longer. According to RFC 7230, section 3.2.5, it is okay to discard long header lines, only if that does not change the "message framing or response semantics". So, e.g. an overlong Location header must not be discarded if `follow_location` is set (the default), and probably should not even if `follow_location` is not set. We therefore let the attempt to open the stream fail in this case.
Member
|
This doesn't really seem like a solution to the problem. IMHO we should be allowing arbitrary header lengths here, and not limiting them by the size of an internal buffer. For example we could call php_stream_get_line with a NULL buffer, in which case it will allocate one of appropriate size itself. (If we want to avoid allocations in the common case, we'd have to change php_stream_get_line to support both an initial fixed-size buffer and dynamic growth.) |
Member
Author
|
Thanks! Makes sense; I'll overhaul the PR ASAP. |
Member
Author
|
Closing in favor of PR #6720. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently, the HTTP wrapper reads and processes header lines of at most
1024 bytes (including the trailing line break). This appears to be
overly restrictive, particularly with regard to Location headers, which
may contain long URIs. There is no standard regarding the maximum
length of an URI, but assuming about 2000 bytes appears to be prudent.
We therefore increase the size of the buffer to 2048.
That still does not properly cater to header lines which are even
longer. According to RFC 7230, section 3.2.5, it is okay to discard
long header lines, only if that does not change the "message framing or
response semantics". So, e.g. an overlong Location header must not be
discarded if
follow_locationis set (the default), and probablyshould not even if
follow_locationis not set. We therefore let theattempt to open the stream fail in this case.