diff --git a/BRACE-AGENTS.md b/BRACE-AGENTS.md index 3f0a7a2..d83ed68 100644 --- a/BRACE-AGENTS.md +++ b/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 diff --git a/docs/2026-07-24-correctness-review-todos.md b/docs/2026-07-24-correctness-review-todos.md new file mode 100644 index 0000000..df1f419 --- /dev/null +++ b/docs/2026-07-24-correctness-review-todos.md @@ -0,0 +1,509 @@ +# Correctness Review: 2026-07-24 (Opus 5) + +## Summary + +First **Correctness** review — a fourth category alongside Security, Token Efficiency, and +Runtime Performance (see `docs/reviews/README.md`). It looks for plain bugs: wrong results, +silently dropped data, unbounded growth, work that is lost rather than retried, and API +behavior that contradicts its own documentation. It is not a security or performance pass; +where a finding also has a security or perf flavor, that is noted but is not the reason it +is listed. + +28 findings: 4 High, 12 Medium, 12 Low. Every High and most Mediums were reproduced against +a running app with a throwaway probe test, not just read — the reproduction is recorded +inline as "Confirmed:". + +**Review baseline is `b3409ee`**; line numbers cite that commit. Rechecked against `ce085c0` +after the job-system work landed on `main`: **H4 is resolved upstream** (see its entry — the +fix is `96f37a2`, and it is a better fix than the one specified here). `cdc4f07` bounded the +Mailer's SMTP timeouts but did not touch M10 (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 below stand as written. Remaining: **3 High, 12 Medium, 12 Low.** + +Branch: `claude/correctness-review-ey31yz`. One commit per finding, +`fix(correctness): `, each commit ticks its checkbox here and passes +`mvn test`. User-visible changes get migration-guide entries per AGENTS.md. + +Dimensions swept: request lifecycle (`BraceHandler`, `Request`, `Result`, `Router`, `Route`); +sessions/CSRF/forms/views; database wrapper and HQL rewriting; background work (`JobScheduler`, +`JobPoller`, `Counters`); caching and rate limiting; observability (`Stats`, `Log`, `Redactor`, +`ErrorStore`); outbound clients (`Http`, `Storage`, `Mailer`); WebSockets; app wiring and +lifecycle (`Brace`, `TestApp`, `DatabaseFactory`). + +**Not covered** (deliberate scope cut, candidates for a follow-up pass): the CLI +(`Cli*`, `BuildCommands`, `ProjectGenerator`, `Toolchains`), `OpsHandler`/`OpsDashboard` +rendering, `JfrProfiler`, and the Flyway migration SQL itself. + +--- + +## High + +- [x] **H1: Per-route stats are keyed by the concrete URL, so the `routes` map grows without bound** + - Severity: High. Files: `BraceHandler.java:422,437,457`; `Stats.java:53-73` (`recordRequest` vs `recordRequestPattern`). + - Every request-recording call site uses `stats.recordRequest(method, path, …)` — the **raw path** + variant. `Stats.recordRequestPattern`, added by the runtime-performance review's H7 fix + *specifically* to bound this map by the route table, has **no caller in `src/main`**; only + `StatsTest` uses it. Its own Javadoc says matched requests go through it. They don't. + `Redactor.redactPath` only collapses high-entropy segments, so ordinary ids and slugs survive: + one `ConcurrentHashMap` entry (plus two `LongAdder`s) per distinct URL ever requested, for the + life of the process. `/ops/routes` degrades from a route table into a URL dump. + - Confirmed: three requests to `/users/{id}` produced + `routeStats().keySet() == [GET /users/1, GET /users/2, GET /users/3, …]`. + - Fix: record the **route pattern** whenever one exists, the raw path only when it doesn't. + 1. Hoist `RouteMatch match` out of the `try` in `handle`, next to the already-hoisted `db`, + `session`, and `csrfOnlySession`. Today it is declared at `:192` inside the `try`, so neither + catch block can see it — which is why all three recording sites take the raw path. + 2. Add a private helper — `recordAndLog(match, method, path, status, durationUs, qc, qu)` — that + calls `stats.recordRequestPattern(method, match.route().pattern(), …)` when `match != null` + and falls back to `stats.recordRequest(method, path, …)` otherwise. The fallback is load- + bearing, not defensive: an exception thrown before `router.match` returns (a malformed + multipart body, for instance) legitimately reaches the catch with no match. + 3. Point all three sites at it: `:422` (success), `:437` (`NotFoundException` — a handler on a + matched route choosing to 404, so it has a pattern), `:457` (500). + 4. `Log.request` keeps the **concrete** (redacted) path. Correcting the spec above: making the + log agree with the routes table would be a real loss — the routes map is a bounded latency + aggregate, but the log is an unbounded stream where the actual URL is the whole diagnostic + value ("`GET /users/{id}` 404'd" is useless without the id). Only stats change. + The redaction split stays: `recordRequest` runs `Redactor.redactPath` because a raw path can + carry a token; `recordRequestPattern` skips it because patterns are code-site literals. + Don't redact patterns — `/reset/{token}` would otherwise be mangled into a different key + than it renders as. + - Not part of this fix, but the two interlock: H2 moves recording to the `writeResult` choke + point, which needs the same hoisted `match`. Landing H1 first is fine — H2 then relocates the + call rather than changing what it records. + - User-visible: `/ops/routes` and the `route` field in request logs change from concrete URLs to + patterns (`GET /users/42` → `GET /users/{id}`). That is what the endpoint always claimed to + show, and it is what makes per-route latency averages meaningful, but it is a visible change to + anything parsing that output — it needs a migration-guide entry. The `routes` map is cumulative + and never reset, so entries accrued before the fix simply age out at the next restart. + - Model: smaller model OK (mechanical), but the test is the point: assert + `stats.routeStats().keySet()` holds exactly one entry after N requests to N distinct ids under + one pattern. This exact fix existed once (perf review H7) and silently reverted, and nothing in + the suite noticed. + - **Resolved as:** the plan above, with two adjustments found while implementing. + (a) `Log.request` keeps the concrete redacted path (see step 4) — the spec's "same value" would + have thrown away the log's diagnostic value to no benefit. + (b) The `match == null` fallback records a constant `(unmatched)` bucket rather than the raw + path. Keying the fallback by path would have left the leak wide open on exactly the input an + attacker controls: every `/` that throws before routing would mint a permanent key. + `Stats.recordRequest` is now unused by the framework; kept public, with its Javadoc corrected + to stop claiming the handler uses it and to warn about key cardinality. + New `RouteStatsKeyTest` covers pattern-keying for 200s, handler-thrown 404s, and 500s; that no + concrete or unmatched URL ever becomes a key; and the pre-routing throw (a malformed + percent-escape in the query string, which `parseQuery` hits before `router.match`) landing in + the unmatched bucket. That last case needs a raw socket — `java.net.URI` rejects `%zz` + client-side, so the JDK HTTP client cannot produce the request. + +- [x] **H2: Every response that short-circuits before the handler is invisible to stats and the request log** + - Severity: High. Files: `BraceHandler.java:210,223,245,266,279,283,329` (early `return true`) vs the + recording sites at `:419-424`, `:431-439`, `:456-461`. + - Stats and `Log.request` run only on the success path and in the two catch blocks. Every other + exit — before-middleware short-circuits (**rate-limiter 429s**, auth redirects), session-middleware + short-circuits, **CSRF 403s**, 413 payload-too-large, static-file serves, and the + **unmatched-route 404** at `:283` — returns without recording anything. The signals most wanted + during an incident are exactly the ones missing, and `/ops/status` under-reports total traffic. + - Confirmed: with a `before("/blocked", …)` returning 429 and a GET to an unregistered path, after + 7 handler requests + 1 blocked + 1 unmatched, `statusCodeCounts()` was `{200=7}` — no 429, no 404. + - Fix: record once at the write-back choke point (`writeResult(result, response, callback, session, + csrfOnlySession)`), which already sees every exit, instead of at each return site. Pass the + matched route pattern (H1) and the db handle through, or capture them in fields on a per-request + context. Keep the existing behavior that a null `Stats` disables both stats and logging. + - Model: frontier (touches the choke point every path funnels through; must not double-count the + success path, and must not start logging static-asset requests without a deliberate decision). + - **Resolved as:** a per-request `Exchange` holder (start time, method, path, match, db, plus + `recorded`/`logged` flags) built before the `try` so the catch paths share it, and + `recordAndLog` moved inside the choke-point `writeResult` overload. All ten exits are now + covered; `recorded` makes it idempotent so no path can double-count. + Three decisions the finding left open: + (a) **Static files are recorded and logged.** The alternative — a request log that silently + omits a class of request — is the same defect one level down, and a static serve that took + 40ms of disk is real latency worth seeing. `Log.level`/`BRACE_LOG_LEVEL` is the volume knob. + (b) **A 500 stays one log line.** The error path already emits `http.error` with the exception + and app frame, so it sets `exchange.logged` and the choke point records stats without a + duplicate `http.request` line. Log shape is unchanged from before the fix; only stats gained. + (c) **Static files get their own `(static)` bucket**, not the `(unmatched)` one, so asset + traffic doesn't inflate the 404 count. Both are constants for the H1 reason: the filename is + client-supplied, so a miss like `/assets/.css` must not mint a key. + New `ShortCircuitStatsTest` covers the 429, the guard redirect, the CSRF 403, the unmatched + 404, static hits and misses, and that a normal response is counted exactly once. + +- [x] **H3: Path parameters are never URL-decoded** + - Severity: High. Files: `BraceHandler.java:174` (`getHttpURI().getPath()`), `Route.java:70-78`, + `BraceHandler.java:569-658` (static files). + - The router matches against Jetty's **raw**, still-percent-encoded path and copies regex groups + straight into `pathParams`. Query params and form params *are* decoded (`Request.scanPairs`), so + the same string round-trips differently depending on where it rides. Any route whose parameter is + not a bare integer — slug, email, filename, tag, title — hands the handler corrupt data, and a + lookup like `db.findBy(User.class, "email", req.pathParam("email"))` silently returns null instead + of failing loudly. Static-file serving has the same defect: `/assets/my%20file.css` is looked up as + a literal `my%20file.css` and 404s. + - Confirmed: `GET /users/John%20Doe` → `req.pathParam("id")` is `John%20Doe`. + - Fix: decode **after** matching, per captured segment (decoding before matching would let `%2F` + forge segment boundaries — that is why the raw path must stay the routing input). Use a + path-segment decoder, **not** `URLDecoder`: `+` is a literal plus in a path, not a space. Keep the + raw path for `Stats`/`Log`/`Redactor` keys. Decode the static-file relative path the same way, + before the `..` and `startsWith(baseDir)` containment checks — and re-verify those checks hold + against decoded input (`%2e%2e%2f` must not escape the base directory). + - Model: frontier (traversal-adjacent; the decode-after-match ordering is the whole correctness + argument, and getting it backwards is a path-traversal hole). + - **Resolved as:** `Request.decodePathSegment` / `Request.decodePath` (deliberately not + `URLDecoder`, which is form decoding: it treats `+` as a space, silently renaming + `/files/a+b`, and *throws* on a malformed escape, which on a request path would turn a stray + `%` into a 500 — the new decoder keeps bad escapes literal, as browsers and mainstream servers + do). `Route.match` decodes each captured group **after** the regex match, so a `%2F` stays + inside the value it was written in. Static files decode **before** the `..` check (decoding + after it would let `%2e%2e` slip past) and before `resolve`/`normalize`/`startsWith`, and + `Assets.currentVersion` now receives the decoded URL path so an encoded filename can still + match its own fingerprint. + - **Correction to the finding's risk framing:** the traversal exposure was smaller than stated. + Jetty's default `UriCompliance` rejects `%2F`, `%25`, `%2e` and malformed escapes with a 400 + before the handler runs, so those inputs never reached the old `..` check either. The + decode-after-match ordering is still the right design — compliance is configurable and + `Route.match` is public API callable directly — but this was a data-correctness bug, not a + live traversal hole. `PathDecodingTest` splits along that seam: HTTP-level tests for what + actually crosses the wire (spaces, UTF-8, `+`, `&`, `=`, `?`, `#`), unit tests for the + encodings Jetty refuses to forward, and a traversal test that asserts 4xx-and-no-leak rather + than pinning which layer said no. + - Left alone deliberately: `req.path()` 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. Matches how Express and friends split it. + +- [x] **H4: A durable job whose process dies mid-execution is stuck "running" forever** + - Severity: High. Files (as reviewed, at `b3409ee`): `JobPoller.java:181-191` (PG claim), `:257` + (H2 claim), `:281-332` (`runJobBody`), `:362-374` (`purgeFinishedJobs`); `Brace.java:988-1011` + (`stop`). + - Both claim paths set `started_at`, and every claim predicate requires `started_at IS NULL`. + `started_at` is reset in exactly one place: `runJobBody`'s retry branch (`:315`). If the JVM dies + between the claim and the terminal mark — deploy, OOM, pod eviction, or plain `Brace.stop()`, + which stops the poller but never waits for in-flight job threads — the row is never reclaimed by + any instance, never fails, never retries, and is never purged (`purgeFinishedJobs` requires + `completed_at`/`failed_at`). Silent permanent work loss; the only symptom is the `/ops` "running" + count creeping up (`getDurableJobStats` counts exactly these rows as running). + - Fix: a stale-claim reaper. Rows with `started_at < now - visibilityTimeout` and no terminal mark + get `started_at = NULL` (retry, respecting `attempts < max_attempts`) or `failed_at` set once + attempts are exhausted. Run it from the poll loop or the existing daily prune. Add a configurable + `jobVisibilityTimeout` (default generously above the slowest expected job — a too-short timeout + double-runs a live job, which is the worse failure). Also make `stop()` drain in-flight job + threads with a bounded wait so clean shutdowns stop creating orphans in the first place. + - Model: frontier (at-least-once semantics, multi-instance interplay with SKIP LOCKED, and a + too-aggressive timeout causes concurrent duplicate execution). + - **Resolved upstream, independently, before this branch touched it** — `96f37a2` ("fix: recover + durable jobs stranded by a dead instance") on `main`, refined by `c9c679d`/`7681c47`. Rechecked + against `ce085c0`: `JobPoller.reclaimStalledJobs` runs the exact two-statement recovery this + finding specified (`attempts < max_attempts` → `started_at = NULL`; `attempts >= max_attempts` → + `failed_at`), the spent attempt deliberately not refunded so repeated stranding exhausts a budget + instead of looping; `Brace.jobLease` (Duration or interval string, default 30m, null/zero + disables) is the configurable timeout; `migration_pg/V16` indexes currently-claimed rows so the + sweep is an index scan. The sweeper runs on its own thread rather than inside `pollLoop` — + better than what this finding proposed, since a poll loop parked in `limiter.acquire()` with + every slot held by a hung job is exactly when a sweep is most needed and would never run. + - **Residual, deliberate, not reopened:** `stop()` still does not join per-job virtual threads, so + an ordinary deploy still strands up to `poolSize/2` jobs per instance — recovery covers them + rather than prevention avoiding them, at a cost of up to `lease + SWEEP_INTERVAL` (~31 min at + the default) before the work reruns. Draining on `stop()` would shrink that window for the + common case; it is a latency improvement, not a correctness one, so it belongs to a perf pass. + - Two lease artifacts worth knowing, both inherent to leases and consistent with `DurableJob`'s + documented at-least-once contract — noted here so a future reviewer doesn't re-file them: + a reclaimed-but-still-live job can write `completed_at` on a row a second runner later marks + `failed_at`, and `getDurableJobStats` then counts that row in both buckets; and a reclaim + overwrites `error`, so a genuine failure message from an earlier attempt is replaced by the + reclaim text. + +--- + +## Medium + +- [x] **M1: Multipart form fields collapse to a single value per name** + - Files: `BraceHandler.java:849,875,882-888`. + - `parseMultipart` accumulates non-file parts into a `LinkedHashMap` (last wins) and + only then re-encodes them into the `&`-joined body that `Request.formParams` re-parses. A + checkbox group or ` — + // used to keep only its LAST value here, so the same submission yielded one value as + // multipart and all of them as x-www-form-urlencoded. The single-value view downstream + // (Request.parseSingleValues) still does last-wins, so formParam(name) is unchanged. + var formBody = new StringBuilder(); var files = new LinkedHashMap>(); try { @@ -872,21 +977,17 @@ private MultipartResult parseMultipart(org.eclipse.jetty.server.Request jettyReq var uploaded = new UploadedFile(fileName, partContentType, bytes); files.computeIfAbsent(name, k -> new ArrayList<>()).add(uploaded); } else { - formParams.put(name, part.getContentAsString(StandardCharsets.UTF_8)); + if (!formBody.isEmpty()) formBody.append('&'); + formBody.append(java.net.URLEncoder.encode(name, StandardCharsets.UTF_8)); + formBody.append('='); + formBody.append(java.net.URLEncoder.encode( + part.getContentAsString(StandardCharsets.UTF_8), StandardCharsets.UTF_8)); } } } finally { parts.close(); } - var formBody = new StringBuilder(); - for (var entry : formParams.entrySet()) { - if (!formBody.isEmpty()) formBody.append('&'); - formBody.append(java.net.URLEncoder.encode(entry.getKey(), StandardCharsets.UTF_8)); - formBody.append('='); - formBody.append(java.net.URLEncoder.encode(entry.getValue(), StandardCharsets.UTF_8)); - } - return new MultipartResult(formBody.toString(), files); } } diff --git a/src/main/java/com/larvalabs/brace/Cache.java b/src/main/java/com/larvalabs/brace/Cache.java index 8d29f72..040e358 100644 --- a/src/main/java/com/larvalabs/brace/Cache.java +++ b/src/main/java/com/larvalabs/brace/Cache.java @@ -105,8 +105,11 @@ public void set(String key, Object value, String ttl, String... tags) { @SuppressWarnings("unchecked") public T getOrSet(String key, String ttl, Supplier supplier) { if (!serializes) { - var computed = ((InMemoryBackend) backend) - .getOrCompute(key, parseTtl(ttl), () -> requireValue(key, supplier.get())); + // M9: through the SPI, not a cast to the built-in backend. getOrSet is the cache call + // the docs recommend most, and casting here meant any third-party non-serializing + // backend hit a ClassCastException on it. + var computed = backend.getOrCompute( + key, parseTtl(ttl), () -> requireValue(key, supplier.get())); if (computed.hit()) hits.increment(); else misses.increment(); return (T) computed.value(); } diff --git a/src/main/java/com/larvalabs/brace/CacheBackend.java b/src/main/java/com/larvalabs/brace/CacheBackend.java index e4c5941..7b0d0e6 100644 --- a/src/main/java/com/larvalabs/brace/CacheBackend.java +++ b/src/main/java/com/larvalabs/brace/CacheBackend.java @@ -60,6 +60,32 @@ default void setObject(String key, Object value, Duration ttl, String[] tags) { throw new UnsupportedOperationException("backend does not store live objects"); } + /** Result of {@link #getOrCompute}: the value, plus whether it was already cached. */ + record Computed(Object value, boolean hit) {} + + /** + * Get-or-compute for a live-object backend, backing {@code Cache.getOrSet} (M9). + * + *

This is on the SPI rather than being a cast to the built-in backend. {@code Cache.getOrSet} + * used to branch on {@link #requiresSerialization()} and then cast the backend to the concrete + * {@code InMemoryBackend} — so any third-party non-serializing backend threw + * {@code ClassCastException} on the cache call the docs recommend most. + * + *

The default is a plain get / compute / set, which is what the serializing path does. The + * built-in in-memory backend overrides it to add per-key single-flight, so concurrent callers + * for the same cold key run the supplier exactly once. That is an optimization, not a contract: + * an implementation is free to leave the default and let concurrent misses each compute. + */ + default Computed getOrCompute(String key, Duration ttl, java.util.function.Supplier supplier) { + Object existing = getObject(key); + if (existing != null) { + return new Computed(existing, true); + } + Object computed = supplier.get(); + setObject(key, computed, ttl, new String[0]); + return new Computed(computed, false); + } + // --- Value-agnostic ops (every backend implements these) --- void delete(String key); diff --git a/src/main/java/com/larvalabs/brace/Database.java b/src/main/java/com/larvalabs/brace/Database.java index 38e5226..3400b48 100644 --- a/src/main/java/com/larvalabs/brace/Database.java +++ b/src/main/java/com/larvalabs/brace/Database.java @@ -307,12 +307,11 @@ public int deleteBy(Class type, String field, Object value) { // --- Raw queries --- - @SuppressWarnings("unchecked") public List hql(String hql, Object... params) { long start = System.nanoTime(); Query query = session.createQuery(convertPositionalParams(hql)); bindParams(query, params); - List result = (List) query.getResultList(); + List result = asRows(query.getResultList()); queryDurationUs += (System.nanoTime() - start) / 1000; queryCount++; return result; @@ -327,15 +326,14 @@ public void sql(String sql, Object... params) { queryCount++; } - @SuppressWarnings("unchecked") public List sqlQuery(String sql, Object... params) { long start = System.nanoTime(); var query = session.createNativeQuery(convertPositionalParams(sql)); bindParams(query, params); - var results = query.getResultList(); + List results = asRows(query.getResultList()); queryDurationUs += (System.nanoTime() - start) / 1000; queryCount++; - return (List) (List) results; + return results; } @SuppressWarnings("unchecked") @@ -485,7 +483,12 @@ private static String scanPositionalParams(String hql) { char c = hql.charAt(i); // ── E-string: E'...' or e'...' — backslash escapes active ────────────────────── - if ((c == 'E' || c == 'e') && i + 1 < n && hql.charAt(i + 1) == '\'') { + // L11: the E must START a token. Without that check the trailing 'E' of any identifier + // or keyword immediately followed by a quote — "... LIKE'%x%'" — opened backslash-escape + // mode, where a literal backslash before the closing quote swallows the terminator and + // every following '?' is mis-numbered. + if ((c == 'E' || c == 'e') && i + 1 < n && hql.charAt(i + 1) == '\'' + && !isIdentifierChar(i > 0 ? hql.charAt(i - 1) : ' ')) { sb.append(c); // emit the E/e prefix i++; sb.append('\''); // emit the opening quote @@ -619,6 +622,33 @@ private static String scanPositionalParams(String hql) { return sb.toString(); } + /** + * Normalize a result list to {@code List} — one array per row (M7). + * + *

Hibernate returns a list of scalars, not arrays, when the select has a single + * item, so {@code hql}/{@code sqlQuery} used to reach their declared type through an unchecked + * cast that was simply false for that shape. The failure landed as a + * {@code ClassCastException} inside the CALLER's {@code for (Object[] row : ...)} loop, with a + * stack trace pointing nowhere near the query. Wrapping here makes the declared type true for + * every query, so a one-column select behaves like any other: + * {@code row[0]} is the value. + * + *

{@code sqlQueryLong} already normalized both shapes; this brings the list accessors in + * line with it. + */ + private static List asRows(List results) { + var rows = new java.util.ArrayList(results.size()); + for (Object row : results) { + rows.add(row instanceof Object[] array ? array : new Object[]{row}); + } + return rows; + } + + /** True for a character that can appear inside a SQL identifier or keyword (see L11). */ + private static boolean isIdentifierChar(char c) { + return Character.isLetterOrDigit(c) || c == '_' || c == '$'; + } + private void bindParams(Query query, Object[] params) { for (int i = 0; i < params.length; i++) { query.setParameter(i + 1, params[i]); diff --git a/src/main/java/com/larvalabs/brace/FormBinder.java b/src/main/java/com/larvalabs/brace/FormBinder.java index 48047ee..48756ed 100644 --- a/src/main/java/com/larvalabs/brace/FormBinder.java +++ b/src/main/java/com/larvalabs/brace/FormBinder.java @@ -127,7 +127,7 @@ private static Object convert(String raw, Class type, String name, Errors err if (type == long.class || type == Long.class) return Long.parseLong(raw); if (type == double.class || type == Double.class) return Double.parseDouble(raw); if (type == float.class || type == Float.class) return Float.parseFloat(raw); - if (type == boolean.class || type == Boolean.class) return Boolean.parseBoolean(raw); + if (type == boolean.class || type == Boolean.class) return parseCheckbox(raw); if (type == java.math.BigDecimal.class) return new java.math.BigDecimal(raw); return raw; } catch (NumberFormatException e) { @@ -140,6 +140,23 @@ private static Object convert(String raw, Class type, String name, Errors err } } + /** + * Truthy values accepted for a {@code boolean} form field (M2). An HTML checkbox submits + * {@code name=on} when checked and nothing at all when unchecked, so + * {@code Boolean.parseBoolean} — true for the literal string "true" and nothing else — bound a + * CHECKED box to {@code false}, making the control silently inert. Absence still means false + * (handled in {@link #convert}'s empty branch), which is what an unchecked box relies on. + * + *

{@code "on"} is the HTML default; {@code "1"}/{@code "yes"}/{@code "checked"} cover the + * common hand-rolled and JSON-ish variants ({@code jsonForm} runs through the same converter, + * where a real JSON {@code true} arrives as {@code "true"}). + */ + private static final Set TRUTHY = Set.of("true", "on", "1", "yes", "checked"); + + private static boolean parseCheckbox(String raw) { + return TRUTHY.contains(raw.trim().toLowerCase()); + } + @SuppressWarnings({"unchecked", "rawtypes"}) private static Object convertEnum(String raw, Class type, String name, Errors errors) { try { diff --git a/src/main/java/com/larvalabs/brace/Http.java b/src/main/java/com/larvalabs/brace/Http.java index d40c5a2..f3bdd25 100644 --- a/src/main/java/com/larvalabs/brace/Http.java +++ b/src/main/java/com/larvalabs/brace/Http.java @@ -15,6 +15,10 @@ public class Http { + /** + * Shared client. {@code followRedirects} is deliberately left at the JDK default + * ({@code NEVER}) — see {@link #fetch()} (L7). + */ private static final HttpClient CLIENT = HttpClient.newBuilder() .connectTimeout(Duration.ofSeconds(10)) .build(); @@ -100,6 +104,21 @@ private HttpRequest buildRequest() { return builder.build(); } + /** + * Send the request and return the response, whatever its status. A 4xx or 5xx + * is a {@link Response} with that {@link Response#status()}, not an exception — check + * {@link Response#ok()} yourself. Only a transport failure (connect refused, timeout, DNS) + * throws. + * + *

Redirects are not followed: a 301/302 comes back as itself, so read + * {@code header("Location")} and re-issue if you want to follow it. This is the JDK's default + * and is kept because silently following a redirect can replay a request body or leak an + * {@code Authorization} header to another host. + * + *

{@link #fetchBytes()} is the exception to the first rule: it throws on a non-2xx, because + * a caller asking for bytes has nowhere to put an error body. (L7 — the asymmetry is + * deliberate but was previously undocumented.) + */ public Response fetch() { try { var httpResponse = CLIENT.send(buildRequest(), HttpResponse.BodyHandlers.ofString()); @@ -112,15 +131,27 @@ public Response fetch() { } } + /** + * Send the request and parse the body as JSON, whatever the status. On a 500 + * returning an HTML error page this fails with a parse error, not an HTTP error — the + * exception message carries the status and body so the cause is visible. Call {@link #fetch()} + * and check {@link Response#ok()} first if you need to distinguish the two. (L7) + */ public T fetchJson(Class type) { var response = fetch(); return response.as(type); } + /** Send the request and return the body as a String, whatever the status. See {@link #fetch()}. */ public String fetchString() { return fetch().body(); } + /** + * Send the request and return the raw body bytes, throwing on a non-2xx status + * — unlike {@link #fetch()} and friends, which hand the status back. A caller asking for bytes + * has nowhere to put an error body, so failing loudly is the useful behavior. (L7) + */ public byte[] fetchBytes() { try { var httpResponse = CLIENT.send(buildRequest(), HttpResponse.BodyHandlers.ofByteArray()); @@ -164,9 +195,29 @@ public Multipart field(String name, byte[] bytes, String filename, String conten public Multipart bearer(String token) { http.bearer(token); return this; } public Multipart timeout(Duration timeout) { http.timeout(timeout); return this; } - public Response fetch() { finalizeBody(); return http.fetch(); } + /** + * Send the request and return the response, whatever its status. A 4xx or 5xx + * is a {@link Response} with that {@link Response#status()}, not an exception — check + * {@link Response#ok()} yourself. Only a transport failure (connect refused, timeout, DNS) + * throws. + * + *

Redirects are not followed: a 301/302 comes back as itself, so read + * {@code header("Location")} and re-issue if you want to follow it. This is the JDK's default + * and is kept because silently following a redirect can replay a request body or leak an + * {@code Authorization} header to another host. + * + *

{@link #fetchBytes()} is the exception to the first rule: it throws on a non-2xx, because + * a caller asking for bytes has nowhere to put an error body. (L7 — the asymmetry is + * deliberate but was previously undocumented.) + */ + public Response fetch() { finalizeBody(); return http.fetch(); } public String fetchString() { finalizeBody(); return http.fetchString(); } - public byte[] fetchBytes() { finalizeBody(); return http.fetchBytes(); } + /** + * Send the request and return the raw body bytes, throwing on a non-2xx status + * — unlike {@link #fetch()} and friends, which hand the status back. A caller asking for bytes + * has nowhere to put an error body, so failing loudly is the useful behavior. (L7) + */ + public byte[] fetchBytes() { finalizeBody(); return http.fetchBytes(); } public T fetchJson(Class type) { finalizeBody(); return http.fetchJson(type); } private void finalizeBody() { @@ -175,13 +226,13 @@ private void finalizeBody() { for (var part : parts) { writeAscii(out, "--" + boundary + "\r\n"); if (part.filename != null) { - writeAscii(out, "Content-Disposition: form-data; name=\"" + part.name - + "\"; filename=\"" + part.filename + "\"\r\n"); + writeAscii(out, "Content-Disposition: form-data; name=\"" + quoted(part.name) + + "\"; filename=\"" + quoted(part.filename) + "\"\r\n"); writeAscii(out, "Content-Type: " + (part.contentType != null ? part.contentType : "application/octet-stream") + "\r\n"); } else { - writeAscii(out, "Content-Disposition: form-data; name=\"" + part.name + "\"\r\n"); + writeAscii(out, "Content-Disposition: form-data; name=\"" + quoted(part.name) + "\"\r\n"); } writeAscii(out, "\r\n"); out.write(part.bytes); @@ -195,6 +246,27 @@ private void finalizeBody() { http.headers.put("Content-Type", "multipart/form-data; boundary=" + boundary); } + /** + * Sanitize a value destined for a quoted {@code Content-Disposition} parameter (L6). + * + *

These headers were built by raw concatenation, and a filename usually comes from + * {@code UploadedFile.filename()} — i.e. from a remote client. A quote corrupted the body + * and a CR/LF injected arbitrary part headers. CR and LF are dropped outright (there is no + * correct escaping for them in a header) and a quote or backslash is backslash-escaped, per + * RFC 6266's quoted-string rules. + */ + private static String quoted(String value) { + if (value == null) return ""; + var out = new StringBuilder(value.length()); + for (int i = 0; i < value.length(); i++) { + char c = value.charAt(i); + if (c == '\r' || c == '\n') continue; + if (c == '"' || c == '\\') out.append('\\'); + out.append(c); + } + return out.toString(); + } + private static void writeAscii(ByteArrayOutputStream out, String s) throws IOException { out.write(s.getBytes(StandardCharsets.US_ASCII)); } diff --git a/src/main/java/com/larvalabs/brace/InMemoryBackend.java b/src/main/java/com/larvalabs/brace/InMemoryBackend.java index 2512b66..c05ff8d 100644 --- a/src/main/java/com/larvalabs/brace/InMemoryBackend.java +++ b/src/main/java/com/larvalabs/brace/InMemoryBackend.java @@ -23,9 +23,6 @@ boolean expired() { } } - /** Result of {@link #getOrCompute}: the value plus whether it was already cached. */ - record Computed(Object value, boolean hit) {} - static final int DEFAULT_MAX_ENTRIES = 10_000; private final ConcurrentHashMap store = new ConcurrentHashMap<>(); @@ -106,7 +103,8 @@ public void setObject(String key, Object value, Duration ttl, String[] tags) { * and the exception propagates to every caller currently awaiting this key (each retries on its * next call). The non-concurrent case is identical to before — throw, cache nothing, retry next time. */ - Computed getOrCompute(String key, Duration ttl, Supplier supplier) { + @Override + public Computed getOrCompute(String key, Duration ttl, Supplier supplier) { var current = store.get(key); if (current != null && !current.expired()) { return new Computed(current.value(), true); diff --git a/src/main/java/com/larvalabs/brace/JobScheduler.java b/src/main/java/com/larvalabs/brace/JobScheduler.java index 194eb79..fd38fd9 100644 --- a/src/main/java/com/larvalabs/brace/JobScheduler.java +++ b/src/main/java/com/larvalabs/brace/JobScheduler.java @@ -20,8 +20,18 @@ public record JobStatus( int failCount, Instant nextRun, String lastMessage ) {} + /** + * {@code dailyAt} is the configured local time for a {@code daily(...)} job, null for an + * interval job. It is what makes DST-correct rescheduling possible (M11): the next run is + * recomputed from the wall clock each time rather than assumed to be exactly 24h later. + */ private record RegisteredJob(String name, String schedule, long periodMs, long initialDelayMs, - Job job, boolean local) {} + Job job, boolean local, LocalTime dailyAt) { + RegisteredJob(String name, String schedule, long periodMs, long initialDelayMs, + Job job, boolean local) { + this(name, schedule, periodMs, initialDelayMs, job, local, null); + } + } private final CopyOnWriteArrayList registeredJobs = new CopyOnWriteArrayList<>(); private final CopyOnWriteArrayList statuses = new CopyOnWriteArrayList<>(); @@ -62,7 +72,8 @@ public void daily(String time, String name, Job job) { LocalTime targetTime = LocalTime.parse(time); long initialDelayMs = computeDelayUntil(targetTime); long periodMs = Duration.ofHours(24).toMillis(); - var rj = new RegisteredJob(name, "daily at " + time, periodMs, initialDelayMs, job, false); + var rj = new RegisteredJob(name, "daily at " + time, periodMs, initialDelayMs, job, false, + targetTime); registeredJobs.add(rj); Instant nextRun = Instant.now().plusMillis(initialDelayMs); statuses.add(new JobStatus(name, "daily at " + time, null, 0, "pending", null, 0, nextRun, null)); @@ -73,12 +84,34 @@ public void daily(String time, String name, Job job) { if (scheduler != null) { final int index = registeredJobs.size() - 1; seedRunRow(name); - scheduler.scheduleAtFixedRate(() -> { - Thread.startVirtualThread(() -> executeJob(index, rj)); - }, rj.initialDelayMs(), rj.periodMs(), TimeUnit.MILLISECONDS); + scheduleDaily(index, rj, rj.initialDelayMs()); } } + /** + * Schedule one firing of a daily job, which re-arms itself from the wall clock afterwards (M11). + * + *

Not {@code scheduleAtFixedRate} with a 24h period: a fixed period is 24h of elapsed time, + * but "daily at 03:00" is a wall-clock statement, and the two diverge at every DST transition. + * The old form drifted an hour — permanently, until restart — and, worse, could lose a day + * outright: cluster dedupe slots on {@code floor(epochMillis / 86_400_000)}, 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, where the second is deduped away and simply never happens. + * + *

Recomputing the delay after each firing costs one {@code ZonedDateTime} per day and makes + * the schedule mean what it says in local time, across DST and across a zone change. + */ + private void scheduleDaily(int index, RegisteredJob rj, long delayMs) { + scheduler.schedule(() -> { + Thread.startVirtualThread(() -> executeJob(index, rj)); + // Re-arm before the body runs to completion — the next run is a function of the clock, + // not of how long this one takes. + if (scheduler != null && !scheduler.isShutdown()) { + scheduleDaily(index, rj, computeDelayUntil(rj.dailyAt())); + } + }, delayMs, TimeUnit.MILLISECONDS); + } + public void start(DatabaseFactory dbFactory) { this.dbFactory = dbFactory; this.scheduler = Executors.newScheduledThreadPool(1); @@ -87,9 +120,13 @@ public void start(DatabaseFactory dbFactory) { var rj = registeredJobs.get(i); final int index = i; if (!rj.local()) seedRunRow(rj.name()); - scheduler.scheduleAtFixedRate(() -> { - Thread.startVirtualThread(() -> executeJob(index, rj)); - }, rj.initialDelayMs(), rj.periodMs(), TimeUnit.MILLISECONDS); + if (rj.dailyAt() != null) { + scheduleDaily(index, rj, rj.initialDelayMs()); + } else { + scheduler.scheduleAtFixedRate(() -> { + Thread.startVirtualThread(() -> executeJob(index, rj)); + }, rj.initialDelayMs(), rj.periodMs(), TimeUnit.MILLISECONDS); + } } } @@ -198,7 +235,7 @@ private void seedRunRow(String name) { * advisory lock. */ private boolean claimRun(RegisteredJob rj) { - long currentSlot = Instant.now().toEpochMilli() / rj.periodMs(); + long currentSlot = currentSlot(rj); // Fast path (M14): a NON-locking read. On a multi-instance cluster the slot is almost always // already claimed — by another instance, or by this one on an earlier tick within the same slot @@ -290,6 +327,29 @@ private boolean claimUnderLock(RegisteredJob rj, long currentSlot) { } } + /** + * The time-slot a run belongs to, for cluster-wide exactly-once dedupe. + * + *

Interval jobs slot on {@code floor(now / period)} — every instance derives the same number + * from wall-clock, so a claim is exactly-once per interval regardless of tick stagger. + * + *

Daily jobs slot on the local calendar day (M11), not + * {@code floor(epochMillis / 86_400_000)} — which is a *UTC* day and therefore does not line up + * with "daily at 03:00" local. The mismatch was not just cosmetic: for a job whose local time + * sits near the UTC-day boundary, a DST shift could put two consecutive runs in the same UTC + * slot, and the second would be deduped away and never happen at all. + * + *

Assumes instances share a time zone, which is also what firing at a local time assumes: + * in mixed zones each instance fires at its own local 03:00, so they were never running at the + * same moment to begin with. Run the fleet in one zone (UTC is the usual choice). + */ + private static long currentSlot(RegisteredJob rj) { + if (rj.dailyAt() != null) { + return java.time.LocalDate.now(ZoneId.systemDefault()).toEpochDay(); + } + return Instant.now().toEpochMilli() / rj.periodMs(); + } + static long parseInterval(String interval) { if (interval == null || interval.length() < 2) { throw new IllegalArgumentException("Invalid interval: " + interval); @@ -301,7 +361,12 @@ static long parseInterval(String interval) { case 's' -> value * 1000; case 'm' -> value * 60 * 1000; case 'h' -> value * 60 * 60 * 1000; - default -> throw new IllegalArgumentException("Unknown time unit: " + unit + " in interval: " + interval); + // L8: 'd' accepted so this grammar matches Cache.parseTtl's. every("1d", ...) used to + // throw while cache.set(k, v, "1d") worked — two grammars behind identical-looking + // duration strings, with no hint which was which. + case 'd' -> value * 24 * 60 * 60 * 1000; + default -> throw new IllegalArgumentException( + "Unknown time unit: " + unit + " in interval: " + interval + " (expected s, m, h, or d)"); }; } diff --git a/src/main/java/com/larvalabs/brace/Log.java b/src/main/java/com/larvalabs/brace/Log.java index ee142aa..0d72b80 100644 --- a/src/main/java/com/larvalabs/brace/Log.java +++ b/src/main/java/com/larvalabs/brace/Log.java @@ -283,7 +283,11 @@ public static void error(String message, Throwable throwable) { entry.put("level", "ERROR"); entry.put("message", message); entry.put("error", throwable.getClass().getSimpleName()); - entry.put("errorMessage", throwable.getMessage()); + // L10: value-shaped redaction, matching the request-path error(method, path, Throwable) + // above. println's redact() pass is NAME-based, and "errorMessage" is not a sensitive- + // looking name, so a raw exception message carrying a bearer token or SQL literal used to + // reach stdout and /ops/logs untouched from this overload only. + entry.put("errorMessage", Redactor.redactMessage(throwable.getMessage())); println(entry); } diff --git a/src/main/java/com/larvalabs/brace/Mailer.java b/src/main/java/com/larvalabs/brace/Mailer.java index 4da52dd..a6415fa 100644 --- a/src/main/java/com/larvalabs/brace/Mailer.java +++ b/src/main/java/com/larvalabs/brace/Mailer.java @@ -118,9 +118,13 @@ private void sendSmtp(EmailBuilder email, String from) { String user = null; String pass = null; if (url.getUserInfo() != null) { + // M10: percent-decode, matching DatabaseFactory.parseDbConfig. A password + // containing '@', '/' or ':' MUST be encoded to survive URI parsing at all, and + // without decoding here it would then authenticate with the literal "%40" — an + // auth failure whose cause is invisible from the error. var parts = url.getUserInfo().split(":", 2); - user = parts[0]; - pass = parts.length > 1 ? parts[1] : null; + user = decodeUserInfo(parts[0]); + pass = parts.length > 1 ? decodeUserInfo(parts[1]) : null; } var props = smtpProperties(host, port, scheme); @@ -188,6 +192,11 @@ Properties smtpProperties(String host, int port, String scheme) { return props; } + /** Percent-decode one half of a URI user-info component. See the call site (M10). */ + private static String decodeUserInfo(String value) { + return java.net.URLDecoder.decode(value, java.nio.charset.StandardCharsets.UTF_8); + } + public record CapturedEmail(String to, String cc, String subject, String text, String html, String from) {} public static class EmailBuilder { diff --git a/src/main/java/com/larvalabs/brace/RateLimiter.java b/src/main/java/com/larvalabs/brace/RateLimiter.java index 625c726..29c49fa 100644 --- a/src/main/java/com/larvalabs/brace/RateLimiter.java +++ b/src/main/java/com/larvalabs/brace/RateLimiter.java @@ -153,11 +153,21 @@ static void useSharedBackend(Counters counters) { sharedCounters = counters; } - /** Revert to per-process counting (test teardown). */ + /** Revert to per-process counting. Called by {@code Brace.stop()} and in test teardown. */ static void disableSharedBackend() { sharedCounters = null; } + /** + * Drop every registered limiter from the ops registry (M5). {@link #ALL} is a process-global + * list that limiters only ever joined, so across app restarts in one JVM — tests, or several + * {@code Brace} instances — {@link #allStats()} kept reporting limiters whose app was long + * gone, with their final counts frozen. Called by {@code Brace.stop()}. + */ + static void forgetLimiters() { + synchronized (ALL) { ALL.clear(); } + } + Result check(Request req) { var rawKey = keyExtractor.apply(req); diff --git a/src/main/java/com/larvalabs/brace/Redactor.java b/src/main/java/com/larvalabs/brace/Redactor.java index 02001e1..002ba76 100644 --- a/src/main/java/com/larvalabs/brace/Redactor.java +++ b/src/main/java/com/larvalabs/brace/Redactor.java @@ -205,23 +205,36 @@ public static String redactMessage(String message) { if (message == null || message.isEmpty()) return message; // Check for JWT at the whole-message level first (message may be just a token) if (JWT_SHAPE.matcher(message).matches()) return PLACEHOLDER; - String[] tokens = MESSAGE_DELIMITERS.split(message, -1); - // Fast path: no token is long enough to be a secret - boolean anyCandidate = false; - for (String t : tokens) { - if (t.length() >= MIN_SECRET_LENGTH) { anyCandidate = true; break; } - } - if (!anyCandidate) return message; - // Replace token by token; rebuild with single spaces as separators so the - // message stays readable. Leading/trailing delimiters produce empty strings - // at the split edges — these are included as empty strings in the output. - var out = new StringBuilder(message.length()); - for (int i = 0; i < tokens.length; i++) { - if (i > 0) out.append(' '); - String t = tokens[i]; - out.append(isSecretShaped(t) ? "[redacted]" : t); + // L12: splice redacted spans into the ORIGINAL string instead of splitting into tokens and + // rejoining with single spaces. The old rebuild replaced every delimiter run — commas, + // colons, brackets, quotes, newlines — with one space, so a Hibernate message like + // `could not execute statement [n/a]; SQL: select ...` lost its punctuation and line + // structure even when nothing in it was actually redacted. This text is what + // `ops_errors.message` stores and `/ops/errors` shows. + var matcher = MESSAGE_DELIMITERS.matcher(message); + StringBuilder out = null; // stays null until something is actually redacted + int cursor = 0; + int tokenStart = 0; + while (true) { + boolean atEnd = !matcher.find(cursor); + int tokenEnd = atEnd ? message.length() : matcher.start(); + if (tokenEnd > tokenStart) { + String token = message.substring(tokenStart, tokenEnd); + if (token.length() >= MIN_SECRET_LENGTH && isSecretShaped(token)) { + if (out == null) out = new StringBuilder(message.length()).append(message, 0, tokenStart); + out.append("[redacted]"); + } else if (out != null) { + out.append(token); + } + } + if (atEnd) break; + // Copy the delimiter run through verbatim — that is the structure being preserved. + if (out != null) out.append(message, matcher.start(), matcher.end()); + cursor = matcher.end(); + tokenStart = cursor; + if (cursor >= message.length()) break; } - return out.toString(); + return out == null ? message : out.toString(); } /** diff --git a/src/main/java/com/larvalabs/brace/Request.java b/src/main/java/com/larvalabs/brace/Request.java index 6aaa78e..6e370e8 100644 --- a/src/main/java/com/larvalabs/brace/Request.java +++ b/src/main/java/com/larvalabs/brace/Request.java @@ -191,6 +191,75 @@ private static void scanPairs(String raw, boolean bareKeyOnLeadingEq, } } + /** + * Percent-decode one URL path segment (H3). + * + *

Deliberately not {@link URLDecoder}, which implements + * {@code application/x-www-form-urlencoded}: there {@code +} means space, but in a path a + * {@code +} is a literal plus, so decoding {@code /files/a+b} with the form decoder silently + * renames the file. {@code URLDecoder} also throws on a malformed escape, which on a + * request path would turn a stray {@code %} into a 500; here an incomplete or non-hex escape + * is kept literally, matching how browsers and every mainstream server treat it. + * + *

Callers must decode per segment, never a whole path at once. A + * {@code %2F} inside one segment decodes to a literal {@code /}, and decoding across + * separators would let it forge a segment boundary — the standard traversal trick. + */ + static String decodePathSegment(String segment) { + if (segment == null || segment.indexOf('%') < 0) { + return segment; // the overwhelmingly common case: nothing to do, no allocation + } + var bytes = new java.io.ByteArrayOutputStream(segment.length()); + int i = 0; + while (i < segment.length()) { + char c = segment.charAt(i); + if (c == '%' && i + 2 < segment.length()) { + int hi = Character.digit(segment.charAt(i + 1), 16); + int lo = Character.digit(segment.charAt(i + 2), 16); + if (hi >= 0 && lo >= 0) { + bytes.write((hi << 4) + lo); + i += 3; + continue; + } + } + if (c < 0x80) { + bytes.write(c); + i++; + } else { + // Non-ASCII shouldn't reach a raw request path, but if it does, re-encode the + // whole run at once so surrogate pairs stay intact. + int start = i; + while (i < segment.length() && segment.charAt(i) >= 0x80) i++; + byte[] encoded = segment.substring(start, i).getBytes(StandardCharsets.UTF_8); + bytes.write(encoded, 0, encoded.length); + } + } + return bytes.toString(StandardCharsets.UTF_8); + } + + /** + * Percent-decode every segment of a {@code /}-separated path, preserving the separators. + * Each segment goes through {@link #decodePathSegment}, so a {@code %2F} decodes to a literal + * {@code /} within its segment and callers must still reject {@code ..} on the + * decoded result before touching the filesystem. + */ + static String decodePath(String path) { + if (path == null || path.indexOf('%') < 0) { + return path; + } + var out = new StringBuilder(path.length()); + int start = 0; + while (true) { + int slash = path.indexOf('/', start); + if (slash < 0) { + out.append(decodePathSegment(path.substring(start))); + return out.toString(); + } + out.append(decodePathSegment(path.substring(start, slash))).append('/'); + start = slash + 1; + } + } + /** Multi-value view of {@link #scanPairs}: keys map to their values in order of appearance. */ static Map> parsePairs(String raw, boolean bareKeyOnLeadingEq) { if (raw == null || raw.isEmpty()) return Map.of(); diff --git a/src/main/java/com/larvalabs/brace/Route.java b/src/main/java/com/larvalabs/brace/Route.java index 7b9a97d..84b3e30 100644 --- a/src/main/java/com/larvalabs/brace/Route.java +++ b/src/main/java/com/larvalabs/brace/Route.java @@ -67,12 +67,22 @@ void setCsrfRequired(boolean required) { this.csrfRequired = required; } + /** + * Match {@code path} (the RAW, still percent-encoded request path) against this route, + * returning the captured parameters or null. + * + *

Matching runs on the raw path and captured values are decoded afterwards (H3). + * The order is the whole safety argument: decoding first would turn a {@code %2F} into a real + * separator, so {@code /files/a%2F..%2Fb} would match a two-segment route and hand a handler + * an escaped path. Decoding after the capture keeps {@code %2F} inside the value it was + * written in, where it is just a character. + */ public Map match(String path) { var matcher = compiledPattern.matcher(path); if (!matcher.matches()) return null; var params = new LinkedHashMap(); for (int i = 0; i < paramNames.size(); i++) { - params.put(paramNames.get(i), matcher.group(i + 1)); + params.put(paramNames.get(i), Request.decodePathSegment(matcher.group(i + 1))); } return params; } diff --git a/src/main/java/com/larvalabs/brace/Router.java b/src/main/java/com/larvalabs/brace/Router.java index 473f987..4e96cce 100644 --- a/src/main/java/com/larvalabs/brace/Router.java +++ b/src/main/java/com/larvalabs/brace/Router.java @@ -41,6 +41,20 @@ private Route register(Route route) { } public RouteMatch match(String method, String path) { + var found = matchExact(method, path); + if (found != null) return found; + // L1: a trailing slash is not a different resource. "/users/" compiled to nothing that + // could match "/users", so a user who typed the trailing slash — or a link that carried + // one — got a bare 404 with no hint. Retry once against the canonical form rather than + // registering two routes or redirecting (a redirect would turn a POST into a GET). + // "/" itself is canonical and is handled by the exact pass above. + if (path.length() > 1 && path.endsWith("/")) { + return matchExact(method, stripTrailingSlashes(path)); + } + return null; + } + + private RouteMatch matchExact(String method, String path) { var route = staticRoutes.get(method + ' ' + path); if (route != null) return new RouteMatch(route, Map.of()); for (var candidate : dynamicRoutes.getOrDefault(method, List.of())) { @@ -50,6 +64,13 @@ public RouteMatch match(String method, String path) { return null; } + /** Drop trailing slashes, keeping at least "/" — {@code "/a//"} and {@code "/a/"} → {@code "/a"}. */ + private static String stripTrailingSlashes(String path) { + int end = path.length(); + while (end > 1 && path.charAt(end - 1) == '/') end--; + return path.substring(0, end); + } + public List routes() { return List.copyOf(routes); } diff --git a/src/main/java/com/larvalabs/brace/Session.java b/src/main/java/com/larvalabs/brace/Session.java index 7e6fc9e..5478267 100644 --- a/src/main/java/com/larvalabs/brace/Session.java +++ b/src/main/java/com/larvalabs/brace/Session.java @@ -120,8 +120,14 @@ public void set(String key, String value) { if (EXPIRY_KEY.equals(key)) { return; } - data.put(key, value); - modified = true; + // L4: only a real change marks the session modified. A no-op write used to force a full + // AES-GCM re-mint, a Set-Cookie, and (via attachSessionCookie) Cache-Control: private on + // the response — so a guard doing an unconditional session.set(...) made every response + // uncacheable and re-issued the cookie on every request. + var previous = data.put(key, value); + if (!java.util.Objects.equals(previous, value)) { + modified = true; + } } /** @@ -143,13 +149,17 @@ public void set(String key, long value) { } public void remove(String key) { - data.remove(key); - modified = true; + // Removing an absent key changes nothing — see the note in set(). (L4) + if (data.remove(key) != null) { + modified = true; + } } public void clear() { - data.clear(); - modified = true; + if (!data.isEmpty()) { + data.clear(); + modified = true; + } } public boolean isModified() { diff --git a/src/main/java/com/larvalabs/brace/SessionOptions.java b/src/main/java/com/larvalabs/brace/SessionOptions.java index 40c5525..5f3ece3 100644 --- a/src/main/java/com/larvalabs/brace/SessionOptions.java +++ b/src/main/java/com/larvalabs/brace/SessionOptions.java @@ -53,11 +53,31 @@ public SessionOptions secure(boolean secure) { } /** - * Set SameSite attribute: "Strict", "Lax", or "None". - * Note: "None" requires Secure=true. + * Set the SameSite attribute: {@code "Strict"}, {@code "Lax"}, or {@code "None"} + * (case-insensitive). Any other value is rejected — it would otherwise be written verbatim into + * the header, where browsers ignore the whole attribute and silently fall back to their own + * default. + * + *

{@code "None"} also sets {@code Secure} (M12). Every current browser rejects + * {@code SameSite=None} without {@code Secure} outright, so the combination doesn't weaken the + * cookie — it discards it, and the symptom (nobody stays logged in) points nowhere near the + * config line. {@link #sameSiteNone()} already did this; the string form silently did not. */ public SessionOptions sameSite(String sameSite) { - this.sameSite = sameSite; + if (sameSite == null) { + throw new IllegalArgumentException("sameSite must be one of: Strict, Lax, None"); + } + var normalized = switch (sameSite.trim().toLowerCase()) { + case "strict" -> "Strict"; + case "lax" -> "Lax"; + case "none" -> "None"; + default -> throw new IllegalArgumentException( + "Invalid SameSite value: \"" + sameSite + "\". Must be Strict, Lax, or None."); + }; + this.sameSite = normalized; + if (normalized.equals("None")) { + this.secure = true; // SameSite=None without Secure is rejected by browsers + } return this; } diff --git a/src/main/java/com/larvalabs/brace/Stats.java b/src/main/java/com/larvalabs/brace/Stats.java index 642c8bc..2bff015 100644 --- a/src/main/java/com/larvalabs/brace/Stats.java +++ b/src/main/java/com/larvalabs/brace/Stats.java @@ -53,8 +53,14 @@ public class Stats { /** * Records a request against a raw URL path. The path is redacted (secrets must never * reach /ops/status) but remains concrete — {@code /users/1} and {@code /users/2} are - * distinct keys — so this is only for requests with no matched route. Matched requests - * go through {@link #recordRequestPattern} which is bounded by the route table (H7). + * distinct keys, so every distinct URL mints a permanent entry in the never-reset + * {@code routes} map. + * + *

The framework does not use this. {@code BraceHandler} routes every + * request — matched or not — through {@link #recordRequestPattern}, which is bounded by the + * route table (H7, re-broken and re-fixed as correctness H1). It stays public for apps that + * want to record their own synthetic entries and can vouch for the key's cardinality; if the + * path is at all user-influenced, use {@link #recordRequestPattern} with a constant instead. */ public void recordRequest(String method, String path, int status, long latencyUs, int queryCount, long queryUs) { diff --git a/src/main/java/com/larvalabs/brace/Storage.java b/src/main/java/com/larvalabs/brace/Storage.java index e82e9a2..6f4f39f 100644 --- a/src/main/java/com/larvalabs/brace/Storage.java +++ b/src/main/java/com/larvalabs/brace/Storage.java @@ -3,7 +3,6 @@ import javax.crypto.Mac; import javax.crypto.spec.SecretKeySpec; import java.net.URLDecoder; -import java.net.URLEncoder; import java.net.http.HttpClient; import java.net.http.HttpRequest; import java.net.http.HttpResponse; @@ -320,11 +319,25 @@ private String canonicalUri(String key) { return "/" + encoded; } + /** + * URI-encode an object key for both the request URL and the SigV4 canonical request (L5). + * + *

Not {@link URLEncoder}, which is {@code application/x-www-form-urlencoded} and diverges + * from SigV4's required set in both directions: it leaves {@code *} literal where SigV4 wants + * {@code %2A}, and encodes {@code ~} as {@code %7E} where SigV4 wants it literal. A key + * containing either character produced a canonical request that did not match what S3 + * recomputed, i.e. {@code SignatureDoesNotMatch}. (The default {@link #safeKey} path — UUID + * plus an alphanumeric extension — never hits it, so this only reached callers passing their + * own keys.) + * + *

SigV4's unreserved set is exactly RFC 3986's: {@code A-Za-z0-9-._~}. Everything else is + * percent-encoded over its UTF-8 bytes, with {@code /} preserved as the separator. + */ static String uriEncodePath(String key) { var sb = new StringBuilder(); for (var segment : key.split("/", -1)) { if (!sb.isEmpty()) sb.append("/"); - sb.append(URLEncoder.encode(segment, StandardCharsets.UTF_8).replace("+", "%20")); + sb.append(Url.encodeSegment(segment)); } return sb.toString(); } diff --git a/src/main/java/com/larvalabs/brace/TestApp.java b/src/main/java/com/larvalabs/brace/TestApp.java index 6403ace..c1db212 100644 --- a/src/main/java/com/larvalabs/brace/TestApp.java +++ b/src/main/java/com/larvalabs/brace/TestApp.java @@ -248,11 +248,13 @@ public void resetDatabase() { db.beginTransaction(); try { db.sql("SET REFERENTIAL_INTEGRITY FALSE"); - @SuppressWarnings("unchecked") - var tables = (java.util.List) (java.util.List) db.sqlQuery( + // One column per row, read as row[0] (M7). This used to cast the result to + // List and call toString() on each element, which worked only because + // sqlQuery's declared List was a lie for single-column selects. + var tables = db.sqlQuery( "SELECT TABLE_NAME FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_SCHEMA = 'PUBLIC'"); for (var row : tables) { - String tableName = row.toString(); + String tableName = String.valueOf(row[0]); if (!tableName.toLowerCase().startsWith("flyway_")) { db.sql("TRUNCATE TABLE " + tableName); } diff --git a/src/main/java/com/larvalabs/brace/TrustedProxies.java b/src/main/java/com/larvalabs/brace/TrustedProxies.java index 2953c87..57747e0 100644 --- a/src/main/java/com/larvalabs/brace/TrustedProxies.java +++ b/src/main/java/com/larvalabs/brace/TrustedProxies.java @@ -119,6 +119,15 @@ static CidrRange parse(String cidr) { var parts = cidr.split("/"); var addr = InetAddress.getByName(parts[0]); var prefix = Integer.parseInt(parts[1]); + // L9: bound the prefix. createMask(-1, 4) produced an all-zero mask, which matches + // EVERY address — so a typo like "10.0.0.0/-1" silently became "trust every + // forwarding header", the exact opposite of what the caller was configuring. An + // over-wide prefix was silently clamped instead of reported. + int maxPrefix = addr.getAddress().length * 8; + if (prefix < 0 || prefix > maxPrefix) { + throw new IllegalArgumentException("Prefix length must be 0.." + maxPrefix + + " for " + addr.getHostAddress() + ", got /" + prefix); + } return new CidrRange(addr, prefix); } catch (Exception e) { throw new IllegalArgumentException("Invalid CIDR: " + cidr, e); diff --git a/src/main/java/com/larvalabs/brace/Url.java b/src/main/java/com/larvalabs/brace/Url.java index 6bcbb6e..241011a 100644 --- a/src/main/java/com/larvalabs/brace/Url.java +++ b/src/main/java/com/larvalabs/brace/Url.java @@ -1,9 +1,17 @@ package com.larvalabs.brace; +import java.nio.charset.StandardCharsets; + /** * URL generation from route patterns. *

* Usage: {@code Url.to("/users/{id}", 42)} → {@code "/users/42"} + *

+ * Substituted values are percent-encoded for a path segment, so a value containing {@code /}, + * {@code ?}, {@code #}, {@code &} or a space produces a valid URL that routes back to the same + * value: {@code Url.to("/tags/{name}", "a/b")} → {@code "/tags/a%2Fb"}, which + * {@code req.pathParam("name")} reads back as {@code "a/b"}. The literal segments of the pattern + * are emitted as written — they are code, not data. */ public class Url { @@ -19,7 +27,7 @@ public static String to(String pattern, Object... params) { throw new IllegalArgumentException("Not enough params for pattern: " + pattern + " (expected param for " + part + ")"); } - result.append(params[paramIndex++]); + result.append(encodeSegment(String.valueOf(params[paramIndex++]))); } else { result.append(part); } @@ -27,4 +35,44 @@ public static String to(String pattern, Object... params) { if (result.isEmpty()) result.append("/"); return result.toString(); } + + /** + * Percent-encode one path segment (M6). + * + *

Deliberately not {@link java.net.URLEncoder}, which is + * {@code application/x-www-form-urlencoded}: it encodes a space as {@code +}, and in a path a + * {@code +} is a literal plus, so the value would not survive the round trip back through + * {@link Request#decodePathSegment}. This applies the RFC 3986 rule instead — the unreserved + * set {@code A-Za-z0-9-._~} passes through, everything else becomes {@code %XX} over its UTF-8 + * bytes — making it the exact inverse of the decoder used on the way in. + */ + static String encodeSegment(String value) { + if (value == null || value.isEmpty()) return ""; + boolean needsEncoding = false; + for (int i = 0; i < value.length(); i++) { + if (!isUnreserved(value.charAt(i))) { + needsEncoding = true; + break; + } + } + if (!needsEncoding) return value; // the common case: ids and slugs, no allocation + var out = new StringBuilder(value.length() + 8); + for (byte b : value.getBytes(StandardCharsets.UTF_8)) { + char c = (char) (b & 0xFF); + if (isUnreserved(c)) { + out.append(c); + } else { + out.append('%') + .append(Character.toUpperCase(Character.forDigit((b >> 4) & 0xF, 16))) + .append(Character.toUpperCase(Character.forDigit(b & 0xF, 16))); + } + } + return out.toString(); + } + + /** The RFC 3986 unreserved set: never percent-encoded, and never decoded to anything else. */ + private static boolean isUnreserved(char c) { + return (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z') || (c >= '0' && c <= '9') + || c == '-' || c == '.' || c == '_' || c == '~'; + } } diff --git a/src/main/java/com/larvalabs/brace/View.java b/src/main/java/com/larvalabs/brace/View.java index 7ae5a09..1f84cb2 100644 --- a/src/main/java/com/larvalabs/brace/View.java +++ b/src/main/java/com/larvalabs/brace/View.java @@ -46,10 +46,7 @@ static void setEngine(TemplateEngine engine) { } public static View of(String template, Object... keyValues) { - var params = new LinkedHashMap(); - for (int i = 0; i < keyValues.length - 1; i += 2) { - params.put((String) keyValues[i], keyValues[i + 1]); - } + var params = toParams(template, keyValues); // Resolve the request-scoped CSRF field and flash NOW, while their ThreadLocals are still set // (the handler is mid-flight). getCsrfField() mints the token, so the session is marked modified // here — before the cookie-write decision — exactly as it was when rendering happened eagerly. @@ -93,16 +90,34 @@ void materialize() { } public static String render(String template, Object... keyValues) { - var params = new LinkedHashMap(); - for (int i = 0; i < keyValues.length - 1; i += 2) { - params.put((String) keyValues[i], keyValues[i + 1]); - } + var params = toParams(template, keyValues); if (engine != null) { return engine.render(template, params); } return "[Template: " + template + " | Params: " + params.keySet() + "]"; } + /** + * Build the parameter map from alternating key/value varargs, rejecting an odd count (L3). + * + *

The loop used to stop at {@code length - 1}, silently discarding a trailing key: a typo'd + * {@code View.of("page", "a", 1, "b")} rendered a template missing {@code b} with no error + * anywhere, and the failure surfaced as a blank spot in the page. {@code Session.of} has always + * thrown on an odd count; this makes the two agree. + */ + private static LinkedHashMap toParams(String template, Object... keyValues) { + if (keyValues.length % 2 != 0) { + throw new IllegalArgumentException( + "View params for \"" + template + "\" must be key-value pairs, but got " + + keyValues.length + " arguments (trailing key: " + keyValues[keyValues.length - 1] + ")"); + } + var params = new LinkedHashMap(); + for (int i = 0; i < keyValues.length; i += 2) { + params.put((String) keyValues[i], keyValues[i + 1]); + } + return params; + } + public String template() { return template; } public Map params() { return params; } } diff --git a/src/main/java/com/larvalabs/brace/WsContext.java b/src/main/java/com/larvalabs/brace/WsContext.java index 7c92eaa..ea00284 100644 --- a/src/main/java/com/larvalabs/brace/WsContext.java +++ b/src/main/java/com/larvalabs/brace/WsContext.java @@ -57,12 +57,22 @@ public void send(String message) { return; } queuedBytes.addAndGet(size); - jettySession.sendText(message, Callback.from( - () -> queuedBytes.addAndGet(-size), - failure -> { - queuedBytes.addAndGet(-size); - closed.set(true); // broken connection — stop sending; Jetty fires onClose/onError → cleanup - })); + try { + jettySession.sendText(message, Callback.from( + () -> queuedBytes.addAndGet(-size), + failure -> { + queuedBytes.addAndGet(-size); + closed.set(true); // broken connection — stop sending; Jetty fires onClose/onError → cleanup + })); + } catch (RuntimeException e) { + // M8: a synchronous throw (e.g. sending on a session Jetty already closed) never + // reaches the callback, so without this the reservation above is never released. The + // connection would then carry a permanent phantom backlog and eventually be + // force-closed as a "slow consumer" it never was. + queuedBytes.addAndGet(-size); + closed.set(true); + throw e; + } } /** diff --git a/src/main/java/com/larvalabs/brace/WsRegistry.java b/src/main/java/com/larvalabs/brace/WsRegistry.java index af92895..d603c99 100644 --- a/src/main/java/com/larvalabs/brace/WsRegistry.java +++ b/src/main/java/com/larvalabs/brace/WsRegistry.java @@ -58,12 +58,24 @@ void broadcast(String room, String message) { bus.publish(room, message); } - /** Deliver a message to members connected to THIS instance. Invoked by the {@link MessageBus}. */ + /** + * Deliver a message to members connected to THIS instance. Invoked by the {@link MessageBus}. + * + *

M8: each send is isolated. The loop used to be unguarded, so anything thrown for one + * member — a session Jetty has already closed, say — aborted delivery to every remaining + * member, and one bad connection silently dropped the broadcast for the whole room. A failing + * member is logged and skipped; its own close/error callback handles cleanup. + */ private void deliverLocal(String room, String message) { var members = rooms.get(room); - if (members != null) { - for (var ctx : members) { + if (members == null) { + return; + } + for (var ctx : members) { + try { ctx.send(message); + } catch (RuntimeException e) { + Log.warn("ws-broadcast-send-failed room=" + room + " error=" + e); } } } diff --git a/src/test/java/com/larvalabs/brace/CheckboxAndVaryTest.java b/src/test/java/com/larvalabs/brace/CheckboxAndVaryTest.java new file mode 100644 index 0000000..91c31a9 --- /dev/null +++ b/src/test/java/com/larvalabs/brace/CheckboxAndVaryTest.java @@ -0,0 +1,112 @@ +package com.larvalabs.brace; + +import com.larvalabs.brace.annotation.Required; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Correctness review M2 (HTML checkboxes bind to {@code boolean}) and M3 (the framework's + * {@code Vary: HX-Request} appends rather than clobbering). + */ +class CheckboxAndVaryTest { + + record Signup(@Required String email, boolean agree, boolean newsletter) {} + + static TestApp app; + + @BeforeAll + static void setup() throws Exception { + app = Brace.test().start(a -> { + a.post("/signup", req -> { + var form = req.form(Signup.class); + return Result.text(form.value().agree() + "/" + form.value().newsletter()); + }).csrf(false); + a.get("/vary-none", req -> Result.text("x")); + a.get("/vary-one", req -> Result.text("x").header("Vary", "Accept-Encoding")); + a.get("/vary-many", req -> + Result.text("x").header("Vary", "Accept-Encoding, Accept-Language")); + a.get("/vary-already", req -> Result.text("x").header("Vary", "HX-Request")); + a.get("/vary-already-cased", req -> Result.text("x").header("Vary", "hx-request")); + }); + } + + @AfterAll + static void teardown() throws Exception { + app.stop(); + } + + // --- M2: checkbox binding --- + + @Test + void checkedCheckboxBindsTrue() { + // This is what a browser actually submits for . + assertEquals("true/false", post("email=a@b.com&agree=on")); + } + + @Test + void absentCheckboxBindsFalse() { + assertEquals("false/false", post("email=a@b.com")); + } + + @Test + void otherTruthySpellingsBind() { + assertEquals("true/true", post("email=a@b.com&agree=true&newsletter=1")); + assertEquals("true/true", post("email=a@b.com&agree=yes&newsletter=checked")); + assertEquals("true/true", post("email=a@b.com&agree=ON&newsletter=True")); + } + + @Test + void nonTruthyValuesBindFalse() { + assertEquals("false/false", post("email=a@b.com&agree=off&newsletter=0")); + assertEquals("false/false", post("email=a@b.com&agree=no&newsletter=false")); + } + + private static String post(String body) { + return app.request("POST", "/signup") + .body(body, "application/x-www-form-urlencoded").send().body(); + } + + // --- M3: Vary --- + + @Test + void varyIsSetWhenTheHandlerDeclaredNone() { + assertEquals("HX-Request", vary("/vary-none")); + } + + @Test + void varyPreservesTheHandlersOwnDimension() { + assertEquals("Accept-Encoding, HX-Request", vary("/vary-one")); + } + + @Test + void varyPreservesEveryExistingDimension() { + assertEquals("Accept-Encoding, Accept-Language, HX-Request", vary("/vary-many")); + } + + @Test + void varyIsNotDuplicatedWhenAlreadyDeclared() { + assertEquals("HX-Request", vary("/vary-already")); + assertEquals("hx-request", vary("/vary-already-cased")); + } + + @Test + void varyIsUntouchedForNonHtmxRequests() { + var res = app.get("/vary-one"); + assertEquals("Accept-Encoding", res.header("Vary")); + assertFalse(String.valueOf(res.header("Vary")).contains("HX-Request")); + } + + @Test + void varyStillCoversHtmxOnEveryPath() { + assertTrue(vary("/vary-many").contains("HX-Request")); + } + + private static String vary(String path) { + return app.request("GET", path).header("HX-Request", "true").send().header("Vary"); + } +} diff --git a/src/test/java/com/larvalabs/brace/CustomCacheBackendTest.java b/src/test/java/com/larvalabs/brace/CustomCacheBackendTest.java new file mode 100644 index 0000000..2f3b822 --- /dev/null +++ b/src/test/java/com/larvalabs/brace/CustomCacheBackendTest.java @@ -0,0 +1,106 @@ +package com.larvalabs.brace; + +import org.junit.jupiter.api.Test; + +import java.time.Duration; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicInteger; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; + +/** + * Correctness review M9: {@code CacheBackend} is a public SPI, so every {@code Cache} operation + * must work through it. {@code getOrSet} used to branch on {@code requiresSerialization()} and + * then cast the backend to the concrete built-in {@code InMemoryBackend}, so a third-party + * live-object backend threw {@code ClassCastException} on the call the docs recommend most. + */ +class CustomCacheBackendTest { + + /** A minimal third-party live-object backend — implements the SPI and nothing else. */ + static final class SimpleBackend implements CacheBackend { + final Map store = new ConcurrentHashMap<>(); + + @Override public boolean requiresSerialization() { return false; } + @Override public Object getObject(String key) { return store.get(key); } + @Override public void setObject(String key, Object value, Duration ttl, String[] tags) { + store.put(key, value); + } + @Override public void delete(String key) { store.remove(key); } + @Override public void deletePrefix(String prefix) { + store.keySet().removeIf(k -> k.startsWith(prefix)); + } + @Override public long clearTag(String tag) { return 0; } + @Override public void clear() { store.clear(); } + @Override public long incr(String key, long delta) { return delta; } + @Override public int size() { return store.size(); } + @Override public long evictExpired() { return 0; } + } + + @Test + void getOrSetWorksOnAThirdPartyLiveObjectBackend() { + var cache = new Cache(new SimpleBackend()); + try { + var computeCount = new AtomicInteger(); + String first = cache.getOrSet("k", "1m", () -> { + computeCount.incrementAndGet(); + return "computed"; + }); + String second = cache.getOrSet("k", "1m", () -> { + computeCount.incrementAndGet(); + return "recomputed"; + }); + + assertEquals("computed", first); + assertEquals("computed", second, "second call must hit the cache"); + assertEquals(1, computeCount.get(), "the supplier must run once"); + assertEquals(1, cache.hits()); + assertEquals(1, cache.misses()); + } finally { + cache.close(); + } + } + + @Test + void theRestOfTheFacadeAlsoWorksThroughTheSpi() { + var cache = new Cache(new SimpleBackend()); + try { + cache.set("a", "1"); + assertEquals("1", cache.get("a", String.class)); + cache.delete("a"); + assertNull(cache.get("a", String.class)); + } finally { + cache.close(); + } + } + + @Test + void builtInBackendStillSingleFlightsConcurrentMisses() throws Exception { + var cache = new Cache(); + try { + var computeCount = new AtomicInteger(); + var threads = new Thread[8]; + var results = new String[threads.length]; + for (int i = 0; i < threads.length; i++) { + final int idx = i; + threads[i] = Thread.ofVirtual().unstarted(() -> + results[idx] = cache.getOrSet("cold", "1m", () -> { + computeCount.incrementAndGet(); + try { Thread.sleep(50); } catch (InterruptedException ignored) {} + return "once"; + })); + } + for (var t : threads) t.start(); + for (var t : threads) t.join(); + + assertEquals(1, computeCount.get(), + "the in-memory backend's single-flight must survive the move to the SPI"); + for (var r : results) { + assertEquals("once", r); + } + } finally { + cache.close(); + } + } +} diff --git a/src/test/java/com/larvalabs/brace/DurableJobTest.java b/src/test/java/com/larvalabs/brace/DurableJobTest.java index b123216..ac6532d 100644 --- a/src/test/java/com/larvalabs/brace/DurableJobTest.java +++ b/src/test/java/com/larvalabs/brace/DurableJobTest.java @@ -594,10 +594,20 @@ void jobPollIntervalMustBePositive() { @Test void jobLeaseRejectsMalformedIntervals() { assertThrows(IllegalArgumentException.class, () -> Brace.app().jobLease("15")); - assertThrows(IllegalArgumentException.class, () -> Brace.app().jobLease("15d")); + // "15y" stands in for what "15d" used to test. Correctness review L8 added 'd' to the + // interval grammar so it matches Cache.parseTtl's — this test was pinning the absence of + // the unit rather than a deliberate rejection of day-length leases, which are a perfectly + // sensible thing to express. + assertThrows(IllegalArgumentException.class, () -> Brace.app().jobLease("15y")); assertThrows(NumberFormatException.class, () -> Brace.app().jobLease("abcm")); } + @Test + void jobLeaseAcceptsDayIntervals() { + assertEquals(Duration.ofDays(15).toMillis(), + JobScheduler.parseInterval("15d")); + } + @Test void reclaimingStrandedParentUnblocksItsDependents() { long parent = scheduleJob(new TestJob("parent"), new JobOptions()); diff --git a/src/test/java/com/larvalabs/brace/MultiValueParamsTest.java b/src/test/java/com/larvalabs/brace/MultiValueParamsTest.java index 2e58c1c..8cb71ea 100644 --- a/src/test/java/com/larvalabs/brace/MultiValueParamsTest.java +++ b/src/test/java/com/larvalabs/brace/MultiValueParamsTest.java @@ -79,6 +79,50 @@ void formParamsAbsentIsEmptyList() { assertEquals("|null", res.body()); } + // --- Multipart (correctness review M1) --- + + /** + * A repeated field must survive multipart parsing. The parser used to accumulate non-file + * parts into a {@code Map} before re-encoding them, so a checkbox group + * submitted as multipart kept only its last value while the byte-identical urlencoded + * submission kept all of them. + */ + @Test + void multipartFormParamsReturnsAllValuesInOrder() { + assertEquals("a,b,c|c", multipartPost("tag", "a", "tag", "b", "tag", "c")); + } + + @Test + void multipartSingleValueIsUnchanged() { + assertEquals("only|only", multipartPost("tag", "only")); + } + + @Test + void multipartValuesNeedingEncodingRoundTrip() { + assertEquals("a&b,c=d,e f|e f", multipartPost("tag", "a&b", "tag", "c=d", "tag", "e f")); + } + + @Test + void multipartInterleavedFieldsKeepPerNameOrder() { + assertEquals("a,b|b", multipartPost("tag", "a", "other", "x", "tag", "b")); + } + + /** POST the given name/value pairs as {@code multipart/form-data}, returning "/f"'s body. */ + private static String multipartPost(String... namesAndValues) { + String boundary = "----braceMultiValueTest"; + var body = new StringBuilder(); + for (int i = 0; i < namesAndValues.length; i += 2) { + body.append("--").append(boundary).append("\r\n") + .append("Content-Disposition: form-data; name=\"").append(namesAndValues[i]) + .append("\"\r\n\r\n") + .append(namesAndValues[i + 1]).append("\r\n"); + } + body.append("--").append(boundary).append("--\r\n"); + return app.request("POST", "/f") + .body(body.toString(), "multipart/form-data; boundary=" + boundary) + .send().body(); + } + // --- Unit-level: hand-constructed Request (no raw query string available) --- @Test diff --git a/src/test/java/com/larvalabs/brace/PathDecodingTest.java b/src/test/java/com/larvalabs/brace/PathDecodingTest.java new file mode 100644 index 0000000..060a5e9 --- /dev/null +++ b/src/test/java/com/larvalabs/brace/PathDecodingTest.java @@ -0,0 +1,166 @@ +package com.larvalabs.brace; + +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +import java.nio.file.Files; +import java.nio.file.Path; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Correctness review H3: path parameters and static-file paths are percent-decoded, so a value + * round-trips the same whether it rides in the path, the query string, or a form body. Before + * this fix the router matched — and handed handlers — the raw encoded path, so + * {@code /users/John%20Doe} yielded the literal {@code "John%20Doe"} and any lookup on it + * silently missed. + * + *

Two layers, tested separately. Jetty's default {@code UriCompliance} + * rejects the ambiguous encodings — {@code %2F} (separator), {@code %25} (encoding), {@code %2e} + * (segment), and malformed escapes — with a 400 before any of this code runs. So the decoder's + * behavior for those inputs is exercised as a unit test, not over HTTP: it is defense in depth + * (compliance is configurable, and {@code Route.match} is public API callable directly), and the + * HTTP tests assert what actually crosses the wire. + */ +class PathDecodingTest { + + static TestApp app; + static Path assetDir; + + @BeforeAll + static void setup() throws Exception { + assetDir = Files.createTempDirectory("brace-h3-assets"); + Files.writeString(assetDir.resolve("my file.css"), "body{color:red}"); + Files.writeString(assetDir.resolve("plain.css"), "body{}"); + Files.createDirectory(assetDir.resolve("sub")); + Files.writeString(assetDir.resolve("sub").resolve("nested.css"), "a{}"); + // A file OUTSIDE the mapped directory, as a traversal target. + Files.writeString(assetDir.getParent().resolve("brace-h3-secret.txt"), "secret"); + + app = Brace.test().start(a -> { + a.staticFiles("/assets", assetDir.toString()); + a.get("/users/{name}", req -> Result.text(req.pathParam("name"))); + a.get("/echo/{a}/{b}", req -> Result.text(req.pathParam("a") + "|" + req.pathParam("b"))); + }); + } + + @AfterAll + static void teardown() throws Exception { + app.stop(); + } + + // --- Over the wire --- + + @Test + void spacesAreDecoded() { + assertEquals("John Doe", app.get("/users/John%20Doe").body()); + } + + @Test + void plusIsALiteralPlusNotASpace() { + // The form decoder would give "a b" here. In a path, "+" is just a plus. + assertEquals("a+b", app.get("/users/a+b").body()); + } + + @Test + void nonAsciiRoundTripsAsUtf8() { + assertEquals("café", app.get("/users/caf%C3%A9").body()); + assertEquals("日本", app.get("/users/%E6%97%A5%E6%9C%AC").body()); + } + + @Test + void reservedCharactersInAValueSurvive() { + assertEquals("a&b=c", app.get("/users/a%26b%3Dc").body()); + assertEquals("a?b#c", app.get("/users/a%3Fb%23c").body()); + } + + @Test + void multipleParametersAreEachDecoded() { + assertEquals("a b|c d", app.get("/echo/a%20b/c%20d").body()); + } + + @Test + void pathParamNowAgreesWithQueryAndFormDecodingForTheSameValue() { + // The point of the fix: one value, three carriers, one result. + String viaPath = app.get("/users/John%20Doe").body(); + assertEquals("John Doe", viaPath); + assertNotEquals("John%20Doe", viaPath); + } + + // --- Static files --- + + @Test + void staticFilesWithEncodedNamesResolve() { + var res = app.get("/assets/my%20file.css"); + assertEquals(200, res.status()); + assertEquals("body{color:red}", res.body()); + } + + @Test + void staticFilesWithoutEncodingStillResolve() { + assertEquals(200, app.get("/assets/plain.css").status()); + assertEquals(200, app.get("/assets/sub/nested.css").status()); + } + + @Test + void traversalIsRejectedAndNeverServesTheTarget() { + // Plain ".." is ours to reject; the encoded forms are stopped by Jetty's UriCompliance + // with a 400 before we see them. Either way the status is 4xx and the file never leaks — + // that is what this asserts, rather than pinning which layer said no. + for (String attempt : new String[]{ + "/assets/../brace-h3-secret.txt", + "/assets/%2e%2e/brace-h3-secret.txt", + "/assets/..%2Fbrace-h3-secret.txt", + "/assets/sub/%2e%2e/%2e%2e/brace-h3-secret.txt", + "/assets/%2E%2E%2F%2E%2E%2Fetc/passwd"}) { + var res = app.get(attempt); + assertTrue(res.status() >= 400, attempt + " must not succeed, got " + res.status()); + assertFalse(res.body().contains("secret"), attempt + " leaked the target file"); + } + } + + // --- Decoder unit tests (inputs Jetty's compliance layer refuses to forward) --- + + @Test + void decoderKeepsEncodedSlashInsideItsSegment() { + // %2F decodes to a literal "/" in the VALUE. Because Route.match decodes after the regex + // capture, it can never act as a segment separator and forge a path boundary. + assertEquals("a/b", Request.decodePathSegment("a%2Fb")); + assertEquals("a/b", Request.decodePathSegment("a%2fb")); + } + + @Test + void decoderHandlesEncodedPercent() { + assertEquals("100%", Request.decodePathSegment("100%25")); + assertEquals("%20", Request.decodePathSegment("%2520")); + } + + @Test + void decoderKeepsMalformedEscapesLiterallyInsteadOfThrowing() { + // URLDecoder throws on all of these; on a request path that would be a 500 for a stray '%'. + assertEquals("a%zb", Request.decodePathSegment("a%zb")); + assertEquals("trailing%", Request.decodePathSegment("trailing%")); + assertEquals("half%4", Request.decodePathSegment("half%4")); + assertEquals("%", Request.decodePathSegment("%")); + } + + @Test + void decoderLeavesPlusAndUnencodedTextAlone() { + assertEquals("a+b", Request.decodePathSegment("a+b")); + assertEquals("plain", Request.decodePathSegment("plain")); + assertEquals("", Request.decodePathSegment("")); + } + + @Test + void decodePathPreservesSeparatorsAndDecodesEachSegment() { + assertEquals("a b/c d", Request.decodePath("a%20b/c%20d")); + assertEquals("sub/my file.css", Request.decodePath("sub/my%20file.css")); + // A %2F inside a segment decodes to "/" — which is exactly why the static-file path + // re-checks ".." on the decoded result before it touches the filesystem. + assertEquals("a/../b", Request.decodePath("a%2F..%2Fb")); + } +} diff --git a/src/test/java/com/larvalabs/brace/RedactMessageStructureTest.java b/src/test/java/com/larvalabs/brace/RedactMessageStructureTest.java new file mode 100644 index 0000000..53b829b --- /dev/null +++ b/src/test/java/com/larvalabs/brace/RedactMessageStructureTest.java @@ -0,0 +1,75 @@ +package com.larvalabs.brace; + +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Correctness review L12: {@code redactMessage} must preserve the message's structure. It used to + * split on a delimiter class and rejoin with single spaces, so commas, colons, brackets, quotes and + * newlines were all replaced with a space — even when nothing was redacted. This is the text stored + * in {@code ops_errors.message} and shown on {@code /ops/errors}. + */ +class RedactMessageStructureTest { + + @Test + void punctuationSurvivesWhenNothingIsRedacted() { + // A realistic Hibernate message: long enough to pass the old fast path, nothing secret. + String message = "could not execute statement [n/a]; SQL: select p1_0.id from posts p1_0"; + assertEquals(message, Redactor.redactMessage(message)); + } + + @Test + void newlinesAndIndentationSurvive() { + String message = "Constraint violation:\n table = posts\n column = title_unique_index"; + assertEquals(message, Redactor.redactMessage(message)); + } + + @Test + void punctuationSurvivesAroundARedactedToken() { + String message = "auth failed (token=A1b2C3d4E5f6G7h8J9k0, retrying)"; + String redacted = Redactor.redactMessage(message); + assertEquals("auth failed (token=[redacted], retrying)", redacted); + } + + @Test + void theSecretIsStillRemoved() { + String message = "bearer A1b2C3d4E5f6G7h8J9k0L1m2 rejected"; + String redacted = Redactor.redactMessage(message); + assertFalse(redacted.contains("A1b2C3d4E5f6G7h8J9k0L1m2"), redacted); + assertTrue(redacted.contains("[redacted]"), redacted); + assertEquals("bearer [redacted] rejected", redacted); + } + + @Test + void multipleSecretsAreEachRedactedInPlace() { + String message = "a=A1b2C3d4E5f6G7h8J9k0; b=Z9y8X7w6V5u4T3s2R1q0"; + assertEquals("a=[redacted]; b=[redacted]", Redactor.redactMessage(message)); + } + + @Test + void shortTokensAndOrdinaryWordsAreUntouched() { + assertEquals("user 42 not found", Redactor.redactMessage("user 42 not found")); + } + + @Test + void nullAndEmptyAreUnchanged() { + assertEquals(null, Redactor.redactMessage(null)); + assertEquals("", Redactor.redactMessage("")); + } + + @Test + void aBareJwtIsStillFullyReplaced() { + String jwt = "eyJhbGciOiJIUzI1NiJ9.eyJzdWIiOiIxIn0.dBjftJeZ4CVPmB92K27uhbUJU1p1r_wW1gFWFOEjXk"; + assertEquals(Redactor.PLACEHOLDER, Redactor.redactMessage(jwt)); + } + + @Test + void leadingAndTrailingDelimitersAreNotLost() { + assertEquals(" spaced ", Redactor.redactMessage(" spaced ")); + assertEquals("(A1b2C3d4E5f6G7h8J9k0)".replace("A1b2C3d4E5f6G7h8J9k0", "[redacted]"), + Redactor.redactMessage("(A1b2C3d4E5f6G7h8J9k0)")); + } +} diff --git a/src/test/java/com/larvalabs/brace/RouteStatsKeyTest.java b/src/test/java/com/larvalabs/brace/RouteStatsKeyTest.java new file mode 100644 index 0000000..91b90f1 --- /dev/null +++ b/src/test/java/com/larvalabs/brace/RouteStatsKeyTest.java @@ -0,0 +1,111 @@ +package com.larvalabs.brace; + +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Correctness review H1: {@code Stats.routes} is a cumulative map that is never reset, so it must + * be keyed by the matched route PATTERN, never the concrete URL. Keyed by path it leaks one entry + * per distinct URL ever requested — unbounded in the app's own ids, and unbounded in whatever an + * attacker types on the 404 path. + * + *

This invariant was established once by the runtime-performance review (H7, + * {@code recordRequestPattern}) and silently reverted: the method was added but never wired into + * {@code BraceHandler}, and nothing in the suite noticed. That is what these tests are for. + */ +class RouteStatsKeyTest { + + static TestApp app; + + @BeforeAll + static void setup() throws Exception { + app = Brace.test().start(a -> { + a.get("/users/{id}", req -> Result.text("user " + req.pathParam("id"))); + a.get("/boom/{id}", req -> { throw new IllegalStateException("kaboom"); }); + a.get("/missing/{id}", req -> Result.notFoundIfNull(null)); + }); + } + + @AfterAll + static void teardown() throws Exception { + app.stop(); + } + + @Test + void manyIdsUnderOnePatternCollapseToOneKey() { + var stats = app.app().stats(); + for (int i = 0; i < 25; i++) { + app.get("/users/" + i); + } + assertTrue(stats.routeStats().containsKey("GET /users/{id}"), + "expected the route pattern as the key, got: " + stats.routeStats().keySet()); + assertEquals(25, stats.routeStats().get("GET /users/{id}").count()); + assertTrue(stats.routeStats().keySet().stream().noneMatch(k -> k.matches("GET /users/\\d+")), + "concrete URLs must never become stats keys: " + stats.routeStats().keySet()); + } + + /** + * Unmatched URLs are attacker-controlled, so they must never reach the map. Note this asserts + * only the negative: whether an unmatched request is recorded at all is H2's business + * (today it is not — the no-route path returns before any recording), and the companion + * assertion that it lands in {@link BraceHandler#UNMATCHED_ROUTE_KEY} lives with that fix. + */ + @Test + void unmatchedUrlsNeverBecomeStatsKeys() { + var stats = app.app().stats(); + for (int i = 0; i < 25; i++) { + app.get("/no-such-route-" + i); + } + assertTrue(stats.routeStats().keySet().stream().noneMatch(k -> k.contains("no-such-route")), + "unmatched URLs must not become stats keys: " + stats.routeStats().keySet()); + } + + /** + * A malformed percent-escape in the query string throws out of {@code parseQuery} — which runs + * before {@code router.match} — so this is the live path where the 500 handler has no + * match to key on. It must fall back to the unmatched bucket, not to the raw URL. + */ + @Test + void throwBeforeRoutingFallsBackToTheUnmatchedBucket() throws Exception { + var stats = app.app().stats(); + // Sent over a raw socket: java.net.URI rejects "%zz" client-side, so the JDK HTTP client + // can't produce this request at all. + rawGet("/users/1?bad=%zz"); + assertTrue(stats.routeStats().containsKey("GET " + BraceHandler.UNMATCHED_ROUTE_KEY), + "expected the unmatched bucket, got: " + stats.routeStats().keySet()); + assertTrue(stats.routeStats().keySet().stream().noneMatch(k -> k.contains("%zz")), + "the raw URL must not become a stats key: " + stats.routeStats().keySet()); + } + + /** Minimal HTTP/1.1 GET over a socket, for request lines the JDK client refuses to build. */ + private static void rawGet(String target) throws Exception { + try (var socket = new java.net.Socket("localhost", app.port())) { + socket.getOutputStream().write( + ("GET " + target + " HTTP/1.1\r\nHost: localhost\r\nConnection: close\r\n\r\n") + .getBytes(java.nio.charset.StandardCharsets.US_ASCII)); + socket.getOutputStream().flush(); + socket.getInputStream().readAllBytes(); // drain so the server finishes the exchange + } + } + + @Test + void thrownNotFoundIsKeyedByPatternNotPath() { + var stats = app.app().stats(); + app.get("/missing/7"); + app.get("/missing/8"); + // A handler on a real route choosing to 404 still has a pattern — use it. + assertEquals(2, stats.routeStats().get("GET /missing/{id}").count()); + } + + @Test + void serverErrorIsKeyedByPatternNotPath() { + var stats = app.app().stats(); + app.get("/boom/7"); + app.get("/boom/8"); + assertEquals(2, stats.routeStats().get("GET /boom/{id}").count()); + } +} diff --git a/src/test/java/com/larvalabs/brace/RouterTest.java b/src/test/java/com/larvalabs/brace/RouterTest.java index d27b97f..2853b29 100644 --- a/src/test/java/com/larvalabs/brace/RouterTest.java +++ b/src/test/java/com/larvalabs/brace/RouterTest.java @@ -82,11 +82,32 @@ void matchesRootRoute() { @Test void trailingSlashPatternNormalized() { - // "/about/" compiles to the same matcher as "/about": the bare path matches, - // a trailing-slash request does not. + // "/about/" compiles to the same matcher as "/about", so the bare path matches. router.add("GET", "/about/", this::dummyHandler); assertNotNull(router.match("GET", "/about")); - assertNull(router.match("GET", "/about/")); + // ...and since correctness review L1, so does the trailing-slash request. This assertion + // used to be assertNull, which pinned the bug rather than a requirement: registering + // "/about/" and then 404ing a request for "/about/" is indefensible either way round. + assertNotNull(router.match("GET", "/about/")); + } + + @Test + void trailingSlashMatchesTheCanonicalRoute() { + // L1: a trailing slash is not a different resource. Matching (rather than redirecting) + // keeps non-GET verbs intact — a 301 would turn a POST into a GET and drop its body. + router.add("GET", "/users", this::dummyHandler); + router.add("GET", "/posts/{id}", this::dummyHandler); + + assertNotNull(router.match("GET", "/users/")); + assertNotNull(router.match("GET", "/posts/42/")); + assertEquals("42", router.match("GET", "/posts/42/").pathParams().get("id")); + } + + @Test + void trailingSlashDoesNotInventRoutes() { + router.add("GET", "/users", this::dummyHandler); + assertNull(router.match("GET", "/unknown/")); + assertNull(router.match("POST", "/users/"), "the method must still have to match"); } @Test diff --git a/src/test/java/com/larvalabs/brace/SessionOptionsSameSiteTest.java b/src/test/java/com/larvalabs/brace/SessionOptionsSameSiteTest.java new file mode 100644 index 0000000..6b3269a --- /dev/null +++ b/src/test/java/com/larvalabs/brace/SessionOptionsSameSiteTest.java @@ -0,0 +1,61 @@ +package com.larvalabs.brace; + +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Correctness review M12: {@code sameSite("None")} must imply {@code Secure}. Browsers reject + * {@code SameSite=None} without {@code Secure} outright, so the combination did not weaken the + * session cookie — it discarded it, and the symptom (nobody stays logged in) pointed nowhere near + * the config line. {@code sameSiteNone()} always set Secure; the string form silently did not. + */ +class SessionOptionsSameSiteTest { + + private static final String SECRET = "session-options-same-site-secret-32+"; + + @Test + void stringNoneImpliesSecure() { + var opts = SessionOptions.of(SECRET).sameSite("None"); + assertTrue(opts.secure(), "SameSite=None must imply Secure"); + assertTrue(opts.buildSetCookie("v").contains("; Secure")); + assertTrue(opts.buildSetCookie("v").contains("; SameSite=None")); + } + + @Test + void stringNoneIsCaseInsensitive() { + assertTrue(SessionOptions.of(SECRET).sameSite("none").secure()); + assertTrue(SessionOptions.of(SECRET).sameSite("NONE").secure()); + } + + @Test + void valuesAreNormalizedToTheCanonicalSpelling() { + assertEquals("Strict", SessionOptions.of(SECRET).sameSite("strict").sameSite()); + assertEquals("Lax", SessionOptions.of(SECRET).sameSite("LAX").sameSite()); + } + + @Test + void strictAndLaxDoNotForceSecure() { + assertFalse(SessionOptions.of(SECRET).sameSite("Strict").secure()); + assertFalse(SessionOptions.of(SECRET).sameSite("Lax").secure()); + } + + @Test + void invalidValuesAreRejectedRatherThanWrittenVerbatim() { + // A bogus value used to reach the header, where browsers ignore the attribute entirely + // and fall back to their own default — a silent downgrade. + assertThrows(IllegalArgumentException.class, () -> SessionOptions.of(SECRET).sameSite("Loose")); + assertThrows(IllegalArgumentException.class, () -> SessionOptions.of(SECRET).sameSite("")); + assertThrows(IllegalArgumentException.class, () -> SessionOptions.of(SECRET).sameSite(null)); + } + + @Test + void fluentNoneHelperStillBehavesTheSame() { + var opts = SessionOptions.of(SECRET).sameSiteNone(); + assertTrue(opts.secure()); + assertEquals("None", opts.sameSite()); + } +} diff --git a/src/test/java/com/larvalabs/brace/ShortCircuitStatsTest.java b/src/test/java/com/larvalabs/brace/ShortCircuitStatsTest.java new file mode 100644 index 0000000..94379fa --- /dev/null +++ b/src/test/java/com/larvalabs/brace/ShortCircuitStatsTest.java @@ -0,0 +1,111 @@ +package com.larvalabs.brace; + +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Correctness review H2: every response leaving the handler must be counted, not just the three + * paths that happened to have a recording call. Before this fix, stats and the request log ran + * only on the success path and the two catch blocks — so rate-limiter 429s, CSRF 403s, 413s, + * static files and unmatched 404s were invisible to {@code /ops/status} entirely, which is + * precisely the traffic an incident is about. + */ +class ShortCircuitStatsTest { + + static TestApp app; + static Path assetDir; + + @BeforeAll + static void setup() throws Exception { + assetDir = Files.createTempDirectory("brace-h2-assets"); + Files.writeString(assetDir.resolve("app.css"), "body{}"); + app = Brace.test().sessions("h2-short-circuit-secret-at-least-32-chars").start(a -> { + a.staticFiles("/assets", assetDir.toString()); + a.get("/ok", req -> Result.text("ok")); + a.before("/blocked", req -> Result.error(429, "Too Many Requests")); + a.get("/blocked", req -> Result.text("never reached")); + a.before("/guarded", (req, session) -> Redirect.to("/login")); + a.get("/guarded", req -> Result.text("never reached")); + a.post("/mutate", req -> Result.text("mutated")); + }); + } + + @AfterAll + static void teardown() throws Exception { + app.stop(); + } + + private static long countOf(int status) { + return app.app().stats().statusCodeCounts().getOrDefault(status, 0L); + } + + @Test + void beforeMiddlewareShortCircuitIsCounted() { + long before = countOf(429); + assertEquals(429, app.get("/blocked").status()); + assertEquals(before + 1, countOf(429), "a 429 from before-middleware must reach stats"); + } + + @Test + void sessionMiddlewareShortCircuitIsCounted() { + long before = countOf(302); + assertEquals(302, app.get("/guarded").status()); + assertEquals(before + 1, countOf(302), "a guard redirect must reach stats"); + } + + @Test + void csrfRejectionIsCounted() { + long before = countOf(403); + // No _csrf param and no X-CSRF-Token: rejected before the handler runs. + assertEquals(403, app.post("/mutate", Map.of("x", "1")).status()); + assertEquals(before + 1, countOf(403), "a CSRF 403 must reach stats"); + } + + @Test + void unmatchedRouteIsCountedInTheUnmatchedBucket() { + long before = countOf(404); + assertEquals(404, app.get("/no-such-route-at-all").status()); + assertEquals(before + 1, countOf(404), "an unmatched 404 must reach stats"); + assertTrue(app.app().stats().routeStats() + .containsKey("GET " + BraceHandler.UNMATCHED_ROUTE_KEY), + "expected the unmatched bucket, got: " + app.app().stats().routeStats().keySet()); + } + + @Test + void staticFilesAreCountedUnderTheirOwnBucket() { + assertEquals(200, app.get("/assets/app.css").status()); + var keys = app.app().stats().routeStats().keySet(); + assertTrue(keys.contains("GET " + BraceHandler.STATIC_ROUTE_KEY), + "expected the static bucket, got: " + keys); + // The filename is client-supplied — it must not become a key of its own. + assertTrue(keys.stream().noneMatch(k -> k.contains("app.css")), + "asset filenames must not become stats keys: " + keys); + } + + @Test + void missingAssetsShareTheStaticBucketRatherThanMintingKeys() { + for (int i = 0; i < 20; i++) { + app.get("/assets/nope-" + i + ".css"); + } + var keys = app.app().stats().routeStats().keySet(); + assertTrue(keys.stream().noneMatch(k -> k.contains("nope-")), + "missing-asset URLs must not become stats keys: " + keys); + } + + @Test + void everyResponseIsCountedExactlyOnce() { + long before = countOf(200); + for (int i = 0; i < 5; i++) { + app.get("/ok"); + } + assertEquals(before + 5, countOf(200), "each response must be recorded exactly once"); + } +} diff --git a/src/test/java/com/larvalabs/brace/SmallCorrectnessFixesTest.java b/src/test/java/com/larvalabs/brace/SmallCorrectnessFixesTest.java new file mode 100644 index 0000000..90eed69 --- /dev/null +++ b/src/test/java/com/larvalabs/brace/SmallCorrectnessFixesTest.java @@ -0,0 +1,136 @@ +package com.larvalabs.brace; + +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** Correctness review L1 (trailing slash), L3 ({@code View.of} odd args), L4 (no-op session writes). */ +class SmallCorrectnessFixesTest { + + static TestApp app; + + @BeforeAll + static void setup() throws Exception { + app = Brace.test().start(a -> { + a.get("/plain", req -> Result.text("plain")); + a.get("/users/{id}", req -> Result.text("user " + req.pathParam("id"))); + a.get("/", req -> Result.text("root")); + a.post("/submit", req -> Result.text("posted")).csrf(false); + }); + } + + @AfterAll + static void teardown() throws Exception { + app.stop(); + } + + // --- L1: trailing slash --- + + @Test + void trailingSlashMatchesTheCanonicalStaticRoute() { + assertEquals(200, app.get("/plain/").status()); + assertEquals("plain", app.get("/plain/").body()); + } + + @Test + void trailingSlashMatchesADynamicRoute() { + assertEquals("user 42", app.get("/users/42/").body()); + } + + @Test + void trailingSlashWorksForNonGetVerbsWithoutARedirect() { + // Matching (rather than 301-ing) matters here: a redirect would turn this POST into a GET + // and silently drop the body. + var res = app.request("POST", "/submit/").body("a=1", "application/x-www-form-urlencoded").send(); + assertEquals(200, res.status()); + assertEquals("posted", res.body()); + } + + @Test + void rootIsUnaffected() { + assertEquals("root", app.get("/").body()); + } + + @Test + void aGenuinelyUnknownPathStill404s() { + assertEquals(404, app.get("/nope/").status()); + assertEquals(404, app.get("/nope").status()); + } + + // --- L3: View.of / View.render arg count --- + + @Test + void viewOfRejectsAnOddArgumentCount() { + var e = assertThrows(IllegalArgumentException.class, + () -> View.of("page", "a", 1, "trailing")); + assertTrue(e.getMessage().contains("trailing"), "the message should name the dangling key"); + } + + @Test + void viewRenderRejectsAnOddArgumentCount() { + assertThrows(IllegalArgumentException.class, () -> View.render("page", "a", 1, "trailing")); + } + + @Test + void viewOfStillAcceptsWellFormedPairs() { + var view = View.of("page", "a", 1, "b", 2); + assertEquals(1, view.params().get("a")); + assertEquals(2, view.params().get("b")); + } + + // --- L4: no-op session writes --- + + @Test + void settingTheSameValueDoesNotMarkTheSessionModified() { + var session = new Session(); + session.set("user", "42"); + assertTrue(session.isModified()); + + var reread = Session.fromCookie(session.toCookie(secret()), secret()); + assertFalse(reread.isModified()); + reread.set("user", "42"); + assertFalse(reread.isModified(), "re-setting an identical value is not a change"); + } + + @Test + void settingADifferentValueDoesMarkModified() { + var session = new Session(); + session.set("user", "42"); + var reread = Session.fromCookie(session.toCookie(secret()), secret()); + reread.set("user", "43"); + assertTrue(reread.isModified()); + } + + @Test + void removingAnAbsentKeyDoesNotMarkModified() { + var session = new Session(); + session.remove("never-set"); + assertFalse(session.isModified(), + "an unconditional remove() in a guard must not make every response uncacheable"); + } + + @Test + void removingAPresentKeyDoesMarkModified() { + var session = new Session(); + session.set("user", "42"); + var reread = Session.fromCookie(session.toCookie(secret()), secret()); + reread.remove("user"); + assertTrue(reread.isModified()); + } + + @Test + void clearingAnEmptySessionDoesNotMarkModified() { + var session = new Session(); + session.clear(); + assertFalse(session.isModified()); + } + + private static String secret() { + return "small-correctness-fixes-secret-32-chars"; + } +} diff --git a/src/test/java/com/larvalabs/brace/SmallFixesUnitTest.java b/src/test/java/com/larvalabs/brace/SmallFixesUnitTest.java new file mode 100644 index 0000000..54145f2 --- /dev/null +++ b/src/test/java/com/larvalabs/brace/SmallFixesUnitTest.java @@ -0,0 +1,94 @@ +package com.larvalabs.brace; + +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** Correctness review L5 (SigV4 encoding), L8 (interval units), L9 (CIDR prefix validation). */ +class SmallFixesUnitTest { + + // --- L5: Storage.uriEncodePath follows SigV4's unreserved set --- + + @Test + void sigV4UnreservedCharactersArePreserved() { + // RFC 3986 / SigV4 unreserved: A-Za-z0-9-._~ — notably "~" must NOT be encoded. + assertEquals("a-b_c.d~e", Storage.uriEncodePath("a-b_c.d~e")); + } + + @Test + void asteriskIsEncodedAsSigV4Requires() { + // URLEncoder leaves "*" literal; SigV4 wants %2A. A key containing it used to produce a + // canonical request S3 could not reproduce, i.e. SignatureDoesNotMatch. + assertEquals("star%2Aname", Storage.uriEncodePath("star*name")); + } + + @Test + void spacesAreEncodedAsPercent20NotPlus() { + assertEquals("my%20file.jpg", Storage.uriEncodePath("my file.jpg")); + } + + @Test + void slashesRemainSeparators() { + assertEquals("avatars/2026/my%20pic.png", Storage.uriEncodePath("avatars/2026/my pic.png")); + } + + @Test + void nonAsciiIsUtf8PercentEncoded() { + assertEquals("caf%C3%A9.txt", Storage.uriEncodePath("café.txt")); + } + + // --- L8: interval grammar matches Cache.parseTtl --- + + @Test + void intervalsAcceptDaysLikeCacheTtlsDo() { + assertEquals(86_400_000L, JobScheduler.parseInterval("1d")); + assertEquals(2 * 86_400_000L, JobScheduler.parseInterval("2d")); + // The pairing that used to disagree: same string, two grammars. + assertEquals(java.time.Duration.ofDays(1).toMillis(), JobScheduler.parseInterval("1d")); + assertEquals(java.time.Duration.ofDays(1), Cache.parseTtl("1d")); + } + + @Test + void existingIntervalUnitsAreUnchanged() { + assertEquals(1000L, JobScheduler.parseInterval("1s")); + assertEquals(60_000L, JobScheduler.parseInterval("1m")); + assertEquals(3_600_000L, JobScheduler.parseInterval("1h")); + } + + @Test + void unknownIntervalUnitsStillThrowAndListTheValidOnes() { + var e = assertThrows(IllegalArgumentException.class, () -> JobScheduler.parseInterval("1y")); + assertTrue(e.getMessage().contains("s, m, h, or d"), e.getMessage()); + } + + // --- L9: CIDR prefix bounds --- + + @Test + void negativeCidrPrefixIsRejectedRatherThanTrustingEverything() { + // "/-1" used to produce an all-zero mask, which matches every address — turning a config + // typo into "trust all forwarding headers". + assertThrows(IllegalArgumentException.class, () -> new TrustedProxies("10.0.0.0/-1")); + } + + @Test + void oversizedCidrPrefixIsRejected() { + assertThrows(IllegalArgumentException.class, () -> new TrustedProxies("10.0.0.0/33")); + assertThrows(IllegalArgumentException.class, () -> new TrustedProxies("::1/129")); + } + + @Test + void validCidrsStillWork() { + var proxies = new TrustedProxies("10.0.0.0/8", "127.0.0.1", "192.168.1.0/24"); + assertTrue(proxies.isTrusted("10.1.2.3")); + assertTrue(proxies.isTrusted("127.0.0.1")); + assertTrue(proxies.isTrusted("192.168.1.7")); + assertEquals(false, proxies.isTrusted("8.8.8.8")); + } + + @Test + void zeroPrefixStillMeansEverythingBecauseThatIsWhatItMeans() { + assertTrue(new TrustedProxies("0.0.0.0/0").isTrusted("8.8.8.8")); + } +} diff --git a/src/test/java/com/larvalabs/brace/StopReleasesResourcesTest.java b/src/test/java/com/larvalabs/brace/StopReleasesResourcesTest.java new file mode 100644 index 0000000..1be663d --- /dev/null +++ b/src/test/java/com/larvalabs/brace/StopReleasesResourcesTest.java @@ -0,0 +1,61 @@ +package com.larvalabs.brace; + +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Correctness review M4 and M5: {@code Brace.stop()} must release what {@code start()} took. + * It previously left the HikariCP pool and Hibernate SessionFactory open (poolSize live + * connections per stopped app, since {@code minimumIdle == maximumPoolSize}) and left the + * rate limiter's process-global statics pointing at the app that just went away. + */ +class StopReleasesResourcesTest { + + + @Test + void stopClosesTheDatabaseFactoryItOwns() throws Exception { + var app = Brace.test().entities(com.larvalabs.brace.testmodels.User.class).start(a -> { + a.get("/", req -> Result.text("ok")); + }); + var factory = app.app().databaseFactory(); + // Live before stop. + factory.openSession().close(); + + app.stop(); + + assertThrows(Exception.class, factory::openSession, + "stop() must close the pool it owns, so opening a session afterwards fails"); + } + + @Test + void ownsDatabaseFalseLeavesTheFactoryToTheCaller() throws Exception { + var factory = new DatabaseFactory( + "jdbc:h2:mem:owns-database-false;DB_CLOSE_DELAY=-1", null, null, + java.util.List.of(com.larvalabs.brace.testmodels.User.class)); + + var app = Brace.app().port(0).banner(false).database(factory).ownsDatabase(false); + app.get("/", req -> Result.text("ok")); + app.start(); + app.stop(); + + // Still usable — the caller said it owns the lifecycle. + factory.openSession().close(); + factory.close(); + } + + @Test + void stopReleasesTheRateLimiterRegistry() throws Exception { + var app = Brace.test().start(a -> + a.before("/limited", RateLimiter.perIp(5, "1m"))); + assertFalse(RateLimiter.allStats().isEmpty(), "the limiter should be registered while running"); + + app.stop(); + + assertTrue(RateLimiter.allStats().isEmpty(), + "stop() must drop limiters from the process-global registry, " + + "or /ops keeps reporting limiters whose app is gone"); + } +} diff --git a/src/test/java/com/larvalabs/brace/UrlEncodingAndRowShapeTest.java b/src/test/java/com/larvalabs/brace/UrlEncodingAndRowShapeTest.java new file mode 100644 index 0000000..36bc9a1 --- /dev/null +++ b/src/test/java/com/larvalabs/brace/UrlEncodingAndRowShapeTest.java @@ -0,0 +1,147 @@ +package com.larvalabs.brace; + +import com.larvalabs.brace.testmodels.Post; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; + +/** + * Correctness review M6 ({@code Url.to} percent-encodes substituted values) and M7 + * ({@code db.sqlQuery}/{@code db.hql} really return {@code List}, including for + * single-column selects). + */ +class UrlEncodingAndRowShapeTest { + + static TestApp app; + + @BeforeAll + static void setup() throws Exception { + app = Brace.test().entities(Post.class).start(a -> + a.get("/tags/{name}", req -> Result.text(req.pathParam("name")))); + app.withDb(db -> { + var p = new Post(); + p.title = "Ada"; + p.body = "first"; + db.insert(p); + }); + } + + @AfterAll + static void teardown() throws Exception { + app.stop(); + } + + // --- M6: Url.to --- + + @Test + void plainValuesAreUnchanged() { + assertEquals("/users/42", Url.to("/users/{id}", 42)); + assertEquals("/posts/hello-world", Url.to("/posts/{slug}", "hello-world")); + assertEquals("/files/a.b_c~d", Url.to("/files/{n}", "a.b_c~d")); + } + + @Test + void slashInAValueDoesNotCreateAnExtraSegment() { + assertEquals("/tags/a%2Fb", Url.to("/tags/{name}", "a/b")); + } + + @Test + void spacesAndReservedCharactersAreEncoded() { + assertEquals("/tags/John%20Doe", Url.to("/tags/{name}", "John Doe")); + assertEquals("/tags/a%26b", Url.to("/tags/{name}", "a&b")); + assertEquals("/tags/a%3Fb", Url.to("/tags/{name}", "a?b")); + assertEquals("/tags/a%23b", Url.to("/tags/{name}", "a#b")); + // "+" must be encoded, not passed through: in a path it is a literal plus, and leaving + // it bare would be ambiguous with the form-encoded spelling of a space. + assertEquals("/tags/a%2Bb", Url.to("/tags/{name}", "a+b")); + } + + @Test + void nonAsciiIsEncodedAsUtf8() { + assertEquals("/tags/caf%C3%A9", Url.to("/tags/{name}", "café")); + } + + @Test + void generatedUrlRoundTripsBackToTheOriginalValue() { + // The whole point: Url.to and pathParam are inverses (H3 + M6). + for (String value : new String[]{"John Doe", "a&b", "a?b", "a#b", "café", "a+b"}) { + String url = Url.to("/tags/{name}", value); + assertEquals(value, app.get(url).body(), "round trip failed for: " + value); + } + } + + /** + * Two values encode correctly but cannot make the round trip over HTTP: Jetty's default + * {@code UriCompliance} rejects {@code %2F} (ambiguous path separator) and {@code %25} + * (ambiguous encoding) with a 400 before the handler runs. Encoding them as anything else + * would be wrong — leaving {@code /} bare really would forge a segment — so the assertion is + * on the encoder, and the transport limit is recorded rather than papered over. + * + *

Practical consequence for apps: a path parameter cannot carry a value containing + * {@code /} or {@code %} unless the server relaxes URI compliance. Put those in the query + * string, which has no such restriction. + */ + @Test + void slashAndPercentEncodeCorrectlyEvenThoughJettyRefusesToCarryThem() { + assertEquals("/tags/a%2Fb", Url.to("/tags/{name}", "a/b")); + assertEquals(400, app.get("/tags/a%2Fb").status()); + + assertEquals("/tags/100%25", Url.to("/tags/{name}", "100%")); + assertEquals(400, app.get("/tags/100%25").status()); + } + + @Test + void multipleParametersAreEachEncoded() { + assertEquals("/a/x%20y/b/z%26w", Url.to("/a/{p}/b/{q}", "x y", "z&w")); + } + + @Test + void literalPatternSegmentsAreNotEncoded() { + // Pattern text is code, not data — it must pass through untouched. + assertEquals("/a-b/c.d/42", Url.to("/a-b/c.d/{id}", 42)); + } + + // --- M7: row shape --- + + @Test + void singleColumnSqlQueryReturnsRowsNotBareScalars() { + var rows = app.db().sqlQuery("SELECT title FROM posts"); + assertFalse(rows.isEmpty()); + // Before the fix this threw ClassCastException in the caller's loop, because Hibernate + // hands back a List for a one-column select while the signature promises rows. + for (Object[] row : rows) { + assertEquals("Ada", row[0]); + } + } + + @Test + void multiColumnSqlQueryIsUnchanged() { + var rows = app.db().sqlQuery("SELECT title, body FROM posts"); + assertEquals("Ada", rows.get(0)[0]); + assertEquals("first", rows.get(0)[1]); + } + + @Test + void singleColumnHqlReturnsRowsNotBareScalars() { + var rows = app.db().hql("SELECT p.title FROM Post p"); + assertFalse(rows.isEmpty()); + for (Object[] row : rows) { + assertEquals("Ada", row[0]); + } + } + + @Test + void multiColumnHqlIsUnchanged() { + var rows = app.db().hql("SELECT p.title, p.body FROM Post p"); + assertEquals("Ada", rows.get(0)[0]); + assertEquals("first", rows.get(0)[1]); + } + + @Test + void emptyResultIsAnEmptyList() { + assertEquals(0, app.db().sqlQuery("SELECT title FROM posts WHERE title = ?", "nobody").size()); + } +}