Fix auth keys get 404 and improve Control API error logging#142
Fix auth keys get 404 and improve Control API error logging#142mattheworiordan wants to merge 1 commit intomainfrom
Conversation
The keys get command hit a non-existent GET /v1/apps/{appId}/keys/{keyId}
endpoint, causing 404 errors. The Control API only supports listing all
keys. Rewrite getKey() to always list keys and filter client-side,
supporting lookup by key ID, APP_ID.KEY_ID, full key value, or label
name (e.g. "Root").
Also add request method and URL to Control API error logging so failed
requests are easier to debug.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThe pull request refactors key retrieval logic to support multiple lookup mechanisms. The Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Command
participant Service as getKey Service
participant API as Backend API
rect rgba(0, 100, 200, 0.5)
Note over CLI,API: Old Flow (Direct ID Lookup)
CLI->>Service: getKey(appId, keyId)
Service->>API: GET /v1/apps/{appId}/keys/{keyId}
API-->>Service: Key object
Service-->>CLI: Return Key
end
rect rgba(0, 150, 100, 0.5)
Note over CLI,API: New Flow (List & Filter)
CLI->>Service: getKey(appId, keyIdentifier)
Service->>API: GET /v1/apps/{appId}/keys
API-->>Service: List of Keys
Service->>Service: Filter by identifier<br/>(full key, APP_ID.KEY_ID,<br/>key ID, or label)
Service-->>CLI: Return matching Key
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/unit/commands/auth/keys/revoke.test.ts (1)
46-49:⚠️ Potential issue | 🔴 CriticalUpdate tests to use DELETE instead of POST and remove /revoke suffix.
The tests mock
POST /v1/apps/${appId}/keys/${mockKeyId}/revoke, butrevokeKeyinsrc/services/control-api.ts(line 425) callsthis.request<void>(\/apps/${appId}/keys/${keyId}`, "DELETE"), which resolves toDELETE /v1/apps/${appId}/keys/${keyId}`.The mocked endpoints won't be called by the implementation, breaking test coverage. Change the mocks to use
.delete()instead of.post()and remove the/revokesuffix:.delete(`/v1/apps/${appId}/keys/${mockKeyId}`) .reply(200, {});This applies to lines 46-49, 68-70, and 85-87.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/auth/keys/revoke.test.ts` around lines 46 - 49, The test mocks in revoke.test.ts are calling POST /v1/apps/${appId}/keys/${mockKeyId}/revoke but the implementation revokeKey uses this.request<void>(`/apps/${appId}/keys/${keyId}`, "DELETE"), so update the nock mocks to match: replace .post(...) with .delete(...) and remove the trailing /revoke from the mocked path (i.e., mock DELETE /v1/apps/${appId}/keys/${mockKeyId}); apply the same change to the other two mocked blocks referenced around lines 68-70 and 85-87 so the tests exercise revokeKey correctly.
🧹 Nitpick comments (1)
test/unit/commands/auth/keys/update.test.ts (1)
6-27: Consider extracting shared test helpers to reduce duplication.The
mockKeysListandbuildMockKeyfunctions are duplicated acrossget.test.ts,update.test.ts, andrevoke.test.ts. Extracting them to a shared location (e.g.,test/helpers/key-mock-helpers.ts) would improve maintainability.♻️ Example shared helper
// test/helpers/key-mock-helpers.ts import nock from "nock"; export function mockKeysList(appId: string, keys: Record<string, unknown>[]) { return nock("https://control.ably.net") .get(`/v1/apps/${appId}/keys`) .reply(200, keys); } export function buildMockKey( appId: string, keyId: string, overrides: Record<string, unknown> = {}, ) { return { id: keyId, appId, name: "Test Key", key: `${appId}.${keyId}:secret`, capability: { "*": ["publish", "subscribe"] }, created: 1709251200000, // Fixed timestamp for deterministic tests modified: 1709251200000, ...overrides, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/auth/keys/update.test.ts` around lines 6 - 27, Extract the duplicated test helpers into a single module and update the tests to import them: move mockKeysList and buildMockKey into a new file (e.g., test/helpers/key-mock-helpers.ts), export both functions, replace the inline definitions in get.test.ts, update.test.ts, and revoke.test.ts to import mockKeysList and buildMockKey, and ensure buildMockKey uses a deterministic timestamp for created/modified to make tests stable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/auth/keys/get.ts`:
- Around line 43-49: The current parsing of keyIdentifier (the if block using
keyIdentifier.includes("."), parts = keyIdentifier.split("."), and appId = appId
|| parts[0]) can misinterpret labels containing a period (e.g., "v1.0") as
APP_ID.KEY_ID; update the check to only treat the left-side as an appId when it
matches a valid app id pattern: validate parts[0] against an appId
regex/validator (e.g., allowed chars/length or the project’s app id format)
before assigning to appId, and keep the existing behavior of preserving an
explicit appId flag (appId || parts[0]); apply this change in the conditional
that uses keyIdentifier, parts, and appId.
---
Outside diff comments:
In `@test/unit/commands/auth/keys/revoke.test.ts`:
- Around line 46-49: The test mocks in revoke.test.ts are calling POST
/v1/apps/${appId}/keys/${mockKeyId}/revoke but the implementation revokeKey uses
this.request<void>(`/apps/${appId}/keys/${keyId}`, "DELETE"), so update the nock
mocks to match: replace .post(...) with .delete(...) and remove the trailing
/revoke from the mocked path (i.e., mock DELETE
/v1/apps/${appId}/keys/${mockKeyId}); apply the same change to the other two
mocked blocks referenced around lines 68-70 and 85-87 so the tests exercise
revokeKey correctly.
---
Nitpick comments:
In `@test/unit/commands/auth/keys/update.test.ts`:
- Around line 6-27: Extract the duplicated test helpers into a single module and
update the tests to import them: move mockKeysList and buildMockKey into a new
file (e.g., test/helpers/key-mock-helpers.ts), export both functions, replace
the inline definitions in get.test.ts, update.test.ts, and revoke.test.ts to
import mockKeysList and buildMockKey, and ensure buildMockKey uses a
deterministic timestamp for created/modified to make tests stable.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/commands/auth/keys/get.tssrc/services/control-api.tstest/unit/commands/auth/keys/get.test.tstest/unit/commands/auth/keys/revoke.test.tstest/unit/commands/auth/keys/update.test.ts
| // If keyNameOrValue is in APP_ID.KEY_ID format (one period, no colon), extract appId | ||
| if (keyIdentifier.includes(".") && !keyIdentifier.includes(":")) { | ||
| const parts = keyIdentifier.split("."); | ||
| if (parts.length === 2) { | ||
| appId = appId || parts[0]; | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential edge case: labels containing a period could be misinterpreted.
If a user has a key with a label like "v1.0" or "Test.Key" and runs ably auth keys get "v1.0" --app APP_ID, the condition on line 44 would match (contains ., no :), and parts.length === 2 would also pass. This would incorrectly attempt to use "v1" as the appId.
Since the user explicitly provides --app, line 47's appId || parts[0] means the explicit flag takes precedence, so this edge case is mitigated when --app is provided. However, without --app, a label like "v1.0" would fail confusingly.
Consider a more robust check, such as validating that the first part looks like a valid app ID format:
🔧 Suggested improvement
// If keyNameOrValue is in APP_ID.KEY_ID format (one period, no colon), extract appId
if (keyIdentifier.includes(".") && !keyIdentifier.includes(":")) {
const parts = keyIdentifier.split(".");
- if (parts.length === 2) {
+ // Only treat as APP_ID.KEY_ID if exactly 2 parts and first part looks like an app ID
+ // App IDs are typically alphanumeric (e.g., "s57drg")
+ if (parts.length === 2 && /^[a-zA-Z0-9]+$/.test(parts[0])) {
appId = appId || parts[0];
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // If keyNameOrValue is in APP_ID.KEY_ID format (one period, no colon), extract appId | |
| if (keyIdentifier.includes(".") && !keyIdentifier.includes(":")) { | |
| const parts = keyIdentifier.split("."); | |
| if (parts.length === 2) { | |
| appId = appId || parts[0]; | |
| } | |
| } | |
| // If keyNameOrValue is in APP_ID.KEY_ID format (one period, no colon), extract appId | |
| if (keyIdentifier.includes(".") && !keyIdentifier.includes(":")) { | |
| const parts = keyIdentifier.split("."); | |
| // Only treat as APP_ID.KEY_ID if exactly 2 parts and first part looks like an app ID | |
| // App IDs are typically alphanumeric (e.g., "s57drg") | |
| if (parts.length === 2 && /^[a-zA-Z0-9]+$/.test(parts[0])) { | |
| appId = appId || parts[0]; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/auth/keys/get.ts` around lines 43 - 49, The current parsing of
keyIdentifier (the if block using keyIdentifier.includes("."), parts =
keyIdentifier.split("."), and appId = appId || parts[0]) can misinterpret labels
containing a period (e.g., "v1.0") as APP_ID.KEY_ID; update the check to only
treat the left-side as an appId when it matches a valid app id pattern: validate
parts[0] against an appId regex/validator (e.g., allowed chars/length or the
project’s app id format) before assigning to appId, and keep the existing
behavior of preserving an explicit appId flag (appId || parts[0]); apply this
change in the conditional that uses keyIdentifier, parts, and appId.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Free Tier Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Key lookup uses wrong app when current app differs
- Updated
auth keys getto always deriveappIdfrom an explicitAPP_ID.KEY_IDinput so lookup targets the correct app even when a different current app is configured.
- Updated
Or push these changes by commenting:
@cursor push 0ed189aa59
Preview (0ed189aa59)
diff --git a/src/commands/auth/keys/get.ts b/src/commands/auth/keys/get.ts
--- a/src/commands/auth/keys/get.ts
+++ b/src/commands/auth/keys/get.ts
@@ -44,7 +44,7 @@
if (keyIdentifier.includes(".") && !keyIdentifier.includes(":")) {
const parts = keyIdentifier.split(".");
if (parts.length === 2) {
- appId = appId || parts[0];
+ appId = parts[0];
}
}| if (keyIdentifier.includes(".") && !keyIdentifier.includes(":")) { | ||
| const parts = keyIdentifier.split("."); | ||
| if (parts.length === 2) { | ||
| appId = appId || parts[0]; |
There was a problem hiding this comment.
Key lookup uses wrong app when current app differs
High Severity
appId = appId || parts[0] only extracts the APP_ID from a full APP_ID.KEY_ID input when no app is already set. If the user has a current app configured (or passes --app) that differs from the APP_ID in the key name, the command lists keys from the wrong app and fails to find the key. All sibling commands (revoke.ts, update.ts, switch.ts) use appId = parts[0] unconditionally, which is the correct behavior.
1 participant
Summary
Note
Medium Risk
Changes key retrieval to always use
listKeys+ client-side matching, which affects behavior/performance of all commands that callgetKeyand introduces potential ambiguity when matching by label. Scope is limited to CLI key operations and logging, with tests updated accordingly.Overview
Fixes
auth keys get(and other key flows that rely onControlApi.getKey) by stopping use of the non-existent single-key GET endpoint and instead fetching/keysand filtering locally.auth keys getnow accepts multiple key identifiers (full key value,APP_ID.KEY_ID, key ID, or key label likeRoot) and updates help/examples accordingly. Control API error logging is improved to include the requestmethodandurl, and unit tests forget/revoke/updateare updated to mock the list-keys behavior and new not-found errors.Written by Cursor Bugbot for commit 054fd5b. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Release Notes
New Features
Improvements