GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — fix(webapp): let an admin switch impersonation target without stopping first by isshaddad · Pull Request #4576 · triggerdotdev/trigger.dev Conversation
Collaborator
Changes
Switching straight between targets never passes through TestingNo 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:
Split out of #4571, which bundled this with an unrelated customer-card fix.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions…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.
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
|
There was an error while loading. Please reload this page.
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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| 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.
The reason will be displayed to describe this comment to others. Learn more.
🟡 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.
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).
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| 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.
The reason will be displayed to describe this comment to others. Learn more.
🟡 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.
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.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
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
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8208f523-b3fc-475d-b3ca-438e9dff508b
apps/webapp/app/models/admin.server.tsapps/webapp/app/routes/[email protected].$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📄 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/[email protected].$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📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/routes/[email protected].$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📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
apps/webapp/app/routes/[email protected].$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📄 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/[email protected].$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📄 CodeRabbit inference engine (AGENTS.md)
For apps, use
typecheckfor verification and never usebuildas the correctness check.
Files:
apps/webapp/app/routes/[email protected].$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📄 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/[email protected].$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📄 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📄 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.tsLearnt 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/[email protected].$organizationSlug.$.tsxapps/webapp/app/routes/admin_.impersonate.tsxLearnt 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/[email protected].$organizationSlug.$.tsxapps/webapp/app/routes/admin_.impersonate.tsxLearnt 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/[email protected].$organizationSlug.$.tsxapps/webapp/app/routes/admin_.impersonate.tsxLearnt 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/[email protected].$organizationSlug.$.tsxapps/webapp/app/routes/admin_.impersonate.tsxLearnt 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/[email protected].$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.tsLearnt 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/[email protected].$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.tsLearnt 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/[email protected].$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.tsLearnt 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/[email protected].$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.tsLearnt 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/[email protected].$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.tsLearnt 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/[email protected].$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.tsLearnt 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/[email protected].$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.tsLearnt 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/[email protected].$organizationSlug.$.tsxapps/webapp/app/routes/admin_.impersonate.tsxLearnt 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/[email protected].$organizationSlug.$.tsxapps/webapp/app/routes/admin_.impersonate.tsxLearnt 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/[email protected].$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.tsLearnt 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/[email protected].$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.tsLearnt 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/[email protected].$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.tsLearnt 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/[email protected].$organizationSlug.$.tsxapps/webapp/app/routes/admin_.impersonate.tsxLearnt 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.tsLearnt 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.tsLearnt 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.tsLearnt 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.tsapps/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/[email protected].$organizationSlug.$.tsx (1)
64-64: LGTM!Also applies to: 151-151
apps/webapp/app/services/impersonation.server.ts (1)
48-55: LGTM!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| * `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.
The reason will be displayed to describe this comment to others. Learn more.
🔒 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:
env from app/env.server.ts.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
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| // 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.
The reason will be displayed to describe this comment to others. Learn more.
🔒 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:
STOP-without-START trail. } catch (error) {
logger.error("Failed to create impersonation audit log", {
error,
adminId: admin.id,
targetId: userId,
previousTargetId,
});
+ throw error;
}Also applies to: 277-284
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| 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.
The reason will be displayed to describe this comment to others. Learn more.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🏁 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" done
Repository: 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 -240
Repository: 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/app
Repository: 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| const payload = Object.fromEntries(await request.formData()); | ||
| const { id } = FormSchema.parse(payload); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
🩺 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.
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); }
Sorry, something went wrong.
There was an error while loading. Please reload this page.
At least 1 approving review is required to merge this pull request.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.