Implementation for FLIP 321: Stricter Validation of Node Network Addresses #484
Conversation
contracts/FlowIDTableStaking.cdc
Outdated
| // Check if domain is an IP address (contains only numbers and dots) | ||
| let domainParts = domain.split(separator: ".") | ||
| if domainParts.length == 4 { | ||
| var isIP = true | ||
| for part in domainParts { | ||
| let num = UInt8.fromString(part) | ||
| if num == nil || num! > 255 { | ||
| isIP = false | ||
| break | ||
| } | ||
| } | ||
| if isIP { | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| // Check if domain has at least one dot and valid characters | ||
| if !domain.contains(".") { | ||
| return false | ||
| } |
There was a problem hiding this comment.
here better way is to check last segment is not numeric. ( ip has a lot of quirks: 51.5499081 is a valid IP address for example )
There was a problem hiding this comment.
excellent suggestion!
There was a problem hiding this comment.
I have update the changes to be more robust.
contracts/FlowIDTableStaking.cdc
Outdated
| for char in domain.utf8 { | ||
| // Check if character is: | ||
| // - a letter (a-z, A-Z) | ||
| // - a number (0-9) | ||
| // - a dot (.) | ||
| // - a hyphen (-) | ||
| if !((char >= 48 && char <= 57) || // numbers | ||
| (char >= 65 && char <= 90) || // uppercase letters | ||
| (char >= 97 && char <= 122) || // lowercase letters | ||
| char == 46 || // dot | ||
| char == 45) { // hyphen | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
maybe something like this ?
var validChars = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789.-".utf8
for char in domain.utf8{
if !validChars.contains(char) {
return false
}
}
| @@ -391,7 +391,63 @@ func TestIDTableRegistration(t *testing.T) { | |||
| idTableAddress, | |||
| IDTableSigner, | |||
| adminID, | |||
| fmt.Sprintf("%0128d", admin), | |||
| // Invalid Networking Address: IPv4 address instead of domain name | |||
There was a problem hiding this comment.
Unit tests to check that IPv4, IPv6, missing port and invalid domain are caught by the validation.
…ving length check to inside the network address validation
| @@ -166,7 +166,7 @@ access(all) contract FlowIDTableStaking { | |||
| FlowIDTableStaking.isValidNodeID(id): "The node ID must have only numbers and lowercase hex characters" | |||
| FlowIDTableStaking.nodes[id] == nil: "The ID cannot already exist in the record" | |||
| role >= UInt8(1) && role <= UInt8(5): "The role must be 1, 2, 3, 4, or 5" | |||
| networkingAddress.length > 0 && networkingAddress.length <= 510: "The networkingAddress must be less than 510 characters" | |||
There was a problem hiding this comment.
moved this check inside of the isValidateNetworkingAddress function.
vishalchangrani
changed the title
| @@ -430,6 +416,40 @@ func TestIDTableRegistration(t *testing.T) { | |||
| true) | |||
| }) | |||
|
|
|||
| t.Run("Shouldn't be able to create Node struct with an invalid networking address", func(t *testing.T) { | |||
There was a problem hiding this comment.
separated out the test for networking address in it's own unit test.
| /// 1. Must not be an IP address | ||
| /// 2. Must contain a port number after a colon | ||
| /// 3. Must be a valid domain name format | ||
| access(contract) view fun isValidNetworkingAddress(address: String): Bool { |
There was a problem hiding this comment.
I feel like this function should be in a string utils contract and not in the staking contract. What do you think? We have one deployed for the bridge that we could potentially put it in
There was a problem hiding this comment.
Can you point me to it? I can't find the StringUtils contract.
FLIP PR: onflow/flips#333
FLIP Issue: onflow/flips#332