refactor: pull signature and ttl validation out into separate file with it's own tests #804
Conversation
- add game-debug service to docker config - Created config for running tests with Nakama - Created config for running Test Game under Delve debugger - Created config for doing remote debugging into game-debug container - started new README-devs for info about world engine development
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
- sign_tests and nakama_test pass. - kms_test fail with an error "failed to find recovery id for KMS signature"
README-devs.md -> DEVELOPMENT.md reordered Cardinal test game Docker build steps for better cache utilization
- add game-debug service to docker config - Created config for running tests with Nakama - Created config for running Test Game under Delve debugger - Created config for doing remote debugging into game-debug container - started new README-devs for info about world engine development
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
README-devs.md -> DEVELOPMENT.md reordered Cardinal test game Docker build steps for better cache utilization
- clarified why we get websocket errors - use WORKDIR instead of cd in Dockerfile
- Modified Nakama to allow use of signed code directly from world engine project rather than exclusively through separate package. - removed nonce code from Nakama side - cardinal now errors on receipt of data because the nonce is a "0" value.
- switch to using local version of sign and rift for test code - replaced GetNonce() with GetCreated() in router - removed check for nonce in server tx handler - got rid of nonce from some test utilities
- Added freecache to project for in-mem caching of recently used message hashes - Added server option DisableReplayProtection to disable all hash checking - Added server option WithMessageExpiration to set allowable message lifetime - Added server option WithHashCacheSize to set size of in-mem hash cache - passing server settings into PostTransaction - updated sign.Transaction comment for clarity and exposed IsZeroHash for tx.go to call.
- removed unneeded option DisableReplayProtection - collected the hash verification settings into a single struct and pass that rather than individual variables.
- shifted rift to use grpc v1.63.2 - removed nonce params from server_test
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
- Replaced incorrect use of Nonce in router/iterator/iterator_test - regenerated protobuffers with new transaction struct - added comment about cacheRetentionExtraSeconds for clarity - fixed potential issue CodeRabbit identified in sign_test.go
- also fixed some Code Rabbit things
- also updated tags
- removed "hash" as a field to be sent in a transaction
also added make push-check to do all the pre-push checks together
- 2 uses of Unix() instead of UnixMilli() in iterator_test.go - updated comment in sign.go to refer to milliseconds
- work in progress - doesn't currently even compile
Your org has enabled the Graphite merge queue for merging into mainAdd the label “graphite/merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link. |
WalkthroughThe changes involve a significant refactoring of the transaction validation logic within the Changes
Assessment against linked issues
🪧 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
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #804 +/- ##
==========================================
+ Coverage 57.95% 58.35% +0.39%
==========================================
Files 143 144 +1
Lines 9188 9218 +30
==========================================
+ Hits 5325 5379 +54
+ Misses 3414 3397 -17
+ Partials 449 442 -7
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (9)
cardinal/server/handler/tx.go(6 hunks)cardinal/server/option.go(3 hunks)cardinal/server/server.go(4 hunks)cardinal/server/types/provider.go(1 hunks)cardinal/server/validator/validator.go(1 hunks)cardinal/server/validator/validator_test.go(1 hunks)cardinal/world_persona.go(1 hunks)sign/sign.go(1 hunks)sign/sign_test.go(2 hunks)
🧰 Additional context used
📓 Learnings (3)
cardinal/server/handler/tx.go (1)
Learnt from: ezavada
PR: Argus-Labs/world-engine#801
File: cardinal/server/handler/tx.go:59-59
Timestamp: 2024-10-21T15:25:22.783Z
Learning: In the file `cardinal/server/handler/tx.go`, functions like `PostTransaction` can remain as is even if they have high cognitive complexity, provided they are well-commented and necessary.
sign/sign.go (1)
Learnt from: ezavada
PR: Argus-Labs/world-engine#801
File: sign/sign.go:51-53
Timestamp: 2024-10-25T15:24:10.401Z
Learning: In `sign/sign.go`, the `TimestampAt` function is deliberately abstracting the value of `t.UnixMicro()`.
sign/sign_test.go (1)
Learnt from: ezavada
PR: Argus-Labs/world-engine#801
File: sign/sign_test.go:22-24
Timestamp: 2024-10-21T15:26:02.039Z
Learning: When tests in `sign/sign_test.go` already assert that a timestamp is within a range between a stored time and `time.Now().UnixMicro()`, suggesting to add additional tolerance may not be necessary.
🔇 Additional comments (19)
cardinal/server/types/provider.go (2)
6-6: LGTM: Clean import addition
The validator package import is properly added and aligns with the refactoring objectives.
12-12: Verify implementations of ProviderWorld interface
The embedding of validator.SignerAddressProvider is a breaking change that requires all implementations of ProviderWorld to implement the new interface.
Let's verify all implementations to ensure they're updated:
cardinal/server/option.go (2)
22-22: LGTM! Clean refactoring of configuration fields.
The migration of validation-related settings from verify to config struct is well-organized and consistent. This change aligns perfectly with the PR's objective of improving the validation code structure.
Also applies to: 36-36, 45-45
36-36: Verify clock synchronization handling.
The comment mentions a critical requirement about clock synchronization between sender and receiver. This could lead to issues in distributed environments.
Let's check if there are any related tests or validation:
✅ Verification successful
Let me gather more specific information about the message expiration implementation and validation.
Based on the search results, I can now provide a final response as there's clear evidence of clock synchronization handling in the codebase.
Clock synchronization is properly handled with timestamp validation
The implementation includes robust timestamp validation in cardinal/server/validator/validator.go:
- Messages are checked against both past and future time windows
- Expired messages (older than configured seconds) are rejected with
ErrMessageExpired - Future messages (more than allowed seconds ahead) are rejected with
ErrBadTimestamp - Tests verify this behavior with both expired and future timestamps
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for clock sync related tests and validation
echo "Searching for clock sync related tests..."
rg -l "clock|sync|time" --type go | grep "_test.go"
echo "Searching for time-related validation..."
ast-grep --pattern 'func $_($$) {
$$$
time.$_($$$)
$$$
}'
Length of output: 1369
Script:
#!/bin/bash
# Search for message expiration related code and validation
echo "Searching for message expiration implementation..."
rg -A 5 -B 5 "messageExpirationSeconds" --type go
echo "\nSearching for time validation in server package..."
rg -A 10 "time\." cardinal/server/
echo "\nSearching for message validation patterns..."
ast-grep --pattern 'if $_ := time.$_($$$); $$ {
$$$
}'
Length of output: 7982
cardinal/world_persona.go (1)
14-16: Verify integration with validation framework.
The method appears to be part of the SignerAddressProvider interface. Let's verify its usage in the validation framework.
✅ Verification successful
Let me gather more context about the validation framework's implementation to ensure proper integration.
Let me check one more aspect to verify the complete validation flow, particularly the GetSignerForPersonaTag method that's being called.
Based on the gathered information, I can now provide the final response:
Integration with validation framework is properly implemented ✓
The method is correctly integrated within the validation framework:
- Implements the
SignerAddressProviderinterface defined invalidator.go - Used appropriately in the validator's signature verification flow
- Properly delegates to
GetSignerForPersonaTagwith correct error handling - Test coverage exists through
ProviderFixturein validator tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration of GetSignerAddressForPersonaTag with the validation framework
# Check for interface definition and usage
echo "Checking interface definition and implementation..."
rg -p "SignerAddressProvider" --type go
# Check for method usage in validation logic
echo "Checking usage in validation..."
rg -p "GetSignerAddressForPersonaTag" --type go
Length of output: 1088
Script:
#!/bin/bash
# Check the validator implementation to understand the validation flow
echo "Checking validator implementation..."
rg -p -B 5 -A 10 "GetSignerAddressForPersonaTag" cardinal/server/validator/validator.go
# Check test implementation for validation scenarios
echo "Checking test implementation..."
rg -p -B 5 -A 5 "GetSignerAddressForPersonaTag" cardinal/server/validator/validator_test.go
# Check the interface implementation in World
echo "Checking World implementation..."
rg -p -B 5 -A 5 "GetSignerAddressForPersonaTag" cardinal/world_persona.go
Length of output: 3079
Script:
#!/bin/bash
# Check the implementation of GetSignerForPersonaTag
echo "Checking GetSignerForPersonaTag implementation..."
rg -p -B 5 -A 10 "GetSignerForPersonaTag" cardinal/world_persona.go
# Check for any validation-related tests
echo "Checking validation tests..."
rg -p -B 5 -A 10 "TestValidateSignature" cardinal/server/validator/
Length of output: 1411
cardinal/server/server.go (3)
17-17: LGTM: Import addition aligns with validation refactoring.
The addition of the validator package import supports the PR objective of extracting validation logic into a separate file.
39-41: LGTM: Server struct changes align with validation refactoring.
The replacement of the verify field with validator and addition of config field improves the structure and maintainability of the server.
187-187: LGTM: Route handler updated correctly.
The PostTransaction handler has been updated to use the new validator.
Let's verify that all transaction handlers have been updated to use the new validator:
✅ Verification successful
Let me gather more specific information about the transaction handlers and validator usage.
Route handler implementation is correctly updated
The verification confirms that:
- There is only one
PostTransactionhandler implementation incardinal/server/handler/tx.go - The handler correctly accepts and uses the new
SignatureValidatorparameter - The remaining references to "SignatureVerification" are in documentation and test files, which is expected
The implementation is consistent and complete.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to SignatureVerification
rg -l "SignatureVerification"
# Search for all transaction handlers to ensure they use SignatureValidator
ast-grep --pattern 'handler.Post$_Transaction'
Length of output: 247
Script:
#!/bin/bash
# Search for PostTransaction function definitions and their context
rg -A 5 "func.*PostTransaction"
# Search for validator usage in handler package
rg -l "validator" cardinal/server/handler/
# Check the specific implementation of transaction handlers
ast-grep --pattern 'func PostTransaction($$$) {
$$$
}'
Length of output: 612
sign/sign.go (1)
58-60: 🛠️ Refactor suggestion
Consider validating the timestamp parameter.
The function accepts any int64 value without validation. Consider adding checks for:
- Negative timestamps
- Timestamps too far in the future
- Zero timestamps
This would help prevent potential issues with invalid timestamps in the validation logic.
Let's check if there are any timestamp validation patterns in the codebase:
Example implementation:
func Timestamp(t int64) time.Time {
+ if t < 0 {
+ panic("negative timestamp")
+ }
return time.UnixMilli(t)
}sign/sign_test.go (2)
260-271: LGTM! Good error handling coverage.
The test effectively validates the handling of invalid JSON inputs and empty strings.
117-125: Consider making timestamp tests more resilient to system load.
The use of time.Sleep and strict timestamp comparisons could make these tests flaky under heavy system load. Consider:
- Using larger tolerance windows for timestamp comparisons
- Avoiding
time.Sleepby mocking time in tests
Let's check if these tests have been flaky in CI:
cardinal/server/handler/tx.go (8)
9-9: Importing the validator package is appropriate
The addition of the "pkg.world.dev/world-engine/cardinal/server/validator" import is necessary for the new validation logic.
38-38: Function signature updated appropriately to include the validator
Including validator *SignatureValidator in the PostTransaction function parameters aligns with the refactored validation logic and centralizes validation responsibilities.
48-48: Passing the validator to extractTx is appropriate
Passing validator to extractTx allows for conditional parsing based on the validation settings, ensuring that transaction extraction respects the validation configuration.
106-106: Updated function signature aligns with new validation logic
Adding validator *SignatureValidator to the PostGameTransaction function signature ensures consistency with the updated PostTransaction function and promotes uniform validation handling.
108-108: Delegating to PostTransaction improves code reuse
Returning PostTransaction(world, msgs, validator) simplifies the PostGameTransaction implementation and eliminates redundant code.
127-127: Updated function signature aligns with new validation logic
Including validator *SignatureValidator in the PostPersonaTransaction function signature maintains consistency across transaction handling functions.
129-129: Delegating to PostTransaction improves code reuse
By returning PostTransaction(world, msgs, validator), the PostPersonaTransaction function efficiently reuses existing logic.
Line range hint 137-143: Consider potential security implications of bypassing signature verification
When validator.IsDisabled is true, the code uses a generic body parser without extra checks. Ensure that this behavior is intentional and that bypassing signature verification does not introduce security vulnerabilities.
Run the following script to verify that disabling signature verification is properly controlled and does not affect security-sensitive endpoints:
✅ Verification successful
Let me gather more information about the signature validation implementation and its usage.
Based on the gathered information, I can now provide the final response:
Bypassing signature verification is intentionally controlled and properly isolated
The code's behavior is safe because:
- Signature verification is only disabled through explicit configuration (
WithDisableSignatureVerification()option) - The validator maintains proper separation between signature validation and message parsing
- Test coverage shows thorough validation of both enabled and disabled signature verification scenarios
- When disabled, it's typically used in controlled environments (tests) or specific development scenarios
Key security measures in place:
- Even with disabled signatures, basic message structure validation is still performed
- The feature is properly isolated and cannot be toggled at runtime
- The codebase includes comprehensive test cases verifying both secure and development paths
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any endpoints that might inadvertently accept unsigned transactions when validation is disabled.
# Test: Find all HTTP handlers that use `extractTx` and confirm they handle `validator.IsDisabled` appropriately.
rg -A 5 'func.*\(.*ctx \*fiber\.Ctx.*\) error' --glob '**/*.go'
Length of output: 2940
Script:
#!/bin/bash
# Search for IsDisabled configuration and its usage
rg -A 5 "IsDisabled" --glob '**/*.go'
# Search for signature validation related code
rg -A 10 "UnmarshalTransaction|sign\.Transaction" --glob '**/*.go'
# Search for any security-related comments or documentation
rg -i "security|signature|validation" --glob '**/*.{go,md}'
Length of output: 83726
|
@smsunarto @rmrt1n git is unfortunately show this PR as if it included all my changes from the WORLD-1190 PR. Scroll way down to ec1973a to see the actual changes for this PR. Sorry about that, I'll wait to merge main until the reviews are done in the future. |
|
Alternatively, you can look at server/handler/tx.go, server/validator/ folder, world_persona.go, and sign/sign.go All the changes were in those files (sign.go are comments and one new function for completeness of timestamp handling) |
There was a problem hiding this comment.
Actionable comments posted: 12
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (5)
cardinal/server/handler/tx.go(6 hunks)cardinal/server/validator/validator.go(1 hunks)cardinal/server/validator/validator_test.go(1 hunks)cardinal/world_persona.go(1 hunks)sign/sign.go(1 hunks)
🧰 Additional context used
📓 Learnings (3)
cardinal/server/handler/tx.go (1)
Learnt from: ezavada
PR: Argus-Labs/world-engine#801
File: cardinal/server/handler/tx.go:59-59
Timestamp: 2024-10-21T15:25:22.783Z
Learning: In the file `cardinal/server/handler/tx.go`, functions like `PostTransaction` can remain as is even if they have high cognitive complexity, provided they are well-commented and necessary.
cardinal/server/validator/validator.go (1)
Learnt from: ezavada
PR: Argus-Labs/world-engine#804
File: cardinal/server/validator/validator.go:174-176
Timestamp: 2024-11-01T14:41:04.393Z
Learning: In `validator.go`, ensure that the `ValidateTransactionTTL` function is restricted to validating the TTL of the message only. Do not include other validations, such as namespace validation, within this function.
sign/sign.go (1)
Learnt from: ezavada
PR: Argus-Labs/world-engine#801
File: sign/sign.go:51-53
Timestamp: 2024-10-25T15:24:10.401Z
Learning: In `sign/sign.go`, the `TimestampAt` function is deliberately abstracting the value of `t.UnixMicro()`.
🔇 Additional comments (9)
cardinal/world_persona.go (3)
14-18: Enhance method documentation.
While the inline comment indicates the interface implementation, it would be beneficial to add a proper documentation comment explaining:
- The purpose of this method
- The expected format/constraints of the personaTag parameter
- Possible error conditions
- Why it uses a fixed tick of 0
14-18: Verify integration with the new validator package.
The implementation looks correct, but let's verify how this method is used in the new validation framework.
✅ Verification successful
Based on the results, I can now generate the final response as we have sufficient information about the integration points.
Integration with validator package is correctly implemented
The method is properly integrated into the validation framework:
- Implements the
SignerAddressProviderinterface defined incardinal/server/validator/validator.go - Used correctly within the validator package for signature verification
- Has test coverage via
ProviderFixturein validator tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of GetSignerAddressForPersonaTag to understand integration
rg -A 3 "GetSignerAddressForPersonaTag"
# Search for implementations of SignerAddressProvider interface
ast-grep --pattern 'type $_ interface {
$$$
GetSignerAddressForPersonaTag($_) (string, error)
$$$
}'
Length of output: 1743
14-14:
Add import for the validator package.
The comment indicates this implements validator.SignerAddressProvider, but the validator package is not imported.
Add this import:
+import "pkg.world.dev/world-engine/cardinal/server/validator"Likely invalid or redundant comment.
sign/sign.go (1)
60-63: Implementation looks good!
The function correctly converts millisecond timestamps back to time.Time objects, providing a complete set of timestamp conversion utilities alongside TimestampNow and TimestampAt.
cardinal/server/validator/validator.go (1)
1-173: Well-structured and maintainable validation logic
The implementation of the SignatureValidator and its methods provides a clear and effective approach to transaction validation. The code is well-organized and aligns with the PR objectives to refactor the validation logic into a separate file with dedicated tests.
cardinal/server/validator/validator_test.go (1)
382-398: Kudos on preventing replay attacks in TestRejectsDuplicateTx
The test effectively ensures that duplicate transactions are rejected, which is crucial for security against replay attacks.
cardinal/server/handler/tx.go (3)
38-38: Consistent Function Signature Updates to Include *SignatureValidator
The functions PostTransaction, PostGameTransaction, and PostPersonaTransaction have been updated to accept a *SignatureValidator as a parameter. This change enhances modularity by centralizing validation logic within the validator package.
Also applies to: 106-108, 127-129
53-56: Effective Use of Centralized Validation Methods
The transaction expiration and signature are now validated using the centralized methods validator.ValidateTransactionTTL(tx) and validator.ValidateTransactionSignature(tx, signerAddress). This improves maintainability and ensures consistent validation across transactions.
Also applies to: 74-77
Line range hint 132-143: Conditional Transaction Parsing Based on Validation Configuration
The extractTx function now conditionally parses the transaction based on the validator.IsDisabled flag. This allows the system to toggle between strict and lenient parsing modes, providing flexibility for different operational scenarios.
|
Just rebase the PR |
Closes: WORLD-1237
Overview
Cleaned up the validation code and put it into a separately testable validator.go file. Also added a number of tests that don't depend on the overall server infrastructure to operate.
Summary by CodeRabbit
Release Notes
New Features
SignatureValidatorfor enhanced transaction validation, improving security and modularity.Improvements
Tests