8000 Fix: Allows headers without whitespace after colon. by AlliBalliBaba · Pull Request #1741 · php/frankenphp · GitHub
[go: up one dir, main page]
Skip to content

Fix: Allows headers without whitespace after colon.#1741

Merged
dunglas merged 4 commits intomainfrom
fix/allow-headers-without-whitespace
Jul 16, 2025
Merged

Fix: Allows headers without whitespace after colon.#1741
dunglas merged 4 commits intomainfrom
fix/allow-headers-without-whitespace

Conversation

@AlliBalliBaba
Copy link
Contributor
@AlliBalliBaba AlliBalliBaba commented Jul 12, 2025

Fixes #1738

rfc7230 apparently doesn't explicitly disallow headers without a whitespace after the colon.

This PR allows both ways of calling the PHP ´header` function, since apparently FPM/nginx also allows them:

header('Content-Type: application/json'); # well formed, currently forwarded
header('Content-Type:application/json'); # not well formed, but still valid and currently not forwarded

@dunglas
Copy link
Member
dunglas commented Jul 12, 2025

It will likely be faster to split using a regex than calling split 2 times.

@AlliBalliBaba
Copy link
Contributor Author
AlliBalliBaba commented Jul 12, 2025

The performance will be the same in this case for 'well formed' headers, but worse for non-well formed headers.

I think this could actually be made faster though by scanning for the colon in c and then calling GoStringN() on the respective parts of the *char.

@AlliBalliBaba
Copy link
Contributor Author
AlliBalliBaba commented Jul 12, 2025 •
edited
Loading

I rewrote the header splitting to ignore all whitespaces after ':', it will now also directly allocate key and value from the *char.

This is a bit faster than strings.SplitN(C.GoStringN(cstringForSplit, length), ": ", 2), but only a small optimization in the context of a whole request.

Test with go test -bench=. (only splitting this specific header without adding it: "Header-Key: value")

BenchmarkHeaderSplitOld-20              21330975                50.82 ns/op
BenchmarkHeaderSplitNew-20              39266910                29.69 ns/op

@dunglas dunglas merged commit a161af2 into main Jul 16, 2025
22 checks passed
@dunglas dunglas deleted the fix/allow-headers-without-whitespace branch July 16, 2025 06:57
@dunglas
Copy link
Member
dunglas commented Jul 16, 2025

Good one! Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dunglas dunglas dunglas approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bug - Location header ignored when no space follows the colon

2 participants

@AlliBalliBaba @dunglas
0