| 1 | # Security review — 2026-09-10 |
| 2 | |
| 3 | A pass over the whole surface: every `#[page]` and `#[route]` in |
| 4 | `src/infrastructure/web/`, the git transport, the CLI, the outbound paths, and the |
| 5 | dependency tree. Steid 0.4.0, branch `security/review-2026-09-10`, verified against a |
| 6 | running instance on **:3135** unless a finding says otherwise. |
| 7 | |
| 8 | **Seven findings: one high, four medium, two low. All seven fixed**, one commit each. |
| 9 | Four things are deferred and are carried into `current.md`. |
| 10 | |
| 11 | The headline is that the boundary work is in good shape — no enumeration hole, no |
| 12 | authorization gap, no injection — and every finding is about *resource* limits or about |
| 13 | a value reaching somewhere it was never checked for. The two that matter most are both |
| 14 | "a library's default is not what we assumed": Topcoat reads a request body with |
| 15 | `usize::MAX`, and `ureq` follows ten redirects. |
| 16 | |
| 17 | ## What was looked for and not found |
| 18 | |
| 19 | Stated first so a later reader knows these were checked rather than skipped. |
| 20 | |
| 21 | - **Enumeration.** A private repository, an absent one and a mistyped handle all answer |
| 22 | the same. The page 404s are byte-identical (`diff` of the two bodies), the git |
| 23 | transport answers a uniform 401 challenge for all three, and `/api/setup` has one |
| 24 | refusal for a wrong token, an already-claimed instance and a bad password. `/metrics` |
| 25 | and the detailed `/api/instance` answer 404 rather than 401 for a wrong bearer. |
| 26 | `/auth/forgot` answers the same 303 whatever the address was. |
| 27 | - **Authorization.** `application/access.rs` is the only place that decides, and |
| 28 | `access_tests.rs` already walks `writable_repo` against `changeable_repo` for every |
| 29 | actor. No write is reachable without `Role::Owner`. The side channels named in the |
| 30 | brief all resolve through the same predicates: feeds, search, archive, blame, the |
| 31 | compare page, the `/issues/{n}` ↔ `/pulls/{n}` redirect (which only ever redirects |
| 32 | *within* a repository the actor already read), the JSON API, and the per-file |
| 33 | last-commit column. |
| 34 | - **Injection.** Markdown never passes raw HTML through and allowlists link schemes on |
| 35 | the *control-stripped* string; the Atom writer escapes every value through one |
| 36 | function; both `Content-Disposition` builders reduce the filename to a set that |
| 37 | cannot end the quoted string; the archive prefix is sanitised in the use case; and |
| 38 | every `git` argument that could come from a URL is either a resolved object id or is |
| 39 | preceded by `--`, with `RefName` and `RepoPath` refusing a leading `-` besides. |
| 40 | - **Secrets.** Nothing in `eprintln!`/`println!` carries a token, a password, a reset |
| 41 | link or an SMTP URL — the reset link is printed by `steid reset-password`, which is |
| 42 | its whole purpose. `SetupToken`, `ResetSecret`, `TokenSecret` and `Secret` all have |
| 43 | redacting `Debug`. One exception, fixed below. |
| 44 | - **Headers.** CSP, `nosniff`, `Referrer-Policy: no-referrer` and `X-Frame-Options` are |
| 45 | on every response including error responses. HSTS is **Caddy's job** and |
| 46 | `deploy/Caddyfile` is where it belongs — Steid terminates no TLS. Cookie flags are |
| 47 | Topcoat's `SessionConfig::default()`, downgraded only by `STEID_INSECURE_COOKIES`, |
| 48 | which announces itself at boot. `Cache-Control` was the gap; fixed below. |
| 49 | - **Dependencies.** `cargo audit`: **zero vulnerabilities**, three standing warnings — |
| 50 | `bincode` 1.3.3 unmaintained (RUSTSEC-2025-0141), `yaml-rust` 0.4.5 unmaintained |
| 51 | (RUSTSEC-2024-0320), `chacha20` 0.10.1 yanked. All three are transitive, none is a |
| 52 | vulnerability, and none has an upgrade that does not mean waiting on `syntect` and |
| 53 | Topcoat. |
| 54 | |
| 55 | ## The surface |
| 56 | |
| 57 | Authentication is the **session cookie** unless a row says otherwise. "Rate limit" names |
| 58 | the limiter from `web::rate_limit`; the login limiter is 10 per client and 60 globally |
| 59 | per minute, git-auth 60/600 per minute, archive 10/60 per minute, instance 60/600 per |
| 60 | minute, metrics 30/300 per minute, reset 30/120 per minute, backup 2/8 per **hour**. A |
| 61 | refusal is "uniform" when a caller cannot tell an absent thing from one they may not see. |
| 62 | |
| 63 | ### Pages, read |
| 64 | |
| 65 | |
| 66 | |
| 67 | | `GET /` | none | — | redirects to `/auth/setup` when unclaimed, else to the owner's profile | n/a | |
| 68 | | `GET /{handle}` | none | — | public profile and the repositories that caller may see | yes | |
| 69 | | `GET /{handle}/repos` | none | — | same, paged | yes | |
| 70 | | `GET /{handle}/repos/{name}` | none | — | public repositories only; private and absent both 404 | yes | |
| 71 | | `GET …/tree/{rev}`, `…/tree/{rev}/-/{*path}` | none | — | as above | yes | |
| 72 | | `GET …/raw/{rev}/-/{*path}` | none | — | as above; `application/octet-stream` + `attachment` + `default-src 'none'; sandbox`, capped at `MAX_RAW_BYTES` (10 MiB) | yes | |
| 73 | | `GET …/log`, `…/log/{rev}`, `…/log/{rev}/-/{*path}` | none | — | as above | yes | |
| 74 | | `GET …/blame/{rev}/-/{*path}` | none | — | as above; one `git blame` under the 20s cap | yes | |
| 75 | | `GET …/commits/{sha}`, `…/compare`, `…/compare/{spec}` | none | — | as above; diffs read through a byte cap | yes | |
| 76 | | `GET …/branches`, `…/tags` | none | — | as above | yes | |
| 77 | | `GET …/search` | none | **none** — deferred | as above; grep now read through a 4 MiB cap | yes | |
| 78 | | `GET …/issues`, `…/issues/{number}`, `…/issues/new` | none | — | as above; `new` needs ownership and 404s otherwise | yes | |
| 79 | | `GET …/pulls`, `…/pulls/{number}`, `…/{number}/commits`, `…/{number}/files`, `…/pulls/new` | none | — | as above; the merge preview is cached per branch pair | yes | |
| 80 | | `GET …/labels` | none | — | as above | yes | |
| 81 | | `GET …/settings`, `…/settings/hooks`, `…/settings/hooks/{hook}` | owner | — | 404 | yes | |
| 82 | | `GET /{handle}/settings`, `…/settings/tokens` | owner | — | 404 | yes | |
| 83 | | `GET /auth/login`, `/auth/setup`, `/auth/forgot` | none | — | the form; `/auth/setup` redirects away once claimed | n/a | |
| 84 | | `GET /auth/reset/{token}` | the token | reset | the form, or the dead-link page — unknown, expired and spent are one answer | yes | |
| 85 | |
| 86 | ### Pages, write — every one CSRF-checked before its use case |
| 87 | |
| 88 | |
| 89 | |
| 90 | | `POST /{handle}/repos/new`, `…/repos/import` | owner | 403 (CSRF runs first, and consults nothing) | yes | |
| 91 | | `POST …/{name}/import/retry` | owner | 403 | yes | |
| 92 | | `POST …/issues`, `…/issues/{n}/comments`, `…/state`, `…/edit`, `…/labels` | owner | 403 | yes | |
| 93 | | `POST …/pulls`, `…/pulls/{n}/comments`, `…/state`, `…/edit`, `…/labels`, `…/merge` | owner | 403 | yes | |
| 94 | | `POST …/labels`, `…/labels/{id}/edit`, `…/labels/{id}/delete` | owner | 403 | yes | |
| 95 | | `POST …/settings`, `…/settings/archive`, `…/default-branch`, `…/rename`, `…/delete` | owner | 403 | yes | |
| 96 | | `POST …/settings/hooks`, `…/hooks/{h}/delete`, `…/toggle`, `…/deliveries/{d}/redeliver` | owner | 403 | yes | |
| 97 | | `POST /{handle}/settings` | owner | 403 | yes | |
| 98 | | `POST /{handle}/settings/tokens`, `…/tokens/{token}/revoke` | owner | 403 | yes | |
| 99 | |
| 100 | ### Routes |
| 101 | |
| 102 | |
| 103 | |
| 104 | | `GET /healthz` | none | — | `ok` or `unavailable`, and nothing else | n/a | |
| 105 | | `GET /robots.txt` | none | — | a fixed document | n/a | |
| 106 | | `GET /metrics` | operator bearer, constant-time | metrics, **only on failure** | 404 — the surface does not exist without a token | yes | |
| 107 | | `GET /api/instance` | operator bearer, optional | instance, **only unauthenticated** | `{"claimed":…}` and nothing else | yes | |
| 108 | | `POST /api/setup` | setup token | login | one 404, byte-identical for every refusal, before Argon2 | yes | |
| 109 | | `GET /api/me` | session | — | 401 | n/a | |
| 110 | | `GET /api/users/{handle}` | none | — | the public profile; 404 for an unknown handle | yes | |
| 111 | | `GET /api/users/{handle}/repos` | none | — | only visible repositories; `[]` rather than 404 for a handle that owns nothing visible | yes | |
| 112 | | `GET /{handle}/feed.atom`, `…/commits.atom`, `…/issues.atom` | none | — | only what the caller may see | yes | |
| 113 | | `GET …/archive/{file}` | none | archive, **counted before the 404** | public only; `ETag` is the resolved commit | yes | |
| 114 | | `GET /{handle}/settings/backup.tar.gz` | instance owner **and** profile owner | backup, **after** the ownership check | 404 | yes | |
| 115 | | `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 | |
| 116 | | `POST …/git-upload-pack`, `…/git-receive-pack` | as above | as above | 401 challenge; archived is 403 with a sentence, importing is 409 | yes | |
| 117 | | `POST /auth/login`, `/auth/setup` | none | login, **before Argon2** | 303 back to the form, saying nothing | yes | |
| 118 | | `POST /auth/forgot` | none | login | the same 303 whatever the address | yes | |
| 119 | | `POST /auth/reset/{token}` | the token | reset | 303; dead and unknown are one answer | yes | |
| 120 | | `POST /auth/logout` | session | — | 403 without a CSRF token | yes | |
| 121 | | `POST /{handle}/settings/password` | owner | — | 403 without a CSRF token | yes | |
| 122 | |
| 123 | ### Commands |
| 124 | |
| 125 | |
| 126 | |
| 127 | | `steid` | — | serves; prints the setup token once, only while unclaimed, only when it generated it | |
| 128 | | `steid backup <destination>` | the filesystem | a full copy of the instance. Does not migrate, deliberately | |
| 129 | | `steid restore <archive>` | the filesystem | refuses anything but an empty instance, before a byte is written | |
| 130 | | `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 | |
| 131 | |
| 132 | ## Findings |
| 133 | |
| 134 | ### 1 · High · No limit on a request body, so any POST is a memory amplifier |
| 135 | |
| 136 | `Form<T>` and `Json<T>` both collect with `to_bytes(body, usize::MAX)` — read in the |
| 137 | vendored `topcoat-router-0.5.0/src/content/{form,json}.rs`, and there is no builder knob |
| 138 | for it. So every form endpoint held whatever arrived, `/auth/login` and `/api/setup` |
| 139 | included, both anonymous and both reachable before any authentication. |
| 140 | |
| 141 | Measured against :3135: |
| 142 | |
| 143 | ``` |
| 144 | $ ps -o rss= -p $(pgrep -f target/debug/steid) # 20080 KB |
| 145 | $ curl -X POST --data-binary @<(head -c 200000000 /dev/zero | tr '\0' a) \ |
| 146 | -H 'Content-Type: application/x-www-form-urlencoded' \ |
| 147 | http://127.0.0.1:3135/auth/login # 400 |
| 148 | $ ps -o rss= -p $(pgrep -f target/debug/steid) # 811616 KB |
| 149 | ``` |
| 150 | |
| 151 | One request, 20 MB → 811 MB. A handful in parallel is the machine, and the reply is a |
| 152 | 400 either way, so nothing about the endpoint even discourages it. |
| 153 | |
| 154 | **Fixed** in `842e3f0`: `web::body_limit`, applied in the one layer at `/`. A mebibyte, |
| 155 | which is three orders of magnitude above the largest real form. A declared |
| 156 | `Content-Length` over the cap is refused with 413 before a byte is read; an undeclared |
| 157 | one fails while it arrives and surfaces as 400. The two git RPC paths are exempt, |
| 158 | matched on the whole four-segment route rather than the last segment — `RepoName` |
| 159 | reserves neither `git-upload-pack` nor `git-receive-pack`, so a repository may be called |
| 160 | that, and the test in the module is what caught it. |
| 161 | |
| 162 | Re-measured: 413 and 400 respectively, resident memory flat at 21 MB, and a 4 MiB |
| 163 | `git push` plus the clone back both succeed. |
| 164 | |
| 165 | ### 2 · Medium · A reset token from a URL could panic the worker answering it |
| 166 | |
| 167 | `POST /auth/reset/{token}` interpolated the path parameter into `see_other`, and a path |
| 168 | parameter arrives **percent-decoded** — so `/auth/reset/a%0d%0ab` produced a `Location` |
| 169 | carrying a real CRLF. `see_other` refuses that by **panicking**, not by returning an |
| 170 | error. |
| 171 | |
| 172 | ``` |
| 173 | $ curl -o /dev/null -w '%{http_code}\n' -X POST \ |
| 174 | -d 'password=aaaaaaaaaaaa&confirmation=aaaaaaaaaaaa' \ |
| 175 | 'http://127.0.0.1:3135/auth/reset/a%0d%0ab' |
| 176 | 000 |
| 177 | # server log: |
| 178 | thread 'tokio-rt-worker' panicked at topcoat-router-0.5.0/src/error/redirect.rs:140:50: |
| 179 | redirect uri is not a valid header value: InvalidHeaderValue |
| 180 | ``` |
| 181 | |
| 182 | `000` is curl's "no response at all". The process survives — the unwind is per task — |
| 183 | but an anonymous caller can drop a connection and write a panic to the log at will, and |
| 184 | this is the shape header injection would have taken had `HeaderValue` been laxer. |
| 185 | |
| 186 | **Fixed** in `2c5f03a`: the token is percent-encoded on its way into the `Location`. |
| 187 | Encoding rather than a shape check, because a real token is 64 hex characters and comes |
| 188 | back from `encode` byte for byte — so the legitimate path is unchanged and there is no |
| 189 | second spelling of `ResetSecret`'s shape to drift out of step with the domain. Two tests |
| 190 | pin it. Every other `see_other`/`redirect` call site in the web layer was checked: the |
| 191 | rest take a literal or a validated handle. |
| 192 | |
| 193 | ### 3 · Medium · Webhook delivery followed redirects, walking past the SSRF check |
| 194 | |
| 195 | `check_resolves_publicly` resolves the saved URL's host and refuses unless **every** |
| 196 | address is public. `ureq`'s default `max_redirects` is **10** |
| 197 | (`ureq-3.3.0/src/config.rs:874`), and nothing re-checked the target of a redirect. So a |
| 198 | receiver on a perfectly public host answering `302 Location: |
| 199 | http://169.254.169.254/latest/meta-data/` sent the delivery there past both halves of |
| 200 | the rule — and worse than a blind request, because the response body is stored on the |
| 201 | delivery row and rendered on the deliveries page, so it reads back. |
| 202 | |
| 203 | Only the instance owner can add a hook today, so this is self-inflicted for now. It is |
| 204 | exactly the assumption Milestone 7 removes, and the whole reason `WebhookUrl` is as |
| 205 | narrow as it is. |
| 206 | |
| 207 | **Fixed** in `6b69831`: `max_redirects(0)`. The 3xx is then recorded as the delivery's |
| 208 | own answer, which is the honest thing to show a receiver that redirects; nothing that |
| 209 | legitimately receives a webhook needs one. A test reads the setting back off the agent. |
| 210 | |
| 211 | ### 4 · Medium · The import clone followed redirects too |
| 212 | |
| 213 | The same hole, one layer over. `resolve_public_host` checks every address the source's |
| 214 | host answers with, and then `git clone` runs with `http.followRedirects` at its default |
| 215 | of `initial` — which permits the *first* request to be redirected and re-resolves |
| 216 | whatever host it is sent to. |
| 217 | |
| 218 | **Fixed** in `4be05bf`: `-c http.followRedirects=false` on the clone, verified accepted |
| 219 | by the installed git. The cost is real and is the reason this one is a trade rather than |
| 220 | a free fix: a source whose URL only works through a redirect — a repository renamed on |
| 221 | its host — now fails the import instead of silently following. The failure names the |
| 222 | URL and the real one can be pasted in its place, which is better than a check that reads |
| 223 | as a guarantee and is not one. |
| 224 | |
| 225 | ### 5 · Medium · A repository search collected everything git matched |
| 226 | |
| 227 | `grep` was the last read on an anonymous page still using the uncapped `run`. The search |
| 228 | page takes its query from a URL, has no rate limit of its own, and keeps at most |
| 229 | `SEARCH_MAX_MATCHES` (1000) of them — but `?q=e` had git write every matching line in |
| 230 | the repository into memory first, and only then discarded all but those. |
| 231 | |
| 232 | ``` |
| 233 | $ git grep -I -n -z -F --column --no-color -e e HEAD -- | wc -c |
| 234 | 5305298 |
| 235 | ``` |
| 236 | |
| 237 | 5.3 MB from one letter over Steid's own source, unauthenticated, per request, for any |
| 238 | public repository. A kernel-sized tree is orders of magnitude worse, and the 20-second |
| 239 | timeout is a long time to spend filling a `Vec`. |
| 240 | |
| 241 | **Fixed** in `56dc6cd`: `run_capped_allowing`, a 4 MiB ceiling, and the exception |
| 242 | `run_allowing` carried — grep's exit 1 for "matched nothing" — carried over with it. A |
| 243 | truncated tail costs a result nobody was going to be shown, and a partial final record |
| 244 | simply fails to parse. Both halves are pinned by a test. |
| 245 | |
| 246 | ### 6 · Low · The operator bearer token was the one secret with a plain `Debug` |
| 247 | |
| 248 | `AppConfig` derives `Debug` and lives in the app context, which is why `setup_token` and |
| 249 | `smtp_url` are `Secret`. `operator_token` was a bare `String` — and it is the worst of |
| 250 | the three to print, because unlike the setup token it is long-lived and stays in context |
| 251 | for the life of the process. Nothing renders the config today; this is the class the |
| 252 | type exists to prevent. |
| 253 | |
| 254 | **Fixed** in `6eba149`, with the test the other two already had. |
| 255 | |
| 256 | ### 7 · Low · No `Cache-Control` on anything |
| 257 | |
| 258 | Every URL under `/{handle}/…` is the same shape whether it renders something public or |
| 259 | private, so an intermediary had nothing to tell them apart by, and a browser was free to |
| 260 | heuristically cache a private repository's page. |
| 261 | |
| 262 | ``` |
| 263 | $ curl -D - -o /dev/null http://127.0.0.1:3135/ada/repos/secret | grep -i cache |
| 264 | (nothing) |
| 265 | ``` |
| 266 | |
| 267 | **Fixed** in `c410dc3`: `private, no-cache` as a default in the same layer, via |
| 268 | `or_insert`, so the three responses that already decided keep their answer — the asset |
| 269 | routes' year-long `public, immutable`, git's `must-revalidate` on an advertisement, and |
| 270 | the backup's `no-store`. `no-cache` rather than `no-store` for the archive's sake: a |
| 271 | response the browser was told not to store has nothing left to answer `If-None-Match` |
| 272 | with. Checked in the running app — all four are as described and the archive's 304 still |
| 273 | happens. |
| 274 | |
| 275 | ## Deferred |
| 276 | |
| 277 | Carried into `current.md`. |
| 278 | |
| 279 | - **The search page has no rate limit.** The byte cap bounds the memory; the *CPU* is |
| 280 | still a full `git grep` per request on an anonymous page, and it is the most expensive |
| 281 | unauthenticated read after the archive. It wants a `SearchLimiter` alongside |
| 282 | `ArchiveLimiter` — mechanical, but a limiter is a policy decision (how many, over what |
| 283 | window, and whether an empty query counts) and those belong to the user. |
| 284 | - **CSRF on the signed-out forms.** `/auth/login`, `/auth/setup`, `/auth/forgot` and |
| 285 | `/auth/reset/{token}` are exempt with a written reason: there is no session to bind a |
| 286 | double-submit token to. Login CSRF is real and needs a pre-session cookie. Already |
| 287 | noted by 0008; restated here because a review that did not mention it would read as |
| 288 | having missed it. |
| 289 | - **DNS rebinding on both outbound paths.** Both re-resolve at connect time, so a record |
| 290 | whose TTL expires between the check and the dial still gets through. Closing it means |
| 291 | a custom resolver, or pinning the address and setting `Host`/SNI by hand — for `ureq` |
| 292 | and for `git` separately. Recorded in both modules already. |
| 293 | - **`run` still collects without a byte cap** for the reads that are not grep or diff — |
| 294 | `log`, `for-each-ref`, `cat-file`. Each is bounded by `--max-count`, by the ref count, |
| 295 | or by an object size checked from its header first, so none is reachable in the shape |
| 296 | grep was. Worth a sweep if a page ever asks git something open-ended again. |
| 297 | |
| 298 | ## Verification |
| 299 | |
| 300 | - `cargo test` — 1309 pass, 0 fail. |
| 301 | - `cargo clippy --all-targets` — zero warnings. |
| 302 | - `cargo fmt` — clean. |
| 303 | - `cargo audit` — zero vulnerabilities, the three standing warnings above. |
| 304 | - Against a real instance on :3135: claimed headlessly, signed in, a public and a private |
| 305 | repository created, a 4 MiB push and clone over HTTP Basic with a personal access |
| 306 | token, and every probe quoted above. |