Conversation
SkArchon
requested review from
Noroth,
StarpTech,
devsergiy and
jensneuse
as code owners
WalkthroughAudience validation was added to JWT authentication by extending configuration schemas and internal structures to support an "audiences" field for JWKS entries. The token decoder logic was refactored to enforce audience checks and support multiple JWKS sources. Related tests were updated and expanded to verify audience validation and error handling for duplicate JWKS configurations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~80 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
🧰 Additional context used🧠 Learnings (2)📓 Common learningsrouter/pkg/config/config_test.go (2)Learnt from: SkArchon Learnt from: SkArchon ⏰ 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). (5)
🔇 Additional comments (3)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
router/pkg/config/config.schema.json (1)
1637-1643: Consider tightening validation constraints for the newaudiencesarrayThe array currently accepts an empty or duplicated list, which could lead to configuration mistakes slipping through (e.g.
[]or["foo","foo"]). A minimal, unique non-empty list seems more appropriate.- "audiences": { - "type": "array", - "description": "The audiences of the JWKs. The audiences are used to verify the JWT (JSON Web Token). The audiences are specified as a list of strings.", - "items": { - "type": "string" - } - }, + "audiences": { + "type": "array", + "minItems": 1, + "uniqueItems": true, + "description": "One or more expected JWT `aud` claims accepted for this JWKS.", + "items": { + "type": "string", + "minLength": 1 + } + },Adds:
•minItems: 1– disallow empty lists.
•uniqueItems: true– prevent duplicates.
•minLength: 1for each string to block"".If the runtime treats an empty slice as “no audience validation”, consider documenting that and omitting the field instead of allowing an empty list.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
router-tests/authentication_test.go(8 hunks)router/core/supervisor_instance.go(1 hunks)router/pkg/authentication/authentication.go(1 hunks)router/pkg/authentication/jwks_token_decoder.go(6 hunks)router/pkg/config/config.go(1 hunks)router/pkg/config/config.schema.json(1 hunks)router/pkg/config/testdata/config_full.json(2 hunks)
🧠 Learnings (7)
📓 Common learnings
Learnt from: SkArchon
PR: wundergraph/cosmo#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.
Learnt from: SkArchon
PR: wundergraph/cosmo#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.
router/pkg/config/testdata/config_full.json (2)
Learnt from: SkArchon
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.
Learnt from: SkArchon
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.
router/pkg/config/config.go (2)
Learnt from: SkArchon
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.
Learnt from: SkArchon
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.
router/core/supervisor_instance.go (1)
Learnt from: SkArchon
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.
router-tests/authentication_test.go (2)
Learnt from: SkArchon
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.
Learnt from: SkArchon
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.
router/pkg/config/config.schema.json (3)
Learnt from: SkArchon
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.
Learnt from: SkArchon
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.
Learnt from: SkArchon
PR: #1929
File: router/internal/circuit/manager.go:16-25
Timestamp: 2025-06-30T20:39:02.387Z
Learning: In the Cosmo router project, parameter validation for circuit breaker configuration is handled at the JSON schema level rather than through runtime validation methods on structs. The config.schema.json file contains comprehensive validation constraints for circuit breaker parameters.
router/pkg/authentication/jwks_token_decoder.go (1)
Learnt from: SkArchon
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.
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: SkArchon
PR: wundergraph/cosmo#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.
Learnt from: SkArchon
PR: wundergraph/cosmo#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.
router/pkg/config/testdata/config_full.json (2)
Learnt from: SkArchon
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.
Learnt from: SkArchon
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.
router/pkg/config/config.go (2)
Learnt from: SkArchon
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.
Learnt from: SkArchon
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.
router/core/supervisor_instance.go (1)
Learnt from: SkArchon
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.
router-tests/authentication_test.go (2)
Learnt from: SkArchon
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.
Learnt from: SkArchon
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.
router/pkg/config/config.schema.json (3)
Learnt from: SkArchon
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.
Learnt from: SkArchon
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.
Learnt from: SkArchon
PR: #1929
File: router/internal/circuit/manager.go:16-25
Timestamp: 2025-06-30T20:39:02.387Z
Learning: In the Cosmo router project, parameter validation for circuit breaker configuration is handled at the JSON schema level rather than through runtime validation methods on structs. The config.schema.json file contains comprehensive validation constraints for circuit breaker parameters.
router/pkg/authentication/jwks_token_decoder.go (1)
Learnt from: SkArchon
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.
⏰ 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). (12)
- GitHub Check: build-router
- GitHub Check: build_test
- GitHub Check: image_scan
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (12)
router/pkg/authentication/authentication.go (1)
80-80: LGTM! Well-defined error variable for audience validation.The new error variable follows Go conventions and provides a clear message for audience validation failures. The placement at package level makes it appropriately accessible to the JWT token decoder.
router/pkg/config/testdata/config_full.json (1)
472-474: LGTM! Test configuration properly updated for audience validation.The addition of the "Audiences" field with null values to all JWKS entries correctly reflects the new schema structure. This allows for testing both scenarios where audiences are specified and where they are not.
Also applies to: 484-486, 493-495
router/pkg/config/config.go (1)
470-472: Well-positioned field addition for audience validation.The
Audiences []stringfield is appropriately placed in the "Common" section of the JWKSConfiguration struct, making it available for both URL-based and secret-based JWKS configurations. The field type and YAML tag are consistent with other fields in the struct.router/core/supervisor_instance.go (1)
263-263: Proper integration of audience configuration into authentication setup.This change correctly passes the audience configuration from the JWT JWKS config to the authentication layer, completing the data flow needed for audience validation in the token decoder.
router-tests/authentication_test.go (5)
686-705: LGTM! Good test coverage for duplicate URL validation.The test properly validates that startup fails when duplicate JWKS URLs are specified, ensuring the system catches configuration errors early.
789-792: LGTM! Properly tests audience claims in multi-JWKS configuration.The addition of the "aud" claim ensures the test validates audience handling when multiple JWKS configurations are present.
834-854: LGTM! Good validation for duplicate key IDs.The test ensures that duplicate key IDs in manually specified JWKS configurations are detected at startup.
2077-2503: Excellent comprehensive test coverage for audience validation!The test function thoroughly covers all critical audience validation scenarios:
- Authentication failure when audiences don't match
- Authentication success when audiences match
- Proper handling of different audience claim formats (string vs string array)
- Error handling for invalid audience formats
- Validation bypass when no expected audience is configured
Each scenario is tested with both HTTP-based and secret-based JWKS configurations, ensuring complete coverage.
2513-2517: LGTM! Clean backward-compatible implementation.The function properly handles custom claims while maintaining backward compatibility by initializing empty claims when nil is passed.
router/pkg/authentication/jwks_token_decoder.go (3)
24-29: LGTM! Clean type updates for audience validation support.The changes properly update the types to support the new audience validation feature:
- Migration from
keyfunc.Keyfunctojwt.Keyfuncis consistentAudKeystruct appropriately handles both URL and key ID based identificationAlso applies to: 51-57
189-220: LGTM! Well-implemented helper functions.Both helper functions are clean and efficient:
createKeyFuncproperly encapsulates the keyfunc creation logichasAudienceefficiently checks for audience intersection using a map-based approach
175-175: errUnacceptableAud is defined and properly scoped
The variableerrUnacceptableAudis declared inrouter/pkg/authentication/authentication.goand is accessible within theauthenticationpackage. No further action needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
router-tests/authentication_test.go (1)
2077-2505: Excellent comprehensive test coverage for audience validation.This test function thoroughly covers all critical audience validation scenarios:
- Authentication failures with mismatched audiences (slice and string formats)
- Authentication success with matching audiences
- Invalid audience format handling
- Behavior when no expected audience is configured
- Both HTTP-based and secret-based JWKS configurations
The test structure is well-organized with proper parallel execution and consistent assertion patterns.
Minor suggestion for future maintainability:
Consider extracting common test patterns into helper functions to reduce code duplication, especially for the HTTP vs secret-based configuration setup patterns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
router-tests/authentication_test.go(8 hunks)router/pkg/authentication/jwks_token_decoder.go(6 hunks)
🧠 Learnings (3)
📓 Common learnings
Learnt from: SkArchon
PR: wundergraph/cosmo#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.
Learnt from: SkArchon
PR: wundergraph/cosmo#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.
router-tests/authentication_test.go (2)
Learnt from: SkArchon
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.
Learnt from: SkArchon
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.
router/pkg/authentication/jwks_token_decoder.go (1)
Learnt from: SkArchon
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.
🧬 Code Graph Analysis (1)
router/pkg/authentication/jwks_token_decoder.go (1)
router/pkg/authentication/authentication.go (1)
Claims(10-10)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: SkArchon
PR: wundergraph/cosmo#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.
Learnt from: SkArchon
PR: wundergraph/cosmo#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.
router-tests/authentication_test.go (2)
Learnt from: SkArchon
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.
Learnt from: SkArchon
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.
router/pkg/authentication/jwks_token_decoder.go (1)
Learnt from: SkArchon
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.
🧬 Code Graph Analysis (1)
router/pkg/authentication/jwks_token_decoder.go (1)
router/pkg/authentication/authentication.go (1)
Claims(10-10)
⏰ 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). (12)
- GitHub Check: build-router
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: build_test
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (15)
router-tests/authentication_test.go (7)
686-705: LGTM! Well-structured test for duplicate URL detection.The test correctly validates that the system prevents duplicate JWKS URLs in configuration and fails fast with an appropriate error message.
789-792: LGTM! Appropriate addition of audience claim for testing.The aud claim addition supports testing the new audience validation functionality in multi-JWKS configurations.
834-854: LGTM! Comprehensive test for duplicate key ID detection.The test correctly validates that the system prevents duplicate key IDs in secret-based JWKS configurations and provides appropriate error feedback.
869-869: LGTM! Proper update for modified generateToken signature.The nil claims parameter maintains existing test behavior while supporting the enhanced function signature.
912-912: LGTM! Consistent update for generateToken function.Appropriately passes nil claims to maintain existing test behavior.
951-951: LGTM! Consistent parameter handling.Correctly maintains existing behavior by passing nil for the new claims parameter.
2515-2524: LGTM! Well-designed function enhancement.The updated generateToken function properly supports custom claims while maintaining backward compatibility:
- Handles nil claims gracefully by initializing empty MapClaims
- Uses appropriate jwt.NewWithClaims method
- Maintains existing functionality for existing callers
This enhancement enables comprehensive testing of audience validation scenarios.
router/pkg/authentication/jwks_token_decoder.go (8)
5-5: LGTM! Necessary import for error handling.The errors import is required for the new error aggregation logic using errors.Join.
24-24: LGTM! Appropriate type change for multi-JWKS support.The change from
keyfunc.Keyfunctojwt.Keyfuncaligns with the new architecture that aggregates multiple JWKS sources into a single key function.
29-29: LGTM! Correct usage of the new jwt.Keyfunc type.The direct usage of j.jwks with jwt.Parse is appropriate since j.jwks is now a jwt.Keyfunc rather than a keyfunc.Keyfunc.
51-52: LGTM! Appropriate extension for audience validation.The Audiences field enables per-JWKS configuration of expected token audiences, supporting the new audience validation feature.
54-57: LGTM! Well-designed key identification struct.The AudKey struct provides a clean way to uniquely identify JWKS configurations by either URL or key ID, enabling unified handling of different configuration types.
59-186: Excellent refactoring for multi-JWKS support with audience validation.The refactored function successfully implements:
✅ Duplicate Detection: Properly prevents duplicate URLs and key IDs
✅ Multi-JWKS Support: Handles both HTTP-based and secret-based configurations
✅ Audience Validation: Enforces audience checks when configured
✅ Error Handling: Aggregates errors from multiple key attempts using errors.Join correctly
✅ Unified Interface: Returns a single jwt.Keyfunc that handles all configurationsThe implementation correctly addresses the issues mentioned in past review comments:
- Error messages now reference the correct identifier (key ID vs URL)
- No duplicate audience assignments
- Proper usage of errors.Join with assignment
The logic flow is sound: try each configured key function, validate audiences if expected, return the first successful match, or aggregate all errors if none match.
188-205: LGTM! Well-designed helper function.The createKeyFunc helper effectively reduces code duplication and provides a clean interface for creating keyfunc.Keyfunc instances from HTTP client options. Good separation of concerns with proper error handling.
207-219: LGTM! Efficient audience intersection implementation.The hasAudience function uses an optimal set-based approach for checking audience intersection with O(1) lookup time. The implementation is correct and well-documented.
jensneuse
left a comment
jensneuse
left a comment
There was a problem hiding this comment.
One minor feedback. Please also check with @StarpTech
SkArchon
force-pushed
the
milinda/eng-7659-wun-q225-09-lack-of-audience-aud-validation-in-jwt
branch
from
5d07aa2 to
28931b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
router-tests/authentication_test.go (1)
2077-2505: Comprehensive audience validation test coverage with minor comment fixes needed.The test function provides excellent coverage of audience validation scenarios including:
- Failure cases with no audience match (both string and slice formats)
- Success cases with matching audiences
- Invalid audience format handling
- Validation bypass when no expected audience is configured
- Both HTTP-based and secret-based JWKS configurations
However, there are misleading comments that should be corrected:
- // Operations with a token should succeed + // Operations with a token should fail due to audience mismatchThis comment appears in multiple test cases where failure is expected (lines 2111, 2195, 2235, 2451). Please update all instances for clarity.
The test structure and coverage are excellent for validating the new audience validation feature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
router-tests/authentication_test.go(8 hunks)router/core/supervisor_instance.go(1 hunks)router/pkg/authentication/authentication.go(1 hunks)router/pkg/authentication/jwks_token_decoder.go(6 hunks)router/pkg/config/config.go(1 hunks)router/pkg/config/config.schema.json(1 hunks)router/pkg/config/testdata/config_full.json(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- router/pkg/authentication/authentication.go
🚧 Files skipped from review as they are similar to previous changes (5)
- router/core/supervisor_instance.go
- router/pkg/config/config.go
- router/pkg/config/testdata/config_full.json
- router/pkg/config/config.schema.json
- router/pkg/authentication/jwks_token_decoder.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: build-router
- GitHub Check: integration_test (./telemetry)
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./events)
- GitHub Check: build_test
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: build_test
🔇 Additional comments (7)
router-tests/authentication_test.go (7)
686-705: LGTM: Proper validation of duplicate JWKS URL detection.The test correctly validates that the system prevents startup when duplicate JWKS URLs are configured, which is an important safety feature mentioned in the PR objectives.
789-792: LGTM: Appropriate addition of audience claim for testing.The addition of the "aud" claim with a realistic URL value is appropriate for testing the new audience validation feature in multi-JWKS configurations.
834-854: LGTM: Proper validation of duplicate key ID detection.The test correctly validates that the system prevents startup when duplicate key IDs are manually specified in JWKS configurations, providing comprehensive duplicate detection coverage alongside the URL-based test.
869-869: LGTM: Backward compatible function call update.The addition of the
nilclaims parameter maintains backward compatibility while enabling the enhanced token generation functionality needed for audience validation testing.
912-912: LGTM: Consistent function signature update.The
nilclaims parameter is consistently applied across all existing generateToken calls, maintaining backward compatibility.
951-951: LGTM: Consistent parameter update.The function call is properly updated to match the new signature while preserving existing behavior.
2515-2524: LGTM: Well-designed function enhancement with backward compatibility.The updated
generateTokenfunction properly:
- Accepts optional claims parameter for flexibility
- Handles
nilclaims gracefully by initializing empty MapClaims- Uses
jwt.NewWithClaimsappropriately- Maintains all existing functionality for key ID and signing
This enhancement enables comprehensive audience validation testing while preserving backward compatibility.
SkArchon
deleted the
milinda/eng-7659-wun-q225-09-lack-of-audience-aud-validation-in-jwt
branch
4 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Warning
Potential Breaking Change: If you have the same KID manually specified (in the config) or are using the same URL, the router won't startup.
Checklist