fix: add 403 error when 0 key verification perms #4483
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
📝 WalkthroughWalkthroughAdds an early root-key permission check to the keys.verify endpoint, updates OpenAPI descriptions to clarify 403/NOT_FOUND behaviors, introduces KeyVerifier.HasAnyPermission, and adds/adjusts tests covering root-key permission permutations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTP_Handler as Handler
participant KeyVerifier
participant KeyStore as DataStore
Client->>Handler: POST /v2/keys/verify (Authorization: root key)
Handler->>KeyVerifier: HasAnyPermission(resource: "api", action: "verify")
alt Root key has no verify permission
KeyVerifier-->>Handler: false
Handler-->>Client: 403 Forbidden (auth.VerifyRootKey)
else Root key has verify permission (wildcard or for API)
KeyVerifier-->>Handler: true
Handler->>KeyStore: Lookup target key
alt Key not found or permissions mismatch by API
KeyStore-->>Handler: not found
Handler-->>Client: 200 with code NOT_FOUND (avoid leakage)
else Key found and valid
KeyStore-->>Handler: key data
Handler-->>Client: 200 VALID
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
go/apps/api/openapi/openapi-generated.yaml (1)
5436-5449: Fix inconsistency: endpoint no longer “always returns 200”.The description still claims “Always returns HTTP 200” (Line 5436), but you’ve added a documented 403 case (Line 5449). Please update the text to reflect the exception.
Suggested wording (update the source OpenAPI, not the generated file):
-**Important**: Always returns HTTP 200. Check the `valid` field in response data to determine if the key is authorized. +**Important**: Returns HTTP 200 for verification outcomes (success or failure). Exception: if the calling root key has no verify permissions at all, this endpoint returns HTTP 403. Otherwise, check the `valid` field in response data to determine if the key is authorized.This keeps the no‑leak posture while accurately documenting the new 403 behavior. Based on learnings, authentication/authorization errors typically map to NOT_FOUND to avoid leakage; this narrow 403 exception should be clearly called out wherever “always 200” is mentioned.
go/apps/api/openapi/spec/paths/v2/keys/verifyKey/index.yaml (1)
10-10: Clarify 200 vs 4xx behavior in the descriptionThe new note and the 403 description look consistent with the handler/tests and nicely call out the 200 +
code: NOT_FOUNDbehavior to avoid leaking key existence (which matches the broader pattern in this codebase of hiding existence via NOT_FOUND). Based on learnings, this is the right direction for avoiding information disclosure.However, the description still says this endpoint “Always returns HTTP 200” in two places (Line 10 and in the 200 response description at Line 37), which is now clearly inaccurate given the explicit 403 case for “no verify permissions at all” (and the existing 401/404/500 responses).
Consider rewording those statements along the lines of:
- “For authorized root keys, verification results are always encoded in a 200 response; use
validandcodeto distinguish outcomes. Authentication/authorization failures still use standard 4xx/5xx responses.”That keeps the main contract while making the 403/401/etc. behavior explicit and non‑contradictory.
Also applies to: 23-23, 36-37, 76-77
🧹 Nitpick comments (3)
go/apps/api/openapi/openapi-generated.yaml (1)
5499-5505: 403 semantics LGTM; add an example for parity and verify handler/tests.The 403 description precisely scopes the case (zero verify perms across all APIs). Consider adding a small example error body here, like other endpoints do for 400/403, to aid SDKs and docs consistency. Also ensure the handler returns 403 only for this case and that tests cover:
- zero verify perms → 403
- verify perms for other API → 200 with code NOT_FOUND
I can help draft the example snippet if useful.
go/internal/services/keys/validation.go (1)
80-95: HasAnyPermission implementation looks correct; minor doc‑comment nitThe helper cleanly captures the “any
<resource>.*.<action>/<resource>.<id>.<action>” pattern and should behave as intended forapi.*.verify_keyandapi.<apiId>.verify_keygiven the current naming scheme.Very small nit: the first comment line says “checks if the key has any permission matching the given action”, but the method actually matches both the
resourceTypeandaction. Consider tweaking the docstring to mention both so the exported API is fully self‑describing.No functional issues from this change.
go/apps/api/routes/v2_keys_verify_key/403_test.go (1)
15-160: Good coverage of 403 vs 200/NOT_FOUND/VALID semanticsThe test matrix here does a nice job pinning down the intended behavior:
- 403 when the root key has no verify permissions at all (including the “no permissions” root‑key case).
- 200 +
code: NOT_FOUND,valid=falsewhen the root key has verify permission, but only for a different API (preventing key‑existence leaks across APIs).- 200 +
code: VALID,valid=truefor both wildcard and API‑specific verify permissions.This aligns with the new OpenAPI note and the helper semantics in
KeyVerifier.HasAnyPermission, and with the broader pattern of masking existence via NOT_FOUND where appropriate. Based on learnings, this keeps the security story consistent.Minor nit (optional):
res.Body.Errorinopenapi.ForbiddenErrorResponseis a value field, sorequire.NotNil(t, res.Body.Error)will always pass; you could drop that assertion and rely on checkingError.Detail/Error.Statusinstead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
go/apps/api/openapi/openapi-generated.yaml(2 hunks)go/apps/api/openapi/spec/paths/v2/keys/verifyKey/index.yaml(2 hunks)go/apps/api/routes/v2_keys_verify_key/200_test.go(0 hunks)go/apps/api/routes/v2_keys_verify_key/403_test.go(1 hunks)go/apps/api/routes/v2_keys_verify_key/handler.go(1 hunks)go/internal/services/keys/validation.go(2 hunks)
💤 Files with no reviewable changes (1)
- go/apps/api/routes/v2_keys_verify_key/200_test.go
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/runbook-freshness-check.yaml:157-173
Timestamp: 2025-08-08T14:59:52.283Z
Learning: Repo unkeyed/unkey: When a CI/workflow fix is deferred, imeyer prefers a thorough GitHub issue be opened with sections (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References) and assigned to imeyer, including backlinks to the originating PR and comment.
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/job_detect_changes.yaml:111-114
Timestamp: 2025-08-08T16:10:00.224Z
Learning: Repo: unkeyed/unkey — When imeyer writes “make me an issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
📚 Learning: 2025-08-29T12:08:57.482Z
Learnt from: perkinsjr
Repo: unkeyed/unkey PR: 3880
File: apps/dashboard/app/(app)/settings/root-keys/components/root-key/root-key-dialog.tsx:137-141
Timestamp: 2025-08-29T12:08:57.482Z
Learning: Root keys in the Unkey system require at least one permission at minimum, so there is no valid case where a root key would have zero permissions. This means checking for permission existence when determining UI labels is unnecessary.
Applied to files:
go/apps/api/routes/v2_keys_verify_key/handler.go
📚 Learning: 2025-07-15T14:25:05.608Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 3560
File: go/apps/api/routes/v2_keys_create_key/handler.go:353-466
Timestamp: 2025-07-15T14:25:05.608Z
Learning: In the Unkey codebase, input validation for API endpoints is handled at the OpenAPI schema layer, which validates request fields like permission slugs (pattern: "^[a-zA-Z0-9_]+$", length: 1-100 characters) before requests reach the handler code. This validation occurs during the zen.BindBody call in handlers.
Applied to files:
go/apps/api/routes/v2_keys_verify_key/handler.go
📚 Learning: 2025-07-16T17:51:57.297Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 3617
File: go/apps/api/openapi/openapi.yaml:3309-3312
Timestamp: 2025-07-16T17:51:57.297Z
Learning: In the Unkey API OpenAPI schema, the permissions query regex for the verifyKey endpoint intentionally allows all whitespace characters (including tabs and newlines) via `\s`. Do not flag this as an error in future reviews.
Applied to files:
go/apps/api/openapi/spec/paths/v2/keys/verifyKey/index.yamlgo/apps/api/openapi/openapi-generated.yaml
📚 Learning: 2025-07-03T05:58:10.699Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3421
File: go/apps/api/openapi/openapi.yaml:196-200
Timestamp: 2025-07-03T05:58:10.699Z
Learning: In the Unkey codebase, OpenAPI 3.1 is used, which allows sibling keys (such as `description`) alongside `$ref` in schema objects. Do not flag this as an error in future reviews.
Applied to files:
go/apps/api/openapi/spec/paths/v2/keys/verifyKey/index.yamlgo/apps/api/openapi/openapi-generated.yaml
📚 Learning: 2025-09-15T19:53:28.487Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 3952
File: go/proto/ctrl/v1/routing.proto:0-0
Timestamp: 2025-09-15T19:53:28.487Z
Learning: In the Unkey codebase, authentication/authorization errors intentionally return NOT_FOUND error codes instead of distinct auth error codes (like FORBIDDEN or UNAUTHORIZED) for security reasons. This prevents attackers from distinguishing between "resource doesn't exist" and "you don't have permission to access this resource", avoiding information disclosure about workspace existence and access boundaries.
Applied to files:
go/apps/api/openapi/spec/paths/v2/keys/verifyKey/index.yamlgo/apps/api/openapi/openapi-generated.yaml
📚 Learning: 2025-08-14T16:25:48.167Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3785
File: go/apps/api/routes/v2_keys_reroll_key/401_test.go:52-61
Timestamp: 2025-08-14T16:25:48.167Z
Learning: User Flo4604 requested creation of a GitHub issue to track converting all test files to use table-driven test patterns as a broader codebase improvement, following the suggestion made during review of go/apps/api/routes/v2_keys_reroll_key/401_test.go.
Applied to files:
go/apps/api/routes/v2_keys_verify_key/403_test.go
🧬 Code graph analysis (3)
go/apps/api/routes/v2_keys_verify_key/handler.go (3)
go/pkg/db/models_generated.go (1)
Api(648-659)go/pkg/rbac/permissions.go (3)
VerifyKey(67-67)Tuple(178-187)ResourceType(11-11)go/internal/services/keys/options.go (1)
WithPermissions(47-52)
go/internal/services/keys/validation.go (2)
go/internal/services/keys/verifier.go (1)
KeyVerifier(32-61)go/pkg/rbac/permissions.go (2)
ResourceType(11-11)ActionType(15-15)
go/apps/api/routes/v2_keys_verify_key/403_test.go (5)
go/pkg/testutil/http.go (2)
NewHarness(59-209)CallRoute(405-439)go/apps/api/routes/v2_keys_verify_key/handler.go (3)
Handler(32-38)Request(25-25)Response(26-26)go/pkg/testutil/seed/seed.go (1)
CreateApiRequest(84-92)go/pkg/rbac/permissions.go (1)
CreateKey(49-49)go/apps/api/openapi/gen.go (4)
ForbiddenErrorResponse(138-144)Meta(279-282)NOTFOUND(40-40)VALID(43-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test API / API Test Local
- GitHub Check: Lint Go Code / Lint
- GitHub Check: Test Go API Local / Test
- GitHub Check: Build / Build
🔇 Additional comments (1)
go/apps/api/routes/v2_keys_verify_key/handler.go (1)
124-149: LGTM! The later permission check correctly uses the actual API ID.This check correctly uses
key.Key.ApiIDfrom the looked-up key, properly validating permissions for the specific API. The 200 status with NOT_FOUND code (lines 137-148) is the correct approach to prevent leaking key existence when the root key lacks permissions for this specific API, even if it has permissions for other APIs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
go/apps/api/routes/v2_keys_verify_key/403_test.go (1)
15-15: Consider a more descriptive test function name.The test function name
TestForbidden_NoVerifyPermissionsdescribes only the first subtest's scenario, but the function contains 4 subtests covering broader permission validation (forbidden, NOT_FOUND for wrong API, and successful cases). A name likeTestVerifyKey_RootKeyPermissionswould better reflect the comprehensive permission scenarios being tested.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
go/apps/api/routes/v2_keys_verify_key/403_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/runbook-freshness-check.yaml:157-173
Timestamp: 2025-08-08T14:59:52.283Z
Learning: Repo unkeyed/unkey: When a CI/workflow fix is deferred, imeyer prefers a thorough GitHub issue be opened with sections (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References) and assigned to imeyer, including backlinks to the originating PR and comment.
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/actions/setup-node/action.yaml:0-0
Timestamp: 2025-08-08T15:10:46.436Z
Learning: Repo: unkeyed/unkey — Preference: If imeyer comments “issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/job_detect_changes.yaml:111-114
Timestamp: 2025-08-08T16:10:00.224Z
Learning: Repo: unkeyed/unkey — When imeyer writes “make me an issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Learnt from: perkinsjr
Repo: unkeyed/unkey PR: 3880
File: apps/dashboard/app/(app)/settings/root-keys/components/root-key/root-key-dialog.tsx:137-141
Timestamp: 2025-08-29T12:08:57.482Z
Learning: Root keys in the Unkey system require at least one permission at minimum, so there is no valid case where a root key would have zero permissions. This means checking for permission existence when determining UI labels is unnecessary.
Learnt from: chronark
Repo: unkeyed/unkey PR: 3617
File: go/apps/api/openapi/openapi.yaml:3309-3312
Timestamp: 2025-07-16T17:51:57.297Z
Learning: In the Unkey API OpenAPI schema, the permissions query regex for the verifyKey endpoint intentionally allows all whitespace characters (including tabs and newlines) via `\s`. Do not flag this as an error in future reviews.
📚 Learning: 2025-08-14T16:25:48.167Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3785
File: go/apps/api/routes/v2_keys_reroll_key/401_test.go:52-61
Timestamp: 2025-08-14T16:25:48.167Z
Learning: User Flo4604 requested creation of a GitHub issue to track converting all test files to use table-driven test patterns as a broader codebase improvement, following the suggestion made during review of go/apps/api/routes/v2_keys_reroll_key/401_test.go.
Applied to files:
go/apps/api/routes/v2_keys_verify_key/403_test.go
📚 Learning: 2025-09-15T19:53:28.487Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 3952
File: go/proto/ctrl/v1/routing.proto:0-0
Timestamp: 2025-09-15T19:53:28.487Z
Learning: In the Unkey codebase, authentication/authorization errors intentionally return NOT_FOUND error codes instead of distinct auth error codes (like FORBIDDEN or UNAUTHORIZED) for security reasons. This prevents attackers from distinguishing between "resource doesn't exist" and "you don't have permission to access this resource", avoiding information disclosure about workspace existence and access boundaries.
Applied to files:
go/apps/api/routes/v2_keys_verify_key/403_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test API / API Test Local
🔇 Additional comments (5)
go/apps/api/routes/v2_keys_verify_key/403_test.go (5)
15-33: LGTM! Well-structured test setup.The test harness is properly initialized with all required handler dependencies, and the base fixtures (workspace, API, key) are correctly created for use across subtests.
35-57: Excellent permission validation and error detail checking.The test correctly validates the 403 response when the root key lacks verify permissions, and the assertions on lines 55-56 ensure the error message helpfully mentions both permission options (
api.*.verify_keyandapi.<API_ID>.verify_key). The RequestId assertion on line 52 is good practice for validating error response metadata.
59-82: Excellent security-conscious test design.This test correctly validates that when a root key has verify permission for a different API, the endpoint returns 200 with
NOT_FOUND(not 403) to avoid leaking key existence—a critical security pattern. The comment on line 76 clearly documents this behavior. Based on learnings, this aligns with the intentional design to prevent information disclosure about workspace boundaries.
84-102: LGTM! Wildcard permission validation is correct.The test properly validates that a root key with wildcard
api.*.verify_keypermission successfully verifies keys across all APIs, returning 200 withVALIDcode.
104-122: LGTM! Specific API permission validation is correct.The test properly validates that a root key with a specific API verify permission (
api.<API_ID>.verify_key) successfully verifies keys for that API, returning 200 withVALIDcode.
What does this PR do?
When someone forgets to set any verify key permissions we now throw a 403 error.
Fixes #4482
Type of change
How should this be tested?
Tests
Create root key without permissions, verify a key -> no matter if the key exists or doesn't we should respond with a 403 error.
Checklist
Required
pnpm buildpnpm fmtmake fmton/godirectoryconsole.logsgit pull origin mainAppreciated