Skip to content

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

Merged
merged 17 commits into from
Jul 11, 2025
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 88 additions & 2 deletions contracts/FlowIDTableStaking.cdc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor Author

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.

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"
Expand Down Expand Up @@ -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"
}

Expand Down Expand Up @@ -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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you guys think about making this access(all) instead access(contract) so that any client side apps like FlowPort can use it to verify the networking address before submitting the transaction.
The function does not modify state so should be safe to expose.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, we should make this access(all)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed it to access(all).

// Check length
if address.length == 0 && address.length > 510 {
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
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original implementation was way more efficient: 62f22a4 (#484)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@turbolent Actually I suggested this one for readability mostly, didn't seem to be too inefficient at first sight. ( ~ maybe half efficient ) and I assumed this would be called once ofc.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Expand Down
6 changes: 3 additions & 3 deletions lib/go/contracts/internal/assets/assets.go

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion lib/go/test/epoch_test_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -500,7 +500,7 @@ func registerNodesForEpochs(
authorizer,
signers[i],
ids[i],
fmt.Sprintf("%0128d", i),
getNetworkingAddress(i),
networkingkeys[i],
stakingKeys[i],
stakingKeysPOPs[i],
Expand Down
6 changes: 3 additions & 3 deletions lib/go/test/flow_idtable_nodes_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ func TestIDTableManyNodes(t *testing.T) {

nodeRoles[i] = cadence.NewUInt8(uint8((i % 4) + 1))

networkingAddress := fmt.Sprintf("%0128d", i)
networkingAddress := getNetworkingAddress(i)

nodeNetworkingAddresses[i] = CadenceString(networkingAddress)

Expand Down Expand Up @@ -446,7 +446,7 @@ func TestIDTableOutOfBoundsAccess(t *testing.T) {

nodeRoles[i] = cadence.NewUInt8(uint8((i % 4) + 1))

networkingAddress := fmt.Sprintf("%0128d", i)
networkingAddress := getNetworkingAddress(i)

nodeNetworkingAddresses[i] = CadenceString(networkingAddress)

Expand Down Expand Up @@ -608,7 +608,7 @@ func TestIDTableUnstakeAllManyDelegators(t *testing.T) {
require.NoError(t, err)
err = tx.AddArgument(cadence.NewUInt8(role))
require.NoError(t, err)
err = tx.AddArgument(CadenceString(fmt.Sprintf("%0128d", i)))
err = tx.AddArgument(CadenceString(getNetworkingAddress(i)))
require.NoError(t, err)
err = tx.AddArgument(CadenceString(networkingKey))
require.NoError(t, err)
Expand Down
Loading
Loading