9e6756f678
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>
201 lines
11 KiB
Markdown
201 lines
11 KiB
Markdown
# 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, 4–20 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.
|