Skip to content

epic 2.1: a malformed body answers 400, and a redirect cannot point at itself - #53

Merged
kuyazee merged 3 commits into
mainfrom
task/epic-2-1-c
Aug 29, 2026
Merged

epic 2.1: a malformed body answers 400, and a redirect cannot point at itself#53
kuyazee merged 3 commits into
mainfrom
task/epic-2-1-c

Conversation

@kuyazee

@kuyazee kuyazee commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Two items from epic 2.1, plus one commit closing every finding the review gauntlet confirmed.

Items

  • T2.1.18 - a malformed request body answers 400 instead of 500 (commit 79f0b05)
  • T2.1.8 - a self-referencing redirect is refused, and each key shows its redirect count (commit 6fd8cc7)
  • Review findings from all three lenses, eleven of them (commit f3f6b4b)

Tests: 108 to 122, no failures. Smoke: 215 to 217 ok-lines, exit 0.

T2.1.18 - a malformed request body answers 400 instead of 500

express.json() runs in strict mode, so a body of null, "x" or 5 never reached a route.
body-parser threw a SyntaxError carrying status: 400, expose: true and
type: 'entity.parse.failed', and the error handler ignored all three: every typo answered
{"error":"internal server error"} with a 500 and a full stack trace in the log. A client could not
tell "I sent bad JSON" from "the server broke", and a retry loop keyed on 5xx retried a request that
could never succeed.

lib/errors.js gained clientFacingError(err), which holds the whole decision about which errors
get to name themselves, so it can be tested without going through express. The rules: a parse
failure answers 400 invalid JSON body: expected a JSON object, a body over the limit keeps the
existing 413, and anything else needs expose === true together with an integer 4xx status before
its message leaves the process. A 5xx never speaks for itself, and an ApiError built with a 5xx is
refused there too so the next person who writes new ApiError(500, err.message) cannot leak through
it. Client errors are no longer logged.

The message names the real problem rather than the apparent one. null, 5, "x" and true are
all valid JSON; what refuses them is strict mode wanting an object or an array, so "invalid JSON"
alone sent a reader hunting for a syntax error that was not there.

Six parser messages can now reach a client that could not before, all of them caller-supplied text
echoed back: invalid JSON body: expected a JSON object (400), unsupported charset "X" (415),
unsupported content encoding "X" (415), content encoding unsupported (415), request aborted
(400), and request size did not match content length (400). The security lens walked the dependency
tree for anything else carrying expose: true and found the two candidates both blocked: send
forces expose: false on its fs errors, and express never sets expose on a param-decode URIError.
docs/api.md used to promise that every failure other than the listed ones was a bare 500. That
sentence is now false, so it was rewritten rather than left to rot.

Tests: test/errors.test.js covers the decision directly, red first at
SyntaxError: The requested module '../lib/errors.js' does not provide an export named 'clientFacingError'.
Smoke covers all four bad-body shapes on POST /api/artifacts.

The bare-500 half of the done-when is proved by unit test, not by smoke. Every throw reachable over
HTTP is an ApiError with its own status, so triggering a real internal error from a shell script
needs a broken filesystem. The QA lens confirmed the behaviour out of band by making the data
directory read-only: 500 {"error":"internal server error"} with the EACCES stack going to stderr
only.

T2.1.8 - a self-referencing redirect is refused, and each key shows its redirect count

A redirect target pointing back at its own slug looped until the visitor's browser gave up, and
nothing detected it. Separately, nothing surfaced how many redirects one publish-scoped key had
minted, which is the knob a self-hoster wants when a key leaks and the domain starts hosting phishing
hops.

lib/redirect.js gained pointsAtOwnSlug(target, slug, baseUrl) and countRedirectsByKey(metas).
The refusal runs on POST and PUT before the first storage.put, on a PATCH rename before
storage.move, and on duplicate before storage.copySlug, so a refusal writes and moves nothing.
Redirects now store meta.keyId, GET /api/keys reports a redirects count per key, and both the
CLI and the dashboard key row show it when it is not zero.

On the "capped or surfaced" choice in the done-when: surfaced. A cap changes behaviour for anyone
already using redirects and needs a number nobody has picked; a count needs no decision and is the
thing an operator actually looks at.

keyId is not in PUBLIC_META_FIELDS, so it never reaches GET /api/artifacts. Smoke asserts that,
and the security lens checked every other way a meta object leaves the process (the serve paths,
/source, the MCP tools, the dashboard payload, capability links, the QR route, ?raw=1) and found
no leak.

What the guard is, plainly. It catches the typo, not the attacker. It compares the target's host
and port against BASE_URL, so it is off entirely on a deploy that never set BASE_URL, and any
other name or IP for the same server walks past it. Two redirects pointing at each other, and a
cross-host chain that comes back here, both need a lookup a single publish does not have. All of this
is written into the comment and into docs/formats.md rather than being sold as loop prevention.

Tests: four cases in test/redirect.test.js, red first at
ReferenceError: pointsAtOwnSlug is not defined. Smoke covers three target shapes on POST, a PUT
repoint, a PATCH rename onto the target, a check that the refused rename left the artifact in place,
and the count going 0 to 3 with html not counted, a PUT not double-counting, and a delete
decrementing.

Review findings

Three lenses ran against the branch: adversarial, security, and QA plus UX. Eleven findings were
confirmed and reproduced against a live server. All eleven are fixed in f3f6b4b. None were filed for
later.

The two that mattered. Both were in the feature this branch shipped, and both were reachable
through a supported route.

POST /api/artifacts/:slug/duplicate skipped the self-redirect guard entirely. copyArtifact
re-ran parseRedirectTarget but never pointsAtOwnSlug, and its own comment said "a copy is a
publish, so it answers to the publish rules", which was the one rule it did not answer to. Publish
pub-src pointing at /a/pub-dst, duplicate it to slug pub-dst, and the copy served a 301 to
itself while the identical target was refused on a direct POST.

The same route dropped key attribution, so the count undercounted. A key at redirects: 1 could
duplicate its own redirect twice, end up with three live hops, and still read 1. That is one extra
publish-scoped call to defeat the number the feature exists to surface.

The rest.

  • A dashboard or bootstrap PUT erased the attribution, because the redirect branch cleared
    meta.keyId when the caller had none. A dashboard session has no keyId at all, so repointing a
    leaked key's hop from the dashboard, which is exactly what an operator does on finding one, reset
    the count to zero. The clear is gone from the redirect branch and stays in the other.
  • The rename guard read meta.target directly, so a redirect published before that field existed
    (target only in source.url) renamed straight into a loop. It now resolves the target the way the
    serve path does.
  • Three shapes walked past pointsAtOwnSlug, each reproduced as a live loop: an uppercase path
    (express routes case-insensitively, the compare did not), a trailing-dot host (localhost.), and
    decodeURIComponent over the whole pathname turning /a%2fhop into a false positive. The compare
    now uses hostname plus port with one trailing dot stripped, and decodes only the captured segment,
    which is what express does.
  • clientFacingError's ApiError branch returned err.message for any status. Inert today, since
    no ApiError is built with a 5xx, and now refused outright.
  • docs/api.md promised a contract the code no longer honoured. Rewritten with the six parser
    messages listed.
  • docs/auth.md and docs/formats.md called the count "live" while it counts disabled and expired
    redirects too. Reworded to "stored", which is smaller than filtering and truer: a disabled record
    is one PATCH away from serving.
  • POST /api/keys returned a shape without redirects while GET /api/keys had it. Both clients
    already guarded the missing field, so this is one line for shape parity.

Smoke gained a case per behaviour fix. The replace case now uses a different principal from the one
that published, which is why the erased-attribution bug got past the first pass.

Confirmed and deliberately not fixed. Each is either pre-existing or needs a decision this branch
should not make.

  • GET /api/keys now reads every artifact's meta to build the count. GET /api/artifacts already
    did the same scan at a lower scope, so this is an operational cost on a large instance, not a new
    vector.
  • Bearer-key 401s on /api/* and /mcp are neither logged nor rate limited, so key grinding leaves
    no trace. Pre-existing, outside this branch, and worth its own item.
  • Two redirects pointing at each other, and a cross-host chain back here. Both need a lookup a single
    publish does not have. Documented in docs/formats.md.
  • pointsAtOwnSlug assumes BASE_URL has no path prefix. Routes mount at the root in this app.
  • A malformed body on /mcp answers a plain {"error":...} rather than a JSON-RPC error object. It
    did this before too, as a 500, so the shape is wrong but unchanged.

Verification

  • npm test: 122 pass, 0 fail. Was 108 on main.
  • bash .github/workflows/smoke.sh http://localhost:3011 test against a clean data directory:
    exit 0, 217 ok-lines, all smoke tests passed. Was 215 before the fix pass.
  • git diff origin/main..HEAD -U0 | grep "^+" | grep "—\|–" returns nothing.

https://claude.ai/code/session_01JPiFnXvsTMjDJUdRZNwTea

kuyazee added 3 commits August 29, 2026 03:42
@kuyazee
fix(T2.1.18): a malformed body answers 400 instead of 500
79f0b05 
express.json() runs in strict mode, so a body of null, "x" or 5 never reaches a
route. body-parser throws first, carrying status 400, expose: true and
type 'entity.parse.failed'. The error handler ignored all three, answered
{"error":"internal server error"} with a 500, and logged a stack trace per typo.
A client could not tell "I sent bad JSON" from "the server broke", and a retry
loop keyed on 5xx retried a request that can never succeed.

The decision about which errors get to name themselves moves into
lib/errors.js as clientFacingError(), so a test can hand it the shapes
body-parser throws without booting express. It answers a parse failure as 400
"invalid JSON body", keeps the existing 413 for a body past the limit, and
passes through any other body-parser refusal (an encoding or charset it cannot
read) on its own message. A message only leaves the process when the error
carries both expose: true and a 4xx status: expose alone shows up on errors
this repo did not write, and a 4xx alone says nothing about whether the text
names an internal path. A 5xx never speaks for itself. Everything else still
gets the bare 500 and the log line.

Client errors are no longer logged. A malformed body is a typo on the other
end, and a stack trace per typo buries the logs that matter.

Smoke covers the 400 half on POST /api/artifacts across four bad bodies, checks
the message names the format, checks a good body with a bad field still gets the
route's own message, and checks nothing about the parser leaks. The 500 half has
no trigger from outside a healthy server, so it is pinned in test/errors.test.js
rather than faked in smoke.

Claude-Session: https://claude.ai/code/session_01JPiFnXvsTMjDJUdRZNwTea
@kuyazee
feat(T2.1.8): refuse a self-referencing redirect and show each key's …
6fd8cc7 
…redirect count

Two halves of the same item, both known and accepted when T2.1.4 merged.

A redirect whose target is its own slug answers its own 301, so a visitor's
browser hops until it gives up on an error page. The server never follows a
target, so nothing amplifies, but nothing caught it either and the publisher
found out from a browser rather than from the API. pointsAtOwnSlug() in
lib/redirect.js decides what "its own slug" means: the BASE_URL host, either
scheme (a proxy terminates TLS and the origin behind it answers on the other
one), the path /a/<slug> with or without a trailing slash, and the path segment
decoded first because express decodes it too. It runs on POST, on a PUT that
repoints a slug, and on a PATCH that renames a redirect onto the slug it
already targets, which closes the same loop with no target changing. Two
artifacts pointing at each other, and a target on another host that redirects
back here, still get through: both need a lookup a publish does not have.

The volume half surfaces rather than caps. A cap changes what an existing key
is allowed to do, which is the self-hoster's call to make, and a count they can
see is what tells them to make it. A redirect now stores the managed key that
published it in meta.keyId, and GET /api/keys counts them per key off the
stored records, so a delete comes off the count and a repointing PUT is the
same hop rather than a new one. keyId is not in PUBLIC_META_FIELDS, so it never
rides out on the artifact list every read-scoped key can call. Publishes from
the bootstrap key or a dashboard session name no key and land in nobody's
count. The dashboard key row and `artifacts keys list` print the number on keys
that have any, and leave it off the ones that do not.

listArtifactMetas() splits out of listArtifacts so the key route can read a
field the list API does not hand out.

Claude-Session: https://claude.ai/code/session_01JPiFnXvsTMjDJUdRZNwTea
@kuyazee
fix(T2.1.8, T2.1.18): close the review findings
f3f6b4b 
Three review passes found eleven problems in the redirect self-reference
guard, the per-key redirect count and the body-parser error contract. Every
one was reproduced against a running server first.

Duplicate skipped the self-reference guard. POST /api/artifacts/:slug/duplicate
re-parsed the target but never asked whether it pointed at the slug the copy
was landing on, so duplicating a hop onto the very slug it targets built the
loop a direct publish refuses. The check now runs before storage.copySlug, off
the original's stored target, so a refusal writes nothing.

Duplicate dropped key attribution. The route passed no keyId and the copied
meta never carried one, so a key could mint hops through /duplicate all day
while GET /api/keys stood still. req.principal.keyId now rides through
duplicateArtifact into the copy, the way storeArtifact already takes it.

A dashboard or bootstrap PUT erased the attribution. storeArtifact cleared
meta.keyId whenever the principal had none of its own, so repointing a leaked
key's redirect, which is the first thing an operator does, deleted the count
that showed them the leak. The redirect branch now sets keyId when there is
one and leaves what is stored alone otherwise. The non-redirect branch still
clears it.

The rename guard missed a target stored only in source.url. applyPatch read
meta.target straight off the record. On a redirect published before that field
existed, new URL(undefined) threw inside the check, which answered false, and
the rename closed the loop. The target is now resolved the way the serve path
resolves it, through the new storedTargetPointsAtSlug helper.

pointsAtOwnSlug missed three shapes, all live loops. Express routes
case-insensitively, so /a/CASE-LOOP reached slug case-loop and walked past a
case-sensitive compare. A trailing dot on the host is the same name to DNS but
not to a string compare, so http://localhost.:3021/a/dot-loop was accepted.
And decodeURIComponent ran over the whole path, so /a%2fhop decoded to /a/hop
and was refused even though it reaches a 404. The compare now folds case,
strips one trailing dot from each host name, and splits the path before
decoding the captured segment, which is what express does.

docs/api.md claimed a contract the code no longer honours. It said every
failure other than the JSON one is a bare 500. Any body-parser refusal with a
4xx and expose set answers for itself now: Content-Encoding: br returns 415
with the parser's own text. The bullet lists the full set that can reach a
client, all of it caller-supplied.

"invalid JSON body" named the wrong problem. null, 5, "x" and true are all
valid JSON; what refuses them is express.json() strict mode wanting an object
or an array. The message is now "invalid JSON body: expected a JSON object".

clientFacingError returned an ApiError's message at any status. No route
builds a 5xx one today, so nothing leaked, but new ApiError(500, err.message)
would have shipped an internal string the day someone wrote it. A 5xx, or a
status that is not an integer, now falls through to the bare 500.

The redirect count covers stored records, not serving ones. countRedirectsByKey
counts disabled and expired redirects too. Rewording was smaller than
filtering, and a record that is switched off is one PATCH from serving again,
so docs/auth.md and docs/formats.md now say stored and spell out what counts.

The self-reference guard is documented as what it is. It catches the typo, not
the attacker: it is off entirely on a deploy that never set BASE_URL, and any
other name or IP for the same server walks past it. The comment in
lib/redirect.js and the bullet in docs/formats.md say so instead of implying it
prevents loops.

POST /api/keys now returns redirects: 0, so a create response has the same
shape as a row from GET /api/keys.

Tests: 122 unit tests pass, up from 120. New smoke cases cover the duplicate
guard, the duplicate's attribution, and a replace by a different principal
than the one that published, which is the case the existing replace test
missed. The suite is 217 ok lines, exit 0.

Claude-Session: https://claude.ai/code/session_01JPiFnXvsTMjDJUdRZNwTea
@kuyazee
kuyazee merged commit a2d171e into main Aug 29, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@kuyazee