Repository navigation
feat(auth): accept pending drive invites on login - #1236
Merged
Merged
Conversation
Adds the post-login pending invitation acceptance hook (Epic 3 of the pu/invites redo) so a user with a pending drive_members row gains drive access on their next successful login via any auth flow. Why: Epic 1 hardened authz on acceptedAt IS NOT NULL, which made any pending row invisible to drive queries. Without this hook, an invitee could authenticate but never reach the drive they were invited to. Implementation: - driveInviteRepository gains findPendingMembersForUser + acceptPendingMember (race-safe conditional UPDATE). - Pure helper acceptUserPendingInvitations orchestrates accept + best- effort broadcast. Acceptance writes propagate so callers can revoke the session; broadcast/recipient-resolution failures log and continue (the original PR coupled them, which review flagged as a flaw). - Wired into all 9 session-creating auth routes: magic-link verify, passkey authenticate, signup-passkey, google callback/native/one-tap, apple callback/native, and mobile google exchange. Magic-link verify also honours an optional ?inviteDriveId redirect hint. - Coverage gate test asserts every future session-creating route under /api/auth either calls the helper or appears on a justified allow-list (currently: ws-token, device/refresh, mobile/refresh). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds a post-login step that accepts pending drive invitations after session creation across OAuth, passkey, magic-link, and signup flows; introduces repository support, websocket fan-out helpers, tests, and a Vitest coverage gate enforcing acceptance presence in session-creating routes. Error paths revoke the newly-created session and return/redirect with server error. ChangesPost-Login Pending Invitation Acceptance
Sequence DiagramsequenceDiagram
actor User
participant AuthRoute as Auth Route
participant SessionSvc as Session Service
participant AcceptLogic as acceptUserPendingInvitations
participant InviteRepo as Drive Invite Repo
participant DriveRecipients as getDriveRecipientUserIds
participant WebSocket as WebSocket Broadcast
User->>AuthRoute: Request (callback/exchange/verify/authenticate)
AuthRoute->>SessionSvc: createSession(user)
SessionSvc-->>AuthRoute: session created (token)
AuthRoute->>AuthRoute: generate CSRF / prepare response
AuthRoute->>AcceptLogic: acceptUserPendingInvitations(user.id)
AcceptLogic->>InviteRepo: findPendingMembersForUser(user.id)
InviteRepo-->>AcceptLogic: pending invitations
loop per pending invitation
AcceptLogic->>InviteRepo: acceptPendingMember(memberId)
InviteRepo-->>AcceptLogic: accepted? (true/false)
alt accepted
AcceptLogic->>DriveRecipients: getDriveRecipientUserIds(driveId)
DriveRecipients-->>AcceptLogic: recipient user IDs
AcceptLogic->>WebSocket: broadcastDriveMemberEventToRecipients(payload, recipients)
WebSocket-->>AcceptLogic: broadcast settled
else skipped (race)
AcceptLogic-->>AcceptLogic: skip broadcast
end
end
alt acceptance threw
AcceptLogic-->>AuthRoute: error
AuthRoute->>SessionSvc: revokeSession(sessionToken, "pending_invite_acceptance_failed")
AuthRoute-->>User: error response / redirect (500 or signin?error=server_error)
else accepted successfully
AcceptLogic-->>AuthRoute: AcceptedInvitation[]
AuthRoute-->>User: continue with normal successful login response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Owner
Author
Rubric self-reviewScoring per Appendix (0–2 each):
Notes
Pre-submission checklist (§12)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/app/api/auth/apple/callback/route.ts (1)
234-239:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
trackAuthEventreceives raw unmasked email — PII leak.
verificationResult.userInfo.maskEmailis already imported (Line 16) and correctly used elsewhere in this file (e.g. Lines 145, 158). The parallelgoogle/callback/route.tsusesmaskEmail(email)in its equivalenttrackAuthEventcall.🛡️ Proposed fix
- trackAuthEvent(user.id, 'login', { - email, - ip: clientIP, - provider: 'apple', - userAgent: req.headers.get('user-agent') - }); + trackAuthEvent(user.id, 'login', { + email: maskEmail(email), + ip: clientIP, + provider: 'apple', + userAgent: req.headers.get('user-agent') + });Based on learnings: "In auth route handlers (e.g., under
apps/web/src/app/api/auth/**), ensuretrackAuthEventdoes not receive raw email (PII). Before callingtrackAuthEvent, mask the email using the existingmaskEmailutility frompackages/lib/src/audit/index.ts."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/auth/apple/callback/route.ts` around lines 234 - 239, The trackAuthEvent call is passing raw PII (email) from verificationResult.userInfo; replace that with the masked value by applying the existing maskEmail utility before invoking trackAuthEvent. Concretely, ensure the local variable used in the trackAuthEvent payload (currently email) is set to maskEmail(email) or pass maskEmail(email) directly to trackAuthEvent so the provider 'apple' login event never receives the unmasked email; update the call in route.ts where trackAuthEvent(user.id, 'login', {...}) is invoked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/app/api/auth/google/one-tap/route.ts`:
- Around line 246-253: The rate-limit reset and success telemetry are currently
executed before acceptUserPendingInvitations(), so a failure there can still
appear as a successful login; move the success-only side effects (the earlier
rate-limit reset call and the trackAuthEvent(...) invocation) to execute only
after acceptUserPendingInvitations() completes successfully — keep the try/catch
around acceptUserPendingInvitations() and keep
sessionService.revokeAllUserSessions(...) in the catch path, and place the
rate-limit reset and trackAuthEvent calls after the try block so they never run
when acceptUserPendingInvitations fails.
In `@apps/web/src/app/api/auth/mobile/oauth/google/exchange/route.ts`:
- Around line 270-277: The rollback currently calls
sessionService.revokeAllUserSessions(user.id,
'pending_invite_acceptance_failed') which will log the user out everywhere;
instead revoke only the session created by this request. Replace the
revokeAllUserSessions call with a targeted revoke that uses the session
object/ID returned when creating the session (e.g., newSession or session —
whatever variable holds the created session), for example
sessionService.revokeSession(createdSession.id,
'pending_invite_acceptance_failed') or
sessionService.revokeUserSession(session.id, ...), and keep the error log and
response behavior the same.
In `@apps/web/src/app/api/auth/signup-passkey/route.ts`:
- Around line 213-223: The current signup-passkey route revokes the newly
created session and returns 500 if acceptUserPendingInvitations(userId) fails,
which leaves users stranded; change this to treat invitation-acceptance failures
as non-fatal for the signup flow: in the catch block for
acceptUserPendingInvitations(userId) (same block that calls
sessionService.revokeAllUserSessions and NextResponse.json), remove the
revoke-and-500 behavior and instead log the error with loggers.auth.error
(including the error and userId), keep the session intact, return the normal
successful signup response, and (optionally) enqueue a background retry or set a
persistent flag so invitations can be retried on next login—this keeps behavior
local to the signup-passkey route while preserving diagnostics and recovery
paths.
In `@apps/web/src/lib/auth/post-login-pending-acceptance.ts`:
- Around line 49-57: getDriveRecipientUserIds is called after acceptedAt is
written so the newly accepted user will appear in driveRecipients; before
calling broadcastDriveMemberEventToRecipients(filter) remove userId from the
recipients array (e.g., driveRecipients = driveRecipients.filter(id => id !==
userId)), and if the filtered list is empty skip the broadcast to avoid sending
the member_added event to the accepter; keep using createDriveMemberEventPayload
and broadcastDriveMemberEventToRecipients but pass the filtered recipient list.
---
Outside diff comments:
In `@apps/web/src/app/api/auth/apple/callback/route.ts`:
- Around line 234-239: The trackAuthEvent call is passing raw PII (email) from
verificationResult.userInfo; replace that with the masked value by applying the
existing maskEmail utility before invoking trackAuthEvent. Concretely, ensure
the local variable used in the trackAuthEvent payload (currently email) is set
to maskEmail(email) or pass maskEmail(email) directly to trackAuthEvent so the
provider 'apple' login event never receives the unmasked email; update the call
in route.ts where trackAuthEvent(user.id, 'login', {...}) is invoked.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a391a95b-2a31-44ae-8364-db8eb2d9cba2
📒 Files selected for processing (30)
apps/web/src/app/api/auth/__tests__/google-callback-redirect.test.tsapps/web/src/app/api/auth/__tests__/mobile-oauth-google-exchange.test.tsapps/web/src/app/api/auth/__tests__/post-login-acceptance-coverage.test.tsapps/web/src/app/api/auth/apple/callback/__tests__/route.test.tsapps/web/src/app/api/auth/apple/callback/route.tsapps/web/src/app/api/auth/apple/native/__tests__/route.test.tsapps/web/src/app/api/auth/apple/native/route.tsapps/web/src/app/api/auth/google/__tests__/google-callback-redirect.test.tsapps/web/src/app/api/auth/google/__tests__/one-tap.test.tsapps/web/src/app/api/auth/google/__tests__/open-redirect-protection.test.tsapps/web/src/app/api/auth/google/callback/__tests__/route.test.tsapps/web/src/app/api/auth/google/callback/route.tsapps/web/src/app/api/auth/google/native/__tests__/route.test.tsapps/web/src/app/api/auth/google/native/route.tsapps/web/src/app/api/auth/google/one-tap/__tests__/route.test.tsapps/web/src/app/api/auth/google/one-tap/route.tsapps/web/src/app/api/auth/magic-link/verify/__tests__/desktop-verify.test.tsapps/web/src/app/api/auth/magic-link/verify/__tests__/route.test.tsapps/web/src/app/api/auth/magic-link/verify/route.tsapps/web/src/app/api/auth/mobile/oauth/google/exchange/__tests__/route.test.tsapps/web/src/app/api/auth/mobile/oauth/google/exchange/route.tsapps/web/src/app/api/auth/passkey/authenticate/__tests__/route.test.tsapps/web/src/app/api/auth/passkey/authenticate/route.tsapps/web/src/app/api/auth/signup-passkey/__tests__/route.test.tsapps/web/src/app/api/auth/signup-passkey/route.tsapps/web/src/lib/auth/__tests__/post-login-pending-acceptance.test.tsapps/web/src/lib/auth/post-login-pending-acceptance.tsapps/web/src/lib/repositories/__tests__/drive-invite-repository.test.tsapps/web/src/lib/repositories/drive-invite-repository.tsapps/web/src/lib/websocket/socket-utils.ts
Four reviewer-flagged issues plus an adjacent PII fix:
1. Post-login helper no longer echoes `member_added` to the just-accepted user.
`getDriveRecipientUserIds()` now runs after `acceptedAt` is written, so the
acceptee was previously included in their own broadcast. Filter them out
before fan-out and skip the broadcast entirely when no other recipients
remain (e.g. solo drive). New tests cover both branches.
2. Scope the rollback to the just-created session via
`sessionService.revokeSession(sessionToken, …)` instead of
`revokeAllUserSessions`. On routes that did not pre-revoke (mobile OAuth
exchange, google/apple callbacks, one-tap) the prior code would log the
user out on every device whenever invitation acceptance failed. Tests now
assert the targeted revoke and that revokeAllUserSessions is NOT called
with the new reason.
3. Move the rate-limit reset and `trackAuthEvent('login')` calls in
google/one-tap to AFTER the acceptance hook, so a thrown
`acceptUserPendingInvitations()` no longer clears failure counters or
emits a misleading success-login event before the rollback path runs.
4. Mask the email passed to `trackAuthEvent` in apple/callback (the original
CodeRabbit flag) and the parallel apple/native, google/native, and
mobile/oauth/google/exchange handlers — same PII leak pattern that
google/callback already guarded against. Existing route tests updated to
assert masked-only emails.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
CodeRabbit review responses (commit c73ed0b)Replying inline since the per-thread reply API was 502'ing — happy to also resolve threads once stable. 1.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/web/src/app/api/auth/magic-link/verify/route.ts (1)
164-176: ⚡ Quick winExtract the redirect-path resolution into one helper.
The
matchedInviteDrive/isNewUser/provisionGettingStartedDriveIfNeededbranch now exists twice. Keeping it duplicated makes the desktop and web post-login flows easy to drift apart on the next redirect-rule change.♻️ Possible extraction
+async function resolvePostLoginRedirectPath({ + matchedInviteDriveId, + isNewUser, + userId, +}: { + matchedInviteDriveId: string | null; + isNewUser: boolean; + userId: string; +}): Promise<string> { + if (matchedInviteDriveId) { + return `/dashboard/${matchedInviteDriveId}`; + } + + if (!isNewUser) { + return '/dashboard'; + } + + try { + const provisionedDrive = await provisionGettingStartedDriveIfNeeded(userId); + if (provisionedDrive) { + return `/dashboard/${provisionedDrive.driveId}`; + } + } catch (error) { + loggers.auth.error('Failed to provision Getting Started drive', error as Error, { userId }); + } + + return '/dashboard'; +} + - let desktopRedirectPath = '/dashboard'; - if (matchedInviteDrive) { - desktopRedirectPath = `/dashboard/${matchedInviteDrive.driveId}`; - } else if (isNewUser) { - try { - const provisionedDrive = await provisionGettingStartedDriveIfNeeded(userId); - if (provisionedDrive) { - desktopRedirectPath = `/dashboard/${provisionedDrive.driveId}`; - } - } catch (error) { - loggers.auth.error('Failed to provision Getting Started drive', error as Error, { userId }); - } - } + const desktopRedirectPath = await resolvePostLoginRedirectPath({ + matchedInviteDriveId: matchedInviteDrive?.driveId ?? null, + isNewUser, + userId, + }); ... - let redirectPath = '/dashboard'; - - if (matchedInviteDrive) { - redirectPath = `/dashboard/${matchedInviteDrive.driveId}`; - } else if (isNewUser) { - try { - const provisionedDrive = await provisionGettingStartedDriveIfNeeded(userId); - if (provisionedDrive) { - redirectPath = `/dashboard/${provisionedDrive.driveId}`; - } - } catch (error) { - loggers.auth.error('Failed to provision Getting Started drive', error as Error, { - userId, - }); - // Continue with default dashboard redirect - } - } + const redirectPath = await resolvePostLoginRedirectPath({ + matchedInviteDriveId: matchedInviteDrive?.driveId ?? null, + isNewUser, + userId, + });Also applies to: 228-244
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/auth/magic-link/verify/route.ts` around lines 164 - 176, Duplicate redirect-path resolution logic (matchedInviteDrive / isNewUser / provisionGettingStartedDriveIfNeeded) appears in multiple places (e.g., the block setting desktopRedirectPath and again around lines 228-244); extract that logic into a single helper function (e.g., resolvePostLoginRedirect(userId, matchedInviteDrive, isNewUser)) that returns the final redirect path string, replace both inline branches with calls to that helper, and preserve existing error handling (loggers.auth.error) inside the helper when provisioning fails.apps/web/src/lib/auth/__tests__/post-login-pending-acceptance.test.ts (1)
115-129: ⚡ Quick winAdd invocation-order assertions to verify
acceptPendingMemberis called beforegetDriveRecipientUserIds.Test 3 exists precisely because the implementation accepts the member first (writing
acceptedAt) and then queries recipients — so the acceptee is already a member and appears in the recipient list, requiring the filter. The mock returns the same fixed recipient list regardless of call timing, so the test passes even if the order were reversed in a future refactor, giving a false green for a semantic regression.The PR's own testing rules call for
mock.invocationCallOrderfor exactly this scenario. The same gap exists in Tests 2 and 6.♻️ Suggested addition to Test 3 (and equivalently Tests 2/6)
await acceptUserPendingInvitations('user_x'); + // Critical: accept must be written before recipients are queried, so the + // acceptee appears in the list and can then be filtered out. + const acceptOrder = + vi.mocked(driveInviteRepository.acceptPendingMember).mock.invocationCallOrder[0]; + const recipientsOrder = + vi.mocked(getDriveRecipientUserIds).mock.invocationCallOrder[0]; + expect(acceptOrder).toBeLessThan(recipientsOrder!); + expect(broadcastDriveMemberEventToRecipients).toHaveBeenCalledTimes(1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/auth/__tests__/post-login-pending-acceptance.test.ts` around lines 115 - 129, Add invocation-order assertions to ensure the implementation calls acceptPendingMember before querying recipients: after invoking acceptUserPendingInvitations('user_x') in this test, capture invocation orders for vi.mocked(driveInviteRepository.acceptPendingMember) and vi.mocked(getDriveRecipientUserIds) using mock.invocationCallOrder and assert the acceptPendingMember call order is less than the getDriveRecipientUserIds call order; apply the same pattern to the other two failing tests (the ones labelled Test 2 and Test 6) to prevent a future refactor from reversing the semantic order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/app/api/auth/google/one-tap/route.ts`:
- Around line 247-254: The inline regex masking for maskedEmail can fail for
1-char local parts and may leak raw email; replace the manual regex with the
already-imported maskEmail utility (use maskedEmail = maskEmail(email)) before
calling trackAuthEvent(user.id, isNewUser ? 'signup' : 'login', ...), keeping
the same payload keys (email: maskedEmail, ip: clientIP, provider:
'google-one-tap', userAgent: req.headers.get('user-agent')) so all auth handlers
consistently pass masked emails.
---
Nitpick comments:
In `@apps/web/src/app/api/auth/magic-link/verify/route.ts`:
- Around line 164-176: Duplicate redirect-path resolution logic
(matchedInviteDrive / isNewUser / provisionGettingStartedDriveIfNeeded) appears
in multiple places (e.g., the block setting desktopRedirectPath and again around
lines 228-244); extract that logic into a single helper function (e.g.,
resolvePostLoginRedirect(userId, matchedInviteDrive, isNewUser)) that returns
the final redirect path string, replace both inline branches with calls to that
helper, and preserve existing error handling (loggers.auth.error) inside the
helper when provisioning fails.
In `@apps/web/src/lib/auth/__tests__/post-login-pending-acceptance.test.ts`:
- Around line 115-129: Add invocation-order assertions to ensure the
implementation calls acceptPendingMember before querying recipients: after
invoking acceptUserPendingInvitations('user_x') in this test, capture invocation
orders for vi.mocked(driveInviteRepository.acceptPendingMember) and
vi.mocked(getDriveRecipientUserIds) using mock.invocationCallOrder and assert
the acceptPendingMember call order is less than the getDriveRecipientUserIds
call order; apply the same pattern to the other two failing tests (the ones
labelled Test 2 and Test 6) to prevent a future refactor from reversing the
semantic order.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 38da952f-37a0-4d89-be90-f6b2b751c7c9
📒 Files selected for processing (21)
apps/web/src/app/api/auth/__tests__/mobile-oauth-google-exchange.test.tsapps/web/src/app/api/auth/apple/callback/__tests__/route.test.tsapps/web/src/app/api/auth/apple/callback/route.tsapps/web/src/app/api/auth/apple/native/__tests__/route.test.tsapps/web/src/app/api/auth/apple/native/route.tsapps/web/src/app/api/auth/google/callback/__tests__/route.test.tsapps/web/src/app/api/auth/google/callback/route.tsapps/web/src/app/api/auth/google/native/__tests__/route.test.tsapps/web/src/app/api/auth/google/native/route.tsapps/web/src/app/api/auth/google/one-tap/__tests__/route.test.tsapps/web/src/app/api/auth/google/one-tap/route.tsapps/web/src/app/api/auth/magic-link/verify/__tests__/route.test.tsapps/web/src/app/api/auth/magic-link/verify/route.tsapps/web/src/app/api/auth/mobile/oauth/google/exchange/__tests__/route.test.tsapps/web/src/app/api/auth/mobile/oauth/google/exchange/route.tsapps/web/src/app/api/auth/passkey/authenticate/__tests__/route.test.tsapps/web/src/app/api/auth/passkey/authenticate/route.tsapps/web/src/app/api/auth/signup-passkey/__tests__/route.test.tsapps/web/src/app/api/auth/signup-passkey/route.tsapps/web/src/lib/auth/__tests__/post-login-pending-acceptance.test.tsapps/web/src/lib/auth/post-login-pending-acceptance.ts
✅ Files skipped from review due to trivial changes (1)
- apps/web/src/app/api/auth/tests/mobile-oauth-google-exchange.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/web/src/app/api/auth/signup-passkey/tests/route.test.ts
- apps/web/src/app/api/auth/apple/callback/tests/route.test.ts
- apps/web/src/app/api/auth/google/one-tap/tests/route.test.ts
- apps/web/src/app/api/auth/passkey/authenticate/tests/route.test.ts
- apps/web/src/lib/auth/post-login-pending-acceptance.ts
- apps/web/src/app/api/auth/mobile/oauth/google/exchange/route.ts
- apps/web/src/app/api/auth/google/callback/route.ts
- apps/web/src/app/api/auth/magic-link/verify/tests/route.test.ts
- Replace inline regex maskEmail in google/one-tap with the shared maskEmail utility — the regex form would emit raw email when the pattern did not match (e.g. 1-char local parts). - Extract resolvePostLoginRedirectPath helper in magic-link/verify so the desktop exchange and web cookie redirect flows share one source of truth and cannot drift on the next redirect-rule change. - Add invocation-order assertions in post-login-pending-acceptance tests so the load-bearing acceptance-then-recipients-then-broadcast ordering is locked. Without these, a future refactor pulling recipients before acceptance would silently make the self-filter dead code and re-introduce the member_added echo to the acceptee. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
CodeRabbit round 2 responses (commit 83cb037)Actionable: google/one-tap regex maskEmailFixed. Replaced the inline Nitpick: extract redirect-path resolution helper in magic-link/verifyDone. New private helper Nitpick: invocation-order assertions in helper testsDone. Added
These guard against a future refactor that pulls recipients before acceptance, which would silently make the self-filter dead code and re-introduce the member_added echo regression. |
Owner
Author
|
@coderabbitai review Triggering an explicit re-review on commit 83cb037 (round 2 nitpicks: maskEmail utility, redirect-path helper extraction, invocation-order assertions). All prior threads have been resolved. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
[review] |
Tighten the acceptedInvitations local type binding from a structural
{driveId, driveName, role: string} to the exported AcceptedInvitation
type so the role field stays narrowed to 'OWNER' | 'ADMIN' | 'MEMBER'
instead of widening to string.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add unit tests for the new fan-out helper:
- multi-recipient broadcasts route to each user:{userId}:drives channel
- empty recipient list short-circuits before fetch
- absent INTERNAL_REALTIME_URL short-circuits before fetch
- partial fetch failures do not propagate (best-effort contract;
broadcast must never abort the calling operation, e.g. login)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Same class of issue as the google/one-tap regex maskEmail: the inline
form (email.substring(0, 3) + '***@' + domain) leaks domain characters
into the local segment for short addresses. For example
'a@example.com' → 'a@e***@example.com'. Use the shared maskEmail
utility (already imported) which always emits a clean
{firstTwoChars}***@{domain} form. Test assertion updated to the
canonical 2-char prefix.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review Re-triggering review on latest HEAD (4bd6eaf). Recent commits since the last review pass:
All prior CodeRabbit threads are resolved. Local validation: 939/939 tests, typecheck/lint green. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
All prior threads resolved and 939/939 tests green — reviewing now. [review] |
This was referenced May 5, 2026
2witstudios
added a commit
that referenced
this pull request
May 5, 2026
* fix(realtime): fan out member_removed to recipients Switch the DELETE handler from broadcastDriveMemberEvent (single-recipient, sent to the just-removed user) to broadcastDriveMemberEventToRecipients fanned out across getDriveRecipientUserIds(driveId). Other admins watching the members page now see the row disappear in realtime instead of having to refresh, matching the parity already in place for member_added. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(realtime): inspect Response.ok in fan-out tally broadcastDriveEvent and broadcastDriveMemberEventToRecipients used Promise.allSettled to track per-recipient success but only counted status === 'rejected'. fetch resolves on HTTP 4xx/5xx (it does not throw — only network errors throw), so silent broadcast failures were being logged as successes. Surface !response.ok inside the map callback so it shows up as a rejected settled-result and the failed/total counters in the warn log become accurate. Network errors continue to count as failures unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: remove stray ralph-loop.local.md The file was accidentally committed in PR #1236; *.local.md is agent-loop state that doesn't belong in source control. Add a recursive **/.claude/*.local.md gitignore rule so future stray files in nested .claude/ directories (e.g. apps/web/.claude/) are ignored as well — the existing top-level .claude/*.local.md rule only matched the repo root. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(realtime): make post-commit broadcast non-fatal in member-removed Codex review flagged that getDriveRecipientUserIds runs after the membership-delete transaction but inside the main request try/catch, so a transient DB blip during recipient lookup would surface as a 500 even though the member was already removed — a false failure signal that can trigger client retries against already-applied state. Wrap the recipient lookup + broadcast in an inner try/catch so any post-commit failure is logged as best-effort and the handler still returns 200. Mirrors the pattern already used in post-login-pending-acceptance for member_added. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(realtime): make post-commit kicks non-fatal in member-removed Apply the same best-effort contract to the page-room kick block as the broadcast block above. The pre-existing db.select for drivePages also runs after the membership-delete commit; without a wrap, a transient DB blip there would mask a successful removal as a 500 — the very same bug shape Codex flagged for the broadcast. Now both post-commit side-effect blocks (broadcast + kicks) log on failure and return 200, so the post-commit semantics of this handler are uniform. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(tests): align createDriveMemberEventPayload mock with production shape CodeRabbit nitpick: the mocked factory returned { event, data } but the real helper from socket-utils.ts returns { operation, ...options }. The existing assertions rely only on call args (not return shape), so this is purely a fidelity fix — no test behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 6, 2026
2witstudios
added a commit
that referenced
this pull request
May 15, 2026
* feat(auth): accept pending drive invites on login
Adds the post-login pending invitation acceptance hook (Epic 3 of the
pu/invites redo) so a user with a pending drive_members row gains drive
access on their next successful login via any auth flow.
Why: Epic 1 hardened authz on acceptedAt IS NOT NULL, which made any
pending row invisible to drive queries. Without this hook, an invitee
could authenticate but never reach the drive they were invited to.
Implementation:
- driveInviteRepository gains findPendingMembersForUser +
acceptPendingMember (race-safe conditional UPDATE).
- Pure helper acceptUserPendingInvitations orchestrates accept + best-
effort broadcast. Acceptance writes propagate so callers can revoke
the session; broadcast/recipient-resolution failures log and continue
(the original PR coupled them, which review flagged as a flaw).
- Wired into all 9 session-creating auth routes: magic-link verify,
passkey authenticate, signup-passkey, google callback/native/one-tap,
apple callback/native, and mobile google exchange. Magic-link verify
also honours an optional ?inviteDriveId redirect hint.
- Coverage gate test asserts every future session-creating route under
/api/auth either calls the helper or appears on a justified allow-list
(currently: ws-token, device/refresh, mobile/refresh).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(auth): address CodeRabbit review feedback on Epic 3
Four reviewer-flagged issues plus an adjacent PII fix:
1. Post-login helper no longer echoes `member_added` to the just-accepted user.
`getDriveRecipientUserIds()` now runs after `acceptedAt` is written, so the
acceptee was previously included in their own broadcast. Filter them out
before fan-out and skip the broadcast entirely when no other recipients
remain (e.g. solo drive). New tests cover both branches.
2. Scope the rollback to the just-created session via
`sessionService.revokeSession(sessionToken, …)` instead of
`revokeAllUserSessions`. On routes that did not pre-revoke (mobile OAuth
exchange, google/apple callbacks, one-tap) the prior code would log the
user out on every device whenever invitation acceptance failed. Tests now
assert the targeted revoke and that revokeAllUserSessions is NOT called
with the new reason.
3. Move the rate-limit reset and `trackAuthEvent('login')` calls in
google/one-tap to AFTER the acceptance hook, so a thrown
`acceptUserPendingInvitations()` no longer clears failure counters or
emits a misleading success-login event before the rollback path runs.
4. Mask the email passed to `trackAuthEvent` in apple/callback (the original
CodeRabbit flag) and the parallel apple/native, google/native, and
mobile/oauth/google/exchange handlers — same PII leak pattern that
google/callback already guarded against. Existing route tests updated to
assert masked-only emails.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(auth): apply CodeRabbit round 2 nitpicks
- Replace inline regex maskEmail in google/one-tap with the shared
maskEmail utility — the regex form would emit raw email when the
pattern did not match (e.g. 1-char local parts).
- Extract resolvePostLoginRedirectPath helper in magic-link/verify so
the desktop exchange and web cookie redirect flows share one
source of truth and cannot drift on the next redirect-rule change.
- Add invocation-order assertions in post-login-pending-acceptance
tests so the load-bearing acceptance-then-recipients-then-broadcast
ordering is locked. Without these, a future refactor pulling
recipients before acceptance would silently make the self-filter
dead code and re-introduce the member_added echo to the acceptee.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(auth): use AcceptedInvitation type in magic-link verify
Tighten the acceptedInvitations local type binding from a structural
{driveId, driveName, role: string} to the exported AcceptedInvitation
type so the role field stays narrowed to 'OWNER' | 'ADMIN' | 'MEMBER'
instead of widening to string.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test(socket-utils): cover broadcastDriveMemberEventToRecipients
Add unit tests for the new fan-out helper:
- multi-recipient broadcasts route to each user:{userId}:drives channel
- empty recipient list short-circuits before fetch
- absent INTERNAL_REALTIME_URL short-circuits before fetch
- partial fetch failures do not propagate (best-effort contract;
broadcast must never abort the calling operation, e.g. login)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(auth): replace inline maskEmail with utility in signup-passkey
Same class of issue as the google/one-tap regex maskEmail: the inline
form (email.substring(0, 3) + '***@' + domain) leaks domain characters
into the local segment for short addresses. For example
'a@example.com' → 'a@e***@example.com'. Use the shared maskEmail
utility (already imported) which always emits a clean
{firstTwoChars}***@{domain} form. Test assertion updated to the
canonical 2-char prefix.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2witstudios
added a commit
that referenced
this pull request
May 15, 2026
* fix(realtime): fan out member_removed to recipients Switch the DELETE handler from broadcastDriveMemberEvent (single-recipient, sent to the just-removed user) to broadcastDriveMemberEventToRecipients fanned out across getDriveRecipientUserIds(driveId). Other admins watching the members page now see the row disappear in realtime instead of having to refresh, matching the parity already in place for member_added. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(realtime): inspect Response.ok in fan-out tally broadcastDriveEvent and broadcastDriveMemberEventToRecipients used Promise.allSettled to track per-recipient success but only counted status === 'rejected'. fetch resolves on HTTP 4xx/5xx (it does not throw — only network errors throw), so silent broadcast failures were being logged as successes. Surface !response.ok inside the map callback so it shows up as a rejected settled-result and the failed/total counters in the warn log become accurate. Network errors continue to count as failures unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: remove stray ralph-loop.local.md The file was accidentally committed in PR #1236; *.local.md is agent-loop state that doesn't belong in source control. Add a recursive **/.claude/*.local.md gitignore rule so future stray files in nested .claude/ directories (e.g. apps/web/.claude/) are ignored as well — the existing top-level .claude/*.local.md rule only matched the repo root. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(realtime): make post-commit broadcast non-fatal in member-removed Codex review flagged that getDriveRecipientUserIds runs after the membership-delete transaction but inside the main request try/catch, so a transient DB blip during recipient lookup would surface as a 500 even though the member was already removed — a false failure signal that can trigger client retries against already-applied state. Wrap the recipient lookup + broadcast in an inner try/catch so any post-commit failure is logged as best-effort and the handler still returns 200. Mirrors the pattern already used in post-login-pending-acceptance for member_added. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(realtime): make post-commit kicks non-fatal in member-removed Apply the same best-effort contract to the page-room kick block as the broadcast block above. The pre-existing db.select for drivePages also runs after the membership-delete commit; without a wrap, a transient DB blip there would mask a successful removal as a 500 — the very same bug shape Codex flagged for the broadcast. Now both post-commit side-effect blocks (broadcast + kicks) log on failure and return 200, so the post-commit semantics of this handler are uniform. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(tests): align createDriveMemberEventPayload mock with production shape CodeRabbit nitpick: the mocked factory returned { event, data } but the real helper from socket-utils.ts returns { operation, ...options }. The existing assertions rely only on call args (not return shape), so this is purely a fidelity fix — no test behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
Adds the post-login pending invitation acceptance hook — Epic 3 of the
pu/invitesredo. After Epic 1 hardened authz to filter onacceptedAt IS NOT NULL, any pendingdrive_membersrow is invisible to drive queries. This PR wires every successful login to flip pending → accepted so an invitee actually reaches the drive they were invited to.driveInviteRepository) gainsfindPendingMembersForUser+ race-safeacceptPendingMember.acceptUserPendingInvitationsorchestrates accept + best-effort broadcast. Broadcast/recipient-resolution failures are caught and logged — they never abort login. (The original PR coupled broadcast errors to session revocation; review flagged that as a flaw and this PR fixes it.) Genuine acceptance-write errors do propagate so the caller can revoke the just-created session.getDriveRecipientUserIds()before fan-out so they don't receive their ownmember_addedecho (they appear in the recipient list becauseacceptedAtwas already written when recipients are queried).sessionService.revokeSession(sessionToken, …)— arevokeAllUserSessionscall would have logged the user out on every device.?inviteDriveIdredirect hint via the new sharedresolvePostLoginRedirectPath()helper (single source of truth for desktop and web flows).trackAuthEventcalls in apple/callback, apple/native, google/native, and mobile/oauth/google/exchange now mask emails (same pattern google/callback already had); google/one-tap no longer firestrackAuthEvent('login')or resets rate-limit counters until after acceptance succeeds, so a failed acceptance no longer emits a misleading success-login event before the rollback path runs. The inline regex maskEmail in one-tap was replaced with the sharedmaskEmailutility (the regex would emit raw email for 1-character local parts).post-login-acceptance-coverage.test.ts) asserts every future session-creating route under/api/auth/**/route.tseither calls the helper or appears on a justified allow-list (currentlyws-token,device/refresh,mobile/refresh— these don't transition unauthenticated → authenticated).Why this epic ships now
Wave 2 of the redo. Wave 1 (#1233 repo seam, #1234 authz gate) merged. Wave 3 (Epic 4 email branch) creates pending rows that no login flow currently resolves — this hook is the prerequisite.
Out of scope (per Epic 3 brief)
POST /api/drives/[driveId]/membersroute (Epic 4)members/[userId]/resendroute (Epic 6)Test plan
pnpm --filter web typecheck— cleanpnpm --filter web lint— clean (only pre-existing warning in unrelated file)pnpm exec vitest run src/app/api/auth/+src/lib/auth/__tests__/post-login-pending-acceptance.test.ts+src/lib/repositories/— 939/939 passpnpm exec vitest run src/app/api/drives/— 549/549 pass (no regression in drive routes that share the websocket helper)🤖 Generated with Claude Code