SUPP-53: buffer url-encoded request bodies so the security filter cannot drain them

claude commented

2026-09-14 19:59:11 +00:00

Member

Closes #53.

Summary

Pac4jHttpSecurity drained url-encoded request bodies before the route could read
them, so an authenticated form POST could reach its handler with an empty form and
still answer 200. Nothing failed and nothing was logged — the loss was silent, and
intermittent, because it depended on how the server framed the body on the wire.

The security filter needs to read form parameters, and http4s request bodies are
single-read streams. The fix buffers the body once, before the security filter runs,
so both the filter and the route read the same bytes.

Defect fixed

Phase 1 — Security filter drains the request body before the route reads it
(merged via #54).

New UrlEncodedBodyBuffer middleware buffers a url-encoded request body into memory
and rebuilds the request around it, up to a configured cap.
The rebuilt request carries framing headers describing the buffered body:
Content-Length is set to the measured size and chunked is dropped, so the
headers no longer describe the original stream.
Pac4jHttpSecurity.secure wires the middleware in ahead of the security filter.
Pac4jSecurityConfig gains a trailing maxFormBodySize: Long, read from
security.maxformbodysize, default 2 MiB.

Only application/x-www-form-urlencoded requests are buffered; every other content
type passes through untouched.

Two consequences worth reading before upgrading

Pac4jSecurityConfig has a new trailing field. Positional destructuring or
unapply over it will no longer compile. Named construction and config-file
loading are unaffected.
A body over the cap is rejected with a bare 413, no body. htmx will not swap
such a response, so the user sees nothing happen. Applications that accept large
forms should raise the cap or handle the 413 themselves.

Testing

19 regression tests added — 17 in UrlEncodedBodyBufferSpec, 2 in
Pac4jSecurityConfigSpec. The server/http module goes from 52 to 71 tests.

There was no HTTP-level test fixture in server/http/src/test before this work; the
harness (a single-read Ref-backed body stream, probe routes, a pac4j
AnonymousClient config) was written from scratch and is reusable.

Every test was proved fallible by mutation: removing the fix wiring fails the four
reproduction tests while the four guards keep passing, with the headline test failing
as form = Map() at status 200 rather than as an exception.

Verified locally: ./mill __.test green (pre-push hook, whole repo), ./mill http.checkFormat clean, 0 compiler warnings.

Also in this branch

54896fae points .iw/config.conf at the Forgejo tracker (it still said github).
Unrelated to the defect, but the workflow tooling needed it to read this issue. Say
the word and I will pull it out into its own change.

Follow-ups

Filed to the tracker rather than fixed here: #55, #56, #57, #58.

Release notes

project-management/issues/SUPP-53/release-notes.md (Czech), written for 0.1.24.

Delivery

This PR targets release/0.1.x — the consumer pins 0.1.23, so the fix ships as
0.1.24 on the maintenance line. A forward-port to main follows, as with #45/#46.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BDfb2ghkkKiwTKVrcufmaT

Closes #53. ## Summary `Pac4jHttpSecurity` drained url-encoded request bodies before the route could read them, so an authenticated form POST could reach its handler with an empty form and still answer 200. Nothing failed and nothing was logged — the loss was silent, and intermittent, because it depended on how the server framed the body on the wire. The security filter needs to read form parameters, and http4s request bodies are single-read streams. The fix buffers the body once, before the security filter runs, so both the filter and the route read the same bytes. ## Defect fixed **Phase 1 — Security filter drains the request body before the route reads it** (merged via #54). - New `UrlEncodedBodyBuffer` middleware buffers a url-encoded request body into memory and rebuilds the request around it, up to a configured cap. - The rebuilt request carries framing headers describing the buffered body: `Content-Length` is set to the measured size and `chunked` is dropped, so the headers no longer describe the original stream. - `Pac4jHttpSecurity.secure` wires the middleware in ahead of the security filter. - `Pac4jSecurityConfig` gains a trailing `maxFormBodySize: Long`, read from `security.maxformbodysize`, default 2 MiB. Only `application/x-www-form-urlencoded` requests are buffered; every other content type passes through untouched. ## Two consequences worth reading before upgrading - `Pac4jSecurityConfig` has a **new trailing field**. Positional destructuring or `unapply` over it will no longer compile. Named construction and config-file loading are unaffected. - A body over the cap is rejected with a **bare `413`, no body**. htmx will not swap such a response, so the user sees nothing happen. Applications that accept large forms should raise the cap or handle the 413 themselves. ## Testing 19 regression tests added — 17 in `UrlEncodedBodyBufferSpec`, 2 in `Pac4jSecurityConfigSpec`. The `server/http` module goes from 52 to 71 tests. There was no HTTP-level test fixture in `server/http/src/test` before this work; the harness (a single-read `Ref`-backed body stream, probe routes, a pac4j `AnonymousClient` config) was written from scratch and is reusable. Every test was proved fallible by mutation: removing the fix wiring fails the four reproduction tests while the four guards keep passing, with the headline test failing as `form = Map()` at status 200 rather than as an exception. Verified locally: `./mill __.test` green (pre-push hook, whole repo), `./mill http.checkFormat` clean, 0 compiler warnings. ## Also in this branch `54896fae` points `.iw/config.conf` at the Forgejo tracker (it still said `github`). Unrelated to the defect, but the workflow tooling needed it to read this issue. Say the word and I will pull it out into its own change. ## Follow-ups Filed to the tracker rather than fixed here: #55, #56, #57, #58. ## Release notes `project-management/issues/SUPP-53/release-notes.md` (Czech), written for **0.1.24**. ## Delivery This PR targets `release/0.1.x` — the consumer pins 0.1.23, so the fix ships as **0.1.24** on the maintenance line. A forward-port to `main` follows, as with #45/#46. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01BDfb2ghkkKiwTKVrcufmaT

claude added 19 commits

2026-09-14 19:59:11 +00:00

workflow(dx): Create diagnostic analysis for SUPP-53 05c61005a4

Pac4jHttpSecurity: the security filter consumes url-encoded request
bodies, so a route can receive an empty form

Artifacts:
- analysis.md
- FOLLOW-UPS.md

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BDfb2ghkkKiwTKVrcufmaT

workflow(dx): Record analysis-ready review state for SUPP-53 053e4c5f15

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BDfb2ghkkKiwTKVrcufmaT

workflow(dx): Resolve SUPP-53 clarifications 6db612c45e

Michal's decisions:
- maxFormBodySize is a defaulted trailing field; this one
  backward-compatibility measure is approved
- The 413 carries no entity; presentation is the consumer's

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BDfb2ghkkKiwTKVrcufmaT

workflow(dx): Create task index for SUPP-53 e3fd24132d

Pac4jHttpSecurity: the security filter consumes url-encoded request
bodies, so a route can receive an empty form

Artifacts:
- tasks.md
- review-state.json (tasks_ready)

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BDfb2ghkkKiwTKVrcufmaT

Phase 1: context ready 3f952afcd2

Issue: SUPP-53
Phase: 1 - Security filter drains the request body before the route reads it

This commit marks the checkpoint for phase 1 context generation.
Use dx-fix SUPP-53 to return to this point.

The context review also found a narrower claim than the source supports:
a route with no client list is not exempt. SecurityFilterMiddleware passes
clients.orNull and DefaultSecurityClientFinder resolves a null list from the
pac4j Clients before the blank check, so Pac4jModuleRegistry's own default
of None is exposed. analysis.md and FOLLOW-UPS.md are corrected to agree.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BDfb2ghkkKiwTKVrcufmaT

chore(SUPP-53): update review-state to context_ready 9551492ce5

fix(iw): point the project config at the Forgejo tracker 54896fae3d

The repository has no GitLab or GitHub remote for code: origin is Forgejo
at code.iterative.works. `iw project-context` read `type = github` and
reported GitLab with glab instructions, which no command here can follow.

Set `type = forgejo` and add `tracker.baseUrl`, so project-context reports
the Forgejo instructions and `iw phase-pr` / `iw phase-merge` can reach the
API directly.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BDfb2ghkkKiwTKVrcufmaT

workflow(dx): Generate Phase 1 tasks for SUPP-53 e89e65f225

Security filter drains the request body before the route reads it

14 tasks in the sequencing of the phase context: reproduce with the
singleReadBody fixture and Test 1, then the trailing maxFormBodySize
config field, then UrlEncodedBodyBuffer and the .andThen wiring, then
the guards, the size-limit cases and the full server/http run.

The root cause is confirmed in source, so the investigation task is a
short confirmation that Test 1 fails as an empty form with status 200
rather than an open-ended hypothesis hunt.

Artifacts:
- phase-01-tasks.md

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BDfb2ghkkKiwTKVrcufmaT

chore(SUPP-53): update review-state to tasks_ready 100b60f48d

chore(SUPP-53): update review-state for phase 01 dbc10762d8

Security filter drains the request body before the route reads it 387783cc95

- Buffer url-encoded bodies into a strict chunk ahead of the pac4j filter, so a single-read body reaches both the filter and the route intact
- Reject an oversized url-encoded body with a bare 413, measured rather than trusting Content-Length
- Rebuild the request with framing headers describing its buffered body
- Reject a size limit outside (0, Int.MaxValue) at construction, closing an overflow that would pass an emptied body through
- Add 19 regression tests and the HTTP-level test harness the module lacked

chore(SUPP-53): register phase 1 code review artifact 61bef424eb

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BDfb2ghkkKiwTKVrcufmaT

chore(SUPP-53): update review-state for phase 01 0af9f0a465

chore(SUPP-53): phase 1 PR open, awaiting review 9d6990b6d9

Repo CI triggers only on pull requests into main or release/*, so it does
not run on a phase PR into the feature branch. The four checks it would
have applied were run locally across every module and all pass:
__.compile, __.checkFormat, __.fix --check and __.test (436 tests).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BDfb2ghkkKiwTKVrcufmaT

Merge pull request 'Phase 1: Security filter drains the request body before the route reads it' (#54 ) from SUPP-53-phase-01 into SUPP-53 5e2dd2b4ae

Reviewed-on: #54

chore(SUPP-53): update review-state for phase 01 4f713eeb3f

chore(SUPP-53): mark phase 01 complete in tasks.md 3eeddbc6ae

chore(SUPP-53): file follow-ups to tracker 4510c4d013

All four lines from FOLLOW-UPS.md are now tracker issues; none matched an
existing issue, so each became a new one:

- #55 Pac4jCsrfSupport.tokenFor builds a body-reading web context
- #56 Body-borne pac4jCsrfToken fails CSRF check with a charset parameter
- #57 Four pac4j security files carry no PURPOSE header
- #58 maxformbodysize is not bounds-checked

The file is empty and removed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BDfb2ghkkKiwTKVrcufmaT

docs(SUPP-53): release notes for 0.1.24

CI / Check Linting (pull_request) Has been cancelled

Details

CI / Run Tests (pull_request) Has been cancelled

Details

CI / Compile (pull_request) Has been cancelled

Details

CI / Check Formatting (pull_request) Has been cancelled

Details

1f123a2fc3

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BDfb2ghkkKiwTKVrcufmaT

mph added 1 commit

2026-09-14 20:00:45 +00:00

chore(SUPP-53): update review-state to all_complete

CI / Check Formatting (pull_request) Successful in 1m23s

Details

CI / Compile (pull_request) Successful in 4m13s

Details

CI / Check Linting (pull_request) Successful in 3m26s

Details

CI / Run Tests (pull_request) Successful in 1m55s

Details

81d99439d8

mph merged commit a9cd69b488 into release/0.1.x

2026-09-14 20:08:52 +00:00

mph referenced this pull request from a commit

2026-09-14 20:08:54 +00:00

Merge pull request 'SUPP-53: buffer url-encoded request bodies so the security filter cannot drain them' (#59) from SUPP-53 into release/0.1.x

claude referenced this pull request

2026-09-14 20:24:46 +00:00

fix(server-http): buffer url-encoded bodies so the security filter cannot drain them #60

claude referenced this pull request

2026-09-15 09:46:58 +00:00

fix(iw): point the project config at the Forgejo tracker #61

Rows
Columns

SUPP-53: buffer url-encoded request bodies so the security filter cannot drain them #59

Summary

Defect fixed

Two consequences worth reading before upgrading

Testing

Also in this branch

Follow-ups

Release notes

Delivery