refactor: separate validator from rest of server code #806
Conversation
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 in this pull request primarily focus on refactoring the transaction validation logic within the server's handling of transactions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Outside diff range comments (4)
cardinal/server/option.go (1)
Line range hint
1-45: Consider adding configuration documentationThe consolidation of configuration fields is a good architectural improvement. Consider adding a documented
Configstruct type to make the configuration structure more explicit and self-documenting.Example addition:
// Config holds all server configuration options type Config struct { port string isSwaggerDisabled bool isSignatureValidationDisabled bool messageExpirationSeconds int messageHashCacheSizeKB int // ... other config fields }cardinal/server/server.go (1)
Line range hint
161-161: Replaces.verifywiths.validatorinhandler.PostTransactionThe
s.verifyfield no longer exists in theServerstruct after the refactoring. It should be updated tos.validatorto reflect the changes and ensure the transaction handler uses the correct validator.Apply this diff to fix the issue:
-tx.Post("/:group/:name", handler.PostTransaction(world, msgIndex, s.verify)) +tx.Post("/:group/:name", handler.PostTransaction(world, msgIndex, s.validator))cardinal/server/handler/tx.go (2)
Line range hint
174-182: Fix undefined variable 'verify' to 'validator' in conditional statementIn the
extractTxfunction, the variableverifyis not defined in this scope. It appears you intended to usevalidatorinstead ofverify.Apply the following changes to correct the variable name and references:
- if !verify.IsDisabled { + if !validator.IsDisabled { // we are doing signature verification, so use sign's Unmarshal which does extra checks tx, err = sign.UnmarshalTransaction(ctx.Body()) } else { // we aren't doing signature verification, so just use the generic body parser which is more forgiving tx = new(sign.Transaction) err = ctx.BodyParser(tx) } if err != nil { log.Errorf("body parse failed: %v", err) return nil, fiber.NewError(fiber.StatusBadRequest, "Bad Request - unparseable body") } - if !verify.IsDisabled { + if !validator.IsDisabled { txEarliestValidTimestamp := sign.TimestampAt( - time.Now().Add(-(time.Duration(verify.MessageExpirationSeconds) * time.Second))) + time.Now().Add(-(time.Duration(validator.MessageExpirationSeconds) * time.Second)))🧰 Tools
🪛 GitHub Check: Lint (go)
[failure] 9-9:
"github.com/ethereum/go-ethereum/common" imported and not used
Line range hint
184-192: Remove obsolete cache logic and undefined function 'isHashInCache'The cache-related logic for replay protection has been removed, but the code still references
isHashInCacheandvalidator.Cache. SinceisHashInCacheis undefined and the cache logic is no longer utilized, these lines should be removed to prevent compilation errors.Apply the following diff to eliminate the obsolete cache check:
// before we even create the hash or validate the signature, check to see if the message has expired if tx.Timestamp < txEarliestValidTimestamp { log.Errorf("message older than %d seconds. Got timestamp: %d, current timestamp: %d ", - verify.MessageExpirationSeconds, tx.Timestamp, sign.TimestampNow()) + validator.MessageExpirationSeconds, tx.Timestamp, sign.TimestampNow()) return nil, fiber.NewError(fiber.StatusRequestTimeout, "Request Timeout - signature too old") } - // check for duplicate message via hash cache - if found, err := isHashInCache(tx.Hash, validator.Cache); err != nil { - log.Errorf("unexpected cache error %v. message %s ignored", err, tx.Hash.String()) - return nil, fiber.NewError(fiber.StatusInternalServerError, "Internal Server Error - cache failed") - } else if found { - // if found in the cache, the message hash has already been used, so reject it - log.Errorf("message %s already handled", tx.Hash.String()) - return nil, fiber.NewError(fiber.StatusForbidden, "Forbidden - duplicate message") - } // at this point we know that the generated hash is not in the cache, so this message is not a replay }🧰 Tools
🪛 GitHub Check: Lint (go)
[failure] 9-9:
"github.com/ethereum/go-ethereum/common" imported and not used
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (9)
cardinal/server/handler/tx.go(7 hunks)cardinal/server/option.go(3 hunks)cardinal/server/server.go(3 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(1 hunks)
🧰 Additional context used
📓 Learnings (2)
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_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 (17)
cardinal/server/types/provider.go (2)
6-6: LGTM: Clean import addition
The addition of the validator package import aligns with the PR's objective of separating validator code.
12-12: LGTM: Good use of interface embedding
The embedding of SignerAddressProvider follows Go best practices and promotes better separation of concerns. Let's verify the implementation completeness across the codebase.
✅ Verification successful
Based on the results, I can now generate the final response since I have found the implementations of GetSignerAddressForPersonaTag:
Verified: Interface implementation is complete and correct
The SignerAddressProvider interface is properly implemented in both required locations:
- Main implementation in
Worldstruct (cardinal/world_persona.go) which delegates toGetSignerForPersonaTag - Test implementation in
ProviderFixture(cardinal/server/validator/validator_test.go) for validation testing
Both implementations follow the correct method signature and provide the expected functionality for persona tag validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all implementations of ProviderWorld interface
# also implement SignerAddressProvider methods
# First, find the SignerAddressProvider interface definition
echo "Checking SignerAddressProvider interface definition:"
ast-grep --pattern 'type SignerAddressProvider interface {
$$$
}'
# Then, find all structs that implement ProviderWorld
echo -e "\nChecking ProviderWorld implementations:"
ast-grep --pattern 'type $_ struct {
$$$
}'
# Finally, search for GetSignerAddressForPersonaTag implementations
echo -e "\nChecking GetSignerAddressForPersonaTag implementations:"
ast-grep --pattern 'func ($_ $_) GetSignerAddressForPersonaTag($_) ($_, $_) {
$$$
}'
Length of output: 138062
cardinal/server/option.go (3)
22-22: LGTM! Field rename improves clarity
The change from verify.IsDisabled to config.isSignatureValidationDisabled is more explicit and aligns well with the refactoring goal of centralizing configuration.
45-45: Verify hash cache initialization in validator
The configuration move looks good and the renamed field better indicates its purpose. Since this affects replay protection, let's verify the cache initialization.
#!/bin/bash
# Description: Verify proper hash cache initialization and usage
# Expected: Find cache initialization code and size configuration usage
# Search for cache initialization
ast-grep --pattern 'type $_ struct {
$$$
cache $$$
$$$
}'
# Search for cache size configuration usage
rg -A 3 "messageHashCacheSizeKB" --type go36-36: Verify message expiration handling in validator package
The configuration move looks good, but let's verify that the new validator package correctly accesses this configuration for expiration checks.
✅ Verification successful
Let me gather more information about how the message expiration is used in validation.
Configuration change is properly handled in validator package
The verification shows that the message expiration configuration is correctly used in the validator package:
- The validator correctly receives and stores the expiration seconds in
MessageExpirationSecondsfield - The expiration check is properly implemented in validation logic using this configuration
- The cache retention also correctly uses this value
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the message expiration config is properly used in the validator
# Expected: Find references to messageExpirationSeconds in validator logic
# Search for message expiration usage in validator package
rg -l "messageExpirationSeconds" --type go
# Search for time-based validation logic in validator
ast-grep --pattern 'func $_($$) {
$$$
time.$$$
$$$
}'
Length of output: 1470
Script:
#!/bin/bash
# Search for validation logic related to message expiration
rg -A 5 "MessageExpirationSeconds" --type go
# Search for time-based validation in validator package
rg -A 5 "time\." cardinal/server/validator/
Length of output: 5839
cardinal/world_persona.go (2)
15-17: LGTM: Clean integration with existing code.
The implementation correctly leverages the existing GetSignerForPersonaTag method while maintaining proper error handling and preserving the original behavior.
14-18: Verify SignerAddressProvider implementation completeness.
Let's verify this is the only implementation of SignerAddressProvider and check its usage.
✅ Verification successful
Let me generate additional verification scripts to check the interface definition and its complete usage pattern.
Implementation verified and correctly follows interface contract
The verification confirms:
- The interface
SignerAddressProvideris properly defined incardinal/server/validator/validator.go - The
Worldstruct correctly implements the interface with matching method signature - There are two implementations:
- Main implementation in
Worldstruct - Test implementation in
ProviderFixturefor testing purposes
- Main implementation in
- The interface is used appropriately in the validator package for signature verification
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other SignerAddressProvider implementations
echo "Searching for SignerAddressProvider interface implementations..."
ast-grep --pattern 'type $_ struct {
$$$
}
func ($_ $_) GetSignerAddressForPersonaTag($_) ($_, $_) {
$$$
}'
# Search for method usage
echo "Searching for GetSignerAddressForPersonaTag usage..."
rg "GetSignerAddressForPersonaTag" -A 3
Length of output: 2003
Script:
#!/bin/bash
# Search for SignerAddressProvider interface definition
echo "Searching for SignerAddressProvider interface definition..."
ast-grep --pattern 'type SignerAddressProvider interface {
$$$
}'
# Search for signerAddressProvider field usage
echo "Searching for signerAddressProvider field usage..."
rg "signerAddressProvider" -A 2
# Search for any other potential implementations
echo "Searching for potential struct types implementing the interface..."
rg "SignerAddressProvider" --type go
Length of output: 1887
sign/sign.go (1)
50-64: Verify the usage of timestamp functions across the codebase.
Let's ensure all callers of these functions handle the timestamps correctly.
✅ Verification successful
Based on the search results, I can now provide a final response as the verification is complete:
Timestamp functions are used consistently and safely across the codebase
The verification shows:
- All timestamp functions are used correctly in their respective contexts
- Proper validation of timestamps is implemented in validators and handlers
- Consistent millisecond precision is maintained throughout
- No breaking changes detected as all callers handle timestamps appropriately
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of the timestamp functions to verify proper handling
echo "Searching for TimestampNow usage..."
rg "TimestampNow\(\)" -A 2
echo "Searching for TimestampAt usage..."
rg "TimestampAt\(" -A 2
echo "Searching for direct Timestamp usage..."
rg "Timestamp\(" -A 2
# Look for potential timestamp comparisons that might need validation
echo "Checking for timestamp comparisons..."
rg "UnixMilli|\.Timestamp" -A 2
Length of output: 17767
sign/sign_test.go (1)
112-116: LGTM: Basic timestamp validation is correct.
The test correctly verifies that transaction timestamps are generated within an acceptable range of the current time.
cardinal/server/server.go (6)
17-17: Importing the validator package
The addition of the validator package import is necessary for the implementation of the SignatureValidator and aligns with the refactoring objectives.
27-27: Define defaultHashCacheSizeKB constant appropriately
Setting defaultHashCacheSizeKB to 1024 KB (1MB) provides a reasonable default size for the hash cache.
31-35: Enhance configuration flexibility with additional fields
Adding new fields to the config struct allows for greater customization and better management of server settings such as port, Swagger availability, signature validation, message expiration, and hash cache size.
39-41: Include validator in the Server struct
Incorporating the validator field into the Server struct encapsulates the signature validation logic and improves the separation of concerns.
59-63: Initialize configuration with default values
Initializing config with default values ensures consistent server behavior when specific options are not provided.
70-77: Instantiate SignatureValidator with current configuration
Creating the SignatureValidator using the existing configuration parameters helps maintain consistency and centralizes signature validation logic.
cardinal/server/handler/tx.go (1)
Line range hint 157-170: Ensure consistent usage of 'validator' in 'extractTx' function
While you've updated the extractTx function to accept validator, ensure that all references within the function consistently use validator instead of any outdated variables.
Please review the function to confirm that all instances of the old variable (e.g., verify) have been replaced with validator.
cardinal/server/validator/validator_test.go (1)
103-398: Comprehensive test coverage
The test suite provides extensive coverage of the SignatureValidator functionality, testing various scenarios including signature verification enabled and disabled, invalid signatures, missing persona tags, altered transactions, and replay attacks. This thorough testing ensures the robustness and reliability of the validation logic.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #806 +/- ##
==========================================
+ Coverage 57.95% 58.23% +0.27%
==========================================
Files 143 144 +1
Lines 9188 9223 +35
==========================================
+ Hits 5325 5371 +46
+ Misses 3414 3407 -7
+ Partials 449 445 -4
|
ezavada
force-pushed
the
edz/world-1237-new
branch
from
6dbf173
to
5c9d0f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 12
📜 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(1 hunks)
🧰 Additional context used
📓 Learnings (2)
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_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 (22)
cardinal/server/types/provider.go (2)
6-6: LGTM: Import addition aligns with refactoring goals.
The new validator package import supports the separation of validation logic into its own package.
12-12: Verify all ProviderWorld implementations are updated.
The embedding of validator.SignerAddressProvider aligns well with the refactoring goals. However, this is a breaking change that requires all implementations of ProviderWorld to implement the SignerAddressProvider interface methods.
Let's verify all implementations are properly updated:
✅ Verification successful
Let me gather more information about the ProviderWorld interface and its implementations.
Let me search for the World struct implementation since it's likely the main implementer of ProviderWorld.
Based on the evidence gathered, I can now provide a final response:
Implementation is properly updated and consistent
The World struct correctly implements the SignerAddressProvider interface through its GetSignerAddressForPersonaTag method, which internally calls GetSignerForPersonaTag with appropriate tick handling. The implementation is consistent with the interface embedding in ProviderWorld.
Key observations:
GetSignerAddressForPersonaTagis implemented incardinal/world_persona.go- The implementation preserves existing behavior by using tick 0 for backward compatibility
- The method properly delegates to
GetSignerForPersonaTagwhich contains the core logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations of ProviderWorld to ensure they're updated
# with SignerAddressProvider methods
# Search for struct types that might implement ProviderWorld
ast-grep --pattern 'type $_ struct {
$$$
}'
# Search for potential interface implementation methods
rg -A 2 "func \(.*\) GetSignerAddressForPersonaTag"
Length of output: 136692
Script:
#!/bin/bash
# Find the ProviderWorld interface definition and its implementations
ast-grep --pattern 'type ProviderWorld interface {
$$$
}'
# Search for potential implementations
rg -A 5 'type \w+ struct.*{.*}.*\n.*implements.*ProviderWorld'
Length of output: 1619
Script:
#!/bin/bash
# Search for World struct implementation of GetSignerAddressForPersonaTag
rg -A 5 'func \(w \*World\) GetSignerAddressForPersonaTag'
# Search for World struct implementation of GetSignerForPersonaTag
rg -A 5 'func \(w \*World\) GetSignerForPersonaTag'
# Look for any other implementations
rg -A 5 'implements.*ProviderWorld'
Length of output: 1061
cardinal/server/option.go (2)
22-22: LGTM! Clear and consistent configuration field name.
The renamed configuration field isSignatureValidationDisabled is more explicit and follows Go naming conventions.
Line range hint 22-45: Verify complete migration of verification-related configurations.
The consolidation of configuration fields from verify to config looks good. Let's ensure all verification-related configurations have been migrated consistently across the codebase.
✅ Verification successful
Let me gather more specific information about the configuration usage.
Based on the results, I can now generate the final response since I have sufficient evidence about the configuration migration.
Configuration migration is properly implemented
The verification-related configurations have been correctly migrated to the config structure and are consistently used throughout the codebase:
- All configuration fields (
isSignatureValidationDisabled,messageExpirationSeconds,messageHashCacheSizeKB) are properly defined in the server's config structure - The configuration values are correctly passed to the
SignatureValidatorduring initialization - No lingering references to the old verify structure were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old verify structure
rg -l "verify\." --type go
# Search for all config field usages to ensure consistent naming
ast-grep --pattern 'config.$_'
Length of output: 164
Script:
#!/bin/bash
# Search for verify-related configuration in the codebase
rg -A 3 "isSignatureValidation|messageExpiration|hashCache" --type go
# Look for any Server struct definitions to understand the configuration structure
ast-grep --pattern 'type Server struct {
$$$
}'
# Search for any verify package imports
rg -l "\".*\/verify\"" --type go
Length of output: 2336
cardinal/world_persona.go (1)
14-18: LGTM with verification needed.
The implementation correctly delegates to GetSignerForPersonaTag while preserving existing behavior. However, let's verify the error handling aligns with the validator's expectations.
✅ Verification successful
Error handling in GetSignerAddressForPersonaTag is correctly implemented
The implementation aligns perfectly with the validator's expectations:
- The interface in
cardinal/server/validator/validator.gorequires(string, error)return values, which matches the implementation - The validator properly handles errors from the provider by wrapping them in a validation error with appropriate HTTP status code (401 Unauthorized)
- The test fixture demonstrates the expected error handling pattern, particularly for the "no signer" scenario
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify error handling alignment between World and SignatureValidator
# Test 1: Check how SignatureValidator handles errors from SignerAddressProvider
ast-grep --pattern 'type SignerAddressProvider interface {
$$$
GetSignerAddressForPersonaTag($$$) (string, error)
$$$
}'
# Test 2: Check error handling in SignatureValidator when using the provider
rg -A 5 'GetSignerAddressForPersonaTag' 'server/validator/'
Length of output: 244
Script:
#!/bin/bash
# Let's try searching in the entire codebase for the interface and its usage
# Search for interface definition
ast-grep --pattern 'type SignerAddressProvider interface {
$$$
}'
# Search for validator package and interface usage
rg -l "SignerAddressProvider"
# Search for any validation-related code that uses GetSignerAddressForPersonaTag
rg -A 5 "GetSignerAddressForPersonaTag"
Length of output: 2727
sign/sign.go (3)
50-53: Improve function documentation to follow Go standards.
The documentation should follow Go standards and clearly specify the timestamp format.
55-58: Improve function documentation to follow Go standards.
The documentation should follow Go standards and clearly specify the timestamp format.
60-64: Add input validation for secure timestamp handling.
Given that this function is used by the new validator package for transaction TTL checks, proper input validation is crucial for secure transaction validation.
Let's verify the validator's usage of this function:
#!/bin/bash
# Search for usage of Timestamp function in validator package
rg -A 5 "Timestamp\(" server/validator/sign/sign_test.go (1)
117-125: Consider improving test reliability by mocking time.
The test uses time.Sleep and makes millisecond-level timing assumptions which could lead to flaky tests, especially in CI environments with varying load conditions.
This was previously suggested in an earlier review. The recommendation to use a clock interface for deterministic testing still applies here.
cardinal/server/server.go (8)
17-17: Necessary import of the validator package added
The import statement correctly adds the validator package, which is required for the SignatureValidator implementation.
27-27: Default hash cache size constant defined appropriately
The defaultHashCacheSizeKB constant is set to 1024, accurately representing a default cache size of 1MB as per the comment.
31-35: Configuration struct updated with new fields for enhanced flexibility
The config struct now includes additional fields:
portisSwaggerDisabledisSignatureValidationDisabledmessageExpirationSecondsmessageHashCacheSizeKB
These fields enhance the server's configurability and allow for more granular control over its behavior.
39-41: Server struct updated with configuration and validator
The Server struct now includes:
configfield for server configurationvalidatorfield of type*validator.SignatureValidatorfor encapsulating validation logic
These additions improve the modularity and maintainability of the server code.
59-63: Default configuration values are set appropriately
The default values assigned to the config fields ensure sensible defaults:
port: defaults to"4040"isSwaggerDisabled:false(Swagger is enabled by default)isSignatureValidationDisabled:false(Signature validation is enabled by default)messageExpirationSeconds: set to10secondsmessageHashCacheSizeKB: set to1024KB (1MB)
70-77: SignatureValidator initialized correctly with configuration parameters
The SignatureValidator is instantiated using the updated configuration fields and the world object:
s.config.isSignatureValidationDisableds.config.messageExpirationSecondss.config.messageHashCacheSizeKBworld.Namespace()world(as a provider of signature addresses)
This setup centralizes the validation logic and simplifies transaction handling.
187-187: Transaction handler updated to use the new SignatureValidator
The PostTransaction handler now includes s.validator as a parameter, aligning with the refactored validation logic.
70-77:
Ensure proper error handling when initializing SignatureValidator
Currently, the NewSignatureValidator function is called without checking for errors. If NewSignatureValidator can return an error, it should be handled to prevent potential runtime issues.
Apply this diff to handle possible errors:
func New(
world servertypes.ProviderWorld,
components []types.ComponentMetadata,
messages []types.Message,
opts ...Option,
) (*Server, error) {
// Existing code...
// Initialize the SignatureValidator
- s.validator = validator.NewSignatureValidator(
+ validator, err := validator.NewSignatureValidator(
s.config.isSignatureValidationDisabled,
s.config.messageExpirationSeconds,
s.config.messageHashCacheSizeKB,
world.Namespace(),
world, // world is a provider of signature addresses
)
+ if err != nil {
+ return nil, eris.Wrap(err, "failed to create SignatureValidator")
+ }
+ s.validator = validator
// Existing code...
}Please ensure that NewSignatureValidator returns an error if initialization fails.
cardinal/server/handler/tx.go (3)
138-140: 🧹 Nitpick (assertive)
Fix typo in comment
There's a typo in the comment: "with" should be "which".
Apply this diff to correct the typo:
} else {
- // we aren't doing signature verification, so just use the generic body parser with is more forgiving
+ // we aren't doing signature verification, so just use the generic body parser which is more forgiving
tx = new(sign.Transaction)Likely invalid or redundant comment.
5-5:
Import the standard 'log' package instead of 'github.com/gofiber/fiber/v2/log'
The github.com/gofiber/fiber/v2/log package does not exist. To use logging functions like log.Errorf, you should import the standard library's log package.
Apply this diff to fix the import:
import (
"github.com/gofiber/fiber/v2"
- "github.com/gofiber/fiber/v2/log"
+ "log"
personaMsg "pkg.world.dev/world-engine/cardinal/persona/msg"
servertypes "pkg.world.dev/world-engine/cardinal/server/types"Likely invalid or redundant comment.
75-77: Ensure ValidateTransactionSignature handles empty signerAddress correctly
When signerAddress is an empty string (for messages other than CreatePersona), confirm that validator.ValidateTransactionSignature handles this case appropriately and does not result in incorrect signature validation.
Run the following script to verify the handling of empty signerAddress:
This script searches for the implementation of ValidateTransactionSignature in the codebase. Please review the function to ensure it safely handles an empty signerAddress.
✅ Verification successful
These scripts will help us:
- Find direct references to ValidateTransactionSignature
- Locate validator-related files
- Find any signature validation related code using broader patterns
This will help us understand the complete implementation of the validator, including how it handles empty signerAddress cases.
Empty signerAddress is properly handled by the validator
The implementation in validator.go shows that empty signerAddress is handled correctly:
- When
signerAddressis empty, the validator automatically looks up the correct signer address using thesignerAddressProvider:
if signerAddress == "" {
signerAddress, err = validator.signerAddressProvider.GetSignerAddressForPersonaTag(tx.PersonaTag)
if err != nil {
return &validationError{ErrInvalidSignature, http.StatusUnauthorized,
fmt.Sprintf("could not get signer for persona %s: %v", tx.PersonaTag, err)}
}
}- The only requirement is that
PersonaTagmust not be empty, which is checked first:
if tx.PersonaTag == "" {
return &validationError{ErrNoPersonaTag, http.StatusBadRequest,
fmt.Sprintf("Missing persona tag for message %s", tx.Hash.String())}
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if ValidateTransactionSignature handles empty signerAddress.
# Test: Search for the implementation of ValidateTransactionSignature.
# Expect: It should handle empty signerAddress without errors.
ast-grep --pattern $'func ValidateTransactionSignature($_, $_) $$$_' --lang go
Length of output: 79
Script:
#!/bin/bash
# Let's try a broader search to locate the validator package and its implementation
rg -g "*.go" "ValidateTransactionSignature" -A 5
Length of output: 12935
Script:
#!/bin/bash
# Also search for any validator related files
fd validator -e go
Length of output: 103
Script:
#!/bin/bash
# Search for any signature validation related code
rg -g "*.go" "signature.*validation|validate.*signature" -i
Length of output: 6957
Script:
#!/bin/bash
# Let's examine the validator implementation more closely
cat cardinal/server/validator/validator.go
Length of output: 7122
cardinal/server/validator/validator.go (2)
89-117: Efficient Time Validation in ValidateTransactionTTL
The function correctly calculates the acceptable time range for transactions and properly handles expired and future-dated messages. This ensures that only transactions within the valid time window are processed.
119-158: Comprehensive Signature Validation in ValidateTransactionSignature
The method thoroughly validates transaction signatures, including handling cases where the signer address needs to be retrieved. It also appropriately caches valid transaction hashes to prevent replay attacks, enhancing security.
| @@ -69,15 +45,15 @@ func PostTransaction( | |||
| } | |||
|
|
|||
| // extract the transaction from the fiber context | |||
| tx, fiberErr := extractTx(ctx, verify) | |||
| tx, fiberErr := extractTx(ctx, validator) | |||
There was a problem hiding this comment.
It's idiomatic in Go to just reuse the err variable. You don't have to provide a different name for each error.
In this case, you can just do
tx, err := extractTx(ctx, validator)
| log.Errorf("message %s has invalid transaction payload: %v", tx.Hash.String(), err) | ||
| return fiber.NewError(fiber.StatusBadRequest, "Bad Request - invalid payload") | ||
| // make sure the transaction hasn't expired | ||
| if validationErr := validator.ValidateTransactionTTL(tx); validationErr != nil { |
| GetLogMessage() string | ||
| GetStatusCode() int | ||
| } | ||
| type validationError struct { |
There was a problem hiding this comment.
This custom error type is not necessary and we generally try to avoid since the basic error primitive is more than sufficient.
-
I'd prefer not to control the status code from the validator because this breaks separation of concern; the validator doesn't necessarily have to be consumed by an HTTP server. Instead, in the server handler, use
errors.Is/eris.Isto check for sentinel errors (the one you declared above) and handle the HTTP response accordingly there. -
Instead of using
LogMsg, you can justeris.Wrapyour sentinel error to provide additional context to the error which you can log/print out.
By doing these 2 things, you should be able to eliminate this custom error type.
| func TimestampNow() int64 { | ||
| return time.Now().UnixMilli() // millisecond accuracy on timestamps, easily available on all platforms | ||
| } | ||
|
|
||
| // returns a sign compatible timestamp for the time passed in |
- Still need to get rid of the custom ValidationError in favor of eris.wrap
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range comments (1)
cardinal/server/handler/tx.go (1)
Line range hint
133-148: Returnfiber.Errorfor proper HTTP status codesIn
extractTx, wrapping the error witheris.Wrapmay result in a 500 Internal Server Error when returned. To return the intended 400 Bad Request status, usefiber.NewError.Apply this diff to return a
fiber.Errorwith the correct status code:- return nil, eris.Wrap(err, "Bad Request - unparseable body") + return nil, fiber.NewError(fiber.StatusBadRequest, "Bad Request - unparseable body")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (7)
cardinal/option.go(1 hunks)cardinal/server/handler/tx.go(8 hunks)cardinal/server/option.go(2 hunks)cardinal/server/server.go(4 hunks)cardinal/server/validator/validator.go(1 hunks)cardinal/server/validator/validator_test.go(1 hunks)cardinal/world_persona.go(1 hunks)
🧰 Additional context used
📓 Learnings (1)
cardinal/server/option.go (1)
Learnt from: smsunarto
PR: Argus-Labs/world-engine#806
File: cardinal/server/option.go:45-45
Timestamp: 2024-11-02T13:36:23.178Z
Learning: In `cardinal/server/option.go`, within the `WithHashCacheSize` function, enforce a minimum cache size and log a warning if the provided size is too small.
🔇 Additional comments (23)
cardinal/server/option.go (2)
22-22: LGTM: Config field name change is consistent.
The transition from verify.IsDisabled to config.isSignatureValidationDisabled aligns well with the refactoring objectives while maintaining clear naming.
34-36: LGTM: Type change improves parameter validation.
The change from int to uint naturally prevents negative values, which is an improvement over the previous implementation. The config field change aligns with the overall refactoring.
cardinal/world_persona.go (2)
Line range hint 1-11: Missing validator package import.
The code indicates implementation of validator.SignerAddressProvider interface but the validator package is not imported. This is a critical issue that needs to be addressed.
Since this was flagged in a previous review and is still valid, please add the missing import:
import (
"errors"
"github.com/rotisserie/eris"
"pkg.world.dev/world-engine/cardinal/filter"
"pkg.world.dev/world-engine/cardinal/persona"
"pkg.world.dev/world-engine/cardinal/persona/component"
"pkg.world.dev/world-engine/cardinal/types"
+ "pkg.world.dev/world-engine/server/validator"
)Line range hint 17-45: Verify interface contract implementation.
The method seems to implement the validator.SignerAddressProvider interface, but we should verify that it fully satisfies the interface contract from the validator package.
✅ Verification successful
Based on the output, I can now generate the final response since I have found the interface definition and can verify the implementation matches.
Interface contract is correctly implemented ✅
The GetSignerForPersonaTag method in World correctly implements the SignerAddressProvider interface from cardinal/server/validator/validator.go. The method signature matches exactly:
- Same name:
GetSignerForPersonaTag - Same parameters:
personaTag string, tick uint64 - Same return types:
(addr string, err error)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the SignerAddressProvider interface implementation
# Expected: Find the interface definition and any other implementations for comparison
# Search for the interface definition
echo "=== SignerAddressProvider Interface Definition ==="
ast-grep --pattern 'type SignerAddressProvider interface {
$$$
}'
# Search for other implementations of the interface
echo -e "\n=== Other Interface Implementations ==="
rg -p "implements.*SignerAddressProvider" -g '!world_persona.go'
Length of output: 837
cardinal/option.go (2)
59-63: LGTM! Good type safety improvement.
The change from int to uint for the seconds parameter is a good improvement as it prevents negative expiration times.
65-72: LGTM! Verify default cache size sufficiency.
The new option for configuring hash cache size is well-documented and properly implemented. However, we should verify if the default 1MB cache size is sufficient for common use cases.
Let's check for any existing performance metrics or discussions about cache size:
✅ Verification successful
Default 1MB hash cache size is well-justified
The default 1MB cache size appears sufficient based on the codebase analysis:
- The cache is specifically used for replay protection of message signatures
- The default message expiration is 10 seconds, which limits the time window for cached hashes
- The underlying
freecacheimplementation enforces a minimum size of 512KB, and 1MB provides good headroom above this minimum - The size is configurable via
WithHashCacheSizefor cases where larger cache is needed (e.g., with increased message expiration times)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related performance discussions or metrics
rg -i "hash.*cache.*size|cache.*performance" --type md
Length of output: 56
Script:
#!/bin/bash
# Let's search for hash cache size configurations and defaults across the codebase
rg -i "hash.*cache.*size|defaulthashcache" -A 3 -B 3
# Also check for any replay protection or hash cache related tests
fd -e test -e spec -e _test "hash|cache|replay" --exec cat {}
Length of output: 6079
cardinal/server/server.go (7)
17-17: Importing the validator package
The validator package is correctly imported to utilize the SignatureValidator functionality.
27-27: Definition of defaultHashCacheSizeKB constant
The defaultHashCacheSizeKB constant is set to 1024, providing a default hash cache size of 1 MB as indicated by the comment.
31-35: Consider renaming isSignatureValidationDisabled for consistency
As previously suggested, consider renaming isSignatureValidationDisabled to disableSignatureValidation to align with Go's naming conventions for boolean fields.
39-41: Adding validator field to Server struct
The validator field is appropriately added to the Server struct to encapsulate the signature validation logic.
59-63: Initialization of config struct with default values
The default values are correctly assigned to the config struct, ensuring that the server has appropriate initial settings.
70-77: Initialization of SignatureValidator
The SignatureValidator is properly initialized with the configuration parameters and world context. This centralizes the validation logic and improves code maintainability.
187-187: Updating PostTransaction handler with SignatureValidator
The PostTransaction handler is updated to accept the SignatureValidator, aligning with the refactored validation logic and enhancing separation of concerns.
cardinal/server/validator/validator.go (1)
192-192: Simplify Error Wrapping in validateSignature
Wrapping the error returned by tx.Verify(signerAddr) with an empty string using eris.Wrap does not add meaningful context. Consider simplifying the error handling.
cardinal/server/validator/validator_test.go (3)
60-62: Well-structured test suite using testify/suite
Great job setting up the test suite with testify/suite. This approach enhances the organization and readability of your tests.
97-197: Comprehensive testing with signature verification disabled
The test cases thoroughly cover various scenarios when signature verification is disabled, ensuring that transactions are appropriately handled in these conditions.
199-394: Extensive validation tests with signature verification enabled
Your test suite effectively covers a wide range of scenarios, including handling invalid timestamps, altered transaction data, and duplicate transactions. This comprehensive testing enhances the reliability and security of the validation logic.
cardinal/server/handler/tx.go (6)
10-10: Import validator package for signature validation
The addition of the validator package import is appropriate to utilize the SignatureValidator for transaction validation.
36-36: Update function signature to include SignatureValidator
The PostTransaction function now correctly accepts a pointer to SignatureValidator, aligning with the refactored validation logic.
52-54: Consistent error handling with fiber.Error
The use of validator.ValidateTransactionTTL is correct, and logging the internal details is appropriate. Returning a fiber.Error ensures the correct HTTP status code is sent in the response.
107-109: Function PostGameTransaction passes validator correctly
The PostGameTransaction function now correctly delegates to PostTransaction with the validator, ensuring that game transactions are properly validated.
128-130: Function PostPersonaTransaction passes validator correctly
The PostPersonaTransaction function now correctly delegates to PostTransaction with the validator, ensuring that persona transactions are properly validated.
46-46: 🧹 Nitpick (assertive)
Reuse the err variable instead of declaring fiberErr
It's idiomatic in Go to reuse the err variable rather than introducing a new variable like fiberErr. This makes the code cleaner and more consistent.
Apply this diff to reuse the err variable:
- tx, fiberErr := extractTx(ctx, validator)
- if fiberErr != nil {
- return fiberErr
+ tx, err := extractTx(ctx, validator)
+ if err != nil {
+ return errLikely invalid or redundant comment.
| func WithHashCacheSize(sizeKB uint) Option { | ||
| return func(s *Server) { | ||
| s.verify.HashCacheSizeKB = sizeKB | ||
| s.config.messageHashCacheSizeKB = sizeKB |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Document FreeCache's minimum size requirement.
While the type change to uint is good, it would be helpful to document that FreeCache has a minimum cache size requirement of 512KB. This information is crucial for users configuring the cache size.
Add a note in the documentation:
// WithHashCacheSize how big the cache of hashes used for replay protection
// is allowed to be. Default is 1MB.
+// Note: The underlying FreeCache implementation enforces a minimum size of 512KB.
// This setting is ignored if the DisableSignatureVerification option is used📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func WithHashCacheSize(sizeKB uint) Option { | |
| return func(s *Server) { | |
| s.verify.HashCacheSizeKB = sizeKB | |
| s.config.messageHashCacheSizeKB = sizeKB | |
| // WithHashCacheSize how big the cache of hashes used for replay protection | |
| // is allowed to be. Default is 1MB. | |
| // Note: The underlying FreeCache implementation enforces a minimum size of 512KB. | |
| // This setting is ignored if the DisableSignatureVerification option is used | |
| func WithHashCacheSize(sizeKB uint) Option { | |
| return func(s *Server) { | |
| s.config.messageHashCacheSizeKB = sizeKB |
| IsDisabled bool | ||
| MessageExpirationSeconds uint | ||
| HashCacheSizeKB uint | ||
| namespace string | ||
| cache *freecache.Cache | ||
| signerAddressProvider SignerAddressProvider |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider Grouping Related Configuration Fields
In the SignatureValidator struct, grouping related configuration fields together can enhance readability and maintainability. For instance, you can group cache-related fields and signature configuration fields.
| txEarliestValidTimestamp := sign.TimestampAt( | ||
| now.Add(-(time.Duration(validator.MessageExpirationSeconds) * time.Second))) | ||
| txLatestValidTimestamp := sign.TimestampAt(now.Add(time.Duration(ttlMaxFutureSeconds) * time.Second)) | ||
| // before we even create the hash or validator the signature, check to see if the message has expired |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Typographical Error in Comment
There's a typo in the comment. The word "validator" should be "validate".
Apply this diff to correct the typo:
- // before we even create the hash or validator the signature, check to see if the message has expired
+ // before we even create the hash or validate the signature, check to see if the message has expired📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // before we even create the hash or validator the signature, check to see if the message has expired | |
| // before we even create the hash or validate the signature, check to see if the message has expired |
| // this allows for some degree of clock drift. It's safe enought to accept a message that's stamped from the near | ||
| // future because we will still keep it in the hash cache and prevent it from being a replay attack vector. However, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Typographical Error in Comment
There's a typo in the comment. The word "enought" should be "enough".
Apply this diff to correct the typo:
-// this allows for some degree of clock drift. It's safe enought to accept a message that's stamped from the near
+// this allows for some degree of clock drift. It's safe enough to accept a message that's stamped from the near📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // this allows for some degree of clock drift. It's safe enought to accept a message that's stamped from the near | |
| // future because we will still keep it in the hash cache and prevent it from being a replay attack vector. However, | |
| // this allows for some degree of clock drift. It's safe enough to accept a message that's stamped from the near | |
| // future because we will still keep it in the hash cache and prevent it from being a replay attack vector. However, |
| return &validationError{ErrInvalidSignature, http.StatusUnauthorized, | ||
| fmt.Sprintf("could not get signer for persona %s: %v", tx.PersonaTag, err)} |
There was a problem hiding this comment.
Avoid Exposing Internal Error Details to Clients
The error message returned includes internal error details (err), which may expose sensitive information to the client. It's better to provide a generic error message to the client and log the detailed error internally.
Apply this diff to modify the error response:
return &validationError{ErrInvalidSignature, http.StatusUnauthorized,
- fmt.Sprintf("could not get signer for persona %s: %v", tx.PersonaTag, err)}
+ fmt.Sprintf("could not get signer for persona %s", tx.PersonaTag),
+ LogMsg: fmt.Sprintf("Signer retrieval error for persona %s: %v", tx.PersonaTag, err)}Committable suggestion skipped: line range outside the PR's diff.
| // don't need to check the cast bc we already validated this above | ||
| createPersonaMsg, ok := msg.(personaMsg.CreatePersona) | ||
| if !ok { | ||
| return fiber.NewError(fiber.StatusInternalServerError, "Internal Server Error - bad message type") |
There was a problem hiding this comment.
Update misleading comment and improve clarity
The comment states there's no need to check the cast, but the code is performing a type assertion check. This inconsistency may cause confusion.
Apply this diff to correct the comment and enhance readability:
- // don't need to check the cast bc we already validated this above
+ // checking the type assertion for safety📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // don't need to check the cast bc we already validated this above | |
| createPersonaMsg, ok := msg.(personaMsg.CreatePersona) | |
| if !ok { | |
| return fiber.NewError(fiber.StatusInternalServerError, "Internal Server Error - bad message type") | |
| // checking the type assertion for safety | |
| createPersonaMsg, ok := msg.(personaMsg.CreatePersona) | |
| if !ok { | |
| return fiber.NewError(fiber.StatusInternalServerError, "Internal Server Error - bad message type") |
Closes: WORLD-1237
Overview
Separates out the validation code into SignatureValidator object in a server/validator subpackage to reduce complexity and improve separation of concerns, and makes it independently testable.
Brief Changelog
Testing and Verifying
All CI tests have been run and passed, including the new cardinal/server/validator/validator_test.go suite
Summary by CodeRabbit
Release Notes
New Features
SignatureValidatorfor improved transaction validation, enhancing security against replay attacks.Bug Fixes
Documentation
Tests