Filter integer header names during SAPI discovery #157
Conversation
This file contains 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
Integer strings have the nasty habit of being cast to actual integers by PHP, making them problematic for usage in an associative array, despite being valid per the RFC 7230 ABNF. Additionally, having them pass through `marshal_headers_from_sapi()` means that once `ServerRequest` gets them and tries to use them, `HeaderSecurity::assertValidName()` will raise an exception for integers, which could lead to unexpected server errors. This patch chooses to filter such header names out entirely. Doing so prevents those server errors. Signed-off-by: Matthew Weier O'Phinney <[email protected]>
Creates a "security features" document that brings in the former v2 "forward migration" document around filtering x-forwarded-* headers, and also adds narrative around filtering integer headers. Signed-off-by: Matthew Weier O'Phinney <[email protected]>
…hange Signed-off-by: Matthew Weier O'Phinney <[email protected]>
Xerkus
reviewed
May 4, 2023
Xerkus
reviewed
May 4, 2023
Co-authored-by: Aleksei Khudiakov <[email protected]> Signed-off-by: Matthew Weier O'Phinney <[email protected]>
Signed-off-by: Matthew Weier O'Phinney <[email protected]>
Adds integer and string-integer keys from the `marshalHeadersFromSapi()` test case to the ServerRequestFactory test cases to demonstrate they get filtered out. Signed-off-by: Matthew Weier O'Phinney <[email protected]>
Links to PHP bug 80309 in discussion of the string integer value to integer conversion. Signed-off-by: Matthew Weier O'Phinney <[email protected]>
When a header field is added with `withHeader()` or `withHeaderLine()`, these will accept strings that have digits only, which can also lead to the issues presented elsewhere. Signed-off-by: Matthew Weier O'Phinney <[email protected]>
Signed-off-by: Matthew Weier O'Phinney <[email protected]>
Xerkus
reviewed
May 4, 2023
Co-authored-by: Aleksei Khudiakov <[email protected]> Signed-off-by: Matthew Weier O'Phinney <[email protected]>
Xerkus
approved these changes
May 4, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Integer strings have the nasty habit of being cast to actual integers by PHP, making them problematic for usage in an associative array, despite being valid per the RFC 7230 ABNF.
Additionally, having them pass through
marshal_headers_from_sapi()means that onceServerRequestgets them and tries to use them,HeaderSecurity::assertValidName()will raise an exception for integers, which could lead to unexpected server errors.This patch chooses to filter such header names out entirely.
It also documents the change, and ways to address it if you previously depended on integer header field names.
Fixes #11