fix(impersonate): revoke the live session when the actor token was already accepted#382
Conversation
🦋 Changeset detectedLatest commit: b6363a4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughImpersonation revocation now accepts an optional Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/cli-core/src/commands/impersonate/README.md (1)
65-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language to the fenced transcript block.
markdownlint flags this fence for missing a language identifier (MD040).
🔧 Suggested fix
-``` +```sh $ clerk imp revoke act_29w9... --user user_2x9k ▲ Token already accepted — an active impersonation session exists. ◇ Found session sess_9k2j... (actor: cli:[email protected]) ✔ Revoked session sess_9k2j... — impersonation ended.</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@packages/cli-core/src/commands/impersonate/README.mdaround lines 65 - 70,
Update the fenced transcript block in the impersonation README to specify the
shell language identifiersh, while preserving all command output unchanged.</details> <!-- cr-comment:v1:25336be72a8ccb1b93388119 --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>packages/cli-core/src/commands/impersonate/revoke.ts (1)</summary><blockquote> `107-116`: _🩺 Stability & Availability_ | _🔵 Trivial_ | _⚡ Quick win_ **Partial revoke progress is lost if a mid-loop revoke fails.** If session N of M fails to revoke, `revokedSessionIds` collected so far (and any human-mode success logs already printed) are discarded — the thrown error gives no indication that earlier sessions in this batch were already revoked. A retry is safe (the "active" status filter naturally excludes already-revoked sessions), but the immediate failure is misleading about what state was left behind. Consider catching per-session failures to report which sessions were revoked before continuing to throw. <details> <summary>♻️ Suggested fix: report partial progress before rethrowing</summary> ```diff const revokedSessionIds: string[] = []; for (const session of matches) { if (isHuman()) log.info(`Found session ${session.id} (actor: ${session.actor?.sub})`); - await withApiContext( - revokeSession(secretKey, session.id), - `Failed to revoke session ${session.id}`, - ); - revokedSessionIds.push(session.id); - if (isHuman()) log.success(`Revoked session ${session.id} — impersonation ended.`); + try { + await withApiContext( + revokeSession(secretKey, session.id), + `Failed to revoke session ${session.id}`, + ); + } catch (error) { + if (revokedSessionIds.length > 0 && isHuman()) { + log.warn(`Already revoked before this failure: ${revokedSessionIds.join(", ")}`); + } + throw error; + } + revokedSessionIds.push(session.id); + if (isHuman()) log.success(`Revoked session ${session.id} — impersonation ended.`); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli-core/src/commands/impersonate/revoke.ts` around lines 107 - 116, Update the revoke loop around revokedSessionIds and withApiContext so a per-session failure reports the session IDs successfully revoked earlier in the batch before rethrowing the original error. Preserve the existing success logging and continue propagating failures, while making the partial-progress report available when any mid-loop revoke fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli-core/src/lib/sessions.ts`:
- Around line 31-54: Update listUserSessions to request all relevant sessions by
adding the API’s supported pagination parameter with a limit of 500 to the
URLSearchParams before calling bapiRequest. Preserve the existing user_id/status
filters and response-shape handling so revokeImpersonationSessions can find
later active sessions.
---
Nitpick comments:
In `@packages/cli-core/src/commands/impersonate/README.md`:
- Around line 65-70: Update the fenced transcript block in the impersonation
README to specify the shell language identifier `sh`, while preserving all
command output unchanged.
In `@packages/cli-core/src/commands/impersonate/revoke.ts`:
- Around line 107-116: Update the revoke loop around revokedSessionIds and
withApiContext so a per-session failure reports the session IDs successfully
revoked earlier in the batch before rethrowing the original error. Preserve the
existing success logging and continue propagating failures, while making the
partial-progress report available when any mid-loop revoke fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1800be1f-f424-49a3-a786-cf1c56fcbf42
📒 Files selected for processing (10)
.changeset/imp-revoke-fix.mdpackages/cli-core/src/commands/impersonate/README.mdpackages/cli-core/src/commands/impersonate/impersonate.test.tspackages/cli-core/src/commands/impersonate/impersonate.tspackages/cli-core/src/commands/impersonate/index.tspackages/cli-core/src/commands/impersonate/revoke.test.tspackages/cli-core/src/commands/impersonate/revoke.tspackages/cli-core/src/lib/actor-tokens.tspackages/cli-core/src/lib/errors.tspackages/cli-core/src/lib/sessions.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)clerk/dashboard(manual)clerk/accounts(manual)clerk/backoffice(manual)clerk/clerk(manual)clerk/clerk-docs(manual)clerk/cloudflare-workers(manual)
…ready accepted
BAPI's POST /v1/actor_tokens/{id}/revoke only revokes pending tokens —
opening the sign-in URL consumes the ticket, flips the token to accepted,
and the revoke answers 400 forever after, while the impersonation lives on
as a session. `clerk imp revoke` now falls back on that 400: with a new
--user <user_id> option it lists the user's active sessions, keeps only
those stamped with the operator's cli:<email> actor (prefix match so
+<context> variants count), and revokes each one. Without --user it errors
with actor_token_already_accepted and shows how to re-run. The creation
hint printed by `clerk imp` now includes --user so the copied command
works in both the pending and accepted states. Agent mode emits
{ id, status: "accepted", revokedSessionIds } when the fallback runs.
rafa-thayto
force-pushed
the
rafa-thayto/imp-revoke-fix
branch
from
d16bfc6 to
ea3e1a9
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/cli-core/src/commands/impersonate/README.md (1)
65-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language to the transcript fence (MD040).
Static analysis flags this fenced block as missing a language identifier.
📝 Proposed fix
-``` +```text $ clerk imp revoke act_29w9... --user user_2x9k ▲ Token already accepted — an active impersonation session exists. ◇ Found session sess_9k2j... (actor: cli:[email protected]) ✔ Revoked session sess_9k2j... — impersonation ended.</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@packages/cli-core/src/commands/impersonate/README.mdaround lines 65 - 70,
Update the transcript code fence in the impersonation README example to include
the text language identifier, preserving the command output contents unchanged.</details> <!-- cr-comment:v1:71ce6db23071f3dea1555ab1 --> _Source: Linters/SAST tools_ </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Nitpick comments:
In@packages/cli-core/src/commands/impersonate/README.md:
- Around line 65-70: Update the transcript code fence in the impersonation
README example to include the text language identifier, preserving the command
output contents unchanged.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro Plus **Run ID**: `838d0ecd-a0a5-4ff3-9b62-c4c21f5e90ce` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between d16bfc6293c5c6e4811338ddaeb415ca739450ed and ea3e1a9ac59324ddc3662e1fc079cca228320b74. </details> <details> <summary>📒 Files selected for processing (10)</summary> * `.changeset/imp-revoke-fix.md` * `packages/cli-core/src/commands/impersonate/README.md` * `packages/cli-core/src/commands/impersonate/impersonate.test.ts` * `packages/cli-core/src/commands/impersonate/impersonate.ts` * `packages/cli-core/src/commands/impersonate/index.ts` * `packages/cli-core/src/commands/impersonate/revoke.test.ts` * `packages/cli-core/src/commands/impersonate/revoke.ts` * `packages/cli-core/src/lib/actor-tokens.ts` * `packages/cli-core/src/lib/errors.ts` * `packages/cli-core/src/lib/sessions.ts` </details> <details> <summary>🔗 Linked repositories identified</summary> CodeRabbit considers these linked repositories for cross-repo context during reviews: - `clerk/clerk_go` _(manual)_ - `clerk/dashboard` _(manual)_ - `clerk/accounts` _(manual)_ - `clerk/backoffice` _(manual)_ - `clerk/clerk` _(manual)_ - `clerk/clerk-docs` _(manual)_ - `clerk/cloudflare-workers` _(manual)_ </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (5)</summary> * packages/cli-core/src/commands/impersonate/index.ts * packages/cli-core/src/lib/errors.ts * packages/cli-core/src/lib/sessions.ts * packages/cli-core/src/commands/impersonate/impersonate.ts * packages/cli-core/src/commands/impersonate/revoke.ts </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
GET /v1/sessions is unpaginated and always returns a plain array, same
as /v1/users — the `{ data: [...] }` envelope branch was dead code.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/cli-core/src/lib/sessions.ts (1)
31-50: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDefault
limit(10) onGET /v1/sessionscan still cause missed matches.The prior review flagged that
GET /v1/sessionsdefaults to a 10-item page, solistUserSessionscould silently miss later active sessions and cause the caller (revokeImpersonationSessions) to wrongly reportimpersonation_session_not_found. The maintainer's reply ("This list endpoint only exists in a single state, let's simplify this here") addressed the response-shape normalization (dropping the paginated-envelope branch, per the commit message), but the request itself still only sendsuser_id/status(Lines 35-38) — nolimitorpaginatedparam was added. For users with many sessions, matching sessions beyond the first page will still be missed.🐛 Proposed fix to request a larger page
const params = new URLSearchParams({ user_id: query.userId }); if (query.status) { params.set("status", query.status); } + params.set("limit", "500");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli-core/src/lib/sessions.ts` around lines 31 - 50, Update listUserSessions to request all relevant sessions by adding the API’s supported unpaginated or sufficiently large limit parameter to the URLSearchParams alongside user_id and status. Preserve the existing array response normalization and ensure revokeImpersonationSessions can find matches beyond the default 10-item page.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/cli-core/src/lib/sessions.ts`:
- Around line 31-50: Update listUserSessions to request all relevant sessions by
adding the API’s supported unpaginated or sufficiently large limit parameter to
the URLSearchParams alongside user_id and status. Preserve the existing array
response normalization and ensure revokeImpersonationSessions can find matches
beyond the default 10-item page.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 27d853c6-160f-41ee-a55b-24f1ab6ca4d3
📒 Files selected for processing (2)
packages/cli-core/src/commands/impersonate/revoke.test.tspackages/cli-core/src/lib/sessions.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)clerk/dashboard(manual)clerk/accounts(manual)clerk/backoffice(manual)clerk/clerk(manual)clerk/clerk-docs(manual)clerk/cloudflare-workers(manual)
💤 Files with no reviewable changes (1)
- packages/cli-core/src/commands/impersonate/revoke.test.ts
2 participants
Summary
clerk imp revoke act_...failed with(400): cannot revokeonce the impersonation sign-in URL had been opened: BAPI'sPOST /v1/actor_tokens/{id}/revokeonly revokes pending tokens, and accepting the ticket flips the token toacceptedwhile the live impersonation persists as a session. There was no way to end that session from the CLI.clerk imp revokenow detects the 400 and falls back to session revocation:GET /v1/sessions?user_id=<user>&status=active, filtered to sessions whoseactor.submatches the operator'scli:<email>stamp (prefix match, so--actor+<context>variants also match), thenPOST /v1/sessions/{id}/revokefor each — sessions started by other operators are never touched.--user <user_id>option onclerk imp revoke(required for the fallback — BAPI has noGET /v1/actor_tokens/{id}, so the target user can't be recovered from the token ID). Without it, the fallback errors with the newactor_token_already_acceptedcode and shows how to re-run; no matching session errors withimpersonation_session_not_found.Revoke with:hint printed at creation now includes--user <user_id>so the copied command works in both the pending and accepted states.{ "id": "<act_id>", "status": "accepted", "revokedSessionIds": ["sess_..."] }when the fallback runs, and never prompts.lib/sessions.tsBAPI client (tolerates both plain-array and paginated{ data }list shapes),isActorTokenNotRevocableErrorhelper inlib/actor-tokens.ts, README + changeset (minor) updates.Test plan
revoke.test.ts(TDD, watched fail first): fallback with/without--user, actor-stamp filtering incl.+<context>and foreign/plain sessions, agent-mode JSON payload, no-match error, paginated list shape, non-400 passthrough,--userformat validationimpersonate.test.tsasserts the creation hint includes--user <user_id>bun run format:check,bun run lint,bun run typecheck,bun run test(1904 pass) all greenbun run test:e2e:op— could not run from this environment (1Password vault unavailable); CI runstest:e2e