Remediate security scan findings SEC-01..SEC-12 #28

graham · 2026-09-23T15:57:13Z

graham commented

2026-09-23 15:57:13 +00:00

Fixes every finding in tmp/security.md (2026-09-23 scan: 2 high, 5 medium, 5 low).

High

SEC-01 — TOTP seeds encrypted at rest: new Microwaveprop.FieldCrypto (AES-256-GCM, versioned <<1, iv, tag, ct>> blobs, key = HMAC-SHA256(secret_key_base, purpose string), fail-closed decrypt with legacy-plaintext passthrough for rolling deploys). Migration encrypts existing seeds.
SEC-02 — Beacon-monitor tokens encrypted at rest with a SHA-256 token_digest for the unique-indexed lookup. Plaintext stays retrievable on the monitor page (deliberate product decision from PR #4) via a virtual token_plaintext; new monitors get a 365-day expiry renewed on regeneration; every display/rotation is audit-logged; redact: true.

Medium

SEC-03 — /submit enforces a server-side daily quota (10/day anonymous per IP, 60/day per account) on top of the per-LiveView cooldown.
SEC-04 — Invalid bearer tokens hit a per-IP budget before the DB lookup (peek-only, so valid tokens never drain it).
SEC-05 — Registration returns a byte-identical flash for new vs. duplicate accounts.
SEC-06 — RateLimiter counters move to Valkey (INCR + EXPIRE NX) when VALKEY_URL is set — cluster-wide, restart-surviving; ETS fallback on error/unconfigured.
SEC-07 — All container images pinned by digest: Dockerfile, Dockerfile.base, Dockerfile.ci, rust/prop_grid_rs/Dockerfile, all four k8s deployments, CI postgres service. (Deployment manifests pin :latest digests — the original build tags were GCed by the registry.)

Low

SEC-08 — Account-confirm and email-change tokens consumed by POST; GET renders a CSRF-protected confirm page so mail scanners cannot burn single-use links.
SEC-09 — CSP img-src/connect-src narrowed to the actual tile/radar/socket hosts.
SEC-10 — Tile/score responses marked private; session writes skipped when values are unchanged so Set-Cookie is not re-emitted on cacheable responses.
SEC-11 — Rust /metrics bearer compare is constant-time (subtle::ct_eq).
SEC-12 — readOnlyRootFilesystem: true on all pods + /tmp emptyDir; RELEASE_TMP=/tmp; backtest-reports emptyDir for the admin report path.

Also fixed

ContactLive.Show internal-network gate was dead code — it read session[:remote_ip] with an atom key, but Plug stringifies session keys.
A committed unresolved merge-conflict block in CHANGELOG.md (was rendering on /changelog).

Carries the focused fixes from fix/audit-remainder (invalid-token budget, registration parity, POST token consumption, CSP allowlist, private cache policy, monitor-token redaction, metrics fail-closed, get_user!→get_user rename) without that branch's unrelated changes.

Deploy notes

Two migrations run on boot: encrypt_totp_secrets (in-place) and encrypt_beacon_monitor_tokens (adds token_digest, encrypts token in place). Both are reversible given the same secret_key_base. During a rolling deploy, old pods reading a migrated row will fail TOTP/monitor auth for that row — the window is seconds on this cluster.
Digest pins mean base-image bumps are now deliberate: rebuild prop-base/elixir-base, then update the digest ARGs.

Verification

make precommit green (5038 tests, credo --strict, xref); cargo clippy --all-targets -- -D warnings + cargo test --release green (263 tests).

Fixes every finding in `tmp/security.md` (2026-09-23 scan: 2 high, 5 medium, 5 low). ## High - **SEC-01** — TOTP seeds encrypted at rest: new `Microwaveprop.FieldCrypto` (AES-256-GCM, versioned `<<1, iv, tag, ct>>` blobs, key = HMAC-SHA256(secret_key_base, purpose string), fail-closed decrypt with legacy-plaintext passthrough for rolling deploys). Migration encrypts existing seeds. - **SEC-02** — Beacon-monitor tokens encrypted at rest with a SHA-256 `token_digest` for the unique-indexed lookup. Plaintext stays retrievable on the monitor page (deliberate product decision from PR #4) via a virtual `token_plaintext`; new monitors get a 365-day expiry renewed on regeneration; every display/rotation is audit-logged; `redact: true`. ## Medium - **SEC-03** — `/submit` enforces a server-side daily quota (10/day anonymous per IP, 60/day per account) on top of the per-LiveView cooldown. - **SEC-04** — Invalid bearer tokens hit a per-IP budget *before* the DB lookup (peek-only, so valid tokens never drain it). - **SEC-05** — Registration returns a byte-identical flash for new vs. duplicate accounts. - **SEC-06** — RateLimiter counters move to Valkey (`INCR` + `EXPIRE NX`) when `VALKEY_URL` is set — cluster-wide, restart-surviving; ETS fallback on error/unconfigured. - **SEC-07** — All container images pinned by digest: `Dockerfile`, `Dockerfile.base`, `Dockerfile.ci`, `rust/prop_grid_rs/Dockerfile`, all four k8s deployments, CI postgres service. (Deployment manifests pin `:latest` digests — the original build tags were GCed by the registry.) ## Low - **SEC-08** — Account-confirm and email-change tokens consumed by POST; GET renders a CSRF-protected confirm page so mail scanners cannot burn single-use links. - **SEC-09** — CSP `img-src`/`connect-src` narrowed to the actual tile/radar/socket hosts. - **SEC-10** — Tile/score responses marked `private`; session writes skipped when values are unchanged so `Set-Cookie` is not re-emitted on cacheable responses. - **SEC-11** — Rust `/metrics` bearer compare is constant-time (`subtle::ct_eq`). - **SEC-12** — `readOnlyRootFilesystem: true` on all pods + `/tmp` emptyDir; `RELEASE_TMP=/tmp`; `backtest-reports` emptyDir for the admin report path. ## Also fixed - `ContactLive.Show` internal-network gate was dead code — it read `session[:remote_ip]` with an atom key, but Plug stringifies session keys. - A committed unresolved merge-conflict block in `CHANGELOG.md` (was rendering on `/changelog`). Carries the focused fixes from `fix/audit-remainder` (invalid-token budget, registration parity, POST token consumption, CSP allowlist, private cache policy, monitor-token redaction, metrics fail-closed, `get_user!`→`get_user` rename) without that branch's unrelated changes. ## Deploy notes - Two migrations run on boot: `encrypt_totp_secrets` (in-place) and `encrypt_beacon_monitor_tokens` (adds `token_digest`, encrypts `token` in place). Both are reversible given the same `secret_key_base`. During a rolling deploy, old pods reading a migrated row will fail TOTP/monitor auth for that row — the window is seconds on this cluster. - Digest pins mean base-image bumps are now deliberate: rebuild `prop-base`/`elixir-base`, then update the digest ARGs. ## Verification `make precommit` green (5038 tests, credo --strict, xref); `cargo clippy --all-targets -- -D warnings` + `cargo test --release` green (263 tests).

graham added 1 commit

2026-09-23 15:57:13 +00:00

Remediate security scan findings SEC-01..SEC-12

skippy-bot/review Skippy review: 2 blocking findings open — see the PR thread

Details

54c8002a6c

High: TOTP seeds and beacon-monitor tokens encrypted at rest via new FieldCrypto (AES-256-GCM, key derived from secret_key_base); monitor tokens stay retrievable on the page per PR #4, now via token_digest lookup + virtual plaintext, 365-day default expiry renewed on regeneration, display audit logging. Medium: /submit daily quota (10/day anon IP, 60/day account); invalid bearer tokens budgeted per-IP before the DB lookup; registration flash byte-identical for duplicate accounts; RateLimiter counters on Valkey when configured (ETS fallback); all container images digest-pinned. Low: confirm/email-change tokens consumed by POST behind a confirm page; CSP img-src/connect-src narrowed to real hosts; tile responses private + session writes skipped when unchanged; Rust /metrics bearer compare constant-time; readOnlyRootFilesystem on all pods with /tmp emptyDir + RELEASE_TMP. Also fixes ContactLive.Show's dead internal-network gate (atom vs string session key) and a committed merge-conflict block in CHANGELOG.md.

skippy-bot reviewed

2026-09-23 16:03:57 +00:00

skippy-bot left a comment

🤖 Skippy PR review

2 findings — 2 blocking before merge.

Severity	Location	Issue
🟠 High	`priv/repo/migrations/20260923153615_encrypt_totp_secrets.exs:15`	Version-byte check leaves a 1-in-256 plaintext seed behind, and locks that user out of 2FA
🟡 Warning	`lib/microwaveprop_web/api/auth.ex:100`	Invalid-token budget rejects valid tokens from the same IP

_{Reviewed 54c8002a6c1d. Comment skippy review to re-run.}

### 🤖 Skippy PR review **2 findings** — 2 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟠 High | `priv/repo/migrations/20260923153615_encrypt_totp_secrets.exs:15` | Version-byte check leaves a 1-in-256 plaintext seed behind, and locks that user out of 2FA | | 🟡 Warning | `lib/microwaveprop_web/api/auth.ex:100` | Invalid-token budget rejects valid tokens from the same IP | <sub>Reviewed `54c8002a6c1d`. Comment `skippy review` to re-run.</sub>

lib/microwaveprop_web/api/auth.ex Outdated

					
				@ -71,0 +97,4 @@

				  # counts: the hit is recorded only when the token actually fails, so

				  # valid-token traffic never drains this budget.

				  defp invalid_token_budget(conn) do

				    if RateLimiter.exhausted?({:invalid_token, ip_string(conn)}, @invalid_token_ip_limit) do

🟡 Warning — Invalid-token budget rejects valid tokens from the same IP

invalid_token_budget/1 runs before Accounts.get_user_by_api_token/1 in authenticate/1, and the bucket is keyed on the client IP. So once any one client behind an address burns 30 misses inside a 60 s window, every request from that address short-circuits to 401 "unauthorized" — valid bearer tokens included — and it stays that way as long as the bad client keeps retrying, because each retry hits the bucket again (a script with a revoked token on a 1 s retry loop keeps the bucket exhausted indefinitely).

That is the exact collateral the comment above invalid_token_budget/1 says the separate bucket avoids for the anonymous {:ip, ip} bucket: a shared NAT (office, club, mobile carrier) loses its whole API access to one misbehaving client on the same egress IP.

Fix: bucket on the presented token rather than the IP (hash the bearer value) so only the repeated bad value is throttled, or keep the pre-lookup gate but answer 429 with a distinct problem title and a higher limit — the invariant to preserve is that a bucket hit must never deny a request whose token would have authenticated.

**🟡 Warning — Invalid-token budget rejects valid tokens from the same IP** `invalid_token_budget/1` runs *before* `Accounts.get_user_by_api_token/1` in `authenticate/1`, and the bucket is keyed on the client IP. So once any one client behind an address burns 30 misses inside a 60 s window, every request from that address short-circuits to 401 "unauthorized" — valid bearer tokens included — and it stays that way as long as the bad client keeps retrying, because each retry hits the bucket again (a script with a revoked token on a 1 s retry loop keeps the bucket exhausted indefinitely). That is the exact collateral the comment above `invalid_token_budget/1` says the separate bucket avoids for the anonymous `{:ip, ip}` bucket: a shared NAT (office, club, mobile carrier) loses its whole API access to one misbehaving client on the same egress IP. Fix: bucket on the presented token rather than the IP (hash the bearer value) so only the repeated bad value is throttled, or keep the pre-lookup gate but answer 429 with a distinct problem title and a higher limit — the invariant to preserve is that a bucket hit must never deny a request whose token would have authenticated.

skippy-bot marked this conversation as resolved

priv/repo/migrations/20260923153615_encrypt_totp_secrets.exs Outdated

					
				@ -0,0 +12,4 @@

				    rows = repo.query!("SELECT id, totp_secret FROM users WHERE totp_secret IS NOT NULL", [])

				    Enum.each(rows.rows, fn [id, secret] ->

				      unless match?(<<1, _::binary>>, secret) do

🟠 High — Version-byte check leaves a 1-in-256 plaintext seed behind, and locks that user out of 2FA

match?(<<1, _::binary>>, secret) uses the FieldCrypto version byte to mean "already encrypted", but a pre-migration seed is 20 random bytes (NimbleTOTP.secret/0), so roughly 1 in 256 of them start with 0x01 and are skipped: that row stays plaintext in the column, which is exactly what SEC-01 exists to remove.

The skipped row is then worse than unencrypted: the app reads it as a ciphertext blob, the AEAD tag fails, and totp_secret/1 fails closed, so that user can never pass the second factor again (only a saved recovery code gets them back in). down/0 also raises on that row ("does not decrypt under the current key"), so a rollback of this migration is blocked until someone hand-fixes it. The same ambiguity catches a plaintext row written by an old pod after the migration ran.

Fix: make "already a blob" unambiguous, e.g. version byte and a plausible length (byte_size(secret) >= 29, since the shortest blob is 1 + 12 + 16 bytes, and a seed blob is 49), or add FieldCrypto.blob?/1 and use it here and in with_token_plaintext/1.

**🟠 High — Version-byte check leaves a 1-in-256 plaintext seed behind, and locks that user out of 2FA** `match?(<<1, _::binary>>, secret)` uses the FieldCrypto version byte to mean "already encrypted", but a pre-migration seed is 20 random bytes (`NimbleTOTP.secret/0`), so roughly 1 in 256 of them start with 0x01 and are skipped: that row stays plaintext in the column, which is exactly what SEC-01 exists to remove. The skipped row is then worse than unencrypted: the app reads it as a ciphertext blob, the AEAD tag fails, and `totp_secret/1` fails closed, so that user can never pass the second factor again (only a saved recovery code gets them back in). `down/0` also `raise`s on that row ("does not decrypt under the current key"), so a rollback of this migration is blocked until someone hand-fixes it. The same ambiguity catches a plaintext row written by an old pod after the migration ran. Fix: make "already a blob" unambiguous, e.g. version byte **and** a plausible length (`byte_size(secret) >= 29`, since the shortest blob is 1 + 12 + 16 bytes, and a seed blob is 49), or add `FieldCrypto.blob?/1` and use it here and in `with_token_plaintext/1`.

skippy-bot marked this conversation as resolved

graham referenced this pull request from a commit

2026-09-23 16:25:08 +00:00

Address skippy review on PR #28

graham added 1 commit

2026-09-23 16:25:08 +00:00

Address skippy review on PR #28

skippy-bot/review Skippy review: clean — no open findings

Details

1b1a0a8d34

- encrypt_totp_secrets: decide plaintext-vs-encrypted by attempting
  FieldCrypto.decrypt, not by sniffing the 0x01 version byte. A seed
  whose first byte is 0x01 was previously skipped, left readable at
  rest, and then failed authentication on every read — locking that
  user out of 2FA. Passthrough (decrypt returns the input) means
  plaintext -> encrypt; a blob that authenticates is skipped; a
  versioned value that fails auth is re-encrypted whole (a 0x01-prefixed
  plaintext seed survives intact; ciphertext under a retired key was
  already unrecoverable).

- Api.Auth: two-tier per-IP budget. Past the 30/min invalid-token miss
  bucket the token lookup still runs — a shared-NAT address no longer
  starts rejecting valid tokens — but every lookup then counts against
  a 300/min lookup ceiling, so garbage spray stays bounded.

skippy-bot commented

2026-09-23 16:26:20 +00:00

First-time contributor

Resolved 2 of 2 earlier findings.

Migration version-byte ambiguity (priv/repo/migrations/20260923153615_encrypt_totp_secrets.exs): each row is now classified by attempting FieldCrypto.decrypt/1 instead of sniffing the version byte, so a plaintext seed can no longer be skipped, left readable at rest, and then unreadable by totp_secret/1; the unauthenticatable-blob branch is logged and re-encrypted. down/0 no longer hits a row it cannot handle.
Invalid-token budget (lib/microwaveprop_web/api/auth.ex): the miss bucket no longer short-circuits ahead of the lookup. Once it is spent, lookups are bounded by a separate 300/min per-IP ceiling ({:token_lookup, ip}), so a revoked-token retry loop can no longer deny every valid token sharing its egress IP. New coverage for both paths in test/microwaveprop_web/api/auth_test.exs.

Nothing new in 54c8002a6c1d..1b1a0a8d34fd (4 files: auth.ex, rate_limiter.ex, the migration, auth_test.exs).

Resolved 2 of 2 earlier findings. - **Migration version-byte ambiguity** (`priv/repo/migrations/20260923153615_encrypt_totp_secrets.exs`): each row is now classified by attempting `FieldCrypto.decrypt/1` instead of sniffing the version byte, so a plaintext seed can no longer be skipped, left readable at rest, and then unreadable by `totp_secret/1`; the unauthenticatable-blob branch is logged and re-encrypted. `down/0` no longer hits a row it cannot handle. - **Invalid-token budget** (`lib/microwaveprop_web/api/auth.ex`): the miss bucket no longer short-circuits ahead of the lookup. Once it is spent, lookups are bounded by a separate 300/min per-IP ceiling (`{:token_lookup, ip}`), so a revoked-token retry loop can no longer deny every valid token sharing its egress IP. New coverage for both paths in `test/microwaveprop_web/api/auth_test.exs`. Nothing new in `54c8002a6c1d..1b1a0a8d34fd` (4 files: `auth.ex`, `rate_limiter.ex`, the migration, `auth_test.exs`).

graham added 1 commit

2026-09-23 16:40:37 +00:00

Merge main into fix/security-scan-2026-09-23

skippy-bot/review Skippy review: clean — no open findings

Details

d2ace3a3ac

Conflicts resolved in favor of this branch's versions: encrypted
beacon-monitor token schema, two-tier invalid-token budget, Valkey-backed
rate limiter, encrypted-TOTP test assertion. CHANGELOG keeps both
2026-09-23 entries.

graham merged commit 37d2f5d517 into main

2026-09-23 16:41:25 +00:00

graham deleted branch fix/security-scan-2026-09-23

2026-09-23 16:41:25 +00:00

graham referenced this pull request from a commit

2026-09-23 16:41:27 +00:00

Remediate security scan findings SEC-01..SEC-12 (#28)

Sign in to join this conversation.

No reviewers

No labels

No milestone

No project