fix(router): check persisted ops hashes to be well-formed #2078
Conversation
ysmolski
requested review from
Noroth,
StarpTech,
devsergiy and
jensneuse
as code owners
|
""" WalkthroughThis change replaces hardcoded persisted query hash strings with variables across multiple test files for consistency. It introduces a new test verifying server responses to invalid persisted query hashes. The core operation processor now validates persisted query hashes as well-formed SHA256 strings, returning HTTP 400 errors for invalid hashes. Error messages for malformed requests were refined in tests, and minor comment clarifications were made in the prehandler. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15–20 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 (12)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (11)
⏰ 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)
✨ 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 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: 0
🧹 Nitpick comments (1)
router/core/operation_processor.go (1)
195-212: Consider using standard library for hex validation.The validation logic is correct, but you could leverage Go's standard library for a more idiomatic approach:
-// isWellFormed verifies if Sha256Hash is valid and well-formed. -// Empty string is a valid value. -func (pq *GraphQLRequestExtensionsPersistedQuery) isWellFormed() bool { - if len(pq.Sha256Hash) == 0 { - return true - } - if len(pq.Sha256Hash) != 64 { - return false - } - // Iterating over bytes of the string - for i := 0; i < len(pq.Sha256Hash); i++ { - b := pq.Sha256Hash[i] - if !(b >= '0' && b <= '9' || b >= 'a' && b <= 'f' || b >= 'A' && b <= 'F') { - return false - } - } - return true -} +// isWellFormed verifies if Sha256Hash is valid and well-formed. +// Empty string is a valid value. +func (pq *GraphQLRequestExtensionsPersistedQuery) isWellFormed() bool { + if len(pq.Sha256Hash) == 0 { + return true + } + if len(pq.Sha256Hash) != 64 { + return false + } + _, err := hex.DecodeString(pq.Sha256Hash) + return err == nil +}The current implementation is perfectly functional, but
hex.DecodeStringprovides the same validation with less code and is more explicit about the intent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
router-tests/automatic_persisted_queries_test.go(2 hunks)router-tests/header_propagation_test.go(1 hunks)router-tests/persisted_operations_over_get_test.go(2 hunks)router-tests/persisted_operations_test.go(17 hunks)router-tests/telemetry/telemetry_test.go(2 hunks)router/core/graphql_prehandler.go(1 hunks)router/core/operation_processor.go(2 hunks)
🧠 Learnings (1)
📓 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.
🧰 Additional context used
🧠 Learnings (1)
📓 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.
⏰ 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: build_push_image (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_test
🔇 Additional comments (14)
router/core/operation_processor.go (1)
330-335: LGTM! Well-integrated validation logic.The validation is properly placed in the request processing pipeline with appropriate error handling and HTTP status codes. Early validation prevents unnecessary processing of malformed requests.
router-tests/persisted_operations_over_get_test.go (2)
26-26: LGTM! Improved test maintainability.Replacing the hardcoded hash string with a named variable improves readability and maintainability.
102-102: LGTM! Consistent improvement.Using the same named variable across test cases ensures consistency and maintainability.
router/core/graphql_prehandler.go (1)
480-480: LGTM! Minor documentation improvement.Adding proper punctuation improves the professionalism and readability of the code comments.
router-tests/automatic_persisted_queries_test.go (2)
201-201: Consistent with the refactoring pattern.This change aligns with the similar update on line 38 and maintains consistency across test cases.
38-38: Confirmed:cacheHashNotStoredis defined and accessibleThe constant
cacheHashNotStoredis declared inrouter-tests/persisted_operations_test.go(sameintegrationpackage), so the refactoring is valid.No further action required; approving these changes.
router-tests/header_propagation_test.go (1)
51-51: Excellent consistency improvement.This change aligns with the broader refactoring effort to centralize persisted query hash values across test files, improving maintainability while preserving the test's original intent to verify PERSISTED_QUERY_NOT_FOUND error handling.
router-tests/telemetry/telemetry_test.go (2)
4042-4042: Good refactoring for maintainability.Extracting the persisted query hash into a variable improves test maintainability and consistency. The hash value is a valid 64-character SHA256 hex string.
4047-4047: Consistent usage of the hash variable.The variable is properly reused in both GraphQL requests within the same test, ensuring consistency between the two operations being tested.
Also applies to: 4075-4075
router-tests/persisted_operations_test.go (5)
19-22: Well-implemented constant for test consistency.Good practice to centralize the non-existent hash value with clear documentation. The 64-character hex string format correctly represents a SHA256 hash.
29-29: Good use of the centralized constant.This change improves maintainability by replacing the hardcoded hash with the well-named constant.
36-67: Comprehensive test coverage for hash validation.Excellent test implementation that thoroughly covers various invalid hash scenarios including:
- Non-hex characters and path traversal attempts (security)
- Length validation (too short/long)
- Single character corruption cases
The test properly validates the expected HTTP 400 response and error message, aligning perfectly with the PR objective to validate persistent query hashes.
481-481: Good refactoring to centralize hash values.The introduction of the
nestedVariableExtractionvariable and its consistent usage improves test maintainability by eliminating duplicate hardcoded hash strings.Also applies to: 486-486, 495-495, 504-504
518-518: Systematic refactoring improves test maintainability.Excellent systematic approach to replacing hardcoded hash strings with meaningful variables across multiple test functions. This refactoring:
- Centralizes hash values within each test function
- Uses descriptive variable names that reflect the test context
- Maintains consistent SHA256 hash format (64 hex characters)
- Eliminates code duplication and improves maintainability
Also applies to: 523-523, 532-532, 541-541, 550-550, 559-559, 568-568, 582-582, 587-587, 596-596, 605-605, 613-613, 616-616, 625-625, 634-634, 643-643, 651-651, 655-655, 664-664
ysmolski
force-pushed
the
yury/eng-7655-wun-q225-07-arbitrary-json-file-access-through-persistent
branch
3 times, most recently
from
2ebd12e to
0904223
Compare
ysmolski
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
It is ready for review. Please take a look. |
ysmolski
force-pushed
the
yury/eng-7655-wun-q225-07-arbitrary-json-file-access-through-persistent
branch
2 times, most recently
from
e3fe79a to
043764a
Compare
ysmolski
force-pushed
the
yury/eng-7655-wun-q225-07-arbitrary-json-file-access-through-persistent
branch
from
043764a to
2ca479c
Compare
ysmolski
force-pushed
the
yury/eng-7655-wun-q225-07-arbitrary-json-file-access-through-persistent
branch
from
2ca479c to
b0eac7c
Compare
ysmolski
force-pushed
the
yury/eng-7655-wun-q225-07-arbitrary-json-file-access-through-persistent
branch
from
078054b to
7e18bf1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
router-tests/graphql_over_get_test.go(1 hunks)router-tests/graphql_over_http_test.go(6 hunks)router/core/operation_processor.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- router-tests/graphql_over_http_test.go
- router/core/operation_processor.go
🧰 Additional context used
🧠 Learnings (2)
📓 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/graphql_over_get_test.go (1)
Learnt from: Noroth
PR: #2088
File: demo/pkg/subgraphs/projects/src/main_test.go:0-0
Timestamp: 2025-07-29T08:19:55.720Z
Learning: In Go testing, t.Fatal, t.FailNow, t.Skip* and similar methods cannot be called from goroutines other than the main test goroutine, as they will cause a runtime panic. Use channels, t.Error*, or other synchronization mechanisms to communicate errors from goroutines back to the main test thread.
⏰ 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: integration_test (./telemetry)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
router-tests/graphql_over_get_test.go (3)
49-61: LGTM! Well-structured test for invalid JSON variables.The test correctly validates error handling for malformed JSON in the variables field, using a realistic error case (unquoted string value) and asserting the appropriate HTTP status code and error message.
49-91: Excellent addition of comprehensive error handling tests.These four new test cases provide thorough coverage of JSON parsing and unmarshaling errors for GraphQL GET requests. The tests are well-structured, follow established patterns, and include specific error message assertions that will help developers debug issues. This aligns well with the PR's objective of improving request validation and error handling.
78-91: cacheHashNotStored reference is valid
ThecacheHashNotStoredconstant is declared in router-tests/persisted_operations_test.go (also in theintegrationpackage), so the reference in graphql_over_get_test.go resolves correctly.Good coverage for the boolean‐version mismatch scenario.
ysmolski
force-pushed
the
yury/eng-7655-wun-q225-07-arbitrary-json-file-access-through-persistent
branch
2 times, most recently
from
dc69ef6 to
250da13
Compare
The OperationKit.unmarshalOperation method checks if non-empty sha256 hash in GraphQLRequestExtensions.PersistedQuery.Sha256Hash is valid. It rejects hex strings that could not be sha256 hash and raises en error that it cannot parse the body. I have decided to do strict validation for SHA-256 hashes. I kept the old behvaiour of not accepting an empty hash value as valid. Tests now deal only with valid hash values.
ysmolski
force-pushed
the
yury/eng-7655-wun-q225-07-arbitrary-json-file-access-through-persistent
branch
from
250da13 to
1afc3a5
Compare
ysmolski
deleted the
yury/eng-7655-wun-q225-07-arbitrary-json-file-access-through-persistent
branch
The OperationKit.unmarshalOperation method checks if non-empty sha256 hash in GraphQLRequestExtensions.PersistedQuery.Sha256Hash is valid.
It rejects hex strings that could not be sha256 hash and raises en error that it cannot parse the body.
I have decided to do strict validation for SHA-256 hashes.
I kept the old behvaiour of not accepting an empty hash value as valid.
Tests now deal only with valid hash values.
I have changed how we handle errors returned by
operationKit.UnmarshalOperation*methods.Now the client will see the error returned by the "UnmarshalOperationFromBody".
I added couple of tested for "UnmarshalOperationFromURL".
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Style