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.

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

Conversation

thomaspanf
Copy link
Member

Copy link
Contributor

@jshufro jshufro left a comment

Choose a reason for hiding this comment

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

hate to be a party pooper

@@ -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

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.

@thomaspanf thomaspanf closed this Jul 23, 2024
@thomaspanf
Copy link
Member Author

This piece of functionality is going to included in package eip712 within smartnode pr #599.

Closing #17 now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants