Conversation
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
…es to the PR Motivation: The document had grown to ten thousand words, and the growth was not in its claims but in narrative about how each claim came to be stated: four "how P-n came to be stated" essays under the §8 table, §14 answers that re-argued the reasoning the owning section had already absorbed, §9 bullets that explained each fixed defect's mechanism, a Confidence paragraph recounting which questions turned out to be corrections, and the same fixes listed in three or four places each. Its readers are triagers and the report-generating tools that cite it; they need the properties, the limits, the dispositions and the non-findings, and they need them fast. The review history is real but belongs in the pull request, not in the artefact reporters read. Modification: Cut to 6,500 words with no claim, ruling, disposition, table row or default removed. The four §8 notes become one verification paragraph naming each defect and its PR in a clause; §14 is retitled "Maintainer rulings" and each answer reduced to its ruling, with the reasoning kept only where it is the ruling (Q3's three tiers); §9's essays are cut to the boundary they draw plus one PR reference per fixed case; the Confidence paragraph is replaced by one sentence; the §5b.4 tally, the §12 near-miss narrative and the §5 shutdown-hook caveat are each reduced to a sentence; line-number citations are dropped in favour of file or symbol names, since the pin fixes the commit and the numbers rot on every merge. Every PR a claim depends on is still referenced, once. Result: Same model, 36% shorter, with §14 no longer presenting settled rulings as open questions. Tests: Not run - docs only References: Refs apache#1295
pjfanning
force-pushed
the
threat-model/post-merge-updates
branch
from
September 11, 2026 15:09
7b4bc71 to
956c984
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
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
Two things, in two commits so they can be reviewed separately.
Content. Nineteen commits landed on
mainafter the model was pinned to6740cbd, ten of which touch a claim it makes. One changes a triage line: #1194 documentsidle-timeoutas 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 alone (#1252, #1297).Length. The document had grown to ~10,000 words, and the growth was narrative about how each claim came to be stated rather than the claims themselves. Its readers are triagers and the report-generating tools that cite it; they need the properties, limits, dispositions and non-findings, fast. The review history is real but belongs here, in the PR — which is where this description puts it.
Modification
Commit 1 — re-pin to
40b07a2and absorb what merged:400on its own stream and the connection carries on (RequestErrorFlow, since #59). Two paths around it were found and closed during review — #1252 and #1297, belowreference.confandtimeouts.md(#1194). A trickling client isBY-DESIGN: property-disclaimedwith a documented citation; the in-scope shape is a connection with no traffic outliving the timeout, or a bound whose cleanup leaks (#1284)max-part-count = 10000,error-logging-verbosity = fullIllegalRequestContext.rawRequestTargetrowverbose-error-messages = offgoverns the client, not the log — what reaches the log iserror-logging-verbosityerror-logging-verbosity = simplewhere logs are shipped or alerted onRaw-Request-URIfrom request input (#1280 extended it to HTTP/2:path); echoingrawRequestTargetunescaped from a customParsingErrorHandlerCommit 2 — trim to 6,500 words, no claim removed:
The review history this document no longer carries
For the record — the model's claims were checked against the code as it was written, and the check kept finding defects. All are fixed at or before the pinned commit unless noted.
P1 — inbound messages are bounded. Asserted from the §5a table, then checked:
max-chunk-counthad noelsebranch, sofailEntityStream's result was discarded and the parser trampolined into the next chunk — the limit emitted an error per remaining chunk instead of stopping (Stop parsing chunks once the chunk count limit is reached #1220).max-header-countwas not incremented on the branch merging repeatedConnectionheaders, so that header could be repeated without limit (fix: count repeated Connection headers towards max-header-count #1255).totalBufferedDatawas never decremented when a buffered stream was discarded onRST_STREAM, downstream cancellation, or a stream-levelFLOW_CONTROL_ERROR; once the leak passed half ofincoming-connection-level-buffer-sizethe connection window drained to zero and every stream stalled (fix: release connection-level flow control accounting for discarded buffered data #1259, fix: release buffered data accounting when an incoming HTTP/2 stream is shut down #1281).max-content-lengthpacked with minimal parts ran to well over a hundred thousand of them (feat: bound the number of parts in a multipart entity #1266).Http2FrameParsingtakes a frame of any declared length, up to 16 MiB − 1, before any §5a bound applies. feat: bound the size of an incoming HTTP/2 frame #1264 addsmax-frame-size. The document states this gap and rules an oversized-frame reportVALID.P2 — response splitting is blocked. Asserted from the two
illegal-response-header-*-processing-modedefaults, then checked: the guard lived in one~~(HttpHeader)overload, so chunked-response trailers and chunk extensions rendered around it (#1256); it tested CR and LF but not NUL (#1260); the HTTP/2 header path had no equivalent check, so a mitigation relied on under HTTP/1.1 disappeared on upgrade (#1258). The rule now lives in one predicate,Rendering.isIllegalHeaderChar.P9 — HTTP/2 stream isolation. A header that failed to parse unwound out of the HPACK decoder before the entry reached the dynamic table and before the rest of the block was read, desynchronising the table from the peer's encoder for every later
HEADERSframe on the connection; an unknown method was enough (#1252, merged through #1251). A field the HTTP/1.1 header parser rejects — NUL in a value, an illegal character in a name, a value overmax-header-value-length— was reported with that parser's internal exception type, which nothing on the HTTP/2 side caught, failing the connection instead of the stream; the same change closed the RFC 9113 §8.2.1 gap of a CR LF value being accepted silently truncated (#1297).Q3 — path traversal. The draft asserted containment held.
checkIsSafeDescendantcompared canonical paths as strings, so a symlink resolving into/var/www-privatepassed for a root of/var/www(#1218). Reviewing the other directives found three tiers, not one:safeDirectoryChildPath(segment filter + containment) for the directory-serving directives;safeJoinPathsonly forgetFromResourceDirectory; nothing forgetFromFileandgetFromResource.Q4, Q5, Q9 — corrections to the draft's facts. Q4: the draft said Pekko HTTP does not parse forwarding headers;
extractClientIPdoes, and the point is that it never does so implicitly. Q5: the draft had constant-time comparison backwards;Credentials.verifydoes usesecure_==. Q9: the draft said Pekko HTTP writes no files; the upload directives do. The dispositions survived in all three; the stated basis did not.CORS enforcement.
validateOriginsaccepted a request if any origin matched while the response echoed every origin it was given (#1262). The document now distinguishes the permissive default (BY-DESIGN) from a restrictive setting that fails to restrict (VALID).#1217 near-miss. Originally registered a JVM shutdown hook from Pekko HTTP itself, which would have falsified §5's "registers no shutdown hook"; reworked to a
CoordinatedShutdowntask before merging.#1246 and the error path. Checking whether the default
ParsingErrorHandlerlogs the newrawRequestTarget(it does not) surfaced thaterror-logging-verbosity = fullhas always written the failing request target into the log at warning level viaUriParser.fail's error line — pre-existing, undocumented in the model, and the reason §5a, §9 and §10.9 gained their entries. #1294 escapes control characters in that line; once it lands the §9 false-friend narrows from "injection and flooding" to flooding.Result
Every claim is verified against
40b07a2. Provenance is 23 documented / 33 maintainer / 0 inferred at the content commit; the trim removes tags only on sentences that go with them.Draft until #1264 and #1294 land — each is a one-line edit to a claim this touches, and the model should not merge a week stale.
Tests
Not run - docs only
References
Refs #1194, #1246, #1251, #1252, #1257, #1266, #1279, #1280, #1281, #1284, #1297