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

Allow strict checks for Content-* headers #66

Conversation

weierophinney
Copy link
Member

Previously, we would check any key starting with CONTENT_ in the $_SERVER array, and treat it as a header. However, per #63, this is incorrect, and we should only look for CONTENT_TYPE, CONTENT_LENGTH, and CONTENT_MD5. To keep backwards compatibility, this patch implements a switch, using the ENV variable LAMINAS_DIACTOROS_STRICT_CONTENT_HEADER_LOOKUP. When this value is found in $_SERVER (which aggregates ENV variables as well), the logic for identifying headers in the $_SERVER array will become strict.

For version 3.0, we will make the strict lookup the default.

Fixes #63

Previously, we would check any key starting with `CONTENT_` in the `$_SERVER` array, and treat it as a header.
However, per laminas#63, this is incorrect, and we should only look for `CONTENT_TYPE`, `CONTENT_LENGTH`, and `CONTENT_MD5`.
To keep backwards compatibility, this patch implements a switch, using the ENV variable LAMINAS_DIACTOROS_STRICT_CONTENT_HEADER_LOOKUP.
When this value is found in `$_SERVER` (which aggregates ENV variables as well), the logic for identifying headers in the `$_SERVER` array will become strict.

For version 3.0, we will make the strict lookup the default.

Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
@weierophinney weierophinney added this to the 2.6.0 milestone May 17, 2021
@weierophinney weierophinney added the Bug Something isn't working label May 17, 2021
@weierophinney weierophinney force-pushed the feature/limit-content-headers-from-server branch from 75e9caa to 9b37fc1 Compare May 17, 2021 22:01
Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
@weierophinney weierophinney force-pushed the feature/limit-content-headers-from-server branch from 9b37fc1 to b8d2371 Compare May 17, 2021 22:05
Copy link
Member

@boesing boesing left a comment

Choose a reason for hiding this comment

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

LGTM - maybe using static functions could lead to minor performance improvements but not 100% sure about this.

using a private constant could also be a way to get rid of the static variable.

src/functions/marshal_headers_from_sapi.php Outdated Show resolved Hide resolved
src/functions/marshal_headers_from_sapi.php Outdated Show resolved Hide resolved
@bcremer
Copy link

bcremer commented May 18, 2021

Fix looks good for me 👍

@weierophinney
Copy link
Member Author

using a private constant could also be a way to get rid of the static variable.

That would work if this were a class. However, since it's a function within a namespace, all constants are thus publicly visible.

Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Signed-Off-By: <matthew@weierophinney.net>

Co-authored-by: Frank Brückner <info@froschdesignstudio.de>
@weierophinney weierophinney merged commit dfca348 into laminas:2.6.x May 18, 2021
@weierophinney weierophinney deleted the feature/limit-content-headers-from-server branch May 18, 2021 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Environment variables prefixed with CONTENT_ may overwrite HTTP-Headers provided by the client
4 participants