Skip to content

security(verify-email): one decision core, reject empty-subject token - #755

Merged
lakhansamani merged 1 commit into
mainfrom
security/verify-email-core-retarget
Aug 7, 2026
Merged

security(verify-email): one decision core, reject empty-subject token#755
lakhansamani merged 1 commit into
mainfrom
security/verify-email-core-retarget

Conversation

@lakhansamani

Copy link
Copy Markdown
Contributor

Re-targets #752 onto main. That PR is merged — which is why it does not show up in the open list — but it merged into security/2.4.0-audit-part-2 four minutes after that branch had already merged to main, so it landed in a dead end.

Re-verified against current main (3f17f68a) before opening this:

$ git merge-base --is-ancestor 01e0f76b origin/main   →  NO, not in main
$ git ls-tree origin/main internal/service/           →  no verify_email_core.go
$ git grep "verification token has no subject" main   →  ABSENT

None of it shipped, including in 2.4.0-rc.16.

What it does

One decision core. The GraphQL and REST verify-email paths had drifted apart twice, each time because the same decision was written in two places — the second drift is the bug users hit, where a verification click that landed on the MFA gate never wrote email_verified_at.

Both paths now call ConsumeEmailVerificationToken, which owns token lookup, JWT validation, purpose binding, subject handling, user lookup, the revoked check, and the verified write. The write happens before any caller gate — that ordering is precisely what the earlier bug got wrong.

Empty-subject tokens are refused. ValidateJWTClaims passes on an empty sub, because User.ID is "" at that call site — so a token with no subject validated and resolved to no principal instead of failing.

The one conflict, and why both sides are kept

signup_verification_matrix_test.go. Main gained TestTOTPEnrollmentWorksForPhoneOnlyAccounts from #753 while this branch added the expired-link and empty-subject tests.

Worth noting how this failed loudly rather than silently: my first resolution stripped the conflict markers mechanically, which spliced two functions together because HEAD's closing brace sat after the >>>>>>> marker as shared context. gofmt/go vet caught it immediately. All three tests are present and the file parses.

Verified

  • go build ./... — 0
  • go vet ./... — 0
  • All 10 tests in the verification matrix pass, including the two this PR adds
  • make test42 packages, 0 failures
  • make lint — clean

Once merged this should go into an rc — rc.16 went out without it.

Re-targets PR #752 onto main. That PR merged into security/2.4.0-audit-part-2
four minutes AFTER that branch had already merged to main, so it landed in a
dead end: GitHub shows it merged, but its merge commit is not an ancestor of
main and none of its content ever shipped.

The GraphQL and REST verify-email paths had drifted apart twice, each time
because the same decision was implemented in two places. Both now route through
ConsumeEmailVerificationToken, which owns token lookup, JWT validation, purpose
binding, subject handling, user lookup, the revoked check, and the
email_verified_at write — the write happens before any caller gate, which is
what the earlier bug got wrong.

Also rejects a verification token with an empty subject. ValidateJWTClaims
passes on an empty sub because User.ID is "" at that call site, so the token
resolved to no principal instead of failing.

Cherry-picked with one conflict, in the signup matrix test: main gained
TestTOTPEnrollmentWorksForPhoneOnlyAccounts from #753 while this branch added
the expired-link and empty-subject tests. All three are kept.
@lakhansamani
lakhansamani merged commit 0ada7ef into main Aug 7, 2026
4 checks passed
lakhansamani added a commit to authorizerdev/docs that referenced this pull request Aug 7, 2026
authorizerdev/authorizer#755 landed the verify-email core on main, which
documents two user-visible behaviours the ported page lacked: requesting a new
link invalidates the previous one immediately (nonce rotation), and the resend
response is deliberately generic so it cannot be used to probe which addresses
are registered.
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