feat(mail): lead inbox connect with Google OAuth, app password as fallback (HT-123) - #192
Merged
Merged
Conversation
…lback (HT-123) Engine: the Gmail OAuth callback redirects back into the app (?connected=<address> / ?connect_error=<code>) when uiBaseUrl is configured, instead of stranding the operator on a bare HTML page; unchanged when it isn't. Web: adds a gmailBeginConnect API call + beginGoogleConnect server action, and reorders the connect screen so Google is the primary path for gmail.com/googlemail.com (with app password as a collapsed fallback), a Workspace-domain hint for unrecognized domains, and an honest "not yet supported" state for Microsoft domains instead of a non-working button. The mailbox list toasts on the redirect params.
…123) Independent Codex security pass on the OAuth flow: - The two unhandled-error sinks logged the caught object on the OAuth path. An upstream failure there (token exchange, provider HTTP error) can carry a token or authorization code in its text, so this violated the never-log- a-secret guarantee. Now logs the error's CLASS only. Pre-existing from HT-40, not introduced by the OAuth-first work; fixed while in the file. - The mailbox list showed a 'Mailbox connected' toast from the ?connected= query param alone, so any link could fake a success for an address that was never connected (UI integrity, not XSS — React escapes it). The toast now only fires when that address is actually in the server-fetched roster. Codex confirmed correct and unchanged: no open redirect (uiBaseUrl is a validated bare origin; the address only reaches searchParams), the Bearer token stays server-side, the app-password path is fully intact for every non-Google provider, and Microsoft offers no non-working button. Gates: root+web typecheck clean, biome clean, 106 files / 2014 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR adds Gmail OAuth initiation and domain-based connection routing. Configured callbacks redirect to the mailbox screen with sanitized results. The mailbox screen displays and clears callback notifications while preserving existing HTML responses without a UI base URL. ChangesGmail OAuth connection flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ConnectInboxForm
participant beginGoogleConnect
participant gmailBeginConnect
participant Google
participant handleGmailConnectCallback
participant MailboxListScreen
ConnectInboxForm->>beginGoogleConnect: Request consent URL
beginGoogleConnect->>gmailBeginConnect: POST Gmail connect request
gmailBeginConnect-->>beginGoogleConnect: Return consentUrl
beginGoogleConnect-->>ConnectInboxForm: Return consentUrl
ConnectInboxForm->>Google: Redirect to consent URL
Google->>handleGmailConnectCallback: Send OAuth callback
handleGmailConnectCallback-->>MailboxListScreen: Redirect with connected or connect_error
MailboxListScreen->>MailboxListScreen: Show notification and clear parameters
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/api/gmail-connect.test.ts`:
- Around line 322-337: Add an assertion in the “never carries the code, state,
or a GmailConnectError message” test to verify the redirect Location does not
contain the GmailConnectError message “This connect link is invalid or has
expired.”, alongside the existing code and state assertions.
In `@web/src/components/ConnectInboxForm.tsx`:
- Around line 341-353: Update offerGoogleAlternative in the ConnectInboxForm
render-mode logic to include recognized Google domains such as gmail.com and
googlemail.com, while still excluding empty addresses and preserving the
lockAddress and microsoftOnly restrictions. Ensure the Google alternative
remains available when connectModeOverride is 'password'.
In `@web/src/components/MailboxListScreen.tsx`:
- Around line 86-89: Update the isConnected check in the MailboxListScreen
mailbox connection flow to compare mailbox.address and connected
case-insensitively, while preserving the existing success-toast behavior and
parameter cleanup.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: df7cdf88-5b1c-4470-a904-c6a2628ac3b8
📒 Files selected for processing (8)
src/api/gmail-connect.test.tssrc/api/gmail-connect.tssrc/composition/root.tsweb/src/components/ConnectInboxForm.tsxweb/src/components/MailboxListScreen.tsxweb/src/lib/api-types.tsweb/src/lib/api.tsweb/src/lib/mailbox-actions.ts
…-123) Three findings, all real: - The connected-address roster check used exact string equality, but the address in the redirect comes from Google's getProfile() and need not match the stored row's casing. A genuinely successful connect could therefore show NO confirmation at all (the param is stripped either way). Now compares case-insensitively. Introduced by this PR's own anti-spoofing fix. - A Gmail operator who opened 'Use an app password instead' had no way back to 'Connect with Google' — offerGoogleAlternative required preset === undefined, which excludes gmail.com/googlemail.com. Now offered for Google domains too, with a label that fits each case. Known non-Google presets still never see it. - The redirect-secrecy test's title promised the GmailConnectError message never reaches the Location, but it only asserted the code and state. Now asserts the message's absence AND pins the whole redirect URL, so anything unexpected fails by construction rather than by enumerating known secrets. Gates: root+web typecheck clean, biome clean, 106 files / 2014 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
…T-123) Reverts the case-insensitive comparison added in 8252cff. That change was made on a review finding whose premise does not hold: both sides of the comparison are the SAME string. The callback redirects with the address completeConnect resolved from getProfile(), and upsertConnectedMailbox stored that same value verbatim — there is no case normalization in the store or the schema. An exact match therefore always succeeds for a real connect. Relaxing it was a net loss: lowercasing is not injective (distinct Unicode strings can fold together), so a crafted ?connected= value could match a DIFFERENT roster address and render a success confirmation for a mailbox that was never connected — the spoofing hole this check exists to close. Comment added so the reasoning survives the next reader. Verified: web typecheck clean, biome clean, src/api/gmail-connect.test.ts 26/26. The full engine suite could not be trusted locally (another worktree's concurrent run pushed load average past 74); CI is the authoritative run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai full review |
|
With no address typed, `domainFromAddress` returns null and `preset` is undefined, so the "Connect with Google Workspace instead" link rendered on a blank form — before there is any domain to route on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai full review |
|
…s (HT-123) Adversarial review found that this module's actions never verified the operator session. Next.js dispatches Server Actions by a build-stable action-ID hash that is invokable against any route, including the one public path middleware waves through, so middleware is not a sufficient gate — actions.ts already documents this and guards every action it exports. This module holds the same server-only Bearer token and did not. Unguarded, beginGoogleConnect would hand a valid Google consent URL to an unauthenticated caller, whose consent the pre-auth callback would then persist as a connected mailbox. Guards all three actions, not only the one this branch adds: the other two share the module's Bearer token and the identical exposure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai full review |
|
Contributor
Author
Post-merge live check — greenMerge The callback's new redirect branch is live and exercised against production, unauthenticated: Before this change that request returned an HTML 400 page. The 302, the fixed error code, and the absence of any Not verified live: the connect screen itself sits behind the operator session, and the Google consent round-trip needs real credentials. Those remain the operator's first run, as stated above. |
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.
🟢 SAFE TO MERGE
Gates green. No INFERRED rows remain — the three open questions are answered below. CodeRabbit: 3 findings — 2 real and fixed, 1 wrong. Codex, in its place on the current head: 4 findings — 1 real and fixed here, 2 real but pre-existing and filed separately, 1 accepted limit.
Decision provenance
What changed
The engine has had a complete Google OAuth flow since HT-40, but the web app had no client for it — it was unreachable. This connects it and flips the emphasis.
code, orstate. When no UI base URL is configured the old page remains, unchanged.Why OAuth first: the grant is scoped to what Helpthread needs and revocable from the Google account. An app password cannot be scoped, does not expire, and Google documents it as legacy.
Review
CodeRabbit: 3 findings — 2 real and fixed, 1 wrong. The redirect-secrecy test asserted less than its title promised (real; it now pins the whole redirect URL, so any unanticipated leak fails by construction). Gmail users who opened the app-password option had no way back to OAuth (real; the Google option is now offered for Google domains too, and no longer appears on an empty address, which was the residual half of the same finding). Its claim that stored and redirected addresses can differ in case was wrong: both sides are the same string resolved from the OAuth grant, and lowercasing is not injective, so the suggested relaxation would let a crafted
?connected=value fold onto a different roster address and show a false confirmation. Reverted, with the reasoning recorded in the code.Codex (adversarial, in place of CodeRabbit — rate limited on this head, no review object produced): 4 findings — 1 real and fixed here, 3 real but pre-existing and tracked separately.
actions.tsalready documents this and guards everything it exports; this module, holding the same server-only Bearer token, did not. Unguarded, the newbeginGoogleConnectwould hand a valid consent URL to an unauthenticated caller, whose consent the pre-auth callback would persist as a connected mailbox. All three actions in the module are now guarded, not just the one this branch adds — the other two carry the identical exposure.statetoken is verified for signature and TTL but is never consumed or bound to the initiating session, so it can be replayed within its window. The callback's HTML fallback interpolates upstream error text (Google'serror_description) into a public page — this PR narrows that exposure, since the redirect path it adds carries only a fixed code. Both are filed as their own work rather than folded in here.?connected=link naming an address that is already connected still shows a success toast. The exact-match check proves the mailbox exists, not that a callback occurred; a truly authenticated confirmation needs a single-use server-issued marker. Noted in the code.uiBaseUrlis deployment config and query values go throughURLSearchParams), redirect and log leakage on the new path, token storage (AES-256-GCM, fresh 12-byte IV per encryption, validated key length, no plaintext writes), or connect-form routing (no state strands the operator).How this was verified, and how it will be
There are no PR previews in this repo — both Vercel projects deploy
mainonly. So the pre-merge evidence is tests and gates, and the live check happens immediately after merge, not before:src/api/gmail-connect.test.ts26/26 ✓ · full suite green on CI.inbox.resonantiq.apponce the deploy lands. Merging ships nothing on its own here — the deploy is a separate manual step.Not done
Microsoft OAuth connector — HT-124. The largest remaining gap, since M365 business accounts cannot use app passwords at all.
Follow-up
These screens (
ConnectInboxForm,MailboxListScreen) are app-level surface, so under the both-directions reconciliation rule they owe a trip back up to the design project. Not a merge blocker; tracked separately.