feat(keys): enforce explicitly requested scope prerequisites - #52
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35e80a9415
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const prefix = config.prefix ?? 'ak_' | ||
| const validScopes = config.validScopes ?? ['chat'] | ||
| if (validScopes.length === 0) throw new TypeError('At least one API key scope is required') | ||
| const dependencies = new Map(Object.entries(config.scopeDependencies ?? {})) |
There was a problem hiding this comment.
__proto__ dependency entry
When an app uses the explicitly supported __proto__ scope and configures it with the natural typed literal scopeDependencies: { __proto__: ['read'] }, JavaScript treats that entry as the object's prototype, so Object.entries returns no pair. The router consequently skips both configuration validation and dependency enforcement and can mint a __proto__-only key; use or require a representation that reliably retains this property.
Useful? React with 👍 / 👎.
| ``` | ||
|
|
||
| Missing prerequisites return HTTP 400 with `api_key.scope_dependency` before key creation. | ||
| The issuer never adds permissions the caller did not request. |
There was a problem hiding this comment.
This guarantee contradicts the existing behavior in src/api-keys.ts:259-263: when a caller explicitly supplies only invalid scopes, the issuer discards them and inserts defaultScope (a case asserted by tests/api-keys.test.ts:204-218). Consumers therefore cannot rely on every issued permission having appeared in the request; either distinguish an omitted scope list from an invalid explicit list or qualify this statement to refer only to dependency expansion.
Useful? React with 👍 / 👎.
|
Both review findings are addressed in acf0168. Nonstandard dependency-record prototypes are rejected at router setup, including the ambiguous Final source validation: 510 tests, typecheck, and build pass under Node 24.18.0. @codex review the current delta. |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
Independently reviewed the issuer dependency validation and its final prototype handling. Missing prerequisites are rejected before key generation/storage; transitive requirements are enforced by checking each explicitly selected scope. Own-entry Map lookup preserves prototype-named scopes, and plain-record validation rejects ambiguous object-literal prototype configuration. Sparse prerequisites fail startup validation. README now accurately distinguishes dependency rejection from the existing default-scope behavior.
Reproduced the earlier inherited-property HTTP 500, verified its correction, and independently ran tests/api-keys.test.ts on this head: 34 tests pass. No remaining blocking finding in this scoped change.
2 participants
Apps need Run keys to include Read, but the shared issuer could mint unusable Run-only keys. Add optional
scopeDependenciestocreateApiKeyRoutes, rejecting incomplete grants with HTTP 400 before storing them. Permissions must be explicitly requested; the issuer never expands them.The change validates configured prerequisites and uses own entries so legitimate scopes such as
toStringremain usable. GTM, Creative, and Legal can share this enforcement instead of adding local issuer validators.Validation: 508 tests pass, typecheck and build pass under Node 24.18.0. The prerequisite regressions fail against the predecessor. Independent review reproduced and resolved the inherited-property crash; regressions cover prototype-named scopes and sparse configuration. Package 0.11.2 publishes after a reviewed merge and release tag.