Skip to content

fix: reject a message whose Transfer-Encoding value cannot be parsed - #1267

Merged
pjfanning merged 2 commits into
apache:mainfrom
pjfanning:reject-unparseable-transfer-encoding
Sep 6, 2026
Merged

fix: reject a message whose Transfer-Encoding value cannot be parsed#1267
pjfanning merged 2 commits into
apache:mainfrom
pjfanning:reject-unparseable-transfer-encoding

Conversation

@pjfanning

@pjfanning pjfanning commented Aug 31, 2026

Copy link
Copy Markdown
Member

Motivation

When a modelled header value fails to parse, ModeledHeaderValueParser calls onIllegalHeader — which by default only logs — and degrades the header to a RawHeader:

case HeaderParser.Failure(error) =>
  onIllegalHeader(error.withSummaryPrepended(s"Illegal '$headerName' header")...)
  RawHeader(headerName, trimmedHeaderValue)

For Transfer-Encoding that is unsafe. The degraded header never reaches the case h: Transfer-Encoding arm in parseHeaderLines, so isChunked stays false and — with a Content-Length also present — the message is framed by Content-Length. An upstream that does parse the value (stripping quotes, tolerating trailing junk) frames the same message as chunked. The two then disagree about where the message ends, which is a request-smuggling primitive.

Pekko HTTP already rejects the clear cases: chunked together with a Content-Length, an unsupported coding, and multiple entries. Only the unparseable value was silently tolerated.

Modification

Fail the message when a Transfer-Encoding header arrives as a RawHeader. That can only happen when the modelled parse failed: transfer-encoding is in alwaysParsedHeaders, so it is modelled even when modeled-header-parsing is off, and a well-formed value always reaches the modelled arm. The change is therefore scoped to this one header and does not alter the general illegal-header contract.

Result

A message whose Transfer-Encoding cannot be understood is rejected, rather than framed by a different rule than the sender used.

Tests

  • sbt "http-core/testOnly org.apache.pekko.http.impl.engine.parsing.*" — pass (246 tests). A new test sends Transfer-Encoding: "chunked" alongside a Content-Length and expects a 400. Verified it fails without the change, where the message is accepted and framed by Content-Length.
  • sbt http-core/mimaReportBinaryIssues — pass.
  • Native scalafmt clean.

References

None - rejects a message whose Transfer-Encoding value cannot be parsed

🤖 Generated with Claude Code

@pjfanning
fix: reject a message whose Transfer-Encoding value cannot be parsed
e5bafc5 
Motivation:
When a modelled header value fails to parse, `ModeledHeaderValueParser`
calls `onIllegalHeader` - which by default only logs - and degrades the
header to a `RawHeader`. For `Transfer-Encoding` that is unsafe: the
degraded header never reaches the `case h: Transfer-Encoding` arm in
`parseHeaderLines`, so `isChunked` stays false and, with a
Content-Length also present, the message is framed by Content-Length.
An upstream that does parse the value - stripping quotes, tolerating
trailing junk - frames the same message as chunked. The two disagree
about where the message ends, which is a request smuggling primitive.

Pekko HTTP already rejects the clear cases: `chunked` together with a
Content-Length, an unsupported coding, and multiple entries. Only the
unparseable value was silently tolerated.

Modification:
Fail the message when a `Transfer-Encoding` header arrives as a
`RawHeader`. That can only happen when the modelled parse failed:
`transfer-encoding` is in `alwaysParsedHeaders`, so it is modelled even
when `modeled-header-parsing` is off, and a well-formed value always
reaches the modelled arm.

Result:
A message whose Transfer-Encoding cannot be understood is rejected
rather than framed by a different rule than the sender used.

Tests:
- sbt "http-core/testOnly org.apache.pekko.http.impl.engine.parsing.*" - pass (246 tests); a new test sends `Transfer-Encoding: "chunked"` alongside a Content-Length and expects a 400. Verified it fails without the change, where the message is accepted and framed by Content-Length.
- sbt http-core/mimaReportBinaryIssues - pass

References:
None - rejects a message whose Transfer-Encoding value cannot be parsed
@pjfanning
Merge remote-tracking branch 'upstream/main' into pr1267-merge
ba06614 
# Conflicts:
#	http-core/src/test/scala/org/apache/pekko/http/impl/engine/parsing/RequestParserSpec.scala
@pjfanning pjfanning added this to the 2.0.0-M2 milestone Sep 6, 2026
@pjfanning
pjfanning merged commit 44b01c3 into apache:main Sep 6, 2026
6 checks passed
@pjfanning
pjfanning deleted the reject-unparseable-transfer-encoding branch September 6, 2026 11:01
pjfanning added a commit to potiuk/pekko-http that referenced this pull request Sep 9, 2026
@pjfanning
Account for the rest of the fixes merged since the pin, and fix four …
2d24669 
…citations

Motivation:
Re-pinning caught apache#1217, apache#1218 and the three response-splitting fixes,
but six more merged between `85d7243` and `6740cbd` and the document
accounts for none of them. Two matter: P1 was asserted the same way P2
was — from the §5a table, which records what each limit is meant to
bound rather than what the code does when one is reached — and three of
those bounds did not hold. Separately, four source citations no longer
point at the code they name.

Modification:
Restate P1 as a claim about enforcement and add the note recording how it
came to be stated that way: `max-chunk-count` reported the limit without
stopping the parse (apache#1220), `max-header-count` skipped the branch that
merges repeated Connection headers (apache#1255), and HTTP/2 connection-level
buffer accounting never decremented on a discarded buffer, stalling every
stream on the connection once the leak passed half the configured size
(apache#1259). Give the §9 smuggling bullet the framing decision Pekko HTTP
does own, citing apache#1267. Add the three HTTP/2 buffer bounds to §5a and to
the §15 back-map, together with the decoding limits.

Citations: `Http.scala` no longer has public `bind`/`bindAndHandle`
methods, so §5 now names `newServerAt` and the `ServerBuilder` it
returns; `FileAndResourceDirectives.scala:229-274` moved to `246-296`;
`CorsSettingsImpl.scala:64` is `63`. §2 listed `http2-tests` as the
in-model HTTP/2 module while the same table marks it out of model — the
engine is `http-core`.

Result:
Every fix merged since the pinned commit is either reflected in a claim
or deliberately not: apache#1239 (no body on an HTTP/2 HEAD response) and apache#1261
(ignoring the reserved bit of a stream identifier) are conformance fixes
that bear on no claim the document makes. Provenance is 20 documented /
31 maintainer / 0 inferred.

Tests:
Not run - docs only

References:
Refs apache#1242, apache#1220, apache#1255, apache#1259, apache#1267
pjfanning added a commit that referenced this pull request Sep 10, 2026
@potiuk @pjfanning
Add a draft threat model and wire it for discoverability (#1242)
c428fed 
* Add a draft threat model and wire it for discoverability

### Motivation
The Pekko PMC asked the ASF Security team for a draft threat model to refine,
as preparation for a Claude security scan. This is the companion to the same
change in apache/pekko.

docs/src/main/paradox/security.md already has a "Security model" section, and
it makes the most important statement in this document: applications should not
be exposed to the public internet directly, and DoS resistance is claimed only
as "pretty well under most known" attacks. But that is three sentences, and the
quantitative envelope that actually bounds untrusted input -- the parsing limits
in http-core's reference.conf -- is not connected to it anywhere.

Separately, the AGENTS.md -> SECURITY.md chain a scanning agent follows to
locate a project's model does not resolve: there is no SECURITY.md in the repo.

### Modification
- THREAT_MODEL.md: a v0 draft following the Scovetta rubric. §5a collects the
  shipped parsing limits, timeouts and CORS defaults as the model's quantitative
  spine; §8 states the six default-on properties they underwrite.
- SECURITY.md: reporting policy pointing at security@apache.org, plus the three
  points that catch most reporters.
- AGENTS.md: a Security section pointing at both.

Nothing security.md asserts has been dropped or weakened; §15 back-maps each
existing statement to the section that now carries it.

### Result
The chain AGENTS.md -> SECURITY.md -> THREAT_MODEL.md resolves on main, and
triagers have a citable document with a closed set of dispositions (§13).

§14 holds 10 questions, each stated as a proposed answer. The two worth reading
first are Q1 (where exactly is the DoS line, given security.md's deliberately
graded claim?) and Q2 (http-cors ships allowed-origins = "*" with
allow-credentials = yes, which per its own reference.conf echoes the request
Origin -- deliberate, or should the default change?).

The document is not ready to be treated as canonical until those are answered.

### Tests
Not run - docs only

### References
None - requested by the Pekko PMC chair on the ASF security scan thread

* Update THREAT_MODEL.md

* Answer Q1 and Q2, and record the defaults posture as §5b

Motivation:
The draft threat model left ten questions open in §14. Two of them were not
lookups but rulings only the PMC could make, and until they were settled a
triager had no line to apply: Q1, the boundary of the "behaves pretty well
under most known DoS attacks" claim, and Q2, the CORS defaults. Separately,
the model had no statement of the project's standing position on configuration
defaults, so every "this default should be stricter" report had to be argued
from scratch.

Modification:
- §14 Q1 answered: the DoS line is content vs. volume. A single request within
  every §5a limit that provokes super-linear CPU or memory is VALID; a finding
  needing a limit raised is OUT-OF-MODEL: non-default-build; one depending on
  request volume is BY-DESIGN: property-disclaimed. Propagated to §4, §5, §5a,
  §7, §8, §9, §11a and §13.
- New §5b, "Security posture: hardening, not secure-by-default", mirroring
  apache/pekko#3478 §5b: defaults are compatibility choices, a request to
  tighten one is a change request not a vulnerability, proposals belong on the
  development list, and an implementation that does not do what it documents is
  a defect this project fixes.
- §14 Q2 answered from §5b: the CORS pair is a compatibility default, inherited
  with the code donated by Lomig Mégard (legal/CorsNotice.txt). A request to
  change it is BY-DESIGN: default-configuration; a cors() directive that admits
  an origin its configuration should have rejected remains in scope.
- New §13 disposition BY-DESIGN: default-configuration, disambiguated from
  OUT-OF-MODEL: non-default-build (a real defect reachable only off-default,
  versus no defect at all).
- SECURITY.md states both rulings for reporters, and closes on what the project
  does want: an implementation that does not match its documentation.

Result:
Resource exhaustion and default-configuration reports now route to exactly one
§13 disposition instead of being judgment calls. Four of the ten §14 questions
are closed; the six remaining are confirm-or-correct. Provenance moves from
17 documented / 0 maintainer / 15 inferred to 17 / 6 / 13.

Tests:
Not run - docs only

References:
Refs #1242

* Answer Q4-Q8, resolve Q9 against the source, correct the client-IP claim

Motivation:
Six of the ten §14 questions were still open, and two of them rested on
statements that the source contradicts. Q4 asserted that Pekko HTTP "neither
parses nor trusts forwarding headers", which is not what extractClientIP does.
Q9 held §5's negative claims as the document's last uncited assertions, and the
equivalent section in apache/pekko#3478 turned out to be wrong on review, so
asserting rather than checking them was not good enough here either.

Modification:
- Q4 answered, and its stated basis corrected: extractClientIP resolves
  X-Forwarded-For (first address) -> X-Real-Ip -> the remoteAddress attribute
  (MiscDirectives.scala:142-145), so forwarding headers are parsed, just never
  implicitly. The spoofability is documented at the directive and answered by
  extractDirectClientIP (#1219), which reads the attribute alone. Disposition is
  unchanged: BY-DESIGN: property-disclaimed, with a defect in
  extractDirectClientIP being VALID. Propagated to §5, §6, §9, §10.5, §11, §11a.
- Q5, Q6, Q7 and Q8 answered as proposed: the verify/provideVerify split, the §2
  module in/out split, the non-goals and adversary split, and TLS cipher
  selection as a deployment property. Q8 additionally records that in-process
  termination is supported while the fronted posture is what the docs recommend.
- Q9 resolved by scanning the main sources of http-core, http, parsing,
  http-caching and http-cors: no addShutdownHook, ProcessBuilder/Runtime.exec,
  Signal/SignalHandler, file-writing API, System.setProperty/Security.*, or bind
  outside the public Http().bind* entry points. §5 now cites the scan, and
  records the inherited caveat that the ActorSystem registers shutdown hooks
  Pekko HTTP does not.

Result:
No claim in the document is inferred any more; provenance moves from
17 documented / 6 maintainer / 13 inferred to 20 / 24 / 0. Q10 (coexistence with
security.md) is the only question left open.

Tests:
Not run - docs only

References:
Refs #1242

* Answer Q9: Pekko HTTP registers no shutdown hook of its own

Motivation:
Q9 was resolved against the source but left the disposition open: §5's scan
showed Pekko HTTP adds no JVM shutdown hook, while the ActorSystem it runs on
does. Whether to state that boundary here or defer the topic to the companion
model needed a ruling, and the distinction is easy to misattribute in triage.

Modification:
- Q9 answered: highlight the ActorSystem's hooks in §5 so integrators are not
  surprised, but the claim this document makes is that Pekko HTTP registers
  none of its own.
- Promoted "Registers no JVM shutdown hook" from a trailing clause on the
  signal-handler bullet to its own §5 claim, so the load-bearing statement is
  stated directly rather than implied.
- Sharpened the §5 caveat to separate the two readings: "a Pekko HTTP process
  has no shutdown hook" is false, "Pekko HTTP registers no shutdown hook" is
  true and is what this section asserts.
- New §11a non-finding: a report that the process registers shutdown hooks is
  OUT-OF-MODEL: unsupported-component, since the hooks are CoordinatedShutdown's
  and, with remoting enabled, Artery's.

Result:
Q10 (coexistence with security.md) is the only question left open. Provenance is
20 documented / 26 maintainer / 0 inferred.

Tests:
Not run - docs only

References:
Refs #1242

* Answer Q10: three documents, each canonical for one thing

Motivation:
Q10 was the last open question: which document is canonical for what, now that
THREAT_MODEL.md, SECURITY.md and docs/src/main/paradox/security.md all carry
security information. apache/pekko#3478 settled the same question for the
companion project, and following its split keeps the two models consistent for
anyone triaging across both.

Modification:
- Q10 answered with the three-document table from apache/pekko#3478: SECURITY.md
  is canonical for the reporting policy, THREAT_MODEL.md for scope and triage,
  and docs/src/main/paradox/security.md for announcements and the docs-site
  index. Every other document links rather than restates.
- docs/src/main/paradox/security.md gains links to SECURITY.md and
  THREAT_MODEL.md, and its reporting paragraph now names security@apache.org
  rather than "our private security mailing list", which does not exist. This
  mirrors the equivalent change in apache/pekko#3478.
- Corrected the draft's own proposal, which suggested reducing the "Security
  model" section of security.md to a pointer. It is kept: §4 quotes it as the
  documented source of the "should not be exposed to the public internet
  directly" posture and §15 back-maps four claims to it, so reducing it would
  delete the evidence this model is built on.

Result:
All ten §14 questions are answered. One item is referred rather than settled, as
the ruling requires: security.md carries an upstream-coordination sentence about
sharing reports with the Lightbend Akka team that SECURITY.md does not, and a
reporting statement is promoted verbatim or dropped by maintainer decision, never
silently moved. Provenance is 20 documented / 27 maintainer / 0 inferred.

Tests:
Not run - docs only

References:
Refs #1242

* Correct two §5/§9 claims against the source and answer Q3

Motivation:
Maintainer review found two claims that do not hold on main and one
internal inconsistency:
- §5 claimed Pekko HTTP writes no files, but fileUploadAll creates temp
  files and storeUploadedFile(s) writes entity bytes to an
  application-chosen destination (FileUploadDirectives.scala:178).
- §9 and §14 Q3 described safeDirectoryChildPath as containing
  traversal, but its canonical-path check compares strings, so a
  symlink resolving into a sibling directory that shares the served
  root as a string prefix escapes it (fix in flight in #1218).
- The status line said both "Q1-Q8 answered" and "all ten answered"
  while Q3 still ended in an open question, and the pinned commit was
  the PR's own first commit rather than the main commit reviewed.

Modification:
Restate the §5 file-system claim with the upload-directive carve-out
and correct Q9 accordingly. Answer Q3 as a correction: the escape is
VALID under §5b.4 and fixed by #1218; update §9 and the §15 back-map to
match. Note in §12 that #1217 would invalidate the shutdown-hook claim
on merge. Fix the status line, the tag tally, and the commit pin
(444d939 -> 85d7243, the main commit the branch is based on).

Result:
Every §5 negative claim matches the source at the pinned commit, Q3 is
answered consistently with the "all ten answered" status, and the two
in-flight PRs that touch the model's claims (#1217, #1218) are
cross-linked.

Tests:
Not run - docs only

References:
Refs #1218, Refs #1217

* Update the §12 note on #1217: reworked to a CoordinatedShutdown task

Motivation:
§12 flagged #1217's raw JVM shutdown hook as invalidating the §5
"registers no shutdown hook" claim on merge. That PR has since been
reworked to register its temp-file cleanup as a CoordinatedShutdown
task on the actor system instead, so no §5 claim is affected.

Modification:
Restate the §12 bullet as a recorded near-miss rather than a pending
invalidation.

Result:
§12 matches the current state of #1217 and the §5/§11a claims stand.

Tests:
Not run - docs only

References:
Refs #1217

* Fold the reworked #1217 and #1218 into §5, §9 and Q3

Motivation:
Both in-flight PRs the model references changed shape after review.
#1218 gained a documented platform caveat - File.getCanonicalPath does
not resolve NTFS symbolic links or junctions on Windows, so the
link-escape class stays open there - and now rejects path segments that
no file-system path may contain instead of erroring. #1217 replaced the
per-file deleteOnExit with one temp directory per actor system removed
by a CoordinatedShutdown task. Q3's answer claimed symlink escapes are
rejected "whatever its target is named", which overclaims on Windows.

Modification:
Scope the Q3 and §9 symlink-rejection claims to platforms where
canonicalization resolves links, record the Windows residual and the
toRealPath follow-up, note the invalid-segment hardening, and update
the §5 upload-directive note to describe #1217's per-system directory
and CoordinatedShutdown cleanup.

Result:
The model's containment and file-writing claims match what #1217 and
#1218 actually implement, on every platform they address.

Tests:
Not run - docs only

References:
Refs #1217, Refs #1218

* Record the decoding limits that bound decompression bombs

Motivation:
Review of the threat model pointed out that §9 lists decompression bombs
as an attack class "left to the caller", which is not accurate: the
routing block ships two limits that bound a decoded entity, and the
decoding directives apply them without the caller doing anything.

Modification:
Add `routing.decode-max-bytes-per-chunk` and `routing.decode-max-size` to
the §5a limits table, and amend its intro and citation to cover the
second reference.conf they live in. Rewrite the §9 bullet to state what
the directives bound, cited to CodingDirectives.scala:93-102, and narrow
what remains the caller's: decoding outside the directives, and raising
decode-max-size to infinite.

Result:
§5a lists every shipped limit that bounds an entity, decompression
included, and §9 no longer disclaims a control the project provides.

Tests:
Not run - docs only

References:
Refs #1242

* Re-pin the model to the rebased base and correct P2 against the render path

Motivation:
Rebasing the branch moved its base from `85d7243` to `6740cbd`, twenty-four
commits later, so the document's "written against" pin no longer named a
commit on the branch. Several of those commits touched claims the document
makes, and one of them falsified a §8 property as stated.

P2 ("response splitting is blocked") was asserted from the two
`illegal-response-header-*-processing-mode` defaults alone, without
walking the path those settings govern. Walking it finds the guard lived
in a single `~~(HttpHeader)` overload, so chunked-response trailers and
chunk extensions rendered around it (#1256), it tested CR and LF but not
NUL (#1260), and the HTTP/2 header path had no equivalent check, so the
mitigation an application relies on under HTTP/1.1 disappeared on upgrade
(#1258). All three are fixed at the new base. #1217 and #1218, which the
document tracked as in flight, have also landed.

Modification:
Re-pin to `6740cbd`. Restate P2 as a claim about the merged enforcement
path — naming the trailer, chunk-extension, NUL and HTTP/2 gaps and the
single `Rendering.isIllegalHeaderChar` predicate that now holds the rule
— and record how it came to be stated that precisely. Update §5 and §12
for #1217 merging in its reworked `CoordinatedShutdown` form, re-checked
against the merged code: no `addShutdownHook` outside a comment saying
why one is not used. Make §5b.4's three examples concrete, two of them
having stopped being hypothetical during review. Add to the §5a CORS
ruling that it governs the default and never the enforcement, citing
#1262, where `validateOrigins` accepted a request if any origin matched
while the response echoed every origin it was given.

Result:
The pin names the commit the claims are verified against, P2 describes
what the code does rather than what two settings imply, and the CORS
section distinguishes a permissive default from a restrictive setting
that fails to restrict.

Tests:
Not run - docs only

References:
Refs #1242, #1256, #1258, #1260, #1262, #1217, #1218

* Account for the rest of the fixes merged since the pin, and fix four citations

Motivation:
Re-pinning caught #1217, #1218 and the three response-splitting fixes,
but six more merged between `85d7243` and `6740cbd` and the document
accounts for none of them. Two matter: P1 was asserted the same way P2
was — from the §5a table, which records what each limit is meant to
bound rather than what the code does when one is reached — and three of
those bounds did not hold. Separately, four source citations no longer
point at the code they name.

Modification:
Restate P1 as a claim about enforcement and add the note recording how it
came to be stated that way: `max-chunk-count` reported the limit without
stopping the parse (#1220), `max-header-count` skipped the branch that
merges repeated Connection headers (#1255), and HTTP/2 connection-level
buffer accounting never decremented on a discarded buffer, stalling every
stream on the connection once the leak passed half the configured size
(#1259). Give the §9 smuggling bullet the framing decision Pekko HTTP
does own, citing #1267. Add the three HTTP/2 buffer bounds to §5a and to
the §15 back-map, together with the decoding limits.

Citations: `Http.scala` no longer has public `bind`/`bindAndHandle`
methods, so §5 now names `newServerAt` and the `ServerBuilder` it
returns; `FileAndResourceDirectives.scala:229-274` moved to `246-296`;
`CorsSettingsImpl.scala:64` is `63`. §2 listed `http2-tests` as the
in-model HTTP/2 module while the same table marks it out of model — the
engine is `http-core`.

Result:
Every fix merged since the pinned commit is either reflected in a claim
or deliberately not: #1239 (no body on an HTTP/2 HEAD response) and #1261
(ignoring the reserved bit of a stream identifier) are conformance fixes
that bear on no claim the document makes. Provenance is 20 documented /
31 maintainer / 0 inferred.

Tests:
Not run - docs only

References:
Refs #1242, #1220, #1255, #1259, #1267

* Close three gaps the open PRs exposed in the model

Motivation:
Reviewing the open PRs from the last three weeks against the document
turned up three claims that are wrong or missing today, independently of
whether any of those PRs merge.

Modification:
P1 claims inbound messages are bounded by the §5a limits, and for HTTP/2
frames nothing bounds them: the frame parser reads the 24-bit length
field and takes that many bytes, up to 16 MiB - 1, before HPACK decoding
and before the entity is assembled, so none of the three HTTP/2 buffer
bounds apply. State the gap under P1, name #1264 as the fix in flight,
and rule an oversized-frame report VALID against P1 rather than a §12
model gap in the meantime.

The `frame-type-throttle` block was absent from the whole document, even
though it ships on by default charging RST_STREAM at 100/s and is the
project's HTTP/2 Rapid Reset mitigation (CVE-2023-44487). Add it to §5a
and to the §15 back-map, and restate P6: `max-concurrent-streams` alone
bounds concurrency and not work, because a reset frees the slot at once.

§9 claimed traversal containment for "file-serving directives" without
qualification. Three tiers, checked against the source: `getFromDirectory`
and `listDirectoryContents` (and the browseable pair built on them) get
`safeDirectoryChildPath`, segment filter plus canonical containment;
`getFromResourceDirectory` gets `safeJoinPaths` alone, coherent since a
class-loader resource name has no canonical form; `getFromFile` and
`getFromResource` get neither. Scope §14 Q3's answer to the first tier
and name `getFromResource` in the §11 misuse list, where only
`getFromFile` was listed.

Result:
The containment claim matches what each directive implements, the Rapid
Reset mitigation is claimed where a reader of §8 will find it, and the
one bound P1 asserts but does not yet have is written down. Provenance is
20 documented / 32 maintainer / 0 inferred.

Tests:
Not run - docs only

References:
Refs #1242, #1264, #1263

---------

Co-authored-by: PJ Fanning <pjfanning@users.noreply.github.com>
@pjfanning pjfanning mentioned this pull request Sep 10, 2026
Merged
pjfanning added a commit that referenced this pull request Sep 10, 2026
@pjfanning
fix: reject a message whose Transfer-Encoding value cannot be parsed (#…
202e1d2 
…1267) (#1291)

Motivation:
When a modelled header value fails to parse, `ModeledHeaderValueParser`
calls `onIllegalHeader` - which by default only logs - and degrades the
header to a `RawHeader`. For `Transfer-Encoding` that is unsafe: the
degraded header never reaches the `case h: Transfer-Encoding` arm in
`parseHeaderLines`, so `isChunked` stays false and, with a
Content-Length also present, the message is framed by Content-Length.
An upstream that does parse the value - stripping quotes, tolerating
trailing junk - frames the same message as chunked. The two disagree
about where the message ends, which is a request smuggling primitive.

Pekko HTTP already rejects the clear cases: `chunked` together with a
Content-Length, an unsupported coding, and multiple entries. Only the
unparseable value was silently tolerated.

Modification:
Fail the message when a `Transfer-Encoding` header arrives as a
`RawHeader`. That can only happen when the modelled parse failed:
`transfer-encoding` is in `alwaysParsedHeaders`, so it is modelled even
when `modeled-header-parsing` is off, and a well-formed value always
reaches the modelled arm.

Result:
A message whose Transfer-Encoding cannot be understood is rejected
rather than framed by a different rule than the sender used.

Tests:
- sbt "http-core/testOnly org.apache.pekko.http.impl.engine.parsing.*" - pass (246 tests); a new test sends `Transfer-Encoding: "chunked"` alongside a Content-Length and expects a 400. Verified it fails without the change, where the message is accepted and framed by Content-Length.
- sbt http-core/mimaReportBinaryIssues - pass

References:
None - rejects a message whose Transfer-Encoding value cannot be parsed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nvollmar nvollmar nvollmar approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.0.0-M2

Development

Successfully merging this pull request may close these issues.

2 participants

@pjfanning @nvollmar