Added webAuthDomain parameter and validation to SEP-10 util functions #607
Conversation
There was a problem hiding this comment.
The JS looks good & comprehensive. I can't speak to the validity of the implementation (largely unfamiliar with SEP-10) so wait on @leighmcculloch for that, but otherwise this LGTM 👍. Thanks for doing this after raising #606, Jake!
|
I had to force push after rebasing this branch on the update master, sorry |
| } | ||
| if ( | ||
| op.name === "web_auth_domain" && | ||
| op.value.compare(Buffer.from(webAuthDomain)) |
There was a problem hiding this comment.
Love it. 🚀
Initially I wasn't sure if this would work because compare returns a -1, 0, or 1 depending how the two buffers sort, but TIL it does work because:
> !!-1
true
> !!1
true
> !!0
false
test/unit/utils_test.js
Outdated
| "testanchor.stellar.org", | ||
| "testanchor.stellar.org" |
There was a problem hiding this comment.
💡 For the sake of making sure the correct parameter is being matched against could you make the home domain an web auth domain different?
| @@ -234,6 +245,19 @@ export namespace Utils { | |||
| "The transaction has operations that are unrecognized", | |||
| ); | |||
| } | |||
| if (op.value === undefined) { | |||
There was a problem hiding this comment.
Hi @leighmcculloch @JakeUrban, I have some confusion about this, does the SEP-10 protocol specify that the values in other ManageData Operation cannot be null?
There was a problem hiding this comment.
That's a good point, it does not specify this and shouldn't reject a challenge transaction because of this. This is a bug.
There was a problem hiding this comment.
Nice catch. I'll fix it today, before release.
resolves #606
Changes:
webAuthDomainparameter toreadChallengeTx(),buildChallengeTx(),verifyChallengeTxSigners(), andverifyChallengeTxThreshold()buildChallengeTx()now adds an additional Manage Data operation as described in the SEP-10 v3.1 changesreadChallengeTx()now verifies that the addedwebAuthDomainparameter matches the added Manage Data operation value if included in the challengeverifyChallengeTxSigners()andverifyChallengeTxThreshold()passwebAuthDomaintoreadChallengeTx()untils_test.js) have been added & updated