SEP-24: Add callback signature requirement #1263
Conversation
If a callback is used, it needs to be signed by the Anchor so that the Wallet can verify the provenance and integrity.
|
@JakeUrban please take a look thanks! |
Co-authored-by: Howard Tinghao Chen <howard7708@gmail.com>
There was a problem hiding this comment.
LGTM. Let's have another pair of eyes on it before merging it tho. @leighmcculloch can you take a look?
ecosystem/sep-0024.md
Outdated
| `callback` | string | (optional) [`postMessage`](https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage) or a URL that the anchor should `POST` a JSON message to when the user successfully completes the interactive flow. The callback needs to be signed by the anchor and the signature needs to be verified by the wallet according to the [callback signature specification](#callback-signature). | ||
| `on_change_callback` | string | (optional) [`postMessage`](https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage) or a URL that the anchor should `POST` a JSON message to when the `status` or `kyc_verified` properties change. The callback needs to be signed by the anchor and the signature needs to be verified by the wallet according to the [callback signature specification](#callback-signature). |
There was a problem hiding this comment.
We need to clarify that the added statement only applies to URLs, not postmessage callbacks.
There was a problem hiding this comment.
Actually don't you think we should add a signature requirement for the messages sent by the anchor if postMessage is used? That would solve some of the mentioned security concerns.
There was a problem hiding this comment.
postmessage is a JavaScript construct, its not made with a HTTPS request, so the description of how the callback should be made doesn't fit the mechanism by which the message is sent in this case.
There was a problem hiding this comment.
Yes sure we will need to come up with another way to compute the signature but don't you think that the messages should be signed somehow?
There was a problem hiding this comment.
Are we overlapping with guarantees the browser provides signing the postMessage callbacks?
There was a problem hiding this comment.
I don't think its the same level of concern as the HTTPS callbacks, because the wallet verified the anchor's identity when it authenticated via SEP-10, which is required for requesting a webview URL.
So I would say the wallet has a higher level of confidence on the origin of the postmessage request because it comes from the webview, not a random client on the web.
There was a problem hiding this comment.
Yes I agree with you the risk is lower.
That being said when listening for events from postMessage all the messages are queued whatever the origin is.
For example with the code below:
<!-- wallet.html -->
<html>
<h1>Wallet</h1>
<body>
<script type="text/Javascript">
function OpenPopupWindow(url, name, where)
{
myRef = window.open(url , name,'left=20,top=20,width=500,height=500,toolbar=1,resizable=0');
myRef.focus()
}
window.addEventListener("message", (event) => {
console.log(event.data);
}, false);
</script>
<form>
<input type=button value="Anchor" onClick="OpenPopupWindow('anchor.html', 'Anchor');">
<input type=button value="Evil" onClick="OpenPopupWindow('evil.html', 'Evil');">
</form>
</body>
</html>
<!-- anchor.html -->
<html>
<h1>Anchor</h1>
<body>
<p>ANCHOR</p>
<script type="text/Javascript">
window.opener.postMessage("ANCHOR", "*");
</script>
</body>
</html>
<!-- evil.html -->
<html>
<h1>Evil</h1>
<body>
<p>EVIL</p>
<script type="text/Javascript">
window.opener.postMessage("ANCHOR", "*");
</script>
</body>
</html>The wallet will not be able to distinguish calls from the Anchor and calls from Evil.
This can happen if the wallet uses iframe for ads and integrate 3rd party JS/CSS/HTML libraries.
A simpler solution would be to add a requirement for the wallet to check that event.origin is set to the anchor host when listening for events.
There was a problem hiding this comment.
@JakeUrban what do you think? Maybe I can specify this does not apply to postMessage callbacks to merge this and open another discussion about it? Thanks!
There was a problem hiding this comment.
I'm up for discussing how to handle postmessage callbacks but I agree it would be ideal if we didn't block our solution for HTTP callbacks. Lets merge this and start another thread.
|
This pull request is stale because it has been open for 30 days with no activity. It will be closed in 30 days unless the stale label is removed. |
The callback signature discussed here does not apply to postMessage type callbacks
C0x41lch0x41
dismissed stale reviews from leighmcculloch and howardtw
via
8c1763f
|
@JakeUrban see my update mentioning postMessage callbacks are out of scope for the signature. Thanks! |
Adding callback signature requirements from SEP12