Skip to content

fix(api): stop minting legacy API keys and make revoking one delete it - #582

Draft
alukach wants to merge 1 commit into
mainfrom
chore/retire-legacy-api-keys
Draft

alukach wants to merge 1 commit into
mainfrom
chore/retire-legacy-api-keys

Conversation

@alukach

@alukach alukach commented Sep 25, 2026

Copy link
Copy Markdown
Contributor

What I'm changing

The legacy API keys in the api-keys table grant no access. The data proxy stopped accepting them in source-cooperative/data.source.coop#116, and this API has accepted only the proxy's signed tokens and session cookies since #283. Six routes still touch the table, though: two POSTs mint new plaintext secrets, and DELETE /api/v1/api-keys/{access_key_id} authorizes the caller, deletes nothing, and answers "API key deleted successfully", so every key a user believes they revoked is still in the table in plaintext.

This PR retires those routes (paths under /api/v1):

Route Before After
POST /accounts/{account_id}/api-keys mints a key 410 Gone with a JSON error pointing to service-account API keys; reads no session or body, writes nothing
POST /products/{account_id}/{repository_id}/api-keys mints a key 410 Gone, as above
GET /accounts/{account_id}/api-keys lists redacted keys same response, plus Deprecation and Sunset headers
GET /products/{account_id}/{repository_id}/api-keys lists redacted keys same response, plus Deprecation and Sunset headers
GET /api-keys/{access_key_id}/auth returns a key's plaintext secret to an admin 410 Gone
DELETE /api-keys/{access_key_id} authorizes, deletes nothing, claims success deletes the row, with Deprecation and Sunset headers; 500 rather than success if the delete fails

Every 410 answers {"error": "Legacy API keys are retired and grant no access. For software that needs access, create a service account in your account settings and issue it an API key."}.

Code with no remaining caller goes: generateAccessKeyID and generateSecretAccessKey (all of src/lib/actions/crypto.ts), APIKeyRequestSchema, the api_key:create action with its authz rule and test, and the table client's create, update and upsert. The table client keeps fetchById and listByAccount for the routes that still serve, and gains delete. No UI ever managed these keys, so there is no UI or Storybook change.

The table stays. deploy/, scripts/init-local.ts, fixtures/api-keys.json and the fixture loader that seeds it are untouched; dropping the table is sequenced after the routes are gone (see Follow-ups).

Decisions to flag

The list GETs keep serving for one release, with Deprecation and Sunset headers. They return only redacted fields, so serving them costs nothing, and they are how an owner finds a key's id in order to delete it. Answering 410 at once would break any script without notice and leave owners no way to find their keys. The headers are Deprecation: @1790294400 (2026-09-25; RFC 9745) and Sunset: Sun, 01 Nov 2026 00:00:00 GMT (RFC 8594). The sunset date is my choice: about five weeks out, which on the recent release cadence (v1.5.0 on 4 Aug to v1.6.1 on 24 Sep, with gaps of 9 to 25 days) spans the release carrying this PR and the one after. The follow-up that removes the routes should not ship before it; if this PR merges late, move the date so the window still spans a release.

GET /api-keys/{access_key_id}/auth answers 410 now, which departs from the issue's "deprecation response for the GET routes". It is not a listing. It was the pre-Workers proxy's credential lookup, and it returns a key's secret_access_key in plaintext to an admin. Its only caller was removed in source-cooperative/data.source.coop#116, and that caller authenticated with a raw API key header that getApiSession stopped accepting in #283, so no client can depend on it. A deprecation window would keep serving plaintext secrets to protect a caller that cannot exist.

DELETE deletes, rather than answering 410. It is the smallest honest fix, and with the list routes it lets owners purge their plaintext keys during the window instead of waiting for the table drop. It keeps its authorization (api_key:revoke) and its 200 body, which is now true. That rule still refuses a non-admin on a key marked disabled; no route sets the flag, and an admin can delete any such key. Rows deleted this way are gone before the audit below, though point-in-time recovery keeps them restorable for its retention window.

fix, not chore. release-please leaves chore out of the changelog, and the release notes are where the deprecation window gets announced.

OpenAPI. Each changed route's @openapi JSDoc marks it deprecated: true and describes its new behaviour; the account route gains the JSDoc it lacked. That JSDoc does not reach the served spec: /api/openapi has served paths: {} since swagger-jsdoc was removed in #369. The served spec changes only in its components. APIKey, which published secret_access_key, and APIKeyRequest go; RedactedAPIKey stays for the list routes; and the ApiKeyAuth security scheme goes, because it described the <access-key-id> <secret-access-key> header this API stopped accepting in #283. No replacement scheme is added here.

The 410 points to service-account API keys, which ship with #570. Service accounts are already on main (#567). If this merges before #570, the pointer is one step ahead until #570 lands.

A quirk left alone: GET /accounts/{account_id}/api-keys lists the signed-in account's keys whatever account_id names, as it always has. Its new JSDoc says "the signed-in account's". Fixing a route that is going away isn't worth it.

How you can test it

What I ran on this branch:

  • npm run type-check: clean.
  • npx jest api-keys src/lib/api/authz.test.ts "src/app/api/v1/products/\[account_id\]/route.test.ts" "src/app/api/v1/products/\[account_id\]/\[repository_id\]/route.test.ts" --forceExit: 8 suites, 80 tests pass.
  • npx jest --forceExit (the whole suite): 79 suites, 813 tests pass.
  • npm run lint: exits 0. It prints warnings only, and none on lines this PR adds.
  • A mutation check: deleting the await apiKeysTable.delete(...) line, or only its await, fails the DELETE suite.

The tests cover: 410 on each POST and on /auth; the list GETs still return redacted keys and carry both headers; DELETE deletes the row when authorized, deletes nothing when the caller may not (401) or the key is missing (404), and answers 500 when the delete fails; APIKeysTable.delete sends a DeleteCommand keyed by access_key_id to the right table.

Not run: npm run build (it needs AWS credentials), and nothing against the preview, staging or a local DynamoDB.

By hand, from a signed-in browser console on the preview: (await fetch("/api/v1/accounts/<you>/api-keys", { method: "POST" })).status gives 410; (await fetch("/api/v1/accounts/<you>/api-keys")).headers.get("Sunset") gives the sunset date; and after fetch("/api/v1/api-keys/<id>", { method: "DELETE" }) the key no longer appears in the list.

Docs and ADRs

  • docs.source.coop: I searched main for API-key instructions. No page describes the legacy keys: docs/using-source/data-upload.md covers STS session credentials only, and already tells users not to request permanent access keys. Nothing is invalidated.
  • data.source.coop ADRs: I checked ADR-001, and ADR-013 both on main and as revised in docs(adr): revise ADR-013 — API keys are opaque secrets resolved by the platform data.source.coop#234. ADR-013's context note says the api-keys endpoints are legacy, unrelated to its design, and not accepted by the proxy. That still holds. This PR moves no decision, so no ADR changes.
  • Stale once the table drops, not because of this PR: the org profile README (profile/README.md in source-cooperative/.github) lists api-keys: Stores API keys for authentication.

Follow-ups

These follow #549's sequence, each as its own change after this one ships:

  1. After the sunset date, remove the six routes, the table client, APIKeySchema and RedactedAPIKeySchema, the remaining api_key:* and *:listAPIKeys authz actions, fixtures/api-keys.json and its loader.
  2. Audit the retained rows before deleting them.
  3. Drop the table from CDK (deploy/lib/database-construct.ts, and the Vercel grant in deploy/lib/api-stack.ts) and from scripts/init-local.ts. Production sets RETAIN, with point-in-time recovery on, so then delete the table out of band.
  4. Update the org profile README's table list.

Related

Part of #549: the routes are not gone yet, and the table drop, delete and audit remain. Part of #491. This re-cuts the stale draft #374 from current main instead of rebasing it; #374 is left as it is.

🤖 Generated with Claude Code

https://claude.ai/code/session_01R1eiTse4416N6uTgAy4Ddd

Legacy API keys grant no access: the data proxy has not accepted them since source-cooperative/data.source.coop#116, and this API has accepted only the proxy's signed tokens and session cookies since #283. Yet both create routes still minted plaintext secrets, and DELETE /api/v1/api-keys/{access_key_id} authorized the caller, deleted nothing, and answered "API key deleted successfully".

POST /api/v1/accounts/{account_id}/api-keys and POST /api/v1/products/{account_id}/{repository_id}/api-keys now answer 410 Gone with a JSON error pointing to service-account API keys, without reading the session or the body. GET /api/v1/api-keys/{access_key_id}/auth answers 410 too: its only caller was the pre-Workers proxy, which authenticated with a credential this API no longer accepts, and it was the one route that returned a secret_access_key.

The two list routes keep serving redacted keys for one release with Deprecation (RFC 9745) and Sunset (RFC 8594) headers, so owners can find their keys. DELETE now deletes the row, carries the same headers, and answers 500 rather than success when the delete fails.

Code with no remaining caller goes: generateAccessKeyID and generateSecretAccessKey, APIKeyRequestSchema, the api_key:create action and its rule, and the table client's create, update and upsert. The served OpenAPI components drop APIKey and APIKeyRequest, and the ApiKeyAuth scheme that described the retired credential. The api-keys table, its CDK definition and its local seed stay until the routes are removed.

Part of #549.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R1eiTse4416N6uTgAy4Ddd
@vercel

vercel Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

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

Project Deployment Actions Updated
source-coop-ui Ready Ready Preview Sep 25, 2026 9:09pm UTC
source-cooperative Ready Ready Preview Sep 25, 2026 9:09pm UTC

Request Review

@claude

claude Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @alukach's task in 1m 19s —— View job


Review

  • Gather context (PR description, diff)
  • Review correctness/security/best-practice issues in the diff
  • Apply ponytail-review for over-engineering
  • Check docs/ADR requirement per CLAUDE.md
  • Post final review

✅ No blocking issues — safe to merge.

Went through the full diff (git diff origin/main...HEAD) route by route:

  • DELETE /api-keys/{id}: now actually deletes (apiKeysTable.delete) before returning 200, and a delete failure hits the generic catch → 500, matching the PR description. Authorization order (fetch → authz check → delete) is unchanged and correct.
  • Both POST routes now short-circuit to legacyApiKeysGone (410) before touching session/body — verified via the tests asserting getApiSession is never called.
  • GET /api-keys/{id}/auth correctly drops the plaintext-secret-returning handler entirely rather than keeping it behind a deprecation window, consistent with the stated rationale (no caller can reach it post-feat: OIDC auth #283).
  • List routes (GET on accounts/products) keep working, now carry Deprecation/Sunset headers via the shared LEGACY_API_KEY_DEPRECATION constant.
  • Checked for dangling references to everything removed (generateAccessKeyID, generateSecretAccessKey, APIKeyRequestSchema, Actions.CreateAPIKey, apiKeysTable.create/update/upsert, ApiKeyAuth) — none remain anywhere in src/ or scripts/.
  • fixtures/api-keys.json, scripts/init-local.ts, and deploy/ are untouched, as the description says.
  • New APIKeysTable.delete test confirms it sends a DeleteCommand keyed correctly on access_key_id against the right table name.
  • The @1790294400 Deprecation timestamp decodes to 2026-09-25, matching today and the PR's stated rationale; Sunset (2026-11-01) is after it, so the header pair is internally consistent.

Nothing here reads as a correctness, security, or best-practice problem.

Simplify (ponytail): nothing to flag — this PR is net −316 lines, the new legacy-api-keys.ts helper is the minimum needed to share the 410 body and deprecation headers across five routes, and no new abstraction or dependency was introduced.

Docs: The description already covers this per CLAUDE.md's requirement — it states docs.source.coop has no page describing legacy keys (checked docs/using-source/data-upload.md), and that ADR-001/ADR-013 in data.source.coop still hold (ADR-013 already treats these endpoints as legacy/unrelated to the proxy's design). No ADR or doc change is needed for this diff, and the description says why. Nothing further to add.


💰 Estimated review cost: $0.57 · 1m19s · 25 turns

This branch was successfully deployed

2 active deployments
Preview – source-cooperative — 8c3997fb Deployed Sep 25, 2026 by vercel[bot]
Preview – source-coop-ui — 8c3997fb Deployed Sep 25, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant