-
Notifications
You must be signed in to change notification settings - Fork 51
Implementation for FLIP 332: Stricter Validation of Node Network Addresses #484
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 9 commits
139cbc5
4cc64b2
76e5e49
d9af62d
3dcd1ca
62f22a4
f80b74c
a032312
c12f967
6234e1c
0d08959
4ca5509
cbb117c
ca5115f
d0a230e
37235e0
775d934
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
FlowIDTableStaking.isValidNetworkingAddress(address: networkingAddress): "The networkingAddress must be a valid domain name with a port (e.g., node.flow.com:3569), must not exceed 510 characters, and cannot be an IP address" | ||
networkingKey.length == 128: "The networkingKey length must be exactly 64 bytes (128 hex characters)" | ||
stakingKey.length == 192: "The stakingKey length must be exactly 96 bytes (192 hex characters)" | ||
!FlowIDTableStaking.getNetworkingAddressClaimed(address: networkingAddress): "The networkingAddress cannot have already been claimed" | ||
|
@@ -437,7 +437,7 @@ access(all) contract FlowIDTableStaking { | |
access(NodeOperator) fun updateNetworkingAddress(_ newAddress: String) { | ||
pre { | ||
FlowIDTableStaking.stakingEnabled(): "Cannot update networking address if the staking auction isn't in progress" | ||
newAddress.length > 0 && newAddress.length <= 510: "The networkingAddress must be less than 510 characters" | ||
FlowIDTableStaking.isValidNetworkingAddress(address: newAddress): "The networkingAddress must be a valid domain name with a port (e.g., node.flow.com:3569), must not exceed 510 characters, and cannot be an IP address" | ||
!FlowIDTableStaking.getNetworkingAddressClaimed(address: newAddress): "The networkingAddress cannot have already been claimed" | ||
} | ||
|
||
|
@@ -1822,6 +1822,92 @@ access(all) contract FlowIDTableStaking { | |
return true | ||
} | ||
|
||
/// Validates that a networking address is properly formatted | ||
/// Requirements: | ||
/// 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 { | ||
joshuahannan marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what do you guys think about making this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, we should make this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changed it to |
||
// Check length | ||
if address.length == 0 && address.length > 510 { | ||
vishalchangrani marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return false | ||
} | ||
|
||
// Split the address into domain and port | ||
let parts = address.split(separator: ":") | ||
if parts.length != 2 { | ||
return false | ||
} | ||
|
||
let domain = parts[0] | ||
let port = parts[1] | ||
|
||
// Check if port is a valid number between 1 and 65535 | ||
let portNum = UInt16.fromString(port) | ||
if portNum == nil || portNum! < 1 || portNum! > 65535 { | ||
return false | ||
} | ||
|
||
// Check if domain has at least one dot and valid characters | ||
if !domain.contains(".") { | ||
return false | ||
} | ||
|
||
// Check if domain contains only letters, digits and hyphens | ||
let validChars = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789.-".utf8 | ||
for char in domain.utf8 { | ||
if !validChars.contains(char) { | ||
return false | ||
} | ||
} | ||
Comment on lines
+1857
to
+1862
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might be quite inefficient: The lookup array of valid chars is re-built on each call of the function, and the loop ends up effectively performing a nested linear scan: first over all characters of the domain, and then again nested over all valid characters. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The original implementation was way more efficient: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @turbolent Actually I suggested this one for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I see now! Nice 👍 Yeah, this is definitely more readable, the original implementation is not very obvious. I don't know how often this will be called. In case it is a heavily called function, we might want to trade some readability / "straight-forwardness" code for performance. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This validation will only be called when a new node is registered or when the networking address of an existing node is update. Both of those actions don't happen frequently - once or twice every couple of epoch at most. Hence, I would rather have more readability than performance. Open to suggestions tho. |
||
|
||
let hyphen = "-".utf8[0] | ||
let labels = domain.split(separator: ".") | ||
for label in labels { | ||
// Label should not be empty | ||
if label.length == 0 { | ||
return false | ||
} | ||
|
||
// Label should not start or ends with a hyphen | ||
var labelChars = label.utf8 | ||
if labelChars[0] == hyphen || labelChars[labelChars.length - 1] == hyphen { | ||
return false | ||
} | ||
} | ||
|
||
let tld = labels[labels.length - 1] | ||
|
||
// TLD must be atleast 2 characters long | ||
if tld.length < 2 { | ||
return false | ||
} | ||
|
||
// TLD must not be all digits | ||
var hasLetter = false | ||
for c in tld.utf8 { | ||
|
||
// TLD must not contain a hyphen | ||
if c == hyphen { | ||
return false | ||
} | ||
|
||
// TLD must not be all digits | ||
let isUpper = c >= 65 && c <= 90 // 'A'-'Z' | ||
let isLower = c >= 97 && c <= 122 // 'a'-'z' | ||
if isUpper || isLower { | ||
hasLetter = true | ||
break | ||
} | ||
} | ||
|
||
if !hasLetter { | ||
return false | ||
} | ||
|
||
return true | ||
} | ||
|
||
/// Indicates if the staking auction is currently enabled | ||
access(all) view fun stakingEnabled(): Bool { | ||
return self.account.storage.copy<Bool>(from: /storage/stakingEnabled) ?? false | ||
|
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved this check inside of the
isValidateNetworkingAddress
function.