fix(webapp): let an admin switch impersonation target without stopping first - #4576
fix(webapp): let an admin switch impersonation target without stopping first#4576isshaddad wants to merge 1 commit into
Conversation
…g first getUserId resolves to the impersonated user id while impersonating, by design, so requireUser answers "who is this request acting as". Every impersonation entry point gated on it, so while impersonating a customer user.admin was that customer's flag and starting on a second target silently redirected to / — you had to stop impersonating first. - New getRealUser resolves the authenticated user, ignoring the impersonation cookie, and applies the same session controls getUserId does for the real user (SSO revalidation and the auto-logout deadline) so this can't become a way around them. - redirectWithImpersonation gates on it rather than taking a user from the caller, and attributes the audit row to the real admin. - The route moves to admin_.impersonate.tsx to opt out of the admin layout, whose requireSuper gate resolves the same impersonated identity. It checks canSuper() against the real admin directly, since the raw User.admin column only equals canSuper() in the OSS fallback. - Unauthenticated requests redirect to login carrying the original URL, so the impersonation link survives the round trip. - The view-as-user flag is cleared when the target changes; it is scoped to a single impersonation session. Switching straight between targets never passes through clearImpersonation, so a STOP for the previous target is written alongside the new START, both in one transaction with explicit timestamps — Postgres now() is the transaction timestamp, so the default would stamp both rows identically.
|
WalkthroughThe change adds 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Observability mapAs of 19/100 over 417 measured of 433 entry points (base 19, no change) What this PR changed
1 entries removed FIX FIRST
AUDIT 3 of 50 sensitive mutations record an actor. 47 without one. What the score is made ofThe score and findings here are report-only and never gate the merge. Separately, a required test suite keeps this tool's symbol and route lists in sync with the code they name, and can fail a pull request that renames or removes a symbol they reference, or that adds the first route with a segment they anticipate. Each failure names the list to edit. The rules and their reasons: internal-packages/observability-map/README.md. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| import { | ||
| redirect, | ||
| type ActionFunctionArgs, | ||
| type LoaderFunctionArgs, | ||
| } from "@remix-run/server-runtime"; | ||
| import { z } from "zod"; | ||
| import { redirectWithImpersonation } from "~/models/admin.server"; | ||
| import { authenticator } from "~/services/auth.server"; | ||
| import { rbac } from "~/services/rbac.server"; | ||
| import { getRealUser } from "~/services/session.server"; | ||
| import { validateAndConsumeImpersonationToken } from "~/services/impersonation.server"; | ||
| import { logger } from "~/services/logger.server"; | ||
| import { sanitizeRedirectPath } from "~/utils"; |
There was a problem hiding this comment.
🟡 Release notes will not mention this webapp fix
This change only touches server code under apps/webapp/ but ships without the required release-note entry in .server-changes/, so the fix will be missing from user-facing release notes.
Impact: Users reading the release notes will not see that impersonation switching was fixed.
Repository rule: server-only changes require a `.server-changes/` file
AGENTS.md ("Changesets and Server Changes") and CONTRIBUTING.md ("Adding server changes") both state that a PR changing only server components (apps/webapp/, apps/supervisor/, …) with no package changes must add a .server-changes/ markdown file with area and type frontmatter. This PR modifies only apps/webapp/app/** and adds no such file (the directory contains only pre-existing entries).
Prompt for agents
The repository requires a `.server-changes/` entry for PRs that change only server components (see AGENTS.md "Changesets and Server Changes" and CONTRIBUTING.md "Adding server changes"). This PR changes only apps/webapp. Add a new markdown file under .server-changes/ with frontmatter `area: webapp` and `type: fix`, and a one-line, user-facing description of the behaviour change (an admin can switch who they are impersonating without stopping first), written for users rather than maintainers.
Was this helpful? React with 👍 or 👎 to provide feedback.
| try { | ||
| await prismaClient.impersonationAuditLog.create({ | ||
| data: { | ||
| action: "START", | ||
| adminId: user.id, | ||
| targetId: userId, | ||
| ipAddress, | ||
| }, | ||
| await $transaction(prismaClient, "startImpersonationAudit", async (tx) => { | ||
| if (previousTargetId && previousTargetId !== userId) { | ||
| await tx.impersonationAuditLog.create({ | ||
| data: { | ||
| action: "STOP", | ||
| adminId: admin.id, | ||
| targetId: previousTargetId, | ||
| ipAddress, | ||
| createdAt: closedAt, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| await tx.impersonationAuditLog.create({ | ||
| data: { | ||
| action: "START", | ||
| adminId: admin.id, | ||
| targetId: userId, | ||
| ipAddress, | ||
| createdAt: startedAt, | ||
| }, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🟡 Switching impersonation target can leave no record at all when one record fails to save
Both audit entries are now written together in a single all-or-nothing database write ($transaction at apps/webapp/app/models/admin.server.ts:254) while any failure is still ignored and impersonation continues, so one failing entry now also destroys the entry for the new session that would previously have been saved.
Impact: In the failure case an admin ends up acting as another user with no trace in the audit trail, where previously a record was still kept.
Mechanism: atomic write plus swallowed error removes the previously independent START row
Before this change, redirectWithImpersonation created only the START row; a failure there was logged and impersonation still proceeded.
Now, when previousTargetId is set and differs from the new target, a STOP row and the START row are created inside one transaction (apps/webapp/app/models/admin.server.ts:254-275). If the STOP insert fails — the most realistic case is a foreign-key violation because the previously impersonated user row has since been deleted, but any transient error qualifies — the whole transaction rolls back, so the START row is lost too. The surrounding try/catch (apps/webapp/app/models/admin.server.ts:277-284) only logs, and the cookie is then set at apps/webapp/app/models/admin.server.ts:286, so impersonation starts with zero audit rows.
The in-code comment claims the transaction prevents "an admin acting as someone with no record of it", but because the error is swallowed rather than aborting the impersonation, the transaction actually widens that window instead of closing it. Either the START row should be attempted separately when the STOP write fails, or the failure should abort the impersonation.
Prompt for agents
In apps/webapp/app/models/admin.server.ts, redirectWithImpersonation now writes the STOP row for the previous target and the START row for the new target inside one $transaction, but the surrounding try/catch swallows any error and impersonation proceeds anyway. That means a failure in the STOP insert (for example a foreign-key violation because the previously impersonated user has since been deleted, or any transient error) now also rolls back the START row, leaving an active impersonation with no audit record — the exact outcome the transaction comment says it prevents. Consider either making the audit failure fatal (do not set the impersonation cookie if the audit write fails), or falling back to writing the START row on its own when the combined transaction fails, so the new session is always recorded.
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
apps/webapp/app/models/admin.server.ts (1)
250-251: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚖️ Poor tradeoffConfirm the audit ordering assumption for concurrent switches.
closedAtisstartedAt - 1ms. TheSTOProw for the previous target is therefore stamped 1 ms in the past. If a previousSTARTrow was written less than 1 ms earlier, the newSTOPsorts before thatSTART, and an audit view ordered bycreatedAtshows the stop before the start it closes.Two HTTP requests within the same millisecond are unlikely but not impossible. If strict ordering matters for the audit trail, derive
closedAtfrom the previousSTARTtimestamp, or order audit views by a monotonic sequence column instead ofcreatedAt.Also applies to: 255-275
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8208f523-b3fc-475d-b3ca-438e9dff508b
📒 Files selected for processing (6)
apps/webapp/app/models/admin.server.tsapps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsxapps/webapp/app/routes/admin.impersonate.tsxapps/webapp/app/routes/admin_.impersonate.tsxapps/webapp/app/services/impersonation.server.tsapps/webapp/app/services/session.server.ts
💤 Files with no reviewable changes (1)
- apps/webapp/app/routes/admin.impersonate.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (7)
- GitHub Check: report
- GitHub Check: code-quality / code-quality
- GitHub Check: audit
- GitHub Check: audit
- GitHub Check: check-vouch
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (actions)
🧰 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}: Prefer static imports over dynamicimport(); use dynamic imports only for unresolvable circular dependencies, genuine performance code splitting, or conditional runtime loading.
Import Trigger.dev tasks from@trigger.dev/sdk; never use@trigger.dev/sdk/v3or deprecatedclient.defineJob.
Add agentcrumbs while writing code using approved namespaces; mark lines with//@Crumbsor blocks with `// `#region` `@crumbs, and strip them before merging.
Files:
apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsxapps/webapp/app/services/impersonation.server.tsapps/webapp/app/routes/admin_.impersonate.tsxapps/webapp/app/services/session.server.tsapps/webapp/app/models/admin.server.ts
{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/routes/_app.@.orgs.$organizationSlug.$.tsxapps/webapp/app/services/impersonation.server.tsapps/webapp/app/routes/admin_.impersonate.tsxapps/webapp/app/services/session.server.tsapps/webapp/app/models/admin.server.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsxapps/webapp/app/services/impersonation.server.tsapps/webapp/app/routes/admin_.impersonate.tsxapps/webapp/app/services/session.server.tsapps/webapp/app/models/admin.server.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: Access environment variables through theenvexport ofenv.server.tsinstead of directly accessingprocess.env
Use subpath exports from@trigger.dev/corepackage instead of importing from the root@trigger.dev/corepathDo not reintroduce the removed v1 execution path;
RunEngineVersion.V1branches may only reject or finalize gracefully so v3 clients receive a clean 4xx, never a 5xx.
Files:
apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsxapps/webapp/app/services/impersonation.server.tsapps/webapp/app/routes/admin_.impersonate.tsxapps/webapp/app/services/session.server.tsapps/webapp/app/models/admin.server.ts
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
For apps, use
typecheckfor verification and never usebuildas the correctness check.
Files:
apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsxapps/webapp/app/services/impersonation.server.tsapps/webapp/app/routes/admin_.impersonate.tsxapps/webapp/app/services/session.server.tsapps/webapp/app/models/admin.server.ts
apps/webapp/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
apps/webapp/app/**/*.{ts,tsx}: For dashboard changes, visually verify the running Remix app with Chrome DevTools MCP, using snapshots, screenshots, interaction, and console-message checks as appropriate.
UseuseCallbackanduseMemoonly for context provider values, expensive derived data used as a dependency, or stable references required by dependency arrays; do not wrap ordinary event handlers or trivial computations.
Use named constants for sentinel or placeholder values instead of scattering raw string literals across comparisons.
Files:
apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsxapps/webapp/app/services/impersonation.server.tsapps/webapp/app/routes/admin_.impersonate.tsxapps/webapp/app/services/session.server.tsapps/webapp/app/models/admin.server.ts
**/*.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/services/session.server.tsapps/webapp/app/models/admin.server.ts
apps/webapp/app/**/*.ts
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
apps/webapp/app/**/*.ts: Never userequest.signalto detect client disconnects. UsegetRequestAbortSignal()fromapp/services/httpAsyncStorage.server.ts, which is wired to Express response close events.
Access environment variables through theenvexport fromapp/env.server.ts; never useprocess.envdirectly.
Always use PrismafindFirstinstead offindUnique.
Always use the$transactionhelper from~/db.server, never callprisma.$transactionor$replica.$transactiondirectly. Pass isolation levels as strings, useSerializablefor correctness-critical read-then-write invariants, and guard possibly undefined helper results when a definite value is required.
Files:
apps/webapp/app/services/impersonation.server.tsapps/webapp/app/services/session.server.tsapps/webapp/app/models/admin.server.ts
🧠 Learnings (21)
📚 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/_app.@.orgs.$organizationSlug.$.tsxapps/webapp/app/routes/admin_.impersonate.tsx
📚 Learning: 2026-07-22T11:16:06.546Z
Learnt from: samejr
Repo: triggerdotdev/trigger.dev PR: 4332
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.regions/route.tsx:166-168
Timestamp: 2026-07-22T11:16:06.546Z
Learning: In the Trigger.dev web app, copy-interaction accessibility (keyboard and touch behavior for the copy affordance) is owned by the shared `CopyableText` `icon-right` primitive. When reviewing route-level code (e.g., admin debug panels and runs tables) that intentionally reuses this pattern, avoid suggesting divergent call-site-only accessibility fixes; instead, route any accessibility changes back to a holistic update of the `CopyableText` `icon-right` implementation so all reuse sites benefit consistently.
Applied to files:
apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.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/_app.@.orgs.$organizationSlug.$.tsxapps/webapp/app/routes/admin_.impersonate.tsx
📚 Learning: 2026-07-28T21:57:20.061Z
Learnt from: samejr
Repo: triggerdotdev/trigger.dev PR: 4411
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.team/route.tsx:818-843
Timestamp: 2026-07-28T21:57:20.061Z
Learning: When using Radix UI `DialogClose` with `asChild` (e.g., Trigger.dev dashboard components), note that it injects `type="button"` into its child via `Slot`. If the child is a local `Button` that forwards its `type` prop to the native `<button>`, then placing it inside a `<form>` will *not* submit unless you explicitly set `type="submit"` (or otherwise override the injected type / wire up submission behavior). Review form actions to ensure the intended submit vs non-submit behavior is preserved.
Applied to files:
apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsxapps/webapp/app/routes/admin_.impersonate.tsx
📚 Learning: 2026-03-22T13:26:12.060Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3244
File: apps/webapp/app/components/code/TextEditor.tsx:81-86
Timestamp: 2026-03-22T13:26:12.060Z
Learning: In the triggerdotdev/trigger.dev codebase, do not flag `navigator.clipboard.writeText(...)` calls for `missing-await`/`unhandled-promise` issues. These clipboard writes are intentionally invoked without `await` and without `catch` handlers across the project; keep that behavior consistent when reviewing TypeScript/TSX files (e.g., usages like in `apps/webapp/app/components/code/TextEditor.tsx`).
Applied to files:
apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsxapps/webapp/app/services/impersonation.server.tsapps/webapp/app/routes/admin_.impersonate.tsxapps/webapp/app/services/session.server.tsapps/webapp/app/models/admin.server.ts
📚 Learning: 2026-03-22T19:24:14.403Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3187
File: apps/webapp/app/v3/services/alerts/deliverErrorGroupAlert.server.ts:200-204
Timestamp: 2026-03-22T19:24:14.403Z
Learning: In the triggerdotdev/trigger.dev codebase, webhook URLs are not expected to contain embedded credentials/secrets (e.g., fields like `ProjectAlertWebhookProperties` should only hold credential-free webhook endpoints). During code review, if you see logging or inclusion of raw webhook URLs in error messages, do not automatically treat it as a credential-leak/secrets-in-logs issue by default—first verify the URL does not contain embedded credentials (for example, no username/password in the URL, no obvious secret/token query params or fragments). If the URL is credential-free per this project’s conventions, allow the logging.
Applied to files:
apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsxapps/webapp/app/services/impersonation.server.tsapps/webapp/app/routes/admin_.impersonate.tsxapps/webapp/app/services/session.server.tsapps/webapp/app/models/admin.server.ts
📚 Learning: 2026-05-18T08:21:27.694Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3632
File: apps/webapp/sentry.server.ts:4-21
Timestamp: 2026-05-18T08:21:27.694Z
Learning: When handling Prisma error P1001 ("Can't reach database server") in TypeScript, don’t assume a single error shape. Prisma can surface P1001 via two different error classes/fields: `PrismaClientKnownRequestError` exposes it as `err.code === "P1001"` (common during mid-query connection drops), while `PrismaClientInitializationError` exposes it as `err.errorCode === "P1001"` (common on client startup failure). Therefore, predicates should use `err.code === "P1001" || err.errorCode === "P1001"`. Do not flag `err.code === "P1001"` as “unreachable/never matches,” as it is expected in production.
Applied to files:
apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsxapps/webapp/app/services/impersonation.server.tsapps/webapp/app/routes/admin_.impersonate.tsxapps/webapp/app/services/session.server.tsapps/webapp/app/models/admin.server.ts
📚 Learning: 2026-05-18T08:21:27.694Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3632
File: apps/webapp/sentry.server.ts:4-21
Timestamp: 2026-05-18T08:21:27.694Z
Learning: When handling Prisma errors for P1001 ("Can't reach database server"), do not assume it only appears under a single property name. Prisma may surface P1001 via either `PrismaClientKnownRequestError` (`err.code === "P1001"`, e.g., mid-query connection drops) or `PrismaClientInitializationError` (`err.errorCode === "P1001"`, e.g., client startup connection failure). To reliably detect the condition, check `err.code === "P1001" || err.errorCode === "P1001"`, and avoid review rules that would incorrectly flag `err.code === "P1001"` as unreachable/never-matching.
Applied to files:
apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsxapps/webapp/app/services/impersonation.server.tsapps/webapp/app/routes/admin_.impersonate.tsxapps/webapp/app/services/session.server.tsapps/webapp/app/models/admin.server.ts
📚 Learning: 2026-06-13T19:53:13.759Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3937
File: packages/trigger-sdk/skills/realtime-and-frontend/SKILL.md:258-260
Timestamp: 2026-06-13T19:53:13.759Z
Learning: When reviewing code that uses `trigger.dev/react-hooks`’s `useRealtimeRun`, preserve the call signature where the first argument is the full realtime handle object (not `handle.id`). This is intentional to maintain type-safety and is consistent with the official docs; do not suggest changing the first argument from the handle object to `handle.id`.
Applied to files:
apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsxapps/webapp/app/services/impersonation.server.tsapps/webapp/app/routes/admin_.impersonate.tsxapps/webapp/app/services/session.server.tsapps/webapp/app/models/admin.server.ts
📚 Learning: 2026-06-17T17:13:49.929Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3948
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.bulk-actions.$bulkActionParam/route.tsx:48-62
Timestamp: 2026-06-17T17:13:49.929Z
Learning: In triggerdotdev/trigger.dev, within `dashboardLoader`/`dashboardAction` (or similar context resolver code) whenever you resolve an organization ID from an organization slug for RBAC/enterprise authorization scope, always read from the primary Prisma client (`prisma`), not `$replica`. Using `$replica` can hit replica-lag and cause the RBAC lookup/authorization to run without the correct org scope (bypassing intended role enforcement). Implement the slug→org lookup with `prisma.organization.findFirst(...)` (or equivalent primary-client query) and add an inline comment documenting why the primary client is required (replica lag could lead to unscoped RBAC checks).
Applied to files:
apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsxapps/webapp/app/services/impersonation.server.tsapps/webapp/app/routes/admin_.impersonate.tsxapps/webapp/app/services/session.server.tsapps/webapp/app/models/admin.server.ts
📚 Learning: 2026-06-23T13:04:21.413Z
Learnt from: carderne
Repo: triggerdotdev/trigger.dev PR: 4023
File: apps/webapp/app/services/upsertBranch.server.ts:14-18
Timestamp: 2026-06-23T13:04:21.413Z
Learning: In TypeScript, it’s valid to `import { type X }` and then use `typeof X` in a type-only position, e.g. `type Alias = z.infer<typeof X>`. The `type` modifier suppresses the runtime import, but the type checker still has the full exported type so `z.infer<typeof X>` can resolve correctly. In code reviews, don’t flag this as a TypeScript compile error as long as `typeof X` is used in a type context (e.g., with `z.infer`, `type` aliases, generics), not as a runtime value.
Applied to files:
apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsxapps/webapp/app/services/impersonation.server.tsapps/webapp/app/routes/admin_.impersonate.tsxapps/webapp/app/services/session.server.tsapps/webapp/app/models/admin.server.ts
📚 Learning: 2026-05-08T21:00:20.973Z
Learnt from: samejr
Repo: triggerdotdev/trigger.dev PR: 3538
File: apps/webapp/app/components/primitives/Resizable.tsx:60-78
Timestamp: 2026-05-08T21:00:20.973Z
Learning: In the triggerdotdev/trigger.dev codebase, treat Zod as a boundary validation tool (API handlers, request/response validation, and storage/DB read/write validation), not as inline render-time validation inside React components/primitive UI code. For render-time guards, prefer small manual type-narrowing checks (e.g., a short predicate like ~10–20 lines) over importing Zod into UI primitives, to avoid per-render schema-parse overhead and unnecessary abstraction. Use the manual guard approach unless you truly need schema validation at a boundary; only then introduce Zod.
Applied to files:
apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsxapps/webapp/app/routes/admin_.impersonate.tsx
📚 Learning: 2026-06-25T18:21:55.847Z
Learnt from: carderne
Repo: triggerdotdev/trigger.dev PR: 4039
File: apps/webapp/app/routes/invite-resend.tsx:0-0
Timestamp: 2026-06-25T18:21:55.847Z
Learning: In the triggerdotdev/trigger.dev Zod 4 migration, avoid importing from the root package `conform-to/zod` in webapp code. It can resolve to the Zod 3 build and may crash at module load under Zod 4. When reviewing TypeScript/TSX files in `apps/webapp`, prefer importing from the Zod 4 subpath `conform-to/zod/v4` for Zod 4-compatible schemas/types.
Applied to files:
apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsxapps/webapp/app/routes/admin_.impersonate.tsx
📚 Learning: 2026-05-12T21:04:05.815Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3542
File: apps/webapp/app/components/sessions/v1/SessionStatus.tsx:1-3
Timestamp: 2026-05-12T21:04:05.815Z
Learning: In this Remix + TypeScript codebase, do not flag a server/client boundary violation when a file imports only types from a module matching `*.server`.
Specifically, it’s safe to import types using `import type { Foo } from "*.server"` or `import { type Foo } from "*.server"` because TypeScript erases type-only imports at compile time and they emit no JavaScript, so they won’t cross the Remix server/client bundle boundary.
Only raise the boundary concern for value imports (e.g., `import { Foo }` without `type`, or `import Foo`), since those produce JavaScript output.
Applied to files:
apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsxapps/webapp/app/services/impersonation.server.tsapps/webapp/app/routes/admin_.impersonate.tsxapps/webapp/app/services/session.server.tsapps/webapp/app/models/admin.server.ts
📚 Learning: 2026-06-25T18:21:51.905Z
Learnt from: carderne
Repo: triggerdotdev/trigger.dev PR: 4039
File: apps/webapp/app/routes/invite-revoke.tsx:0-0
Timestamp: 2026-06-25T18:21:51.905Z
Learning: During the Zod v4 migration in the triggerdotdev/trigger.dev webapp, ensure any imports from `conform-to/zod` use the Zod-4 subpath: `conform-to/zod/v4` (e.g., `import { parseWithZod } from "conform-to/zod/v4"`). Do not import from the package root `conform-to/zod`, because it is the Zod 3 implementation and may load Zod-3-only symbols (e.g., `ZodBranded`, `ZodEffects`), which can throw at module load (notably with `zod4.4.3`). This should be enforced across `apps/webapp/**/*` where helpers like `parseWithZod` and `conformZodMessage` are used.
Applied to files:
apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsxapps/webapp/app/services/impersonation.server.tsapps/webapp/app/routes/admin_.impersonate.tsxapps/webapp/app/services/session.server.tsapps/webapp/app/models/admin.server.ts
📚 Learning: 2026-07-03T17:10:21.498Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 4148
File: apps/webapp/app/models/orgMember.server.ts:149-168
Timestamp: 2026-07-03T17:10:21.498Z
Learning: In triggerdotdev/trigger.dev, `User.email` (Prisma schema: `internal-packages/database/prisma/schema.prisma`) currently does NOT use `citext` and does NOT have a `lower(email)` functional unique index. Therefore, do not introduce Prisma queries like `where: { email: { equals: <value>, mode: "insensitive" } }` (or any case-insensitive lookup) against `User.email`, because it can force sequential scans of the `users` table under load. During review, ensure email is normalized (e.g., lowercased/trimmed) before both writes and subsequent lookups, and if true case-insensitive behavior/uniqueness is required, implement it via a separate app-wide migration (e.g., switch to `citext` and/or add a functional unique index with backfill) rather than bolting it onto individual feature PRs.
Applied to files:
apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsxapps/webapp/app/services/impersonation.server.tsapps/webapp/app/routes/admin_.impersonate.tsxapps/webapp/app/services/session.server.tsapps/webapp/app/models/admin.server.ts
📚 Learning: 2026-06-25T18:21:54.729Z
Learnt from: carderne
Repo: triggerdotdev/trigger.dev PR: 4039
File: apps/webapp/app/routes/confirm-basic-details.tsx:0-0
Timestamp: 2026-06-25T18:21:54.729Z
Learning: For Remix + TypeScript files that use Conform v1 (conform-to/react) and its getInputProps helper, when you intend to suppress the helper-provided default value for non-checkbox/non-radio inputs (e.g., hidden inputs managed via an explicit value prop), use the Conform v1 option key `value: false`. Do not recommend `defaultValue: false` here, because `defaultValue` is not a valid option key for these input types in Conform v1 typings.
Applied to files:
apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsxapps/webapp/app/routes/admin_.impersonate.tsx
📚 Learning: 2026-03-26T09:02:07.973Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 3274
File: apps/webapp/app/services/runsReplicationService.server.ts:922-924
Timestamp: 2026-03-26T09:02:07.973Z
Learning: When parsing Trigger.dev task run annotations in server-side services, keep `TaskRun.annotations` strictly conforming to the `RunAnnotations` schema from `trigger.dev/core/v3`. If the code already uses `RunAnnotations.safeParse` (e.g., in a `#parseAnnotations` helper), treat that as intentional/necessary for atomic, schema-accurate annotation handling. Do not recommend relaxing the annotation payload schema or using a permissive “passthrough” parse path, since the annotations are expected to be written atomically in one operation and should not contain partial/legacy payloads that would require a looser parser.
Applied to files:
apps/webapp/app/services/impersonation.server.tsapps/webapp/app/services/session.server.ts
📚 Learning: 2026-05-05T09:38:02.512Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3523
File: apps/webapp/app/routes/api.v3.batches.ts:178-181
Timestamp: 2026-05-05T09:38:02.512Z
Learning: When reviewing code that catches `ServiceValidationError` in `*.server.ts` files, do not blindly forward `error.status` to HTTP responses, because SVEs may be thrown with non-default statuses (e.g., 400/500) and forwarding them can cause client-visible behavioral regressions (e.g., surfacing 500s to clients). Prefer a safe default response status of `error.status ?? 422`, but only after confirming via the reachable call graph that the caught `ServiceValidationError` instances are expected to carry those non-default statuses; otherwise, normalize to `422` to avoid unexpected client-visible 5xx behavior.
Applied to files:
apps/webapp/app/services/impersonation.server.tsapps/webapp/app/services/session.server.tsapps/webapp/app/models/admin.server.ts
📚 Learning: 2026-06-04T18:16:35.386Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 3836
File: apps/supervisor/src/backpressure/backpressureMonitor.ts:3-5
Timestamp: 2026-06-04T18:16:35.386Z
Learning: When reviewing TypeScript in this repo, apply the rule “prefer type aliases over interfaces” only to data/object shapes and union/intersection type modeling. If an interface is being used as a behavioral contract for collaborators to implement (e.g., method-shape interfaces that define required behavior, such as `BackpressureLogger` / `BackpressureSignalSource` in `apps/supervisor/src/backpressure/backpressureMonitor.ts`), keep it as an `interface` and do not flag it as a type-alias-vs-interface violation.
Applied to files:
apps/webapp/app/services/impersonation.server.tsapps/webapp/app/services/session.server.tsapps/webapp/app/models/admin.server.ts
📚 Learning: 2026-06-09T17:58:04.699Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 3879
File: apps/webapp/app/models/vercelIntegration.server.ts:619-630
Timestamp: 2026-06-09T17:58:04.699Z
Learning: In this codebase, outbound raw `fetch` calls should typically rely on Node/undici’s default request timeout (about ~300s) rather than adding a per-call `AbortController` + `setTimeout` wrapper inside individual functions (e.g. in files like `apps/webapp/app/models/vercelIntegration.server.ts`). During code review, do not flag the absence of a per-call timeout on a single `fetch` as an issue; if per-call timeouts are needed, they should be implemented via a codebase-wide convention (e.g., a shared fetch wrapper or documented pattern) rather than ad-hoc per-function changes.
Applied to files:
apps/webapp/app/services/impersonation.server.tsapps/webapp/app/services/session.server.tsapps/webapp/app/models/admin.server.ts
🔇 Additional comments (7)
apps/webapp/app/services/session.server.ts (1)
2-2: LGTM!Also applies to: 128-164
apps/webapp/app/models/admin.server.ts (2)
2-2: LGTM!Also applies to: 12-12
254-254: 🗄️ Data Integrity & IntegrationNo change required.
$transaction(prismaClient, "startImpersonationAudit", fn)matches the helper signature and uses the database-configured isolation level when no options are provided.apps/webapp/app/routes/admin_.impersonate.tsx (2)
1-13: LGTM!Also applies to: 15-56, 71-99
57-58: 🔒 Security & Privacy
userIdis honored byauthenticateSession. The contract requires the caller-provideduserId; the controller forwards it unchanged, and the fallback builds the user and ability fromcontext.userId.admin.idis therefore used forcanSuper().> Likely an incorrect or invalid review comment.apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsx (1)
64-64: LGTM!Also applies to: 151-151
apps/webapp/app/services/impersonation.server.ts (1)
48-55: LGTM!
| * `verifiedAdmin` exists only so tests can supply an admin without a session cookie. Production | ||
| * callers must not pass it: passing a `requireUser` result is exactly the bug described above. | ||
| */ | ||
| export async function redirectWithImpersonation( | ||
| request: Request, | ||
| userId: string, | ||
| path: string, | ||
| currentUser?: { id: string; admin: boolean }, | ||
| verifiedAdmin?: { id: string; admin: boolean }, | ||
| prismaClient: PrismaClientOrTransaction = prisma | ||
| ) { | ||
| const user = currentUser ?? (await requireUser(request)); | ||
| if (!user.admin) { | ||
| const admin = verifiedAdmin ?? (await getRealUser(request, prismaClient)); | ||
| if (!admin?.admin) { | ||
| throw new Error("Unauthorized"); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Restrict the verifiedAdmin authentication bypass.
verifiedAdmin skips getRealUser completely. The only protection is the docstring at Lines 221-222. startImpersonation also forwards this parameter as a public optional argument (Lines 352-353, 370), so both exported functions accept a caller-supplied admin identity that is never verified against the session.
A future caller can pass { id, admin: true } and start impersonation for any target without an authenticated admin session. The audit row then records that unverified id as the actor.
Prefer a test seam that cannot become an auth bypass. Two options:
- Inject the resolver instead of the result, so production always authenticates.
- Gate the override on a non-production environment flag read through
envfromapp/env.server.ts.
🔒 Option 1: inject the resolver
export async function redirectWithImpersonation(
request: Request,
userId: string,
path: string,
- verifiedAdmin?: { id: string; admin: boolean },
- prismaClient: PrismaClientOrTransaction = prisma
+ prismaClient: PrismaClientOrTransaction = prisma,
+ resolveAdmin: (
+ request: Request,
+ client: PrismaClientOrTransaction
+ ) => Promise<{ id: string; admin: boolean } | null> = getRealUser
) {
- const admin = verifiedAdmin ?? (await getRealUser(request, prismaClient));
+ const admin = await resolveAdmin(request, prismaClient);
if (!admin?.admin) {
throw new Error("Unauthorized");
}Update startImpersonation to forward the same seam.
Also applies to: 352-353
Source: Coding guidelines
| // Both rows are written in one transaction: as separate statements, a failure between them could | ||
| // start an impersonation whose only audit row is the STOP for the previous target — an admin | ||
| // acting as someone with no record of it. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
The catch block contradicts the stated audit guarantee.
The comment at Lines 243-245 states the transaction prevents "an admin acting as someone with no record of it". The catch block at Lines 277-284 logs the failure and then execution continues. Lines 286-290 set the impersonation cookie and redirect. If the audit transaction fails, impersonation starts with no START record.
Choose one behavior and make the code and the comment agree:
- Fail closed: rethrow after logging, so no impersonation begins without an audit row.
- Fail open: keep the catch and correct the comment to state that the transaction only prevents a partial
STOP-without-STARTtrail.
🔒 Fail-closed variant
} catch (error) {
logger.error("Failed to create impersonation audit log", {
error,
adminId: admin.id,
targetId: userId,
previousTargetId,
});
+ throw error;
}Also applies to: 277-284
| async function handleImpersonationRequest(request: Request, userId: string): Promise<Response> { | ||
| const admin = await requireRealAdmin(request); | ||
| if (!admin) { | ||
| return redirect("/"); | ||
| } | ||
| return redirectWithImpersonation(request, userId, "/"); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the SameSite attribute of the auth and impersonation session cookies.
rg -nP --type=ts -C 6 'createCookieSessionStorage|sameSite' apps/webapp/app | head -120
# Confirm the same-origin helper contract used by the sibling route.
fd -t f 'sameOriginNavigation.ts' apps/webapp | while IFS= read -r f; do
echo "=== $f ==="
cat -n "$f"
doneRepository: triggerdotdev/trigger.dev
Length of output: 11284
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '=== admin impersonation route ==='
fd -t f 'admin_.impersonate.tsx' apps/webapp | while IFS= read -r f; do
cat -n "$f"
done
printf '%s\n' '=== sibling action and token usage ==='
rg -n -C 12 'isSameOriginNavigation|impersonationToken|handleImpersonationRequest|redirectWithImpersonation' apps/webapp/app/routes apps/webapp/app/services apps/webapp/app/utils
printf '%s\n' '=== session cookie consumers ==='
rg -n -C 8 'sessionStorage|getSession\\(|__session|requireRealAdmin' apps/webapp/app/services apps/webapp/app/routes apps/webapp/app/utils | head -240Repository: triggerdotdev/trigger.dev
Length of output: 36844
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '=== impersonation redirect implementation ==='
rg -n -C 16 'function redirectWithImpersonation|const redirectWithImpersonation|export .*redirectWithImpersonation' apps/webapp/app/models apps/webapp/app/services
printf '%s\n' '=== authentication session storage ==='
rg -n -C 12 'sessionStorage|authenticator|createCookieSessionStorage|sameSite' apps/webapp/app/services/auth.server.ts apps/webapp/app/services/session.server.ts apps/webapp/app/services/sessionStorage.server.ts
printf '%s\n' '=== all admin impersonation entry points ==='
rg -n -C 8 'admin/impersonate|redirectWithImpersonation\\(' apps/webapp/appRepository: triggerdotdev/trigger.dev
Length of output: 16209
Add the same-origin check to the POST action.
The __session and __impersonate cookies use SameSite=Lax, so a normal cross-site form POST does not carry the authentication cookie. The action still permits same-site cross-origin requests and lacks the defense used by the sibling impersonation route. Reject non-same-origin requests before processing the form data.
| const payload = Object.fromEntries(await request.formData()); | ||
| const { id } = FormSchema.parse(payload); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Use safeParse and return a 400.
FormSchema.parse throws a ZodError for a missing or non-string id. Remix converts the uncaught throw into a 500 response. Malformed input must not produce a 5xx.
The parse also runs before requireRealAdmin, so an unauthenticated POST with a malformed body returns a 500 instead of the login redirect.
🐛 Proposed fix
const payload = Object.fromEntries(await request.formData());
- const { id } = FormSchema.parse(payload);
+ const parsed = FormSchema.safeParse(payload);
+ if (!parsed.success) {
+ return new Response("Bad request", { status: 400 });
+ }
- return handleImpersonationRequest(request, id);
+ return handleImpersonationRequest(request, parsed.data.id);
}1 participant
getUserIdresolves to the impersonated user id while impersonating — by design, sorequireUseranswers "who is this request acting as". Every impersonation entry point gated on it, so while impersonating one user,user.adminwas that user's flag and starting on a second target silently redirected to/. You had to stop impersonating first, then start again.Changes
getRealUserresolves the authenticated user, ignoring the impersonation cookie. It applies the same session controlsgetUserIdapplies to the real user — SSO revalidation and the auto-logout deadline — so it can't become a way around them.redirectWithImpersonationgates on it rather than taking a user from the caller, and attributes the audit row to the real admin. Previously that row would have named the impersonated user as the actor.admin_.impersonate.tsx, opting out of theadmin.tsxlayout, whoserequireSupergate resolves the same impersonated identity. Nesting left the fix depending on the router preferring the deepest redirect. It now checkscanSuper()against the real admin directly — the rawUser.admincolumn only equalscanSuper()in the OSS fallback, and a plugin is free to be stricter.Switching straight between targets never passes through
clearImpersonation, so aSTOPfor the previous target is written alongside the newSTART. Both go in one transaction with explicit timestamps: Postgresnow()is the transaction timestamp, so the default would stamp both rows identically and an audit view ordered by that column couldn't tell which came first.Testing
No automated coverage — the behaviour needs a real session plus an impersonation cookie, and the integration test that covered it isn't included here. It was verified locally against a real Postgres container, including reproducing the original bug by making the gate resolve the impersonation target and confirming it fails.
Worth a manual pass before merge:
ImpersonationAuditLogshowsSTOPthenSTART, both with the admin's id asadminId.Split out of #4571, which bundled this with an unrelated customer-card fix.