Conversation
|
Review CompleteYour review story is ready! Comment !reviewfast on this PR to re-generate the story. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds three optional env vars: PLAIN_CUSTOMER_CARDS_SECRET, PLAIN_CUSTOMER_CARDS_KEY, and PLAIN_CUSTOMER_CARDS_HEADERS. Adds a POST route at /api.v1/plain/customer-cards that authenticates via bearer/plain token, validates JSON, looks up users by externalId or email, and returns assembled UI card payloads (account-details, organizations, projects) including impersonation action links. Adds that endpoint to the rate-limit whitelist. Extracts impersonation handling from the admin index, introducing a dedicated /admin/impersonate route and a new impersonation service that issues and validates short-lived JWT one‑time tokens with Redis-backed replay protection. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
ericallam
left a comment
ericallam
left a comment
There was a problem hiding this comment.
This looks great but needs one tweak before getting merged: instead of using the /admin route for the impersonation request, could you add a new route /admin/impersonate and move the impersonation code into there? That way the /admin route isn't overloaded with impersonation code.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
apps/webapp/app/routes/admin.impersonate.tsx (1)
10-16: DoublerequireUsercall on both GET and POST paths.
handleImpersonationRequestcallsrequireUser, and thenredirectWithImpersonation(inadmin.server.tsline 216) callsrequireUseragain internally. This results in two separate DB lookups per impersonation request. For an admin-only action this is low impact, but you could avoid it by passing the already-fetched user toredirectWithImpersonationor by inlining the logic.Also applies to: 32-35, 45-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/routes/admin.impersonate.tsx` around lines 10 - 16, handleImpersonationRequest currently calls requireUser and then calls redirectWithImpersonation which itself calls requireUser again, causing duplicate DB lookups; fix by changing redirectWithImpersonation to accept an optional authenticated user (e.g., add a parameter like currentUser) or create a new overload, then pass the already-fetched user from handleImpersonationRequest into redirectWithImpersonation (or inline the impersonation check into handleImpersonationRequest) and remove the second requireUser call inside redirectWithImpersonation so only one DB lookup occurs; update all call sites (including those at lines referenced) to use the new signature or inline behavior and keep permission check using user.admin from the single fetched user.apps/webapp/app/routes/api.v1.plain.customer-cards.ts (1)
119-166: Duplicated Prisma query — extract to a helper.The two
prisma.user.findUniquecalls (lines 122-142 and 145-165) share identicalincludeclauses, differing only in thewherecondition. Consider extracting the shared include into a constant and using a single query path.♻️ Proposed refactor
+ const userInclude = { + orgMemberships: { + where: { organization: { deletedAt: null } }, + include: { + organization: { + include: { + projects: { + where: { deletedAt: null }, + take: 10, + orderBy: { createdAt: "desc" as const }, + }, + }, + }, + }, + }, + }; + + const whereClause = customer.externalId + ? { id: customer.externalId } + : { email: customer.email! }; + + const user = await prisma.user.findUnique({ + where: whereClause, + include: userInclude, + }); - let user = null; - if (customer.externalId) { - user = await prisma.user.findUnique({ - where: { id: customer.externalId }, - include: { ... }, - }); - } else if (customer.email) { - user = await prisma.user.findUnique({ - where: { email: customer.email }, - include: { ... }, - }); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/routes/api.v1.plain.customer-cards.ts` around lines 119 - 166, The two prisma.user.findUnique calls duplicate the same include graph; extract the shared include into a constant (e.g., userInclude or USER_INCLUDE) and build a single where object based on customer.externalId or customer.email (e.g., const where = customer.externalId ? { id: customer.externalId } : customer.email ? { email: customer.email } : null) then call prisma.user.findUnique({ where, include: userInclude }) only when where is non-null; update references to orgMemberships/organization/projects remain unchanged.apps/webapp/app/services/impersonation.server.ts (1)
111-131: Replay protection uses the full JWT as Redis key — consider using a JTI claim instead.The full JWT string can be several hundred bytes. Using a shorter, unique identifier (e.g., a
jticlaim set during signing) as the Redis key would reduce memory usage and key size without losing any security properties. This is optional and non-blocking.♻️ Proposed refactor using JTI
In
generateImpersonationToken:+import { randomUUID } from "crypto"; + export async function generateImpersonationToken(userId: string): Promise<string> { const secret = getImpersonationTokenSecret(); const now = Math.floor(Date.now() / 1000); + const jti = randomUUID(); const token = await new SignJWT({ userId }) .setProtectedHeader({ alg: "HS256" }) .setIssuedAt(now) .setExpirationTime(now + IMPERSONATION_TOKEN_EXPIRY_SECONDS) .setIssuer("https://trigger.dev") .setAudience("https://trigger.dev/admin") + .setJti(jti) .sign(secret); return token; }In
validateAndConsumeImpersonationToken, usepayload.jtias the Redis key instead of the full token string.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/app/services/impersonation.server.ts` around lines 111 - 131, The replay-protection currently uses the full JWT string as the Redis key; change it to use a short unique identifier by adding a jti when signing in generateImpersonationToken and then using payload.jti as the Redis key in validateAndConsumeImpersonationToken (instead of token). Ensure getImpersonationTokenRedisClient() calls still use IMPERSONATION_TOKEN_EXPIRY_SECONDS, and add a defensive check in validateAndConsumeImpersonationToken to reject or derive a stable short key (e.g., compute a hash) if payload.jti is missing so behavior remains safe when older tokens without jti are encountered.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/webapp/app/routes/admin._index.tsxapps/webapp/app/routes/admin.impersonate.tsxapps/webapp/app/routes/api.v1.plain.customer-cards.tsapps/webapp/app/services/impersonation.server.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
**/*.{ts,tsx}: Always import tasks from@trigger.dev/sdk, never use@trigger.dev/sdk/v3or deprecatedclient.defineJobpattern
Every Trigger.dev task must be exported and have a uniqueidproperty with no timeouts in the run function
Files:
apps/webapp/app/services/impersonation.server.tsapps/webapp/app/routes/api.v1.plain.customer-cards.tsapps/webapp/app/routes/admin._index.tsxapps/webapp/app/routes/admin.impersonate.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/services/impersonation.server.tsapps/webapp/app/routes/api.v1.plain.customer-cards.tsapps/webapp/app/routes/admin._index.tsxapps/webapp/app/routes/admin.impersonate.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Import from
@trigger.dev/coreusing subpaths only, never import from root
Files:
apps/webapp/app/services/impersonation.server.tsapps/webapp/app/routes/api.v1.plain.customer-cards.tsapps/webapp/app/routes/admin._index.tsxapps/webapp/app/routes/admin.impersonate.tsx
apps/webapp/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access all environment variables through the
envexport ofenv.server.tsinstead of directly accessingprocess.envin the Trigger.dev webapp
Files:
apps/webapp/app/services/impersonation.server.tsapps/webapp/app/routes/api.v1.plain.customer-cards.tsapps/webapp/app/routes/admin._index.tsxapps/webapp/app/routes/admin.impersonate.tsx
apps/webapp/app/services/**/*.server.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Separate testable services from configuration files; follow the pattern of
realtimeClient.server.ts(testable service) andrealtimeClientGlobal.server.ts(configuration) in the webapp
Files:
apps/webapp/app/services/impersonation.server.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, use subpath exports from the package.json instead of importing from the root path
Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webappAccess environment variables via
envexport fromapps/webapp/app/env.server.ts, never useprocess.envdirectly
Files:
apps/webapp/app/services/impersonation.server.tsapps/webapp/app/routes/api.v1.plain.customer-cards.tsapps/webapp/app/routes/admin._index.tsxapps/webapp/app/routes/admin.impersonate.tsx
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.ts: When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
apps/webapp/app/services/impersonation.server.tsapps/webapp/app/routes/api.v1.plain.customer-cards.ts
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
apps/webapp/app/services/impersonation.server.tsapps/webapp/app/routes/api.v1.plain.customer-cards.tsapps/webapp/app/routes/admin._index.tsxapps/webapp/app/routes/admin.impersonate.tsx
🧠 Learnings (21)
📚 Learning: 2025-08-14T10:53:54.526Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 2391
File: apps/webapp/app/services/organizationAccessToken.server.ts:50-0
Timestamp: 2025-08-14T10:53:54.526Z
Learning: In the Trigger.dev codebase, token service functions (like revokePersonalAccessToken and revokeOrganizationAccessToken) don't include tenant scoping in their database queries. Instead, authorization and tenant scoping happens at a higher level in the authentication flow (typically in route handlers) before these service functions are called. This is a consistent pattern across both Personal Access Tokens (PATs) and Organization Access Tokens (OATs).
Applied to files:
apps/webapp/app/services/impersonation.server.tsapps/webapp/app/routes/admin._index.tsx
📚 Learning: 2025-11-27T16:26:58.661Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-11-27T16:26:58.661Z
Learning: Applies to apps/webapp/**/*.{ts,tsx} : Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webapp
Applied to files:
apps/webapp/app/routes/api.v1.plain.customer-cards.tsapps/webapp/app/routes/admin._index.tsxapps/webapp/app/routes/admin.impersonate.tsx
📚 Learning: 2025-09-03T14:35:52.384Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 2464
File: apps/webapp/app/utils/pathBuilder.ts:144-146
Timestamp: 2025-09-03T14:35:52.384Z
Learning: In the trigger.dev codebase, organization slugs are safe for URL query parameters and don't require URL encoding, as confirmed by the maintainer in apps/webapp/app/utils/pathBuilder.ts.
Applied to files:
apps/webapp/app/routes/api.v1.plain.customer-cards.tsapps/webapp/app/routes/admin._index.tsx
📚 Learning: 2025-11-27T16:26:58.661Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-11-27T16:26:58.661Z
Learning: Applies to apps/webapp/app/**/*.{ts,tsx} : Access all environment variables through the `env` export of `env.server.ts` instead of directly accessing `process.env` in the Trigger.dev webapp
Applied to files:
apps/webapp/app/routes/api.v1.plain.customer-cards.tsapps/webapp/app/routes/admin._index.tsx
📚 Learning: 2026-02-11T16:50:14.167Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3019
File: apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboards.$dashboardId.widgets.tsx:126-131
Timestamp: 2026-02-11T16:50:14.167Z
Learning: In apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboards.$dashboardId.widgets.tsx, MetricsDashboard entities are intentionally scoped to the organization level, not the project level. The dashboard lookup should filter by organizationId only (not projectId), allowing dashboards to be accessed across projects within the same organization. The optional projectId field on MetricsDashboard serves other purposes and should not be used as an authorization constraint.
Applied to files:
apps/webapp/app/routes/api.v1.plain.customer-cards.ts
📚 Learning: 2025-11-27T16:26:58.661Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-11-27T16:26:58.661Z
Learning: Applies to apps/webapp/**/*.{ts,tsx} : When importing from `trigger.dev/core` in the webapp, use subpath exports from the package.json instead of importing from the root path
Applied to files:
apps/webapp/app/routes/api.v1.plain.customer-cards.tsapps/webapp/app/routes/admin._index.tsx
📚 Learning: 2026-01-15T11:50:06.067Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T11:50:06.067Z
Learning: Applies to apps/webapp/**/*.{ts,tsx} : Access environment variables via `env` export from `apps/webapp/app/env.server.ts`, never use `process.env` directly
Applied to files:
apps/webapp/app/routes/api.v1.plain.customer-cards.tsapps/webapp/app/routes/admin._index.tsx
📚 Learning: 2026-02-04T16:34:48.876Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2994
File: apps/webapp/app/routes/vercel.connect.tsx:13-27
Timestamp: 2026-02-04T16:34:48.876Z
Learning: In apps/webapp/app/routes/vercel.connect.tsx, configurationId may be absent for "dashboard" flows but must be present for "marketplace" flows. Enforce this with a Zod superRefine and pass installationId to repository methods only when configurationId is defined (omit the field otherwise).
Applied to files:
apps/webapp/app/routes/api.v1.plain.customer-cards.tsapps/webapp/app/routes/admin.impersonate.tsx
📚 Learning: 2026-02-03T18:27:49.039Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2994
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx:553-555
Timestamp: 2026-02-03T18:27:49.039Z
Learning: In apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx, the menu buttons (like the Edit button with PencilSquareIcon) intentionally have no text labels - only icons are shown in the TableCellMenu. This is a deliberate UI design pattern for compact icon-only menu items.
Applied to files:
apps/webapp/app/routes/api.v1.plain.customer-cards.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use `schemaTask()` from `trigger.dev/sdk/v3` with Zod schema for payload validation
Applied to files:
apps/webapp/app/routes/admin._index.tsx
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Generate example payloads for tasks when possible
Applied to files:
apps/webapp/app/routes/admin._index.tsx
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use `idempotencyKeys.create()` to create idempotency keys for preventing duplicate task executions
Applied to files:
apps/webapp/app/routes/admin._index.tsx
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Scope idempotency keys globally or to current run using the scope parameter
Applied to files:
apps/webapp/app/routes/admin._index.tsx
📚 Learning: 2025-11-27T16:26:37.432Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T16:26:37.432Z
Learning: Applies to packages/trigger-sdk/**/*.{ts,tsx} : In the Trigger.dev SDK (packages/trigger-sdk), prefer isomorphic code like fetch and ReadableStream instead of Node.js-specific code
Applied to files:
apps/webapp/app/routes/admin._index.tsx
📚 Learning: 2025-11-27T16:26:37.432Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T16:26:37.432Z
Learning: The webapp at apps/webapp is a Remix 2.1 application using Node.js v20
Applied to files:
apps/webapp/app/routes/admin._index.tsx
📚 Learning: 2025-11-26T14:40:07.146Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 2710
File: packages/schema-to-json/package.json:0-0
Timestamp: 2025-11-26T14:40:07.146Z
Learning: Node.js 24+ has native TypeScript support and can execute .ts files directly without tsx or ts-node for scripts that use only erasable TypeScript syntax (type annotations, interfaces, etc.). The trigger.dev repository uses Node.js 24.11.1+ and scripts like updateVersion.ts can be run with `node` instead of `tsx`.
Applied to files:
apps/webapp/app/routes/admin._index.tsx
📚 Learning: 2025-11-27T16:26:58.661Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-11-27T16:26:58.661Z
Learning: Applies to apps/webapp/app/services/**/*.server.{ts,tsx} : Separate testable services from configuration files; follow the pattern of `realtimeClient.server.ts` (testable service) and `realtimeClientGlobal.server.ts` (configuration) in the webapp
Applied to files:
apps/webapp/app/routes/admin._index.tsx
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger.config.ts : Configure OpenTelemetry instrumentations and exporters in trigger.config.ts for enhanced logging
Applied to files:
apps/webapp/app/routes/admin._index.tsx
📚 Learning: 2025-09-02T11:27:36.336Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 2463
File: apps/webapp/app/routes/_app.github.callback/route.tsx:33-44
Timestamp: 2025-09-02T11:27:36.336Z
Learning: In the GitHub App installation callback flow in apps/webapp/app/routes/_app.github.callback/route.tsx, the install session cookie is not cleared after use due to interface limitations with redirectWithSuccessMessage/redirectWithErrorMessage not supporting custom headers. The maintainer accepts this design as the 1-hour cookie expiration provides sufficient protection against replay attacks.
Applied to files:
apps/webapp/app/routes/admin._index.tsx
📚 Learning: 2026-02-03T18:27:40.429Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2994
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx:553-555
Timestamp: 2026-02-03T18:27:40.429Z
Learning: In apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx, the menu buttons (e.g., Edit with PencilSquareIcon) in the TableCellMenu are intentionally icon-only with no text labels as a compact UI pattern. This is a deliberate design choice for this route; preserve the icon-only behavior for consistency in this file.
Applied to files:
apps/webapp/app/routes/admin._index.tsxapps/webapp/app/routes/admin.impersonate.tsx
📚 Learning: 2026-02-11T16:37:32.429Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3019
File: apps/webapp/app/components/primitives/charts/Card.tsx:26-30
Timestamp: 2026-02-11T16:37:32.429Z
Learning: In projects using react-grid-layout, avoid relying on drag-handle class to imply draggability. Ensure drag-handle elements only affect dragging when the parent grid item is configured draggable in the layout; conditionally apply cursor styles based on the draggable prop. This improves correctness and accessibility.
Applied to files:
apps/webapp/app/routes/admin._index.tsxapps/webapp/app/routes/admin.impersonate.tsx
🧬 Code graph analysis (2)
apps/webapp/app/routes/admin._index.tsx (4)
apps/webapp/app/routes/admin.impersonate.tsx (1)
loader(18-46)apps/webapp/app/routes/admin.orgs.tsx (1)
loader(31-44)apps/webapp/app/components/primitives/Table.tsx (1)
TableCell(235-317)apps/webapp/app/components/primitives/CopyableText.tsx (1)
CopyableText(8-98)
apps/webapp/app/routes/admin.impersonate.tsx (3)
apps/webapp/app/services/session.server.ts (1)
requireUser(39-62)apps/webapp/app/models/admin.server.ts (1)
redirectWithImpersonation(215-242)apps/webapp/app/services/impersonation.server.ts (1)
validateAndConsumeImpersonationToken(94-144)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (26)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: sdk-compat / Deno Runtime
- GitHub Check: sdk-compat / Cloudflare Workers
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: sdk-compat / Node.js 22.12 (ubuntu-latest)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: sdk-compat / Node.js 20.20 (ubuntu-latest)
- GitHub Check: sdk-compat / Bun Runtime
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
apps/webapp/app/services/impersonation.server.ts (1)
49-89: LGTM — Token generation and signing logic is sound.The JWT-based one-time token approach with HS256 signing, 5-minute expiry, and issuer/audience claims is well-structured for CSRF protection. The use of
SESSION_SECRETas the signing key is acceptable given it's already a high-entropy secret, though a dedicatedIMPERSONATION_TOKEN_SECRETenv var would provide better separation of concerns if this is ever rotated independently.apps/webapp/app/routes/admin._index.tsx (1)
121-137: Clean extraction of impersonation to a dedicated route.The form now correctly posts to
/admin/impersonatewithreloadDocument, properly separating concerns. The hiddenidfield and submit button are preserved. This aligns well with the newadmin.impersonate.tsxroute.apps/webapp/app/routes/admin.impersonate.tsx (2)
18-46: Solid security flow: admin check before token consumption prevents abuse.The ordering — validate admin status before consuming the one-time token — is correct and prevents non-admin users from burning valid tokens. The userId cross-check on line 40 (
validatedUserId !== impersonateUserId) prevents parameter tampering. Well done.
48-57: POST action path lacks token-based CSRF protection.The POST action relies solely on
sameSite: "lax"cookies. Whilelaxprevents cross-site POST CSRF (sincelaxonly sends cookies on top-level navigations with safe HTTP methods like GET), this is adequate here because the form is only rendered within the authenticated admin dashboard. Remix's form submission convention combined withsameSite: "lax"provides sufficient protection for this use case.apps/webapp/app/routes/api.v1.plain.customer-cards.ts (3)
33-45: Good defensive header sanitization.The
sanitizeHeadersutility correctly strips the configurable auth header and cookies before logging, preventing credential leakage. The use ofenv.PLAIN_CUSTOMER_CARDS_HEADERSfor determining which header carries the token is a nice touch for configurability.
48-72: Timing-safe authentication implementation is correct.The
timingSafeEqualusage with the length pre-check is the standard Node.js pattern for constant-time secret comparison. The configurable header name and support for bothBearerprefix and raw token formats are pragmatic.
184-192: Account-details card TTL of 15 seconds is appropriate.Since this card embeds a one-time impersonation token (5-minute expiry), the short 15-second
timeToLiveSecondsensures Plain will re-fetch and get a fresh token on each view, preventing stale token issues. Good alignment between token expiry and card TTL.
🤖 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/webapp/app/routes/admin._index.tsx`:
- Line 44: Remove the blanket "as any" cast on the useTypedLoaderData call so
the loader's return type is preserved (replace const { users, filters, page,
pageCount } = useTypedLoaderData<typeof loader>() as any; with a direct typed
useTypedLoaderData<typeof loader>() call) and also remove the manual type
annotations later in the component (the explicit types you added for users and
filters at the usages around the previous lines 85 and 92) so TypeScript can
infer types from typedjson/adminGetUsers; if a type error appears, fix the
return type in adminGetUsers (or the typedjson wrapper) so it matches the
component's expected shape.
In `@apps/webapp/app/routes/api.v1.plain.customer-cards.ts`:
- Around line 107-113: The logs currently include PII (customer.email) via the
full request body and explicit email variables; remove or redact the email
before logging by either omitting the body from the logger.warn call or
replacing it with a sanitized/redacted copy (e.g., construct redactedBody = {
...body, customer: { ...body.customer, email: "[REDACTED]" } }) and log that
instead; similarly update the user-not-found/error path where email is logged
(references: parsed.success, logger.warn, parsed.error.errors, body, and any
logger calls that reference email) so no raw email value is written to logs—use
redacted value or omit the field entirely.
- Around line 376-398: The empty projects card object added to cards (key:
"projects") has inconsistent indentation around the properties and nested
arrays; reformat the object literal containing key, timeToLiveSeconds,
components and its nested uiComponent.container / uiComponent.text /
uiComponent.spacer calls so all properties align consistently (apply Prettier or
your project's formatter) — locate the block that pushes into cards (the object
using uiComponent.container, uiComponent.text, uiComponent.spacer) and re-indent
the properties and nested content arrays to match surrounding code style.
---
Nitpick comments:
In `@apps/webapp/app/routes/admin.impersonate.tsx`:
- Around line 10-16: handleImpersonationRequest currently calls requireUser and
then calls redirectWithImpersonation which itself calls requireUser again,
causing duplicate DB lookups; fix by changing redirectWithImpersonation to
accept an optional authenticated user (e.g., add a parameter like currentUser)
or create a new overload, then pass the already-fetched user from
handleImpersonationRequest into redirectWithImpersonation (or inline the
impersonation check into handleImpersonationRequest) and remove the second
requireUser call inside redirectWithImpersonation so only one DB lookup occurs;
update all call sites (including those at lines referenced) to use the new
signature or inline behavior and keep permission check using user.admin from the
single fetched user.
In `@apps/webapp/app/routes/api.v1.plain.customer-cards.ts`:
- Around line 119-166: The two prisma.user.findUnique calls duplicate the same
include graph; extract the shared include into a constant (e.g., userInclude or
USER_INCLUDE) and build a single where object based on customer.externalId or
customer.email (e.g., const where = customer.externalId ? { id:
customer.externalId } : customer.email ? { email: customer.email } : null) then
call prisma.user.findUnique({ where, include: userInclude }) only when where is
non-null; update references to orgMemberships/organization/projects remain
unchanged.
In `@apps/webapp/app/services/impersonation.server.ts`:
- Around line 111-131: The replay-protection currently uses the full JWT string
as the Redis key; change it to use a short unique identifier by adding a jti
when signing in generateImpersonationToken and then using payload.jti as the
Redis key in validateAndConsumeImpersonationToken (instead of token). Ensure
getImpersonationTokenRedisClient() calls still use
IMPERSONATION_TOKEN_EXPIRY_SECONDS, and add a defensive check in
validateAndConsumeImpersonationToken to reject or derive a stable short key
(e.g., compute a hash) if payload.jti is missing so behavior remains safe when
older tokens without jti are encountered.
2 participants
Changes
Testing
Setup
Environment Variables
Set these in your
.envfile:Configure Plain Customer Cards
https://your-ngrok-url/api/v1/plain/customer-cardsor your deployed URL)Option 1: Test with cURL
Replace:
test@example.comwith an actual user email from your databaseyour-card-keywith the value you set forPLAIN_CUSTOMER_CARDS_KEY(or use"account-details"if using the default)Option 2: Test with Plain Account