Security review — 2026-09-10

A pass over the whole surface: every #[page] and #[route] in src/infrastructure/web/, the git transport, the CLI, the outbound paths, and the dependency tree. Steid 0.4.0, branch security/review-2026-09-10, verified against a running instance on :3135 unless a finding says otherwise.

Seven findings: one high, four medium, two low. All seven fixed, one commit each. Four things are deferred and are carried into current.md.

The headline is that the boundary work is in good shape — no enumeration hole, no authorization gap, no injection — and every finding is about resource limits or about a value reaching somewhere it was never checked for. The two that matter most are both "a library's default is not what we assumed": Topcoat reads a request body with usize::MAX, and ureq follows ten redirects.

What was looked for and not found

Stated first so a later reader knows these were checked rather than skipped.

• Enumeration. A private repository, an absent one and a mistyped handle all answer the same. The page 404s are byte-identical (diff of the two bodies), the git transport answers a uniform 401 challenge for all three, and /api/setup has one refusal for a wrong token, an already-claimed instance and a bad password. /metrics and the detailed /api/instance answer 404 rather than 401 for a wrong bearer. /auth/forgot answers the same 303 whatever the address was.
• Authorization. application/access.rs is the only place that decides, and access_tests.rs already walks writable_repo against changeable_repo for every actor. No write is reachable without Role::Owner. The side channels named in the brief all resolve through the same predicates: feeds, search, archive, blame, the compare page, the /issues/{n} ↔ /pulls/{n} redirect (which only ever redirects within a repository the actor already read), the JSON API, and the per-file last-commit column.
• Injection. Markdown never passes raw HTML through and allowlists link schemes on the control-stripped string; the Atom writer escapes every value through one function; both Content-Disposition builders reduce the filename to a set that cannot end the quoted string; the archive prefix is sanitised in the use case; and every git argument that could come from a URL is either a resolved object id or is preceded by --, with RefName and RepoPath refusing a leading - besides.
• Secrets. Nothing in eprintln!/println! carries a token, a password, a reset link or an SMTP URL — the reset link is printed by steid reset-password, which is its whole purpose. SetupToken, ResetSecret, TokenSecret and Secret all have redacting Debug. One exception, fixed below.
• Headers. CSP, nosniff, Referrer-Policy: no-referrer and X-Frame-Options are on every response including error responses. HSTS is Caddy's job and deploy/Caddyfile is where it belongs — Steid terminates no TLS. Cookie flags are Topcoat's SessionConfig::default(), downgraded only by STEID_INSECURE_COOKIES, which announces itself at boot. Cache-Control was the gap; fixed below.
• Dependencies. cargo audit: zero vulnerabilities, three standing warnings — bincode 1.3.3 unmaintained (RUSTSEC-2025-0141), yaml-rust 0.4.5 unmaintained (RUSTSEC-2024-0320), chacha20 0.10.1 yanked. All three are transitive, none is a vulnerability, and none has an upgrade that does not mean waiting on syntect and Topcoat.

The surface

Authentication is the session cookie unless a row says otherwise. "Rate limit" names the limiter from web::rate_limit; the login limiter is 10 per client and 60 globally per minute, git-auth 60/600 per minute, archive 10/60 per minute, instance 60/600 per minute, metrics 30/300 per minute, reset 30/120 per minute, backup 2/8 per hour. A refusal is "uniform" when a caller cannot tell an absent thing from one they may not see.

Pages, read

Pages, write — every one CSRF-checked before its use case

Routes

Commands

Findings

1 · High · No limit on a request body, so any POST is a memory amplifier

Form<T> and Json<T> both collect with to_bytes(body, usize::MAX) — read in the vendored topcoat-router-0.5.0/src/content/{form,json}.rs, and there is no builder knob for it. So every form endpoint held whatever arrived, /auth/login and /api/setup included, both anonymous and both reachable before any authentication.

Measured against :3135:

$ ps -o rss= -p $(pgrep -f target/debug/steid)      #  20080 KB
$ curl -X POST --data-binary @<(head -c 200000000 /dev/zero | tr '\0' a) \
       -H 'Content-Type: application/x-www-form-urlencoded' \
       http://127.0.0.1:3135/auth/login              #  400
$ ps -o rss= -p $(pgrep -f target/debug/steid)      # 811616 KB

One request, 20 MB → 811 MB. A handful in parallel is the machine, and the reply is a 400 either way, so nothing about the endpoint even discourages it.

Fixed in 842e3f0: web::body_limit, applied in the one layer at /. A mebibyte, which is three orders of magnitude above the largest real form. A declared Content-Length over the cap is refused with 413 before a byte is read; an undeclared one fails while it arrives and surfaces as 400. The two git RPC paths are exempt, matched on the whole four-segment route rather than the last segment — RepoName reserves neither git-upload-pack nor git-receive-pack, so a repository may be called that, and the test in the module is what caught it.

Re-measured: 413 and 400 respectively, resident memory flat at 21 MB, and a 4 MiB git push plus the clone back both succeed.

2 · Medium · A reset token from a URL could panic the worker answering it

POST /auth/reset/{token} interpolated the path parameter into see_other, and a path parameter arrives percent-decoded — so /auth/reset/a%0d%0ab produced a Location carrying a real CRLF. see_other refuses that by panicking, not by returning an error.

$ curl -o /dev/null -w '%{http_code}\n' -X POST \
       -d 'password=aaaaaaaaaaaa&confirmation=aaaaaaaaaaaa' \
       'http://127.0.0.1:3135/auth/reset/a%0d%0ab'
000
# server log:
thread 'tokio-rt-worker' panicked at topcoat-router-0.5.0/src/error/redirect.rs:140:50:
redirect uri is not a valid header value: InvalidHeaderValue

000 is curl's "no response at all". The process survives — the unwind is per task — but an anonymous caller can drop a connection and write a panic to the log at will, and this is the shape header injection would have taken had HeaderValue been laxer.

Fixed in 2c5f03a: the token is percent-encoded on its way into the Location. Encoding rather than a shape check, because a real token is 64 hex characters and comes back from encode byte for byte — so the legitimate path is unchanged and there is no second spelling of ResetSecret's shape to drift out of step with the domain. Two tests pin it. Every other see_other/redirect call site in the web layer was checked: the rest take a literal or a validated handle.

3 · Medium · Webhook delivery followed redirects, walking past the SSRF check

check_resolves_publicly resolves the saved URL's host and refuses unless every address is public. ureq's default max_redirects is 10 (ureq-3.3.0/src/config.rs:874), and nothing re-checked the target of a redirect. So a receiver on a perfectly public host answering 302 Location: http://169.254.169.254/latest/meta-data/ sent the delivery there past both halves of the rule — and worse than a blind request, because the response body is stored on the delivery row and rendered on the deliveries page, so it reads back.

Only the instance owner can add a hook today, so this is self-inflicted for now. It is exactly the assumption Milestone 7 removes, and the whole reason WebhookUrl is as narrow as it is.

Fixed in 6b69831: max_redirects(0). The 3xx is then recorded as the delivery's own answer, which is the honest thing to show a receiver that redirects; nothing that legitimately receives a webhook needs one. A test reads the setting back off the agent.

4 · Medium · The import clone followed redirects too

The same hole, one layer over. resolve_public_host checks every address the source's host answers with, and then git clone runs with http.followRedirects at its default of initial — which permits the first request to be redirected and re-resolves whatever host it is sent to.

Fixed in 4be05bf: -c http.followRedirects=false on the clone, verified accepted by the installed git. The cost is real and is the reason this one is a trade rather than a free fix: a source whose URL only works through a redirect — a repository renamed on its host — now fails the import instead of silently following. The failure names the URL and the real one can be pasted in its place, which is better than a check that reads as a guarantee and is not one.

5 · Medium · A repository search collected everything git matched

grep was the last read on an anonymous page still using the uncapped run. The search page takes its query from a URL, has no rate limit of its own, and keeps at most SEARCH_MAX_MATCHES (1000) of them — but ?q=e had git write every matching line in the repository into memory first, and only then discarded all but those.

$ git grep -I -n -z -F --column --no-color -e e HEAD -- | wc -c
5305298

5.3 MB from one letter over Steid's own source, unauthenticated, per request, for any public repository. A kernel-sized tree is orders of magnitude worse, and the 20-second timeout is a long time to spend filling a Vec.

Fixed in 56dc6cd: run_capped_allowing, a 4 MiB ceiling, and the exception run_allowing carried — grep's exit 1 for "matched nothing" — carried over with it. A truncated tail costs a result nobody was going to be shown, and a partial final record simply fails to parse. Both halves are pinned by a test.

6 · Low · The operator bearer token was the one secret with a plain Debug

AppConfig derives Debug and lives in the app context, which is why setup_token and smtp_url are Secret. operator_token was a bare String — and it is the worst of the three to print, because unlike the setup token it is long-lived and stays in context for the life of the process. Nothing renders the config today; this is the class the type exists to prevent.

Fixed in 6eba149, with the test the other two already had.

7 · Low · No Cache-Control on anything

Every URL under /{handle}/… is the same shape whether it renders something public or private, so an intermediary had nothing to tell them apart by, and a browser was free to heuristically cache a private repository's page.

$ curl -D - -o /dev/null http://127.0.0.1:3135/ada/repos/secret | grep -i cache
(nothing)

Fixed in c410dc3: private, no-cache as a default in the same layer, via or_insert, so the three responses that already decided keep their answer — the asset routes' year-long public, immutable, git's must-revalidate on an advertisement, and the backup's no-store. no-cache rather than no-store for the archive's sake: a response the browser was told not to store has nothing left to answer If-None-Match with. Checked in the running app — all four are as described and the archive's 304 still happens.

Deferred

Carried into current.md.

• The search page has no rate limit. The byte cap bounds the memory; the CPU is still a full git grep per request on an anonymous page, and it is the most expensive unauthenticated read after the archive. It wants a SearchLimiter alongside ArchiveLimiter — mechanical, but a limiter is a policy decision (how many, over what window, and whether an empty query counts) and those belong to the user.
• CSRF on the signed-out forms. /auth/login, /auth/setup, /auth/forgot and /auth/reset/{token} are exempt with a written reason: there is no session to bind a double-submit token to. Login CSRF is real and needs a pre-session cookie. Already noted by 0008; restated here because a review that did not mention it would read as having missed it.
• DNS rebinding on both outbound paths. Both re-resolve at connect time, so a record whose TTL expires between the check and the dial still gets through. Closing it means a custom resolver, or pinning the address and setting Host/SNI by hand — for ureq and for git separately. Recorded in both modules already.
• run still collects without a byte cap for the reads that are not grep or diff — log, for-each-ref, cat-file. Each is bounded by --max-count, by the ref count, or by an object size checked from its header first, so none is reachable in the shape grep was. Worth a sweep if a page ever asks git something open-ended again.

Verification

• cargo test — 1309 pass, 0 fail.
• cargo clippy --all-targets — zero warnings.
• cargo fmt — clean.
• cargo audit — zero vulnerabilities, the three standing warnings above.
• Against a real instance on :3135: claimed headlessly, signed in, a public and a private repository created, a 4 MiB push and clone over HTTP Basic with a personal access token, and every probe quoted above.