Visitar URL original
fix(mau): stop target --client-id flags from becoming the CLI's identity by ttypic · Pull Request #460 · ably/ably-cli · GitHub
Skip to content

fix(mau): stop target --client-id flags from becoming the CLI's identity - #460

Open
ttypic wants to merge 1 commit into
integration/mau-stable-client-idfrom
integration/mau-client-id-flag
Open

fix(mau): stop target --client-id flags from becoming the CLI's identity#460
ttypic wants to merge 1 commit into
integration/mau-stable-client-idfrom
integration/mau-client-id-flag

Conversation

@ttypic

@ttypic ttypic commented Oct 1, 2026 •
edited
Loading

Copy link
Copy Markdown

Push and token commands declare their own --client-id to name a target
or filter, but the base command read flags["client-id"] regardless of
which definition supplied it. ably push devices remove-where --client-id user123 therefore also connected as user123, billing an
end user who never connected and misattributing an admin write.

The base command now honours --client-id as the CLI's own identity only
when the command declares the shared clientIdFlag definition. Flag
names are unchanged; the eleven local target flags get descriptions
that say what they target. A guard test loads every command and fails
if any local --client-id reaches the client options.

@vercel

vercel Bot commented Oct 1, 2026 •
edited
Loading

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
cli-web-cli Ready Ready Preview Oct 5, 2026 6:57pm UTC

Request Review

@claude-code-ably-assistant

claude-code-ably-assistant Bot commented Oct 1, 2026

Copy link
Copy Markdown

Walkthrough

This PR fixes a security/billing bug where push and token commands that declare their own --client-id for targeting or filtering inadvertently became the CLI's wire identity. For example, ably push devices remove-where --client-id user123 would connect to Ably as user123, billing an end user who never connected. The fix teaches AblyBaseCommand to honour --client-id as identity only when the command declares the canonical shared clientIdFlag object.

Changes

Area Files Summary
Commands – Push src/commands/push/channels/{list,remove,remove-where,save}.ts, src/commands/push/devices/{list,remove-where,save}.ts, src/commands/push/publish.ts Clarified --client-id flag descriptions to say "target/filter" rather than "associate", so users see these as targeting flags, not identity flags
Commands – Auth src/commands/auth/issue-ably-token.ts, src/commands/auth/issue-jwt-token.ts Same clarification: --client-id issues a token to that client, not as that client
Services src/base-command.ts Added private identityFlag() that returns flags["client-id"] only when the command's static flag definition is the same object reference as the shared clientIdFlag; three callsites updated
Config src/flags.ts Added JSDoc on clientIdFlag explaining the identity/target distinction and how the guard works
Tests test/unit/base/client-id-target-guard.test.ts (new) Loads every command via oclif's Config, passes a sentinel value as --client-id to each command that has a local (non-identity) definition, and asserts none of them leak into getClientOptions()
Tests test/unit/base/client-identity.test.ts Adds clientIdFlag to TestCommand so the existing identity tests reflect the new guard correctly
Docs / Skills .claude/skills/ably-new-command/SKILL.md Documents the convention: use a command-local Flags.string() for target/filter client IDs, never spread clientIdFlag

Review Notes

  • Behavioral change: ably push * --client-id <target> and ably auth issue-*-token --client-id <subject> no longer set the CLI's own Ably identity. This is the intended fix but is a real behavioral change for any caller that was (accidentally) relying on the side-effect.
  • Identity check is by reference equality (declared === clientIdFlag["client-id"]). This is intentional and correct — two separate Flags.string() calls with identical options would still be treated as targets. Reviewers should confirm this assumption holds for any new commands added in future (the guard test enforces it automatically).
  • Guard test self-check: The test asserts targetCommands contains push:devices:remove-where to prove the loop actually ran over target commands, preventing the guard from silently passing on an empty iteration.
  • No new dependencies.
  • No migration or deployment steps required — purely a client-side CLI fix.

@ttypic
ttypic requested a review from umair-ably October 1, 2026 13:00
@umair-ably
umair-ably added this pull request to stack #465 October 1, 2026 14:14
@sacOO7
sacOO7 requested a lite review from Copilot October 5, 2026 09:32
Copilot AI reviewed Oct 5, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The guard test must restore any pre-existing ABLY_API_KEY value before approval.

Review effort: Lite
Findings: 2 Low severity

Open (2)
What changed in this PR

Prevents command-local --client-id target values from becoming the CLI’s own identity, avoiding incorrect MAU attribution.

Changes:

  • Restricts identity resolution to the shared clientIdFlag.
  • Adds a guard test covering all commands.
  • Clarifies target-client descriptions and guidance.
File Summary
test/​unit/​base/​client-identity.test.ts Updates identity test setup.
test/​unit/​base/​client-id-target-guard.test.ts Guards against target identity leakage.
src/​flags.ts Documents identity flag semantics.
src/​commands/​push/​publish.ts Clarifies target description.
src/​commands/​push/​devices/​save.ts Clarifies device target.
src/​commands/​push/​devices/​remove-where.ts Clarifies filter target.
src/​commands/​push/​devices/​list.ts Clarifies filter target.
src/​commands/​push/​channels/​save.ts Clarifies subscription target.
src/​commands/​push/​channels/​remove.ts Clarifies subscription target.
src/​commands/​push/​channels/​remove-where.ts Clarifies filter target.
src/​commands/​push/​channels/​list.ts Clarifies filter target.
src/​commands/​auth/​issue-jwt-token.ts Clarifies token subject behavior.
src/​commands/​auth/​issue-ably-token.ts Clarifies token subject behavior.
src/​base-command.ts Restricts client identity resolution.
.claude/​skills/​ably-new-command/​SKILL.md Documents target-versus-identity flags.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread .claude/skills/ably-new-command/SKILL.md

**When to include `clientIdFlag`:** Add `...clientIdFlag` to commands where client identity affects the operation: subscribe, publish, enter, set, acquire, update, delete, append, annotate. The reason is that users may want to test auth scenarios — e.g., "can client B update client A's message?" — so they need the ability to set their client ID. Do NOT add to read-only queries (get, get-all, history, occupancy get) — Ably capabilities are operation-based, not clientId-based, so client identity is irrelevant for pure reads.

**Target client IDs are not identity:** the base command treats `--client-id` as the client ID the CLI acts as only when the command declares it via the shared `clientIdFlag` object. When a command's `--client-id` names a *target* or *filter* (a push recipient, a token's subject), declare a command-local `Flags.string()` instead — never spread `clientIdFlag` for that — so the value can't become the CLI's own identity. `test/unit/base/client-id-target-guard.test.ts` enforces this for every command.
Comment thread src/commands/push/channels/remove.ts
}),
"client-id": Flags.string({
description: "Client ID to unsubscribe",
description: "Client ID whose subscription to remove",
@ttypic @claude
fix(mau): stop target --client-id flags from becoming the CLI's identity
c92afaf 
Push and token commands declare their own --client-id to name a target
or filter, but the base command read `flags["client-id"]` regardless of
which definition supplied it. `ably push devices remove-where
--client-id user123` therefore also connected as user123, billing an
end user who never connected and misattributing an admin write.

The base command now honours --client-id as the CLI's own identity only
when the command declares the shared `clientIdFlag` definition. Flag
names are unchanged; the eleven local target flags get descriptions
that say what they target. A guard test loads every command and fails
if any local --client-id reaches the client options.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@ttypic
ttypic force-pushed the integration/mau-client-id-flag branch from 69e24e1 to c92afaf Compare October 5, 2026 18:57

This branch was successfully deployed

1 active deployment
Preview — c92afafb Deployed Oct 5, 2026 by vercel[bot]
View deployment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@umair-ably umair-ably umair-ably approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ttypic @umair-ably