Skip to content

docs: correct 64KB hardening probe expectation - #836

Open
krypt0bi wants to merge 3 commits into
flop-labs:mainfrom
krypt0bi:fix/http-hardening-probe-64kb
Open

krypt0bi wants to merge 3 commits into
flop-labs:mainfrom
krypt0bi:fix/http-hardening-probe-64kb

Conversation

@krypt0bi

@krypt0bi krypt0bi commented Sep 12, 2026 •

Copy link
Copy Markdown

What

Rewrites the probe docstring's expected-shape table so every line matches measured behaviour at the documented flags. No caller-visible change; this is a comment in a diagnostic script.

Why

The table stated fixed codes for cases the probe can't make deterministic. Any request over --h11-max-incomplete-event-size (2000 headers, 16/64KB header values, a 24KB target) can return either code depending on read segmentation, which one sendall() doesn't control. Under-cap cases (50/200 headers, 8KB header, 12KB target) stay deterministic. Each line was checked with repeated plain and deliberately fragmented sends. The request-line row turned out to be the app's room-name validator, confirmed from the response body. Review from @Minh3132 and @yukkie3276 shaped this. Making the probe assert and exit non-zero is a separate follow-up.

Checks

  • uv run coverage run -m pytest tests -q && uv run coverage report — this file is excluded from pytest collection by design; unaffected
  • uv run ruff check . && uv run ruff format --check . and uv run ty check
  • Docs checked: README.md, src/patterns.md, mcp/README.md — no other doc states the specific 8/16/64KB breakdown
  • Nothing new — no public surface change

@Minh3132 Minh3132 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On exact head b070b9174d5ed34b6a012b8b5f6eac05f7dfc90d, this changes a measured outcome into a deterministic expectation even though the probe itself does not control the condition that decides it.

The docstring immediately below these lines correctly says --h11-max-incomplete-event-size bounds only buffered incomplete data. But send() uses one socket.sendall(raw) and never controls how that write is split into data_received() calls on the server. If the complete header block reaches h11 before it needs more data, the incomplete-event ceiling is not the thing rejecting it and the request can reach HeaderLimits, which then returns 431; if the same bytes arrive fragmented such that h11 is left holding >16 KiB of an incomplete event, h11 rejects first with 400. That is the same transport-segmentation distinction the file already calls out for large request targets.

So a single run observing 64 KiB => 400 does not justify documenting single 64/256KB header value -> 400 as the expected shape. Either describe the 64 KiB band as segmentation-dependent (with 431 when it reaches the app and 400 when h11 trips first), or make the probe deliberately send controlled fragments and label the expectation for that fragmentation pattern. Otherwise this 'correction' can still report a healthy server as violating its own documented expected shape depending only on socket packetization.

@krypt0bi

Copy link
Copy Markdown
Author

On exact head b070b9174d5ed34b6a012b8b5f6eac05f7dfc90d, this changes a measured outcome into a deterministic expectation even though the probe itself does not control the condition that decides it.

The docstring immediately below these lines correctly says --h11-max-incomplete-event-size bounds only buffered incomplete data. But send() uses one socket.sendall(raw) and never controls how that write is split into data_received() calls on the server. If the complete header block reaches h11 before it needs more data, the incomplete-event ceiling is not the thing rejecting it and the request can reach HeaderLimits, which then returns 431; if the same bytes arrive fragmented such that h11 is left holding >16 KiB of an incomplete event, h11 rejects first with 400. That is the same transport-segmentation distinction the file already calls out for large request targets.

So a single run observing 64 KiB => 400 does not justify documenting single 64/256KB header value -> 400 as the expected shape. Either describe the 64 KiB band as segmentation-dependent (with 431 when it reaches the app and 400 when h11 trips first), or make the probe deliberately send controlled fragments and label the expectation for that fragmentation pattern. Otherwise this 'correction' can still report a healthy server as violating its own documented expected shape depending only on socket packetization.

You're right, and it goes further than I expected. I ran the unfragmented case 10 times against an unchanged server and got 431, 400, 400, 431, 400, 400, 400, 431, 400, 400 at 64KB. Same request, same sendall(), three different outcomes on identical bytes, so this isn't a segmentation edge case, it's the normal case at that size. 256KB, by contrast, came back 400 consistently across 10 trials, so that line stays deterministic.

Pushed a follow-up commit splitting 64KB into its own segmentation-dependent line, phrased the same way the file already handles the 12/24KiB request-target case. Thanks for catching this.

Copy link
Copy Markdown

On current head 5656f3dc4b07f10219d49f8b37f5192b210e8036, the same segmentation caveat still applies to the documented 16 KiB header case. The probe constructs a 16 KiB value plus the request line, header name, Host, CRLFs, etc., so the incomplete HTTP event is larger than the configured --h11-max-incomplete-event-size 16384; send() still uses one uncontrolled sendall(). If h11 is left holding that request incomplete after crossing the cap it can return 400, while a complete event that reaches HeaderLimits returns 431. Please make the 16 KiB expectation segmentation-dependent too (8 KiB can remain the deterministic app-bound case), or explicitly fragment/control delivery if a fixed 16 KiB outcome is intended.

@krypt0bi

Copy link
Copy Markdown
Author

On exact head b070b9174d5ed34b6a012b8b5f6eac05f7dfc90d, this changes a measured outcome into a deterministic expectation even though the probe itself does not control the condition that decides it.

The docstring immediately below these lines correctly says --h11-max-incomplete-event-size bounds only buffered incomplete data. But send() uses one socket.sendall(raw) and never controls how that write is split into data_received() calls on the server. If the complete header block reaches h11 before it needs more data, the incomplete-event ceiling is not the thing rejecting it and the request can reach HeaderLimits, which then returns 431; if the same bytes arrive fragmented such that h11 is left holding >16 KiB of an incomplete event, h11 rejects first with 400. That is the same transport-segmentation distinction the file already calls out for large request targets.

So a single run observing 64 KiB => 400 does not justify documenting single 64/256KB header value -> 400 as the expected shape. Either describe the 64 KiB band as segmentation-dependent (with 431 when it reaches the app and 400 when h11 trips first), or make the probe deliberately send controlled fragments and label the expectation for that fragmentation pattern. Otherwise this 'correction' can still report a healthy server as violating its own documented expected shape depending only on socket packetization.

Applied the segmentation-dependent framing to every case that's actually over the --h11-max-incomplete-event-size cap, not just the ones flagged: 16KB and 64KB headers, and 2000 headers, which also crosses the cap in total and reproduces the same 400/431 split under controlled fragmentation. The existing "12/24KB target -> may reach the app" line turned out to be hiding the identical issue, 12KB is genuinely under the cap and deterministic, 24KB is over it and segmentation-dependent the same way, so I split those into two accurate lines instead of one vague one.

256KB stayed consistently 400 under both plain and deliberately fragmented delivery across repeated trials, so it keeps a single line but stated as strong empirical evidence rather than a guarantee, since "far over the cap" doesn't by itself imply determinism (64KB is proof of that).

I also discovered the request-line case (8/64KB) turned out to be a different, unrelated mechanism: it's the app's own room-name validator, not a parser length limit, confirmed by checking the actual response body (400 bad name '...', with the name echoed back) at both sizes, not just the status code.

Every line in the table now traces to either a size argument (under the cap, so the failure mode is structurally impossible) or repeated trials under both plain and forced-fragmented delivery. Appreciate the catch.

This branch has not been deployed

No deployments
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.

3 participants