🧹 chore: Add upper index limit for parsers #3503
Conversation
WalkthroughThe changes add stricter validation for form data slice indices during parsing, introducing an upper limit to prevent excessively large indices. Corresponding tests are added to verify that parsing fails with the appropriate error when the index exceeds this limit. Proxy middleware tests are refactored to use local test servers with IPv4 support and redirect handlers, replacing external URLs and improving test reliability. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant SchemaParser
Client->>Server: Send form data with nested slice index
Server->>SchemaParser: Parse form field path
SchemaParser->>SchemaParser: Validate index (negative or > maxParserIndex)
alt Index invalid
SchemaParser-->>Server: Return error (invalid path or index too large)
Server-->>Client: Respond with decode error
else Index valid
SchemaParser-->>Server: Return parsed path
Server-->>Client: Continue processing
end
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (20)
✨ Finishing Touches
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 (
|
gaby
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR introduces an upper limit for parsed indexes in the schema cache to prevent potential overflows and to distinguish between invalid paths and index overflow errors.
- Added a constant (maxParserIndex) and a new error (errIndexTooLarge) for index overflow handling in the cache.
- Enhanced index validation in the parsePath function.
- Updated tests in ctx_test.go to assert the new error messages.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/schema/cache.go | Introduced index upper limit with new error checks in parsePath. |
| ctx_test.go | Updated test cases to confirm distinct error messages for limits. |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ctx_test.go(1 hunks)internal/schema/cache.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
ctx_test.go (2)
utils/assertions.go (1)
AssertEqual(19-68)helpers.go (1)
MIMEApplicationForm(841-841)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Build (1.23.x, macos-latest)
- GitHub Check: Build (1.23.x, windows-latest)
- GitHub Check: Build (1.22.x, windows-latest)
- GitHub Check: Build (1.22.x, ubuntu-latest)
- GitHub Check: Build (1.21.x, macos-latest)
- GitHub Check: Build (1.21.x, ubuntu-latest)
- GitHub Check: Build (1.21.x, windows-latest)
- GitHub Check: Build (1.20.x, macos-latest)
- GitHub Check: Build (1.19.x, macos-latest)
- GitHub Check: Build (1.18.x, macos-latest)
- GitHub Check: Build (1.20.x, ubuntu-latest)
- GitHub Check: Build (1.20.x, windows-latest)
- GitHub Check: Build (1.19.x, windows-latest)
- GitHub Check: Compare
- GitHub Check: Build (1.18.x, ubuntu-latest)
- GitHub Check: Build (1.17.x, ubuntu-latest)
- GitHub Check: lint
- GitHub Check: Build (1.18.x, windows-latest)
- GitHub Check: Build (1.17.x, macos-latest)
- GitHub Check: Build (1.17.x, windows-latest)
🔇 Additional comments (4)
internal/schema/cache.go (2)
15-15: LGTM: Reasonable upper limit for parser indices.The constant value of 1000 provides a good balance between functionality and security, preventing potential DoS attacks from extremely large indices while allowing reasonable use cases.
17-20: Good error differentiation approach.The separate error variables properly distinguish between general invalid path errors and specific index overflow errors, improving debugging capabilities and error handling granularity as intended by the PR objectives.
ctx_test.go (2)
679-679: LGTM: Updated error message for negative index validationThe test correctly validates the new error message "failed to decode: schema: invalid path" for negative indices, which aligns with the PR's goal to distinguish between different types of validation errors.
681-690: LGTM: Comprehensive test coverage for upper index boundsExcellent addition of test coverage for the new upper index limit of 1000. The test properly:
- Sets up a form field with index 1001 that exceeds the limit
- Validates the specific error message "failed to decode: schema: index exceeds parser limit"
- Follows established testing patterns in the codebase
This ensures the new parser index limits are properly validated and will help prevent regressions.
Co-authored-by: Copilot <[email protected]>
gaby
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
ctx_test.go (2)
657-681: Good test structure with proper cleanup, but consider improving error validation.The test modifications look good overall:
- Proper use of
deferto restore parser configuration- Removal of
t.Parallel()is appropriate due to global state modifications- Testing negative index validation correctly
However, consider the past review feedback about using error comparison methods instead of string comparisons for more robust error validation.
683-706: Excellent addition of boundary test case.This new test function properly validates the upper limit boundary condition (index 1001 exceeding the 1000 limit). The test structure is consistent with the existing test and follows good testing practices.
Same suggestion as above regarding error validation: consider using error comparison methods instead of exact string matching for more robust error checking.
🧹 Nitpick comments (1)
middleware/proxy/proxy_test.go (1)
428-428: Consider consistent timeout usage across similar tests.Some tests now include explicit 2000ms timeouts while similar tests don't. Consider adding timeouts to all tests that make network calls for consistency and to prevent indefinite hangs in CI environments.
Also applies to: 448-448, 467-467, 487-487
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ctx_test.go(2 hunks)middleware/proxy/proxy_test.go(18 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Build (1.21.x, windows-latest)
- GitHub Check: Build (1.18.x, windows-latest)
- GitHub Check: Build (1.21.x, ubuntu-latest)
- GitHub Check: Build (1.22.x, windows-latest)
- GitHub Check: Build (1.23.x, ubuntu-latest)
- GitHub Check: Build (1.22.x, ubuntu-latest)
- GitHub Check: Build (1.18.x, ubuntu-latest)
- GitHub Check: Build (1.19.x, windows-latest)
- GitHub Check: Build (1.19.x, ubuntu-latest)
- GitHub Check: Build (1.17.x, macos-latest)
- GitHub Check: Compare
- GitHub Check: lint
🔇 Additional comments (5)
middleware/proxy/proxy_test.go (5)
27-41: Good refactoring for flexibility.The parameterization of network type and address makes this function more flexible and reusable. The logic is correct.
43-51: Well-designed convenience functions.These wrapper functions improve code readability and follow Go testing best practices with proper
t.Helper()usage.
53-73: Redirect server implementation is correct.The redirect logic properly handles the "/" to "/final" redirect pattern with the correct HTTP status code. The closure pattern for capturing the
addrvariable works correctly since it's captured by reference.
122-122: Excellent refactoring to improve test reliability.Replacing external URLs with local test servers eliminates external dependencies and makes tests more reliable and faster. The consistent use of
createProxyTestServerIPv4improves maintainability.Also applies to: 181-181, 217-217, 270-270, 296-296, 323-323, 347-347, 371-371, 398-398, 419-421, 441-441, 460-460, 480-480, 500-500, 522-522, 540-540, 562-562, 580-580, 657-657, 726-726
434-434: Redirect test assertions correctly updated.The expected response bodies and status codes properly match the behavior of the new local redirect server implementation.
Also applies to: 452-453, 471-471
Summary