Conversation
…doption safely
An onLogin hook that returns { user } bypasses the account-adoption gate, but until now
the plugin stripped its authenticated-source determination before calling the hook — so a
hook could only see oauthUser.emailVerified, which an unsigned UserInfo body can assert
(email_verified: true). Hook-based deployments therefore couldn't distinguish a
JWKS-signed verified email from a spoofable one.
Add oauthUser.emailAuthenticated: the plugin sets it true only when the email is verified
AND came from an authenticated source (a signature-verified, issuer-validated OIDC id
token, or GitHub's authenticated email fetch) — the same trust basis the built-in gate
uses. Hooks should gate adoption of an existing account on emailAuthenticated === true,
not emailVerified. Docs updated; unit tests cover the signed-verified (true) and
unauthenticated-userinfo (false) paths.
Follow-up to #231 (item 2). Does not change the built-in gate.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the emailAuthenticated property to the OAuthUser object, allowing lifecycle hooks like onLogin to securely gate account adoption based on whether the email originates from a cryptographically authenticated source (such as a signature-verified OIDC ID token or GitHub's authenticated email endpoint). The changes include updates to the handler logic, type definitions, documentation, and unit tests. I have no feedback to provide on these changes.
…ign review) Per the Codex design review of #232 (framing-verdict: chosen-approach-sound, Option A), replace the single oauthUser.emailAuthenticated boolean with a grouped, frozen oauthUser.authEvidence object, applying its contract corrections: - Normalize the public emailProvenance: 'signed-oidc' is reported only when the id token signature AND issuer were verified (getUserInfo labels a decoded-only token 'signed-oidc' internally — that must not be exported as trusted); otherwise 'unauthenticated'. - No generic authenticated `subject`: expose idTokenSubject only from a signature+issuer verified token (normalized to string), avoiding the preferIdToken:false trap where a UserInfo subject could be presented as verified. - Preserve emailVerified: boolean | undefined (don't coerce); include the email the evidence describes so a later oauthUser.email mutation can't leave stale evidence. - Attach only when an onLogin hook is registered, non-enumerable + non-writable + frozen, so it isn't persisted into the session and can't be silently replaced. - authEvidence is optional on the exported OAuthUser; OAuthAuthEvidence/EmailProvenance exported from index.ts. Docs updated to gate adoption on authEvidence (missing evidence = insufficient auth, never fall back to emailVerified). Unit tests cover signed/normalized-unsigned/github/plain-userinfo and the no-hook (absent) path. Deeper real-provider + real-Harper JWKS coverage tracked in #230. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…olean) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…suer, readonly)
Implementation review (verdict CHANGES, adjudicated major) fixes:
- emailAuthenticated no longer overstates the gate: it requires a USABLE verified email
(typeof email === 'string' && !== '') from an authenticated source, and attests
authEvidence.email, NOT oauthUser.username. This closes the Okta preferred_username /
GitHub login case where a handle-mapped login could read emailAuthenticated:true and a
hook adopting by username would inherit an account the no-hook gate denies; also the
email_verified-with-no-email case (was emailAuthenticated:true, email:undefined).
- Add authEvidence.idTokenIssuer (the validated iss) so a hook can bind on the
(idTokenIssuer, idTokenSubject) pair — idTokenSubject alone isn't unique when a provider
config lists multiple issuers. Both exposed only when signature+issuer verified.
- Coerce idTokenSubject/idTokenIssuer only from string|number (no [object Object] from a
malformed claim).
- Mark all OAuthAuthEvidence fields readonly so TypeScript rejects a write that would throw
at runtime (strict mode) and be swallowed by HookManager, silently skipping a hook denial.
- Docs: adopt from authEvidence.email (or the issuer+subject pair), never { user: username }
on emailAuthenticated alone; issuerValidated means *a* configured issuer matched.
- CHANGELOG: add an Unreleased entry for the authEvidence public API (2.6.0 is released).
- Tests: independent issuerValidated=false, github-provenance on a non-github provider,
no-usable-email, idTokenIssuer, and the frozen/non-enumerable contract.
Deeper real-provider + real-Harper JWKS-adoption coverage remains tracked in #230.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… 'verified' Round-2 review minors: the authEvidence property itself was type-mutable (TS allowed a reassignment that throws at runtime and is swallowed by HookManager, skipping a hook denial) — make it readonly. And the email field is populated regardless of trust, so 'the verified email' overstated it; clarify it is verified only when emailAuthenticated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
heskew
marked this pull request as ready for review
September 14, 2026 20:46
Contributor
|
Reviewed; no blockers found. Suggestions (non-blocking)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Map to Harper user | ||
| const user = provider.mapUserToHarper(userInfo); | ||
|
|
||
| // Expose authenticated-source evidence to onLogin only (non-enumerable, so it is |
Contributor
There was a problem hiding this comment.
💡 Suggestion (non-blocking) — Extract evidence helper
Consider extracting the authEvidence calculation into a helper function. While clear now, this logic (normalization of provenance, toIdString, etc.) is likely to grow as more providers or trust signals are added.
| @@ -621,6 +621,50 @@ export interface GetUserInfoHelpers { | |||
| logger?: Logger; | |||
Contributor
There was a problem hiding this comment.
💡 Suggestion (non-blocking) — Add JSDoc example
Consider adding an @example block to OAuthAuthEvidence showing the recommended check for account adoption (gating on emailAuthenticated and resolving from email).
Contributor
|
Reviewed; no blockers found. |
This branch has not been deployed
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
1 participant
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.
Summary
Follow-up to #231 (item 2). Gives
onLoginhooks the plugin's own authenticated-source evidence so they can gate account adoption safely.An
onLoginhook that returns{ user }is authoritative and bypasses the built-in account-adoption gate. Until now the plugin computed its trust determination but stripped it before calling the hook, leaving onlyoauthUser.emailVerified— which mirrors a provider claim an unsigned UserInfo body can assert (email_verified: true). So a hook couldn't tell a JWKS-signed verified email from a spoofable one, and the documented "gate onemailVerified" pattern was unsafe for adopting existing accounts.Change
Adds
oauthUser.authEvidence(OAuthAuthEvidence), passed toonLogin:emailProvenance'signed-oidc'(signature and issuer verified),'github-authenticated', or'unauthenticated'— normalized (a decoded-but-unverified token is'unauthenticated', never'signed-oidc')emailAuthenticatedemailVerified === true && emailProvenance !== 'unauthenticated'— the conservative one-line checksignatureVerified/issuerValidatedemailVerifiedboolean | undefined(unknown preserved)emailidTokenSubjectundefinedonLoginhook is registered, non-enumerable + non-writable + frozen (not persisted into the session; can't be silently swapped).OAuthUser;OAuthAuthEvidence/EmailProvenanceexported fromindex.ts.authEvidence(missing evidence = insufficient auth, never fall back toemailVerified).Design review (Codex,
--mode plan)Framing-verdict
chosen-approach-soundfor the grouped-object approach (over flat fields / boolean-only). Its contract corrections are applied here: normalize the public provenance; don't expose a genericsubject(only a verifiedidTokenSubject, avoiding thepreferIdToken:falsewrong-identity trap); freeze only the evidence and make the property non-writable; include the snapshot email; keepemailVerifiedtri-state; build only when a hook exists; make the field optional + exported.Verification
handlers.test.jscases cover signed→authenticated, normalized-unsigned→unauthenticated, github-authenticated, plain-userinfo, and the no-hook (absent) path;tsc --noEmitclean; lint 0 errors (4 pre-existing warnings); Prettier clean.Draft pending your review + the cross-model review.
🤖 Generated with Claude Code