Files
pad/internal/server/middleware_path.go
T
xarmian e4e914d399 fix(server): reject path segments the database cannot be asked about (BUG-2782) (#1207)
* fix(server): reject path segments the database cannot be asked about (BUG-2782)

Every handler that resolves a workspace, collection, item, comment or
attachment from a URL path segment passes that segment to the store
verbatim, and the store binds it into a text comparison. Postgres refuses
a text parameter that is not valid UTF-8 or that contains a NUL (SQLSTATE
22021 / 22P05); the driver surfaces that as a query error and the handler
answers 500. SQLite accepts both bytes and matches nothing, so the same
request is a clean 404 there — a dialect divergence that leaves the defect
invisible to self-hosted installs and live on Pad Cloud.

Measured before the fix, driving every route that carries a path parameter
with one segment set to "bad-%FF-x" (247 probes, one per parameter position
per method, real values elsewhere): Postgres answered 500 to 191 of them,
SQLite to 0. After: 0 and 0, all 247 answered 400.

Fixed with one root-level middleware rather than at ~112 chi.URLParam call
sites, because this is a transport-level input rule and per-call-site fixes
rely on every future route remembering. ValidatePath rejects a request whose
percent-DECODED path is not valid UTF-8 or contains a NUL, before routing.

It validates r.URL.Path rather than what chi hands the handler. chi routes
on RawPath when non-empty and Path otherwise, and Go populates RawPath only
when the client's escaping is not already canonical — Go escapes 0xff as
uppercase "%FF", so the CANONICAL form any ordinary client emits is exactly
the one that reaches the store decoded, and the lowercase "%ff" oddity is
the harmless one. Validating the decoded path answers both identically and
does not depend on chi continuing to prefer RawPath.

It cannot refuse Pad's own URLs: store.slugify emits only [a-z0-9-], ids
are UUIDs or hex, refs are a prefix plus digits. Valid non-ASCII segments
pass through untouched — the database accepts them and they may legitimately
name something. 400 rather than 404 because the request is malformed as a
URI and the answer does not depend on whether anything exists, so it is not
an existence oracle. Scope is the path; the query string is validated at its
points of use, per BUG-2774's validCursorID.

Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X

* fix(server): the invalid-path rejection must look like every other API error (BUG-2782)

Codex round 1, verified before acting on: ValidatePath runs on the root
router, so its rejection short-circuits ABOVE the /api/v1 group's
cors.Handler and jsonContentType and inherited neither. Measured — the 400
carried a JSON body sniffed as text/plain and no CORS headers at all, while
a normal 404 on the same route carried Content-Type: application/json plus
the full CORS set. On a cross-origin deployment (PAD_CORS_ORIGINS set) the
browser refuses to let the page read a response with no Access-Control-
Allow-Origin, so a debuggable 400 arrives as an opaque network error.

Fixed without duplicating the CORS configuration: the group's cors.Handler
is hoisted into one shared instance, the group mounts it as before, and
ValidatePath serves its rejection THROUGH the same instance. Content-Type
is set explicitly, since jsonContentType is mounted below and never runs
for a rejection.

Moving ValidatePath down into the group instead was rejected: two covered
routes live outside it — the SPA catch-all and /api/v1/collab/{itemID} —
and the mutant that makes that move is caught by exactly those two subtests.

A genuine preflight (Origin + Access-Control-Request-Method) to an invalid
path is answered 200 by the shared handler, the same as for any other path:
a preflight asks whether the method and headers are permitted, not whether
the resource exists. The real request that follows still gets the 400, and
can now be read. Asserted rather than described.

The new test compares each header on the 400 against the SAME route
answered normally, for an allowed origin AND a disallowed one, so it pins
parity with the API's own errors rather than a header list copied from a
spec — and the disallowed-origin leg is what would fail if the rejection
echoed origins the shared handler refuses.

Mutation matrix, all nine verified to COMPILE first: dropping the CORS
decoration and dropping the explicit Content-Type are each detected, and
only by this new test.

Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X

* test(server): pin the ordering decision the path check makes (BUG-2782)

Codex round 2, angle rotated to middleware contracts: a rejected request
never reaches TokenAuth, SessionAuth, RateLimit or CSRFProtect, because
ValidatePath sits on the root router above that group. The finding is
factually right and the ordering is deliberate, but nothing in the diff
said so and no test held it — which is the same defect shape as an
undocumented invariant: true today, unenforced tomorrow.

Verified rather than argued, because "bypasses the rate limiter" reads as
a weakening and here the direction is inverted. Before this middleware,
the same request ran SessionAuth — a store.ValidateSession round trip —
then the limiter, then a handler whose query the database refused, and
answered 500. It now costs a UTF-8 scan and a short JSON write with no
database contact, so the unmetered path is strictly cheaper than every
path the limiter protects. The answer is also constant for all inputs of
this shape, independent of auth and of existence, so a flood learns
nothing. And the limiter is a plain token bucket per key — no escalating
ban, no durable block — so skipping it defeats no state that outlives the
request.

The alternative, metering it inside the /api/v1 group, trades this for a
real coverage hole: the SPA catch-all and /api/v1/collab/{itemID} are
mounted outside that group.

The test floods 80 invalid paths from one IP (burst is 60), requires all
80 to be 400 and none 429, then requires a VALID request from the same IP
to still get the resolver's 404 — proving the budget was untouched. It
then asserts its own premise: the same volume of valid requests from a
second IP must actually hit the limiter, because an inert limiter would
produce an identical reading for the first half.

Both mutants land where they should: metering the rejection fails at
request 61 (burst 60 + 1, which independently confirms the constant
cited above), and disabling the limiter fails the premise check rather
than passing quietly.

Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X

* docs(server): correct four claims this branch's own measurements refute (BUG-2782)

Codex round 3, angle rotated onto prose accuracy. No behaviour changes —
every finding is a sentence that was stronger than what was verified, and
in two cases stronger than data already sitting in this branch.

1. "It rejects exactly what the DATABASE rejects." Too strong, and
   inherited verbatim from validCursorID. Postgres refuses these two
   classes under a UTF8 database encoding; SQL_ASCII accepts the same
   bytes, and SQLite's sqlite3_bind_text accepts arbitrary sequences with
   NUL undefined rather than erroring. Pad neither creates nor configures
   that database — nothing issues CREATE DATABASE or sets client_encoding
   — so the encoding is the operator's. Now stated as what it is: the
   strictest reading, applied uniformly so the two backends stop
   disagreeing about the same request, measured against postgres:17-alpine
   at its defaults.

   A first draft of this correction replaced the overstatement with a NEW
   unverified claim ("the encoding Pad's migrations create"). Grepping for
   CREATE DATABASE found it only in test helpers. Fixing an unchecked
   sentence with another unchecked sentence is the same defect wearing the
   repair's clothes.

2. "Against unfixed code these are 500 on Postgres and 404 on SQLite."
   False for 56 of the 247 pre-fix probes, and my own sweep output said so
   — routes whose authorization or configuration gate answers before any
   store call (admin user lookup; attachments with no storage configured).
   Replaced with the pasted distribution: 500:191 404:34 403:12 401:4
   503:4 400:2.

3. "Passed through untouched" oversold what this middleware guarantees.
   It does not touch a valid path, but chi still hands the handler the
   ESCAPED text whenever RawPath is populated: "caf%C3%A9" arrives as
   "café", the non-canonical "caf%c3%a9" arrives literally, and "%2F"
   never becomes a separator. Pre-existing chi behaviour, unaffected by
   this change, written down because the obvious reading is stronger than
   the truth.

4. "The request is malformed as a URI." It is not — "%FF" and "%00" are
   syntactically valid percent-encoded octets. The 400 is because the
   DECODED value cannot be a resource identifier here, which is the actual
   reason and a different one.

Also reconciled the two probe counts that appear in this branch's history
(111/94 GET-only, 247/191 all methods) so a reader meeting both does not
have to guess which is wrong; they are one sweep at two widths.

CONVE-23 sweep: finding 1 falsifies the same sentence in validCursorID
(handlers_timeline.go, BUG-2774), which is where this branch inherited it.
Corrected there too rather than left standing — the rule that comment
describes is unchanged and still right; only its claim about the database
was wrong.

Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X

* test(mcp): drive the in-process transport seam the path check walks past (BUG-2782)

Codex round 4's exploration pointed at the door I had asserted rather than
driven: the remote /mcp transport does not reach the server over a socket.
HTTPHandlerDispatcher SYNTHESIZES an *http.Request and calls
Handler.ServeHTTP in-process, so "the middleware covers every route" was a
claim about a path this transport bypasses on its face.

Driven, it is covered — and for a chain nothing in the tree stated: Handler
is the *server.Server, chi's Mux.ServeHTTP runs mx.handler (middlewares +
routeHTTP) on BOTH branches, and buildAuthedRequest forces the fresh-routing
branch with a typed-nil RouteCtxKey. Every link is load-bearing and none was
written down; this test is what notices if one changes.

The counterfactual was worth more than the confirmation. Unfixed, an MCP
agent that put an invalid byte in a ref got upstream_error on Postgres —
whose hint says the failure is "usually transient, retry" — for an input
that can never succeed. An agent obeying that hint retries forever. That is
the retry-hostile misclassification family BUG-2675 added a code for, and
this change removes an instance of it that nobody had noticed. Now
validation_failed: the agent is told its INPUT is wrong.

The first version of this test named upstream_error in its comment while
running on SQLite, where unfixed gives item_not_found instead — an
assertion that would have failed for a reason other than the one it named.
The comment now states both backends separately and the fixture takes
Postgres when PAD_TEST_POSTGRES_URL is set, so under make test-pg the
dangerous half is what actually runs.

Control leg included: a valid-but-absent ref must still return
item_not_found, or a dispatcher that refused every ref would pass.

Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X

* chore(mcp): remove a throwaway probe that was committed by accident (BUG-2782)

The probe that established the MCP seam behaviour was meant to be deleted
once dispatch_http_invalid_path_test.go replaced it. The 'rm' was written
as the first half of a compound command whose second half the tool layer
REJECTED, so the whole command never executed — and a later 'git add -A'
swept the file in. It duplicates the real test with printf-style output
and no assertions.

The rule this breaks is one I already hold: verify the mutation, not the
report of it. I read 'rm -f X && cat > Y' as having removed X because I
wrote it, when the command never ran at all. A rejected command and a
successful one look identical in a transcript if you do not look.

Caught by a Codex file listing showing an A for a file I believed gone.

Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X

* test: five corrections from the final review pass (BUG-2782)

Codex round 5, judging the whole change. All five are mine; none needed a
behaviour change.

**A flake I built in.** The rate-limit test flooded 80 requests against a
bucket of burst 60 — but a token bucket REFILLS while the loop runs, at
10/s here, so 20 tokens of headroom is 2 seconds of tolerance and a slow or
-race'd run would admit all 80 and fail spuriously. The margin that matters
is not flood-vs-burst but how long the loop must take for refill to cover
the excess. At 400 requests that is (400-60)/10 = 34 seconds against
in-process calls measured in microseconds: four orders of magnitude. The
constant now carries that derivation, including the rate and burst it
depends on. Both mutants still land, and metering the rejection still fails
at request 61 — burst 60 + 1, unchanged by the larger flood.

**A claim about MCP that JSON does not support.** The seam test's comment
said an agent could put "a raw invalid byte" into a ref. Measured with
encoding/json instead of assumed:

    raw 0xff / lone surrogate / truncated sequence → U+FFFD, valid UTF-8
    raw 0x00                                       → JSON parse error
    the u0000 ESCAPE                               → a real NUL

So exactly one of the five cases is reachable end to end over a JSON
transport, and it is the one only the NUL half of validPathText refuses.
The raw-byte cases stay — Dispatch is a Go API and the JSON decode is
upstream of that boundary, so they assert the seam holds for callers that
do not launder their strings through encoding/json — but the comment no
longer offers them as evidence a JSON client can send them.

**Two prose overstatements the earlier sweep missed.** The control test
still said the rule rejects "only what the database rejects", which the
previous commit had already established is false in the permissive
direction. And TestValidatePathPostgresNoInternalError was described as
reproducing the original 500 when it runs the FIXED server and can only
ever observe a 400; the 500 lives in the counterfactual sweep and in the
mutation matrix, and a test cannot both apply a fix and witness the bug.

**One dead construction**, plus a smaller instance of the same habit: the
MCP fixture built a SQLite store and discarded it in Postgres mode. My
first attempt replaced the comment with one claiming the branch had been
hoisted, and left the code as it was — writing the fix into the prose
instead of the code, in the same hour I committed a message about not
doing exactly that. Now actually branched.

Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X

* docs+test: seven more corrections, and one the emoji route earns (BUG-2782)

Codex round 6, reading the three files as they now stand. All prose or
assertion strength; one of them changes what the tests cover.

**Pad DOES emit a non-ASCII path segment, and I said it never does.**
`DELETE /workspaces/{ws}/comments/{commentID}/reactions/{emoji}` — the web
client sends `encodeURIComponent(emoji)`. So the justification I gave for
"it cannot refuse Pad's own URLs" was false in its premise while true in
its conclusion, which is the worst combination: a reader checking the
premise finds a counterexample and has no reason to trust the rest.

It is also the best possible illustration of why the rule permits valid
non-ASCII, so the control test now drives that ACTUAL route with a real
emoji rather than relying on an emoji-shaped item slug — the claim is true
by construction instead of by careful wording. A new mutant confirms the
leg discriminates: a rule that rejects all non-ASCII (the plausible wrong
version, not the absurd one) is caught there.

**"The handler answers 500" was universal and is not.** Handlers that
collapse a resolution error into not-found already answer 404 — the
timeline handler's `err != nil || item == nil` is the example. My own
measured distribution said so; the sentence did not.

**"Self-hosted installs never see it" was wrong about the axis.** The split
is by BACKEND, not deployment: a SQLite install never sees it, any Postgres
install does — Pad Cloud and a self-hoster on Postgres alike.

**A stale cross-reference of my own making.** The previous commit corrected
TestValidatePathPostgresNoInternalError's claim to reproduce the 500, and
left the sentence POINTING at it still saying it does. Fixing a claim at
one site and leaving its pointer false is the CONVE-23 case in miniature.

**The MCP test asserted too little.** validation_failed is how the
dispatcher classifies ANY 400, so the test could have passed on a
mapper-level refusal without ValidatePath running at all. It now pins the
middleware's own message, which rides through on the hint.

**Two overstatements in the same file.** "Exactly one case is reachable"
should be one input CLASS (two cases carry a NUL). And the raw-byte cases
do not cover "the stdio path": local stdio MCP is ExecDispatcher, which
shells out to the binary and never touches this in-process door. Scope now
says HTTPHandlerDispatcher and says what it does not speak for.

Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X

* docs(server): two qualifications the file already owed itself (BUG-2782)

Codex round 7. Two, both narrow — the review is converging (7 findings last
round, 2 this one), and both are internal inconsistencies rather than new
ground.

"Any Postgres install does" contradicted a qualification made forty lines
lower in the same file, where validPathText spells out that SQL_ASCII
Postgres accepts these bytes. Now says a Postgres install whose database
encoding is UTF8, notes that this is initdb's default, and points at the
place the qualification lives so the two cannot drift apart again.

validCursorID's paragraph still described the 500 in the present tense,
though BUG-2774 fixed it — it is the behaviour the guard PREVENTS, not what
the endpoint does. My first attempt at this appended "past tense throughout
this paragraph" and left the following sentence in the present tense, which
is annotating a problem instead of fixing it. Rewritten so the tense
carries the meaning without a note telling the reader to read it
differently.

Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X

* docs(server): the SQLite half of the claim needed the same narrowing (BUG-2782)

Codex round 8, one P3 and it is the mirror image of round 6's. I qualified
"the handler answers 500" on the Postgres side and left the symmetric
sentence — "the same request is a clean 404 there" — universal on the
SQLite side, in the same paragraph. Not every request reaches a store
resolution on either backend; a gate that answers first keeps its own
status, and my own GET-only sweep recorded 102 x 404 alongside 5 x 403,
2 x 200, 1 x 401 and 1 x 503 on SQLite.

Fixing one direction of a symmetric claim and leaving the other is a shape
I have hit before and evidently do not catch by intention. The paragraph
now says the divergence is in what happens once a value REACHES the store,
which is the true and symmetric statement, with the distribution pasted.

Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
2026-08-25 23:20:53 -04:00

152 lines
8.1 KiB
Go
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
package server
import (
"net/http"
"strings"
"unicode/utf8"
)
// ValidatePath rejects a request whose percent-DECODED URL path is not a
// value the database can be asked about.
//
// Every handler that resolves a workspace, collection, item, comment or
// attachment from a path segment hands that segment to the store verbatim,
// and the store binds it into a text comparison. Postgres refuses a text
// parameter that is not valid UTF-8 or that contains a NUL (SQLSTATE 22021
// / 22P05) and the driver surfaces that as a query error. MOST handlers
// turn that into a 500, which is the defect; not all do, and the claim is
// deliberately not universal — handlers that collapse a resolution error
// into not-found already answer 404 (see handlers_timeline.go's
// `err != nil || item == nil`). The measurement below is what the
// distribution actually was.
//
// SQLite accepts both byte classes and simply matches nothing, so a request
// that reaches a store resolution answers 404 there instead. Not every
// request does, on either backend — an authorization or configuration gate
// that answers first keeps its own status, which is why the GET-only sweep
// below found 102 × 404 but also 5 × 403, 2 × 200, 1 × 401 and 1 × 503 on
// SQLite. The divergence is in what happens once the value REACHES the
// store, and it splits by BACKEND rather than by deployment: a SQLite
// install never sees the 500, and a Postgres install whose database
// encoding is UTF8 does — Pad Cloud and a self-hoster on Postgres alike,
// since UTF8 is initdb's default. (Under SQL_ASCII, Postgres accepts the bytes too; see
// validPathText below, which is where that qualification is spelled out.)
// An operator's alerting reads it as the server breaking when a client
// sent a path that cannot name anything.
//
// Measured before this middleware existed, driving every route that carries
// a path parameter with one segment set to "bad-%FF-x" — one request per
// parameter position, real values in the other positions. GET routes alone:
// 111 probes, Postgres answered 500 to 94, SQLite to 0. All methods: 247
// probes, Postgres 191, SQLite 0. (Both figures appear in this branch's
// history; they are the same sweep at two widths, not a disagreement.) This
// is a cross-cutting input rule, not a per-handler bug, which is why it
// lives here rather than at ~112 `chi.URLParam` call sites that each have
// to remember.
//
// WHY THE DECODED PATH, not what chi hands the handler. chi routes on
// r.URL.RawPath when it is non-empty and on r.URL.Path otherwise, and Go
// populates RawPath only when the client's escaping is NOT already
// canonical. Go escapes 0xff as uppercase "%FF", so:
//
// /…/items/bad-%FF-x → RawPath empty → chi routes on Path → URLParam
// yields the raw 0xff byte → reaches the store
// /…/items/bad-%ff-x → RawPath set → chi routes on RawPath → URLParam
// yields the literal text "bad-%ff-x" → harmless
//
// So the reachable vector is the CANONICAL uppercase form any ordinary
// client or proxy emits, and the harmless one is the oddity. Validating
// r.URL.Path answers both the same way, removes a behavioural difference
// that hangs on hex-digit case, and does not depend on chi continuing to
// prefer RawPath.
//
// It cannot refuse Pad's own URLs — but NOT because they are all ASCII,
// which was this comment's first claim and is false. Most are: slugs come
// from store.slugify, which appends only [a-z0-9-]; ids are UUIDs or hex;
// issue refs are a collection prefix plus digits. The exception is the one
// that matters, because it is the case a careless rule would break:
// DELETE /comments/{commentID}/reactions/{emoji} carries an EMOJI, which
// the web client sends as encodeURIComponent(emoji). That is valid UTF-8
// and must keep working, which is exactly what the rule permits and what
// TestValidatePathAllowsValidText pins with an emoji segment.
//
// A path whose decoded form is valid UTF-8 — including non-ASCII — is left
// for the router to handle, unchanged by this middleware. That is a
// statement about THIS middleware only, and not a promise that the handler
// sees the decoded text: by the RawPath rule above, "caf%C3%A9" reaches
// URLParam as "café" while the non-canonical "caf%c3%a9" reaches it as the
// literal text "caf%c3%a9", and an escaped "%2F" never becomes a separator.
// That is chi's pre-existing behaviour, unaffected either way by this
// change; it is written down here because the obvious reading of "passes
// through" is stronger than what is true.
//
// 400 rather than 404: not because the URI is malformed — "%FF" and "%00"
// are syntactically valid percent-encoded octets — but because the decoded
// value cannot be a resource identifier in this system at all. The answer
// does not depend on whether anything exists, so it is not an existence
// oracle. Scope is the PATH only; the query string is validated at its
// points of use (BUG-2774's validCursorID is the model this follows), and
// is measurably still open at the time of writing — see BUG-2784.
//
// The rejection must not be shaped differently from the errors the API
// writes for itself. Because this runs at the root, it short-circuits
// BEFORE the /api/v1 group's cors.Handler and jsonContentType, so a naive
// implementation answers with a JSON body typed text/plain and no CORS
// headers — which on a cross-origin deployment (PAD_CORS_ORIGINS set) the
// browser refuses to let the page read at all, turning a debuggable 400
// into an opaque network error. Hence `decorate`: setupRouter passes the
// SAME cors.Handler instance the group uses, and the rejection is served
// through it, so there is one CORS configuration and not two.
//
// BUG-2782.
func ValidatePath(decorate func(http.Handler) http.Handler) func(http.Handler) http.Handler {
var reject http.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
// Set explicitly rather than relying on jsonContentType, which is
// mounted below this middleware and never runs for a rejection.
// Without it net/http sniffs the JSON body as text/plain.
w.Header().Set("Content-Type", "application/json")
// Not "is not valid UTF-8": a NUL is valid UTF-8 and is rejected
// here too, so that wording would be false for half the inputs
// this refuses.
writeError(w, http.StatusBadRequest, "invalid_path",
"Request path contains invalid UTF-8 or a NUL byte")
})
if decorate != nil {
reject = decorate(reject)
}
return func(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if !validPathText(r.URL.Path) {
reject.ServeHTTP(w, r)
return
}
next.ServeHTTP(w, r)
})
}
}
// validPathText reports whether a decoded request path can be bound into a
// text comparison. The rule is derived from what the database refuses
// rather than from what a path "should" look like — no length bound, no
// character allow-list — for the reason validCursorID records: a bound that
// can only fire on a legitimate value is not protection.
//
// "What the database refuses" is narrower than it sounds, and the phrasing
// inherited from validCursorID overstated it. Postgres rejects these two
// classes when the database encoding is UTF8; under SQL_ASCII it would
// accept the same bytes. Pad does not create or configure that database —
// nothing here issues CREATE DATABASE or sets client_encoding, so the
// encoding is the operator's. UTF8 is initdb's default and is what the
// measurements above were taken against (postgres:17-alpine, defaults).
// SQLite is looser again: sqlite3_bind_text accepts arbitrary byte
// sequences, and an embedded NUL truncates or is otherwise undefined
// rather than erroring.
//
// So this is not the intersection of two engines' rules, and it is not a
// rule the database hands us. It is the strictest reading, applied
// uniformly at the transport, so that a request cannot get one answer on
// SQLite and another on Postgres — which is the actual defect being fixed.
func validPathText(p string) bool {
return utf8.ValidString(p) && !strings.ContainsRune(p, 0)
}