# 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

| Route | Auth | Rate limit | To an anonymous caller | Uniform |
|---|---|---|---|---|
| `GET /` | none | — | redirects to `/auth/setup` when unclaimed, else to the owner's profile | n/a |
| `GET /{handle}` | none | — | public profile and the repositories that caller may see | yes |
| `GET /{handle}/repos` | none | — | same, paged | yes |
| `GET /{handle}/repos/{name}` | none | — | public repositories only; private and absent both 404 | yes |
| `GET …/tree/{rev}`, `…/tree/{rev}/-/{*path}` | none | — | as above | yes |
| `GET …/raw/{rev}/-/{*path}` | none | — | as above; `application/octet-stream` + `attachment` + `default-src 'none'; sandbox`, capped at `MAX_RAW_BYTES` (10 MiB) | yes |
| `GET …/log`, `…/log/{rev}`, `…/log/{rev}/-/{*path}` | none | — | as above | yes |
| `GET …/blame/{rev}/-/{*path}` | none | — | as above; one `git blame` under the 20s cap | yes |
| `GET …/commits/{sha}`, `…/compare`, `…/compare/{spec}` | none | — | as above; diffs read through a byte cap | yes |
| `GET …/branches`, `…/tags` | none | — | as above | yes |
| `GET …/search` | none | **none** — deferred | as above; grep now read through a 4 MiB cap | yes |
| `GET …/issues`, `…/issues/{number}`, `…/issues/new` | none | — | as above; `new` needs ownership and 404s otherwise | yes |
| `GET …/pulls`, `…/pulls/{number}`, `…/{number}/commits`, `…/{number}/files`, `…/pulls/new` | none | — | as above; the merge preview is cached per branch pair | yes |
| `GET …/labels` | none | — | as above | yes |
| `GET …/settings`, `…/settings/hooks`, `…/settings/hooks/{hook}` | owner | — | 404 | yes |
| `GET /{handle}/settings`, `…/settings/tokens` | owner | — | 404 | yes |
| `GET /auth/login`, `/auth/setup`, `/auth/forgot` | none | — | the form; `/auth/setup` redirects away once claimed | n/a |
| `GET /auth/reset/{token}` | the token | reset | the form, or the dead-link page — unknown, expired and spent are one answer | yes |

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

| Route | Auth | To an anonymous caller | Uniform |
|---|---|---|---|
| `POST /{handle}/repos/new`, `…/repos/import` | owner | 403 (CSRF runs first, and consults nothing) | yes |
| `POST …/{name}/import/retry` | owner | 403 | yes |
| `POST …/issues`, `…/issues/{n}/comments`, `…/state`, `…/edit`, `…/labels` | owner | 403 | yes |
| `POST …/pulls`, `…/pulls/{n}/comments`, `…/state`, `…/edit`, `…/labels`, `…/merge` | owner | 403 | yes |
| `POST …/labels`, `…/labels/{id}/edit`, `…/labels/{id}/delete` | owner | 403 | yes |
| `POST …/settings`, `…/settings/archive`, `…/default-branch`, `…/rename`, `…/delete` | owner | 403 | yes |
| `POST …/settings/hooks`, `…/hooks/{h}/delete`, `…/toggle`, `…/deliveries/{d}/redeliver` | owner | 403 | yes |
| `POST /{handle}/settings` | owner | 403 | yes |
| `POST /{handle}/settings/tokens`, `…/tokens/{token}/revoke` | owner | 403 | yes |

### Routes

| Route | Auth | Rate limit | To an anonymous caller | Uniform |
|---|---|---|---|---|
| `GET /healthz` | none | — | `ok` or `unavailable`, and nothing else | n/a |
| `GET /robots.txt` | none | — | a fixed document | n/a |
| `GET /metrics` | operator bearer, constant-time | metrics, **only on failure** | 404 — the surface does not exist without a token | yes |
| `GET /api/instance` | operator bearer, optional | instance, **only unauthenticated** | `{"claimed":…}` and nothing else | yes |
| `POST /api/setup` | setup token | login | one 404, byte-identical for every refusal, before Argon2 | yes |
| `GET /api/me` | session | — | 401 | n/a |
| `GET /api/users/{handle}` | none | — | the public profile; 404 for an unknown handle | yes |
| `GET /api/users/{handle}/repos` | none | — | only visible repositories; `[]` rather than 404 for a handle that owns nothing visible | yes |
| `GET /{handle}/feed.atom`, `…/commits.atom`, `…/issues.atom` | none | — | only what the caller may see | yes |
| `GET …/archive/{file}` | none | archive, **counted before the 404** | public only; `ETag` is the resolved commit | yes |
| `GET /{handle}/settings/backup.tar.gz` | instance owner **and** profile owner | backup, **after** the ownership check | 404 | yes |
| `GET …/{repo}/info/refs` | token (Basic) or session | git-auth, only when a credential is presented | 401 challenge for private *and* absent; no service parameter is a 404, so the dumb protocol is never served | yes |
| `POST …/git-upload-pack`, `…/git-receive-pack` | as above | as above | 401 challenge; archived is 403 with a sentence, importing is 409 | yes |
| `POST /auth/login`, `/auth/setup` | none | login, **before Argon2** | 303 back to the form, saying nothing | yes |
| `POST /auth/forgot` | none | login | the same 303 whatever the address | yes |
| `POST /auth/reset/{token}` | the token | reset | 303; dead and unknown are one answer | yes |
| `POST /auth/logout` | session | — | 403 without a CSRF token | yes |
| `POST /{handle}/settings/password` | owner | — | 403 without a CSRF token | yes |

### Commands

| Command | Auth | Reveals |
|---|---|---|
| `steid` | — | serves; prints the setup token once, only while unclaimed, only when it generated it |
| `steid backup <destination>` | the filesystem | a full copy of the instance. Does not migrate, deliberately |
| `steid restore <archive>` | the filesystem | refuses anything but an empty instance, before a byte is written |
| `steid reset-password <handle>` | the filesystem | a live reset link. **Prints nothing and still exits 0 for an unknown handle** — distinguishable on purpose, since the caller already has the SQLite file |

## 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.