fix: switchout openapi validator #4808
Draft
Conversation
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
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
📝 WalkthroughWalkthroughThis pull request replaces the OpenAPI validation system with a custom implementation based on jsonschema. It introduces a modular validation pipeline (path matching, security, parameters, body), removes dependencies on pb33f/libopenapi, and updates OpenAPI specs to use RFC 7807 problem+json error responses with improved error details. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware
participant PathMatcher
participant SecurityValidator
participant Validator
participant BodyValidator
participant SchemaValidator
participant Response
Client->>Middleware: HTTP Request
Middleware->>PathMatcher: Match(method, path)
PathMatcher-->>Middleware: Operation matched or nil
alt Operation found
Middleware->>SecurityValidator: ValidateSecurity(requirements)
SecurityValidator-->>Middleware: SecurityError or nil
alt Security valid
Middleware->>Validator: validateContentType()
Validator-->>Middleware: BadRequestError or nil
alt Content-Type valid
Middleware->>Validator: validatePathParams()
Validator-->>Middleware: ValidationErrors[]
Middleware->>Validator: validateQueryParams()
Validator-->>Middleware: ValidationErrors[]
Middleware->>Validator: validateHeaderParams()
Validator-->>Middleware: ValidationErrors[]
Middleware->>Validator: validateCookieParams()
Validator-->>Middleware: ValidationErrors[]
alt Parameters valid
Middleware->>BodyValidator: validateBody(context)
BodyValidator->>SchemaValidator: Validate(instance)
SchemaValidator-->>BodyValidator: ValidationError or nil
BodyValidator-->>Middleware: ValidationErrorResponse, ok
alt Body valid
Middleware->>Response: Pass through to handler
else Body invalid
Middleware->>Response: Return BadRequestError
end
else Parameters invalid
Middleware->>Response: Return BadRequestError with details
end
else Content-Type invalid
Middleware->>Response: Return BadRequestError
end
else Security invalid
Middleware->>Response: Return SecurityError/UnauthorizedError
end
else No operation match
Middleware->>Response: Pass through to handler
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
svc/api/routes/v2_keys_add_roles/400_test.go (1)
157-162: Fix test data: not actually empty-string role.This case duplicates “empty roles array”. Use
""element.🐛 Suggested fix
- req := handler.Request{ - KeyId: validKeyID, - Roles: []string{}, - } + req := handler.Request{ + KeyId: validKeyID, + Roles: []string{""}, + }svc/api/routes/v2_ratelimit_delete_override/400_test.go (4)
41-46: Duplicate assertion onError.Title.Lines 42 and 44 assert the same condition. Remove one.
Proposed fix
require.Equal(t, "https://unkey.com/docs/errors/unkey/application/invalid_input", res.Body.Error.Type) require.Equal(t, "Bad Request", res.Body.Error.Title) require.Equal(t, http.StatusBadRequest, res.Body.Error.Status) - require.Equal(t, "Bad Request", res.Body.Error.Title) require.NotEmpty(t, res.Body.Meta.RequestId)
59-64: Duplicate assertion onError.Title.Same issue - lines 60 and 62 are identical.
Proposed fix
require.Equal(t, "https://unkey.com/docs/errors/unkey/application/invalid_input", res.Body.Error.Type) require.Equal(t, "Bad Request", res.Body.Error.Title) require.Equal(t, http.StatusBadRequest, res.Body.Error.Status) - require.Equal(t, "Bad Request", res.Body.Error.Title) require.NotEmpty(t, res.Body.Meta.RequestId)
78-83: Duplicate assertion onError.Title.Lines 79 and 81 are identical.
Proposed fix
require.Equal(t, "https://unkey.com/docs/errors/unkey/application/invalid_input", res.Body.Error.Type) require.Equal(t, "Bad Request", res.Body.Error.Title) require.Equal(t, http.StatusBadRequest, res.Body.Error.Status) - require.Equal(t, "Bad Request", res.Body.Error.Title) require.NotEmpty(t, res.Body.Meta.RequestId)
97-102: Duplicate assertion onError.Title.Lines 98 and 100 are identical.
Proposed fix
require.Equal(t, "https://unkey.com/docs/errors/unkey/application/invalid_input", res.Body.Error.Type) require.Equal(t, "Bad Request", res.Body.Error.Title) require.Equal(t, http.StatusBadRequest, res.Body.Error.Status) - require.Equal(t, "Bad Request", res.Body.Error.Title) require.NotEmpty(t, res.Body.Meta.RequestId)svc/api/routes/v2_apis_get_api/400_test.go (1)
71-91: Test case has inconsistent comment, assertion, and name.Comment states "This will return 404" but assertion expects 400. Test name "valid request" is misleading since it tests a non-existent API. Either:
- Rename test and fix comment to reflect 400 expectation, or
- Fix assertion to 404 and use
NotFoundErrorResponseSuggested fix (if 404 is intended)
- // Test with a valid apiId - t.Run("valid request", func(t *testing.T) { + // Test with a non-existent apiId + t.Run("api not found", func(t *testing.T) { // Create a test API in the database apiID := "api_valid_test_id" req := handler.Request{ ApiId: apiID, } - // This will return 404 since the API doesn't exist res := testutil.CallRoute[handler.Request, openapi.NotFoundErrorResponse]( h, route, validHeaders, req, ) - require.Equal(t, http.StatusBadRequest, res.Status) + require.Equal(t, http.StatusNotFound, res.Status) require.NotEmpty(t, res.Body.Meta.RequestId) require.NotEmpty(t, res.Body.Error) })svc/api/routes/v2_ratelimit_set_override/400_test.go (1)
188-188: Fix comment/assertion mismatch.Comment says "expected 401" but assertion expects
StatusBadRequest(400).Suggested fix
- require.Equal(t, http.StatusBadRequest, res.Status, "expected 401, sent: %+v, received: %s", req, res.RawBody) + require.Equal(t, http.StatusBadRequest, res.Status, "expected 400, sent: %+v, received: %s", req, res.RawBody)svc/api/routes/v2_keys_remove_permissions/401_test.go (1)
116-132: Test assertion will fail for empty bearer token case.The test at line 131 asserts
Contains(res.Body.Error.Detail, "Bearer"), but the error inauth.go(lines 47-48) for empty bearer token is created without aPublic()message:fault.New("invalid token", ...). This causesUserFacingMessage()to return an empty string, making the assertion fail.The malformed header case (lines 40-42) includes
Public("You must provide a valid root key...")which contains "Bearer", so that test passes. Update the empty bearer token error to include aPublic()message with "Bearer", or change the test assertion to match the actual error response.
🤖 Fix all issues with AI agents
In `@pkg/zen/validation/benchmark_comparison_test.go`:
- Around line 5-9: Add the standard "net/http" import to the import block in
pkg/zen/validation/benchmark_comparison_test.go and replace all literal "POST"
usages with http.MethodPost (these occur in the HTTP request constructions
around the test helpers and assertions referenced in this file, e.g., where
requests are created at the places noted in the review: lines near 59, 86, 106,
127, 146, 167, 198, 222). Ensure the import is used so the file compiles and
update each request creation to use http.MethodPost instead of the "POST" string
literal.
In `@pkg/zen/validation/benchmark_test.go`:
- Around line 23-41: The benchmark/test code uses HTTP method string literals in
httptest.NewRequest calls (e.g., in BenchmarkValidateSingleRequest) which
trippers the usestdlibvars linter; update all occurrences (including the other
test functions noted) to use the standard library constants like http.MethodPost
or http.MethodGet instead of raw strings when calling httptest.NewRequest in
these tests (search for httptest.NewRequest in this file and replace the first
argument with the appropriate http.Method* constant), leaving the rest of the
request construction (headers, body, and calls to v.Validate) unchanged.
In `@pkg/zen/validation/body_parsers.go`:
- Around line 111-118: The current stub parseMultipartBytes on type
MultipartFormParser always returns an empty map which causes multipart bodies to
silently pass validation; update parseMultipartBytes(body []byte, schema
map[string]any) (any, error) to detect that multipart parsing is unsupported
(e.g., missing boundary or unimplemented) and return a clear error instead of an
empty map, or implement actual parsing using the multipart package to honor the
Content-Type boundary and extract form fields into a map; reference the
parseMultipartBytes method on MultipartFormParser and ensure callers handle the
returned error.
In `@pkg/zen/validation/compiler.go`:
- Around line 223-262: The fast-path in compileOperationSchema should only
trigger when RequestSchema is exactly a lone $ref; change the initial check to
assert op.RequestSchema is a map and that it contains a string "$ref" and no
other keys (e.g., len(map)==1) before constructing resourceURL and returning
compiler.Compile; otherwise fall through to the existing transformRefs +
cleanForJSONSchema flow so any sibling constraints (description, type, etc.) are
preserved. Reference: compileOperationSchema, op.RequestSchema, transformRefs,
cleanForJSONSchema.
In `@pkg/zen/validation/errors.go`:
- Around line 134-146: buildLocationWithKeyword currently only appends fieldName
for required/additionalProperties when instanceLocation is root, causing nested
errors to return locations like "body[0]" instead of "body[0].name"; update
buildLocationWithKeyword to also check when keyword is "required" or
"additionalProperties" and fieldName != "" for non-root instanceLocation and
append the fieldName to the formatted location (i.e., call
FormatLocation(prefix, instanceLocation) and then add "."+fieldName) so nested
required/additionalProperties errors include the missing field name.
In `@pkg/zen/validation/parameters.go`:
- Around line 20-28: Parameter.Schema currently uses map[string]any and
coerceValue returns any; refactor both boundaries to remove any by (1) changing
Parameter.Schema to hold raw JSON (e.g., json.RawMessage) and a typed wrapper
(e.g., a Schema struct) that you parse once at input time so downstream code in
styles.go and body_parsers.go consumes a typed Schema instead of map[string]any,
and (2) replacing coerceValue's any return with a small discriminated result
type (e.g., CoercedValue { Kind TypeKind; Int int64; Float float64; Bool bool;
Str string } or an interface with typed accessors) so callers handle specific
kinds explicitly; update all usages (notably in styles.go and body_parsers.go)
to switch on the discriminant (TypeKind) instead of type-asserting any.
In `@pkg/zen/validation/security_test.go`:
- Around line 54-69: The test uses non-canonical header casing "X-API-Key" which
triggers the canonicalheader linter; update all occurrences to the canonical
form "X-Api-Key" where the header name is set or compared (e.g., in
TestSecurityScheme_APIKey_Header and the other test blocks referenced around
lines 114-149 and 151-190), including the SecurityScheme{Name: "..."} and any
req.Header.Set/req.Header.Get calls so the header name matches Go's
textproto.CanonicalMIMEHeaderKey capitalization.
In `@pkg/zen/validation/styles.go`:
- Around line 111-153: The code repeatedly uses the string literals "array" and
"object" (e.g., in parseSimpleStyle, parseFormStyle, parseDeepObjectStyle,
parseLabelStyle, etc.); extract them into package-level constants (e.g., const
schemaTypeArray = "array" and const schemaTypeObject = "object") and replace all
occurrences of the literals with those constants so parsing functions
(parseSimpleStyle, parseFormStyle, parseDeepObjectStyle, parseLabelStyle and any
other parse* functions) reuse the same symbols; ensure the new const names are
descriptive and update any coerceValue checks to use the constants.
- Around line 64-109: parseFormStyle currently ignores multiple key=value pairs
when parsing exploded objects and always coerces array/object items to "string";
update the parsing pipeline: change ParseByStyle to accept itemType and property
type info (pass through to parseFormStyle), then modify parseFormStyle (and its
calls) so that for schemaType=="array" you use the provided itemType to coerce
each element (not hardcoded "string"), and for schemaType=="object" with
explode==true iterate over all values (e.g., values like "role=admin" and
"firstName=Alex"), split each into key/value, trim and coerce the value using
the corresponding property type info (or a generic propertyType when provided)
via coerceValue, and for explode==false use the provided property/item types
when coercing; keep using coerceValue for primitive handling. Ensure you update
all callers of ParseByStyle/parseFormStyle to supply the new item/property type
args.
In `@pkg/zen/validation/validate_body.go`:
- Line 94: The validateBody function has an unused parameter op *Operation;
remove this parameter from the function signature (func (v *Validator)
validateBody(ctx context.Context, r *http.Request, compiledOp
*CompiledOperation) ...) and update all call sites (e.g., where validateBody is
invoked from validator.go) to stop passing the Operation argument so the call
matches the new signature; ensure imports/types still resolve and run
tests/static analysis to confirm the unparam warning is resolved.
In `@pkg/zen/validation/validate_params.go`:
- Around line 41-85: The validator currently uses query[param.Name] which is
empty for style "deepObject", causing required deepObject params to be treated
as missing and skipping validation; update validateQueryParams to special-case
when param.Style == "deepObject": determine presence by scanning the full query
map for keys that start with param.Name+"[", treat that as existence for
required checks, skip the empty-value check, and call ParseByStyle with the full
query (not only query[param.Name]) so param.Schema.Validate receives the parsed
deep object; adjust the error Location to still use "query."+param.Name and
reuse transformParamError for schema errors.
In `@pkg/zen/validation/validator.go`:
- Around line 142-156: The validateSecurity function incorrectly short-circuits
to ValidateBearerAuth when any bearer scheme is present, skipping multi-scheme
AND/OR semantics; update validateSecurity to either always call
ValidateSecurity(r, requirements, v.securitySchemes, requestID) (preferred) or,
if keeping the bearer shortcut, only call ValidateBearerAuth when every
SecurityRequirement contains exactly one scheme and that scheme is bearer (use
requiresBearerAuth to detect bearer but additionally inspect each
SecurityRequirement's schemes for sole-bearer-only), otherwise fall back to
ValidateSecurity so combined requirements are validated against
v.securitySchemes.
- Around line 135-139: The call to v.validateBody in validator.go is passing an
unused op parameter; update the call to only pass the required parameters to
match the new signature (remove the op argument from the call in the block that
starts the "validation.ValidateBody" span). Then update the validateBody
function signature in validate_body.go (and any other references) to remove the
op parameter so callers and implementation align with the new signature (ensure
v.validateBody(ctx, r, compiledOp) or the correct reduced param list is used
everywhere).
In `@svc/api/openapi/generate_bundle.go`:
- Around line 88-106: The resolver replaces a map containing a "$ref" with the
referenced file content and discards sibling keys (in resolveRefs when handling
case map[string]any), which breaks OpenAPI 3.1 where sibling keys (e.g.,
description) must be preserved; update resolveRefs to detect a "$ref" string in
v, call resolveFileRef(ref, baseDir, visited) to get the referenced content,
then if the resolved content is a map merge the original map's sibling keys into
that resolved map (without overriding keys present in the resolved content
unless explicitly intended), and return the merged map; otherwise (non-map
resolved content) behave as before. Ensure you reference/modify resolveRefs, use
the existing resolveFileRef, and preserve baseDir/visited handling.
In `@svc/api/routes/v2_identities_delete_identity/401_test.go`:
- Around line 38-40: Replace the loose require.NotEmpty checks on
res.Body.Error.Type with a strict assertion that the error type matches one of
the expected categories (e.g., "authentication/missing" or "invalid_input")
while keeping the Detail check; in the test file
v2_identities_delete_identity/401_test.go update the assertion around
res.Body.Error.Type to verify it equals one of the allowed values (or is
contained in an allowed set) so unrelated error types cannot pass, and keep
require.NotEmpty for res.Body.Error.Detail.
In `@svc/api/routes/v2_identities_get_identity/400_test.go`:
- Around line 37-42: The test contains a duplicated assertion for
res.Body.Error.Title (require.Equal(t, "Bad Request", res.Body.Error.Title)
appears twice); remove the redundant require.Equal line so the test only asserts
the title once while keeping the other assertions (Errors length,
Errors[0].Location, Status, Meta.RequestId) intact.
- Around line 54-57: Remove the duplicate assertion that checks
res.Body.Error.Title in the test: there are two identical require.Equal(t, "Bad
Request", res.Body.Error.Title) calls—delete the redundant one (the second
occurrence) so the test keeps a single assertion for res.Body.Error.Title while
retaining the other checks (require.Equal for res.Body.Error.Status and
require.NotEmpty for res.Body.Meta.RequestId).
In `@svc/api/routes/v2_identities_update_identity/401_test.go`:
- Around line 42-44: Replace the loose assertions on the error payload with
exact equality checks: instead of require.NotEmpty(t, res.Body.Error.Type) and
require.NotEmpty(t, res.Body.Error.Detail), assert require.Equal(t,
<expectedErrorType>, res.Body.Error.Type) using the same expected error type
string or constant used in the "malformed Authorization header" test, and assert
the exact expected error detail (or use require.Contains if only part of the
detail is stable); update the test to reference the existing expected error type
constant/string so this case fails on regressions if the validator returns a
different error.
In `@svc/api/routes/v2_ratelimit_list_overrides/400_test.go`:
- Around line 41-44: In svc/api/routes/v2_ratelimit_list_overrides/400_test.go
remove the duplicated assertion that checks res.Body.Error.Title equals "Bad
Request" (the duplicate of require.Equal(t, "Bad Request",
res.Body.Error.Title)), leaving one assertion for Title plus the existing
assertions for res.Body.Error.Status and res.Body.Meta.RequestId so the test
still verifies the error title, status, and request ID.
- Around line 58-61: Duplicate assertion: remove the redundant check for
res.Body.Error.Title so it is only asserted once; in the test in
svc/api/routes/v2_ratelimit_list_overrides/400_test.go (the block with
require.Equal(t, "Bad Request", res.Body.Error.Title), require.Equal(t,
http.StatusBadRequest, res.Body.Error.Status), require.NotEmpty(t,
res.Body.Meta.RequestId)) delete the repeated require.Equal(t, "Bad Request",
res.Body.Error.Title) line and keep the single Title assertion alongside the
Status and RequestId assertions.
🧹 Nitpick comments (22)
svc/api/routes/v2_permissions_get_permission/400_test.go (1)
50-50: Consider asserting additional error fields for stronger validation coverage.The change to assert
Title == "Bad Request"is correct for the new error format, but it's less specific than the previousDetailcheck. Since the new error response includesfixandlocationfields (per PR objectives), asserting on those would ensure the validation errors contain actionable details.Current tests verify:
- Status 400 ✓
- Error exists ✓
- Title = "Bad Request" ✓
Optional enhancement would be to also verify
res.Body.Error.Locationor similar fields are populated for validation errors.Also applies to: 66-66, 85-85
svc/api/routes/v2_ratelimit_get_override/400_test.go (1)
43-45: Duplicate assertion forError.Title.Lines 43 and 45 both assert
Error.Title == "Bad Request". Same pattern repeats in other test cases (lines 61/63, 80/82, 99/101). Remove one of each pair.Proposed fix
require.Equal(t, "https://unkey.com/docs/errors/unkey/application/invalid_input", res.Body.Error.Type) require.Equal(t, "Bad Request", res.Body.Error.Title) require.Equal(t, http.StatusBadRequest, res.Body.Error.Status) - require.Equal(t, "Bad Request", res.Body.Error.Title) require.NotEmpty(t, res.Body.Meta.RequestId) require.Greater(t, len(res.Body.Error.Errors), 0)Apply the same removal for lines 63, 82, and 101.
svc/api/routes/v2_identities_delete_identity/400_test.go (2)
39-42: Drop duplicate Title assert
Redundant with the Title check below.
53-56: Drop duplicate Title assert
Redundant with the Title check below.svc/api/routes/v2_identities_update_identity/400_test.go (2)
44-47: Remove duplicate assertion.Line 44 and 46 both assert the same condition.
Suggested fix
require.Equal(t, "https://unkey.com/docs/errors/unkey/application/invalid_input", res.Body.Error.Type) require.Equal(t, "Bad Request", res.Body.Error.Title) require.Equal(t, 400, res.Body.Error.Status) - require.Equal(t, "Bad Request", res.Body.Error.Title) require.NotEmpty(t, res.Body.Meta.RequestId)
64-67: Remove duplicate assertion.Line 64 and 66 both assert the same condition.
Suggested fix
require.Equal(t, "https://unkey.com/docs/errors/unkey/application/invalid_input", res.Body.Error.Type) require.Equal(t, "Bad Request", res.Body.Error.Title) require.Equal(t, 400, res.Body.Error.Status) - require.Equal(t, "Bad Request", res.Body.Error.Title) require.NotEmpty(t, res.Body.Meta.RequestId)svc/api/routes/v2_ratelimit_set_override/400_test.go (1)
42-45: Remove duplicate Title assertions throughout this file.Each test case has duplicate
require.Equal(t, "Bad Request", res.Body.Error.Title)assertions (e.g., lines 42+44, 62+64, 83+85, etc.). Remove the redundant ones.Suggested fix (example for first occurrence)
require.Equal(t, "https://unkey.com/docs/errors/unkey/application/invalid_input", res.Body.Error.Type) require.Equal(t, "Bad Request", res.Body.Error.Title) require.Equal(t, http.StatusBadRequest, res.Body.Error.Status) - require.Equal(t, "Bad Request", res.Body.Error.Title) require.NotEmpty(t, res.Body.Meta.RequestId)Apply the same pattern to all other test cases in this file.
svc/api/routes/v2_ratelimit_multi_limit/400_test.go (1)
88-92: Drop duplicate Title assertions.Title checked twice in each block; keep one.
♻️ Proposed minimal cleanup
- require.Equal(t, "Bad Request", res.Body.Error.Title) require.NotEmpty(t, res.Body.Meta.RequestId) @@ - require.Equal(t, "Bad Request", res.Body.Error.Title) require.NotEmpty(t, res.Body.Meta.RequestId)Also applies to: 120-124
svc/api/routes/v2_ratelimit_limit/400_test.go (2)
66-70: Drop duplicate Title assertions. One check is enough.♻️ Proposed minimal cleanup
- require.Equal(t, "Bad Request", res.Body.Error.Title) require.NotEmpty(t, res.Body.Meta.RequestId) @@ - require.Equal(t, "Bad Request", res.Body.Error.Title) require.NotEmpty(t, res.Body.Meta.RequestId)Also applies to: 95-99
140-156: Remove or strengthen this subtest.Unmarshal into
handler.Responseignores unknown fields, so this passes even if error shape regresses. Prefer removing or asserting the error body.♻️ Minimal fix: drop the no-op subtest
- t.Run("missing authorization header response structure", func(t *testing.T) { - headers := http.Header{ - "Content-Type": {"application/json"}, - // No Authorization header - } - - req := handler.Request{ - Namespace: "test_namespace", - Identifier: "user_123", - Limit: 100, - Duration: 60000, - } - - res := testutil.CallRoute[handler.Request, handler.Response](h, route, headers, req) - require.Equal(t, http.StatusBadRequest, res.Status, "Got %s", res.RawBody) - require.NotNil(t, res.Body) - })svc/api/routes/v2_apis_create_api/400_test.go (1)
82-87: Drop duplicate Title assert; keep single correct order.♻️ Minimal cleanup
- require.Equal(t, res.Body.Error.Title, "Bad Request") - require.Equal(t, "Bad Request", res.Body.Error.Title) + require.Equal(t, "Bad Request", res.Body.Error.Title)svc/api/routes/v2_identities_create_identity/400_test.go (1)
41-45: Remove duplicate Title asserts (keep one).♻️ Example (apply same pattern to other blocks)
require.Equal(t, "https://unkey.com/docs/errors/unkey/application/invalid_input", res.Body.Error.Type) require.Equal(t, "Bad Request", res.Body.Error.Title) require.Equal(t, http.StatusBadRequest, res.Body.Error.Status) - require.Equal(t, "Bad Request", res.Body.Error.Title)Also applies to: 55-59, 75-79, 98-102, 123-127, 147-151, 171-175
pkg/zen/validation/security_test.go (1)
11-35: Uset.Runsubtests for multi-case scenarios.
Keeps failures isolated per case. As per coding guidelines.Also applies to: 37-52, 54-69, 71-85, 87-102, 104-112, 114-149, 151-190
pkg/zen/validation/styles.go (1)
26-61: Avoidanyin parse surface.
Consider a typed ParsedValue (kind + value) to keep type safety. As per coding guidelines.pkg/zen/validation/validate_body.go (2)
73-91: HTTP status 415 returned inBadRequestErrorResponsewrapper.Line 80 sets
Status: http.StatusUnsupportedMediaType(415), but the return type isBadRequestErrorResponsewhich semantically represents 400 errors. This inconsistency may confuse clients or monitoring systems expecting the response structure to match the status code.Consider either:
- Creating a separate response type for 415 errors, or
- Documenting that
BadRequestErrorResponseis a generic validation error envelope that can carry different status codes.
99-105: Body reading logic can be simplified.The condition at line 99 checks
r.Body == nil || r.ContentLength == 0, then immediately reads the body anyway. IfContentLength == 0, reading will return empty bytes. The nil check onr.Bodybeforeio.ReadAllis redundant sinceio.ReadAllhandles nil readers.Suggested simplification
if compiledOp != nil && compiledOp.BodyRequired { - if r.Body == nil || r.ContentLength == 0 { - // Try to read body to check if it's truly empty - body, err := io.ReadAll(r.Body) - if err == nil { - r.Body = io.NopCloser(bytes.NewReader(body)) - } - if err != nil || len(body) == 0 { + body, err := io.ReadAll(r.Body) + if err != nil { + // Handle read error + return openapi.BadRequestErrorResponse{...}, false + } + r.Body = io.NopCloser(bytes.NewReader(body)) + if len(body) == 0 { return openapi.BadRequestErrorResponse{ // ... missing body error }, false - } } }pkg/zen/validation/matcher.go (2)
92-95: Silent failure on regex compile error may hide spec issues.If
regexp.Compilefails, the function returnsniland the operation is silently dropped. Consider logging or returning an error to surface malformed path templates during initialization.Suggested improvement
compiled, err := regexp.Compile(regexPattern) if err != nil { + // Consider logging: log.Printf("failed to compile path template %s: %v", path, err) return nil }
29-49: Template route ordering is non-deterministic.Since
operationsis a map, iteration order is random. If multiple template routes could match the same path (e.g.,/users/{id}vs/users/{userId}), the match result depends on iteration order. Consider sortingtemplatePathsby specificity or path length after construction.pkg/zen/validation/parser.go (3)
155-159: Simplify return signature - error is always nil.Per static analysis (unparam), this function never returns an error. Simplify the signature or add actual error handling for edge cases.
Suggested fix
-func (p *SpecParser) parseOperation(method, path string, opData any) (*Operation, error) { +func (p *SpecParser) parseOperation(method, path string, opData any) *Operation { opMap, ok := opData.(map[string]any) if !ok { - return nil, nil + return nil }Update the call site in
NewSpecParser:-op, err := parser.parseOperation(method, path, opData) -if err != nil { - return nil, fmt.Errorf("failed to parse operation %s %s: %w", method, path, err) -} +op := parser.parseOperation(method, path, opData)
161-168: Remove wasted assignment.Per static analysis (wastedassign),
operationIDis assigned""then immediately reassigned in the conditional.Suggested fix
- // Extract operationId - operationID := "" - if opID, ok := opMap["operationId"].(string); ok { - operationID = opID - } else { + // Extract operationId + operationID, ok := opMap["operationId"].(string) + if !ok { // Generate operationId from method and path if not present operationID = method + "_" + strings.ReplaceAll(strings.ReplaceAll(path, "/", "_"), ".", "_") }
213-224: Onlyapplication/jsonschema is extracted.Other content types in
RequestContentTypes(e.g.,application/xml,text/plain) are recorded but their schemas are ignored. If the API has non-JSON request bodies with schemas, they won't be validated.Consider documenting this limitation or extending to support other content types.
pkg/zen/validation/body_parsers.go (1)
203-214: Incomplete content-type handling inParseRequestBody.
multipart/form-datareturnsnil, nilwhich callers may interpret as "no body" rather than "parsing not supported". The default case also returnsnil, nilfor JSON, relying on callers to know JSON needs no special parsing.Consider returning a sentinel error or documenting the nil-nil contract explicitly.
Flo4604
force-pushed
the
fix/circular-reference
branch
2 times, most recently
from
37e4899 to
61be4d5
Compare
Flo4604
force-pushed
the
fix/circular-reference
branch
from
45bfdf5 to
8cb4130
Compare
Flo4604
force-pushed
the
fix/circular-reference
branch
2 times, most recently
from
89b9156 to
8a43663
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
2 similar comments
Contributor
✅ Actions performedReview triggered.
|
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
svc/api/routes/v2_apis_get_api/400_test.go (1)
80-88:⚠️ Potential issue | 🟡 MinorStatus code assertion contradicts comment and response type.
Comment says "This will return 404", response type is
NotFoundErrorResponse, but assertion checksStatusBadRequest(400).Proposed fix
// This will return 404 since the API doesn't exist res := testutil.CallRoute[handler.Request, openapi.NotFoundErrorResponse]( h, route, validHeaders, req, ) - require.Equal(t, http.StatusBadRequest, res.Status) + require.Equal(t, http.StatusNotFound, res.Status) require.NotEmpty(t, res.Body.Meta.RequestId) require.NotEmpty(t, res.Body.Error)svc/api/routes/v2_keys_add_roles/400_test.go (1)
157-175:⚠️ Potential issue | 🟡 MinorTest case appears to duplicate "empty roles array" scenario.
The test name is "role with empty string" but the request sends
Roles: []string{}(empty slice), identical to the "empty roles array" test at lines 118-135. Consider either:
- Sending a role with an actual empty string:
Roles: []string{""}- Removing this duplicate test
🔧 Proposed fix to test empty string role
// Test case for role with empty string id t.Run("role with empty string", func(t *testing.T) { req := handler.Request{ KeyId: validKeyID, - Roles: []string{}, + Roles: []string{""}, }
🤖 Fix all issues with AI agents
In `@pkg/zen/validation/body_test.go`:
- Around line 3-11: Replace uses of string(make([]byte, 51)) (which creates NULs
and produces invalid JSON) with a printable repeated string via
strings.Repeat("a", 51) in the test helpers/assertions; update the import block
to include the "strings" package. Apply the same replacement for the other
occurrences (noted around the later tests at lines 470-472) so JSON parsing sees
valid characters and the maxLength validation is exercised correctly.
In `@pkg/zen/validation/validate_body.go`:
- Around line 82-86: The error object in validate_body.go is using
openapi.BadRequestErrorDetails with Title "Bad Request" while Status is
http.StatusUnsupportedMediaType; update the Title to match the 415 status (e.g.,
use http.StatusText(http.StatusUnsupportedMediaType) or the string "Unsupported
Media Type") so Title and Status align—locate the openapi.BadRequestErrorDetails
construction (the Error variable) and replace the Title value accordingly.
- Around line 103-174: The required-body logic in validate_body.go can panic
when io.ReadAll is called on a nil r.Body and can mistakenly allow empty bodies
when ContentLength < 0; modify the check in the compiledOp.BodyRequired branch
and the subsequent body-read logic so you first guard that r.Body != nil before
calling io.ReadAll, always capture and restore the body into r.Body after
reading, then re-check the actual len(body) (not ContentLength) and return the
BadRequestError from the compiledOp.BodyRequired branch if the read body is
empty; also ensure the later generic read (the one that sets schema :=
compiledOp.BodySchema and does io.ReadAll) similarly guards nil r.Body and
treats an empty read as empty (not valid) when BodyRequired was true.
In `@svc/api/openapi/spec/paths/v2/apis/getApi/V2ApisGetApiRequestBody.yaml`:
- Around line 9-12: The OpenAPI schema's pattern in V2ApisGetApiRequestBody (the
"pattern" property) does not enforce the 'api_' prefix described in the
description; update the pattern to require the prefix (e.g., change the regex to
require ^api_ followed by allowed chars) so IDs must begin with "api_", or if
the prefix is not required, update the description text in the same
V2ApisGetApiRequestBody.yaml to remove the "begins with 'api_'" claim; ensure
you modify the "pattern" or "description" fields respectively to keep schema and
documentation consistent.
In `@svc/api/openapi/spec/paths/v2/deploy/getDeployment/index.yaml`:
- Around line 30-58: The OpenAPI error responses currently claim only
application/problem+json but the runtime in pkg/zen/session.go conditionally
returns application/problem+json or application/json based on the Accept header;
update the spec instead of runtime: for each error response in
paths/v2/deploy/getDeployment/index.yaml (the "400","401","403","404","500"
response blocks) add application/json alongside application/problem+json (both
pointing to the same error schema reference) so the spec reflects the content
negotiation behavior implemented in the session.go handler.
In
`@svc/api/openapi/spec/paths/v2/keys/createKey/V2KeysCreateKeyRequestBody.yaml`:
- Around line 90-105: The `pattern` for validating each permission string is
misplaced as a sibling of `items` under the permissions schema; move the
`pattern` property inside the `items` object so it applies to each array element
(i.e., ensure the `permissions` schema keeps type: array, maxItems, items: {
type: string, minLength: 1, maxLength: 100, pattern: "^[a-zA-Z][a-zA-Z0-9._-]*$"
} ), preserving the description, example, and other array-level properties.
In `@svc/api/openapi/spec/paths/v2/keys/getKey/V2KeysGetKeyRequestBody.yaml`:
- Around line 33-34: The example in V2KeysGetKeyRequestBody.yaml uses the wrong
property name: replace the top-level "key" example field with "keyId" so it
matches the schema's required property and will validate under
additionalProperties: false; update the example object under the
V2KeysGetKeyRequestBody example block to use "keyId: sk_1234abcdef5678" (no
other changes needed).
In
`@svc/api/openapi/spec/paths/v2/keys/updateKey/V2KeysUpdateKeyRequestBody.yaml`:
- Around line 79-95: The schema and descriptions for the keys "credits" and
"ratelimits" are inconsistent: update the OpenAPI schema so both fields allow
explicit null (or both disallow null and descriptions changed). Specifically,
make the "credits" reference to UpdateKeyCreditsData.yaml nullable (e.g., use
oneOf/anyOf with {"$ref": "UpdateKeyCreditsData.yaml"} and {"type":"null"} or
add "nullable: true" if your generator supports it), and make "ratelimits"
accept null as well (change its schema to allow type: array OR null via
oneOf/anyOf or "nullable: true"); then keep or adjust the descriptions to match
the final nullability behavior. Ensure the referenced item schema
RatelimitRequest.yaml remains the array item type.
- Around line 104-127: The roles and permissions regexes are too permissive;
update the OpenAPI schema fields 'roles.items.pattern' to
^[a-zA-Z][a-zA-Z0-9_-]*$ and 'permissions.items.pattern' to ^[a-zA-Z0-9_]+$ so
role names require a leading letter and permissions follow the established
contract, leaving existing minLength/maxLength constraints intact; ensure you
change these patterns in V2KeysUpdateKeyRequestBody.yaml for the 'roles' and
'permissions' definitions and run schema validators to confirm no other
endpoints diverge.
In
`@svc/api/openapi/spec/paths/v2/keys/verifyKey/V2KeysVerifyKeyRequestBody.yaml`:
- Around line 3-8: The examples include an undefined property "apiId" which will
fail validation because the schema has additionalProperties: false; either add
"apiId" to the request body schema properties (e.g., add a properties entry
"apiId" with type: string and an appropriate description) so examples match the
schema, or remove "apiId" from the examples list; locate the YAML block
containing examples, the properties section, and the additionalProperties: false
setting to apply the change (update the properties to include apiId or edit the
examples to omit apiId).
In
`@svc/api/openapi/spec/paths/v2/permissions/createRole/V2PermissionsCreateRoleRequestBody.yaml`:
- Around line 3-35: The OpenAPI schema for the role request body currently
allows dots in role names via the pattern on the name property; update the name
property's regex in V2PermissionsCreateRoleRequestBody.yaml (the name property
pattern) to enforce the project's rule by changing it to
"^[a-zA-Z][a-zA-Z0-9_-]*$" and remove or replace any example values that contain
dots (e.g., "support.readonly") so examples conform to the new pattern; ensure
additionalProperties remains false and re-run schema validation.
In `@svc/api/routes/v2_identities_update_identity/401_test.go`:
- Around line 40-42: The test is asserting a 401 for a missing auth header but
CI shows the code currently returns 400; update the auth-check implementation
(the code path exercised by CallRoute for route/handler.Request) to return
http.StatusUnauthorized and construct an openapi.UnauthorizedErrorResponse with
Error.Type "https://unkey.com/docs/errors/unkey/authentication/missing" when the
Authorization header is absent, so the require.Equal(t, http.StatusUnauthorized,
res.Status) assertion passes; alternatively, if you prefer mapping missing
header to 400, change the test expectation and Error.Type accordingly (but keep
the response shape as openapi.UnauthorizedErrorResponse if that type is used).
🧹 Nitpick comments (13)
svc/api/routes/v2_keys_add_permissions/400_test.go (1)
93-93: Title-only check may be too lax. Consider also asserting a stableError.Typeor a smallDetailtoken so unrelated 400s don’t pass.Also applies to: 114-114, 132-132, 151-151, 172-172, 202-202
svc/api/routes/v2_apis_get_api/400_test.go (1)
54-69: Consider adding Title assertion for consistency.The "missing apiId field" test now asserts
Error.Title == "Bad Request", but this test case doesn't. Consider adding the same assertion for consistency.Suggested addition
require.Equal(t, http.StatusBadRequest, res.Status) require.NotEmpty(t, res.Body.Meta.RequestId) require.NotEmpty(t, res.Body.Error) + require.Equal(t, "Bad Request", res.Body.Error.Title) })svc/api/openapi/spec/paths/v2/deploy/createDeployment/index.yaml (1)
46-47: Minor: Inconsistent$refkey quoting.Line 47 uses unquoted
$ref:while all other schema references in this file use quoted"$ref":. Consider quoting for consistency.schema: - $ref: "../../../../error/ForbiddenErrorResponse.yaml" + "$ref": "../../../../error/ForbiddenErrorResponse.yaml"pkg/zen/session.go (1)
450-451: Consider consistency:JSONusesAdd()whileProblemJSONusesSet()for Content-Type.Line 450 uses
Add("Content-Type", ...)while lines 491/493 useSet("Content-Type", ...). Both work, butSetis more semantically correct for Content-Type since there should only be one value. Consider aligningJSONto useSetas well.♻️ Optional: Align JSON method to use Set
- s.ResponseWriter().Header().Add("Content-Type", "application/json") + s.ResponseWriter().Header().Set("Content-Type", "application/json")Also applies to: 491-494
svc/api/openapi/spec/paths/v2/keys/updateCredits/V2KeysUpdateCreditsResponseBody.yaml (1)
1-40: Consider using consistent 2-space indentation.This file uses 4-space indentation for nested elements (e.g., lines 3-4, 6-9, 17-19), while sibling
index.yamlfiles in this PR use 2-space indentation. While technically valid YAML, inconsistent indentation across the OpenAPI spec can affect maintainability.The examples restructuring from named examples to an anonymous list is valid for OpenAPI 3.1.
svc/api/openapi/spec/paths/v2/keys/updateCredits/V2KeysUpdateCreditsRequestBody.yaml (1)
1-54: Consider using consistent 2-space indentation.Similar to the response body schema, this file uses 4-space indentation while the accompanying
index.yamluses 2-space indentation. Consider aligning indentation style across the OpenAPI spec files for consistency.The schema content and examples are semantically correct.
svc/api/openapi/generate_bundle.go (1)
37-41: Avoidanyin Bundler state/API.Use typed nodes/structs (e.g., yaml.Node or dedicated schema structs) to keep type safety.
As per coding guidelines: Never compromise type safety: No
any, no!(non-null assertion), noas Type.pkg/zen/validation/security_test.go (1)
11-198: Use t.Run for multi-scenario tests.Several tests bundle cases in one function; prefer table-driven subtests like TestValidateBearerAuth_Detailed.
As per coding guidelines:
**/*_test.go: Organize tests usingt.Run()for scenarios.pkg/zen/validation/matcher_test.go (1)
9-36: Consider table-driven tests for better organization.Inline assertions work but table-driven with
t.Run()would improve readability and error diagnostics. Based on learnings, there's already a tracked issue for converting test files to table-driven patterns.♻️ Example refactor to table-driven
func TestPathMatcher(t *testing.T) { t.Parallel() ops := map[string]*Operation{ "POST /v2/keys.setRoles": { Method: "POST", Path: "/v2/keys.setRoles", OperationID: "keys.setRoles", Parameters: ParameterSet{}, Security: nil, }, } matcher := NewPathMatcher(ops) + tests := []struct { + name string + method string + path string + wantFound bool + wantOpID string + }{ + {"exact match", "POST", "/v2/keys.setRoles", true, "keys.setRoles"}, + {"lowercase method", "post", "/v2/keys.setRoles", true, "keys.setRoles"}, + {"wrong method", "GET", "/v2/keys.setRoles", false, ""}, + {"unknown path", "POST", "/unknown", false, ""}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + result, found := matcher.Match(tt.method, tt.path) + require.Equal(t, tt.wantFound, found) + if tt.wantFound { + require.Equal(t, tt.wantOpID, result.Operation.OperationID) + } + }) + } - result, found := matcher.Match("POST", "/v2/keys.setRoles") - require.True(t, found) - require.Equal(t, "keys.setRoles", result.Operation.OperationID) - - result, found = matcher.Match("post", "/v2/keys.setRoles") // lowercase - require.True(t, found) - require.Equal(t, "keys.setRoles", result.Operation.OperationID) - - _, found = matcher.Match("GET", "/v2/keys.setRoles") - require.False(t, found) - - _, found = matcher.Match("POST", "/unknown") - require.False(t, found) }pkg/zen/validation/parameters_test.go (1)
13-49: Consider adding invalid path parameter test cases.Current tests only cover valid scenarios. Adding negative cases (e.g., missing required path param, malformed values) would strengthen coverage.
♻️ Example additional test cases
{ name: "empty path parameter", path: "/test/params/", isValid: false, },pkg/zen/validation/test_helpers_test.go (1)
523-538: Remove unusedmethodparameter or add GET test case.All 15 usages of
makeRequestpasshttp.MethodPost. Either remove the parameter, or add a GET request test to justify keeping it.pkg/zen/validation/matcher.go (1)
56-103: Consider edge case: empty path parameter names.If a path contains
{}(empty braces),paramNameat line 73 would be empty string. This is unlikely in practice but could cause confusing behavior.Optional: validate param name is non-empty
paramName := path[i+1 : i+end] + if paramName == "" { + return nil // Invalid empty param name + } paramNames = append(paramNames, paramName)pkg/zen/validation/parser.go (1)
154-227: Consider simplifyingparseOperationsignature.Static analysis notes that the error return is always
nil. Consider removing the error return or adding actual error cases. TheoperationID := ""assignment at line 162 is also overwritten unconditionally by line 163-167.Optional: simplify initialization
- // Extract operationId - operationID := "" - if opID, ok := opMap["operationId"].(string); ok { - operationID = opID - } else { + // Extract operationId or generate fallback + operationID, _ := opMap["operationId"].(string) + if operationID == "" { // Generate operationId from method and path if not present operationID = method + "_" + strings.ReplaceAll(strings.ReplaceAll(path, "/", "_"), ".", "_") }
svc/api/openapi/spec/paths/v2/apis/getApi/V2ApisGetApiRequestBody.yaml
Outdated
Show resolved
Hide resolved
svc/api/openapi/spec/paths/v2/keys/updateKey/V2KeysUpdateKeyRequestBody.yaml
Show resolved
Hide resolved
svc/api/openapi/spec/paths/v2/keys/updateKey/V2KeysUpdateKeyRequestBody.yaml
Outdated
Show resolved
Hide resolved
svc/api/openapi/spec/paths/v2/keys/verifyKey/V2KeysVerifyKeyRequestBody.yaml
Show resolved
Hide resolved
svc/api/openapi/spec/paths/v2/permissions/createRole/V2PermissionsCreateRoleRequestBody.yaml
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
svc/api/openapi/spec/paths/v2/keys/createKey/V2KeysCreateKeyResponseBody.yaml (1)
10-25:⚠️ Potential issue | 🟠 MajorAvoid key-like example values to prevent secret-scanner failures.
The
sk_...examples look like real API keys and are already flagged by secret scanning. Please switch to clearly fake placeholders that won’t match key detectors.🔧 Proposed safe placeholders
- key: sk_1234abcdef567890 + key: sk_example_1 ... - key: sk_5678efgh90123456 + key: sk_example_2 ... - key: sk_9012ijkl34567890 + key: sk_example_3svc/api/routes/v2_apis_get_api/400_test.go (1)
80-91:⚠️ Potential issue | 🟡 MinorTest case has inconsistent expectations.
Comment says "return 404", response type is
NotFoundErrorResponse, but assertion expectsStatusBadRequest(400). Likely the assertion should behttp.StatusNotFoundor the test setup/response type is wrong.Proposed fix if 404 is intended
- require.Equal(t, http.StatusBadRequest, res.Status) + require.Equal(t, http.StatusNotFound, res.Status)
🤖 Fix all issues with AI agents
In `@pkg/zen/validation/security.go`:
- Around line 156-177: The iteration over req.Schemes in buildSecurityError is
nondeterministic because req.Schemes is a map; to stabilize error responses,
collect the keys from req.Schemes, sort them, and iterate the sorted slice
instead of ranging the map directly. Update buildSecurityError to build a
[]string of scheme names for each SecurityRequirement, sort.Strings(...) the
slice, then loop over the sorted names and use schemes[schemeName] as before
(preserving the existing calls to buildBearerAuthError, buildBasicAuthError, and
buildAPIKeyError).
In `@pkg/zen/validation/styles.go`:
- Around line 330-356: parseDeepObject currently uses only values[0] which drops
additional values for multi-valued deepObject properties; update parseDeepObject
to check the resolved propType (via getPropertyType) and when propType ==
SchemaTypeArray build an array CoercedValue from all values (coercing each entry
with coerceValue using the appropriate item type) and return that (e.g.
ArrayValue([...])) otherwise keep the existing single-value behavior; ensure
NilValue/ObjectValue return contract remains unchanged.
In `@pkg/zen/validation/validate_body.go`:
- Around line 103-174: The required-body check using compiledOp.BodyRequired
currently runs before reading the request stream and uses r.ContentLength, which
misses chunked requests; move the compiledOp.BodyRequired validation to after
the body is read by io.ReadAll (the same place where body is reset with r.Body =
io.NopCloser(bytes.NewReader(body))); ensure you still handle r.Body == nil
early (return error if nil and BodyRequired), then read body, check len(body)==0
when BodyRequired and return the same BadRequestError structure; update
references to compiledOp.BodyRequired, r.ContentLength, io.ReadAll, and the
r.Body reset logic so chunked bodies are validated correctly.
In `@pkg/zen/validation/validator.go`:
- Around line 116-123: The current guard always runs content-type validation
because r.Body is usually non-nil (e.g. http.NoBody); update the condition
around the call to v.validateContentType so it only runs for actual bodies
(e.g., keep r.ContentLength > 0 OR r.Body != nil && r.Body != http.NoBody or
equivalent check), ensuring the block that starts the tracing span and calls
validateContentType only executes for requests with a real body; adjust imports
if necessary and keep the validateContentType call and tracing span names
unchanged.
In `@svc/api/openapi/generate_bundle.go`:
- Around line 119-129: The current code blindly replaces spec["components"] or
its "schemas" key with b.schemas, potentially dropping inline schemas; change
the logic to merge instead: if spec["components"] exists, coerce it to
map[string]any and if components["schemas"] exists coerce to map[string]any,
then iterate b.schemas and for each key check for an existing entry—if a
conflict is found return/propagate an error (or choose a deterministic merge
strategy), otherwise insert the new schema into the existing map; if no
components or schemas exist create them and assign as before. Ensure you
reference the variables/components named spec, components and b.schemas and
return a clear error on key conflicts.
In
`@svc/api/openapi/spec/paths/v2/apis/deleteApi/V2ApisDeleteApiRequestBody.yaml`:
- Around line 9-15: The OpenAPI schema for the apiId field currently uses
pattern "^[a-zA-Z0-9_]+$" which contradicts the description requiring an "api_"
prefix; update the pattern to enforce the prefix (e.g. change pattern to
"^api_[A-Za-z0-9_]+$") and ensure the example remains valid (example:
api_1234abcd); alternatively, if you prefer to allow non-prefixed IDs, relax the
description to remove the "begins with 'api_'" statement and keep the existing
pattern — modify the apiId property's pattern or description accordingly.
In `@svc/api/openapi/spec/paths/v2/apis/getApi/V2ApisGetApiRequestBody.yaml`:
- Around line 5-13: The OpenAPI schema for apiId currently allows any
alphanumeric/underscore but the description requires a leading "api_" prefix;
update the apiId.pattern to enforce that prefix and keep length constraints
consistent (e.g. change pattern to require "^api_[A-Za-z0-9_]{4,251}$" so the
"api_" prefix is mandatory and the remaining characters respect the existing
minLength: 8 and maxLength: 255), ensuring apiId's pattern, minLength and
maxLength (the apiId schema) remain aligned.
In
`@svc/api/openapi/spec/paths/v2/keys/createKey/V2KeysCreateKeyRequestBody.yaml`:
- Around line 91-98: The OpenAPI schema places the "pattern" at the array level
so it's ignored; move the "pattern" entry from the array object into the "items"
object so each permission string is validated (i.e., remove "pattern" from the
array-level block that contains type: array / maxItems and add the same pattern
line inside the items block alongside type: string / minLength / maxLength),
keeping maxItems, minLength and maxLength intact.
- Around line 74-99: The roles and permissions JSON Schema patterns are too
permissive; update the roles "pattern" to "^[a-zA-Z][a-zA-Z0-9_-]*$" (must start
with a letter, allow letters, digits, underscore and hyphen only) and update the
permissions "pattern" to "^[a-zA-Z0-9_]+$" (letters, digits and underscores
only) in the V2KeysCreateKeyRequestBody YAML so the validators for the roles and
permissions arrays (roles.items.pattern and permissions.pattern) match the
established strict schema contract.
In `@svc/api/openapi/spec/paths/v2/keys/getKey/V2KeysGetKeyRequestBody.yaml`:
- Around line 33-34: The example YAML uses the field name "key" which doesn't
match the schema's "keyId"; update the example object to replace "key:
sk_1234abcdef5678" with "keyId: sk_1234abcdef5678" (leaving "decrypt: false"
intact) so the example in V2KeysGetKeyRequestBody matches the defined schema.
In
`@svc/api/openapi/spec/paths/v2/keys/updateKey/V2KeysUpdateKeyRequestBody.yaml`:
- Around line 79-96: The schema allows null in prose but not in the YAML: update
the credits and ratelimits schemas to permit nulls; for credits (which currently
uses "$ref": "./UpdateKeyCreditsData.yaml") add nullable: true or wrap the $ref
in oneOf with { type: "null" }, and for ratelimits (type: array) either add
nullable: true or change type to ["array","null"] so the schema matches the
description about omitting vs setting null to remove or enable unlimited
settings; ensure the descriptions remain and adjust wording if you instead
choose to remove nullability.
- Around line 104-128: The UpdateKey request body defines regex patterns for
roles and permissions (roles: ^[a-zA-Z0-9_:\-\.\*]+$, permissions:
^[a-zA-Z0-9_:\-\.\*]+$) that conflict with the CreateKey and AddRoles endpoints
which require leading-letter patterns (e.g. ^[a-zA-Z][a-zA-Z0-9._-]*$); make
these patterns consistent across key endpoints by choosing one canonical
permission/role slug regex and applying it to UpdateKey's "roles" and
"permissions" entries (or update CreateKey/AddRoles to the UpdateKey pattern) so
that CreateKey, AddRoles, AddPermissions and UpdateKey all use the same regex;
reference the "roles" and "permissions" schema entries in
V2KeysUpdateKeyRequestBody.yaml and align their pattern values with the agreed
canonical pattern.
In
`@svc/api/openapi/spec/paths/v2/keys/verifyKey/V2KeysVerifyKeyRequestBody.yaml`:
- Around line 3-8: The examples in the OpenAPI request body include the
unsupported field apiId which violates the schema's additionalProperties: false;
update the examples array so each example object only contains the supported
fields (key required, and any of tags, permissions, credits, ratelimits,
migrationId) — remove apiId entries from both example objects and ensure
remaining fields (e.g., key and permissions) match the handler's accepted
properties.
In `@svc/api/openapi/spec/paths/v2/keys/whoami/V2KeysWhoamiRequestBody.yaml`:
- Around line 11-16: The example API key values in the request body schema (the
"example" field and the "examples" entry containing "key") use a
realistic-looking key and trigger secret scanners; replace them with a clearly
fake placeholder that won't match API-key regexes (e.g., "sk_<REDACTED>" or
"<API_KEY_PLACEHOLDER>") in both the example and examples.key fields to avoid CI
failures and unsafe copy/paste.
In
`@svc/api/openapi/spec/paths/v2/permissions/getPermission/V2PermissionsGetPermissionRequestBody.yaml`:
- Around line 3-15: The request body schema defines the property "permission"
but the example uses "permissionId"; update the example to use the exact
property name "permission" so it matches the schema (change the example entry
under examples from permissionId: perm_1234567890abcdef to permission:
perm_1234567890abcdef) in V2PermissionsGetPermissionRequestBody.yaml to
eliminate the key mismatch.
🧹 Nitpick comments (15)
svc/api/openapi/spec/paths/v2/identities/getIdentity/index.yaml (1)
69-96: Consider adding examples for 400, 401, and 500 responses for consistency.Examples are provided for 403 and 404 but not for 400, 401, and 500. Adding examples for all error responses would improve API documentation consistency and help consumers understand the expected error payloads.
svc/api/openapi/spec/paths/v2/keys/updateCredits/V2KeysUpdateCreditsRequestBody.yaml (1)
2-4: Consider enforcing conditional requirement forvaluefield.The description on lines 21-23 states that
valueis "Required when using 'increment' or 'decrement' operations." However, the schema only markskeyIdandoperationas required, meaning the API will accept increment/decrement requests without avaluefield at the schema level.If handler-level validation catches this, the current approach is fine. Otherwise, you could use JSON Schema's
if/then/elseoroneOfto enforce this constraint at the schema level for clearer API documentation and earlier validation.svc/api/openapi/spec/paths/v2/keys/verifyKey/V2KeysVerifyKeyResponseBody.yaml (1)
6-9: Minor: Inconsistent quoting of$refkey.Line 7 uses
$refwithout quotes while line 9 uses"$ref"with quotes. Both are functionally equivalent, but consistent styling would improve readability.🧹 Suggested fix for consistency
properties: meta: - $ref: "../../../../common/Meta.yaml" + "$ref": "../../../../common/Meta.yaml" data: "$ref": "./V2KeysVerifyKeyResponseData.yaml"pkg/zen/validation/types.go (1)
21-39:SchemaTypeUnknown.String()returns"string"- may mask bugs.Returning
"string"for unknown types makes debugging harder since you can't distinguish between an explicitly string type and an unrecognized/missing type. Consider returning"unknown"or empty string.func (s SchemaType) String() string { switch s { case SchemaTypeUnknown: - return "string" + return "unknown" case SchemaTypeString:Also, the
"string"literal appears 7 times per static analysis. Consider extracting to a constant if you prefer keeping current behavior.pkg/zen/session.go (1)
489-494: Accept header parsing may be too permissive.
strings.Containscould match unintended values (e.g.,application/problem+json-extended). For robust content negotiation, consider checking for exact media type match or using a proper parser. However, for this use case whereapplication/problem+jsonis a specific RFC 7807 type, false positives are unlikely.♻️ Optional: Stricter Accept header matching
accept := s.Request().Header.Get("Accept") -if strings.Contains(accept, "application/problem+json") { +// Simple check for the media type, handling potential parameters +if strings.Contains(accept, "application/problem+json") && + !strings.Contains(accept, "application/problem+json-") { s.ResponseWriter().Header().Set("Content-Type", "application/problem+json") } else {svc/api/internal/middleware/errors.go (2)
137-148: Minor: Status code mismatch in response body.For
RequestTimeout(408), usingBadRequestErrorResponsetype may be semantically confusing since 408 isn't a 400 error. Consider whether a dedicated timeout response type would be cleaner, though this is a minor concern given the error structure matches RFC 7807.
152-163: Status 499 is non-standard.Using 499 (Client Closed Request) is a common Nginx convention but not an official HTTP status. The comment acknowledges this. Consider documenting this in the OpenAPI spec or using 400 with a specific error code for stricter HTTP compliance.
pkg/zen/validation/test_helpers_test.go (1)
523-538: Consider handling nil body correctly.When
bodyis empty,bodyReaderremains nil, buthttptest.NewRequesthandles nil body gracefully. However, settingContent-Type: application/jsonfor requests without a body (like GET) is unnecessary and could be misleading.♻️ Optional: Only set Content-Type when body is present
func makeRequest(method, path, body string, headers map[string]string) *http.Request { - var bodyReader *bytes.Reader + var bodyReader *bytes.Reader if body != "" { bodyReader = bytes.NewReader([]byte(body)) } req := httptest.NewRequest(method, path, bodyReader) - req.Header.Set("Content-Type", "application/json") + if body != "" { + req.Header.Set("Content-Type", "application/json") + } req.Header.Set("Authorization", "Bearer test_token")Note: The static analysis hint about
methodalways being POST is a false positive—parameters_test.gouseshttp.MethodGet.pkg/zen/validation/parameters_test.go (1)
13-49: Consider adding invalid path parameter test cases.Currently only valid paths are tested. Adding invalid cases would improve coverage.
💡 Suggestion: Add invalid path test cases
{ name: "non-matching path", path: "/test/unknown/123", isValid: false, },pkg/zen/validation/tuple_test.go (1)
92-117: Duplicate test cases.Lines 93-96 ("valid - prefix items only") and lines 113-117 ("valid - empty additional items") have identical bodies
["hello", 42]. Consider removing the duplicate or making the test names consistent to indicate they test the same scenario.♻️ Remove duplicate test case
{ - name: "valid - empty additional items", - body: `["hello", 42]`, - wantStatus: http.StatusOK, - },pkg/zen/validation/discriminator_test.go (1)
15-76: Avoid map[string]any fixtures. Prefer typed schema structs/helpers.
As per coding guidelines: Never compromise type safety: Noany, no!(non-null assertion), noas Type.pkg/zen/validation/validate_body.go (1)
176-179: Avoidanyfor decoded body. Prefer typed input if validator supports it.
As per coding guidelines: Never compromise type safety: Noany, no!(non-null assertion), noas Type.svc/api/openapi/generate_bundle.go (1)
37-42: Avoid map[string]any for OpenAPI model. Prefer typed structs at the boundaries.
As per coding guidelines: Never compromise type safety: Noany, no!(non-null assertion), noas Type.pkg/zen/validation/compiler.go (1)
39-46: Consider typed schema resolver instead ofmap[string]any.
Public API leaksany; prefer TypedSchema/JSON raw at boundary.pkg/zen/validation/parser.go (1)
11-31: Publicmap[string]anyleaks; consider typed schema.
Expose a typed schema (e.g., TypedSchema/Raw JSON) to keep type safety.
svc/api/openapi/spec/paths/v2/keys/updateKey/V2KeysUpdateKeyRequestBody.yaml
Show resolved
Hide resolved
svc/api/openapi/spec/paths/v2/keys/updateKey/V2KeysUpdateKeyRequestBody.yaml
Show resolved
Hide resolved
svc/api/openapi/spec/paths/v2/keys/verifyKey/V2KeysVerifyKeyRequestBody.yaml
Show resolved
Hide resolved
...i/openapi/spec/paths/v2/permissions/getPermission/V2PermissionsGetPermissionRequestBody.yaml
Outdated
Show resolved
Hide resolved
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@pkg/zen/validation/parser.go`:
- Around line 158-189: The parseOperation function currently pre-assigns
operationID (operationID := "") then overwrites it and its signature returns an
unused error; remove the wasted assignment and the unused error return: change
the code in parseOperation to declare operationID only once (e.g., var
operationID string or use := when setting from opMap or generating from
method/path) and change the signature from parseOperation(method, path string,
opData any) (*Operation, error) to parseOperation(method, path string, opData
any) *Operation, then update internal returns (replace any "return nil, nil"
with "return nil") and fix all callers to accept the single *Operation return
value. Ensure references to Operation, operationID, and parseOperation are
updated accordingly.
- Around line 11-53: The code uses map[string]any and opData any in public
types; replace these with a concrete, well-named type (for example a RawJSON
alias to json.RawMessage or a dedicated Schema struct) and update fields and
spec types to use that concrete type instead of any: change
Operation.RequestSchema, SpecParser.schemas, openAPISpec.Components.Schemas,
openAPISpec.Paths and openAPISpec.Security to use map[string]RawJSON or a Schema
type, and replace any occurrences of opData with that concrete type; update the
parsing/unmarshal logic to decode into RawJSON/Schema (using json/yaml
Unmarshal) so callers keep type-safety and downstream code can explicitly decode
schema contents.
In `@pkg/zen/validation/security_test.go`:
- Around line 11-208: Tests like TestSecurityScheme_HTTP_Bearer,
TestSecurityScheme_HTTP_Basic, TestSecurityScheme_APIKey_Header/Query/Cookie,
TestSecurityScheme_OAuth2, TestSecurityRequirement_ORLogic/ANDLogic/Empty pack
multiple assertions per function; refactor each distinct scenario (valid,
missing, wrong-scheme, empty token, etc.) into t.Run subtests so failures are
isolated and readable. For each original test function, wrap each case that
calls validateHTTPScheme, validateAPIKeyScheme, validateOAuth2Scheme or
ValidateSecurity in its own t.Run with a descriptive name, keep t.Parallel()
where appropriate, and move shared setup into closures to avoid test-state
leaks.
In `@pkg/zen/validation/security.go`:
- Around line 209-242: In buildHTTPAuthError guard against an empty scheme
before using scheme[:1]: check len(scheme) == 0 at the top of the function and
return a SecurityError (e.g., using newSecurityError with a clear message and
the malformed/missing auth docs URL) so the function does not panic; then only
compute displayScheme and prefix after that check (references:
buildHTTPAuthError, newSecurityError, displayScheme, prefix). Ensure the
returned error for an empty scheme uses the same error URL pattern used for
other malformed authentication inputs.
In
`@svc/api/openapi/spec/paths/v2/keys/addPermissions/V2KeysAddPermissionsRequestBody.yaml`:
- Line 29: The pattern in V2KeysAddPermissionsRequestBody.yaml
("^[a-zA-Z][a-zA-Z0-9._*-]*$") conflicts with the permission slug regex in
Permission.yaml ("^[a-zA-Z0-9_:\-\.\*]+$"); update the request body schema for
V2KeysAddPermissionsRequestBody (the permission slug pattern) to match the
Permission.yaml regex so this endpoint accepts colons and leading digits (or, if
intended, add a clear description in the V2KeysAddPermissionsRequestBody
documentation explaining the intentional subset restriction and why it differs
from Permission.yaml).
In
`@svc/api/openapi/spec/paths/v2/keys/setPermissions/V2KeysSetPermissionsRequestBody.yaml`:
- Line 31: The new regex in V2KeysSetPermissionsRequestBody.yaml enforces a
leading letter and breaks compatibility with existing permission names; revert
it to the established permission pattern used elsewhere (e.g.,
createPermission/addPermissions/removePermissions and Permission.yaml) by
replacing the pattern on the permission field `pattern:
"^[a-zA-Z][a-zA-Z0-9._*-]*$"` with the standard pattern `^[a-zA-Z0-9_]+$` (or,
if the leading-letter restriction is intentional, add explicit documentation and
a migration note for V2KeysSetPermissionsRequestBody to avoid surprising
callers).
In `@svc/api/openapi/spec/paths/v2/keys/setRoles/V2KeysSetRolesRequestBody.yaml`:
- Line 29: The new regex in V2KeysSetRolesRequestBody.yaml (pattern:
"^[a-zA-Z][a-zA-Z0-9._-]*$") will reject existing role names that start with
digits or contain ':' or '*'; revert or adjust the pattern to preserve
compatibility (e.g., restore the prior pattern "^[a-zA-Z0-9_:\\-\\.\\*]+$" or
expand the current pattern to allow leading digits and the ':' and '*'
characters) and ensure it matches the same format used by addRoles/createKey (or
centralize the regex into a shared role schema) so existing roles continue to
validate.
In
`@svc/api/openapi/spec/paths/v2/permissions/getPermission/V2PermissionsGetPermissionRequestBody.yaml`:
- Around line 9-11: The description claims the permission ID must begin with
"perm_" but the pattern in V2PermissionsGetPermissionRequestBody.yaml only
enforces a leading letter; either update the pattern for the permission ID
schema to require the literal "perm_" prefix (i.e., make the regex start with
"perm_") or remove the "perm_" prefix claim from the description so they match;
edit the pattern line under the permission ID schema or the description text
accordingly in V2PermissionsGetPermissionRequestBody.yaml.
Comment on lines
11
to
53
| // Operation represents an OpenAPI operation with its request schema | ||
| type Operation struct { | ||
| Method string | ||
| Path string | ||
| OperationID string | ||
|
|
||
| // Request body schema | ||
| RequestSchema map[string]any | ||
|
|
||
| // RequestBodySchemaName is the name of the request body schema (extracted from $ref) | ||
| // e.g., "V2KeysCreateKeyRequestBody" | ||
| RequestBodySchemaName string | ||
|
|
||
| // RequestBodyRequired indicates if a request body is required | ||
| RequestBodyRequired bool | ||
|
|
||
| // RequestContentTypes are the supported content types for the request body | ||
| RequestContentTypes []string | ||
|
|
||
| // Parameters grouped by location | ||
| Parameters ParameterSet | ||
|
|
||
| // Security requirements (empty = no auth required, multiple = OR logic) | ||
| Security []SecurityRequirement | ||
| } | ||
|
|
||
| // SpecParser parses an OpenAPI specification and extracts operations and schemas | ||
| type SpecParser struct { | ||
| operations map[string]*Operation // "POST /v2/keys.setRoles" -> Operation | ||
| schemas map[string]any // component schemas for $ref resolution | ||
| securitySchemes map[string]SecurityScheme // component security schemes | ||
| globalSecurity []SecurityRequirement // default security requirements | ||
| } | ||
|
|
||
| // openAPISpec represents the structure of an OpenAPI 3.1 document | ||
| type openAPISpec struct { | ||
| Components struct { | ||
| Schemas map[string]any `yaml:"schemas"` | ||
| SecuritySchemes map[string]map[string]any `yaml:"securitySchemes"` | ||
| } `yaml:"components"` | ||
| Paths map[string]map[string]any `yaml:"paths"` | ||
| Security []map[string]any `yaml:"security"` | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid any in new public types.
map[string]any and opData any violate type-safety rule.
As per coding guidelines, “Never compromise type safety: No any, no !, no as Type”.
🤖 Prompt for AI Agents
In `@pkg/zen/validation/parser.go` around lines 11 - 53, The code uses
map[string]any and opData any in public types; replace these with a concrete,
well-named type (for example a RawJSON alias to json.RawMessage or a dedicated
Schema struct) and update fields and spec types to use that concrete type
instead of any: change Operation.RequestSchema, SpecParser.schemas,
openAPISpec.Components.Schemas, openAPISpec.Paths and openAPISpec.Security to
use map[string]RawJSON or a Schema type, and replace any occurrences of opData
with that concrete type; update the parsing/unmarshal logic to decode into
RawJSON/Schema (using json/yaml Unmarshal) so callers keep type-safety and
downstream code can explicitly decode schema contents.
Comment on lines
158
to
189
| // parseOperation extracts operation details from raw YAML data | ||
| func (p *SpecParser) parseOperation(method, path string, opData any) (*Operation, error) { | ||
| opMap, ok := opData.(map[string]any) | ||
| if !ok { | ||
| return nil, nil | ||
| } | ||
|
|
||
| // Extract operationId | ||
| operationID := "" | ||
| if opID, ok := opMap["operationId"].(string); ok { | ||
| operationID = opID | ||
| } else { | ||
| // Generate operationId from method and path if not present | ||
| operationID = method + "_" + strings.ReplaceAll(strings.ReplaceAll(path, "/", "_"), ".", "_") | ||
| } | ||
|
|
||
| op := &Operation{ | ||
| Method: method, | ||
| Path: path, | ||
| OperationID: operationID, | ||
| RequestSchema: nil, | ||
| RequestBodySchemaName: "", | ||
| RequestBodyRequired: false, | ||
| RequestContentTypes: nil, | ||
| Parameters: ParameterSet{ | ||
| Query: nil, | ||
| Header: nil, | ||
| Cookie: nil, | ||
| Path: nil, | ||
| }, | ||
| Security: nil, | ||
| } |
Contributor
There was a problem hiding this comment.
Fix lint: unparam + wastedassign.
operationID := "" is overwritten, and parseOperation never returns error.
🔧 Minimal fix
- op, err := parser.parseOperation(method, path, opData)
- if err != nil {
- return nil, fmt.Errorf("failed to parse operation %s %s: %w", method, path, err)
- }
+ op := parser.parseOperation(method, path, opData)
if op != nil {
key := method + " " + path
parser.operations[key] = op
}-func (p *SpecParser) parseOperation(method, path string, opData any) (*Operation, error) {
+func (p *SpecParser) parseOperation(method, path string, opData any) *Operation {
opMap, ok := opData.(map[string]any)
if !ok {
- return nil, nil
+ return nil
}
// Extract operationId
- operationID := ""
+ var operationID string
if opID, ok := opMap["operationId"].(string); ok {
operationID = opID
} else {
// Generate operationId from method and path if not present
operationID = method + "_" + strings.ReplaceAll(strings.ReplaceAll(path, "/", "_"), ".", "_")
}
@@
- return op, nil // No request body
+ return op // No request body
@@
- return op, nil
+ return op
@@
- return op, nil
+ return op
@@
- return op, nil
+ return op
}🧰 Tools
🪛 golangci-lint (2.5.0)
[error] 159-159: (*SpecParser).parseOperation - result 1 (error) is always nil
(unparam)
[error] 166-166: assigned to operationID, but reassigned without using the value
(wastedassign)
🤖 Prompt for AI Agents
In `@pkg/zen/validation/parser.go` around lines 158 - 189, The parseOperation
function currently pre-assigns operationID (operationID := "") then overwrites
it and its signature returns an unused error; remove the wasted assignment and
the unused error return: change the code in parseOperation to declare
operationID only once (e.g., var operationID string or use := when setting from
opMap or generating from method/path) and change the signature from
parseOperation(method, path string, opData any) (*Operation, error) to
parseOperation(method, path string, opData any) *Operation, then update internal
returns (replace any "return nil, nil" with "return nil") and fix all callers to
accept the single *Operation return value. Ensure references to Operation,
operationID, and parseOperation are updated accordingly.
Comment on lines
+11
to
+208
| func TestSecurityScheme_HTTP_Bearer(t *testing.T) { | ||
| t.Parallel() | ||
| scheme := SecurityScheme{ | ||
| Type: SecurityTypeHTTP, | ||
| Scheme: "bearer", | ||
| } | ||
|
|
||
| // Valid bearer token | ||
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | ||
| req.Header.Set("Authorization", "Bearer valid_token") | ||
| require.True(t, validateHTTPScheme(req, scheme)) | ||
|
|
||
| // Missing header | ||
| req = httptest.NewRequest(http.MethodGet, "/test", nil) | ||
| require.False(t, validateHTTPScheme(req, scheme)) | ||
|
|
||
| // Wrong scheme | ||
| req = httptest.NewRequest(http.MethodGet, "/test", nil) | ||
| req.Header.Set("Authorization", "Basic abc123") | ||
| require.False(t, validateHTTPScheme(req, scheme)) | ||
|
|
||
| // Empty token | ||
| req = httptest.NewRequest(http.MethodGet, "/test", nil) | ||
| req.Header.Set("Authorization", "Bearer ") | ||
| require.False(t, validateHTTPScheme(req, scheme)) | ||
| } | ||
|
|
||
| func TestSecurityScheme_HTTP_Basic(t *testing.T) { | ||
| t.Parallel() | ||
| scheme := SecurityScheme{ | ||
| Type: SecurityTypeHTTP, | ||
| Scheme: "basic", | ||
| } | ||
|
|
||
| // Valid basic auth | ||
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | ||
| req.Header.Set("Authorization", "Basic dXNlcjpwYXNz") | ||
| require.True(t, validateHTTPScheme(req, scheme)) | ||
|
|
||
| // Wrong scheme | ||
| req = httptest.NewRequest(http.MethodGet, "/test", nil) | ||
| req.Header.Set("Authorization", "Bearer token") | ||
| require.False(t, validateHTTPScheme(req, scheme)) | ||
| } | ||
|
|
||
| func TestSecurityScheme_APIKey_Header(t *testing.T) { | ||
| t.Parallel() | ||
| scheme := SecurityScheme{ | ||
| Type: SecurityTypeAPIKey, | ||
| Name: "X-Api-Key", | ||
| In: LocationHeader, | ||
| } | ||
|
|
||
| // Valid API key in header | ||
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | ||
| req.Header.Set("X-Api-Key", "my-api-key") | ||
| require.True(t, validateAPIKeyScheme(req, scheme)) | ||
|
|
||
| // Missing header | ||
| req = httptest.NewRequest(http.MethodGet, "/test", nil) | ||
| require.False(t, validateAPIKeyScheme(req, scheme)) | ||
| } | ||
|
|
||
| func TestSecurityScheme_APIKey_Query(t *testing.T) { | ||
| t.Parallel() | ||
| scheme := SecurityScheme{ | ||
| Type: SecurityTypeAPIKey, | ||
| Name: "api_key", | ||
| In: LocationQuery, | ||
| } | ||
|
|
||
| // Valid API key in query | ||
| req := httptest.NewRequest(http.MethodGet, "/test?api_key=my-key", nil) | ||
| require.True(t, validateAPIKeyScheme(req, scheme)) | ||
|
|
||
| // Missing query param | ||
| req = httptest.NewRequest(http.MethodGet, "/test", nil) | ||
| require.False(t, validateAPIKeyScheme(req, scheme)) | ||
| } | ||
|
|
||
| func TestSecurityScheme_APIKey_Cookie(t *testing.T) { | ||
| t.Parallel() | ||
| scheme := SecurityScheme{ | ||
| Type: SecurityTypeAPIKey, | ||
| Name: "session", | ||
| In: LocationCookie, | ||
| } | ||
|
|
||
| // Valid API key in cookie | ||
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | ||
| req.AddCookie(&http.Cookie{Name: "session", Value: "abc123"}) | ||
| require.True(t, validateAPIKeyScheme(req, scheme)) | ||
|
|
||
| // Missing cookie | ||
| req = httptest.NewRequest(http.MethodGet, "/test", nil) | ||
| require.False(t, validateAPIKeyScheme(req, scheme)) | ||
| } | ||
|
|
||
| func TestSecurityScheme_OAuth2(t *testing.T) { | ||
| t.Parallel() | ||
| // OAuth2 only checks for presence of Authorization header | ||
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | ||
| req.Header.Set("Authorization", "Bearer oauth_token") | ||
| require.True(t, validateOAuth2Scheme(req)) | ||
|
|
||
| req = httptest.NewRequest(http.MethodGet, "/test", nil) | ||
| require.False(t, validateOAuth2Scheme(req)) | ||
| } | ||
|
|
||
| func TestSecurityRequirement_ORLogic(t *testing.T) { | ||
| t.Parallel() | ||
| schemes := map[string]SecurityScheme{ | ||
| "bearerAuth": { | ||
| Type: SecurityTypeHTTP, | ||
| Scheme: "bearer", | ||
| }, | ||
| "apiKey": { | ||
| Type: SecurityTypeAPIKey, | ||
| Name: "X-Api-Key", | ||
| In: LocationHeader, | ||
| }, | ||
| } | ||
|
|
||
| // Either bearerAuth OR apiKey should work | ||
| requirements := []SecurityRequirement{ | ||
| {Schemes: map[string][]string{"bearerAuth": {}}}, | ||
| {Schemes: map[string][]string{"apiKey": {}}}, | ||
| } | ||
|
|
||
| // Request with bearer auth | ||
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | ||
| req.Header.Set("Authorization", "Bearer valid_token") | ||
| err := ValidateSecurity(req, requirements, schemes, "test-req-id") | ||
| require.Nil(t, err, "bearer auth should satisfy requirements") | ||
|
|
||
| // Request with API key | ||
| req = httptest.NewRequest(http.MethodGet, "/test", nil) | ||
| req.Header.Set("X-Api-Key", "my-key") | ||
| err = ValidateSecurity(req, requirements, schemes, "test-req-id") | ||
| require.Nil(t, err, "API key should satisfy requirements") | ||
|
|
||
| // Request with neither | ||
| req = httptest.NewRequest(http.MethodGet, "/test", nil) | ||
| err = ValidateSecurity(req, requirements, schemes, "test-req-id") | ||
| require.NotNil(t, err, "no auth should fail requirements") | ||
| } | ||
|
|
||
| func TestSecurityRequirement_ANDLogic(t *testing.T) { | ||
| t.Parallel() | ||
| schemes := map[string]SecurityScheme{ | ||
| "bearerAuth": { | ||
| Type: SecurityTypeHTTP, | ||
| Scheme: "bearer", | ||
| }, | ||
| "apiKey": { | ||
| Type: SecurityTypeAPIKey, | ||
| Name: "X-Api-Key", | ||
| In: LocationHeader, | ||
| }, | ||
| } | ||
|
|
||
| // Both bearerAuth AND apiKey required | ||
| requirements := []SecurityRequirement{ | ||
| {Schemes: map[string][]string{ | ||
| "bearerAuth": {}, | ||
| "apiKey": {}, | ||
| }}, | ||
| } | ||
|
|
||
| // Request with both | ||
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | ||
| req.Header.Set("Authorization", "Bearer valid_token") | ||
| req.Header.Set("X-Api-Key", "my-key") | ||
| err := ValidateSecurity(req, requirements, schemes, "test-req-id") | ||
| require.Nil(t, err, "both auths should satisfy requirements") | ||
|
|
||
| // Request with only bearer | ||
| req = httptest.NewRequest(http.MethodGet, "/test", nil) | ||
| req.Header.Set("Authorization", "Bearer valid_token") | ||
| err = ValidateSecurity(req, requirements, schemes, "test-req-id") | ||
| require.NotNil(t, err, "only bearer should fail AND requirements") | ||
|
|
||
| // Request with only API key | ||
| req = httptest.NewRequest(http.MethodGet, "/test", nil) | ||
| req.Header.Set("X-Api-Key", "my-key") | ||
| err = ValidateSecurity(req, requirements, schemes, "test-req-id") | ||
| require.NotNil(t, err, "only API key should fail AND requirements") | ||
| } | ||
|
|
||
| func TestSecurityRequirement_Empty(t *testing.T) { | ||
| t.Parallel() | ||
| schemes := map[string]SecurityScheme{} | ||
| requirements := []SecurityRequirement{} | ||
|
|
||
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | ||
| err := ValidateSecurity(req, requirements, schemes, "test-req-id") | ||
| require.Nil(t, err, "empty security should pass") | ||
| } |
Contributor
There was a problem hiding this comment.
Use t.Run for scenarios.
Each test packs multiple cases; split into subtests.
✅ Minimal pattern
func TestSecurityScheme_HTTP_Bearer(t *testing.T) {
t.Parallel()
scheme := SecurityScheme{
Type: SecurityTypeHTTP,
Scheme: "bearer",
}
- // Valid bearer token
- req := httptest.NewRequest(http.MethodGet, "/test", nil)
- req.Header.Set("Authorization", "Bearer valid_token")
- require.True(t, validateHTTPScheme(req, scheme))
+ t.Run("valid bearer token", func(t *testing.T) {
+ req := httptest.NewRequest(http.MethodGet, "/test", nil)
+ req.Header.Set("Authorization", "Bearer valid_token")
+ require.True(t, validateHTTPScheme(req, scheme))
+ })
- // Missing header
- req = httptest.NewRequest(http.MethodGet, "/test", nil)
- require.False(t, validateHTTPScheme(req, scheme))
+ t.Run("missing header", func(t *testing.T) {
+ req := httptest.NewRequest(http.MethodGet, "/test", nil)
+ require.False(t, validateHTTPScheme(req, scheme))
+ })
- // Wrong scheme
- req = httptest.NewRequest(http.MethodGet, "/test", nil)
- req.Header.Set("Authorization", "Basic abc123")
- require.False(t, validateHTTPScheme(req, scheme))
+ t.Run("wrong scheme", func(t *testing.T) {
+ req := httptest.NewRequest(http.MethodGet, "/test", nil)
+ req.Header.Set("Authorization", "Basic abc123")
+ require.False(t, validateHTTPScheme(req, scheme))
+ })
- // Empty token
- req = httptest.NewRequest(http.MethodGet, "/test", nil)
- req.Header.Set("Authorization", "Bearer ")
- require.False(t, validateHTTPScheme(req, scheme))
+ t.Run("empty token", func(t *testing.T) {
+ req := httptest.NewRequest(http.MethodGet, "/test", nil)
+ req.Header.Set("Authorization", "Bearer ")
+ require.False(t, validateHTTPScheme(req, scheme))
+ })
}As per coding guidelines, “**/*_test.go: Organize tests using t.Run() for scenarios”.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestSecurityScheme_HTTP_Bearer(t *testing.T) { | |
| t.Parallel() | |
| scheme := SecurityScheme{ | |
| Type: SecurityTypeHTTP, | |
| Scheme: "bearer", | |
| } | |
| // Valid bearer token | |
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | |
| req.Header.Set("Authorization", "Bearer valid_token") | |
| require.True(t, validateHTTPScheme(req, scheme)) | |
| // Missing header | |
| req = httptest.NewRequest(http.MethodGet, "/test", nil) | |
| require.False(t, validateHTTPScheme(req, scheme)) | |
| // Wrong scheme | |
| req = httptest.NewRequest(http.MethodGet, "/test", nil) | |
| req.Header.Set("Authorization", "Basic abc123") | |
| require.False(t, validateHTTPScheme(req, scheme)) | |
| // Empty token | |
| req = httptest.NewRequest(http.MethodGet, "/test", nil) | |
| req.Header.Set("Authorization", "Bearer ") | |
| require.False(t, validateHTTPScheme(req, scheme)) | |
| } | |
| func TestSecurityScheme_HTTP_Basic(t *testing.T) { | |
| t.Parallel() | |
| scheme := SecurityScheme{ | |
| Type: SecurityTypeHTTP, | |
| Scheme: "basic", | |
| } | |
| // Valid basic auth | |
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | |
| req.Header.Set("Authorization", "Basic dXNlcjpwYXNz") | |
| require.True(t, validateHTTPScheme(req, scheme)) | |
| // Wrong scheme | |
| req = httptest.NewRequest(http.MethodGet, "/test", nil) | |
| req.Header.Set("Authorization", "Bearer token") | |
| require.False(t, validateHTTPScheme(req, scheme)) | |
| } | |
| func TestSecurityScheme_APIKey_Header(t *testing.T) { | |
| t.Parallel() | |
| scheme := SecurityScheme{ | |
| Type: SecurityTypeAPIKey, | |
| Name: "X-Api-Key", | |
| In: LocationHeader, | |
| } | |
| // Valid API key in header | |
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | |
| req.Header.Set("X-Api-Key", "my-api-key") | |
| require.True(t, validateAPIKeyScheme(req, scheme)) | |
| // Missing header | |
| req = httptest.NewRequest(http.MethodGet, "/test", nil) | |
| require.False(t, validateAPIKeyScheme(req, scheme)) | |
| } | |
| func TestSecurityScheme_APIKey_Query(t *testing.T) { | |
| t.Parallel() | |
| scheme := SecurityScheme{ | |
| Type: SecurityTypeAPIKey, | |
| Name: "api_key", | |
| In: LocationQuery, | |
| } | |
| // Valid API key in query | |
| req := httptest.NewRequest(http.MethodGet, "/test?api_key=my-key", nil) | |
| require.True(t, validateAPIKeyScheme(req, scheme)) | |
| // Missing query param | |
| req = httptest.NewRequest(http.MethodGet, "/test", nil) | |
| require.False(t, validateAPIKeyScheme(req, scheme)) | |
| } | |
| func TestSecurityScheme_APIKey_Cookie(t *testing.T) { | |
| t.Parallel() | |
| scheme := SecurityScheme{ | |
| Type: SecurityTypeAPIKey, | |
| Name: "session", | |
| In: LocationCookie, | |
| } | |
| // Valid API key in cookie | |
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | |
| req.AddCookie(&http.Cookie{Name: "session", Value: "abc123"}) | |
| require.True(t, validateAPIKeyScheme(req, scheme)) | |
| // Missing cookie | |
| req = httptest.NewRequest(http.MethodGet, "/test", nil) | |
| require.False(t, validateAPIKeyScheme(req, scheme)) | |
| } | |
| func TestSecurityScheme_OAuth2(t *testing.T) { | |
| t.Parallel() | |
| // OAuth2 only checks for presence of Authorization header | |
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | |
| req.Header.Set("Authorization", "Bearer oauth_token") | |
| require.True(t, validateOAuth2Scheme(req)) | |
| req = httptest.NewRequest(http.MethodGet, "/test", nil) | |
| require.False(t, validateOAuth2Scheme(req)) | |
| } | |
| func TestSecurityRequirement_ORLogic(t *testing.T) { | |
| t.Parallel() | |
| schemes := map[string]SecurityScheme{ | |
| "bearerAuth": { | |
| Type: SecurityTypeHTTP, | |
| Scheme: "bearer", | |
| }, | |
| "apiKey": { | |
| Type: SecurityTypeAPIKey, | |
| Name: "X-Api-Key", | |
| In: LocationHeader, | |
| }, | |
| } | |
| // Either bearerAuth OR apiKey should work | |
| requirements := []SecurityRequirement{ | |
| {Schemes: map[string][]string{"bearerAuth": {}}}, | |
| {Schemes: map[string][]string{"apiKey": {}}}, | |
| } | |
| // Request with bearer auth | |
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | |
| req.Header.Set("Authorization", "Bearer valid_token") | |
| err := ValidateSecurity(req, requirements, schemes, "test-req-id") | |
| require.Nil(t, err, "bearer auth should satisfy requirements") | |
| // Request with API key | |
| req = httptest.NewRequest(http.MethodGet, "/test", nil) | |
| req.Header.Set("X-Api-Key", "my-key") | |
| err = ValidateSecurity(req, requirements, schemes, "test-req-id") | |
| require.Nil(t, err, "API key should satisfy requirements") | |
| // Request with neither | |
| req = httptest.NewRequest(http.MethodGet, "/test", nil) | |
| err = ValidateSecurity(req, requirements, schemes, "test-req-id") | |
| require.NotNil(t, err, "no auth should fail requirements") | |
| } | |
| func TestSecurityRequirement_ANDLogic(t *testing.T) { | |
| t.Parallel() | |
| schemes := map[string]SecurityScheme{ | |
| "bearerAuth": { | |
| Type: SecurityTypeHTTP, | |
| Scheme: "bearer", | |
| }, | |
| "apiKey": { | |
| Type: SecurityTypeAPIKey, | |
| Name: "X-Api-Key", | |
| In: LocationHeader, | |
| }, | |
| } | |
| // Both bearerAuth AND apiKey required | |
| requirements := []SecurityRequirement{ | |
| {Schemes: map[string][]string{ | |
| "bearerAuth": {}, | |
| "apiKey": {}, | |
| }}, | |
| } | |
| // Request with both | |
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | |
| req.Header.Set("Authorization", "Bearer valid_token") | |
| req.Header.Set("X-Api-Key", "my-key") | |
| err := ValidateSecurity(req, requirements, schemes, "test-req-id") | |
| require.Nil(t, err, "both auths should satisfy requirements") | |
| // Request with only bearer | |
| req = httptest.NewRequest(http.MethodGet, "/test", nil) | |
| req.Header.Set("Authorization", "Bearer valid_token") | |
| err = ValidateSecurity(req, requirements, schemes, "test-req-id") | |
| require.NotNil(t, err, "only bearer should fail AND requirements") | |
| // Request with only API key | |
| req = httptest.NewRequest(http.MethodGet, "/test", nil) | |
| req.Header.Set("X-Api-Key", "my-key") | |
| err = ValidateSecurity(req, requirements, schemes, "test-req-id") | |
| require.NotNil(t, err, "only API key should fail AND requirements") | |
| } | |
| func TestSecurityRequirement_Empty(t *testing.T) { | |
| t.Parallel() | |
| schemes := map[string]SecurityScheme{} | |
| requirements := []SecurityRequirement{} | |
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | |
| err := ValidateSecurity(req, requirements, schemes, "test-req-id") | |
| require.Nil(t, err, "empty security should pass") | |
| } | |
| func TestSecurityScheme_HTTP_Bearer(t *testing.T) { | |
| t.Parallel() | |
| scheme := SecurityScheme{ | |
| Type: SecurityTypeHTTP, | |
| Scheme: "bearer", | |
| } | |
| t.Run("valid bearer token", func(t *testing.T) { | |
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | |
| req.Header.Set("Authorization", "Bearer valid_token") | |
| require.True(t, validateHTTPScheme(req, scheme)) | |
| }) | |
| t.Run("missing header", func(t *testing.T) { | |
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | |
| require.False(t, validateHTTPScheme(req, scheme)) | |
| }) | |
| t.Run("wrong scheme", func(t *testing.T) { | |
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | |
| req.Header.Set("Authorization", "Basic abc123") | |
| require.False(t, validateHTTPScheme(req, scheme)) | |
| }) | |
| t.Run("empty token", func(t *testing.T) { | |
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | |
| req.Header.Set("Authorization", "Bearer ") | |
| require.False(t, validateHTTPScheme(req, scheme)) | |
| }) | |
| } | |
| func TestSecurityScheme_HTTP_Basic(t *testing.T) { | |
| t.Parallel() | |
| scheme := SecurityScheme{ | |
| Type: SecurityTypeHTTP, | |
| Scheme: "basic", | |
| } | |
| t.Run("valid basic auth", func(t *testing.T) { | |
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | |
| req.Header.Set("Authorization", "Basic dXNlcjpwYXNz") | |
| require.True(t, validateHTTPScheme(req, scheme)) | |
| }) | |
| t.Run("wrong scheme", func(t *testing.T) { | |
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | |
| req.Header.Set("Authorization", "Bearer token") | |
| require.False(t, validateHTTPScheme(req, scheme)) | |
| }) | |
| } | |
| func TestSecurityScheme_APIKey_Header(t *testing.T) { | |
| t.Parallel() | |
| scheme := SecurityScheme{ | |
| Type: SecurityTypeAPIKey, | |
| Name: "X-Api-Key", | |
| In: LocationHeader, | |
| } | |
| t.Run("valid API key in header", func(t *testing.T) { | |
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | |
| req.Header.Set("X-Api-Key", "my-api-key") | |
| require.True(t, validateAPIKeyScheme(req, scheme)) | |
| }) | |
| t.Run("missing header", func(t *testing.T) { | |
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | |
| require.False(t, validateAPIKeyScheme(req, scheme)) | |
| }) | |
| } | |
| func TestSecurityScheme_APIKey_Query(t *testing.T) { | |
| t.Parallel() | |
| scheme := SecurityScheme{ | |
| Type: SecurityTypeAPIKey, | |
| Name: "api_key", | |
| In: LocationQuery, | |
| } | |
| t.Run("valid API key in query", func(t *testing.T) { | |
| req := httptest.NewRequest(http.MethodGet, "/test?api_key=my-key", nil) | |
| require.True(t, validateAPIKeyScheme(req, scheme)) | |
| }) | |
| t.Run("missing query param", func(t *testing.T) { | |
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | |
| require.False(t, validateAPIKeyScheme(req, scheme)) | |
| }) | |
| } | |
| func TestSecurityScheme_APIKey_Cookie(t *testing.T) { | |
| t.Parallel() | |
| scheme := SecurityScheme{ | |
| Type: SecurityTypeAPIKey, | |
| Name: "session", | |
| In: LocationCookie, | |
| } | |
| t.Run("valid API key in cookie", func(t *testing.T) { | |
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | |
| req.AddCookie(&http.Cookie{Name: "session", Value: "abc123"}) | |
| require.True(t, validateAPIKeyScheme(req, scheme)) | |
| }) | |
| t.Run("missing cookie", func(t *testing.T) { | |
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | |
| require.False(t, validateAPIKeyScheme(req, scheme)) | |
| }) | |
| } | |
| func TestSecurityScheme_OAuth2(t *testing.T) { | |
| t.Parallel() | |
| t.Run("valid OAuth2", func(t *testing.T) { | |
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | |
| req.Header.Set("Authorization", "Bearer oauth_token") | |
| require.True(t, validateOAuth2Scheme(req)) | |
| }) | |
| t.Run("missing Authorization header", func(t *testing.T) { | |
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | |
| require.False(t, validateOAuth2Scheme(req)) | |
| }) | |
| } | |
| func TestSecurityRequirement_ORLogic(t *testing.T) { | |
| t.Parallel() | |
| schemes := map[string]SecurityScheme{ | |
| "bearerAuth": { | |
| Type: SecurityTypeHTTP, | |
| Scheme: "bearer", | |
| }, | |
| "apiKey": { | |
| Type: SecurityTypeAPIKey, | |
| Name: "X-Api-Key", | |
| In: LocationHeader, | |
| }, | |
| } | |
| requirements := []SecurityRequirement{ | |
| {Schemes: map[string][]string{"bearerAuth": {}}}, | |
| {Schemes: map[string][]string{"apiKey": {}}}, | |
| } | |
| t.Run("request with bearer auth", func(t *testing.T) { | |
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | |
| req.Header.Set("Authorization", "Bearer valid_token") | |
| err := ValidateSecurity(req, requirements, schemes, "test-req-id") | |
| require.Nil(t, err, "bearer auth should satisfy requirements") | |
| }) | |
| t.Run("request with API key", func(t *testing.T) { | |
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | |
| req.Header.Set("X-Api-Key", "my-key") | |
| err := ValidateSecurity(req, requirements, schemes, "test-req-id") | |
| require.Nil(t, err, "API key should satisfy requirements") | |
| }) | |
| t.Run("request with neither", func(t *testing.T) { | |
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | |
| err := ValidateSecurity(req, requirements, schemes, "test-req-id") | |
| require.NotNil(t, err, "no auth should fail requirements") | |
| }) | |
| } | |
| func TestSecurityRequirement_ANDLogic(t *testing.T) { | |
| t.Parallel() | |
| schemes := map[string]SecurityScheme{ | |
| "bearerAuth": { | |
| Type: SecurityTypeHTTP, | |
| Scheme: "bearer", | |
| }, | |
| "apiKey": { | |
| Type: SecurityTypeAPIKey, | |
| Name: "X-Api-Key", | |
| In: LocationHeader, | |
| }, | |
| } | |
| requirements := []SecurityRequirement{ | |
| {Schemes: map[string][]string{ | |
| "bearerAuth": {}, | |
| "apiKey": {}, | |
| }}, | |
| } | |
| t.Run("request with both", func(t *testing.T) { | |
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | |
| req.Header.Set("Authorization", "Bearer valid_token") | |
| req.Header.Set("X-Api-Key", "my-key") | |
| err := ValidateSecurity(req, requirements, schemes, "test-req-id") | |
| require.Nil(t, err, "both auths should satisfy requirements") | |
| }) | |
| t.Run("request with only bearer", func(t *testing.T) { | |
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | |
| req.Header.Set("Authorization", "Bearer valid_token") | |
| err := ValidateSecurity(req, requirements, schemes, "test-req-id") | |
| require.NotNil(t, err, "only bearer should fail AND requirements") | |
| }) | |
| t.Run("request with only API key", func(t *testing.T) { | |
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | |
| req.Header.Set("X-Api-Key", "my-key") | |
| err := ValidateSecurity(req, requirements, schemes, "test-req-id") | |
| require.NotNil(t, err, "only API key should fail AND requirements") | |
| }) | |
| } | |
| func TestSecurityRequirement_Empty(t *testing.T) { | |
| t.Parallel() | |
| schemes := map[string]SecurityScheme{} | |
| requirements := []SecurityRequirement{} | |
| t.Run("empty security", func(t *testing.T) { | |
| req := httptest.NewRequest(http.MethodGet, "/test", nil) | |
| err := ValidateSecurity(req, requirements, schemes, "test-req-id") | |
| require.Nil(t, err, "empty security should pass") | |
| }) | |
| } |
🤖 Prompt for AI Agents
In `@pkg/zen/validation/security_test.go` around lines 11 - 208, Tests like
TestSecurityScheme_HTTP_Bearer, TestSecurityScheme_HTTP_Basic,
TestSecurityScheme_APIKey_Header/Query/Cookie, TestSecurityScheme_OAuth2,
TestSecurityRequirement_ORLogic/ANDLogic/Empty pack multiple assertions per
function; refactor each distinct scenario (valid, missing, wrong-scheme, empty
token, etc.) into t.Run subtests so failures are isolated and readable. For each
original test function, wrap each case that calls validateHTTPScheme,
validateAPIKeyScheme, validateOAuth2Scheme or ValidateSecurity in its own t.Run
with a descriptive name, keep t.Parallel() where appropriate, and move shared
setup into closures to avoid test-state leaks.
Comment on lines
+209
to
+242
| func buildHTTPAuthError(r *http.Request, requestID string, scheme string) *SecurityError { | ||
| // Title-case the scheme for user-facing messages (e.g. "bearer" -> "Bearer") | ||
| displayScheme := strings.ToUpper(scheme[:1]) + strings.ToLower(scheme[1:]) | ||
|
|
||
| authHeader := r.Header.Get("Authorization") | ||
|
|
||
| if authHeader == "" { | ||
| return newSecurityError(requestID, | ||
| "Authorization header is required but was not provided", | ||
| "https://unkey.com/docs/errors/unkey/authentication/missing", | ||
| ) | ||
| } | ||
|
|
||
| prefix := strings.ToLower(scheme) + " " | ||
| if len(authHeader) < len(prefix) || !strings.EqualFold(authHeader[:len(prefix)], prefix) { | ||
| return newSecurityError(requestID, | ||
| "Authorization header must use "+displayScheme+" scheme", | ||
| "https://unkey.com/docs/errors/unkey/authentication/malformed", | ||
| ) | ||
| } | ||
|
|
||
| token := strings.TrimSpace(authHeader[len(prefix):]) | ||
| if token == "" { | ||
| return newSecurityError(requestID, | ||
| displayScheme+" credentials are empty", | ||
| "https://unkey.com/docs/errors/unkey/authentication/malformed", | ||
| ) | ||
| } | ||
|
|
||
| // Unreachable if validateHTTPScheme is consistent, but satisfies the compiler | ||
| return newSecurityError(requestID, | ||
| displayScheme+" authentication failed", | ||
| "https://unkey.com/docs/errors/unkey/authentication/missing", | ||
| ) |
Contributor
There was a problem hiding this comment.
Guard empty scheme to avoid panic.
scheme[:1] panics when scheme is empty (missing/invalid spec).
🔧 Minimal fix
func buildHTTPAuthError(r *http.Request, requestID string, scheme string) *SecurityError {
+ if scheme == "" {
+ return newSecurityError(requestID,
+ "Authorization header is required but was not provided",
+ "https://unkey.com/docs/errors/unkey/authentication/missing",
+ )
+ }
// Title-case the scheme for user-facing messages (e.g. "bearer" -> "Bearer")
displayScheme := strings.ToUpper(scheme[:1]) + strings.ToLower(scheme[1:])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func buildHTTPAuthError(r *http.Request, requestID string, scheme string) *SecurityError { | |
| // Title-case the scheme for user-facing messages (e.g. "bearer" -> "Bearer") | |
| displayScheme := strings.ToUpper(scheme[:1]) + strings.ToLower(scheme[1:]) | |
| authHeader := r.Header.Get("Authorization") | |
| if authHeader == "" { | |
| return newSecurityError(requestID, | |
| "Authorization header is required but was not provided", | |
| "https://unkey.com/docs/errors/unkey/authentication/missing", | |
| ) | |
| } | |
| prefix := strings.ToLower(scheme) + " " | |
| if len(authHeader) < len(prefix) || !strings.EqualFold(authHeader[:len(prefix)], prefix) { | |
| return newSecurityError(requestID, | |
| "Authorization header must use "+displayScheme+" scheme", | |
| "https://unkey.com/docs/errors/unkey/authentication/malformed", | |
| ) | |
| } | |
| token := strings.TrimSpace(authHeader[len(prefix):]) | |
| if token == "" { | |
| return newSecurityError(requestID, | |
| displayScheme+" credentials are empty", | |
| "https://unkey.com/docs/errors/unkey/authentication/malformed", | |
| ) | |
| } | |
| // Unreachable if validateHTTPScheme is consistent, but satisfies the compiler | |
| return newSecurityError(requestID, | |
| displayScheme+" authentication failed", | |
| "https://unkey.com/docs/errors/unkey/authentication/missing", | |
| ) | |
| func buildHTTPAuthError(r *http.Request, requestID string, scheme string) *SecurityError { | |
| if scheme == "" { | |
| return newSecurityError(requestID, | |
| "Authorization header is required but was not provided", | |
| "https://unkey.com/docs/errors/unkey/authentication/missing", | |
| ) | |
| } | |
| // Title-case the scheme for user-facing messages (e.g. "bearer" -> "Bearer") | |
| displayScheme := strings.ToUpper(scheme[:1]) + strings.ToLower(scheme[1:]) | |
| authHeader := r.Header.Get("Authorization") | |
| if authHeader == "" { | |
| return newSecurityError(requestID, | |
| "Authorization header is required but was not provided", | |
| "https://unkey.com/docs/errors/unkey/authentication/missing", | |
| ) | |
| } | |
| prefix := strings.ToLower(scheme) + " " | |
| if len(authHeader) < len(prefix) || !strings.EqualFold(authHeader[:len(prefix)], prefix) { | |
| return newSecurityError(requestID, | |
| "Authorization header must use "+displayScheme+" scheme", | |
| "https://unkey.com/docs/errors/unkey/authentication/malformed", | |
| ) | |
| } | |
| token := strings.TrimSpace(authHeader[len(prefix):]) | |
| if token == "" { | |
| return newSecurityError(requestID, | |
| displayScheme+" credentials are empty", | |
| "https://unkey.com/docs/errors/unkey/authentication/malformed", | |
| ) | |
| } | |
| // Unreachable if validateHTTPScheme is consistent, but satisfies the compiler | |
| return newSecurityError(requestID, | |
| displayScheme+" authentication failed", | |
| "https://unkey.com/docs/errors/unkey/authentication/missing", | |
| ) | |
| } |
🤖 Prompt for AI Agents
In `@pkg/zen/validation/security.go` around lines 209 - 242, In buildHTTPAuthError
guard against an empty scheme before using scheme[:1]: check len(scheme) == 0 at
the top of the function and return a SecurityError (e.g., using newSecurityError
with a clear message and the malformed/missing auth docs URL) so the function
does not panic; then only compute displayScheme and prefix after that check
(references: buildHTTPAuthError, newSecurityError, displayScheme, prefix).
Ensure the returned error for an empty scheme uses the same error URL pattern
used for other malformed authentication inputs.
| type: string | ||
| minLength: 3 | ||
| pattern: ^[a-zA-Z0-9_:\-\.\*]+$ | ||
| pattern: "^[a-zA-Z][a-zA-Z0-9._*-]*$" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check git history for this file to see the original pattern
cd svc/api/openapi/spec/paths/v2/keys/addPermissions && git log --oneline -n 20 V2KeysAddPermissionsRequestBody.yaml 2>/dev/null || echo "File not found or no git history"Repository: unkeyed/unkey
Length of output: 78
🏁 Script executed:
# Find and display the current file and any similar permission validation patterns
fd V2KeysAddPermissionsRequestBody.yaml -x cat {} 2>/dev/null | head -50Repository: unkeyed/unkey
Length of output: 1214
🏁 Script executed:
# Search for other permission-related patterns in OpenAPI specs
rg -n 'pattern.*\^' svc/api/openapi/spec/ --type yaml | grep -i 'permission\|slug' | head -20Repository: unkeyed/unkey
Length of output: 2102
🏁 Script executed:
# Look for permission patterns in the codebase to understand the convention
rg -B3 -A1 'permission.*pattern|slug.*pattern' --type yaml svc/api/openapi/spec/ | head -40Repository: unkeyed/unkey
Length of output: 2515
🏁 Script executed:
cat svc/api/openapi/spec/common/Permission.yamlRepository: unkeyed/unkey
Length of output: 1763
🏁 Script executed:
# Check git diff to see what changed in this commit
git show 2f2aaef3 -- svc/api/openapi/spec/paths/v2/keys/addPermissions/V2KeysAddPermissionsRequestBody.yaml 2>/dev/null | head -80 || echo "Could not show diff"Repository: unkeyed/unkey
Length of output: 1737
Address pattern inconsistency with permission slug specification.
The pattern ^[a-zA-Z][a-zA-Z0-9._*-]*$ is inconsistent with the permission slug specification in Permission.yaml (^[a-zA-Z0-9_:\-\.\*]+$), which allows:
- Colons (e.g.,
api:read) - Leading digits (e.g.,
123test)
This creates an API contract mismatch: permissions with these characters can be created directly but cannot be added to keys via this endpoint. Either:
- Align the pattern here with
Permission.yamlto support all valid permission slugs, or - Document why this endpoint intentionally restricts permission assignment to a subset of valid permissions
🤖 Prompt for AI Agents
In
`@svc/api/openapi/spec/paths/v2/keys/addPermissions/V2KeysAddPermissionsRequestBody.yaml`
at line 29, The pattern in V2KeysAddPermissionsRequestBody.yaml
("^[a-zA-Z][a-zA-Z0-9._*-]*$") conflicts with the permission slug regex in
Permission.yaml ("^[a-zA-Z0-9_:\-\.\*]+$"); update the request body schema for
V2KeysAddPermissionsRequestBody (the permission slug pattern) to match the
Permission.yaml regex so this endpoint accepts colons and leading digits (or, if
intended, add a clear description in the V2KeysAddPermissionsRequestBody
documentation explaining the intentional subset restriction and why it differs
from Permission.yaml).
| type: string | ||
| minLength: 3 | ||
| pattern: ^[a-zA-Z0-9_:\-\.\*]+$ | ||
| pattern: "^[a-zA-Z][a-zA-Z0-9._*-]*$" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and inspect the YAML file
find . -type f -name "V2KeysSetPermissionsRequestBody.yaml" -o -name "*SetPermissions*" | head -20Repository: unkeyed/unkey
Length of output: 325
🏁 Script executed:
# Search for the file in different ways
fd "SetPermissions" --type f | head -20Repository: unkeyed/unkey
Length of output: 319
🏁 Script executed:
# Look for OpenAPI spec files
fd "openapi" --type d | head -10Repository: unkeyed/unkey
Length of output: 282
🏁 Script executed:
cat -n svc/api/openapi/spec/paths/v2/keys/setPermissions/V2KeysSetPermissionsRequestBody.yamlRepository: unkeyed/unkey
Length of output: 1575
🏁 Script executed:
# Also search for other permission patterns in the codebase to understand the standard
rg "pattern.*permission" --type yaml -A 1 -B 1 | head -50Repository: unkeyed/unkey
Length of output: 39
🏁 Script executed:
# Search for permission slug patterns more broadly
rg "\[a-zA-Z0-9_\]" svc/api/openapi/spec/ --type yaml | head -30Repository: unkeyed/unkey
Length of output: 2449
🏁 Script executed:
# Check git history to see what the old pattern was
cd svc/api/openapi/spec/paths/v2/keys/setPermissions && git log -p --follow -S "setPermissions" -- "*.yaml" | head -100Repository: unkeyed/unkey
Length of output: 39
🏁 Script executed:
# Check if there are existing permissions that might start with numbers
rg "^[0-9]" svc/ --type yaml -B 3 -A 1 | head -30Repository: unkeyed/unkey
Length of output: 39
Pattern breaks backward compatibility and deviates from established standard.
New pattern ^[a-zA-Z][a-zA-Z0-9._*-]*$ requires a leading letter, rejecting existing permissions that start with numbers or underscores (e.g., 1_read, _internal). This conflicts with the standard permission pattern ^[a-zA-Z0-9_]+$ used consistently across all other permission endpoints (createPermission, addPermissions, removePermissions, Permission.yaml, etc.). Align with the established pattern or document the intentional breaking change.
🤖 Prompt for AI Agents
In
`@svc/api/openapi/spec/paths/v2/keys/setPermissions/V2KeysSetPermissionsRequestBody.yaml`
at line 31, The new regex in V2KeysSetPermissionsRequestBody.yaml enforces a
leading letter and breaks compatibility with existing permission names; revert
it to the established permission pattern used elsewhere (e.g.,
createPermission/addPermissions/removePermissions and Permission.yaml) by
replacing the pattern on the permission field `pattern:
"^[a-zA-Z][a-zA-Z0-9._*-]*$"` with the standard pattern `^[a-zA-Z0-9_]+$` (or,
if the leading-letter restriction is intentional, add explicit documentation and
a migration note for V2KeysSetPermissionsRequestBody to avoid surprising
callers).
svc/api/openapi/spec/paths/v2/keys/setRoles/V2KeysSetRolesRequestBody.yaml
Show resolved
Hide resolved
...i/openapi/spec/paths/v2/permissions/getPermission/V2PermissionsGetPermissionRequestBody.yaml
Show resolved
Hide resolved
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.
What does this PR do?
Fixes #3852
Fixes #4805
This pr rips out the libopenapi dependency and replaces it by doing half our own openapi validation and relying on
github.com/santhosh-tekuri/jsonschema/v6for request body parsing. This was also user in libopenapi under the hood.By doing so we should hopefully have improved performance as this allows us to tailor the validator to our needs like
xh POST http://127.0.0.1:7070/v2/analytics.getVerifications authorization:"Bearer XXXX" 1query="...."New error message:
before:
having one less dependency
Changes our openapi spec to use
application/problem+jsonfor error responses.adds schema location for the request so ppl know where it went wrong.
Type of change
How should this be tested?
Uhhh test
Checklist
Required
pnpm buildpnpm fmtmake fmton/godirectoryconsole.logsgit pull origin mainAppreciated