Skip to content

[DX-1101] Fix keys revoke to use correct Control API endpoint#335

Open
sacOO7 wants to merge 1 commit intomainfrom
fix/keys-revoke-token
Open

[DX-1101] Fix keys revoke to use correct Control API endpoint#335
sacOO7 wants to merge 1 commit intomainfrom
fix/keys-revoke-token

Conversation

@sacOO7
Copy link
Copy Markdown
Contributor

@sacOO7 sacOO7 commented Apr 16, 2026

@vercel
Copy link
Copy Markdown

vercel bot commented Apr 16, 2026

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

Project Deployment Actions Updated (UTC)
cli-web-cli Ready Ready Preview, Comment Apr 16, 2026 2:05pm

Request Review

@sacOO7 sacOO7 requested a review from AndyTWF April 16, 2026 14:15
@sacOO7 sacOO7 marked this pull request as ready for review April 16, 2026 14:15
@sacOO7 sacOO7 requested a review from umair-ably April 16, 2026 14:15
@claude-code-ably-assistant
Copy link
Copy Markdown

Walkthrough

This PR fixes a bug where the revokeKey method in the Control API service was calling DELETE /apps/{app_id}/keys/{key_id} instead of the correct POST /apps/{app_id}/keys/{key_id}/revoke. It also adds defensive handling for the revoke endpoint's empty 200 response body (using response.text() before JSON.parse rather than response.json() directly). Unit test mocks are updated to match the corrected endpoint.

Changes

Area Files Summary
Services src/services/control-api.ts Changed revokeKey from DELETE .../keys/{id} to POST .../keys/{id}/revoke; added empty-body guard in shared response parser
Tests test/unit/commands/auth/keys/revoke.test.ts Updated all three nock interceptors from .delete() to .post(...revoke)
Tests test/unit/services/control-api.test.ts Updated error-path nock interceptor from .delete() to .post(...revoke)

Review Notes

  • Behavioral fix: The previous DELETE call would have been rejected by the Control API (wrong HTTP method/path), meaning all key revocations were broken before this change.
  • Empty body handling: The revoke endpoint returns an empty 200 OK. The original response.json() call throws on an empty body; the fix reads via response.text() first and skips parsing when the string is empty. This change is in the shared response-parsing path in ControlApi, so any other endpoint that returns an empty body will now also be handled safely.
  • No E2E coverage yet: Per the PR description, E2E test coverage for this flow will be addressed separately in [DX-967] Adds more happy path E2E tests #250.
  • No new dependencies.

Copy link
Copy Markdown

@claude-code-ably-assistant claude-code-ably-assistant bot left a comment

Choose a reason for hiding this comment

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

Review Summary

This PR fixes a real bug: revokeKey was calling DELETE /apps/{appId}/keys/{keyId} when the Control API spec requires POST /apps/{appId}/keys/{keyId}/revoke. The endpoint fix and test updates are correct.

One real gap to address:

The empty-body fix is untested. The PR description says the revoke endpoint returns 200 OK with an empty body, and the fix handles that case in request(). But all three test files mock with reply(200, {}) which sends non-empty JSON. The new empty-body branch (if (!text) return {} as T) is never exercised by any test.

If someone later changed the empty-body guard, the existing tests would still pass, masking the regression. To fix: add a success test in control-api.test.ts that uses reply(200, '') to simulate the real API behavior, and update the command-level mocks from reply(200, {}) to reply(200, '') as well.

Minor robustness note (non-blocking): if (!text) does not guard against whitespace-only bodies. if (!text.trim()) is a safer guard, though in practice the Control API returns empty string not whitespace.

Everything else looks good:

  • Endpoint is correct per spec (POST .../revoke)
  • response.text() then JSON.parse is the right pattern for handling optional response bodies
  • The 204 early-return is preserved and unaffected
  • All three test files consistently updated
  • revoke.ts command logic did not need to change

@sacOO7 sacOO7 changed the title Fix keys revoke to use correct Control API endpoint [DX-1101] Fix keys revoke to use correct Control API endpoint Apr 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant