stellar / js-stellar-sdk

Main Stellar client library for the JavaScript language.
https://stellar.github.io/js-stellar-sdk/
Apache License 2.0
640 stars 313 forks source link

Odd differences between signed and unsigned auth credentials #1070

Open kalepail opened 1 month ago

kalepail commented 1 month ago

Describe the bug

What version are you on? 12.3.0

To Reproduce Review this unsigned auth credential

AAAAAQAAAAF6gVF2E0+pwV5vWTPtG5VyHJ/X54JHeZUczj7CnvD123q5eZAtqGIwAAAAAAAAAAE=

Now review that same credential after it's been signed

AAAAAQAAAAF6gVF2E0+pwV5vWTPtG5VyHJ/X54JHeZUczj7CnvD123q5eZAtqGIwAAJ7YwAAABAAAAABAAAAAQAAABEAAAABAAAAAQAAABAAAAABAAAAAgAAAA8AAAAHRWQyNTUxOQAAAAANAAAAIGsGKxYxF08hS1ap4iEHm23VophJPlpB5uFAHcIbUBDSAAAAEAAAAAEAAAACAAAADwAAAAdFZDI1NTE5AAAAAA0AAABAtUp2f4JU119X1pK1j641RzQO7xJnpE+Qj4NIfCKQwfvLTtAJD4bLiiSR+rZjIjcRjak2TvQua+4zEzK2FqdKBg==

An unsigned credential will mark

entry.credentials().switch() === xdr.SorobanCredentialsType.sorobanCredentialsAddress()

as true But signed will mark that same check as false

Expected behavior Signed auth entries should not change their underlying credential type.

Additional context This issue breaks this check if you're passing in an entry which has already been signed. Thus breaking multisig scenarios for custom accounts https://github.com/stellar/js-stellar-sdk/blob/master/src/contract/assembled_transaction.ts#L839-L842

kalepail commented 1 month ago

"Fun" fact if you change the check to

entry.credentials().switch().name === 'sorobanCredentialsAddress'

it will pass. So likely a type issue. Especially given that once you do that further checks down the stack will fail like:

const authEntryAddress = Address.fromScAddress(
  entry.credentials().address().address(),
).toString();

will fail with

Uncaught (in promise) Error: Unsupported address type
    at Function.fromScAddress (@stellar_stellar-sdk_contract.js?v=c0695b00:11771:21)
    at _AssembledTransaction.signAuthEntries (@stellar_stellar-sdk_contract.js?v=c0695b00:23789:59)
    at PasskeyKit.sign (kit.ts:346:19)
    at HTMLButtonElement.policyTransfer (App.svelte:332:17)
ooochoche commented 1 month ago

@kalepail Can I work on this?

kalepail commented 1 month ago

The way I'm getting around this is just by cloning the credentials XDR. I don't know what that works but it seems to. e.g. const credentials = xdr.SorobanCredentials.fromXDR(entry.credentials().toXDR())

chadoh commented 1 month ago

Added @kalepail's workaround to the implementation of signAuthEntries in #1044