steid
Sign in

@jamesgill /

steid

feat: security headers on every response

The one real finding from reviewing the app before it goes public: HTML
responses carried no security headers at all. Everything else held under
attack — private repositories are 404 on every surface including /api, six
injection attempts through the revision segment and four traversal spellings
through the path all 404 with no execution, login throttles at ten attempts,
raw .html and .svg come back as octet-stream with nosniff and a sandbox CSP, a
filename containing a quote and a semicolon is sanitised before reaching a
header, and the production cookie is __Host-session with Secure and HttpOnly.

A layer, not a per-handler concern: a page added without these would simply not
have them and nothing would fail.

Steid renders no JavaScript at all — not a script tag on any page — so
default-src 'none' is a policy this product can genuinely keep rather than
aspire to. That is stricter than a typical CSP and worth defending on purpose:
if a feature ever needs script, weakening it should be a decision rather than a
quiet edit. 'unsafe-inline' is granted for styles only, and only because
Topcoat's icon macro emits a vertical-align style attribute on every icon.

Referrer-Policy is no-referrer rather than the usual
strict-origin-when-cross-origin, because a private repository's URL contains its
name and a README may link anywhere — the referrer would hand that name to
whatever the visitor clicked.

Headers use entry().or_insert(), never insert(): the raw-file endpoint sets its
own stricter default-src 'none'; sandbox, and a blanket overwrite would quietly
relax the one response that most needs locking down. Verified it survives.

Also recorded: a near-miss where production appeared to serve an insecure
cookie. It was a testing error — dotenvy reads .env from the working directory
and the repo root has a dev one. The same trap applies in production, where the
unit sets WorkingDirectory=/opt/steid.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JZwc7URWKVhkAuRTWiDmjA
JamesPatrickGill authored 1 day agoparentaa20db5Browse files0f196544ea0591c71896899fc1289342b531f58c

4 files changed+139 −1

plans/current.md+7 −1View file
@@ -84,7 +84,9 @@ instance, and Steid's own source is pushed to it and browsable there.
8484 Harmless, same second, but two writes per push.
8585 - **A licence.** A public portfolio repository probably wants one, and the README
8686 deliberately says nothing about licensing rather than guessing.
87−- **Blocked on a domain transfer** (noted 2026-08-29). `git.jpgilldev.com` is the
87+- ~~Blocked on a domain transfer~~ — **transferred 2026-08-29**, so Phases 1–6 of the
88+ deployment runbook are unblocked.
89+- **(historic) Blocked on a domain transfer** (noted 2026-08-29). `git.jpgilldev.com` is the
8890 intended host for both the instance and the release downloads; the transfer is in
8991 flight. Phases 1–6 of the deployment runbook cannot start until DNS resolves, because
9092 Caddy requests a certificate on startup. Nothing else is blocked by it — the artifact
@@ -100,6 +102,10 @@ instance, and Steid's own source is pushed to it and browsable there.
100102 - **`REMOTE_USER` is not set on the backend**, so a push is recorded in the repository's
101103 reflog without naming who made it. Steid knows the actor by then; it simply is not
102104 passed through. Small, and worth doing before anything reads reflogs.
105+- **No automated test asserts the security headers.** There is no HTTP-level test
106+ harness, so losing the CSP would be silent. The likeliest regression is someone using
107+ `insert()` instead of `or_insert()` and flattening the raw endpoint's stricter policy.
108+- **`cargo audit` has never been run**, and the dependency tree has not been reviewed.
103109 - **No rate limiting on token authentication.** A token is 256 bits so guessing is not
104110 the worry; unbounded hashing on an open endpoint is.
105111 - **Tokens have no expiry and no last-used timestamp.** Both deliberate omissions for
plans/progress.md+59 −0View file
@@ -559,6 +559,65 @@ boxes, and a `/{handle}/repos` index for the tab to point at.
559559 falls back to browser defaults — it reads as a design mistake. Cost one confused
560560 screenshot.
561561
562+### Security review before going public · done
563+
564+Attempted with three blind reviewer agents; all three died to the machine sleeping, so
565+the review was done directly instead. **Findings were verified by attacking a running
566+instance, not by reading comments** — the codebase argues confidently for its own
567+safety and those arguments are exactly what needed testing.
568+
569+#### What held, demonstrated rather than assumed
570+
571+| Probe | Result |
572+|---|---|
573+| Private repo: page, tree, log, settings, raw, `/api` | 404 on every surface; `/api` does not leak the name |
574+| `rev=--upload-pack=touch /tmp/pwned` and five other injections | 404, no execution |
575+| Four path-traversal spellings incl. `%2e%2e` and `....//` | 404 |
576+| Login brute force | 10 attempts then 429 |
577+| Error bodies | bare "not found", nothing internal |
578+| Raw `.html` and `.svg` from a repo | `octet-stream` + `nosniff` + `attachment` + sandbox CSP |
579+| Filename `ev"il; drop.txt` in a header | sanitised to `ev_il__drop.txt`, no header injection |
580+| `<script>` in a viewed file | escaped, zero live tags |
581+
582+**The production cookie is `__Host-session; HttpOnly; Secure; SameSite=Lax; Path=/`** —
583+the strictest cookie form available, and it forbids a `Domain` attribute so a
584+compromised subdomain cannot inject one.
585+
586+**A near-miss worth recording:** the first check appeared to show production serving
587+`steid-dev-session` with no `Secure`. It was a testing error — **`dotenvy` reads `.env`
588+from the working directory**, and running the binary from the repo root picked up the
589+gitignored dev `.env`. Verify from a neutral directory, and note the same trap applies
590+to the deployment: the systemd unit's `WorkingDirectory=/opt/steid` means a stray `.env`
591+there would silently override the environment file.
592+
593+#### The one real finding: no security headers on HTML
594+
595+Fixed with a layer, deliberately a layer rather than a per-handler concern — a page
596+added without them would simply not have them and nothing would fail.
597+
598+- **Steid renders no JavaScript at all**, not a script tag on any page, so
599+ `default-src 'none'` is a policy the product can genuinely keep. That is far stricter
600+ than a typical CSP and worth defending: if a feature ever needs script, weakening it
601+ should be a deliberate decision.
602+- `'unsafe-inline'` is granted for **styles only**, and only because Topcoat's icon
603+ macro emits `style="vertical-align: -0.125em"`. A framework constraint, not a choice.
604+- **`Referrer-Policy: no-referrer`**, not the usual `strict-origin-when-cross-origin`: a
605+ private repository's URL contains its name, and a README may link anywhere, so a
606+ referrer would hand that name to whatever the visitor clicked.
607+- Headers are set with `entry().or_insert()`, never `insert()` — the raw endpoint's own
608+ stricter `default-src 'none'; sandbox` must not be relaxed by a blanket overwrite.
609+ Verified that it survives.
610+
611+#### Not covered, and why
612+
613+- **The member-but-not-owner path could not be demonstrated.** There is no registration,
614+ so a second account cannot exist until Milestone 7. It is covered by use-case tests and
615+ nothing else — re-verify for real the moment a second account is possible.
616+- **No automated test asserts the headers.** There is no HTTP-level test harness in this
617+ project, so a regression here would be silent. That is a gap, not a decision.
618+- No dependency audit (`cargo audit` was not run), and no review of the deployment
619+ scripts beyond the earlier container dry-run.
620+
562621 ---
563622
564623 ## Reference: what attempt #2 proved
src/infrastructure/web/mod.rs+1 −0View file
@@ -12,6 +12,7 @@ pub mod profile;
1212 pub mod rate_limit;
1313 pub mod repo;
1414 pub mod repo_settings;
15+pub mod security_headers;
1516 pub mod session_cookie;
1617 pub mod settings;
1718 pub mod setup;
src/infrastructure/web/security_headers.rs+72 −0View file
@@ -0,0 +1,72 @@
1+//! Security headers on every response.
2+//!
3+//! A layer rather than a per-handler concern: a page added without these would simply
4+//! not have them, and nothing would fail. The same reasoning `context.rs` gives for
5+//! preferring functions over middleware runs the other way here — this is a blanket
6+//! property of every response, and the failure mode of forgetting it is silent.
7+
8+use topcoat::{
9+ Result,
10+ context::CxBuilder,
11+ router::{
12+ Body, HeaderValue, Next, Response,
13+ header::{
14+ CONTENT_SECURITY_POLICY, REFERRER_POLICY, X_CONTENT_TYPE_OPTIONS, X_FRAME_OPTIONS,
15+ },
16+ layer,
17+ },
18+};
19+
20+/// What the pages are actually allowed to do.
21+///
22+/// Steid renders **no JavaScript at all** — not a script tag on any page — so
23+/// `default-src 'none'` is a policy the product can genuinely keep rather than a wish.
24+/// That makes this far stricter than a typical site's CSP, and it is worth preserving:
25+/// if a future feature needs script, weakening this is the cost, and it should be a
26+/// deliberate decision rather than a quiet edit.
27+///
28+/// `'unsafe-inline'` for styles only, and only because Topcoat's icon macro emits
29+/// `style="vertical-align: -0.125em"` on every icon. Inline *style* without inline
30+/// *script* is a much smaller surface, but it is a framework constraint rather than a
31+/// choice — a nonce or a hash would be better if Topcoat ever offers one.
32+const POLICY: &str = "default-src 'none'; \
33+ style-src 'self' 'unsafe-inline'; \
34+ img-src 'self' data:; \
35+ font-src 'self'; \
36+ form-action 'self'; \
37+ frame-ancestors 'none'; \
38+ base-uri 'none'";
39+
40+#[layer("/")]
41+async fn security_headers(cx: &mut CxBuilder, body: Body, next: Next<'_>) -> Result<Response> {
42+ let mut response = next.run(cx, body).await?;
43+ let headers = response.headers_mut();
44+
45+ // `entry().or_insert()`, never `insert()`: the raw-file endpoint sets its own,
46+ // stricter `default-src 'none'; sandbox`, and a blanket overwrite here would
47+ // quietly relax the one response that most needs locking down.
48+ headers
49+ .entry(CONTENT_SECURITY_POLICY)
50+ .or_insert(HeaderValue::from_static(POLICY));
51+
52+ headers
53+ .entry(X_CONTENT_TYPE_OPTIONS)
54+ .or_insert(HeaderValue::from_static("nosniff"));
55+
56+ // `no-referrer`, not the usual `strict-origin-when-cross-origin`. A private
57+ // repository's URL contains its name, and a README may link anywhere; sending the
58+ // referrer would hand the name of a private repository to whatever the visitor
59+ // clicked. Nothing here needs a referrer for its own sake.
60+ headers
61+ .entry(REFERRER_POLICY)
62+ .or_insert(HeaderValue::from_static("no-referrer"));
63+
64+ // Redundant with `frame-ancestors` for anything current, kept for browsers that
65+ // predate CSP. Cheap, and clickjacking a one-click control like "revoke token"
66+ // is the realistic attack it stops.
67+ headers
68+ .entry(X_FRAME_OPTIONS)
69+ .or_insert(HeaderValue::from_static("DENY"));
70+
71+ Ok(response)
72+}