fix(keys): require future expiry for configured scopes - #53
Conversation
|
Independent source review of exact head22d5b6369c708c342b813b038676c6e49c29cf42: no blocking findings. I checked constructor validation, configured and normalized/default scope selection, existing date parsing, rejection before storage, and preserved legacy chat behavior. The optional policy rejects unavailable scopes and sparse entries. Missing/null/invalid/past/exact-now expiry and valid future expiry are covered with stored-row assertions. I independently ran |
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
Independent exact-head review receipt: #53 (comment). Reviewed normalized/default scopes, constructor validation including sparse input, future expiry rejection before storage, preserved legacy chat behavior, and independently passing issuer tests. No blocking 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: 22d5b6369c
ℹ️ 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 keyHash = await hashKey(rawKey) | ||
| const keyPrefix = rawKey.slice(0, prefix.length + 8) | ||
|
|
||
| if (scopes.some(scope => expiryScopes.has(scope)) && (!expiresAt || expiresAt.getTime() <= Date.now())) { |
There was a problem hiding this comment.
When the bundled SqlApiKeyStore is used and expiresAt is less than one second in the future, this check accepts the request, but toSqlTimestamp in src/api-key-store-sql.ts floors the value to whole seconds. The route therefore returns 201 with a stored key whose expiry is already past and which verifyApiKeyFromStore immediately rejects. Compare using the store's second-level precision or otherwise ensure the persisted expiry remains future.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Corrected in PR54 (merged6893f3d); the policy compares expiry at whole-second SQL precision. An actual SqlApiKeyStore regression failed before the fix and now verifies rejection without storage plus accepted next-second persistence. Release0.11.4 publication is running.
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. |
2 participants
Native operator authentication requires finite expiry, but direct issuance previously accepted unexpiring keys. Add requireExpiryForScopes to enforce a future expiry for matching normalized scopes before storage while preserving optional expiry for other scopes.
Validation: typecheck, all 523 tests across 31 files, and build pass. Regression cases cover missing, null, malformed, past, exact-now, future, mixed and default scopes, plus invalid policy configuration. New expiry cases fail against the predecessor implementation.