Repository navigation
Take identity from the verified session in API routes - #14
Conversation
Route handlers read the caller's user and tenant from request headers, which the middleware set on the response rather than the request. Identity now comes from Clerk's auth() and the user's own users row: - middleware forwards headers on the request and drops any client x-user-id - validateTenantContext uses auth() and requires authentication by default - new lib/server-auth requireMember() for admin and tenant checks - tenant PUT/DELETE, persona updates, invitations and admin routes require a signed-in admin of that tenant; tenant PUT accepts presentation fields only - onboarding no longer joins an existing tenant or takes a role from the body - registration ignores tenant and access level from the body - access-level lookup and the queue tenant fail closed - the worker refuses requests when CRON_SECRET is unset - document text in the source viewer renders as text, not HTML - error responses no longer include internal messages Adds vitest with regression tests for each of these (npm test).
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fd3a91c64
ℹ️ 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".
|
|
||
| /** Admin is role 'admin' or access level 1, the convention used across the app. */ | ||
| export function isTenantAdmin(member: Pick<VerifiedMember, 'role' | 'accessLevel'>): boolean { | ||
| return member.role === 'admin' || member.accessLevel === 1; |
There was a problem hiding this comment.
Use && here rather than ||. The replaced checks required both fields, but the invitation APIs validate role and access_level independently, so a tenant can contain a role: 'user', access_level: 1 (or role: 'admin', access_level: 2) row. Under this predicate either mismatched member now passes every requireMember({ admin: true }) gate, including tenant deletion and invitation management, despite not satisfying the prior admin invariant.
Useful? React with 👍 / 👎.
From code review of the previous commit. Each item below now has a test
that fails on the code before this commit (10 new; 29 in total).
Still open to anyone, with no session check:
- POST /api/tenant/[tenant]/settings rewrote any tenant's display name,
contact email and AI switch. Now admins of that tenant only; GET needs
a member, since settings include the contact email.
- /api/notifications read, created and marked read the notifications of
any tenantId in the query or body. Tenant now comes from the session;
members act on their own, admins on the tenant's; creating is admin-only.
- /api/auth/verify-admin answered for any {email, tenantId} and accepted
any string as the bearer token: the admin lookup the previous commit
closed in /api/admin/verify. Now answers only for the signed-in caller.
Reachable by any signed-up user, since creating a workspace makes you its
admin:
- /api/rag-metrics DELETE cleared every tenant's metrics, and the recent
and report views returned other tenants' query text and top tenants.
Tenant admins now get their own tenant's view; the platform-wide ones
are refused until there is an operator role to gate them on.
Tightened:
- isTenantAdmin required role admin OR access level 1. Every check it
replaced required both; it does again.
- Onboarding matched membership on emailAddresses[0], which need be
neither primary nor verified. It now uses the verified primary address
and refuses an unverified one before any lookup.
- getVerifiedMember and validateAuth logged nothing when the users lookup
failed, so a database error read as "signed out". Both still fail
closed, and now log.
- The cron secret is compared in constant time.
.github/workflows/test.yml runs npm test on every pull request and push
to main. The suite had no runner, so nothing would have noticed a
regression. None of the four routes above has a caller in the app.
API routes took the caller's user and tenant from request headers. This moves identity to Clerk's verified session and the user's own
usersrow, and requires a signed-in admin of the tenant wherever tenant-level data changes.Changes
middleware.ts: forward headers on the request (NextResponse.next({ request })), drop any clientx-user-id, and stop echoing tenant/user ids on responses. Logic lives inlib/forwarded-headers.ts.lib/api-tenant-validation.ts: identity fromauth();requireAuthnow defaults totrue.lib/server-auth.ts(new):getVerifiedMember(),requireMember({ admin, tenantId }),isTenantAdmin().name,industry,logo_url,theme,settingsonly.getUserAccessLeveland the queue's tenant lookup fail closed. The queue worker refuses requests whenCRON_SECRETis unset.lib/highlight.tsx), not HTML.Tests
Adds vitest (
npm test) with 19 regression tests intests/security/. Against the previous code, 10 of them fail: every route-level case. The other 9 cover the new helpers.Verified locally
npm test: 19/19 pass.tsc --noEmit: no new errors (116 existing errors, unchanged).next buildsucceeds.ESLint has no config in this repo, so lint was not run.
To check on the preview before merging
Follow-ups are not in this PR: dependency upgrades (
next,@clerk/nextjs), Stripe webhook status handling, and Supabase function and RLS review.