Files
session-history/ohm/0026/SESSION-0026.1-TRANSCRIPT-2026-05-28T13-46--2026-05-28T13-50.md
T
Ben Stull 9e6756f678 ohm: migrate orphaned subagent transcripts (0019.x, 0026.x) + renumber dup 0046.0 -> 0073
Backfills transcripts that existed only locally in ohm-infra:
- 0019.1/.2/.3 - UX-polish wave subagent transcripts (driver 0019.0 never finalized)
- 0026.1-.9 - security-audit-0026 subagent transcripts (driver abandoned/closed-out by 0068; audit drove published 0030 remediation)
- 0073.0 - PPE/progressive-delivery + engineering-handbook session, originally drafted as a duplicate 0046.0; reassigned next free number (0072 taken by a concurrent session)

sessions.json: add 0019/0073 titles, update 0026 title.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-05 03:28:59 -07:00

201 lines
11 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# SESSION-0026.1 — AuthN/AuthZ security audit subsession (rfc-app v0.24.0)
Parent: SESSION-0026.0-TRANSCRIPT-2026-05-28T13-46--INPROGRESS.md
Date: 2026-05-28T13-46 → 2026-05-28T13-50 (America/Los_Angeles)
Role: subsession 0026.1 of OHM security-audit driver session 0026.
Mode: READ-ONLY. No edits, no deploys, no mutations. Only file written is this transcript.
## Goal
Audit the authentication / authorization surface of the rfc-app codebase
(`/Users/benstull/git/rfc-app`, tag v0.24.0): the email/OTC login flow,
passcodes, device-trust, Gitea OAuth + core auth, session/cookie security,
the admin role model, and per-endpoint authz guards across every
`api*.py` write/admin/owner endpoint. Verify deployment guarantee #4
("anonymous users are off-limits for any write/mutation"). Hunt for missing
guards, IDOR, and OTC/passcode brute-force/replay.
## Outcome
One High finding (OTC verify has no attempt limit / lockout — asymmetric
with the passcode path which does). Two Low/Info findings (session cookie
`https_only=False`; email-bounce webhook unauthenticated when env unset —
both documented dev contracts). Every enumerated write/admin/owner endpoint
enforces auth; no missing guards, no IDOR found. Admin role model is sound.
## Pre-state (what I found at the start)
- `auth.py` defines the dependency ladder: `require_user` (401 if anon) →
`require_contributor` (adds muted + permission_state='granted' gate) →
`require_admin` (role in owner/admin). Plus per-RFC predicates
(`can_contribute_to_rfc`, `can_discuss_rfc`, `can_invite_to_rfc`,
`is_rfc_owner`). `current_user` re-reads role + permission_state from DB
every request (so grants/revokes take effect next request — good).
- Credentials: OTC 6-digit codes (bcrypt at rest, 10-min TTL, 60s request
cooldown, single live code per email); passcodes (bcrypt, 5-fail/15-min
per-account lockout, denylist, 420 chars); device-trust (256-bit CSPRNG,
bcrypt, 30-day, HttpOnly+Secure+SameSite=Lax cookie); invite tokens
(256-bit CSPRNG, bcrypt, 7-day, single-use).
## Turn-by-turn arc
1. Read auth.py + config.py. Dependency ladder is clean. Noted SECRET_KEY
is `_required` (good — no insecure default). OAuth provisioning sets
owner role only via OWNER_GITEA_LOGIN match; OTC/invite users get
gitea_login="" / gitea_id=0 coalesced.
2. Read otc.py + passcode.py side by side. **Caught the asymmetry**:
`passcode.verify_passcode` has a per-account failed-attempts counter +
15-min lockout; `otc.verify_code` has NOTHING — it walks the last 5
rows, bcrypt-checks, and returns "wrong" with no counter, no lockout,
no per-IP throttle. The 60s cooldown lives in `request_code`, not
`verify_code`. This is the headline finding.
3. Read device_trust.py + email_otc.py. Device-trust is solid (constant-time
bcrypt walk, no early-exit, token never logged, cookie flags correct,
user-scoped revoke). email_otc always returns 202 / logs failures —
no email enumeration via the request path.
4. Read main.py (middleware + all /auth endpoints). Confirmed:
- SessionMiddleware uses `https_only=False` (line 150) — itsdangerous-
signed cookie, HttpOnly by default, but Secure flag off. Dev parity
comment present. Behind HTTPS+proxy in prod this is low-impact but
worth flagging.
- `/auth/otc/verify` has NO rate-limit dependency, confirming finding #1
at the endpoint layer too.
- Turnstile gates `/auth/otc/request` ONLY, not verify.
5. Refutation pass on finding #1: Is there a global limiter? Grepped for
RateLimit/slowapi/limiter/429 across backend/app — only tag_suggest.py
(per-user, in-memory) and the OTC *request* cooldown 429. No global
middleware, no limiter on verify. SPEC §ahead-of (line ~4100) discusses
per-IP throttle for the *passcode* verify (which already has a per-
account lockout) but never notes the OTC verify has no per-account limit
at all. Quantified: single live code per email, bcrypt ~tens of ms,
10^6 keyspace, 10-min TTL. Full sweep not possible in one TTL, but
attacker re-requests (60s cooldown) to keep a live code and runs
parallel verifies; expected ~500k guesses. bcrypt cost + TTL + single-
live-code = real friction but NO attempt cap. Downgraded Critical→High.
6. Read api_admin.py end to end. Every endpoint calls require_admin; the
user-search typeahead intentionally uses require_user (no privileged
data). Role escalation well-guarded: only owner grants/changes owner;
self-downgrade, self-mute, self-permission-flip, self-invite all refused;
admins/owners not write-mutable. No spoof/escalation path. SOLID.
7. Enumerated routes+guards across api.py, api_prs.py, api_branches.py,
api_graduation.py, api_invitations.py, api_notifications.py,
api_discussion.py via grep. Every write/mutation route has require_user
/ require_contributor / require_admin, and the per-RFC write routes
additionally call can_contribute_to_rfc / can_discuss_rfc /
can_invite_to_rfc. GET read routes use current_user (nullable) — anon
read is by design (guarantee #4 is about writes).
8. IDOR hunt — checked every owner/id-comparing helper:
- api_prs `_can_merge`/`_can_withdraw`/`_can_edit_pr_text`: compare
viewer.gitea_login against RFC frontmatter owners/arbiters cache and
pr_row["opened_by"] — server-side data, not user-supplied. Safe.
- api_branches `_require_branch_owner`/`_can_contribute`/
`_can_read_branch`: branch grants keyed on grantee_user_id (session
id), creator compared to gitea_login. Safe.
- notification mute (`/api/users/{user_id}/notification-mute`): SQL
scoped to viewer.user_id; only the {user_id} *target* is user-supplied
and it's just the mute subject, not an ownership pivot. Self-mute
refused. Safe.
- invitations accept: requires auth + case-insensitive email match +
refuses empty email; collaborator row keyed on viewer.user_id. Safe.
- Considered: OTC/invite users have gitea_login="". Could "" match an
owners list? Only if frontmatter literally contained "" — impossible
from gitea_login frontmatter. `"" in []` is False. Killed as non-issue.
9. Checked anonymous-reachable mutation surfaces:
- `/api/email/unsubscribe` (GET+POST): itsdangerous URLSafeSerializer
signed token (SECRET_KEY, salted) → user_id+category not forgeable;
idempotent, low-stakes; no expiry but URLSafeSerializer is fine here.
- `/api/webhooks/email-bounce`: optional X-Webhook-Secret (constant-time
compare) when WEBHOOK_EMAIL_BOUNCE_SECRET set; UNAUTHENTICATED when
unset (documented v1 dev contract). Impact: flip email opt-out flags +
mark outbound rows bounced. Low.
- `/api/webhooks/...` Gitea receiver: mandatory HMAC-SHA256, constant-
time compare, refuses to start without secret unless
RFC_APP_INSECURE_WEBHOOKS=1. SOLID.
- `/api/invites/claim`, `/auth/device-trust/start`: anon by design (they
establish the session); both gated by 256-bit bcrypt-hashed tokens.
10. Confirmed db is a single shared sqlite connection (check_same_thread=
False, isolation_level=None) — no incidental concurrency limiter that
would mitigate the OTC brute-force.
## Cut state (final findings)
### High
- **F1 — OTC verify has no attempt limit / lockout / per-IP throttle.**
`backend/app/otc.py:211 verify_code` (endpoint `backend/app/main.py:279
/auth/otc/verify`). The passcode path (`passcode.py:262`) has a 5-fail/
15-min per-account lockout; the OTC path has none. 60s cooldown is on
*request* only. 6-digit code (10^6), 10-min TTL, single live code per
email. Remediation: add a per-account failed-OTC-attempt counter +
lockout mirroring passcode.py, and/or a per-IP 429 throttle on verify;
consider invalidating the live code after N failures. Confidence: HIGH
the control is absent (verified across module + endpoint + global grep).
Severity argued down from Critical to High because bcrypt cost + 10-min
TTL + single-live-code give real friction; still a genuine gap and an
inconsistency with the passcode path.
### Low / Info
- **F2 — Session cookie `https_only=False`.** `backend/app/main.py:150`
SessionMiddleware. Cookie is itsdangerous-signed + HttpOnly, but Secure
is off "for dev parity." The device-trust cookie correctly sets
Secure=True. In prod behind HTTPS the session cookie should also be
Secure. Remediation: drive https_only from an env flag, true in prod.
Confidence: HIGH it's set false; impact Low given HTTPS + signed cookie.
- **F3 — Email-bounce webhook unauthenticated when env unset.**
`backend/app/api_notifications.py:543`. Documented v1 dev contract;
impact limited to email opt-out flag flips + outbound-row status. Info/
Low. Confidence HIGH on behavior, low on impact.
### What's solid (controls correctly implemented)
- Three-tier dependency ladder; `current_user` re-reads role +
permission_state from DB every request (revokes take effect immediately).
- Admin role model: owner-only owner grants, self-downgrade/self-mute/
self-permission/self-invite refusals, admins/owners not write-mutable.
No escalation/spoof path. Every /api/admin/* endpoint require_admin.
- Every write/mutation endpoint across all api*.py files enforces auth;
per-RFC writes additionally enforce can_*_to_rfc. No missing guard.
- No IDOR: owner/permission checks compare against server-side frontmatter
cache / session user_id / pr opened_by, never a trusted user-supplied id.
- Passcode lockout (5/15min), denylist; OTC single-live-code + consume-
before-provision (no replay, no double-sign-in).
- All tokens (OTC, passcode, device-trust, invite) bcrypt-hashed at rest;
device-trust/invite tokens 256-bit CSPRNG; constant-time bcrypt walks,
no early-exit timing leak; raw tokens never logged.
- Gitea webhook mandatory HMAC-SHA256 + constant-time compare; OAuth state
param checked with secrets.compare_digest.
- SECRET_KEY is _required (no insecure default); unsubscribe tokens signed
with it (salted, not forgeable).
- OTC request returns 202 regardless (no email enumeration); verify
collapses wrong/unknown to one 400 (no enumeration); passcode verify
collapses wrong/unknown likewise.
## What the driver needs to know
- F1 is the only finding worth a remediation slice. It is a real,
exploitable-with-effort gap AND an internal inconsistency (passcode has a
lockout, OTC doesn't). Recommend the driver fold it in at High; if the
driver judges the bcrypt+TTL friction insufficient given OHM is publicly
reachable, it could argue back toward Critical — I left it at High after
refutation.
- F2/F3 are documented dev contracts; flag them but they are low priority.
Worth a one-line confirmation that OHM's prod overlay sets
WEBHOOK_EMAIL_BOUNCE_SECRET and serves over HTTPS (the device-trust
cookie is Secure-only, so if OHM weren't HTTPS, device trust would
already be broken — implying HTTPS is in fact present, which de-risks F2).
- The authz guard coverage is genuinely complete — guarantee #4 holds for
every write/admin/owner endpoint I enumerated. No anon-write hole found.
## §19.2 candidates surfaced
- Per-account OTC-verify lockout + per-IP verify throttle (pairs with the
SPEC's already-listed per-IP passcode-verify candidate). This is the
remediation for F1 and is a flotilla-irrelevant, rfc-app-core change.