Skip to content

fix: drop rendered headers containing NUL as well as CR and LF - #1260

Open
pjfanning wants to merge 1 commit into
apache:mainfrom
pjfanning:guard-nul-in-rendered-headers
Open

fix: drop rendered headers containing NUL as well as CR and LF#1260
pjfanning wants to merge 1 commit into
apache:mainfrom
pjfanning:guard-nul-in-rendered-headers

Conversation

@pjfanning

Copy link
Copy Markdown
Member

Motivation

The guard in Rendering.~~(HttpHeader) renders a header and then scans the rendered bytes, discarding it if it finds a character that must never reach the wire. It only looked for CR and LF:

if (ch == '\r' || ch == '\n') { /* discard */ }

so a RawHeader value carrying a NUL was rendered as-is. NUL is not a legal field-value character, and a downstream consumer that treats the value as a C string truncates it there — so two parties can disagree about where the value ends. The HTTP/2 renderer rejects CR, LF and NUL alike (see #1258), which left HTTP/1.1 as the weaker of the two.

Modification

Add Rendering.isIllegalHeaderChar, covering CR, LF and NUL, and call it from all four check implementations — StringRendering, ByteArrayRendering, ByteStringRendering and CustomCharsetByteStringRendering — rather than repeating the character comparison a fourth time. It takes an Int so the Char- and Byte-based renderings can pass their element straight in.

Result

A header whose name or value contains NUL is discarded like one containing CR or LF, on every rendering implementation, and the rule now lives in one place.

Tests

  • sbt "http-core/testOnly org.apache.pekko.http.impl.util.RenderingSpec org.apache.pekko.http.impl.engine.rendering.ResponseRendererSpec org.apache.pekko.http.impl.engine.rendering.RequestRendererSpec" — pass (80 tests). A new case in the shared rendering table asserts a header with NUL in the value is discarded. Verified it fails with the fix stashed — once for each of the four renderings.
  • sbt http-core/mimaReportBinaryIssues — pass (internal impl.util change, no public API).
  • Native scalafmt on the changed files — clean.

References

None - extends the outgoing header guard to NUL

🤖 Generated with Claude Code

Motivation:
The guard in `Rendering.~~(HttpHeader)` renders a header and then scans
the rendered bytes, discarding the header if it finds one of the
characters that must never reach the wire. It only looked for CR and
LF, so a `RawHeader` value carrying a NUL was rendered as-is. NUL is
not a legal field-value character, and a downstream consumer that
treats the value as a C string truncates it there, so two parties can
disagree about where the value ends. The HTTP/2 renderer rejects CR, LF
and NUL alike, so HTTP/1.1 was the weaker of the two.

Modification:
Add `Rendering.isIllegalHeaderChar`, which covers CR, LF and NUL, and
use it from all four `check` implementations (`StringRendering`,
`ByteArrayRendering`, `ByteStringRendering` and
`CustomCharsetByteStringRendering`) instead of repeating the character
comparison a fourth time. It takes an `Int` so the `Char` and `Byte`
based renderings can pass their element straight in.

Result:
A header whose name or value contains NUL is discarded like one
containing CR or LF, on every rendering implementation, and the rule
now lives in one place.

Tests:
- sbt "http-core/testOnly org.apache.pekko.http.impl.util.RenderingSpec org.apache.pekko.http.impl.engine.rendering.ResponseRendererSpec org.apache.pekko.http.impl.engine.rendering.RequestRendererSpec" - pass (80 tests); a new case in the shared rendering table asserts a header with NUL in the value is discarded. Verified it fails with the fix stashed, once for each of the four renderings.
- sbt http-core/mimaReportBinaryIssues - pass (internal impl.util change, no public API).

References:
None - extends the outgoing header guard to NUL
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.

1 participant