feat: improve logging of authorization errors and allow use customization #2431
Conversation
WalkthroughAdds JWK "use" customization end-to-end: new JWKS server options, JWK marshalling with configurable use, AllowedUse propagation into JWKSConfig and token decoder, and unauthorized error message masking in GraphQL and websocket paths. Tests and config schema updated to exercise the new behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
Comment |
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2431 +/- ##
==========================================
- Coverage 62.75% 60.85% -1.90%
==========================================
Files 296 229 -67
Lines 41353 23830 -17523
Branches 4244 0 -4244
==========================================
- Hits 25951 14502 -11449
+ Misses 15381 8061 -7320
- Partials 21 1267 +1246
🚀 New features to boost your workflow:
|
alepane21
changed the title
…r-with-valid-token
alepane21
requested review from
Noroth,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
router/core/websocket.go (1)
399-410: Consider adding debug logging for the original error.The error sanitization correctly prevents leaking internal error details to clients. However, unlike
handleAuthenticationFailureingraphql_prehandler.go(which logs the original error at debug level on line 1143), this path silently discards the underlying error details. Consider adding a debug log to aid troubleshooting:🔎 Suggested improvement
if h.accessController != nil { handler.request, err = h.accessController.Access(w, r) if err != nil { statusCode := http.StatusForbidden errorMessage := err if errors.Is(err, ErrUnauthorized) { statusCode = http.StatusUnauthorized errorMessage = ErrUnauthorized } + requestLogger.Debug("Failed to authenticate websocket connection from initial payload", zap.Error(err)) http.Error(handler.w, http.StatusText(statusCode), statusCode) _ = handler.writeErrorMessage(requestID, errorMessage) handler.Close(false) return } }router/pkg/authentication/jwks_token_decoder.go (2)
255-265: Consider clarifying the behavior for empty slice vs. nil.The function returns
[UseSig]whenallowedUseisnil, but returns an empty slice whenallowedUseis[]string{}. This distinction might be confusing:
nil→[UseSig](default, restricts to "sig" use)[]string{}(empty slice) →[](empty whitelist, might block all keys)[]string{""}→[USE("")](allows empty "use")While this appears intentional for the use case, consider either:
- Treating empty slice the same as nil (return default), or
- Adding a comment explaining this behavior
💡 Optional: Treat empty slice as default
func getUseWhitelist(allowedUse []string) []jwkset.USE { - if allowedUse == nil { + if len(allowedUse) == 0 { return []jwkset.USE{jwkset.UseSig} } useWhitelist := make([]jwkset.USE, len(allowedUse)) for i, u := range allowedUse { useWhitelist[i] = jwkset.USE(u) } return useWhitelist }
48-48: Add validation for AllowedUse values ingetUseWhitelist().The
AllowedUsestrings are cast directly tojwkset.USEwithout validation. While the jwkset library providesIANARegistered()for validation, the application should validate against the known USE values ("sig" and "enc" per RFC 7517) before passing them downstream. This prevents invalid values from propagating to the jwkset library.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
router-tests/authentication_test.gorouter-tests/cmd/jwks-server/main.gorouter-tests/jwks/crypto.gorouter-tests/jwks/jwks.gorouter/core/access_controller.gorouter/core/graphql_prehandler.gorouter/core/supervisor_instance.gorouter/core/websocket.gorouter/pkg/authentication/jwks_token_decoder.gorouter/pkg/config/config.gorouter/pkg/config/config.schema.jsonrouter/pkg/config/testdata/config_full.json
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2067
File: router/pkg/authentication/jwks_token_decoder.go:80-106
Timestamp: 2025-07-21T14:46:34.879Z
Learning: In the Cosmo router project, required field validation for JWKS configuration (Secret, Algorithm, KeyId) is handled at the JSON schema level in config.schema.json rather than through runtime validation in the Go code at router/pkg/authentication/jwks_token_decoder.go.
📚 Learning: 2025-07-21T14:46:34.879Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2067
File: router/pkg/authentication/jwks_token_decoder.go:80-106
Timestamp: 2025-07-21T14:46:34.879Z
Learning: In the Cosmo router project, required field validation for JWKS configuration (Secret, Algorithm, KeyId) is handled at the JSON schema level in config.schema.json rather than through runtime validation in the Go code at router/pkg/authentication/jwks_token_decoder.go.
Applied to files:
router-tests/cmd/jwks-server/main.gorouter/pkg/config/config.schema.jsonrouter/pkg/authentication/jwks_token_decoder.gorouter/core/supervisor_instance.gorouter/pkg/config/testdata/config_full.jsonrouter-tests/jwks/jwks.gorouter/pkg/config/config.go
📚 Learning: 2025-11-19T15:13:57.821Z
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2273
File: router/core/graphql_handler.go:0-0
Timestamp: 2025-11-19T15:13:57.821Z
Learning: In the Cosmo router (wundergraph/cosmo), error handling follows a two-phase pattern: (1) Prehandler phase handles request parsing, validation, and setup errors using `httpGraphqlError` and `writeOperationError` (in files like graphql_prehandler.go, operation_processor.go, parse_multipart.go, batch.go); (2) Execution phase handles resolver execution errors using `WriteError` in GraphQLHandler.ServeHTTP. Because all `httpGraphqlError` instances are caught in the prehandler before ServeHTTP is invoked, any error type checks for `httpGraphqlError` in the execution-phase WriteError method are unreachable code.
Applied to files:
router/core/graphql_prehandler.go
📚 Learning: 2025-07-21T15:06:36.664Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2067
File: router/pkg/config/config.schema.json:1637-1644
Timestamp: 2025-07-21T15:06:36.664Z
Learning: In the Cosmo router project, when extending JSON schema validation for security-sensitive fields like JWKS secrets, backwards compatibility is maintained by implementing warnings in the Go code rather than hard validation constraints in the schema. This allows existing configurations to continue working while alerting users to potential security issues.
Applied to files:
router/pkg/config/config.schema.jsonrouter/pkg/config/testdata/config_full.jsonrouter/pkg/config/config.go
📚 Learning: 2025-09-17T18:13:31.021Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2224
File: router/pkg/authentication/jwks_token_decoder.go:176-199
Timestamp: 2025-09-17T18:13:31.021Z
Learning: In router/pkg/authentication/jwks_token_decoder.go, when AllowEmptyAlgorithm is true, the allowedAlgorithms slice is populated by getSupportedAlgorithms() from validation_store.go, ensuring it's never empty and preventing unintended token rejections.
Applied to files:
router/pkg/config/config.schema.jsonrouter/pkg/authentication/jwks_token_decoder.gorouter/core/supervisor_instance.gorouter/pkg/config/testdata/config_full.jsonrouter/pkg/config/config.go
📚 Learning: 2025-09-17T18:13:31.021Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2224
File: router/pkg/authentication/jwks_token_decoder.go:176-199
Timestamp: 2025-09-17T18:13:31.021Z
Learning: In router/pkg/authentication/jwks_token_decoder.go, when AllowEmptyAlgorithm is true, the allowedAlgorithms slice is populated by getSupportedAlgorithms() from validation_store.go, ensuring it's never empty and preventing unintended token rejections. The validation store maintains its own internal supported algorithms map that gets returned regardless of input parameters.
Applied to files:
router/pkg/config/config.schema.jsonrouter/pkg/authentication/jwks_token_decoder.gorouter/core/supervisor_instance.gorouter/pkg/config/testdata/config_full.jsonrouter/pkg/config/config.go
📚 Learning: 2025-08-20T10:08:17.857Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: In the Cosmo router codebase, JSON schema validation prevents null values in TrafficShapingRules subgraph configurations, making nil checks unnecessary when dereferencing subgraph rule pointers in NewSubgraphTransportOptions.
Applied to files:
router/pkg/config/config.schema.json
📚 Learning: 2025-08-20T10:08:17.857Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: router/pkg/config/config.schema.json forbids null values for traffic_shaping.subgraphs: additionalProperties references $defs.traffic_shaping_subgraph_request_rule with type "object". Therefore, in core.NewSubgraphTransportOptions, dereferencing each subgraph rule pointer is safe under schema-validated configs, and a nil-check is unnecessary.
Applied to files:
router/pkg/config/config.schema.jsonrouter/pkg/config/testdata/config_full.json
📚 Learning: 2025-09-29T10:28:07.122Z
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2192
File: router/pkg/config/config.go:1028-1029
Timestamp: 2025-09-29T10:28:07.122Z
Learning: The deprecation strategy for IntrospectionEnabled field in router/pkg/config/config.go is to first mark it as deprecated, then migrate all call sites to use IntrospectionConfig.Enabled, and finally remove the deprecated field. The envDefault tag should be kept during the deprecation period for backward compatibility.
Applied to files:
router/pkg/config/config.go
📚 Learning: 2025-08-28T09:59:19.999Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2167
File: router/core/supervisor_instance.go:196-204
Timestamp: 2025-08-28T09:59:19.999Z
Learning: In router/pkg/config/config.go, the BackoffJitterRetry struct uses envDefault tags to provide default values for Algorithm ("backoff_jitter") and Expression ("IsRetryableStatusCode() || IsConnectionError() || IsTimeout()"), ensuring that even when YAML configs omit these fields, valid defaults are applied automatically, preventing ProcessRetryOptions from rejecting empty values.
Applied to files:
router/pkg/config/config.go
🧬 Code graph analysis (3)
router/core/graphql_prehandler.go (2)
router/core/access_controller.go (1)
ErrUnauthorized(16-16)router/pkg/pubsub/datasource/error.go (1)
Error(3-6)
router/core/websocket.go (1)
router/core/access_controller.go (1)
ErrUnauthorized(16-16)
router-tests/jwks/jwks.go (2)
router-tests/jwks/crypto.go (1)
Crypto(17-23)router-tests/cmd/jwks-server/main.go (1)
NewServerWithCrypto(214-252)
🔇 Additional comments (9)
router-tests/jwks/crypto.go (1)
17-23: LGTM! Clean interface extension.The new
MarshalJWKWithUsemethod is properly added to the interface, and existing implementations correctly delegateMarshalJWK()to the new method with the defaultjwkset.UseSigvalue, maintaining backward compatibility.router-tests/cmd/jwks-server/main.go (1)
52-56: LGTM! Import reorganization only.No functional changes; imports were simply reordered for consistency.
router/core/access_controller.go (1)
51-53: LGTM! Good error composition pattern.Using
errors.Join(err, ErrUnauthorized)preserves the underlying authentication error for debugging and logging while ensuringerrors.Is(err, ErrUnauthorized)still returns true. This enables the sanitization logic ingraphql_prehandler.goandwebsocket.goto mask internal error details from clients.router/pkg/config/testdata/config_full.json (1)
489-542: LGTM! Test data updated to reflect new schema.The
AllowedUsefield is correctly added to each JWKS entry withnullvalues, which will exercise default behavior during testing.router/core/graphql_prehandler.go (1)
1149-1158: LGTM! Proper error sanitization for security.The implementation correctly sanitizes error details in the GraphQL response while preserving the full error for logging (line 1143), request context (line 1142), and observability spans (lines 1146-1147). This prevents leaking sensitive authentication failure details to clients.
router/pkg/config/config.go (1)
486-501: LGTM! AllowedUse field added correctly.The
AllowedUse []stringfield is properly added toJWKSConfiguration. Based on learnings, validation for JWKS configuration fields is handled at the JSON schema level rather than runtime validation in Go code.router/core/supervisor_instance.go (1)
290-309: LGTM! Configuration propagation is correct.The
AllowedUsefield is properly propagated from the JWKS configuration to theauthentication.JWKSConfig, consistent with how other JWKS settings are passed through.router-tests/authentication_test.go (1)
4019-4134: LGTM! Well-structured test for the new "use" customization feature.The test properly validates both the default behavior (rejecting tokens when JWK has empty "use" field) and the customized behavior (allowing empty "use" when explicitly configured). The helper functions follow existing patterns and make the test readable.
router-tests/jwks/jwks.go (1)
136-215: LGTM! Clean refactoring to options-based initialization.The introduction of the options pattern is idiomatic and maintains backward compatibility. Key improvements:
- Backward compatibility:
NewServerWithCryptostill works via delegation- Flexibility: Options allow customizing "use" parameter without breaking existing code
- Good defaults: Default
useisUseSig, preserving previous behavior- Validation: Checks that at least one provider is specified
The refactoring properly supports the new JWKS "use" customization feature while keeping the API clean.
…401-on-router-with-valid-token' into ale/eng-8704-jwt-authentication-401-on-router-with-valid-token
…r-with-valid-token
|
|
||
| func createKeyFunc(ctx context.Context, options jwkset.HTTPClientOptions) (keyfunc.Keyfunc, error) { | ||
| func getUseWhitelist(allowedUse []string) []jwkset.USE { | ||
| if allowedUse == nil { |
There was a problem hiding this comment.
Hypothetically allowedUse could also be an empty slice. len returns 0 for both, empty slices and nil.
| if allowedUse == nil { | |
| if len(allowedUse) == 0 { |
| } | ||
|
|
||
| func createKeyFunc(ctx context.Context, options jwkset.HTTPClientOptions) (keyfunc.Keyfunc, error) { | ||
| func getUseWhitelist(allowedUse []string) []jwkset.USE { |
There was a problem hiding this comment.
Function name is a bit wonky imo. Given that jwks.USE is defined as
USE is a set of "JSON Web Key Use" types
How about toKeyUseTypes ?
By default, we force to check that the jwks keys have the use param set to "sig". By standard the value is optional and it could also be left empty.
With this PR I introduce two changes:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist