Conversation
pjfanning
added a commit
to potiuk/pekko-http
that referenced
this pull request
Sep 9, 2026
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 apache#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 apache#1242, apache#1264, apache#1263
pjfanning
requested review from
He-Pin,
Philippus,
jrudolph,
nvollmar,
raboof and
samueleresca
September 9, 2026 22:36
pjfanning
added a commit
that referenced
this pull request
Sep 10, 2026
* 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
added a commit
to pjfanning/incubator-pekko-http
that referenced
this pull request
Sep 11, 2026
Motivation: Nineteen commits landed on main after the model was pinned to `6740cbd`, ten of which touch a claim it makes. One changes a triage line: apache#1194 documents `idle-timeout` as a bidirectional inactivity timeout that a client sending bytes inside every window keeps alive by design, where §9 called such a connection an evasion and therefore in scope. Two others defend a property the model never stated: that a malformed request on one HTTP/2 stream is answered on that stream and leaves the connection, its HPACK state and its other streams alone (apache#1252, apache#1297). Checking the error path for apache#1246 also surfaced a shipped default the model never mentioned: `error-logging-verbosity = full` writes the failing request target into the log at warning level, independent of the client-facing `verbose-error-messages = off` that P3 rests on. Modification: Re-pin to `40b07a2`. Add P9, HTTP/2 stream isolation, cited to `RequestErrorFlow`, with a note recording the two paths around it that review found and closed: a header that failed to parse unwound out of the HPACK decoder before the dynamic table was updated, desynchronising every later HEADERS frame on the connection (apache#1252, merged through apache#1251); and a field the HTTP/1.1 header parser rejects was reported with an exception type nothing on the HTTP/2 side caught, failing the connection (apache#1297, which also closed the RFC 9113 8.2.1 gap of a CR LF value being accepted silently truncated). Restate P5 and the §9 slow-loris bullet around inactivity rather than evasion, citing the new `reference.conf` and `timeouts.md` wording, and record apache#1284's leak of one scheduled task per open request as the in-scope shape. Add apache#1257 to the §9 smuggling bullet beside apache#1267, and apache#1281 to the P1 note beside apache#1259 as the third discard path. Add `max-part-count` (apache#1266) to §5a and a multipart row to §6, which had no multipart input at all, noting apache#1279's header-state bleed between parts. Add `error-logging-verbosity` to §5a, a §9 false-friend entry explaining what `verbose-error-messages` does not govern, and §10.9 recommending `simple` where logs are shipped or alerted on. Add two §11 misuse patterns: building `Raw-Request-URI` from request input, and echoing `IllegalRequestContext.rawRequestTarget` unescaped. Turn §5b.4's two concrete examples into a running tally of the defects review has found of that shape. Extend §15 to match. Result: Every claim is verified against `40b07a2`. Provenance is 23 documented / 33 maintainer / 0 inferred. The P1 frame-size gap stays open; apache#1264 has not merged. Tests: Not run - docs only References: Refs apache#1194, apache#1246, apache#1251, apache#1252, apache#1257, apache#1266, apache#1279, apache#1280, apache#1281, apache#1284, apache#1297
Member
Author
|
This is the highest priority of the remaining PRs for 2.0.0-M2. It is a security hardening fix but introduces a limit that may affect some users. |
Member
|
I will take a review on this |
He-Pin
requested changes
Sep 12, 2026
He-Pin
left a comment
He-Pin
left a comment
Member
There was a problem hiding this comment.
The bound itself is correct. ByteReader.readShortBE() and readByte() both return masked Ints (pekko/stream/.../ByteStringParser.scala:242-247,216-222), so length is always a valid non-negative 24-bit value and the check cannot be bypassed by a large declared length; take(n) throws NeedMoreData and ParsingLogic keeps buffering, so the unbounded 16 MiB buffering it addresses is real. IllegalHttp2FrameSize maps to ErrorCode.FRAME_SIZE_ERROR and closes the connection, matching RFC 9113 §4.2, and not advertising SETTINGS_MAX_FRAME_SIZE is consistent with Http2Demux, which advertises only MAX_CONCURRENT_STREAMS / MAX_HEADER_LIST_SIZE / ENABLE_PUSH — so no spec-compliant peer is affected.
Two things block it: a second oversized-frame test that landed on main after this branch, and two missing MiMa filters. Details inline.
| new WaitingForRequestData { | ||
| // the single frame below is deliberately bigger than the stream-level buffer, and so also bigger than the | ||
| // default max-frame-size; raise that limit so the frame reaches the flow-control check this test is about | ||
| override def settings: ServerSettings = super.settings.mapHttp2Settings(_.withMaxFrameSize(1024 * 1024)) |
Member
There was a problem hiding this comment.
One more test needs this same override: "release connection-level flow control accounting when a stream-level window is exceeded", a few tests below this one. It sends the identical single 512001-byte DataFrame and only overrides withIncomingConnectionLevelBufferSize, so with max-frame-size = 512kB (HOCON kB = 1000, i.e. 512,000) the frame is now rejected here with GOAWAY(FRAME_SIZE_ERROR) instead of reaching the flow-control check, and expectRST_STREAM(TheStreamId, FLOW_CONTROL_ERROR) plus the window assertion below it both fail.
#1281 added that test on Sep 11, eleven days after these commits, and the PR still reports MERGEABLE since the hunks do not overlap — so it only breaks after merge. The PR body's "it was the only test in the suite that needed adjusting" needs updating as well.
Member
Author
There was a problem hiding this comment.
Good catch, thanks. Rebased onto main and added the same withMaxFrameSize(1024 * 1024) override to that test in 7d5398e; both instances of it pass now. PR body updated too.
| ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.pekko.http.javadsl.settings.Http2ClientSettings.maxFrameSize") | ||
| ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.pekko.http.javadsl.settings.Http2ServerSettings.withMaxFrameSize") | ||
| ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.pekko.http.scaladsl.settings.Http2ClientSettings.maxFrameSize") | ||
| ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.pekko.http.scaladsl.settings.Http2ServerSettings.maxFrameSize") |
Member
There was a problem hiding this comment.
Two filters short of the precedent for this exact change shape. http2-max-header-list-size.excludes (#1216 — same structure: a new abstract javadsl/scaladsl member pair plus their concrete companions) carries six, and two of them cover concrete javadsl members that this file omits:
ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.pekko.http.javadsl.settings.Http2ClientSettings.withMaxFrameSize")
ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.pekko.http.javadsl.settings.Http2ServerSettings.getMaxFrameSize")
MiMa reports ReversedMissingMethodProblem for additions to traits even when they have a body, which is why #1216 filtered withMaxHeaderListSize and getMaxHeaderListSize despite both being concrete. The PR body records only sbt http-core/mimaReportBinaryIssues, not the cross-build +mimaReportBinaryIssues that CI runs — please re-run that after adding these.
Member
Author
There was a problem hiding this comment.
Added both, and re-ran sbt "+http-core/mimaReportBinaryIssues" — clean on 2.13.18 and 3.3.8. The rebase also pulled in the 1.0.0 comparison from mima-1.2.0-additional-checks.excludes, which flagged five more problems from this PR (the replaced internal Http2FrameParsing constructor, the abstract Http2CommonSettings.maxFrameSize, and three copy$default$N shifts from the new case-class field). Those are filtered in the same file, same shape as the maxHeaderListSize ones.
| # | ||
| # RFC 9113, section 4.2 constrains this to be between 16 KiB and 16 MiB - 1. The amount of request data | ||
| # buffered overall is bounded separately by the incoming-connection-level-buffer-size and | ||
| # incoming-stream-level-buffer-size settings above. |
Member
There was a problem hiding this comment.
Nit: incoming-connection-level-buffer-size and incoming-stream-level-buffer-size are defined below this block, not above — max-frame-size sits right after max-header-list-size. Same wording in the client block further down.
Motivation: `Http2FrameParsing` read a frame's 24-bit length field and then took that many bytes with no upper bound, so a peer could declare a length up to the field's maximum of 16 MiB - 1 and make the frame parser buffer that much for a single frame. That happens before the HPACK and entity limits apply, so `max-header-list-size`, `incoming-stream-level-buffer-size` and `incoming-connection-level- buffer-size` do not bound it, and it is multiplied by the number of connections. Modification: Add a `max-frame-size` setting to the HTTP/2 server and client settings, defaulting to 512kB, and reject a larger frame with a FRAME_SIZE_ERROR on its frame header, before the payload is buffered. The value is validated against the bounds RFC 9113, section 4.2 sets for SETTINGS_MAX_FRAME_SIZE, 16 KiB to 16 MiB - 1. The setting is a limit on what is accepted, not an advertisement. An earlier revision did advertise it as SETTINGS_MAX_FRAME_SIZE, which h2spec's "4.2 Frame Size" case showed to be actively harmful: DATA frames are flow controlled and the initial window is 64 KiB, so a peer that sizes its first frame to a larger advertised value trips a FLOW_CONTROL_ERROR before the window has grown. Advertising nothing keeps a well-behaved peer at the 16 KiB default it already assumes, and the configured value is then pure leniency for peers that exceed it - always accepting at least as much as any peer is told it may send. Result: A single frame can no longer make the parser hold up to 16 MiB; the bound is 512kB by default and configurable. Frames from a spec compliant peer are unaffected, and pekko's own existing behaviour of accepting frames well above 16 KiB is preserved. Tests: - sbt "http2-tests/testOnly org.apache.pekko.http.impl.engine.http2.Http2ServerSpec org.apache.pekko.http.impl.engine.http2.Http2ClientSpec org.apache.pekko.http.impl.engine.http2.H2SpecIntegrationSpec" - pass (236 tests), including the h2spec conformance suite. A new test sends a frame one byte over a configured 16 KiB limit and expects GOAWAY(FRAME_SIZE_ERROR); verified it fails with the size check disabled. The existing "fail if more data is received than stream-level window allows" test deliberately sends a single 512001 byte frame to exceed the stream buffer, so it now raises max-frame-size to 1 MiB to reach the flow-control check it is about. - sbt http-core/mimaReportBinaryIssues - pass - sbt "+http-core/compile" - pass on 2.13.18 and 3.3.8 References: None - bounds how much a single HTTP/2 frame can buffer
The new abstract members on the HTTP/2 settings traits are only flagged on Scala 3; the 2.13 check filters them under a broader rule, so a scoped run on the default Scala version alone did not surface them.
The "release connection-level flow control accounting when a stream-level window is exceeded" test that apache#1281 added sends the same single 512001-byte DATA frame, so it also needs max-frame-size raised to reach the flow-control check it is about. MiMa filters now cover the concrete javadsl withMaxFrameSize and getMaxFrameSize members, which MiMa still reports as ReversedMissingMethodProblem, plus the problems the 2.13 check against 1.0.0 surfaces after rebasing onto main: the replaced internal Http2FrameParsing constructor, the abstract Http2CommonSettings member, and the copy$default$N shifts from the new case-class field. The reference.conf comment pointed at the buffer-size settings as being above max-frame-size; they are below it.
pjfanning
force-pushed
the
bound-http2-inbound-frame-size
branch
from
September 12, 2026 10:41
7749fec to
7d5398e
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Motivation
Http2FrameParsingread a frame's 24-bit length field and then took that many bytes with no upper bound:So a peer could declare a length up to the field's maximum of 16 MiB - 1 and make the frame parser buffer that much for a single frame. This happens before the HPACK and entity limits apply, so
max-header-list-size,incoming-stream-level-buffer-sizeandincoming-connection-level-buffer-sizedo not bound it — and it is multiplied by the number of connections.Modification
Add a
max-frame-sizesetting to the HTTP/2 server and client settings, defaulting to512kB, and reject a larger frame with aFRAME_SIZE_ERRORon its frame header, before the payload is buffered. The value is validated against the bounds RFC 9113 §4.2 sets forSETTINGS_MAX_FRAME_SIZE(16 KiB … 16 MiB - 1).The setting is a limit on what is accepted, not an advertisement, and that is deliberate. An earlier revision of this branch did advertise it as
SETTINGS_MAX_FRAME_SIZE, and h2spec's "4.2 Frame Size" case showed that to be actively harmful: DATA frames are flow controlled and the initial window is 64 KiB, so a peer that sizes its first frame to a larger advertised value trips aFLOW_CONTROL_ERRORbefore the window has grown. h2spec did exactly that and the conformance test went red.Advertising nothing keeps a well-behaved peer at the 16 KiB default it already assumes, and the configured value is then pure leniency for peers that exceed it — we always accept at least as much as any peer is told it may send. That also preserves pekko's existing behaviour of accepting frames well above 16 KiB, which its own tests rely on.
Why 512 kB and not the RFC ceiling: defaulting to 16 MiB would leave the default posture exactly as it is today and fix nothing unless an operator tuned it. 512 kB matches
incoming-stream-level-buffer-size, so the parser's transient buffering stays the same order as the stream buffer instead of 32× larger, while still allowing 32× the protocol default.Result
A single frame can no longer make the parser hold up to 16 MiB; the bound is 512 kB by default and configurable. Frames from a spec-compliant peer are unaffected.
Tests
sbt "http2-tests/testOnly ...Http2ServerSpec ...Http2ClientSpec ...H2SpecIntegrationSpec"— pass (251 tests), including the h2spec conformance suite. A new test sends a frame one byte over a configured 16 KiB limit and expectsGOAWAY(FRAME_SIZE_ERROR); verified it fails with the size check disabled.max-frame-sizeto 1 MiB in order to reach the flow-control check they are actually about.sbt "+http-core/mimaReportBinaryIssues"— pass on 2.13.18 and 3.3.8. The filters cover the new settings members (including the concrete javadslwithMaxFrameSize/getMaxFrameSize, which MiMa still reports asReversedMissingMethodProblem), the replaced internalHttp2FrameParsingconstructor, and thecopy$default$Nshifts from the new private case-class field.sbt "+http-core/compile"— pass on 2.13.18 and 3.3.8.scalafmton the changed files — clean.References
None - bounds how much a single HTTP/2 frame can buffer
🤖 Generated with Claude Code