fix(api): block API keys on account-level routes#6397
Merged
Conversation
API keys are namespace-scoped tokens and should not be usable on account-level routes. Four routes were missing the BlockAPIKey middleware, allowing API keys to reach handlers they should never reach. The most concrete gap was ListAPIKeys: an API key could enumerate all other API keys in the namespace. The remaining three routes (GetNamespaceList, CreateNamespace, LeaveNamespace) were safe by accident -- the service layer rejected them -- but returned confusing 404s instead of a clean 403. Added BlockAPIKey to all four routes, matching the pattern already used by CreateAPIKey, UpdateAPIKey and DeleteAPIKey. Removed the x-internal overrides and api-key security entries from the OpenAPI spec for the same operations, since the spec now accurately reflects what the code enforces. Removed TestListNamespacesAPIKeyScope, which tested the old scoped behavior that no longer applies.
|
Claude finished @geovannewashington's task in 31s —— View job Code Review CompleteReviewed 6 files across code quality, security, testing, language patterns, and architecture — no issues found. The code looks good as-is. Highlights:
If you push additional changes and want a new review, tag |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Added
BlockAPIKeymiddleware to four routes that were missing it:ListAPIKeys,GetNamespaceList,CreateNamespace, andLeaveNamespace. Removed thex-internaloverrides andapi-keysecurity entries from the OpenAPI spec for the same operations.
Why
API keys are namespace-scoped and not tied to a user. These routes
are account-level actions that should never accept an API key.
ListAPIKeyswas the real gap -- it actually succeeded, allowing anAPI key to enumerate all other keys in the namespace. The other three
were safe by accident (the service layer rejected them) but returned
confusing 404s instead of a clean 403.
Closes #6390. Follow-up to #6392 .
Testing
./bin/docker-compose upcurl -v -H "X-API-Key: <key>" http://localhost/api/namespaces/api-keycurl -v -H "X-API-Key: <key>" http://localhost/api/namespacescurl -v -H "X-API-Key: <key>" -X POST http://localhost/api/namespacescurl -v -H "X-API-Key: <key>" -X DELETE http://localhost/api/namespaces/<tenant>/members