Skip to content

security: nOAuth defense, verification-token purpose binding, admin sessions - #748

Merged
lakhansamani merged 1 commit into
mainfrom
security/2.4.0-pre-release-audit
Aug 7, 2026
Merged

security: nOAuth defense, verification-token purpose binding, admin sessions#748
lakhansamani merged 1 commit into
mainfrom
security/2.4.0-pre-release-audit

Conversation

@lakhansamani

@lakhansamani lakhansamani commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Pre-release security audit remediation, part 1 of 2. Closes audit findings
F1–F5, F11 and F14. Remaining findings (F6–F10, F12, F13, F15–F22) follow in a
second PR stacked on this one.

Audit findings are written F1, not #1. A bare #1 renders as a link to an
unrelated issue in this repo.

Why

Social login resolved a local account from a provider-asserted email with
nothing attesting it. Microsoft Entra v2 ID tokens carry no email_verified
claim at all and email is a mutable directory attribute, so a free
attacker-owned tenant could assert a victim's address and land in the victim's
session. The existing pre-hijack guard did not help: it only removes
unverified local accounts, and verified accounts are exactly the target.

The vulnerability was reproduced live against the pre-fix binary during e2e
(callback returned 302 + a session for an unattested address; 400
email_not_verified after).

Findings closed

Finding Sev Description
F1 Critical nOAuth account takeover via social login
F2 High Social login linked accounts by email, ignoring email_verified
F3 High Verification-token purpose confusion → leaked link becomes ATO
F4 High Admin session was a non-expiring, non-revocable bearer cookie
F5 High No per-account brute-force lockout on password login
F11 Medium TOTP passcodes replayable within the validity window
F14 Medium Admin-secret auth had no lockout

Changes

F1/F2 — email attestation. Every provider now reports its own real signal
(email_verified, Discord verified, GitHub's verified-only filter, Entra
xms_edov). Microsoft tokens are additionally pinned: tid must be present,
iss must match the tenant it claims, and the tenant must be pinned via
--microsoft-tenant-id or listed in the new --microsoft-allowed-tenants. An
unattested address may not select a local account.

--oauth-allow-unverified-provider-email is a compatibility ramp, not an
off switch: even set, an unattested address may only create a new account or
return to one the same provider already owns. It can never cross into an
account another credential owns, which is every practical form of the attack.
Defaults off, warns on every boot, documented for removal.

F3 — purpose binding. Magic-link, signup, invite and forgot-password tokens
share one table keyed by token alone, and no consumer checked what the token
was minted for. A leaked magic link was redeemable at ResetPassword, which
also appends basic_auth to the account — a one-shot passwordless capability
escalated to durable takeover. Both the stored identifier and the signed
token_type are now checked against an allow-list per endpoint.

Gated at all three consumers: the verify_email and reset_password
GraphQL mutations and the GET /verify_email handler. That handler is a
separate implementation of the same flow — and the URL every verification and
magic-link mail actually points at — so gating only the mutations would have
left a forgot-password token redeemable there for a full session. (The same
split already caused the MFA gate to be missed on that handler once.)

F4/F14 — admin credentials. The admin cookie was bcrypt(AdminSecret):
re-derivable, no exp, no server-side record, so a captured copy worked
indefinitely and logout could not invalidate it. Now an opaque 256-bit
server-side handle with an absolute TTL and real revocation. Both
admin-secret comparison paths (login and the x-authorizer-admin-secret
header) share one throttled gate.

That gate counts failed attempts only. It runs on every request that
authenticates with the header, so counting successes would hand a 401 to a
concurrent admin API client presenting the correct secret. And
MetaFromGRPC now falls back to the gRPC peer address, so the counter is keyed
per caller — previously a pure-gRPC caller supplied no forwarded headers, every
gRPC client shared one bucket, and a handful of wrong guesses locked out all of
them at once.

What the throttle does not cover, stated in the code rather than implied: the
client IP still comes from X-Real-Ip/X-Forwarded-For on the HTTP path, so a
guesser on a directly-exposed deployment can rotate it. It is defence in depth
around a high-entropy secret, not a boundary.

F5/F11. Per-account login lockout keyed on user id, so IP rotation does not
defeat it; increment-then-check for concurrency safety, matching the OTP path.
TOTP passcodes are reserved single-use per RFC 6238 §5.2 (pquerna/otp is
stateless and accepts a code for its whole ~90s window).

Note the trade-off, now spelled out at the constant: any per-account lockout is
a DoS against that account, and the window slides. It is the same policy
verify_otp.go already applies; removing the DoS needs progressive delay rather
than a counter, which is not in this PR.

Also fixed (found while working, not in the audit)

  • Pre-hijack delete was unbounded. Deletion is now limited to accounts
    holding no state; anything else refuses the login instead, and the delete is
    audited. Since fix(storage): cascade DeleteUser to all user-keyed tables #749 the cascade destroys those rows rather than orphaning
    them, but an unauthenticated callback still must not take out an account's
    org memberships, MFA enrollment or FGA grants to resolve an email collision —
    and FGA tuples are outside the cascade entirely, since the purge lives in the
    service layer this path does not go through.
  • resend_verify_email silently did nothing when no verification request was
    pending, leaving users with no way to verify at all. It now mints a fresh
    request, gated so it cannot be used as an open mailer.
  • Completing a token password reset now verifies the address regardless of
    signup method — receiving the emailed token is proof of mailbox control.
  • --enable-email-verification with no SMTP is now fatal at boot rather than
    stranding users unverified with no recovery path.
  • Dashboard: split "Verify User" into per-identifier actions plus Resend
    Verification Email
    . The old combined item only appeared when email and
    phone were both unverified.

Verification

  • go build ./..., go vet ./..., full ./internal/... suite (SQLite),
    golangci-lint run ./..., make lint-ts, dashboard build — all green
  • 11 e2e specs against real Docker stacks (Mailpit + mock OAuth)
  • Fail-before/pass-after confirmed by reverting each fix individually,
    including the GET /verify_email purpose gate

New e2e instance authorizer-email-verify: the only stack combining basic-auth
signup with --enable-email-verification, without which the rendered
signup → "check your inbox" → click-link journey is untestable (the shared
stack has verification off; both magic-link stacks hide the password form).

Breaking changes

  • Admin cookies invalidate on deploy. Admins re-login once. The old format
    is the vulnerability, so there is no compatibility path.
  • Admin sessions are now server-side. On multi-replica deployments using the
    in-memory store, sessions are not shared across replicas; the old stateless
    cookie worked anywhere. Redis-backed memory store restores this. User sessions
    already behaved this way. The same caveat applies to the new single-use TOTP
    passcode reservation.
  • --enable-email-verification without SMTP now fails at boot.
  • Microsoft logins on a multi-tenant alias are refused unless the tenant is
    allowlisted or xms_edov is enabled. --microsoft-tenant-id defaults to
    common, so this is the default configuration — expect it to affect every
    Microsoft deployment that has not pinned a tenant. That configuration is the
    exploitable one; see docs/email-verification-contract.md for the three
    remedies.

Rebased onto #749, which landed the DeleteUser cascade this PR originally
carried.

@lakhansamani
lakhansamani force-pushed the security/2.4.0-pre-release-audit branch from d4a1607 to b35be82 Compare August 7, 2026 03:02
…essions

Pre-release audit remediation for 2.4.0: findings F1-F5, F11, F14. (Prefixed
F- rather than written bare: a `#1` renders as a link to an unrelated issue.)

F1/F2 nOAuth account takeover via social login
  Federated logins resolved a local account from a provider-asserted email
  with no attestation behind it. Entra v2 tokens carry no email_verified at
  all and `email` is a mutable directory attribute, so a free attacker-owned
  tenant could assert a victim's address and land in their session.

  Every provider now reports its own real signal, Microsoft tokens are pinned
  to a trusted tenant (or xms_edov), and an unattested address may not select
  a local account. --oauth-allow-unverified-provider-email is a narrowed
  compatibility ramp: it still cannot cross into an account another credential
  owns.

F3 verification-token purpose confusion
  Magic-link, signup, invite and forgot-password tokens shared one table keyed
  by token alone, and no consumer checked purpose. A leaked magic link was
  redeemable at ResetPassword for durable account takeover.

  Gated at all three consumers, not just the GraphQL pair: GET /verify_email
  is a separate implementation of the same flow and is the URL the mails
  actually point at, so gating only the mutations would have left a
  forgot-password token redeemable there for a full session.

F4/F14 admin session and secret
  The admin cookie was bcrypt(AdminSecret): no expiry, no revocation, logout
  could not invalidate a captured copy. Now an opaque server-side handle.

  Both admin-secret comparison paths share one throttled gate. It counts
  FAILED attempts only — it runs on every x-authorizer-admin-secret request,
  so counting successes would 401 a concurrent admin API client presenting the
  correct secret. MetaFromGRPC now falls back to the gRPC peer address, so the
  counter is keyed per caller instead of collapsing to one bucket shared by
  every gRPC client (where a handful of wrong guesses locked out all of them).

F5 per-account login lockout, F11 single-use TOTP passcodes (RFC 6238 5.2).

Also bounds the pre-hijack delete to accounts holding no state. The cascade
(#749) destroys those rows rather than orphaning them, but an unauthenticated
callback still must not take out an account's org memberships, MFA enrollment
or FGA grants to resolve an email collision, and FGA tuples are outside the
cascade entirely. Makes resend_verify_email able to mint a fresh request, and
makes email verification with no SMTP a fatal misconfiguration.
@lakhansamani
lakhansamani force-pushed the security/2.4.0-pre-release-audit branch from b35be82 to 2677a4a Compare August 7, 2026 03:06
@lakhansamani
lakhansamani merged commit 66fe488 into main Aug 7, 2026
6 checks passed
@lakhansamani
lakhansamani deleted the security/2.4.0-pre-release-audit branch August 7, 2026 03:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant