fix(authz): close pending-state authority bypass surfaces

[2witstudios]

Summary

Closes the three corroborated criticals tier findings from the 4-agent review battery on the drive invite-by-email epic (PRs #1233, #1234, #1236, #1239, #1243, #1245). All three findings share the same mental model — a pending or never-authenticated identity must not exercise authority — so they ship in one bundled PR.

Slice 1.1 — Close userId-path emailVerified bypass

Review C1 (/tmp/review-1-end-to-end-report.md finding 3 + /tmp/review-2-zero-trust-report.md §1):

• apps/web/src/app/api/users/search/route.ts:63 returned temp users (created by a prior pending magic-link invite) on exact-email match. An admin could pick the temp user and the userId branch of /api/drives/[driveId]/members/invite/route.ts:118-126,161-228 called createAcceptedMemberWithPermissions — admitting a never-authenticated account.

Fix:

• users/search now filters isNotNull(users.emailVerified) on both the profile-search join and the email-exact-match query, with a JS-side defense-in-depth drop.
• handleUserIdPath looks up target verification status before any membership write. Suspended → 403, missing → 404, unverified → routes through handleEmailPath (issues fresh magic-link, writes acceptedAt: null, sends invitation email). Verified path unchanged.
• New repository method findUserVerificationStatusById.

Slice 1.2 — Close gate-coverage allow-list escalations

Review C2 (/tmp/review-2-zero-trust-report.md §1, §2 Invariant 1):

• apps/web/src/app/api/pages/bulk-move/route.ts:64-71 and /bulk-copy/route.ts:65-72 (HIGH) — pending ADMIN could write into target drive
• apps/web/src/app/api/pages/tree/route.ts:55-62 (HIGH) — pending member could read full page tree
• apps/web/src/app/api/account/handle-drive/route.ts:56-62 (borderline-CRIT) — drive ownership transfer to pending admin
• Four lib-level call sites the regression test couldn't see:
  • apps/web/src/lib/memory/discovery-service.ts:111-114, 161-164
  • apps/web/src/lib/ai/tools/drive-tools.ts:69-72
  • apps/web/src/lib/ai/tools/channel-tools.ts:194-197
  • apps/web/src/lib/ai/tools/activity-tools.ts:357-362

Fix: all eight read sites gated on isNotNull(driveMembers.acceptedAt). Gate-coverage regression test extended to scan apps/web/src/lib/**. Three previously-allow-listed route exemptions removed; remaining allow-list entries point to follow-up #4. The repository seam carries a single allow-list entry that documents why findActivePendingMemberByEmail intentionally inverts the gate (it surfaces pending rows for the pending-list UI).

Slice 1.3 — Magic-link token isolation

Review H1 (/tmp/review-1-end-to-end-report.md finding 1):

• packages/lib/src/auth/magic-link-service.ts:160-168 ran a blind type-wide delete of unused magic-link tokens before each insert. That broke three flows:
  • 7-day pending invitation token nuked when user requested 5-min sign-in
  • drive-A invitation nuked when invitation issued for drive-B same email
  • Resend on same drive — original email's link dies first time admin clicks

Fix: stop the pre-insert deletion entirely. Tokens have a TTL via expiresAt and a unique tokenHash; verifyMagicLinkToken refuses expired/used rows. Stale unused tokens age out naturally.

Adversarial tests

Each slice carries an adversarial test that cites the specific exploit path in the test name:

• users/search: temp-user-via-search re-invite path (verified by spy on isNotNull(users.emailVerified))
• invite/route: temp-user-via-userId-path adversarial path (asserts createAcceptedMemberWithPermissions not called for unverified target)
• bulk-move, bulk-copy, pages/tree, account/handle-drive: pending-admin-can-X path (each pins isNotNull(driveMembers.acceptedAt) on the WHERE clause via the operators-mock spy, so a future drift back to the role-only predicate trips immediately)
• magic-link-service: concurrent-invitations-from-two-drives path (two tokens issued, both remain clickable, both verify)

Out of scope (per follow-up plan)

This PR is one of five slated follow-ups. Not addressed here:

• Email canonicalization (lowercasing, citext, OAuth callbacks) — Follow-up 3
• Redirect-to-invited-drive UX (?inviteDriveId append) — Follow-up 4
• Audit gaps (deny / rate-limit / per-row acceptance) — Follow-up 4
• Trashed-drive invite guard, self-invite race, localhost fallback — Follow-up 4
• member_removed fan-out, Promise.allSettled Response.ok check, ralph-loop cleanup — Follow-up 2

The four Followup allow-list entries that remain reference Follow-up #4 explicitly; the rest are closed.

Test plan

• pnpm lint green
• pnpm typecheck green (via pnpm build)
• All 157 lib unit test files pass (3952 tests)
• All 226 tests across the 10 touched apps/web test files pass
• 26 pre-existing integration-test failures (gift-subscription / admin-role-version) confirmed identical on master — not introduced by this PR
• Diff: 23 files, +469/-74 (well under 800 LOC ceiling)

🤖 Generated with Claude Code

[coderabbitai]

Warning

Rate limit exceeded

@2witstudios has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 27 minutes and 44 seconds before requesting another review.

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5b87ed43-fd31-444e-aece-3c0d0466739f

📥 Commits

Reviewing files that changed from the base of the PR and between e3ed55c and 2fe779f.

📒 Files selected for processing (23)

• apps/web/src/app/api/__tests__/drive-member-gate-coverage.test.ts
• apps/web/src/app/api/account/handle-drive/__tests__/route.test.ts
• apps/web/src/app/api/account/handle-drive/route.ts
• apps/web/src/app/api/drives/[driveId]/members/invite/__tests__/route.test.ts
• apps/web/src/app/api/drives/[driveId]/members/invite/route.ts
• apps/web/src/app/api/pages/bulk-copy/__tests__/route.test.ts
• apps/web/src/app/api/pages/bulk-copy/route.ts
• apps/web/src/app/api/pages/bulk-move/__tests__/route.test.ts
• apps/web/src/app/api/pages/bulk-move/route.ts
• apps/web/src/app/api/pages/tree/__tests__/route.test.ts
• apps/web/src/app/api/pages/tree/route.ts
• apps/web/src/app/api/users/search/__tests__/route.test.ts
• apps/web/src/app/api/users/search/route.ts
• apps/web/src/lib/ai/tools/__tests__/channel-tools.test.ts
• apps/web/src/lib/ai/tools/activity-tools.ts
• apps/web/src/lib/ai/tools/channel-tools.ts
• apps/web/src/lib/ai/tools/drive-tools.ts
• apps/web/src/lib/memory/__tests__/discovery-service.test.ts
• apps/web/src/lib/memory/discovery-service.ts
• apps/web/src/lib/repositories/drive-invite-repository.ts
• packages/lib/src/auth/__tests__/magic-link-service.test.ts
• packages/lib/src/auth/magic-link-service.test.ts
• packages/lib/src/auth/magic-link-service.ts

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch pu/followup-1-authz-completion

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[2witstudios]

Self-review against rubric Appendix

Per .pu/templates/rubric-review.md Appendix "Quick self-review scoring (for agents)". Scoring each touched test file 0–2 on each axis.

apps/web/src/app/api/users/search/__tests__/route.test.ts

Axis	Score	Note
Contract stability	2	New tests are predicate-shape ("WHERE composes isNotNull(users.emailVerified)") and outcome-based ("temp user not in results"). A future refactor that keeps the gate intact will pass.
Mock quality	1	The file's existing setup uses ORM chain mocks (pre-existing convention). My adds extend that convention but also add isNotNull to the operators mock so the test asserts the gate via the spy, not the chain. Score 1 because I did not refactor the chain mocks themselves.
Assertion meaning	2	New tests assert (a) the operator spy was called with the right column and (b) the response body filters the row. Both observable.
Flake risk	2	No timers, no async randomness.
Spec clarity	2	Adversarial test names cite "Review C1 / temp-user-via-search re-invite path" inline.

apps/web/src/app/api/drives/[driveId]/members/invite/__tests__/route.test.ts

Axis	Score	Note
Contract stability	2	New tests added to the userId payload describe block; assert outcomes ("status 200, kind: invited", "createAcceptedMemberWithPermissions not called"), not call ladders.
Mock quality	2	Mocks the driveInviteRepository seam — added findUserVerificationStatusById to the existing mock surface. No ORM chains added.
Assertion meaning	2	Assertions verify the response body, the magic-link/email side effects, and the negative case (auto-accept did NOT happen).
Flake risk	2	No timers, deterministic.
Spec clarity	2	Three named adversarial paths: temp-user-via-userId, suspended-via-userId, missing-user.

apps/web/src/app/api/__tests__/drive-member-gate-coverage.test.ts

Axis	Score	Note
Contract stability	2	The lib sweep is intentionally regex-based on file content; refactor-resistant.
Mock quality	2	No mocks — pure file scan.
Assertion meaning	2	Asserts the violations list is empty, with a printed list of offending paths on failure.
Flake risk	2	Pure FS read.
Spec clarity	2	Each allow-list entry now references either follow-up #4 or the canonical-seam justification.

apps/web/src/app/api/pages/{bulk-move,bulk-copy,tree}/__tests__/route.test.ts and account/handle-drive/__tests__/route.test.ts

Axis	Score	Note
Contract stability	2	Each adversarial test pins isNotNull(driveMembers.acceptedAt) via spy + asserts 403 / 400 outcome. Refactor that keeps the gate will pass.
Mock quality	1	These files already used ORM chain / db.query.X.findFirst mocks — pre-existing convention. I added isNotNull to the operators mock and acceptedAt to the schema mock; did not refactor the chains.
Assertion meaning	2	Outcome (status code) + predicate-shape (operator spy).
Flake risk	2	Deterministic.
Spec clarity	2	Each test name cites the specific Review-C2 exploit path.

packages/lib/src/auth/__tests__/magic-link-service.test.ts

Axis	Score	Note
Contract stability	2	New unit test asserts db.delete not called — exactly the contract the fix establishes.
Mock quality	1	Existing chain mocks (pre-existing convention for this file). My adds extend without growing the chain surface.
Assertion meaning	2	Asserts db.delete mock not called — observable, not procedural.
Flake risk	2	Pure unit test.
Spec clarity	2	Cites Review H1 and the three adversarial flows that the fix unblocks.

packages/lib/src/auth/magic-link-service.test.ts (integration)

Axis	Score	Note
Contract stability	2	The "preserve unused tokens" test exercises the multi-token scenario end-to-end.
Mock quality	2	Real DB; no mocks at all.
Assertion meaning	2	Asserts both tokens persist and both verify successfully.
Flake risk	2	No timers; the only async ops are awaited DB writes.
Spec clarity	2	Test name + comment cite Review H1 and the three downstream flows.

Aggregate

• 29 of 30 axis-scores at 2; one slice axis at 1 (mock quality, where I extended pre-existing chain conventions instead of refactoring them).
• All adversarial tests cite the Review finding and the named exploit path.
• §8 (security/adversarial coverage) explicitly satisfied — every closed gap has a paired adversarial test.

Refactoring the existing chain-mock conventions across the four touched route tests is out of scope for this PR (would balloon diff well past the 800-LOC ceiling and revisit Epic 1 design). I'd rather track that as a separate follow-up if the rubric reviewers decide it's worth doing now vs. with the next route-test author who needs to mock new query shapes.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e5ffd1379b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".