Fix dashboard auth review issues#98
Merged
Merged
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR hardens the dashboard auth system end-to-end: DB-level constraints enforce case-insensitive email uniqueness, account composite uniqueness, and role allow-lists; a new ChangesAuth hardening, invite acceptance flow, and cockpit permission gating
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 6
🧹 Nitpick comments (3)
apps/worker/src/routes/api/v1/invites.test.ts (1)
185-189: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
arrayContainingfor the tag assertion.This only passes when
tagsis exactly a one-element array withinvite_delivery_idin that slot. If the sender later adds another tag, the delivery-correlation contract still holds but the test will fail.expect.arrayContaining([expect.objectContaining({ name: "invite_delivery_id" })])keeps the assertion focused on the real invariant.Suggested test tweak
expect(state.resendSend).toHaveBeenLastCalledWith( expect.objectContaining({ - tags: [expect.objectContaining({ name: "invite_delivery_id" })], + tags: expect.arrayContaining([ + expect.objectContaining({ name: "invite_delivery_id" }), + ]), }), );🤖 Prompt for 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. In `@apps/worker/src/routes/api/v1/invites.test.ts` around lines 185 - 189, The assertion in the resend invite test is too strict because it requires tags to be exactly a single-item array; update the `state.resendSend` expectation in `invites.test.ts` to use `expect.arrayContaining(...)` for the `tags` field so it only verifies that a tag with `name: "invite_delivery_id"` is present. Keep the rest of the `toHaveBeenLastCalledWith` check unchanged and use the existing `expect.objectContaining` around the send payload.apps/dashboard/app/(cockpit)/cockpit-shell.tsx (1)
72-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the “More” ids in one place.
These ids are also encoded in
apps/dashboard/components/cockpit/mobile/more-sheet.tsx, soBottomTabBar.moreActivecan drift from the sheet contents. Reuse a shared constant/helper for both paths.🤖 Prompt for 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. In `@apps/dashboard/app/`(cockpit)/cockpit-shell.tsx around lines 72 - 74, The “More” tab ids are duplicated between cockpit-shell.tsx and more-sheet.tsx, which can cause BottomTabBar.moreActive to drift from the sheet contents. Move the ids into a shared constant or helper and have both cockpitNavItems() filtering and the mobile more-sheet.tsx contents read from that single source. Use the shared symbol to keep the active-state logic and sheet entries in sync.apps/dashboard/lib/auth/session.ts (1)
22-31: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winValidate the session payload at the boundary.
Line 31 trusts arbitrary JSON with a type cast, so a worker/dashboard contract drift becomes a silent auth-UI regression instead of a controlled failure. Parse the response and verify
role/canManageUsersbefore returning it.🤖 Prompt for 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. In `@apps/dashboard/lib/auth/session.ts` around lines 22 - 31, requireSession currently trusts the /api/v1/session JSON via a type cast, so the session payload should be validated before returning it. Update the requireSession function to parse the fetchAuthWorker response and verify the expected DashboardSession shape, especially role and canManageUsers, and redirect to /login if the payload is missing or malformed. Use the existing requireSession symbol as the boundary check point so contract drift fails closed instead of propagating invalid session data.
🤖 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 `@apps/worker/drizzle/0007_auth_invariants.sql`:
- Around line 2-7: The auth invariants migration currently adds unique indexes
and role checks directly, so it can fail on existing bad data. Update the
migration around the user/account/invitation/member DDL to include a preflight
cleanup or validation step before enforcing these constraints, handling
duplicate lowercased user emails, duplicate account provider/account pairs, and
invalid invitation/member roles. Use the existing migration block in the auth
invariants SQL to locate where to insert the cleanup so the constraints can be
applied safely.
In `@apps/worker/scripts/seed-auth-user.ts`:
- Around line 26-45: The missing-env check in seed-auth-user.ts only treats
falsy values as unset, so whitespace-only strings can still pass and later fail
in the auth setup flow. Update the validation in the seed-auth-user script to
trim each required env value before evaluating it, and use the trimmed result
consistently when building missingRequiredEnv and when continuing into the
bootstrap logic. Keep the existing control flow around allowLocalSkip and the
throw path, but ensure DASHBOARD_AUTH_EMAIL, DASHBOARD_AUTH_PASSWORD,
BETTER_AUTH_URL, DATABASE_URL, and BETTER_AUTH_SECRET are considered missing if
they are blank after trimming.
In `@apps/worker/src/lib/auth/invite-acceptance.ts`:
- Around line 53-58: The invite acceptance state is hardcoding the role to
"member", which loses the real privilege from `invitation.role`. Update
`DashboardInviteAcceptanceState` and the related invite flow in
`acceptInvitation`/dashboard form wiring to carry through the actual invite role
(`owner | admin | member`) from `invite.role` instead of narrowing it. Make sure
the preview UI and downstream state consumers read that propagated role so
admin/owner invites are labeled correctly.
In `@apps/worker/src/routes/api/dashboard-auth/invite/accept.post.ts`:
- Around line 10-14: The accept.post handler is swallowing all readBody()
failures by defaulting to an empty object, which turns malformed JSON and other
body-read errors into misleading missing-field responses. Update the body
parsing in this route to either let the readBody() error propagate or catch it
and rethrow a clear invalid-body error, and keep the later
inviteId/name/password validation separate so the existing field checks only run
when the request body was parsed successfully.
In `@apps/worker/src/routes/api/dashboard-auth/sso/consume.post.ts`:
- Around line 8-15: The SSO consume handler is trimming the token before
validating its type, which can throw on non-string payloads and bypass the
intended 400 response. Update the token handling in the consume post route to
first confirm `body.token` is a string in `consumeDashboardSsoHandoff`’s caller
path, then trim it and reject missing/empty values with the existing
`createError` flow so bad requests stay as 400s.
In `@docs/superpowers/specs/2026-06-22-live-polling-toggle-design.md`:
- Around line 118-139: The cockpit context names in this spec don’t match the
real contract, so update the wording to use the actual symbols exposed by the
shared state. Replace references to live and onToggleLive with livePolling and
toggleLive, and keep nextRefreshAt where needed so the desktop top bar and
mobile header implementation aligns with the existing cockpit context API.
---
Nitpick comments:
In `@apps/dashboard/app/`(cockpit)/cockpit-shell.tsx:
- Around line 72-74: The “More” tab ids are duplicated between cockpit-shell.tsx
and more-sheet.tsx, which can cause BottomTabBar.moreActive to drift from the
sheet contents. Move the ids into a shared constant or helper and have both
cockpitNavItems() filtering and the mobile more-sheet.tsx contents read from
that single source. Use the shared symbol to keep the active-state logic and
sheet entries in sync.
In `@apps/dashboard/lib/auth/session.ts`:
- Around line 22-31: requireSession currently trusts the /api/v1/session JSON
via a type cast, so the session payload should be validated before returning it.
Update the requireSession function to parse the fetchAuthWorker response and
verify the expected DashboardSession shape, especially role and canManageUsers,
and redirect to /login if the payload is missing or malformed. Use the existing
requireSession symbol as the boundary check point so contract drift fails closed
instead of propagating invalid session data.
In `@apps/worker/src/routes/api/v1/invites.test.ts`:
- Around line 185-189: The assertion in the resend invite test is too strict
because it requires tags to be exactly a single-item array; update the
`state.resendSend` expectation in `invites.test.ts` to use
`expect.arrayContaining(...)` for the `tags` field so it only verifies that a
tag with `name: "invite_delivery_id"` is present. Keep the rest of the
`toHaveBeenLastCalledWith` check unchanged and use the existing
`expect.objectContaining` around the send payload.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2015e3a9-b119-4eb6-b70e-3a11b71a347f
📒 Files selected for processing (55)
.claude/learnings.mdREADME.mdapps/dashboard/app/(cockpit)/cockpit-shell.tsxapps/dashboard/app/(cockpit)/layout.tsxapps/dashboard/app/api/auth/invite/[inviteId]/route.tsapps/dashboard/app/api/auth/sso/complete/route.tsapps/dashboard/app/invite/accept/invite-accept-form.tsxapps/dashboard/app/invite/accept/page.tsxapps/dashboard/app/login/page.tsxapps/dashboard/app/reset-password/page.tsxapps/dashboard/components/auth/auth-shell.tsxapps/dashboard/components/cockpit/chrome.tsxapps/dashboard/components/cockpit/mobile/more-sheet.tsxapps/dashboard/components/cockpit/screens/trace.tsxapps/dashboard/components/cockpit/screens/users.tsxapps/dashboard/lib/api/proxy.tsapps/dashboard/lib/api/server.tsapps/dashboard/lib/auth/session.tsapps/dashboard/lib/auth/worker.tsapps/worker/drizzle/0004_cuddly_bishop.sqlapps/worker/drizzle/0005_quiet_fantastic_four.sqlapps/worker/drizzle/0007_auth_invariants.sqlapps/worker/drizzle/meta/0004_snapshot.jsonapps/worker/drizzle/meta/0005_snapshot.jsonapps/worker/drizzle/meta/0006_snapshot.jsonapps/worker/drizzle/meta/0007_snapshot.jsonapps/worker/drizzle/meta/_journal.jsonapps/worker/scripts/seed-auth-user.tsapps/worker/src/auth.tsapps/worker/src/db/auth-schema.tsapps/worker/src/lib/auth/invite-acceptance.test.tsapps/worker/src/lib/auth/invite-acceptance.tsapps/worker/src/lib/auth/invites.test.tsapps/worker/src/lib/auth/invites.tsapps/worker/src/lib/auth/users-read.tsapps/worker/src/lib/email/invite-delivery.tsapps/worker/src/lib/email/templates.tsapps/worker/src/routes/api/auth/auth-route.test.tsapps/worker/src/routes/api/dashboard-auth/invite/[inviteId].get.tsapps/worker/src/routes/api/dashboard-auth/invite/accept.post.tsapps/worker/src/routes/api/dashboard-auth/sso/consume.post.tsapps/worker/src/routes/api/dashboard-auth/sso/start.get.test.tsapps/worker/src/routes/api/dashboard-auth/sso/start.get.tsapps/worker/src/routes/api/v1/invites.get.tsapps/worker/src/routes/api/v1/invites.post.tsapps/worker/src/routes/api/v1/invites.test.tsapps/worker/src/routes/api/v1/invites/[inviteId]/cancel.post.tsapps/worker/src/routes/api/v1/invites/[inviteId]/resend.post.tsapps/worker/src/routes/api/v1/session.get.tsapps/worker/src/routes/api/v1/users.get.tsapps/worker/src/routes/api/v1/users.test.tsapps/worker/src/routes/api/v1/users/[userId]/role.patch.tsapps/worker/src/routes/webhooks/resend.post.test.tsdocs/superpowers/plans/2026-06-22-live-polling-toggle.mddocs/superpowers/specs/2026-06-22-live-polling-toggle-design.md
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/dashboard/lib/auth/worker.ts (1)
34-37: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep missing-
WORKER_BASE_URLon the existing null-return path.Line 34 now throws before
fetchAuthWorker()reaches itstry/catch, so a missingWORKER_BASE_URLturns these auth routes into uncaught 500s instead of the intendedauthWorkerUnavailable(...)502 flow.Proposed fix
- const url = workerUrl(path); - try { - return await fetch(url, { + return await fetch(workerUrl(path), {🤖 Prompt for 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. In `@apps/dashboard/lib/auth/worker.ts` around lines 34 - 37, The missing-WORKER_BASE_URL check in fetchAuthWorker() is now happening before the existing try/catch, which bypasses the intended null-return handling and turns auth routes into uncaught 500s. Move the workerUrl(path) resolution back into the guarded path or otherwise catch the missing-base-url case inside fetchAuthWorker() so it returns null and lets authWorkerUnavailable(...) preserve the 502 flow.apps/worker/src/lib/auth/invite-acceptance.ts (1)
253-274: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winHonor the invite role when the user is already in the org.
ensureInviteMembership()still returns early on any existing membership, so an existingmemberwho accepts anadmin/ownerinvite keeps the old role even though the invite is marked accepted. That makes the new dynamic-role flow incorrect for already-enrolled users. This helper needs to reconcilemember.rolewithinput.rolehere, or the flow should reject role-changing invites before acceptance.🤖 Prompt for 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. In `@apps/worker/src/lib/auth/invite-acceptance.ts` around lines 253 - 274, ensureInviteMembership() is skipping role reconciliation for users who already exist in memberTable, so accepting an invite with a higher role leaves the old membership role unchanged. Update the existing-member path in ensureInviteMembership() to either update memberTable.role to input.role when the current role differs, or explicitly reject role-changing invites before acceptance; keep the insert path unchanged for new members.
🤖 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 `@apps/worker/src/lib/auth/seed-auth-env.ts`:
- Around line 9-17: `DASHBOARD_ORIGIN` is incorrectly listed in `OPTIONAL_ENV`,
which lets `seed-auth-user.ts` skip it even though the worker requires it.
Update `seed-auth-env.ts` so `DASHBOARD_ORIGIN` is treated as required, keeping
it out of the optional list used by the auth seeding flow and aligned with the
validation in `apps/worker/env.ts`.
In `@apps/worker/src/routes/api/dashboard-auth/invite/accept.post.ts`:
- Around line 10-21: The request parsing in accept.post.ts only checks JSON
syntax and then trusts the inferred body shape, so malformed field types can
still reach acceptDashboardInvite() and crash on .trim(). Update the body
validation after readBody(event) to reject non-object payloads, require inviteId
and password to be strings, and allow name only when it is absent or a string;
keep the existing Invalid request body handling for any type mismatch before
calling acceptDashboardInvite().
---
Outside diff comments:
In `@apps/dashboard/lib/auth/worker.ts`:
- Around line 34-37: The missing-WORKER_BASE_URL check in fetchAuthWorker() is
now happening before the existing try/catch, which bypasses the intended
null-return handling and turns auth routes into uncaught 500s. Move the
workerUrl(path) resolution back into the guarded path or otherwise catch the
missing-base-url case inside fetchAuthWorker() so it returns null and lets
authWorkerUnavailable(...) preserve the 502 flow.
In `@apps/worker/src/lib/auth/invite-acceptance.ts`:
- Around line 253-274: ensureInviteMembership() is skipping role reconciliation
for users who already exist in memberTable, so accepting an invite with a higher
role leaves the old membership role unchanged. Update the existing-member path
in ensureInviteMembership() to either update memberTable.role to input.role when
the current role differs, or explicitly reject role-changing invites before
acceptance; keep the insert path unchanged for new members.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7817df3-0b2d-4808-ac7d-c965d6a0c1c0
📒 Files selected for processing (28)
.claude/learnings.mdapps/dashboard/app/(cockpit)/cockpit-shell.tsxapps/dashboard/app/api/auth/forgot-password/route.tsapps/dashboard/app/api/auth/login/route.tsapps/dashboard/app/api/auth/logout/route.tsapps/dashboard/app/api/auth/reset-password/route.tsapps/dashboard/app/api/auth/sso/status/route.tsapps/dashboard/app/invite/accept/invite-accept-form.tsxapps/dashboard/app/login/page.tsxapps/dashboard/components/cockpit/chrome.tsxapps/dashboard/components/cockpit/mobile/more-sheet.tsxapps/dashboard/lib/auth/session.tsapps/dashboard/lib/auth/worker.tsapps/worker/drizzle/0007_auth_invariants.sqlapps/worker/scripts/seed-auth-user.tsapps/worker/src/db/auth-schema.test.tsapps/worker/src/lib/auth/invite-acceptance.test.tsapps/worker/src/lib/auth/invite-acceptance.tsapps/worker/src/lib/auth/seed-auth-env.test.tsapps/worker/src/lib/auth/seed-auth-env.tsapps/worker/src/routes/api/dashboard-auth/invite/accept.post.test.tsapps/worker/src/routes/api/dashboard-auth/invite/accept.post.tsapps/worker/src/routes/api/dashboard-auth/sso/consume.post.test.tsapps/worker/src/routes/api/dashboard-auth/sso/consume.post.tsapps/worker/src/routes/api/dashboard-auth/sso/status.get.test.tsapps/worker/src/routes/api/dashboard-auth/sso/status.get.tsapps/worker/src/routes/api/v1/invites.test.tsdocs/superpowers/specs/2026-06-22-live-polling-toggle-design.md
✅ Files skipped from review due to trivial changes (2)
- apps/worker/src/lib/auth/seed-auth-env.test.ts
- .claude/learnings.md
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/worker/src/routes/api/dashboard-auth/sso/consume.post.ts
- apps/worker/src/routes/api/v1/invites.test.ts
- apps/dashboard/components/cockpit/chrome.tsx
- apps/worker/scripts/seed-auth-user.ts
- apps/dashboard/lib/auth/session.ts
- apps/dashboard/app/(cockpit)/cockpit-shell.tsx
- docs/superpowers/specs/2026-06-22-live-polling-toggle-design.md
- apps/dashboard/app/invite/accept/invite-accept-form.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/dashboard/lib/auth/worker.test.ts (1)
4-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise
fetchAuthWorker()here, or rename the test.The test name claims wrapper coverage, but it bypasses
fetchAuthWorker()and only exercisesfetchWorker(). That won’t catch regressions in the wrapper’s env handoff or any wrapper-specific request shaping.🤖 Prompt for 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. In `@apps/dashboard/lib/auth/worker.test.ts` around lines 4 - 6, The test name and implementation are mismatched: it claims to cover fetchAuthWorker(), but it only calls fetchWorker() directly. Update the test in worker.test.ts to exercise fetchAuthWorker() so it verifies the wrapper’s env handoff and request shaping, or rename the test to match the lower-level fetchWorker() behavior if that’s the intended scope. Use the fetchAuthWorker and fetchWorker symbols to align the test with the actual code path.
🤖 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.
Nitpick comments:
In `@apps/dashboard/lib/auth/worker.test.ts`:
- Around line 4-6: The test name and implementation are mismatched: it claims to
cover fetchAuthWorker(), but it only calls fetchWorker() directly. Update the
test in worker.test.ts to exercise fetchAuthWorker() so it verifies the
wrapper’s env handoff and request shaping, or rename the test to match the
lower-level fetchWorker() behavior if that’s the intended scope. Use the
fetchAuthWorker and fetchWorker symbols to align the test with the actual code
path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 644ae343-6bb7-4d87-b401-dfa72120af40
📒 Files selected for processing (9)
apps/dashboard/lib/auth/worker-core.tsapps/dashboard/lib/auth/worker.test.tsapps/dashboard/lib/auth/worker.tsapps/worker/src/lib/auth/invite-acceptance.test.tsapps/worker/src/lib/auth/invite-acceptance.tsapps/worker/src/lib/auth/seed-auth-env.test.tsapps/worker/src/lib/auth/seed-auth-env.tsapps/worker/src/routes/api/dashboard-auth/invite/accept.post.test.tsapps/worker/src/routes/api/dashboard-auth/invite/accept.post.ts
✅ Files skipped from review due to trivial changes (1)
- apps/dashboard/lib/auth/worker-core.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/worker/src/lib/auth/seed-auth-env.test.ts
- apps/worker/src/lib/auth/seed-auth-env.ts
- apps/worker/src/lib/auth/invite-acceptance.test.ts
- apps/worker/src/lib/auth/invite-acceptance.ts
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
Validation
Summary by CodeRabbit