GeezerVison

🟡 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.

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.

🔍 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/[email protected].$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.

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.

🟡 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.

Was this helpful? React with 👍 or 👎 to provide feedback.

🔍 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.

🔍 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.

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.

🟡 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.

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.

🔍 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.

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.

🔍 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.