chore: update gosec #2933
chore: update gosec #2933
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several significant updates across multiple files, primarily focusing on enhancing security practices, improving error handling, and refining logging mechanisms. Key changes include updates to 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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2933 +/- ##
========================================
Coverage 64.47% 64.47%
========================================
Files 412 412
Lines 28976 28985 +9
========================================
+ Hits 18681 18687 +6
- Misses 9512 9515 +3
Partials 783 783
|
gartnera
force-pushed
the
update-gosec
branch
from
76d6879
to
2457031
Compare
gartnera
requested review from
a team,
brewmaster012,
kingpinXD,
lumtis,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
|
!!!WARNING!!! Be very careful about using Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203 Pay extra attention to the way |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (13)
.github/workflows/sast-linters.yml (2)
Line range hint
82-87: Consider using an array for excluded files.The current file exclusion logic could be made more maintainable by using an array of excluded files.
- # Skip this workflow file - if [ "$file" == ".github/workflows/sast-linters.yml" ] || [ "$file" == "changelog.md" ]; then - echo "Skipping nosec check for $file" - continue - fi + # Define excluded files + excluded_files=(".github/workflows/sast-linters.yml" "changelog.md") + if [[ " ${excluded_files[@]} " =~ " ${file} " ]]; then + echo "Skipping nosec check for excluded file: $file" + continue + fi
Line range hint
143-145: Enhance error reporting for #nosec validation.The regex pattern and error message could be more descriptive to help developers understand and fix issues.
- echo "$DIFF" | grep -P '#nosec(?!(\sG\d{3}))(?![^\s\t])([\s\t]*|$)' && echo "nosec without specified rule found!" && exit 1 || exit 0 + # Detect #nosec annotations that don't specify a rule (e.g., G401) + if echo "$DIFF" | grep -P '#nosec(?!(\sG\d{3}))(?![^\s\t])([\s\t]*|$)'; then + echo "ERROR: Found #nosec annotations without specific rule numbers." + echo "Please specify the exact rule(s) being suppressed (e.g., #nosec G401)." + exit 1 + fipkg/ticker/ticker.go (1)
186-186: Fix typo in function comment.There's a typo in the comment: "secodns" should be "seconds".
-// DurationFromUint64Seconds converts uint64 of secodns to time.Duration. +// DurationFromUint64Seconds converts uint64 of seconds to time.Duration.zetaclient/chains/ton/observer/inbound.go (1)
Line range hint
16-18: Enhance documentation for MaxTransactionsPerTickWhile the constant's purpose is clear, consider adding more detailed documentation explaining:
- The rationale behind the 100 transaction limit
- Performance implications
- What happens to remaining transactions
Consider updating the comment to:
-// MaxTransactionsPerTick is the maximum number of transactions to process on a ticker +// MaxTransactionsPerTick limits the number of transactions processed per tick to 100 +// to prevent processing delays and ensure consistent performance. Remaining transactions +// will be processed in subsequent ticks.pkg/contracts/ton/gateway.go (1)
127-127: Enhance documentation for security annotationsWhile the
#nosec G115annotations are correctly placed where type safety is guaranteed by the code structure, it would be beneficial to document the reasoning in more detail. This helps future maintainers understand why these cases are safe.Consider expanding the comments to explain the safety guarantees:
-// #nosec G115 always in range +// #nosec G115 -- Safe: op value is validated by the switch statement below which handles unknown casesAlso applies to: 142-142, 151-151
cmd/zetaclientd/p2p_diagnostics.go (2)
Line range hint
158-217: Enhance P2P communication robustness and maintainability.Several improvements can be made to the peer communication implementation:
- Define message buffer size as a constant:
+const maxMessageSize = 1024 // Maximum size of P2P diagnostic messages - buf := make([]byte, 1024) + buf := make([]byte, maxMessageSize)
- Use
deferfor stream closure consistently:stream, err := host.NewStream(context.Background(), peer.ID, protocol.ID(ProtocolID)) if err != nil { startLogger.Error().Err(err).Msgf("fail to create stream to peer %s", peer) continue } + defer func() { + if err := stream.Close(); err != nil { + startLogger.Warn().Err(err).Msgf("fail to close stream to peer %s", peer) + } + }()
- Define a structured message format:
type DiagnosticMessage struct { Round int `json:"round"` FromID string `json:"from_id"` ToID string `json:"to_id"` }These changes will improve code maintainability and reduce error handling complexity.
Line range hint
218-229: Consider implementing metrics for P2P diagnostics.The diagnostic results could be exposed as metrics to enable monitoring and alerting.
Consider adding the following metrics:
- Peer discovery success rate
- Ping-pong latency
- Number of active peers
- Message exchange success rate
This would enable better observability of the P2P network health.
precompiles/staking/staking.go (1)
Line range hint
441-447: Clean up disabled methods implementation.The disabled methods contain unreachable code after the return statement. While this code is currently commented with
//nolint:govet, it would be cleaner to either:
- Remove the unreachable code until the methods are re-enabled, or
- Comment out the return statement and keep the implementation if re-enabling is imminent.
Also, consider expanding the comment to include a brief reason for the disabling:
-// Disabled until further notice, check https://github.com/zeta-chain/node/issues/3005. +// These staking operations are temporarily disabled due to [brief reason]. +// Track re-enabling at https://github.com/zeta-chain/node/issues/3005.Also applies to: 476-482, 511-517
rpc/backend/node_info.go (1)
84-87: LGTM! Consider enhancing the security comment.The security comments addressing gosec G115 are appropriate since block heights in blockchain context are indeed always positive. However, consider making the comment more descriptive for future maintainers.
Consider enhancing the comment to:
-// #nosec G115 block height always positive +// #nosec G115 block height is guaranteed to be positive in blockchain contextrpc/types/utils.go (1)
136-153: LGTM! Consider adding documentation about value guarantees.The added security comments correctly suppress gosec G115 warnings for values that are guaranteed to be positive in the blockchain context. However, it would be beneficial to add a brief documentation comment explaining why these values are guaranteed to be positive, making it clearer for future maintainers.
Consider adding a documentation comment at the start of the
FormatBlockfunction:// FormatBlock creates an ethereum block from a tendermint header and ethereum-formatted // transactions. // Note: The following fields are guaranteed to be positive: // - block height: Blockchain heights start from 0 and only increment // - size: Block size is calculated from actual data // - gasLimit: Gas limits are configured as positive values // - timestamp: Block timestamps are Unix timestamps (seconds since epoch)zetaclient/chains/bitcoin/observer/inbound.go (1)
253-254: LGTM! Consider adding a clarifying comment.The gosec suppression is justified as the block height in Bitcoin is guaranteed to be positive by the protocol design. Consider adding a brief comment explaining this invariant for future maintainers.
- // #nosec G115 block height always positive + // #nosec G115 Bitcoin block height is guaranteed to be positive by protocol design if !ob.IsBlockConfirmed(uint64(blockVb.Height)) {zetaclient/chains/evm/observer/outbound.go (1)
Line range hint
1-453: Consider architectural improvementsSeveral architectural improvements could enhance the codebase:
- Move configuration values (timeouts, retry limits) to a central configuration
- Add comprehensive unit tests for error conditions
- Consider implementing circuit breakers for external calls
- Add metrics collection for monitoring outbound transaction processing
zetaclient/chains/evm/observer/inbound.go (1)
41-41: LGTM! Consider adding a comment about ticker duration.The change to
DurationFromUint64Secondsimproves clarity. Consider adding a brief comment explaining the expected duration range for better maintainability.+// Convert InboundTicker to duration, expected range: 1s to 1h interval := ticker.DurationFromUint64Seconds(ob.ChainParams().InboundTicker)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (22)
- .github/workflows/sast-linters.yml (1 hunks)
- cmd/zetaclientd/p2p_diagnostics.go (1 hunks)
- e2e/e2etests/test_bitcoin_std_deposit.go (1 hunks)
- e2e/runner/setup_solana.go (1 hunks)
- pkg/contracts/ton/gateway.go (1 hunks)
- pkg/crypto/aes256_gcm.go (1 hunks)
- pkg/memo/codec_compact.go (1 hunks)
- pkg/ticker/ticker.go (1 hunks)
- precompiles/logs/logs.go (1 hunks)
- precompiles/staking/staking.go (1 hunks)
- rpc/backend/node_info.go (1 hunks)
- rpc/backend/tx_info.go (1 hunks)
- rpc/namespaces/ethereum/debug/api.go (8 hunks)
- rpc/types/utils.go (1 hunks)
- scripts/gosec.sh (1 hunks)
- zetaclient/chains/bitcoin/observer/inbound.go (1 hunks)
- zetaclient/chains/evm/observer/inbound.go (2 hunks)
- zetaclient/chains/evm/observer/outbound.go (1 hunks)
- zetaclient/chains/ton/observer/inbound.go (2 hunks)
- zetaclient/orchestrator/orchestrator.go (1 hunks)
- zetaclient/types/dynamic_ticker.go (2 hunks)
- zetaclient/zetacore/client_worker.go (1 hunks)
✅ Files skipped from review due to trivial changes (7)
- e2e/runner/setup_solana.go
- pkg/crypto/aes256_gcm.go
- pkg/memo/codec_compact.go
- precompiles/logs/logs.go
- rpc/backend/tx_info.go
- scripts/gosec.sh
- zetaclient/zetacore/client_worker.go
🧰 Additional context used
📓 Path-based instructions (14)
cmd/zetaclientd/p2p_diagnostics.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_bitcoin_std_deposit.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.pkg/contracts/ton/gateway.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.pkg/ticker/ticker.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.precompiles/staking/staking.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.rpc/backend/node_info.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.rpc/namespaces/ethereum/debug/api.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.rpc/types/utils.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/bitcoin/observer/inbound.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/evm/observer/inbound.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/evm/observer/outbound.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/ton/observer/inbound.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/orchestrator/orchestrator.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/types/dynamic_ticker.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
📓 Learnings (1)
zetaclient/chains/bitcoin/observer/inbound.go (4)
Learnt from: ws4charlie PR: zeta-chain/node#2899 File: zetaclient/chains/bitcoin/observer/inbound.go:131-132 Timestamp: 2024-10-08T15:34:47.578Z Learning: ObserveInbound coverage will be improved in future refactor.Learnt from: ws4charlie PR: zeta-chain/node#2899 File: zetaclient/chains/bitcoin/observer/inbound.go:131-132 Timestamp: 2024-10-08T15:34:48.217Z Learning: ObserveInbound coverage will be improved in future refactor.Learnt from: ws4charlie PR: zeta-chain/node#2899 File: zetaclient/chains/bitcoin/observer/inbound.go:131-132 Timestamp: 2024-10-08T15:34:48.217Z Learning: ObserveInbound coverage will be improved in future refactor.Learnt from: ws4charlie PR: zeta-chain/node#2899 File: zetaclient/chains/bitcoin/observer/inbound.go:131-132 Timestamp: 2024-09-19T18:25:57.534Z Learning: ObserveInbound coverage will be improved in future refactor.
🔇 Additional comments (10)
e2e/e2etests/test_bitcoin_std_deposit.go (1)
62-63: LGTM! The added assertions enhance input validation.The positive check and gosec suppression are appropriate here. The
#nosec G115comment is justified as we've verified the amount is positive before the comparison..github/workflows/sast-linters.yml (1)
28-28: LGTM: Gosec version update aligns with PR objectives.The update to
v2.21.4-zeta2maintains the use of the forked version while incorporating upstream changes, which should help reduce false positives/negatives in G115 rule as intended.pkg/ticker/ticker.go (1)
186-189: Function rename improves clarity and maintainability.The renaming from
SecondsFromUint64toDurationFromUint64Secondsand parameter fromdtosecondsenhances code readability by clearly indicating both the input unit and return type.zetaclient/chains/ton/observer/inbound.go (2)
34-34: LGTM: Improved type safety in interval calculationThe change from
SecondsFromUint64toDurationFromUint64Secondsenhances type safety by making the duration conversion explicit.
Line range hint
115-115: Critical TODOs require attentionTwo important architectural changes are pending:
- Compliance check implementation (TON: Add compliance checks #2916)
- Protocol contract v2 upgrade (Support TON with Protocol contract v2 architecture #2967)
Both changes are critical for security and protocol functionality. Consider prioritizing these tasks.
Let's verify the status of these issues:
Would you like assistance in implementing either of these features? I can help create detailed implementation plans or generate code snippets.
Also applies to: 200-200
✅ Verification successful
TODOs remain relevant - both issues are still open
Both architectural changes mentioned in the TODOs are still pending implementation:
- Issue TON: Add compliance checks #2916 "TON: Add compliance checks" is open
- Issue Support TON with Protocol contract v2 architecture #2967 "Support TON with Protocol contract v2 architecture" is open
The review comment is accurate in highlighting these critical tasks. The TODOs serve as important reminders for pending security and protocol enhancements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if these issues are still open and their current status gh issue view 2916 --json title,state,labels gh issue view 2967 --json title,state,labelsLength of output: 540
pkg/contracts/ton/gateway.go (1)
Line range hint
142-146: Well-structured error handling with clear contextThe error handling has been implemented following best practices:
- Specific error types for different failure cases
- Proper error wrapping with context
- Clear and actionable error messages
precompiles/staking/staking.go (1)
136-138: Verify the range of validator Status values.While the comment indicates that the enum is always in range for uint8, it would be beneficial to add explicit validation to prevent potential overflow issues.
Let's verify the possible values of validator Status:
rpc/namespaces/ethereum/debug/api.go (1)
312-312:⚠️ Potential issueAdd input validation for block numbers.
The block retrieval methods accept any uint64 block number without validation. While the conversion to int64 is safe, there should be checks for:
- Genesis block (number 0) which might require special handling
- Future blocks that haven't been created yet
Let's verify the maximum block height in the codebase:
Consider adding validation:
func (a *API) GetHeaderRlp(number uint64) (hexutil.Bytes, error) { + if number == 0 { + return nil, errors.New("genesis block not supported") + } + latestHeight, err := a.backend.LatestBlockNumber() + if err != nil { + return nil, err + } + if number > uint64(latestHeight) { + return nil, fmt.Errorf("block #%d not found", number) + } // ... rest of the implementation }Apply similar validation to GetBlockRlp, PrintBlock, and SeedHash methods.
Also applies to: 323-323, 334-334, 345-345
zetaclient/orchestrator/orchestrator.go (1)
621-621: Security comment update looks good.The addition of G115 to the security comment is consistent with the PR objectives to continue using the forked gosec version. Both G115 (array length calculations) and G701 (type conversions) rules are being suppressed for this uint64 conversion, which is safe in this context as:
- OutboundScheduleInterval is a positive value controlled by chain parameters
- The conversion is used only for scheduling logic, not for array indexing or memory allocation
Let's verify the safety of OutboundScheduleInterval:
✅ Verification successful
Security comment suppression is appropriate and safe
The suppression of G115 and G701 for the uint64 conversion is justified because:
- The conversion is used only for scheduling interval calculation, not for array indexing or memory allocation
- The value is obtained from chain parameters and used in a controlled context for Solana outbound transaction scheduling
- The conversion appears in a well-defined scope with proper type checking (
solObserver, ok := observer.(*solanaobserver.Observer))🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that OutboundScheduleInterval is always positive and within safe bounds # Test: Search for OutboundScheduleInterval usage in chain parameters rg -A 5 "OutboundScheduleInterval.*int"Length of output: 2038
Script:
#!/bin/bash # Let's verify the initialization and validation of OutboundScheduleInterval # Check for validation rules and default values rg -A 5 "DefaultOutboundScheduleInterval" # Check for parameter validation ast-grep --pattern 'func (p $_) Validate() error { $$$ }'Length of output: 3016
Script:
#!/bin/bash # Let's check the actual usage context of OutboundScheduleInterval in the orchestrator # Check the code around the security comment rg -B 10 -A 5 "#nosec G115 G701 positive" zetaclient/orchestrator/orchestrator.go # Check for any validation or constraints on chain parameters ast-grep --pattern 'func ($_ *ChainParams) Validate() error { $$$ }'Length of output: 685
zetaclient/chains/evm/observer/inbound.go (1)
73-74: LGTM! Consistent duration handling.The change maintains consistency with the earlier duration conversion and properly updates the ticker interval.
reading https://sum.golang.org/lookup/github.com/zeta-chain/[email protected]: 500 Internal Server Error
Description
All my changes were merged upstream. Continue using our fork to avoid docker hub rate limiting. We really should just use the gosec plugin on golangci-lint.
Main impact is a continued reduction in G115 false-negatives and false-positives. We will need to review the new errors before merge.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores