Skip to content
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.

Sign up for GitHub

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

Add ValidateSignature helper function #17

Closed
wants to merge 1 commit into from
Closed

Add ValidateSignature helper function #17

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 12 additions & 0 deletions utils/input/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,18 @@ func ValidateAddress(name, value string) (common.Address, error) {
return common.HexToAddress(value), nil
}

// Validate an EIP-712 signature
func ValidateSignature(name, signature string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

yikes, uh, this doesn't actually validate a signature so it shouldn't be called ValidateSignature.

It just checks that it's 0x-prefixed hex of 65 bytes.

There's no reason to have a function that does that specifically for EIP-712... if we're sanitizing user input i suggest:

const EIP712Length = 65

// SanitizeHexInput checks for and strips a 0x prefix, returning an error if one is not found.
// It returns the byte slice representing the decoded hex, or a decoding error
func SanitizeHex(inp string) ([]byte, error) {
  if !strings.HasPrefix(inp, "0x") {
    return nil, errors.New("expected hex prefix")
  }
  return hex.DecodeString(strings.TrimPrefix(inp, "0x"))
}

// SanitizeEIP712String checks that a 0x-prefixed hex string is the proper length to be
// parsed as a EIP-712 signature, and parses it, returning an error if the length is invalid
// or the hex cannot be decoded.
func SanitizeEIP712String(inp string) ([]byte, error) {
  inp, err := SanitizeHexInput(inp)
  if err != nil {
    return nil, fmt.Errorf("error sanitizing EIP-712 signature string as hex: %w", err)
  }
  if len(out) != EIP712Length {
    return nil, fmt.Errorf("error sanitizing EIP-712 signature string: invalid length %d (expected %d)", len(out), EIP712Length)
  }
  return out, nil
}

Copy link
Member

@jclapis jclapis Jul 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggestion for the sake of common usage: move the r, s, v parsing logic out of the SN and put it into the ValidateSignature function (renamed to ValidateEip712Signature in my PR), perhaps with a struct for the components if Geth doesn't already have one. If the intent of this is "take a string that's supposed to be an EIP-712 signature and turn it into an EIP-712 signature" like the other Validate methods do then that'd make sense here.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://google.github.io/styleguide/go/decisions#initialisms

But yes, either sanitize the input and don't validate, or just do everything here. Both are acceptable- validation of the signature should happen in the front end and the backend.

if len(signature) != 132 || signature[:2] != "0x" {
return "", fmt.Errorf("Invalid %s, '%s'\n", name, signature)
}
signatureTruncated := signature[2:]
if !regexp.MustCompile("^[A-Fa-f0-9]+$").MatchString(signatureTruncated) {
return "", fmt.Errorf("Invalid %s, '%s'\n", name, signature)
}
return signature, nil
}

// Validate a wei amount
func ValidateWeiAmount(name, value string) (*big.Int, error) {
val := new(big.Int)
Expand Down
Loading