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

Implementation not according to spec #30

Open
holiman opened this issue Dec 28, 2021 · 2 comments
Open

Implementation not according to spec #30

holiman opened this issue Dec 28, 2021 · 2 comments

Comments

@holiman
Copy link

holiman commented Dec 28, 2021

I found a couple of things about the spec to be a bit ambiguous fellowship post, and checked out how this reference implementation handles it. Turns out, not according to spec.

Statement regex, https://github.com/spruceid/siwe/blob/main/lib/regex.ts#L4:

const STATEMENT = '((?<statement>[^\\n]+)\\n)?';

As opposed to

const STATEMENT = '((?<statement>[^\\n]*)\\n)?';

Resources

const RESOURCES = `(\\nResources:(?<resources>(\\n- ${URI}?)+))?`;

as opposed to

const RESOURCES = `(\\nResources:(?<resources>(\\n- ${URI}?)*))?`;

So siwe does not allow empty-but-present statement, nor empty-but-present resources. Which I think is fine, but it's not according to spec.

I hope that rather than fixing this issue by making it adhere to the spec, we can update the spec, to change

statement = *( reserved / unreserved / " " )

into

statement = 1*( reserved / unreserved / " " )

and

resources = *( LF resource )

into

resources = 1*( LF resource )
@wyc
Copy link
Contributor

wyc commented Dec 28, 2021

Thank you for the issue, we should review the regex for any discrepancies. It does seem that requiring the statement by specifying at least one character could create a more regular message format, which may be preferable. I think it's also a reasonable suggestion to require the presence resources when "Resources:\n" exists, but this could prevent users from expressing a resource section of zero resources as opposed to an omitted resources section.

I would also like to note that the in-production implementations use the ABNF grammar from the spec directly implemented here, so this shouldn't affect any applications relying on this library to create/validate SIWE messages for anyone following this issue. The regex is provided as an alternative for implementation that can be leveraged when no ABNF parsers are available or for performance.

@fernando-deka
Copy link

Also, this implementation does not allow for the schema to be present with the domain:

sign-in-with-ethereum =
    [ scheme "://" ] domain %s" wants you to sign in with your Ethereum account:" LF
    address LF
    LF
    [ statement LF ]
    LF
    %s"URI: " uri LF
    %s"Version: " version LF
    %s"Chain ID: " chain-id LF
    %s"Nonce: " nonce LF
    %s"Issued At: " issued-at
    [ LF %s"Expiration Time: " expiration-time ]
    [ LF %s"Not Before: " not-before ]
    [ LF %s"Request ID: " request-id ]
    [ LF %s"Resources:"
    resources ]

scheme = ALPHA *( ALPHA / DIGIT / "+" / "-" / "." )
    ; See RFC 3986 for the fully contextualized
    ; definition of "scheme".

localhost:3000 wants you to sign in with your Ethereum account: ...  ✅
http://localhost:3000 wants you to sign in with your Ethereum account: ... ❌

According to the spec, that should be parsed but it fails.

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

No branches or pull requests

4 participants