security: nOAuth defense, verification-token purpose binding, admin sessions - #748
Merged
Merged
Conversation
lakhansamani
force-pushed
the
security/2.4.0-pre-release-audit
branch
from
August 7, 2026 03:02
d4a1607 to
b35be82
Compare
…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
force-pushed
the
security/2.4.0-pre-release-audit
branch
from
August 7, 2026 03:06
b35be82 to
2677a4a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
Why
Social login resolved a local account from a provider-asserted email with
nothing attesting it. Microsoft Entra v2 ID tokens carry no
email_verifiedclaim at all and
emailis a mutable directory attribute, so a freeattacker-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_verifiedafter).Findings closed
email_verifiedChanges
F1/F2 — email attestation. Every provider now reports its own real signal
(
email_verified, Discordverified, GitHub's verified-only filter, Entraxms_edov). Microsoft tokens are additionally pinned:tidmust be present,issmust match the tenant it claims, and the tenant must be pinned via--microsoft-tenant-idor listed in the new--microsoft-allowed-tenants. Anunattested address may not select a local account.
--oauth-allow-unverified-provider-emailis a compatibility ramp, not anoff 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, whichalso appends
basic_authto the account — a one-shot passwordless capabilityescalated to durable takeover. Both the stored identifier and the signed
token_typeare now checked against an allow-list per endpoint.Gated at all three consumers: the
verify_emailandreset_passwordGraphQL mutations and the
GET /verify_emailhandler. That handler is aseparate 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 workedindefinitely 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-secretheader) 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
MetaFromGRPCnow falls back to the gRPC peer address, so the counter is keyedper 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-Foron the HTTP path, so aguesser 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.goalready applies; removing the DoS needs progressive delay ratherthan a counter, which is not in this PR.
Also fixed (found while working, not in the audit)
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_emailsilently did nothing when no verification request waspending, 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.
signup method — receiving the emailed token is proof of mailbox control.
--enable-email-verificationwith no SMTP is now fatal at boot rather thanstranding users unverified with no recovery path.
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 greenincluding the
GET /verify_emailpurpose gateNew e2e instance
authorizer-email-verify: the only stack combining basic-authsignup with
--enable-email-verification, without which the renderedsignup → "check your inbox" → click-link journey is untestable (the shared
stack has verification off; both magic-link stacks hide the password form).
Breaking changes
is the vulnerability, so there is no compatibility path.
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-verificationwithout SMTP now fails at boot.allowlisted or
xms_edovis enabled.--microsoft-tenant-iddefaults tocommon, so this is the default configuration — expect it to affect everyMicrosoft deployment that has not pinned a tenant. That configuration is the
exploitable one; see
docs/email-verification-contract.mdfor the threeremedies.
Rebased onto #749, which landed the
DeleteUsercascade this PR originallycarried.