Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
NEP 0413: Add
signMessage
method to Wallet Standard #413NEP 0413: Add
signMessage
method to Wallet Standard #413Changes from 39 commits
1dbc444
4327348
125ab00
6567fb5
e4ece58
8cc4e6e
763d0ae
287153d
6a14033
af93733
eff4cc9
c2512a4
9327ae8
34fed8c
ab4b493
cba446e
23eeda8
343fe16
dbaeece
b8d698f
0084811
129b452
36b02bc
5b9ddc0
1115399
2e13c7c
9159dec
dd84fb4
a18917a
8721a7a
16eeef1
112dd78
a84a43f
c4065d2
be0e30d
d278e48
17c4ec5
d68a682
2e43d0e
5d5712a
23bd3ac
3a33754
53070e7
8b0b05c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This interface is unclear and redundant. In general, there are two main reasons someone may want to sign a message: as a way of federated login (to prove wallet ownership) and to authenticate a human-readable message for another person. This NEP seems mostly focused on the first use case, for which there is no need for a message. Likewise, the second use case doesn't need all the other parameters.
Those different types of usage should preferably be separated, so that a signature made for one purpose could not be reinterpreted for the other. For example, this NEP may be reformulated to be only for login, in which case the message parameter can be removed.
Another problem here is the
recipient
parameter. Is it an account ID or a domain name? Verifying the recipient properly is very important for security, yet this NEP doesn't specify how this should be done. The most secure approach would be to encode the entire callback URL into the signed message, this should prevent any tampering.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.
Well, it started there, but the scope was decided to be expanded to allow more generic message signing. It could be a separate NEP (if necessary) that will be based on this NEP.
Can you elaborate on which ones and why they are not necessary?
That is a good point, but I believe it is outside of the scope of this NEP. Further protocols based on this NEP (e.g. proof of wallet ownership) should define their message structure and then Wallets would be able to display the requests properly, and thus keep their users informed about the type of request being signed.
@abacabadabacaba I believe this field is used to ensure that the Wallet was signing the message for this recipient alone, and is not used to authenticate the recipient for the Wallet. The recipient could be an account, a domain, or anything else, and it is the responsibility of the recipient of the signed message to validate that the message was not constructed for someone else and reused here. (@gagdiez @gutsyphilip I believe it is important to elaborate on the role of the recipient field in the NEP itself)
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.
@abacabadabacaba The NEP is a result of 100+ messages in this discussion. The community wanted a way to sign a string message for a specific receiver (e.g. "anna.near", "myweb.com", or "aunt Mary"). Since some people wanted to use this as a way of login-while-passing-data (see the discussion), a
nonce
was added, so the same message for the same receiver could not be used twice.While I understand the convenience of having two methods separated, this is not what the community was asking for. If there is no major security drawbacks, I would suggest to keep it like this.
If this does turn to be a major security drawback, we can collaborate on suggesting new changes, and maybe we can create another NEP together.
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.
Until we get more details on why
recipient
,nonce
andcallbackUrl
are not needed for authenticating a human-readable message for another person the only action item I see here is to elaborate on the role of therecipient
field perhaps providing some examples as well on the actual NEP.I agree that it's preferable to have a more flexible standard that can be adapted to both use cases if there are no major security vulnerabilities that implementors of this standard are likely to face.
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.
@gagdiez is there any way/example on how to verify the signature for browser wallets?
The verifySignature function in the example needs the
message
,nonce
,recipient
andcallbackUrl
to re-construct the payload which was actually signed but we lose all this data on redirect?What we get back in the URL is only the
SignedMessage
.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.
with the current approach, you will need the user to pass it back to you. If you are implementing an app in which the user is asked to sign a message, then you will need to persist somewhere the parameters, redirect the user to the wallet, get the key+signature from fragment, and the rest of the parameters from the persisted storage.