fix(webapp): two customer-card and impersonation fixes - #4571
Conversation
|
|
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:
WalkthroughThe changes add real-admin authentication for impersonation authorization and audit attribution. Impersonation requests authorize before token consumption and record STOP and START events transactionally. Callers use revised impersonation signatures. The plain customer card route now uses shared request validation and response completion utilities. Responses include null entries for requested card keys without data. Tests cover the shared schema and card completion behavior. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 18, up 1) 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. |
![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.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| @@ -0,0 +1,59 @@ | |||
| import { z } from "zod"; | |||
There was a problem hiding this comment.
🟡 Server-only change ships without the required release-notes entry
This change touches only server code under apps/webapp/ but adds no .server-changes/ entry (see the new apps/webapp/app/utils/plainCustomerCards.ts), which the repository requires so the fix appears in the user-facing release notes.
Impact: The customer-card and impersonation fixes will be missing from the published release notes.
Rule source
AGENTS.md ("Changesets and Server Changes") and CONTRIBUTING.md ("Adding server changes") both state: when a PR changes only server components (apps/webapp/, apps/supervisor/, …) with no package changes, add a .server-changes/ markdown file with area and type frontmatter and a one-line, user-facing description. git diff --name-only for this PR shows only apps/webapp/** files and no .server-changes/ addition.
Prompt for agents
This PR modifies only apps/webapp (server code) and adds no .server-changes/ entry, which AGENTS.md and CONTRIBUTING.md require for server-only changes. Add a markdown file under .server-changes/ with `area: webapp` and `type: fix` frontmatter and a one-line, user-facing description of the change (plain language about behaviour, not implementation or internal tool names). See .server-changes/README.md for the format.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // our own page and so satisfies the same check. | ||
| if (isSameOriginNavigation(request, env.LOGIN_ORIGIN)) { | ||
| throw await startImpersonation(request, organizationSlug, path, user); | ||
| throw await startImpersonation(request, organizationSlug, path); |
There was a problem hiding this comment.
🔍 The /@/orgs entry point still forces a stop-then-restart when switching target
The fix removes the "you must stop impersonating first" behaviour for /admin/impersonate, but the /@/orgs/<slug>/… entry point still short-circuits on user.isImpersonating and clears impersonation before re-entering (loader at apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsx:34-40 and action at :119-124). That path is functional (it clears, then the follow-up GET starts on the new target and now succeeds because the gate uses the real user), but it means switching via an org link still costs an extra round trip and produces a STOP from clearImpersonation rather than the new paired STOP+START. Worth confirming this asymmetry is intended.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Intended, confirmed. That route short-circuits on user.isImpersonating and clears before re-entering, so switching via an org link costs an extra round trip and produces a lone STOP from clearImpersonation followed by a separate START.
Left as-is for two reasons: the audit trail is still unambiguous there (the rows come from separate requests, so their timestamps genuinely differ), and the clear-then-restart is what makes that path work today rather than something it works around. Changing it would widen this PR into the consent flow for no correctness gain.
Signing in from an impersonation link dropped the destination: the admin gate answered both "not signed in" and "signed in but not an admin" with a redirect to /, so an agent who clicked Impersonate while logged out landed on the dashboard afterwards. Unauthenticated requests now redirect to login carrying the original URL as redirectTo — the one-time token is validated after the gate, so it survives the round trip. The paired STOP/START audit rows were written with createMany, a single insert, so both took the same createdAt and a view ordered by that column could show the new START ahead of the STOP closing the previous session. They are now two statements so the sequence is unambiguous.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| if (previousTargetId && previousTargetId !== userId) { | ||
| await prismaClient.impersonationAuditLog.create({ | ||
| data: { | ||
| action: "STOP", | ||
| adminId: admin.id, | ||
| targetId: previousTargetId, | ||
| ipAddress, | ||
| }, | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 Audit record for a new impersonation is silently skipped when closing the previous one fails
The record that closes the previous impersonation is written before the record that opens the new one (impersonationAuditLog.create at apps/webapp/app/models/admin.server.ts:249-257), and both share one error handler, so if the first write fails the second never runs while the impersonation still starts.
Impact: An admin can begin impersonating someone with no trace of it in the audit trail.
How a failed STOP insert swallows the START insert
Both create calls live inside the same try block (apps/webapp/app/models/admin.server.ts:240-274). The STOP row references previousTargetId via a foreign key on ImpersonationAuditLog.targetId; if that user row has since been deleted (or any other write error occurs), the create throws, control jumps to the catch, and the START row for the new target is never inserted. Execution then continues to setImpersonationId and the redirect, so impersonation begins with no START record. Writing the START row first, or wrapping each create in its own try/catch, would keep the new session's record independent of the close-out record.
Was this helpful? React with 👍 or 👎 to provide feedback.
| export function answerAllCardKeys<TCard extends { key: string }>( | ||
| cardKeys: string[], | ||
| cards: TCard[] | ||
| ): (TCard | NoDataCard)[] { | ||
| const answered = new Set(cards.map((card) => card.key)); | ||
|
|
||
| return [ | ||
| ...cards, | ||
| ...cardKeys | ||
| .filter((key) => !answered.has(key)) | ||
| .map( | ||
| (key): NoDataCard => ({ | ||
| key, | ||
| components: null, | ||
| }) | ||
| ), | ||
| ]; |
There was a problem hiding this comment.
🔍 Cards for keys Plain didn't request are passed through unchanged
answerAllCardKeys only adds missing keys; it never filters out cards whose key wasn't in cardKeys (explicitly asserted by the "ignores extra cards that were not requested" test). Today the route only builds cards while iterating cardKeys, so extras can't occur, but if PLAIN_CUSTOMER_CARDS_KEY is misconfigured or a future branch pushes an unrequested key, Plain will receive a card it did not ask for. Worth confirming Plain tolerates that rather than treating it as an integration error.
Was this helpful? React with 👍 or 👎 to provide feedback.
Closing the previous impersonation and opening the new one were two separate statements sharing one error handler, so a failure between them could start an impersonation whose only record was the STOP for the previous target — an admin acting as someone with no trace of it. Both rows now go through the $transaction helper. createdAt is stamped explicitly because Postgres now() is the transaction timestamp: inside one transaction the default would give both rows the same value, leaving an audit view ordered by that column unable to tell which came first.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
… target The flag is scoped to a single impersonation session, which is why stopping impersonation drops it. Switching straight from one target to another only became possible in this branch, and that path sets the impersonated id without touching the flag, so an admin viewing target A as the user landed on target B with the toggle still on. It is now cleared whenever the target changes.
| return [ | ||
| ...cards, | ||
| ...cardKeys | ||
| .filter((key) => !answered.has(key)) | ||
| .map( | ||
| (key): NoDataCard => ({ | ||
| key, | ||
| components: null, | ||
| }) | ||
| ), | ||
| ]; |
There was a problem hiding this comment.
🔍 No-data cards omit timeToLiveSeconds
Every answered card sets timeToLiveSeconds, but the filler cards produced here only carry key and components: null. Assuming Plain treats the field as optional this is fine; if it isn't, the filler cards would themselves be rejected and reintroduce the integration error this change is meant to remove. Also note that duplicate entries in cardKeys would produce duplicate filler entries, since the filter only dedupes against already-answered keys.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Both are fine, deliberately.
Omitted TTL — Plain documents the field as optional on purpose: "timeToLiveSeconds (optional, nullable): can either be omitted or null. If provided it will override the default time to live value." Omitting it means the filler card falls back to the TTL configured for that card in workspace settings, which is the behaviour we want for a card with no data — pinning a TTL here would override a setting somebody chose.
Duplicate keys — Plain enforces uniqueness: "Card keys must be unique within a workspace", matching [a-zA-Z0-9_-]+. So a cardKeys array cannot contain the same key twice. I dropped a test asserting the duplicate behaviour for exactly that reason: it locked in incidental behaviour rather than a requirement.
The failure that matters is the opposite one — a requested key coming back missing, which Plain records as an integration error. That is what the helper exists to prevent.
The admin layout gates on requireSuper via getUserId, which resolves to the impersonated user while impersonating — so starting on a second target ran that gate against the target and it answered with its own redirect to /. The switch worked only because the router prefers the deepest redirect, which is too subtle a thing for an auth path to depend on. The trailing underscore keeps the route at /admin/impersonate while opting it out of the layout. Nothing is lost: the route only ever redirects, so it never rendered inside it.
| /** | ||
| * Starts (or switches) impersonation. | ||
| * | ||
| * The admin gate resolves the *real* authenticated user itself. `requireUser` returns the | ||
| * impersonation target while impersonating, so callers that gated on it refused an admin who was | ||
| * already impersonating someone — they had to stop first — and would have attributed the audit row | ||
| * to the target rather than the admin. | ||
| * | ||
| * `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. | ||
| */ |
There was a problem hiding this comment.
🟡 Pull request bundles two unrelated fixes
The change combines two unrelated fixes — support-tool card validation and impersonation switching — in one pull request, which the project's contribution rules do not accept.
Impact: The PR risks being rejected or delayed, and either fix cannot be reverted independently of the other.
Repository rule: one issue per PR
CONTRIBUTING.md: "Important: We only accept PRs that address a single issue. Please do not submit PRs containing multiple unrelated fixes or features. If you have multiple contributions, open a separate PR for each one." The author's own description opens with "Two unrelated bugs in admin tooling", and the diff spans the Plain customer-card endpoint (apps/webapp/app/routes/api.v1.plain.customer-cards.ts, apps/webapp/app/utils/plainCustomerCards.ts) and the impersonation flow (apps/webapp/app/models/admin.server.ts, apps/webapp/app/routes/admin_.impersonate.tsx, apps/webapp/app/services/session.server.ts).
Prompt for agents
Split this PR into two: one for the Plain customer-card schema/response changes (api.v1.plain.customer-cards.ts, utils/plainCustomerCards.ts and its test) and one for the impersonation fixes (models/admin.server.ts, services/session.server.ts, services/impersonation.server.ts, the admin_.impersonate route move, and the consent route).
Was this helpful? React with 👍 or 👎 to provide feedback.
| async function requireRealAdmin(request: Request) { | ||
| if (!(await authenticator.isAuthenticated(request))) { | ||
| const url = new URL(request.url); | ||
| const redirectTo = sanitizeRedirectPath(`${url.pathname}${url.search}`); | ||
| throw redirect(`/login?${new URLSearchParams([["redirectTo", redirectTo]])}`); | ||
| } | ||
|
|
||
| const admin = await getRealUser(request); | ||
| return admin?.admin ? admin : null; | ||
| } |
There was a problem hiding this comment.
🔍 canSuper gate and the raw admin column can disagree, producing a 500
requireRealAdmin deliberately gates on auth.ability.canSuper() rather than User.admin, but redirectWithImpersonation then re-checks the raw column (if (!admin?.admin) throw new Error("Unauthorized") at apps/webapp/app/models/admin.server.ts:231-234). If an RBAC plugin ever grants canSuper() to someone whose User.admin is false, this route passes its own gate and then throws an uncaught Error, surfacing as a 500 rather than a redirect. The comment here notes a plugin may be stricter, which is safe; the looser direction is the one that produces the bad response. Worth confirming the plugin contract only ever narrows.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Good catch — this was a real gap my rename introduced, now closed.
buildFallbackAbility(isAdmin) returns superAbility (canSuper: () => true) for admins, so on OSS canSuper() and the User.admin column are identical. But you are right that a plugin is free to be stricter, and this route had become the only admin entry point not evaluated through the ability.
requireRealAdmin now builds the ability explicitly for the real admin and checks it:
const auth = await rbac.authenticateSession(request, { userId: admin.id });
if (!auth.ok || !auth.ability.canSuper()) return null;dashboardLoader({ authorization: { requireSuper: true } }) is still not usable here, because it resolves its subject with getUserId — the impersonated id while impersonating, which is the bug this route exists to fix. requireSuper is the only global gate, so no org/project scope is needed.
…lumn Opting out of the admin layout also opted out of its requireSuper check, leaving this the only admin entry point gated on the raw User.admin column. canSuper() equals that column in the OSS fallback, but an RBAC plugin is free to be stricter. The ability is now built explicitly for the real admin's id and canSuper() is checked directly. dashboardLoader can't be used: it resolves its subject with getUserId, which is the impersonated id while impersonating — the bug this route exists to fix.
| 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.
🔍 Wrapping STOP+START in one transaction can now lose the START audit row
previousTargetId comes from the impersonation cookie, which is unvalidated: it can name a user row that has since been deleted (or was never valid). ImpersonationAuditLog.targetId is a FK to User (internal-packages/database/prisma/schema.prisma:2831), so a stale cookie makes the STOP insert fail, aborting the whole transaction and taking the START row with it. Before this change, only the START row was written and it always succeeded. The failure is swallowed by the surrounding try/catch (apps/webapp/app/models/admin.server.ts:277-284) and impersonation still proceeds, so the outcome is an impersonation session with no audit record at all — exactly the scenario the transaction comment says it wants to avoid. Consider writing the START first (or inserting the STOP best-effort outside the transaction) so a bad previous target can't erase the record of the new one.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Split into two PRs, one per fix, per CONTRIBUTING.md:
Both branch off current Closing in favour of those two. |
1 participant
Two unrelated bugs in admin tooling
1. Customer cards 400'd for customers with no external id
api/v1/plain/customer-cardsvalidatedcustomer.emailandcustomer.externalIdwithz.string().optional(). Plain sends these keys as explicitnulls rather than omitting them, and.optional()acceptsundefinedbut rejectsnull— so the request failed validation before anylookup ran. Customers we don't set an
externalIdfor got a 400 every time; the rest worked, whichis why it looked intermittent.
email,externalIdandthreadare now.nullish(). Therefinestill requires one ofemail/externalId, and the route's existing email fallback resolves these customers.
app/utils/plainCustomerCards.tsso they can beunit-tested without pulling in the db and env modules.
Also fixed in the same file: when no user matched, the route returned
{ cards: [] }. Plain recordsan integration error for any requested card key it doesn't get back, so a partial response shows up
as a broken card rather than a hidden one. Every requested key is now answered, with
components: nullwhere there's no data.2. An admin couldn't switch impersonation target
getUserIdresolves to the impersonated user id while impersonating — by design, sorequireUseranswers "who is this request acting as". The impersonation entry points gated on it, so while
impersonating a customer
user.adminwas that customer's flag (false) and starting on a secondtarget silently redirected to
/. You had to stop impersonating first.getRealUser(request, prismaClient?)insession.server.tsresolves the authenticated user,ignoring the impersonation cookie.
redirectWithImpersonationgates on it rather than taking a user from the caller, and uses thatid for the audit row — which previously would have named the impersonated customer as the actor.
clearImpersonation, so aSTOPfor the previous target is now recorded alongside the newSTART. Without it the audittrail showed two overlapping
STARTs with no close.redirectWithImpersonationkeeps averifiedAdminparameter, used only bytest/impersonationConsent.test.tsto supply an admin without a session cookie. No productioncaller passes it, and its docstring says not to.