Correctness review: 28 findings, all resolved

BRACE-AGENTS.md

@@ -106,7 +106,7 @@ Use the `Read` variants for handlers that only query: GET routes are almost alwa
 `getRead` (or `getReadFull` if they need the session). They skip the per-request
 transaction entirely, which is both faster and signals intent.
 
-Path parameters use `{name}` syntax: `app.get("/posts/{id}", ...)` then `req.pathParam("id")` or `req.intPathParam("id")`.
+Path parameters use `{name}` syntax: `app.get("/posts/{id}", ...)` then `req.pathParam("id")` or `req.intPathParam("id")`. Values are percent-decoded.
 
 Grouping:
 
@@ -163,9 +163,11 @@ app.after("/api/*", (req, result) -> result.header("X-Api-Version", "1"));
 
 ```java
 req.method()                  // "GET", "POST", etc.
-req.path()                    // "/posts/42"
+req.path()                    // "/posts/42" — RAW, still percent-encoded
 
-// Path parameters (from route pattern like /posts/{id})
+// Path parameters (from route pattern like /posts/{id}) — percent-decoded.
+// "/users/John%20Doe" gives "John Doe"; don't decode again. Note "+" is a literal
+// plus in a path (not a space, unlike a form body).
 req.pathParam("id")           // path param as String
 req.intPathParam("id")        // as int
 req.longPathParam("id")       // as long

docs/migrations/brace-0.1.7-to-0.1.8.md

@@ -3,16 +3,24 @@
 **This release has no breaking changes.** No application code needs to change.
 
 It fixes two ways a durable job could be lost or wedged forever, both of which apply to
-apps that never touched the relevant configuration, and cuts durable-job pickup latency:
+apps that never touched the relevant configuration, cuts durable-job pickup latency, and
+lands the first three findings of the correctness review:
 
 - **Durable jobs claimed by an instance that dies are now recovered** instead of being
   stranded permanently.
 - **`Mailer` now bounds its SMTP timeouts**, so a wedged relay fails the send instead of
   hanging the calling thread forever.
 - **Durable jobs now start within ~1 second** on an idle app, down from up to 10.
+- **Path parameters are now URL-decoded** — `/users/John%20Doe` gives you `John Doe`, not
+  `John%20Doe`. If you were decoding by hand, stop.
+- **`/ops/routes` now shows route patterns instead of concrete URLs**, and **every response
+  is now counted** — including 429s, CSRF 403s and 404s that were previously invisible.
+- **HTML checkboxes bind to `boolean`**, repeated multipart fields survive, a trailing slash
+  matches, and 20-odd other correctness fixes — see "Other correctness fixes" below.
 
-All three ship as new *defaults*. The only reason to touch your code is if you run jobs longer
-than 30 minutes, talk to an unusually slow SMTP relay, or want to tune the poll rate — see below.
+They all ship as new *defaults*. The changes that can affect existing code are the ones where you
+may have written a workaround: path decoding, `Url.to` encoding, single-column
+`sqlQuery`/`hql` results, and checkbox binding. Each is called out with a before/after below.
 
 ---
 
@@ -242,6 +250,191 @@ always there, it just used to present as a stuck thread instead of an exception.
 
 ---
 
+## Path parameters are URL-decoded (correctness review H3)
+
+### What was wrong
+
+`req.pathParam(...)` returned the **raw, percent-encoded** segment, while `req.queryParam(...)`
+and `req.formParam(...)` returned decoded values. The same string round-tripped differently
+depending on where it rode:
+
+```java
+// 0.1.7 — GET /users/John%20Doe
+req.pathParam("name")            // "John%20Doe"   ← raw
+req.queryParam("name")           // "John Doe"     ← decoded, for ?name=John%20Doe
+```
+
+Anything but a bare integer id was affected — slugs, emails, filenames, tags. A lookup on the
+value silently missed rather than failing loudly:
+
+```java
+// 0.1.7: never matched a user whose email contains an encoded character
+var user = db.findBy(User.class, "email", req.pathParam("email"));
+```
+
+Static files had the same defect: `/assets/my%20file.css` looked for a file literally named
+`my%20file.css` and 404'd.
+
+### What changed
+
+Captured path parameters are percent-decoded, after the route match rather than before (so an
+encoded `%2F` stays inside the value and cannot forge a segment boundary). Static-file paths are
+decoded before the traversal checks. `+` is a **literal plus** in a path, not a space — this is
+path decoding, not form decoding. A malformed escape (`%zz`, a trailing `%`) is kept literally
+rather than throwing.
+
+`req.path()` is unchanged and still returns the raw path — it feeds route matching, middleware
+path patterns, log redaction and stats keys, all of which want the raw form.
+
+### What you may need to do
+
+**If you were decoding by hand, remove it** — you will now double-decode:
+
+```java
+// Before (0.1.7): the workaround
+var name = URLDecoder.decode(req.pathParam("name"), StandardCharsets.UTF_8);
+
+// After (0.1.8): already decoded
+var name = req.pathParam("name");
+```
+
+Double-decoding is not merely redundant, it is wrong: a name legitimately containing `%20` as
+text now decodes to a space. Grep for `decode(` near `pathParam` before upgrading.
+
+If you built links with `Url.to(...)`, note it does **not** yet encode its values (correctness
+review M6); encode values containing `/`, `?`, `#`, `&` or spaces yourself for now.
+
+---
+
+## Ops: route patterns, and every response counted (correctness review H1, H2)
+
+### What changed
+
+**`/ops/routes` and per-route stats are keyed by route pattern, not the request URL.** Previously
+`GET /users/1` and `GET /users/2` were separate rows, so the table filled with one entry per URL
+ever requested — unbounded, and per-route latency averages were meaningless because every row had
+a count of 1. Now they aggregate under `GET /users/{id}`.
+
+```
+# Before (0.1.7)                    # After (0.1.8)
+GET /users/1     count=1            GET /users/{id}    count=48210
+GET /users/2     count=1            GET /posts/{slug}  count=9930
+GET /users/3     count=1            GET (unmatched)    count=412
+...one row per id, forever...       GET (static)       count=88301
+```
+
+Two constant buckets cover requests with no route: `(unmatched)` for 404s and `(static)` for files
+served from a `staticFiles` mapping. They are constants on purpose — the URL there is
+client-supplied, so a row per `/random-404-url` would be unbounded in whatever an attacker types.
+
+**Every response is now counted.** Before, only the handler success path and the two error paths
+recorded anything, so `/ops/status` under-reported total traffic and these were completely
+invisible:
+
+- rate-limiter 429s and other before-middleware short-circuits
+- auth-guard redirects
+- CSRF 403s
+- 413 payload-too-large
+- static-file serves
+- unmatched-route 404s
+
+Static-file requests now also appear in the request log. If that is too noisy for your deployment,
+raise the log level (`BRACE_LOG_LEVEL=WARN` or `-Dbrace.log.level=WARN`); serving assets from a
+CDN or reverse proxy avoids it entirely.
+
+A 500 still emits exactly one log line (`http.error`, with the exception and app frame) — the
+log shape is unchanged.
+
+### What you may need to do
+
+Nothing, unless you **parse `/ops/status` or `/ops/routes`**. If you do:
+
+- expect route patterns (`/users/{id}`) where you previously saw concrete URLs
+- expect the two literal keys `(unmatched)` and `(static)`
+- expect request counts and status-code totals to go **up**, because they now include traffic
+  that was previously dropped on the floor rather than because traffic changed
+
+If you were relying on the routes table to find out *which* URLs 404, use `/ops/logs` or the error
+store instead — the log still records the concrete (redacted) path for every request.
+
+---
+
+## Other correctness fixes (behavior changes worth knowing)
+
+The rest of the correctness review is bug fixes that need no action from you. These few change
+observable behavior, so check them against your code:
+
+**HTML checkboxes now bind to `boolean` (M2).** `Boolean.parseBoolean` is true only for the literal
+`"true"`, so a *checked* checkbox — which submits `name=on` — used to bind `false`. If you worked
+around it by declaring the field as `String` and comparing to `"on"` yourself, you can switch to
+`boolean`. If you kept a `String` field, nothing changes.
+
+```java
+// The workaround you can now drop:
+record Signup(String email, String agree) {}   // then: "on".equals(form.value().agree())
+record Signup(String email, boolean agree) {}  // 0.1.8: just works
+```
+
+Accepted as true: `on`, `true`, `1`, `yes`, `checked` (case-insensitive). Everything else, and
+absence, is false.
+
+**Repeated multipart fields are preserved (M1).** `req.formParams("tag")` returns every value for a
+`multipart/form-data` submission, as it already did for `application/x-www-form-urlencoded`. It
+previously returned only the last. `req.formParam("tag")` is unchanged (last wins).
+
+**A trailing slash now matches (L1).** `GET /users/` reaches the `/users` handler instead of 404ing.
+Matching rather than redirecting is deliberate: a 301 would turn a `POST /users/` into a GET and
+drop its body. If you relied on the 404 to reject trailing slashes, add an explicit check.
+
+**`sameSite("None")` now implies `Secure`, and invalid values throw (M12).** Browsers reject
+`SameSite=None` without `Secure`, so that combination was silently discarding your session cookie.
+An unrecognized value (`"Loose"`, `""`, null) now throws instead of being written verbatim into the
+header, where browsers ignored the attribute entirely. Valid values: `Strict`, `Lax`, `None`.
+
+**`Url.to(...)` percent-encodes its values (M6).** Previously they were appended raw, so a value
+containing `/` added a path segment and one containing a space produced an invalid URL. If you were
+encoding values before passing them in, remove that — you will now double-encode.
+
+Note a value containing `/` or `%` still cannot ride in a path segment: Jetty's default URI
+compliance rejects `%2F` and `%25` with a 400 before your handler runs. Put those in the query
+string.
+
+**`db.sqlQuery(...)` / `db.hql(...)` return real rows for single-column selects (M7).** They always
+declared `List<Object[]>` but returned bare scalars when the select had one item, so
+`for (Object[] row : ...)` threw `ClassCastException`. Now every row is an `Object[]`; read
+`row[0]`. If you worked around it by casting to `List<Object>` and calling `toString()`, switch to
+`row[0]`:
+
+```java
+// Before (0.1.7): the workaround
+var names = (List<Object>) (List<?>) db.sqlQuery("SELECT name FROM users");
+for (var n : names) { use(n.toString()); }
+
+// After (0.1.8)
+for (var row : db.sqlQuery("SELECT name FROM users")) { use(String.valueOf(row[0])); }
+```
+
+**`View.of(...)` throws on an odd argument count (L3).** It used to silently drop a trailing key, so
+a typo rendered a template missing a variable with no error. If a call was quietly relying on that,
+it now fails loudly at render time — which is the point.
+
+**`stop()` closes the `DatabaseFactory` (M4).** If several `Brace` instances share one factory, or a
+test fixture reuses one across cases, call `.ownsDatabase(false)` and close it yourself:
+
+```java
+app.database(sharedFactory).ownsDatabase(false);
+```
+
+**`daily(...)` jobs no longer drift across DST (M11).** They reschedule from the wall clock instead
+of assuming 24h of elapsed time, and dedupe on the local calendar day. If your instances run in
+different time zones, put them all in one (UTC is the usual choice) — that was already required for
+a local firing time to mean anything across a fleet.
+
+**Interval strings accept `d` (L8).** `every("1d", ...)` and `jobLease("15d")` work now; they used
+to throw while `cache.set(k, v, "1d")` accepted the same string.
+
+---
+
 ## Upgrading
 
 Bump `<brace.version>` to `0.1.8` and re-run `brace agents-md` to regenerate `BRACE-AGENTS.md`