-
Notifications
You must be signed in to change notification settings - Fork 297
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
Update SEP-10 Utils #568
Update SEP-10 Utils #568
Conversation
test/unit/utils_test.js
Outdated
const challenge = StellarSdk.Utils.buildChallengeTx( | ||
keypair, | ||
"GBDIT5GUJ7R5BXO3GJHFXJ6AZ5UQK6MNOIDMPQUSMXLIHTUNR2Q5CFNF", | ||
null, |
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.
The one downside to this approach is that I can't make anchorName
optional, since networkPassphrase
has no default. This may mean that the best approach is having two different functions, one for each SEP-10 protocol major version.
@abuiles @marcelosalloum for reference |
src/utils.ts
Outdated
// fully qualified domain name of the web service requiring authentication | ||
let manageDataKey; | ||
if (homeDomain) { | ||
const homeDomainKeyValue = homeDomain.startsWith("https://") |
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.
what if it starts with "http://" ?
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.
nevermind, I just checked and it seems a toml must be hosted via ssl
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.
I think it might be better to be strict and only accept full https urls
src/utils.ts
Outdated
@@ -185,6 +207,18 @@ export namespace Utils { | |||
); | |||
} | |||
|
|||
// verify homeDomain | |||
if (homeDomain) { | |||
const homeDomainKeyValue = homeDomain.startsWith("https://") |
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.
there seems to be some inconsistency between the code which extracts the home domain value here and the code in buildChallengeTx()
@tamirms thanks for your comments, after the Python SDK was released today I decided to mirror that approach. It replaces the See the python SDK functions here. The optional |
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.
👍
resolves stellar/integration-meta#193
Adds the optional
homeDomain
argument tobuildChallengeTx()
andreadChallengeTx()
.The change to
buildChallengeTx()
will allow anchors to build challenge transactions with their service's home domain included in Manage Data operation.Note that this is not the home domain of the server that returns and verifies the challenge transaction. The home domain referenced in the changes here is in reference to the home domain that hosts the service's stellar.toml file.
The change to
readChallengeTx()
will allow clients to verify that the challenge transaction received includes the home domain of the service hosting the same TOML file used to fetch the endpoint for the authentication request. Anchors can also verify this field to ensure clients do not send challenges received from other anchors.These SDK changes do not guarantee the challenge transaction originated from the same service that hosts the TOML file used. For that, clients must verify the following:
SIGNING_KEY
listed in the same stellar.toml file