Skip to content

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

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

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

alukach added a commit that referenced this pull request Oct 5, 2026
## 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. The six routes that still touch the table only do
harm: two POSTs mint new plaintext secrets, `GET
/api-keys/{access_key_id}/auth` hands a key's plaintext secret to an
admin, and `DELETE /api-keys/{access_key_id}` deletes nothing while
answering "API key deleted successfully".

This PR removes the subsystem outright:

- **Routes** (under `/api/v1`): `POST`/`GET
/accounts/{account_id}/api-keys`, `POST`/`GET
/products/{account_id}/{repository_id}/api-keys`, `GET
/api-keys/{access_key_id}/auth` and `DELETE /api-keys/{access_key_id}`.
All now 404.
- **Code:** the `APIKey`, `APIKeyRequest` and `RedactedAPIKey` schemas,
the `apiKeysTable` client,
`generateAccessKeyID`/`generateSecretAccessKey` (all of
`src/lib/actions/crypto.ts`, which nothing else calls), the
`api_key:get|create|revoke` and
`account:listAPIKeys`/`repository:listAPIKeys` authz actions with their
checks and tests, and the api-key mocks in the product route tests.
- **OpenAPI:** the `ApiKeyAuth` security scheme, which described the
`<access-key-id> <secret-access-key>` header this API stopped accepting
in #283. No replacement scheme is added here.
- **Infrastructure:** the `api-keys` table in `DatabaseConstruct` and
its Vercel grant in `api-stack.ts`, its creation in
`scripts/init-local.ts`, and `fixtures/api-keys.json`.

Untouched: `ORY_PROJECT_API_KEY` and the proxy's Ory `adminApiKey`,
which are unrelated, and the service-account API keys
(`service-account-keys` table) from #567/#580. No UI ever managed legacy
keys, so there is no UI or Storybook change.

## Decisions to flag

**No deprecation window.** #582 proposed one release of
`Deprecation`/`Sunset` headers on the list routes and a working DELETE.
This PR goes straight to removal instead: the keys authorize nothing, so
the only clients the window protects are scripts listing keys that do
nothing. Merging this supersedes #582.

**What happens to the table on deploy.** `api-stack.ts` gives production
`RemovalPolicy.RETAIN`, so dropping the table from CDK leaves the prod
`api-keys` table in place, no longer managed by the stack, plaintext
secrets included. Dev and staging use `DESTROY`, so their tables are
deleted on the next deploy. Once this ships, the prod table still needs
auditing and then deleting out of band (see Follow-ups).

**`fix`, not `chore`.** release-please leaves `chore` out of the
changelog, and the release notes are where removing public routes gets
announced.

## How you can test it

What I ran on this branch, rebased onto `main` at 5c35162:

- `npm run type-check`: clean.
- `npx jest --forceExit`: 83 suites, 873 tests pass.
- `npm run lint`: warnings only, none on lines this PR touches.
- A grep for `api-keys`, `apiKeysTable`, `api_key:`, `listAPIKeys`,
`APIKeySchema`, `ApiKeyAuth` and the crypto helpers across `src`,
`scripts`, `deploy` and `fixtures` finds nothing left. The remaining
`secret_access_key` hits are data-connection credentials.

Not run: `npm run build` (it needs AWS credentials), `cdk diff`, and
nothing against a preview or staging. Expected `cdk diff` against prod:
the `api-keys` table resource and its grant on the Vercel role
disappear, with the table retained.

By hand on the preview: `(await
fetch("/api/v1/accounts/<you>/api-keys")).status` gives 404.

## Docs and ADRs

- **docs.source.coop:** a search of the repo finds no page describing
legacy keys; `docs/using-source/data-upload.md` covers STS session
credentials only. Nothing is invalidated.
- **data.source.coop ADRs:** ADR-013's context note says the `api-keys`
endpoints that "exist in `source.coop` today" are legacy, unrelated to
its design, and not accepted by the proxy (ADR-001). Once this merges,
"today" is out of date, but the note still records the context correctly
and no decision moves, so no ADR changes.
- **Out of date once the prod table is deleted:** the org profile README
(`profile/README.md` in `source-cooperative/.github`) lists `api-keys:
Stores API keys for authentication`.

## Follow-ups

1. Audit the retained prod `api-keys` rows, then delete the table out of
band (point-in-time recovery keeps it restorable for its retention
window).
2. Drop `api-keys` from the org profile README's table list.

## Related

Closes the route and table work in #549; part of #491. Supersedes #582.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
@alukach

alukach commented Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

Overtaken by #374

@alukach alukach closed this Oct 9, 2026
@alukach
alukach deleted the chore/retire-legacy-api-keys branch October 9, 2026 05:29
alukach added a commit that referenced this pull request Oct 17, 2026
…665)

Closes #594. Part of #588.

## What

Service accounts and their API keys over `/api/v1`, documented at
`/api/docs`:

| Endpoint | Operation |
|---|---|
| `GET /accounts/{owner}/service-accounts` | `listServiceAccounts` →
`{items, next_cursor: null}` |
| `POST /accounts/{owner}/service-accounts` | `createServiceAccount` —
`local_id`, `name`, `github_subjects[]`, `grants[{product_id, role}]`,
optional `key{label, expires_at}`; the response carries the issued key
when one is asked for |
| `GET /accounts/{sa}/trusts` | `listTrusts` → `{items, next_cursor:
null}` |
| `POST /accounts/{sa}/trusts` | `addGithubTrust` — `{subject}` |
| `DELETE /accounts/{sa}/trusts?issuer=&subject=` | `removeTrust` — 404
for a trust the account doesn't have; returns the trust as it was |
| `GET /accounts/{sa}/keys` | `listKeys` → `{items, next_cursor: null}`,
revoked and expired keys included, never a hash |
| `POST /accounts/{sa}/keys` | `issueKey` — `{label, expires_at}` → 201
with `key`, the only time it appears |
| `PATCH /accounts/{sa}/keys/{key_id}` | `updateKey` — `{expires_at?,
revoked?: true}` |

`src/lib/operations/service-accounts.ts` holds the rules the issue
names: `GITHUB_ACTIONS_SUBJECT_REGEX` (as a Zod schema shared with the
OpenAPI doc), the read/write-only grant roles, `managedServiceAccount`
(every "not yours / missing / not a service account" answers 404,
keeping the single answer it gave before), and the rule that a service
account can't own another. The server actions in
`src/lib/actions/service-accounts.ts` now adapt FormData into those
operations, and the service-accounts settings page lists through
`listServiceAccounts`. Editing and deleting a service account are main's
`PATCH` and `DELETE /accounts/{account_id}` (#674, #675): rename and
disable call `updateAccount`, and delete calls `deleteAccount`, so this
PR adds no operation of its own for them. Trusts are returned through
`AccountTrustSchema`, so the table's `identity` sort key stays out of
responses.

`setProductAccess` now calls `inviteMember` / `updateMembership` /
`revokeMembership`, and creation grants each product through
`inviteMember` too, so every grant passes the same authorization,
duplicate check and audit log; so product access over the API is the
product members endpoints from #589 with role `read_data` or
`write_data`, and gets no endpoint of its own.

API keys were first left to #570 and #580–#582, which built the keys and
their settings UI but no API; with those merged, this PR adds the API
for them (scope noted on #594). `issueApiKey`, `revokeApiKey`,
`setApiKeyExpiry` and the create action now map form fields onto
`issueKey`, `updateKey` and `createServiceAccount`, so no action touches
the keys table, and the key rules (hash-only storage, the six-character
hint, refusing a disabled account, the expiry range) are tested once, on
the operations.

## Worth a look

- Revoking is `PATCH {"revoked": true}`, not `DELETE`: the record stays,
with `revoked_at`, `revoked_via` and `revoked_by`, as disabling an
account is a `PATCH`. `revoked: false` is refused with 400, since a
revoked key can't be restored; revoking one twice is 409, and a PATCH
with neither field is 400.
- `expires_at` is an ISO time, at most 3650 days away, or `null` for a
key that lasts until revoked. It is required on issue, so a caller
chooses no-expiry deliberately. The settings form still takes days and
converts them.
- A key requested with `POST /service-accounts` is validated with the
rest of the body, before anything is written, and stored after the
grants and trusts.
- `createServiceAccount` checks every grant against `InviteMembership`
before it writes the account, so a grant the caller may not make leaves
nothing behind and a retry with the same `local_id` succeeds.
`inviteMember` can refuse nothing else afterwards: the account is new
and enabled, and the grants are deduplicated first.
- Deleting a service account is main's `deleteAccount` on both the API
and the settings page. It deletes the account's API keys rather than
revoking them, and the key exchange also refuses a key older than its
account, so a key from a deleted account can't sign in as the next
account to take its id. ADR 013 still holds.
- `setProductAccess` refuses a new grant to a disabled service account
(#666) through `inviteMember` and `updateMembership`, which both check
`serviceAccountGrantProblem`. Revoking still works while the account is
disabled.
- Service-account lists use the `{items, next_cursor}` page shape, as
products and data connections do, so pagination can arrive later without
a breaking change.

## Docs, ADRs, llms.txt

- docs.source.coop `docs/using-source/upload/with-service-account.md`
walks through the settings UI, which is unchanged, so it still holds; it
could later mention issuing a key over the API, which #595's reference
covers meanwhile. The API reference is #595's to publish.
- data.source.coop ADRs: I checked 005 (authorization), 010
(account-owned roles) and 013 (API keys). ADR 013 has source.coop
generate the key, store only its hash, show it once, refuse a disabled
service account and keep expiry editable, with `key_id` as the public
handle; the key endpoints do exactly that, so this PR implements
existing decisions and moves none.
- `public/llms.txt`: no change. These endpoints all need auth, so they
can't go on the file's no-auth URL list, and its existing "API
reference" line points at `/api/openapi`, which now includes them.

## Storybook

No component changed. The settings page now gets its list from the
operation, and the key actions keep their form fields and state shapes,
so `IssueApiKeyDialog` and the rest are untouched.

## Testing

- `npm run type-check`: clean.
- `npx jest`: 96 suites, 1068 tests pass. That includes the
service-account action tests, now run through the operations; the key
action tests, now checking only the form mapping; new operation tests
(list authz, JSON create, the 404 on an unknown trust, a refused grant
or bad key writing nothing, issuing, listing, expiring and revoking
keys); and the OpenAPI validity and route-coverage tests.
- Not exercised against a running server.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

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