feat: add access control to project #2687
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Warning Rate limit exceeded@Kitenite has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 5 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (3)
WalkthroughImplements returnUrl-based auth plumbing (persisted with localforage), adds project- and invitation-level access gating, revamps invitation APIs and email/template flows (including constructInvitationLink), updates DB invitation indexing and relations, migrates multiple localStorage usages to localforage, and adjusts related UI and routing constants. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Client as Next.js Client
participant Supabase
participant Store as localforage
participant Router
User->>Client: Visit protected page (project or invitation)
Client->>Supabase: supabase.auth.getSession()
alt No session
Client->>Store: setItem(RETURN_URL, currentPath)
Client->>Router: push(Routes.LOGIN + ?returnUrl)
else Session exists
Client-->>User: Render protected content
end
sequenceDiagram
participant User
participant Page as Invitation Page
participant API as tRPC Invitation Router
participant Email as Email Helper
User->>Page: Click "Accept Invitation"
Page->>API: acceptInvitation({ id, token })
alt Valid & not expired
API-->>Page: success (projectId?)
Page->>Router: navigate(project or projects)
else Error
API-->>Page: Error (not found / email mismatch / expired)
Page-->>User: Show error UI
end
sequenceDiagram
participant Layout as Project [id] Layout
participant API as tRPC Project.hasAccess
participant Child as Page Children
Layout->>API: hasAccess({ projectId })
alt Access granted
API-->>Layout: true
Layout-->>Child: Render children
else Access denied
API-->>Layout: false
Layout-->>Layout: Render NoAccess fallback
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
apps/web/client/src/app/project/[id]/layout.tsx (5)
6-7: Fix Next.js layout prop types:paramsisn’t a Promise; dropawaitNext.js
approuter passesparamsas a plain object. Typing it asPromise<...>and awaiting it works at runtime but is incorrect and weakens type inference.Apply this diff:
-export default async function Layout({ params, children }: Readonly<{ params: Promise<{ id: string }>, children: React.ReactNode }>) { - const projectId = (await params).id; +export default async function Layout({ params, children }: Readonly<{ params: { id: string }; children: React.ReactNode }>) { + const { id: projectId } = params;
25-27: Use a native anchor formailto:instead ofnext/link
Linkis intended for client-side navigation of internal routes. Formailto:(and other external protocols), use<a>.- <Link href={`mailto:${SUPPORT_EMAIL}`} className="text-primary underline"> - {SUPPORT_EMAIL} - </Link> + <a href={`mailto:${SUPPORT_EMAIL}`} className="text-primary underline"> + {SUPPORT_EMAIL} + </a>
1-6: Consider forcing dynamic rendering to avoid caching auth-gated contentThis layout gates children on a per-user check. If the
api.project.hasAccesscode path doesn’t touch cookies/headers in a way Next can detect automatically, the page might be statically cached. Consider marking this route segment dynamic.import { api } from "@/trpc/server"; import { Routes } from "@/utils/constants"; import { Icons } from "@onlook/ui/icons/index"; import Link from "next/link"; +export const dynamic = 'force-dynamic';If you already rely on middleware/session access that triggers dynamic rendering, this change may be unnecessary—please verify.
1-4: Optionally redirect unauthenticated users to LoginIf
protectedProcedurethrowsUNAUTHORIZED, this will bubble and render the error boundary. If you prefer a cleaner UX, catch and redirect to login; keep the NoAccess UI for authenticated users without membership.import { api } from "@/trpc/server"; import { Routes } from "@/utils/constants"; import { Icons } from "@onlook/ui/icons/index"; import Link from "next/link"; +import { redirect } from "next/navigation"; @@ - const hasAccess = await api.project.hasAccess({ projectId }); - if (!hasAccess) { - return <NoAccess />; - } + try { + const hasAccess = await api.project.hasAccess({ projectId }); + if (!hasAccess) { + return <NoAccess />; + } + } catch { + // Unauthenticated or server error: send to Login + redirect(Routes.LOGIN); + }Also applies to: 8-13
16-17: Avoid hardcoding support emailMove
[email protected]to a central config (env or constants) to avoid drift and ease changes.apps/web/client/src/server/api/routers/project/project.ts (2)
36-49: Validate UUID and simplify to an existence check on userProjectsCurrent implementation loads the project with a nested relation to check membership. You can:
- Validate the ID format early (DB uses UUID).
- Query
userProjectsdirectly for existence. This avoids an unnecessary join, reduces payload, and relies on the composite PK to guarantee consistency.- hasAccess: protectedProcedure - .input(z.object({ projectId: z.string() })) - .query(async ({ ctx, input }) => { - const user = ctx.user; - const project = await ctx.db.query.projects.findFirst({ - where: eq(projects.id, input.projectId), - with: { - userProjects: { - where: eq(userProjects.userId, user.id), - }, - }, - }); - return !!project && project.userProjects.length > 0; - }), + hasAccess: protectedProcedure + .input(z.object({ projectId: z.string().uuid() })) + .query(async ({ ctx, input }) => { + const userId = ctx.user.id; + const membership = await ctx.db.query.userProjects.findFirst({ + where: and( + eq(userProjects.projectId, input.projectId), + eq(userProjects.userId, userId), + ), + columns: { projectId: true }, + }); + return !!membership; + }),
36-49: Add tests for access control pathsRecommend tests for:
- returns true when user is a member (any role)
- returns false when user is not a member
- returns false for non-existent project
- rejects unauthenticated access via
protectedProcedureI can scaffold tRPC integration tests with an in-memory DB or transaction sandbox if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/web/client/src/app/project/[id]/layout.tsx(1 hunks)apps/web/client/src/server/api/routers/project/project.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/web/client/src/app/project/[id]/layout.tsx (3)
apps/web/client/src/trpc/react.tsx (1)
api(23-23)apps/web/client/src/utils/constants/index.ts (1)
Routes(1-20)packages/ui/src/components/icons/index.tsx (1)
Icons(138-3503)
apps/web/client/src/server/api/routers/project/project.ts (3)
apps/web/client/src/server/api/trpc.ts (1)
protectedProcedure(130-149)packages/db/src/schema/project/project.ts (1)
projects(12-30)packages/db/src/schema/user/user-project.ts (1)
userProjects(10-23)
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
| @@ -15,15 +15,15 @@ export async function login(provider: SignInMethod.GITHUB | SignInMethod.GOOGLE) | |||
| data: { session }, | |||
| } = await supabase.auth.getSession(); | |||
| if (session) { | |||
| redirect('/'); | |||
| redirect(returnUrl || '/'); | |||
There was a problem hiding this comment.
Validate the 'returnUrl' before using it in redirect calls to avoid open redirect vulnerabilities. Ensure it’s a relative/internal URL.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
apps/web/client/src/app/projects/layout.tsx (1)
19-21: Sanitize x-pathname to prevent open-redirects; also drop unnecessary await on headers()Do not trust header-provided path blindly. Constrain to safe relative paths and avoid host/protocol injection. Also, headers() is synchronous in Next.js; awaiting it is unnecessary.
Apply this diff:
- const headersList = await headers(); - const pathname = headersList.get('x-pathname') || Routes.PROJECTS; + const headersList = headers(); + const pathname = (() => { + const p = headersList.get('x-pathname'); + return p && p.startsWith('/') && !p.startsWith('//') && !p.includes(':') ? p : Routes.PROJECTS; + })(); redirect(`${Routes.LOGIN}?${getReturnUrlQueryParam(pathname)}`);apps/web/client/src/app/invitation/[id]/_components/main.tsx (1)
31-36: Bug: returnUrl not URL-encoded; token query breaks and can be lost on redirectManually concatenating the unencoded current URL will split token into a top-level param on /login. Use the shared helper to encode.
Apply this diff:
const handleLogin = async () => { const supabase = createClient(); await supabase.auth.signOut(); const currentUrl = `${pathname}${searchParams.toString() ? `?${searchParams.toString()}` : ''}`; - router.push(`${Routes.LOGIN}?${LocalForageKeys.RETURN_URL}=${currentUrl}`); + router.push(`${Routes.LOGIN}?${getReturnUrlQueryParam(currentUrl)}`); }apps/web/client/src/server/api/routers/project/invitation.ts (2)
171-175: Ensure constructInvitationLink no longer depends on url-join (remove leftover dep)The email helper still imports and uses url-join per the provided snippet. Replace with the standard URL API to avoid the dependency and edge-case bugs.
Proposed change in packages/email/src/invitation.ts (for that file):
export const constructInvitationLink = ( publicUrl: string, invitationId: string, token: string, ) => { const url = new URL(publicUrl); url.pathname = `/invitation/${invitationId}`; url.searchParams.set('token', token); return url.toString(); };Run this to locate any lingering url-join usage:
#!/bin/bash set -euo pipefail echo "Searching for 'url-join' imports/usages..." rg -nP "url-join" -C2 || true
45-50: Do not expose the invitation token in the GET responseLeaking the token undermines secret-link semantics. Omit it from the response.
Apply this diff:
- return { - ...invitation, - // @ts-expect-error - Drizzle is not typed correctly - inviter: toUser(invitation.inviter), - }; + const { token: _omitToken, ...safeInvitation } = invitation as typeof invitation & { token?: string }; + return { + ...safeInvitation, + // @ts-expect-error - Drizzle is not typed correctly + inviter: toUser(invitation.inviter), + };
🧹 Nitpick comments (4)
apps/web/client/src/utils/url/index.ts (1)
7-32: Add SSR guard to sanitizeReturnUrl and handle percent-encoded inputsTo ensure this runs safely in server contexts and avoids cases where the returnUrl is still percent-encoded, update the function as follows:
export function sanitizeReturnUrl(returnUrl: string | null): string { // Default to home page if no return URL if (!returnUrl) { return Routes.HOME; } + // SSR guard: window isn’t available on the server + if (typeof window === 'undefined') { + // Only allow safe relative paths during SSR + if (returnUrl.startsWith('/') && !returnUrl.startsWith('//')) { + return returnUrl; + } + return Routes.HOME; + } + + // Handle percent-encoded inputs (e.g. "%2Fpath") + try { + returnUrl = decodeURIComponent(returnUrl); + } catch { + // leave original if it’s not valid percent-encoding + } try { // If it's a relative path, it's safe if (returnUrl.startsWith('/') && !returnUrl.startsWith('//')) { return returnUrl; } // Parse as URL to check if it's same-origin const url = new URL(returnUrl, window.location.origin); // Only allow same-origin URLs if (url.origin === window.location.origin) { return url.pathname + url.search + url.hash; } } catch { // Invalid URL format, fall back to default } // Default to home page for any invalid or external URLs return Routes.HOME; }• Preserve the original URL-parsing & same-origin logic.
• Decode percent-encoded paths before validation; this avoids inputs like/https%3A%2F%2F….
• On the server (SSR), only allow safe relative paths and fall back to HOME for everything else.apps/web/client/src/app/invitation/[id]/_components/main.tsx (1)
16-16: Avoid duplicate useSearchParams() callsReuse the existing searchParams instance to read token.
Apply this diff:
- const token = useSearchParams().get('token'); + const token = searchParams.get('token');apps/web/client/src/server/api/routers/project/invitation.ts (2)
73-78: Also strip token in getWithoutToken instead of overriding with nullOverwriting with null is okay but destructuring it out avoids accidental exposure if shapes change.
Apply this diff:
- return { - ...invitation, - token: null, - // @ts-expect-error - Drizzle is not typed correctly - inviter: toUser(invitation.inviter), - }; + const { token: _omitToken, ...safeInvitation } = invitation as typeof invitation & { token?: string }; + return { + ...safeInvitation, + // @ts-expect-error - Drizzle is not typed correctly + inviter: toUser(invitation.inviter), + };
231-235: Minor: remove redundant truthiness check before deleteinvitation is guaranteed truthy here; simplify the expiry delete branch.
Apply this diff:
- if (isAfter(new Date(), invitation.expiresAt)) { - if (invitation) { - await ctx.db - .delete(projectInvitations) - .where(eq(projectInvitations.id, invitation.id)); - } + if (isAfter(new Date(), invitation.expiresAt)) { + await ctx.db + .delete(projectInvitations) + .where(eq(projectInvitations.id, invitation.id));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
apps/web/client/src/app/auth/redirect/page.tsx(1 hunks)apps/web/client/src/app/invitation/[id]/_components/auth.tsx(1 hunks)apps/web/client/src/app/invitation/[id]/_components/main.tsx(3 hunks)apps/web/client/src/app/invitation/[id]/layout.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/members/invite-member-input.tsx(3 hunks)apps/web/client/src/app/projects/layout.tsx(2 hunks)apps/web/client/src/components/ui/avatar-dropdown/index.tsx(2 hunks)apps/web/client/src/server/api/routers/project/invitation.ts(9 hunks)apps/web/client/src/utils/url/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/web/client/src/components/ui/avatar-dropdown/index.tsx
- apps/web/client/src/app/invitation/[id]/layout.tsx
- apps/web/client/src/app/project/[id]/_components/members/invite-member-input.tsx
- apps/web/client/src/app/auth/redirect/page.tsx
🧰 Additional context used
🧬 Code Graph Analysis (4)
apps/web/client/src/utils/url/index.ts (1)
apps/web/client/src/utils/constants/index.ts (2)
LocalForageKeys(42-44)Routes(1-23)
apps/web/client/src/app/invitation/[id]/_components/main.tsx (3)
apps/web/server/src/router/trpc.ts (1)
router(12-12)apps/web/client/src/trpc/react.tsx (1)
api(23-23)apps/web/client/src/utils/constants/index.ts (2)
Routes(1-23)LocalForageKeys(42-44)
apps/web/client/src/app/projects/layout.tsx (2)
apps/web/client/src/utils/constants/index.ts (1)
Routes(1-23)apps/web/client/src/utils/url/index.ts (1)
getReturnUrlQueryParam(3-5)
apps/web/client/src/server/api/routers/project/invitation.ts (6)
packages/db/src/dto/user/user.ts (1)
toUser(18-30)apps/web/client/src/server/api/trpc.ts (1)
protectedProcedure(130-149)packages/db/src/schema/project/invitation.ts (1)
projectInvitations(7-27)packages/db/src/schema/user/user.ts (1)
users(12-24)packages/email/src/invitation.ts (2)
sendInvitationEmail(6-24)constructInvitationLink(26-28)apps/web/client/src/env.ts (1)
env(4-133)
🔇 Additional comments (5)
apps/web/client/src/app/projects/layout.tsx (1)
21-21: Sanitization Confirmed in Auth Redirect FlowI’ve verified that all downstream uses of
returnUrlare sanitized before navigation:
- In
apps/web/client/src/app/auth/redirect/page.tsx(lines 14–18), the code retrieves the storedreturnUrl, passes it throughsanitizeReturnUrl(...), and then callsrouter.replace(sanitizedUrl).- Both the project layout redirect (
layout.tsx) and the invitation flow push the raw return URL into the login query, but it’s always processed throughsanitizeReturnUrlin the redirect page before any client-side navigation.No further changes are needed.
apps/web/client/src/app/invitation/[id]/_components/auth.tsx (1)
14-17: LGTM: returnUrl is correctly URL-encoded and centralizedUsing getReturnUrlQueryParam ensures proper encoding (preserves token) and keeps behavior consistent across the app.
apps/web/client/src/utils/url/index.ts (1)
3-5: LGTM: query param helper encodes returnUrlThe helper returns a key=value pair with URL-encoded value as expected.
apps/web/client/src/app/invitation/[id]/_components/main.tsx (1)
54-86: Good error UX and recovery optionsClear error messaging with actions to go home or re-auth is solid. Once the login redirect encoding fix is applied, this flow should be reliable.
apps/web/client/src/server/api/routers/project/invitation.ts (1)
23-29: Enforce project-level row-level security on project_invitationsWe didn’t find any RLS policies for
project_invitationsin your DB migrations—please verify that reads are scoped to project members only:• apps/web/client/src/server/api/routers/project/invitation.ts
– Ensure theprotectedProcedure.getendpoint only returns invitations for projects the user belongs to.
• DB migrations (e.g. packages/db/migrations/…)
– Confirm aCREATE POLICYexists onproject_invitationsforSELECTthat restricts rows to the current project’s ID.
• Public invite flow (api.invitation.getWithoutToken)
– Verify it only returns minimal invite data when presented with a valid token and strips any sensitive fields.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/web/client/src/app/login/actions.tsx (1)
13-14: Harden origin handling; build redirectTo via URL and drop unnecessary await
- The Origin header can be absent or spoofed; don’t trust it blindly. Prefer a known configured base URL, validate protocol, and construct redirectTo with URL to avoid double slashes.
- headers() is synchronous; the await is unnecessary.
Proposed refactor:
- const origin = (await headers()).get('origin') ?? env.NEXT_PUBLIC_SITE_URL; - const redirectTo = `${origin}${Routes.AUTH_CALLBACK}`; + const originHeader = headers().get('origin') ?? undefined; + const base = originHeader ?? env.NEXT_PUBLIC_SITE_URL; + let baseUrl: URL; + try { + baseUrl = new URL(base); + } catch { + throw new Error('Invalid base URL for OAuth redirect.'); + } + if (!/^https?:$/.test(baseUrl.protocol)) { + throw new Error(`Invalid protocol for OAuth redirect: ${baseUrl.protocol}`); + } + // Use URL to safely join origin and path + const redirectTo = new URL(Routes.AUTH_CALLBACK, baseUrl.origin).toString();Optional: In production, consider ignoring the Origin header and always using env.NEXT_PUBLIC_SITE_URL (or enforcing an allowlist) to mitigate spoofed origins from client-controlled requests.
🧹 Nitpick comments (5)
apps/web/client/src/utils/url/index.ts (2)
11-19: Harden input handling: trim and treat "null"/"undefined" strings as invalidMinor robustness: user-agents or upstream code can pass
" null "or"undefined"strings. Treat them as absent and fall back to HOME. Also operate on the trimmed value thereafter.Apply this diff:
- // Default to home page if no return URL - if (!returnUrl) { + // Default to home page if no return URL + if (!returnUrl) { return Routes.HOME; } + // Normalize common bogus values + const raw = returnUrl.trim(); + if (!raw || raw === 'null' || raw === 'undefined') { + return Routes.HOME; + } try { // If it's a relative path, it's safe - if (returnUrl.startsWith('/') && !returnUrl.startsWith('//')) { - return returnUrl; + if (raw.startsWith('/') && !raw.startsWith('//')) { + return raw; }And update subsequent references to use
raw:- const url = new URL(returnUrl, currentOrigin); + const url = new URL(raw, currentOrigin);
3-5: Optional Refactor: Use URLSearchParams with prefix & sanitizationAll current call sites prepend
?, so behavior won’t change, but adding aprefixoption (and optional sanitization) makes this utility safer and more ergonomic:Call sites found (all prepend
?today):
- apps/web/client/src/app/projects/layout.tsx (line 21)
- apps/web/client/src/components/ui/avatar-dropdown/index.tsx (line 37)
- apps/web/client/src/app/invitation/[id]/_components/auth.tsx (line 16)
Suggested diff:
-export function getReturnUrlQueryParam(returnUrl: string | null): string { - return returnUrl ? `${LocalForageKeys.RETURN_URL}=${encodeURIComponent(returnUrl)}` : ''; -} +export function getReturnUrlQueryParam( + returnUrl: string | null, + opts: { prefix?: '' | '?' | '&'; sanitize?: boolean; origin?: string } = {}, +): string { + if (!returnUrl) return ''; + const prefix = opts.prefix ?? ''; + const value = opts.sanitize + ? sanitizeReturnUrl(returnUrl, { origin: opts.origin }) + : returnUrl; + const params = new URLSearchParams({ [LocalForageKeys.RETURN_URL]: value }); + return `${prefix}${params.toString()}`; +}This preserves existing behavior (default
prefix = '') while allowing callers to avoid manual?/&handling and opt into URL sanitization.packages/email/src/invitation.ts (1)
17-22: Minor: drop redundant await in return
return awaitinside an async function adds no value and slightly complicates stack traces.Apply this diff:
- return await client.emails.send({ + return client.emails.send({ from: 'Onlook <[email protected]>', to: inviteeEmail, subject: `Join ${invitedByName ?? invitedByEmail} on Onlook`, react: InviteUserEmail(inviteParams), });apps/web/client/src/app/login/actions.tsx (2)
29-38: Use route constants for error redirects and guard against missing OAuth URL
- Replace hard-coded "/error" with the existing Routes constant to keep routing consistent.
- Add a defensive guard when data.url is missing and log the error for observability.
- redirectTo, + redirectTo, }, }); - if (error) { - redirect('/error'); - } - - redirect(data.url); + if (error) { + console.error('OAuth sign-in failed:', error); + redirect(Routes.AUTH_CODE_ERROR); + } + if (!data?.url) { + console.error('No OAuth URL returned from Supabase.'); + redirect(Routes.AUTH_CODE_ERROR); + } + redirect(data.url);
62-63: Post dev sign-in redirect to AUTH_REDIRECT is consistentLooks good. Optional: consider capturing errors to telemetry (e.g., Sentry) similar to password sign-in error logging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
apps/web/client/src/app/auth/auth-context.tsx(3 hunks)apps/web/client/src/app/login/actions.tsx(4 hunks)apps/web/client/src/app/project/[id]/_components/members/invite-member-input.tsx(3 hunks)apps/web/client/src/utils/url/index.ts(1 hunks)packages/email/src/invitation.ts(1 hunks)packages/models/src/project/index.ts(0 hunks)packages/models/src/project/invitation.ts(0 hunks)
💤 Files with no reviewable changes (2)
- packages/models/src/project/invitation.ts
- packages/models/src/project/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/client/src/app/project/[id]/_components/members/invite-member-input.tsx
- apps/web/client/src/app/auth/auth-context.tsx
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/email/src/invitation.ts (1)
packages/email/src/templates/invite-user.tsx (1)
InviteUserEmail(23-81)
apps/web/client/src/utils/url/index.ts (1)
apps/web/client/src/utils/constants/index.ts (2)
LocalForageKeys(42-44)Routes(1-23)
apps/web/client/src/app/login/actions.tsx (2)
apps/web/client/src/env.ts (1)
env(4-133)apps/web/client/src/utils/constants/index.ts (1)
Routes(1-23)
🔇 Additional comments (7)
apps/web/client/src/utils/url/index.ts (2)
28-37: Same-origin enforcement and path-only return is solidUsing
new URL(..., currentOrigin)and returning onlypathname + search + hashprevents open redirects and strips origin. Good defensive posture.
21-26: sanitizeReturnUrl usage limited to client components (no SSR risk)After searching the codebase (
rg -nP -C3 '\bsanitizeReturnUrl\s*\('), the only invocation ofsanitizeReturnUrlis in:
- apps/web/client/src/app/auth/redirect/page.tsx
That file is a client component (it uses
localforageand runs in the browser), sowindow.location.originis always defined and the SSR fallback toRoutes.HOMEcan’t occur. No changes needed.packages/email/src/invitation.ts (2)
6-8: Solid refactor to structured inviteParamsSwitching to
[client, inviteParams, options]and destructuring props improves readability and aligns with the updated template props. Looks good.
25-29: Consider dropping the leading slash to preserve any base pathUsing the WHATWG URL API here is spot-on. However,
new URL('/invitation/…', publicUrl)will always reset the path to the domain root. If you ever host your site under a sub-path (e.g.https://example.com/app), that/appwould be discarded. To retain any base path, remove the leading slash:Files to update:
- packages/email/src/invitation.ts
Apply this diff:
--- a/packages/email/src/invitation.ts +++ b/packages/email/src/invitation.ts @@ export const constructInvitationLink = (publicUrl: string, invitationId: string, token: string) => { - const url = new URL(`/invitation/${invitationId}`, publicUrl); + const url = new URL(`invitation/${invitationId}`, publicUrl); url.searchParams.set('token', token); return url.toString(); };Call sites (for reference):
- apps/web/client/src/app/project/[id]/_components/members/invitation-row.tsx
- apps/web/client/src/server/api/routers/project/invitation.ts
If your
NEXT_PUBLIC_SITE_URL(or any other base-URL) never includes a path segment, this is purely optional. Otherwise, please apply the change.apps/web/client/src/app/login/actions.tsx (3)
3-4: Imports of env and Routes are appropriateGood move using centralized env validation and route constants to avoid magic strings.
21-22: Session short-circuit to AUTH_REDIRECT looks goodKeeps the flow consistent with the new post-login redirect handling.
50-51: Dev session short-circuit to AUTH_REDIRECT is consistent with the new flowNo issues. Guard against non-dev environments is correct.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
apps/web/client/src/app/login/actions.tsx (1)
13-14: Remove unnecessary await on headers() and harden redirectTo with URL + protocol validation
- headers() is synchronous; awaiting it is unnecessary and may fail TS checks.
- Build redirectTo with URL to avoid double slashes.
- Validate base URL protocol (http/https only) and use a robust fallback.
This also addresses the earlier feedback about origin fallback and safe URL join.
Apply this diff:
- const origin = (await headers()).get('origin') ?? env.NEXT_PUBLIC_SITE_URL; - const redirectTo = `${origin}${Routes.AUTH_CALLBACK}`; + const headerList = headers(); + const originHeader = headerList.get('origin') ?? null; + const base = originHeader ?? env.NEXT_PUBLIC_SITE_URL; + const baseUrl = new URL(base); + if (!/^https?:$/.test(baseUrl.protocol)) { + throw new Error(`Invalid base URL protocol for OAuth redirect: ${baseUrl.protocol}`); + } + const redirectTo = new URL(Routes.AUTH_CALLBACK, baseUrl.origin).toString();Optionally, consider logging when falling back to env.NEXT_PUBLIC_SITE_URL for observability.
To ensure there aren’t other instances of unnecessary
await headers():#!/bin/bash rg -nP --type=ts --type=tsx -C2 'await\s+headers\s*\(\s*\)'apps/web/client/src/app/auth/auth-context.tsx (2)
17-18: Type fix landed: async handlers now return PromiseThis resolves the TS mismatch noted earlier.
36-42: Correct ordering: persist before invoking server actionSaving RETURN_URL and LAST_SIGN_IN_METHOD_KEY before calling login ensures values persist even when the Server Action redirects immediately. Nice.
apps/web/client/src/utils/url/index.ts (1)
7-38: Solid open-redirect mitigation with same-origin enforcementsanitizeReturnUrl correctly:
- treats single-slash relative paths as safe
- rejects scheme-relative and cross-origin URLs
- uses server-provided origin when available
This addresses prior concerns about validating returnUrl before redirect.
🧹 Nitpick comments (5)
apps/web/client/src/app/login/actions.tsx (1)
33-38: Use an auth error route and guard against missing data.urlRedirecting to a generic '/error' loses context. Prefer the dedicated auth error route and handle the edge where data.url is unexpectedly empty.
Apply this diff:
- if (error) { - redirect('/error'); - } - - redirect(data.url); + if (error) { + redirect(Routes.AUTH_CODE_ERROR); + } + if (!data?.url) { + redirect(Routes.AUTH_CODE_ERROR); + } + redirect(data.url);apps/web/client/src/app/auth/auth-context.tsx (1)
48-53: Persist DEV as last sign-in method for consistencyDev flow mirrors OAuth flow but doesn’t currently persist LAST_SIGN_IN_METHOD_KEY. Persisting SignInMethod.DEV keeps UX consistent (e.g., remembering last method).
Apply this diff:
const handleDevLogin = async (returnUrl: string | null) => { setSigningInMethod(SignInMethod.DEV); if (returnUrl) { await localforage.setItem(LocalForageKeys.RETURN_URL, returnUrl); } + await localforage.setItem(LAST_SIGN_IN_METHOD_KEY, SignInMethod.DEV); await devLogin(); setTimeout(() => { setSigningInMethod(null); }, 5000); }apps/web/client/src/utils/url/index.ts (3)
3-5: Query param helper is fine; consider returning with leading '?'Current return string is 'returnUrl=...'. Call sites need to prepend '?' or '&'. If most consumers need the leading '?', consider returning it here to reduce repetition.
Example alternative:
export function getReturnUrlQueryParam(returnUrl: string | null): string { return returnUrl ? `?${LocalForageKeys.RETURN_URL}=${encodeURIComponent(returnUrl)}` : ''; }
11-19: Minor hardening: trim input before checksTrimming handles accidental whitespace and avoids false negatives in startsWith('/') and URL parsing.
Apply this diff:
- // Default to home page if no return URL - if (!returnUrl) { + // Default to home page if no return URL + const raw = returnUrl?.trim(); + if (!raw) { return Routes.HOME; } try { // If it's a relative path, it's safe - if (returnUrl.startsWith('/') && !returnUrl.startsWith('//')) { - return returnUrl; + if (raw.startsWith('/') && !raw.startsWith('//')) { + return raw; } // Resolve current origin from options or the browser (if available) const currentOrigin = opts.origin ?? (typeof window !== 'undefined' ? window.location.origin : undefined); // On the server (no origin), reject non-relative URLs if (!currentOrigin) { return Routes.HOME; } // Parse as URL to check if it's same-origin - const url = new URL(returnUrl, currentOrigin); + const url = new URL(raw, currentOrigin);Also applies to: 21-32
7-38: Add focused unit tests for sanitizeReturnUrlGiven its security role, add tests covering:
- '/', '/projects', '/projects?id=1#tab'
- '//evil.com', 'http://evil.com', 'https://evil.com'
- absolute same-origin vs different port
- whitespace-wrapped values
- server-side path with opts.origin provided vs missing
I can draft a small vitest/jest test suite for this helper. Want me to open a follow-up PR with tests?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
apps/web/client/src/app/auth/auth-context.tsx(3 hunks)apps/web/client/src/app/login/actions.tsx(4 hunks)apps/web/client/src/app/project/[id]/_components/members/invite-member-input.tsx(3 hunks)apps/web/client/src/utils/url/index.ts(1 hunks)packages/email/src/invitation.ts(1 hunks)packages/models/src/project/index.ts(0 hunks)packages/models/src/project/invitation.ts(0 hunks)
💤 Files with no reviewable changes (2)
- packages/models/src/project/index.ts
- packages/models/src/project/invitation.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/email/src/invitation.ts
- apps/web/client/src/app/project/[id]/_components/members/invite-member-input.tsx
🧰 Additional context used
🧬 Code Graph Analysis (3)
apps/web/client/src/utils/url/index.ts (1)
apps/web/client/src/utils/constants/index.ts (2)
LocalForageKeys(42-44)Routes(1-23)
apps/web/client/src/app/login/actions.tsx (2)
apps/web/client/src/env.ts (1)
env(4-133)apps/web/client/src/utils/constants/index.ts (1)
Routes(1-23)
apps/web/client/src/app/auth/auth-context.tsx (2)
apps/web/client/src/utils/constants/index.ts (1)
LocalForageKeys(42-44)apps/web/client/src/app/login/actions.tsx (1)
login(11-38)
🔇 Additional comments (6)
apps/web/client/src/app/login/actions.tsx (5)
3-4: Good move: centralized env and route constantsImporting env and Routes improves consistency and reduces hard-coded strings across auth flows.
21-21: Consistent redirect targetRedirecting existing sessions to Routes.AUTH_REDIRECT aligns with the new centralized flow.
29-31: LGTM: passing computed redirectTo to Supabase OAuthUsing the computed redirectTo keeps the OAuth callback aligned with the current origin/fallback.
50-50: Consistent redirect on existing dev sessionUsing Routes.AUTH_REDIRECT here keeps dev flow behavior aligned with the main flow.
62-62: Dev login completion redirect is consistentRedirecting to Routes.AUTH_REDIRECT after successful dev login matches the centralized auth strategy.
apps/web/client/src/app/auth/auth-context.tsx (1)
28-34: Lazy-load last sign-in methodFetching and setting lastSignInMethod asynchronously on mount is clean and avoids blocking render.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/web/client/src/components/store/editor/version/git.ts (1)
65-72: Consider using a dedicated no-reply/automation email for commit authorshipUsing SUPPORT_EMAIL for git author email can conflate commit authorship with the support channel (e.g., in blame/logs). If feasible, consider introducing a dedicated NO_REPLY_EMAIL or BOT_EMAIL for automated commits to clarify provenance.
apps/web/client/src/app/project/[id]/_components/left-panel/help-dropdown/index.tsx (1)
131-131: Prefer setting window.location for mailto instead of window.openwindow.open with a mailto: can be blocked by popup blockers and target "_blank" is not meaningful here. Setting location is simpler and more reliable.
Apply this diff:
- onClick={() => window.open(`mailto:${SUPPORT_EMAIL}`, '_blank')} + onClick={() => { + window.location.href = `mailto:${SUPPORT_EMAIL}`; + }}apps/web/client/src/app/project/[id]/layout.tsx (3)
7-9: Type cleanup: params should not be a Promise; avoid unnecessary awaitNext.js passes params as a plain object. Typing it as Promise adds noise and requires an unnecessary await.
Apply this diff:
-export default async function Layout({ params, children }: Readonly<{ params: Promise<{ id: string }>, children: React.ReactNode }>) { - const projectId = (await params).id; +export default async function Layout({ + params, + children, +}: Readonly<{ params: { id: string }; children: React.ReactNode }>) { + const { id: projectId } = params;
9-12: Handle TRPC errors to avoid surfacing 500s on unauthorized/unauthenticatedIf api.project.hasAccess throws (e.g., UNAUTHORIZED), the layout could error. Treat errors as “no access” or redirect, depending on your desired UX.
Apply this diff to fail closed without a 500:
- const hasAccess = await api.project.hasAccess({ projectId }); - if (!hasAccess) { + let hasAccess = false; + try { + hasAccess = await api.project.hasAccess({ projectId }); + } catch { + hasAccess = false; + } + if (!hasAccess) { return <NoAccess />; }If the intended behavior is to redirect unauthenticated users to login instead, say the word and I’ll adjust the snippet to use next/navigation’s redirect with your Routes.LOGIN.
24-29: Use a plain anchor for mailto instead of Next.js LinkLink is intended for client-side routing of internal pages; for mailto, a simple is more appropriate and avoids Next’s prefetch logic.
Apply this diff:
- <Link href={`mailto:${SUPPORT_EMAIL}`} className="text-primary underline"> - {SUPPORT_EMAIL} - </Link> + <a + href={`mailto:${SUPPORT_EMAIL}`} + className="text-primary underline" + > + {SUPPORT_EMAIL} + </a>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
apps/web/client/src/app/auth/callback/route.ts(2 hunks)apps/web/client/src/app/invitation/[id]/_components/main.tsx(3 hunks)apps/web/client/src/app/project/[id]/_components/left-panel/help-dropdown/index.tsx(2 hunks)apps/web/client/src/app/project/[id]/layout.tsx(1 hunks)apps/web/client/src/components/store/editor/version/git.ts(3 hunks)packages/constants/src/contact.ts(1 hunks)packages/constants/src/index.ts(1 hunks)packages/email/src/invitation.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/constants/src/contact.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/web/client/src/app/auth/callback/route.ts
- packages/email/src/invitation.ts
- apps/web/client/src/app/invitation/[id]/_components/main.tsx
🧰 Additional context used
🧬 Code Graph Analysis (3)
apps/web/client/src/components/store/editor/version/git.ts (1)
packages/constants/src/contact.ts (1)
SUPPORT_EMAIL(1-1)
apps/web/client/src/app/project/[id]/layout.tsx (4)
apps/web/client/src/trpc/react.tsx (1)
api(23-23)packages/constants/src/contact.ts (1)
SUPPORT_EMAIL(1-1)apps/web/client/src/utils/constants/index.ts (1)
Routes(1-23)packages/ui/src/components/icons/index.tsx (1)
Icons(138-3503)
apps/web/client/src/app/project/[id]/_components/left-panel/help-dropdown/index.tsx (1)
packages/constants/src/contact.ts (1)
SUPPORT_EMAIL(1-1)
🔇 Additional comments (4)
apps/web/client/src/components/store/editor/version/git.ts (2)
5-5: Good move: centralize git config email via SUPPORT_EMAILReplacing the hard-coded email with the shared SUPPORT_EMAIL constant improves maintainability and avoids drift.
Also applies to: 67-68
20-20: Format-only changeConstructor spacing/nit is fine; no action needed.
packages/constants/src/index.ts (1)
2-2: Barrel re-export looks correctRe-exporting contact makes SUPPORT_EMAIL available app-wide via @onlook/constants. Consistent with other barrel exports.
apps/web/client/src/app/project/[id]/_components/left-panel/help-dropdown/index.tsx (1)
3-3: Centralized support email import is a winImporting SUPPORT_EMAIL from @onlook/constants prevents hard-coding and keeps contact info consistent across the app.
apps/web/client/src/app/project/[id]/_components/members/invitation-row.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/web/client/src/app/project/[id]/_components/members/invitation-row.tsx (1)
23-36: Fix clipboard error handling: writeText is async; current try/catch won’t catch rejectionsnavigator.clipboard.writeText returns a Promise. Without awaiting it, async failures won’t hit the catch block, and the UI will optimistically show “copied.” Also add a non-secure-context fallback.
Apply this diff:
- const copyInvitationLink = () => { - try { - navigator.clipboard.writeText(constructInvitationLink(env.NEXT_PUBLIC_SITE_URL, invitation.id, invitation.token)); - setIsCopied(true); - toast.success('Invitation link copied to clipboard'); - setTimeout(() => { - setIsCopied(false); - }, 2000); - } catch (error) { - console.error('Failed to copy invitation link:', error); - toast.error('Failed to copy invitation link'); - setIsCopied(false); - } - }; + const copyInvitationLink = async () => { + try { + const link = constructInvitationLink( + env.NEXT_PUBLIC_SITE_URL, + invitation.id, + invitation.token + ); + if (navigator.clipboard && window.isSecureContext) { + await navigator.clipboard.writeText(link); + } else { + // Fallback for HTTP/non-secure contexts and older browsers + const el = document.createElement('textarea'); + el.value = link; + el.setAttribute('readonly', ''); + el.style.position = 'fixed'; + el.style.opacity = '0'; + document.body.appendChild(el); + el.focus(); + el.select(); + document.execCommand('copy'); + document.body.removeChild(el); + } + setIsCopied(true); + toast.success('Invitation link copied to clipboard'); + setTimeout(() => { + setIsCopied(false); + }, 2000); + } catch (error) { + console.error('Failed to copy invitation link:', error); + toast.error('Failed to copy invitation link'); + setIsCopied(false); + } + };
🧹 Nitpick comments (3)
apps/web/client/src/app/project/[id]/_components/members/invitation-row.tsx (3)
17-21: Add onError and user feedback for cancel mutationSurface success/error to users and avoid silent failures.
- const cancelInvitationMutation = api.invitation.delete.useMutation({ - onSuccess: () => { - apiUtils.invitation.list.invalidate(); - }, - }); + const cancelInvitationMutation = api.invitation.delete.useMutation({ + onSuccess: () => { + toast.success('Invitation canceled'); + apiUtils.invitation.list.invalidate(); + }, + onError: (err) => { + toast.error(err?.message ?? 'Failed to cancel invitation'); + }, + });
51-56: A11y: icon-only buttons need accessible names; also prevent accidental form submitAdd aria-labels to icon-only buttons. Set type="button" to prevent form submit if rendered inside a form. Optionally disable the cancel action while the mutation is pending.
- <Button + <Button + type="button" variant="ghost" size="icon" + aria-label={isCopied ? 'Copied to clipboard' : 'Copy invitation link'} onClick={copyInvitationLink} > ... - <Button + <Button + type="button" variant="ghost" size="icon" + aria-label="Cancel invitation" + disabled={cancelInvitationMutation.isPending} onClick={() => { cancelInvitationMutation.mutate({ id: invitation.id }); }} >Also applies to: 65-72
28-31: Optional: clear timeout on unmount to avoid state updates after unmountThe setTimeout can resolve after unmount, causing a setState on an unmounted component. Track the timer and clear it on unmount.
- setTimeout(() => { - setIsCopied(false); - }, 2000); + resetTimerRef.current = window.setTimeout(() => { + setIsCopied(false); + }, 2000);Add the following outside the selected lines:
// 1) Update import import { useEffect, useRef, useState } from 'react'; // 2) Inside component const resetTimerRef = useRef<number | undefined>(undefined); useEffect(() => { return () => { if (resetTimerRef.current) { clearTimeout(resetTimerRef.current); } }; }, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/web/client/src/app/project/[id]/_components/members/invitation-row.tsx(2 hunks)
apps/web/client/src/app/project/[id]/_components/members/invitation-row.tsx
Show resolved
Hide resolved
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Important
This PR adds access control and enhanced invitation management to projects, including new authentication flows, email templates, and session handling improvements.
layout.tsxto restrict access based on user permissions.HandleAuthcomponent ininvitation/[id]/_components/auth.tsxfor login prompts.constructInvitationLinkininvitation.tsand updated email templates ininvite-user.tsx.getWithoutTokenquery ininvitation.tsto fetch invitation details without a token.InvitationRowinmembers/invitation-row.tsxto include copy-to-clipboard functionality.SUPPORT_EMAILincontact.tsand used it in email templates and notifications.sendInvitationEmailininvitation.tsto include inviter's name in the subject.localStoragewithlocalforagefor session management in multiple components.sanitizeReturnUrlandgetReturnUrlQueryParaminurl/index.tsfor secure URL handling.dayjsandurl-joinfrompackage.json.features/page.tsxandhero/features-hero.tsx.This description was created by
for df07cef. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Improvements