refactor: improve edge case handling for recursion limits #22988
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several enhancements to the Cosmos SDK, focusing on improving functionality and error handling. Key changes include simulating nested messages, supporting hex key imports via standard input, and updating the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
![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 (3)
CHANGELOG.md (3)
Line range hint
1-1: Add title and description at the top of changelogThe changelog would benefit from having a clear title and brief description at the top explaining its purpose and how to read/interpret the entries.
+# Changelog + +All notable changes to the Cosmos SDK will be documented in this file. + +The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), +and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +
Line range hint
1-2000: Ensure consistent formatting of PR/Issue referencesThe changelog has inconsistent formatting of PR/Issue references - some use full URLs while others use just numbers. Consider standardizing on one format.
For example, standardize all references to use the short form
#1234rather than mixing with full URLs likehttps://github.com/cosmos/cosmos-sdk/pull/1234.
Line range hint
1-2000: Add release dates for all versionsSome version entries are missing their release dates. For historical tracking, all version headers should include their release date.
For example:
-## [v0.44.0] +## [v0.44.0] - 2021-01-08
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)codec/types/interface_registry.go(1 hunks)codec/unknownproto/unknown_fields.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
codec/types/interface_registry.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
codec/unknownproto/unknown_fields.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🔇 Additional comments (3)
codec/types/interface_registry.go (2)
280-282: LGTM: Enhanced validation for maxCalls
The change properly handles edge cases by rejecting both zero and negative values for the call counter, maintaining consistency with the maxDepth check.
277-279: LGTM: Improved validation for maxDepth
The change strengthens the validation by rejecting both zero and negative values for maxDepth, which is a security improvement.
Let's verify there are no direct manipulations of maxDepth that could set it to negative values:
✅ Verification successful
Validation change for maxDepth is safe and well-implemented
The verification shows that maxDepth is safely managed:
- Initialized with constant
MaxUnpackAnyRecursionDepth = 10 - Only decremented during recursion via
cloneForRecursion() - No direct assignments that could set negative values
- Used in a controlled manner within the
statefulUnpackerstruct
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any assignments to maxDepth
rg -p "maxDepth\s*=[^=]" --type go
Length of output: 36
Script:
#!/bin/bash
# Let's try a broader search for maxDepth usage and initialization
rg -p "maxDepth" --type go -C 2
# Also search for the struct field declaration
ast-grep --pattern 'type $_ struct {
$$$
maxDepth $_
$$$
}'
Length of output: 1326
Script:
#!/bin/bash
# Let's check the MaxUnpackAnyRecursionDepth constant value and usage
rg -p "MaxUnpackAnyRecursionDepth" --type go -C 2
# Also check if there are any other places where maxDepth is modified
ast-grep --pattern '$_.maxDepth = $_'
Length of output: 525
codec/unknownproto/unknown_fields.go (1)
57-59: LGTM: Consistent improvement in recursion limit validation
The change aligns with the modifications in interface_registry.go and properly handles negative recursion limits, enhancing security.
Let's verify there are no places where recursionLimit could be set to negative values:
✅ Verification successful
LGTM: Recursion limit validation is properly implemented
The codebase search confirms that recursionLimit is:
- Only used as a function parameter in message processing functions
- Properly decremented (
recursionLimit-1) when processing nested messages - Consistently checked with the new
<= 0condition - No direct assignments that could set it to negative values
The change is safe and improves the handling of invalid recursion limits.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for assignments to recursionLimit
rg -p "recursionLimit\s*=[^=]" --type go
Length of output: 42
Script:
#!/bin/bash
# Let's try a broader search for recursionLimit usage and declarations
rg -p "recursionLimit" --type go -C 2
# Also search for any function parameters or struct fields named recursionLimit
ast-grep --pattern 'struct {
$$$
recursionLimit $_
$$$
}'
ast-grep --pattern 'func $_($$ recursionLimit $_) $_'
Length of output: 2095
|
👍 Can you update https://github.com/cosmos/cosmos-sdk/blob/main/x/tx/decode/unknown.go#L50 as well? |
julienrbrt
added
the
backport/v0.52.x
haiyizxx
requested review from
kocubinski,
testinginprod and
aaronc
as code owners
haiyizxx
changed the title
updated, thanks |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
51-51: Fix spelling error in changelogChange "recusion" to "recursion" in the line describing recursion depth limits.
- * Fix [ABS-0043/ABS-0044](https://github.com/cosmos/cosmos-sdk/security/advisories/GHSA-8wcc-m6j2-qxvm) Limit recusion depth for unknown field detection and unpack any + * Fix [ABS-0043/ABS-0044](https://github.com/cosmos/cosmos-sdk/security/advisories/GHSA-8wcc-m6j2-qxvm) Limit recursion depth for unknown field detection and unpack any🧰 Tools
🪛 GitHub Actions: Spell Check
[warning] 51-51: Spelling error: 'recusion' should be 'recursion' or 'reclusion'
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🪛 GitHub Actions: Spell Check
CHANGELOG.md
[warning] 51-51: Spelling error: 'recusion' should be 'recursion' or 'reclusion'
Head branch was pushed to by a user without write access
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
CHANGELOG.md (2)
Line range hint
1-2000: Improve changelog formatting and organizationThe changelog has good content but could benefit from some improvements:
- Use consistent bullet point style - currently mixes
*and-- Add missing version comparison links at the bottom of each release section
- Consider grouping breaking changes by impact (client, API, state machine) more consistently across all versions
Example format for version links:
+ [v0.47.0]: https://github.com/cosmos/cosmos-sdk/compare/v0.46.0...v0.47.0 + [v0.46.0]: https://github.com/cosmos/cosmos-sdk/compare/v0.45.0...v0.46.0
Line range hint
1-50: Add summary section at the topConsider adding a high-level summary section at the top of the changelog to highlight the most important changes across versions. This helps users quickly understand major changes.
Example:
# Changelog All notable changes to the Cosmos SDK will be documented in this file. ## Summary This changelog covers all releases from v0.42.0 onwards. Major highlights include: - Migration to Protobuf for state serialization - IBC implementation - Stargate upgrade support - Multiple stability and performance improvements
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)x/tx/decode/unknown.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/tx/decode/unknown.go
🧰 Additional context used
📓 Path-based instructions (1)
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
Co-authored-by: Alex | Skip <alex@skip.money> (cherry picked from commit 93282e1) # Conflicts: # CHANGELOG.md # x/tx/decode/unknown.go
Description
Previously, checks for maxDepth and maxCalls.count only triggered when their values were strictly 0, which could lead to unintended behavior or bypassed validations if the surrounding code changes later on.
This PR updates the conditions to trigger for any non-positive values (<= 0), ensuring proper handling of edge cases even if validation was bypassed during one recursion step.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
client.tomland a helper method for broadcasting test transactions.Improvements
Bug Fixes
API Breaking Changes
x/paramsmodule and thetestutil/networkpackage.Invariantsand associated methods.