Correctness review: 28 findings, all resolved by megamattron · Pull Request #5 · larvalabs/brace

megamattron · 2026-07-26T00:33:12Z

First Correctness review of Brace — a fourth periodic-review category alongside Security, Token Efficiency, and Runtime Performance. Where those ask "is it safe / cheap / fast", this asks "is it right": wrong results, silently dropped data, unbounded growth, work lost rather than retried, and APIs that contradict their own documentation.

28 findings (4 High, 12 Medium, 12 Low), all 28 resolved — 27 fixed here, and H4 fixed independently on main while the review was in flight. One commit per finding or tight group; full mvn test green after each.

Every High and most Mediums was reproduced against a running app with a throwaway probe before being written up, not just read from source.

Canonical tracker: docs/2026-07-24-correctness-review-todos.md
Review record: docs/reviews/2026-07-correctness-opus-5.md

High

H1 — per-route stats were keyed by the concrete URL, so the routes map grew without bound.
Stats.recordRequestPattern was added by the runtime-performance review (H7) specifically to bound this map by the route table, and was never wired into BraceHandler — it had no caller in src/main, only tests. All three recording sites used the raw-path variant. Confirmed: three requests to /users/{id} produced three distinct keys, one ConcurrentHashMap entry plus two LongAdders per distinct URL ever requested, for the life of the process.
Fix: hoist RouteMatch out of the try so both catch blocks can see it; funnel all three sites through one helper keying on route().pattern(). The no-match fallback records a constant (unmatched) bucket rather than the raw path — falling back to the path would have left the leak open on exactly the attacker-controlled input, since a malformed percent-escape in the query string throws before router.match runs. Log.request deliberately keeps the concrete path.

H2 — every response that short-circuited before the handler was invisible to stats and the request log.
Recording ran only on the success path and the two catch blocks. Rate-limiter 429s, auth-guard redirects, CSRF 403s, 413s, static-file serves, and unmatched-route 404s all returned earlier. Confirmed: 7 handler requests + 1 blocked + 1 unmatched left statusCodeCounts() at {200=7}.
Fix: recording moves inside the choke-point writeResult, which every response already passes through, so coverage is structural rather than a list of call sites. Idempotent via a recorded flag. Static files are recorded and logged (a log that omits a class of request is the same defect one level down); a 500 still emits exactly one log line.

H3 — path parameters were never URL-decoded.
The router matched Jetty's raw encoded path and copied captures straight through, while query and form params were decoded — so the same value round-tripped differently depending on its carrier. Confirmed: GET /users/John%20Doe → pathParam("id") == "John%20Doe". Any non-integer parameter (slug, email, filename, tag) handed handlers corrupt data, and lookups missed silently. Static files had it too.
Fix: a path-segment decoder — deliberately not URLDecoder, which maps + to space and throws on malformed escapes. Route.match decodes after the regex capture; decoding first would turn %2F into a real separator.

H4 — durable jobs stranded by a dead instance. Fixed upstream by 96f37a2 before this branch touched it; rechecked against ce085c0 and marked resolved, with the residual and two inherent lease artifacts recorded so a later reviewer doesn't re-file them.

Medium


M1	Repeated multipart fields collapsed to their last value — a checkbox group yielded one value as multipart and all of them as urlencoded
M2	A checked HTML checkbox bound to `false`: it submits `name=on`, and `Boolean.parseBoolean` is true only for `"true"`. The control was silently inert
M3	The framework's `Vary: HX-Request` clobbered a handler's own `Vary`, so a shared cache varied on the wrong axis and served the wrong variant
M4	`stop()` never closed the `DatabaseFactory`. With `minimumIdle == maximumPoolSize` that is `poolSize` live connections per stopped app. Added `.ownsDatabase(false)` for shared factories
M5	`stop()` left the rate limiter's process-global statics pointing at the stopped app's factory
M6	`Url.to` appended values raw — `Url.to("/users/{name}", "a/b")` → `/users/a/b`
M7	`sqlQuery`/`hql` declared `List<Object[]>` but returned bare scalars for single-column selects, so `for (Object[] row : …)` threw `ClassCastException` in the caller's loop
M8	WebSocket broadcast had no per-member isolation — one bad connection aborted delivery to the whole room. Plus a `queuedBytes` leak on synchronous send failure
M9	`Cache.getOrSet` branched on the SPI then cast to the built-in backend, so any third-party backend threw `ClassCastException` on the most-recommended cache call
M10	SMTP credentials weren't percent-decoded, while `DatabaseFactory` decodes its own
M11	`daily(…)` drifted an hour at every DST transition, and could lose a day outright — dedupe slotted on a UTC day while firing at a local time
M12	`sameSite("None")` didn't imply `Secure`, so browsers silently discarded the session cookie. Invalid values went verbatim into the header

Low

L1 trailing-slash 404s · L2 dead weak-secret check · L3 View.of silently dropped a trailing key · L4 no-op session writes forced a cookie re-mint and Cache-Control: private · L5 Storage used form encoding where SigV4 needs RFC 3986 · L6 multipart header injection via filename · L7 Http status/redirect asymmetry (documented, not changed) · L8 every("1d") threw while cache.set(…,"1d") worked · L9 a negative CIDR prefix meant trust everything · L10 one Log overload leaked raw exception messages · L11 the HQL E-string branch fired on any e before a quote · L12 redactMessage destroyed message structure even when redacting nothing

Corrections to the review's own claims

Implementation surfaced two places where the write-up was more alarming than the code deserved. Both are recorded next to the original text rather than quietly edited out:

H3 was a data-correctness bug, not a live traversal hole. Jetty's default UriCompliance rejects %2F, %25, %2e and malformed escapes with a 400 before the handler runs — found when four traversal tests returned 400 instead of the expected 404. The decode-after-match ordering is still right (compliance is configurable, Route.match is public API), but the severity claim was wrong.
H1's spec would have made the request log worse. It said to key the log by pattern too "so /ops/logs and /ops/routes agree". They shouldn't: the routes table is a bounded aggregate, the log is a stream where the concrete URL is the whole diagnostic value.

Also: the findings doc's own summary miscounted — 28 findings with 12 Lows, not 25 with 9.

Things worth knowing

A fix that reverts silently will revert again. H1 was already fixed once and reverted with nothing noticing. The regression test is the deliverable, not the fix.
Making a type honest flushes out code that adapted to the lie. M7 immediately broke TestApp.resetDatabase, which had cast a single-column result to List<Object> and called toString() — working only because the declared type was wrong.
Two existing tests were pinning bugs, not requirements, and were updated with reasoning inline: RouterTest.trailingSlashPatternNormalized asserted a router with /about/ registered would not match /about/; DurableJobTest.jobLeaseRejectsMalformedIntervals asserted "15d" was invalid.

Migration guide

docs/migrations/brace-0.1.7-to-0.1.8.md covers every user-visible change, leading with the four where an existing workaround now becomes the bug — hand-decoded path params, hand-encoded Url.to values, the List<Object>+toString() cast around single-column sqlQuery, and checkbox fields declared as String. Double-applying any of those is worse than the original defect.

Validation

Full mvn test green after every commit — 1123 tests at branch tip.
11 new test classes plus cases added to MultiValueParamsTest and RouterTest.
Not run: mvn verify (the Testcontainers Postgres tier). The merge gate requires it and several fixes touch Postgres-specific paths (Counters, ErrorStore upsert, job claim) — please run before merging.
One pre-existing flake noted: DurableJobTest.claimsSizedToCapacityAndSlowJobsDontStallNewBatches failed once under full-suite load and passed on every isolated run, including against a stashed baseline.

🤖 Generated with Claude Code

https://claude.ai/code/session_01FsX1W4Zt8xd34nu34Axq2b

Generated by Claude Code

Kicks off the first Correctness review — a fourth periodic-review category alongside Security, Token Efficiency, and Runtime Performance. It looks for plain bugs: wrong results, silently dropped data, unbounded growth, work lost rather than retried, and behavior that contradicts its own documentation. 25 findings (4 High, 12 Medium, 9 Low). Every High and most Mediums were reproduced against a running app rather than only read; the reproduction is recorded inline in each entry. Highs: H1 per-route stats keyed by concrete URL — recordRequestPattern has no caller in main, so the routes map grows per distinct URL forever H2 every pre-handler short-circuit (429s, CSRF 403s, unmatched 404s, static files) is invisible to stats and the request log H3 path parameters are never URL-decoded, while query and form params are H4 a durable job whose process dies mid-execution is stuck "running" forever — no stale-claim reaper, and stop() does not drain job threads Also records the scope cut (CLI, ops rendering, JfrProfiler, migration SQL) so a follow-up pass knows what was not swept. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FsX1W4Zt8xd34nu34Axq2b

Rechecks the 2026-07-24 correctness findings (reviewed at b3409ee) against ce085c0, after the job-system work landed on main. H4 (durable jobs stranded by a dead instance) is fixed by 96f37a2, refined by c9c679d/7681c47. The upstream fix matches this finding's spec — the same two-statement recovery keyed on attempts, a configurable lease, and a partial index so the sweep is an index scan — and improves on it by running the sweeper on its own thread rather than inside pollLoop, which a poll loop parked in limiter.acquire() would never reach. Marked resolved, with the deliberate residual recorded (stop() still does not drain job threads, so a deploy strands work that recovery reruns up to lease+sweep later) plus two inherent lease artifacts, so a later reviewer does not re-file them. cdc4f07 bounded the Mailer's SMTP timeouts but did not touch M10 — embedded credentials are still not percent-decoded. Nothing on main touched Stats, BraceHandler, Route, FormBinder or Url, so H1/H2/H3 and every Medium and Low stand. Remaining: 3 High, 12 Medium, 9 Low. Also expands H1's fix spec into the concrete change: hoist RouteMatch out of the try in handle() so both catch blocks can see it, funnel all three recording sites through one helper that prefers route().pattern() and falls back to the raw path only when there is genuinely no match, and keep the redaction split (raw paths redacted, patterns not). Notes the user-visible /ops/routes change and that the regression test is the real deliverable — this fix existed once as perf-review H7 and reverted unnoticed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FsX1W4Zt8xd34nu34Axq2b

…te URL Stats.routes is cumulative and never reset, so keying it by the request path leaks one entry plus two LongAdders per distinct URL ever requested, for the life of the process. With ids in the path that is unbounded in the app's own data; on the throw-before-routing path it is unbounded in whatever an attacker types. Stats.recordRequestPattern was added by the runtime-performance review (H7) to bound this map by the route table, but was never wired into BraceHandler and had no caller in src/main — only tests. All three recording sites used the raw-path variant. Confirmed: three requests to /users/{id} produced three distinct keys. RouteMatch is hoisted out of the try in handle() so both catch blocks can see it, and the three sites funnel through one recordAndLog helper that keys on route().pattern(). Two decisions the finding did not settle: Log.request keeps the concrete (redacted) path. The routes table is a bounded latency aggregate; the log is an unbounded stream where the URL is the whole diagnostic value, and "GET /users/{id} 404'd" without the id is not worth having. Only stats change. The no-match fallback records a constant "(unmatched)" bucket rather than the raw path. Falling back to the path would have left the leak open on exactly the attacker-controlled input: a malformed percent-escape in the query string throws out of parseQuery, which runs before router.match, so every /<random> of that shape would mint a permanent key. Which URLs 404 is a question for /ops/logs and the error store; the routes table answers latency per route. Stats.recordRequest is now unused by the framework. Kept public for apps recording synthetic entries, with its Javadoc corrected to stop claiming the handler uses it and to warn about key cardinality. /ops/routes and per-route stats now show patterns instead of concrete URLs — user-visible, migration-guide entry to follow with the rest of the batch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FsX1W4Zt8xd34nu34Axq2b

Stats and Log.request ran only on the success path and the two catch blocks. Every other exit from handle() returned without recording: before-middleware short-circuits (rate-limiter 429s, auth redirects), session-middleware short-circuits, CSRF 403s, 413s, static-file serves, and the unmatched-route 404. So /ops/status under-reported total traffic, and the signals an incident actually needs — 429 floods, 404 storms, CSRF failures — were the ones missing. Confirmed: 7 handler requests plus a blocked one plus an unmatched one left statusCodeCounts() at {200=7}. Recording moves into the choke-point writeResult overload, which every response already passes through, so coverage is structural rather than a list of call sites someone has to remember to extend. A per-request Exchange holder carries what it needs (start time, method, path, match, db) and is built before the try so the catch paths share it. A recorded flag makes it idempotent — no path can double-count. Three decisions the finding left open: Static files are recorded and logged. A request log that silently omits a class of request is the same defect one level down, and a static serve that spent 40ms on disk is real latency worth seeing. BRACE_LOG_LEVEL is the volume knob for anyone who disagrees. A 500 stays one log line. The error path already emits http.error with the exception and app frame, so it marks the exchange logged and the choke point records stats without a duplicate http.request. Log shape is unchanged; only stats gained. Static files get their own (static) bucket rather than sharing (unmatched), so asset traffic does not inflate the 404 count. Both stay constants for the H1 reason: the filename is client-supplied, so /assets/<random>.css must not mint a key per miss. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FsX1W4Zt8xd34nu34Axq2b

…aths The router matched Jetty's raw, still-encoded path and copied regex captures straight into pathParams, while query and form params were decoded — so the same value round-tripped differently depending on which carrier it rode in. Confirmed: GET /users/John%20Doe gave pathParam("id") == "John%20Doe". Any route parameter that is not a bare integer — slug, email, filename, tag — handed the handler corrupt data, and a lookup on it missed silently rather than failing. Static files had it too: /assets/my%20file.css looked for a literal "my%20file.css" and 404'd. Adds Request.decodePathSegment/decodePath rather than reusing URLDecoder, which is form decoding on two counts that matter here: it maps "+" to a space, silently renaming /files/a+b, and it throws on a malformed escape, which on a request path would make a stray "%" a 500. The new decoder keeps bad escapes literal, the way browsers and mainstream servers do. Route.match decodes after the regex capture, never before. That ordering is the safety argument: decoding first would turn %2F into a real separator, so /files/a%2F..%2Fb would match a two-segment route and hand a handler an escaped path. Decoding after keeps %2F inside the value it was written in. Static-file serving decodes before the ".." check — after would let %2e%2e slip past it — and before resolve/normalize/startsWith. Assets.currentVersion now gets the decoded URL path so an encoded filename can still match its own fingerprint instead of always falling back to revalidate-always. Worth recording against the finding's original framing: the traversal exposure was smaller than I claimed. Jetty's default UriCompliance already rejects %2F, %25, %2e and malformed escapes with a 400 before the handler runs, so those never reached the old ".." check either. This was a data-correctness bug, not a live traversal hole. The ordering still matters — compliance is configurable and Route.match is public API — and the tests are split along that seam: over-the-wire for what Jetty forwards, unit tests for what it refuses. req.path() deliberately still returns the raw path: it feeds route matching, middleware PathPattern, Redactor and stats keys, all of which want the raw form. Decoded values are what pathParam is for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FsX1W4Zt8xd34nu34Axq2b

Adds the two user-visible entries to the 0.1.7 -> 0.1.8 guide. Path decoding: shows the old raw-vs-decoded asymmetry between pathParam and queryParam, and calls out the one way this can break an app — code that worked around the bug by decoding pathParam by hand now double-decodes, so a name legitimately containing "%20" as text turns into a space. Says to grep for decode( near pathParam before upgrading. Notes Url.to still does not encode (M6), so links must be encoded by hand for now. Ops output: before/after of the routes table filling with one row per id versus aggregating under a pattern, the two constant buckets and why they are constants, and the list of responses that were previously uncounted. Flags that request counts will appear to jump — that is previously-dropped traffic, not new traffic — and that static requests now reach the log, with BRACE_LOG_LEVEL as the knob. Points anyone who used the routes table to find which URLs 404 at /ops/logs, which still carries the concrete path. BRACE-AGENTS.md: marks pathParam as decoded and path() as raw, and warns that "+" is a literal plus in a path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FsX1W4Zt8xd34nu34Axq2b

parseMultipart accumulated non-file parts into a Map<String,String> and only then re-encoded them into the body Request re-parses, so a repeated field kept only its last value. A checkbox group or <select multiple> submitted as multipart/form-data yielded one value where the byte-identical urlencoded submission yielded all of them. Confirmed: two "tag" parts (a, b) gave formParams("tag") == [b]. The body is now appended to as parts are parsed, with no intermediate map. formParam(name) is unchanged — the single-value view downstream already does last-wins. Also corrects the findings-doc totals: 28 findings, not 25 (there are 12 Lows, not 9). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FsX1W4Zt8xd34nu34Axq2b

…of clobbering Two independent one-line defects in the same request path, fixed together because they share a test. M2: FormBinder used Boolean.parseBoolean, which is true only for the literal string "true". An HTML checkbox submits name=on when checked, so a CHECKED box bound to false — the control was silently inert and the form recorded the opposite of what the user did. Confirmed: agree=on bound to a boolean component gave false. Now accepts on/true/1/yes/checked case-insensitively. Absence still binds false through the existing empty-value branch, which is what an unchecked box relies on. M3: the framework wrote result.header("Vary", "HX-Request") into the single-value header map, replacing whatever the handler or an after-middleware had set. A response that legitimately varied on Accept-Encoding lost that dimension on every htmx request, so a shared cache would then vary on the wrong axis and serve the wrong variant. Confirmed: a handler setting Vary: Accept-Encoding got Vary: HX-Request only. Now appends, case-insensitively idempotent, and treats a "*" as already covering. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FsX1W4Zt8xd34nu34Axq2b

…te-limiter statics Both are stop() failing to release what start() took; fixed together because the ordering between them matters. M4: DatabaseFactory.close() existed and had no caller anywhere in src/main. Since Hikari runs with minimumIdle == maximumPoolSize, every stopped app left poolSize live connections plus a Hibernate SessionFactory behind — across a test suite with a TestApp per class that is a large steady leak, and in production a "graceful" shutdown the database only learns about when the process exits. Ownership is a real question, since the factory is app-constructed and handed in via .database(), so this adds .ownsDatabase(false) for a factory that outlives the app. Closing is the default because one-factory-per-app is the overwhelmingly common shape and the old behavior failed silently. M5: start() installs a static Counters built from this app's factory, and limiters only ever join the static ALL registry. Neither was released, so a second app in the same JVM counted against the previous app's factory — dead once M4 lands — and allStats() kept reporting limiters whose app was gone. stop() now calls disableSharedBackend() (which existed purely for test teardown) and a new forgetLimiters(), both before the factory closes so nothing can reach a closed pool on the way down. Deferred and recorded in the findings doc rather than left silent: each limiter's cleanup virtual thread still runs for the life of the JVM. It parks 60s between sweeps over unreferenced maps, so it is a parked thread rather than growing state; retiring it needs RateLimiter to gain a close() and an owner, which is out of scope here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FsX1W4Zt8xd34nu34Axq2b

…ape honest M6: Url.to appended substituted values raw, so a value containing "/" silently added a path segment and one containing a space, "?", "#" or "&" produced an invalid or truncated URL — in the method that exists specifically to build links. Confirmed: Url.to("/users/{name}", "a/b") gave /users/a/b. Encoding is the RFC 3986 unreserved set, not URLEncoder: URLEncoder is form encoding and emits "+" for a space, which in a path is a literal plus, so it would not invert the path decoder added for H3. Url.to and pathParam are now a real inverse pair, tested by round-tripping values through a live route. Worth recording: a value containing "/" or "%" encodes correctly but still cannot ride in a path segment, because Jetty's default UriCompliance rejects %2F and %25 with a 400 before the handler runs. That is a transport limit, not a Brace bug — encoding them any other way would be wrong — so the test asserts it explicitly and points such values at the query string. M7: hql() and sqlQuery() both declared List<Object[]> and reached it through an unchecked cast that is simply false for a single-column select, where Hibernate returns a list of scalars. The mismatch surfaced as a ClassCastException inside the caller's own for-loop, with a stack trace nowhere near the query. A shared asRows helper wraps non-array elements, so the declared type is true for every query and row[0] works uniformly. sqlQueryLong already normalized both shapes; this brings the list accessors into line with it. Both unchecked-cast suppressions are gone — there is no longer a lying cast to suppress. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FsX1W4Zt8xd34nu34Axq2b

…onest row shape resetDatabase cast a single-column "SELECT TABLE_NAME" to List<Object> and called toString() on each element — which only worked because sqlQuery's declared List<Object[]> was a lie for one-column selects. With the shape now honest it read Object[].toString(), producing "TRUNCATE TABLE [Ljava.lang.Object;@26612078". Reads row[0] instead. Worth noting as evidence the finding was real rather than theoretical: the framework's own code had silently coded against the wrong type, and the full suite caught it the moment the type stopped lying. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FsX1W4Zt8xd34nu34Axq2b

…, M10 decode SMTP creds M8: WsRegistry.deliverLocal iterated room members calling ctx.send with no guard, so anything thrown for one member — a session Jetty had already closed, say — aborted delivery to every remaining member, and one bad connection silently dropped a broadcast for the whole room. Each send is now isolated and a failure is logged and skipped. Second half of the same finding: WsContext.send reserved queuedBytes before calling sendText, and a synchronous throw there never reaches the callback that releases it. The connection then carried a permanent phantom backlog and was eventually force-closed as a "slow consumer" it never was. The reservation still has to be taken before the call (the success callback can fire during it), so it is released in a catch instead. M9: CacheBackend is a documented public SPI, but Cache.getOrSet branched on the SPI method requiresSerialization() and then cast the backend to the concrete built-in InMemoryBackend — so any third-party live-object backend threw ClassCastException on the cache call the docs recommend most. getOrCompute moves onto the SPI with a plain get/compute/set default (what the serializing path already does), and InMemoryBackend overrides it with its existing per-key single-flight. The Javadoc says single-flight is an optimization rather than a contract, so an implementer knows the default is legitimate. The new test defines a minimal third-party backend and drives getOrSet through it, and re-asserts single-flight on the built-in backend. M10: Mailer read the SMTP user-info without percent-decoding, while DatabaseFactory explicitly decodes its own ("libpq parity"). A password containing '@', '/' or ':' must be encoded to survive URI parsing at all, and would then authenticate with the literal "%40" — an auth failure with no visible cause. Now decoded on both halves, matching DatabaseFactory. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FsX1W4Zt8xd34nu34Axq2b

…implies Secure M11: daily() computed a delay to the next local occurrence and then handed scheduleAtFixedRate a fixed 24h period. But a fixed period is 24h of elapsed time while "daily at 03:00" is a wall-clock statement, and the two diverge at every DST transition — so the job ran an hour off its configured local time, permanently, until restart. Worse, it could lose a day outright. Cluster dedupe slotted on floor(epochMillis / 86_400_000), which is a UTC day, so for a job whose local time sits near the UTC-day boundary the shift could put two consecutive runs in the same slot; the second was deduped away and simply never happened. This affects the framework's own brace-jobs-prune (03:23) and ops-metrics-prune (03:17). Daily jobs now schedule a self-rescheduling one-shot that recomputes its delay from the wall clock after each firing, and slot on the local calendar day. Interval jobs keep the elapsed-time slot, which is right for them. The local-day slot assumes instances share a time zone — which is what firing at a local time already assumed: in mixed zones each instance fires at its own local 03:00, so they were never running together to begin with. Documented on the slot helper. M12: sameSiteNone() set Secure; the string setter sameSite("None") did not. Browsers reject SameSite=None without Secure outright, so that combination did not weaken the session cookie, it discarded it — and the symptom, nobody staying logged in, points nowhere near the config line. The string setter now implies Secure, normalizes to canonical spelling, and rejects anything outside Strict/Lax/None: a typo previously went verbatim into the header, where browsers ignore the attribute entirely and fall back to their own default, silently downgrading the very setting the caller was tightening. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FsX1W4Zt8xd34nu34Axq2b

…f arity, L4 no-op session writes Four small independent defects, batched into one commit because each is a few lines and they share no logic. L1: "/plain" compiled to ^/plain$, so a request for "/plain/" matched nothing and got a bare 404 with no hint — for a URL a user typed or a link carried. Router.match now retries once against the slash-stripped path. Matching rather than 301-redirecting is deliberate: a redirect turns a POST into a GET and drops its body, so canonicalize-by-redirect is only correct for GET. "/" stays canonical, the method must still match, and an unknown path still 404s. This made an existing test wrong. RouterTest.trailingSlashPatternNormalized asserted match("GET", "/about/") was null on a router with "/about/" registered — pinning the bug rather than a requirement, since registering "/about/" and then 404ing "/about/" is indefensible either way. Updated with the reasoning inline. L2: validateSecret tested lower.contains("CHANGE-ME-to-a-random-...") against an already-lowercased string, so the clause could never match. Removed rather than repaired: the scaffold value it aimed at is already caught by the "change-me" check on the line above. L3: View.of and View.render looped to length - 1, silently discarding a trailing key, so a typo'd View.of("page", "a", 1, "b") rendered a template missing b with no error and the failure showed up as a blank spot in the page. Both now throw on an odd count, naming the dangling key, matching what Session.of has always done. L4: Session.set/remove/clear flipped modified unconditionally, so a no-op write cost a full AES-GCM re-mint, a Set-Cookie, and Cache-Control: private on the response — meaning a guard doing an unconditional session.set(...) made every response uncacheable and re-issued the cookie every request. Now only a real change marks the session modified. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FsX1W4Zt8xd34nu34Axq2b

…redaction fixes Eight small independent defects. L5: Storage.uriEncodePath used URLEncoder (form encoding), which diverges from SigV4's required set in both directions — "*" left literal where SigV4 wants %2A, "~" encoded where SigV4 wants it literal. A key containing either produced a canonical request S3 could not reproduce, i.e. SignatureDoesNotMatch. Now shares Url.encodeSegment, added for M6: one RFC 3986 encoder, two callers, no drift. L6: Http.Multipart built Content-Disposition by raw concatenation, and a filename usually comes from UploadedFile.filename() — a remote client. A quote corrupted the body; a CR/LF injected arbitrary part headers. CR/LF are dropped (no correct escaping exists for them in a header) and quotes/backslashes escaped per RFC 6266. L7: documented rather than changed. fetch/fetchJson/fetchString return whatever status came back while fetchBytes throws on non-2xx, and redirects are not followed. Both are defensible and changing either would break callers silently — auto-throwing turns a handled 404 into an exception, and auto-following can replay a body or leak an Authorization header to another host. The Javadoc now says which throws when, and why fetchBytes differs. L8: JobScheduler.parseInterval rejected "d" while Cache.parseTtl accepted it, so every("1d", ...) threw while cache.set(k, v, "1d") worked — two grammars behind identical-looking strings. Added, and the error now lists valid units. This changed an existing assertion: jobLeaseRejectsMalformedIntervals expected jobLease("15d") to throw, which pinned the absence of the unit rather than a deliberate rejection of day-length leases. It uses "15y" now. L9: a negative CIDR prefix produced an all-zero mask, which matches every address — turning a typo like "10.0.0.0/-1" into "trust every forwarding header", the exact opposite of the intent. An over-wide prefix was silently clamped. Both now throw, naming the bound and the offending value. "/0" still means everything, because that is what it means. L10: Log.error(String, Throwable) stored the raw exception message under "errorMessage", and println's redaction pass is name-based — "errorMessage" is not a sensitive-looking name, so a message carrying a bearer token reached stdout and /ops/logs untouched from this overload alone. Now runs Redactor.redactMessage like the request-path overload. L11: the E-string branch in the HQL parameter rewriter fired on any "e" immediately before a quote, including the trailing E of "... LIKE'%x%'". That opened backslash-escape mode, where a literal backslash before the closing quote swallows the terminator and every subsequent "?" is mis-numbered. Now requires the E to start a token. L12: redactMessage split on a delimiter class and rejoined with single spaces, so commas, colons, brackets, quotes and newlines were replaced wholesale — even when nothing was redacted. Hibernate messages are exactly that shape, and this text is what ops_errors.message stores and /ops/errors shows. Redacted spans are now spliced into the original string with delimiter runs copied verbatim, and the common case (nothing redacted) allocates nothing at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FsX1W4Zt8xd34nu34Axq2b

Covers the behavior changes from M1-M12 and L1-L12 that a user could notice. Leads with the four where an existing workaround now becomes the bug — checkbox fields declared as String, hand-encoded Url.to values, the List<Object>+toString() cast around single-column sqlQuery, and hand-decoded path params — since double-applying any of those is worse than the original defect. Also documents the transport limit found while testing M6: a value containing "/" or "%" encodes correctly but still cannot ride in a path segment, because Jetty's default URI compliance rejects %2F and %25 with a 400 before the handler runs. Points those at the query string. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FsX1W4Zt8xd34nu34Axq2b

…entry Records scope, the finding summary, the commit story, validation status, and what was deliberately left. Keeps three things a future reviewer would otherwise have to rediscover: - Two corrections to this review's own claims. H3 was written up as traversal-adjacent when Jetty's UriCompliance already rejects the dangerous encodings before any framework code runs — found when four traversal tests returned 400 instead of 404. And H1's spec would have made the request log worse by keying it on route pattern; the routes table and the log want different things. - Why the H1 regression test is the deliverable rather than the fix: the same fix landed once in the runtime-performance review and reverted with nothing in the suite noticing. - That making a type honest flushes out code that adapted to the lie (M7 broke TestApp.resetDatabase immediately), and that two existing tests were pinning bugs rather than requirements. Also flags the merge gate: mvn verify (the Testcontainers Postgres tier) has not been run, and several fixes touch Postgres-specific paths. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FsX1W4Zt8xd34nu34Axq2b

claude added 17 commits July 25, 2026 11:38

Navigation Menu

Uh oh!

Correctness review: 28 findings, all resolved - #5

Correctness review: 28 findings, all resolved#5
megamattron wants to merge 17 commits into
mainfrom
claude/correctness-review-ey31yz

megamattron commented Jul 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

megamattron commented Jul 26, 2026

High

Medium

Low

Corrections to the review's own claims

Things worth knowing

Migration guide

Validation

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants