fix(keys): register :reveal/:enable/:renew before generic CRUD#1155
Merged
Conversation
… 400 NaN
Hono's RegExpRouter resolves overlapping route matches in registration order.
The generic `GET /keys/{keyId}` registered via `keysRouter.openapi(...)`
auto-installs a Zod-validating middleware; with the more specific
`/keys/:keyId{[0-9]+:reveal}` registered AFTER it, every reveal request
matched the generic route first, captured `keyId="155:reveal"`, and was
rejected as `expected number, received NaN` before ever reaching `revealKey`.
The handler-level suffix strip in `parseKeyParams` was correct but never ran.
Move the three custom-method routes (`:enable`, `:renew`, `:reveal`) ahead
of the generic GET/PATCH/DELETE block. Add a route-level integration test
for `:reveal` (the gap that let this regression slip through) and an
OpenAPI doc assertion. Regenerated `openapi-types.gen.ts` reflects only the
path-ordering change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthrough重新排列密钥路由注册顺序，使带方法后缀的路由（ Changes密钥API路由与类型更新
预估代码审查工作量🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
bug
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request reorders the API routes in the keys router to ensure that custom-method routes, such as :reveal, are registered before generic CRUD routes. This change prevents Hono's RegExpRouter from incorrectly capturing custom methods as part of the keyId parameter. Additionally, the PR introduces a new test case for the :reveal endpoint to verify its response status, JSON output, and security headers, while also updating the OpenAPI documentation checks. I have no feedback to provide.
Contributor
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Summary
No significant issues identified in this PR.
PR Size: M
- Lines changed: 401
- Files changed: 3
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Adequate
- Code clarity - Good
Automated review by Codex AI
Contributor
There was a problem hiding this comment.
- Reviewed PR
#1155on branchfix/keys-reveal-route-order. - Computed PR size as M (401 lines changed across 3 files) and applied label
size/M. - Completed 6-perspective diff review of the changed files and submitted a PR review comment; no diff-line issues met the reporting threshold, so no inline comments were posted.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/api-client/v1/openapi-types.gen.ts`:
- Around line 33196-33197: Update the JSDoc for the "key" property (the Unmasked
key value in openapi-types.gen.ts) to accurately state that the unmasked key is
returned to admins and to the key's owner (not only admins); reference the
authorization behavior implemented by getUnmaskedKey in src/actions/keys.ts and
the keys router in src/app/api/v1/resources/keys/router.ts to ensure the
description matches the 403 behavior for non-owner/non-admin callers.
- Line 33715: The generated OpenAPI type for expiresAt is wrong because union
with unknown collapses to unknown; update the OpenAPI type generator logic (the
code that maps Zod unions to TS unions) so that z.union([z.string(), z.null()])
yields "string | null" instead of "string | unknown" (fix the mapping for Zod's
null union case used by the keys schema that produces expiresAt). Also update
the JSDoc text that currently reads "Returned only to admin callers" (near the
JSDoc for the keys endpoint in openapi-types.gen.ts) to reflect the actual
access rules (admins and key owners) as implemented in the router
(src/app/api/v1/resources/keys/router.ts).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0782ca69-3ea1-463a-be57-a5fede778b2e
📒 Files selected for processing (3)
src/app/api/v1/resources/keys/router.tssrc/lib/api-client/v1/openapi-types.gen.tstests/api/v1/keys/keys.test.ts
- expiresAt: switch from `z.union([z.string(), z.null()])` to `z.string().nullable()` so the generated TS type is `string | null` instead of `string | unknown` (which collapses to `unknown` and silently disables type checking on the field). - KeyRevealResponseSchema.key: correct the description — admins AND the key's owner can reveal; non-owners receive 403. The previous "Returned only to admin callers" was misleading consumers. Regenerated openapi-types.gen.ts to reflect both source changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
🧪 测试结果
总体结果: ✅ 所有测试通过 |
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
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
1 participant
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.
Summary
src/app/api/v1/resources/keys/router.tsso the custom-method routes (/keys/{id}:reveal,:enable,:renew) register before the genericGET/PATCH/DELETE /keys/{keyId}block.GET /api/v1/keys/{id}:revealintests/api/v1/keys/keys.test.ts(plus an OpenAPI doc assertion). The pre-existingkeys.crud.test.tsonly string-matches the test file viareadFileSync, so it could not catch this regression.src/lib/api-client/v1/openapi-types.gen.tsto reflect the path ordering — content unchanged, only path-block order shifts.Related Issues & PRs
keys:revealendpoint; this PR fixes a route ordering regression that caused the reveal functionality to return 400 errors.Root Cause
Reproduces
GET /api/v1/keys/155:reveal→ 400request.validation_failed,keyId expected number received NaNon the user management page.Hono's RegExpRouter resolves overlapping matches in registration order. With the generic
GET /keys/{keyId}registered first viakeysRouter.openapi(...), its auto-Zod-validating middleware capturedkeyId="155:reveal",z.coerce.number()produced NaN, and thedefaultHookreturned the 400 the user saw. The specific/keys/:keyId{[0-9]+:reveal}route registered later was never hit, so the suffix-stripping logic inparseKeyParamsnever ran.POST /keys/{id}:enableand:reneware not currently affected only because the generic CRUD routes use GET/PATCH/DELETE — moving them up too defends against a future generic POST route silently re-introducing the same bug.Solution
Reordered route registrations to ensure custom-method routes with colon-suffixes (
:reveal,:enable,:renew) are registered before the generic/keys/{keyId}CRUD routes. This ensures Hono's RegExpRouter matches the more specific patterns first.Changes
Core Fix
src/app/api/v1/resources/keys/router.ts— Moved custom-method route registrations (enable, renew, reveal) above generic CRUD routes (GET/PATCH/DELETE/keys/{keyId})Test Coverage
tests/api/v1/keys/keys.test.ts— Added integration test forGET /api/v1/keys/{id}:revealendpoint with cache-control header assertionsGenerated Types
src/lib/api-client/v1/openapi-types.gen.ts— Regenerated to reflect path ordering changes (no functional changes)Test plan
bunx vitest run tests/api/v1/keys/keys.test.ts— 11/11 pass, includes newreveals unmasked key value with no-store headerscase.bun run typecheck— clean.bun run lint— no new warnings (8 pre-existing, all in unrelated files).bun run openapi:check— generated types in sync.bun run openapi:lint— passes.bun run build— production build succeeds.bun run dev, click copy/eye on a key in user management — no 400 toast, key is revealed/copied.Description enhanced with related issue links
Greptile Summary
Route ordering regression fix: custom-method routes (
:reveal,:enable,:renew) now register before the generic CRUD block, preventing Hono's RegExpRouter from capturing colon-suffixed paths and failing Zod coercion. Also adds a missing integration test for the reveal endpoint, simplifies theexpiresAtZod union type, and regenerates OpenAPI types.Confidence Score: 5/5
Safe to merge — targeted route-ordering fix with matching test coverage and no logic regressions.
No P0 or P1 findings. The route reordering is the correct solution for Hono's RegExpRouter behaviour, the schema change is semantically equivalent, the generated types are in sync, and the new integration test directly exercises the repaired code path.
No files require special attention.
Important Files Changed
Sequence Diagram
sequenceDiagram participant C as Client participant R as Hono RegExpRouter participant SR as Specific route (colon-suffix pattern) participant GR as Generic route /keys/{keyId} participant H as Handler Note over R: Before fix C->>R: GET /keys/155:reveal R->>GR: Matched generic route first GR-->>C: 400 - NaN coercion failure Note over R: After fix C->>R: GET /keys/155:reveal R->>SR: Matched specific route first SR->>H: Strips suffix, parses id H-->>C: 200 with unmasked valueReviews (2): Last reviewed commit: "fix(keys): address PR review on schema a..." | Re-trigger Greptile