search

stellar / django-polaris

An extendable Django app for building modular Stellar services
https://django-polaris.readthedocs.io
Apache License 2.0
94 stars 66 forks source link

SEP-12: default to memo type of ID #683

Open yuriescl opened 1 year ago

yuriescl commented 1 year ago

This might be a bug, or maybe it's intended behavior. When calling the SEP-12 PUT /customer endpoint, I noticed that if I use a SEP-10 token without a custodial memo to authenticate the request, it let's me use memo as one of the fields in the payload. Shouldn't the authenticated SEP-10 account/memo be required to match the PUT /customer account/memo?

How to reproduce:

  1. Generate a non-custodial SEP-10 token, that is, without a memo attached
  2. Use that SEP-10 token to call SEP-12 PUT /customer, and put memo=1 in the payload fields
  3. Polaris will accept the request and let me edit any customer by putting any memo= value in the payload
  4. Is this intended behavior?

Also, apparently I also have to set memo_type=id in the payload, otherwise Polaris will raise an error saying the memo does not match memo_type. memo_type should default to id in SEP-12 calls, right?

How to reproduce:

  1. Call SEP-12 PUT /customer with a memo=1 in the payload, but without memo_type
  2. Polaris will raise an error saying the memo doesn't match the memo_type
  3. Is this intended behavior?
JakeUrban commented 1 year ago

When calling the SEP-12 PUT /customer endpoint, I noticed that if I use a SEP-10 token without a custodial memo to authenticate the request, it let's me use memo as one of the fields in the payload.

This is the intended behavior, because SEP-31 senders don't include a memo in their SEP-10 challenge, since they're authenticating as themselves instead of as one of their users. Instead they have to specify the memos in their PUT /customer requests to identify their sending & receiving customers.

The downside is that this makes it possible for a SEP-6 wallet to omit their customer's memo in SEP-10. In reality though, this has minimal impact since anchors and custodial / omnibus wallets almost always have agreements and the wallet's public key is whitelisted on the anchor's backend.

Also, apparently I also have to set memo_type=id in the payload, otherwise Polaris will raise an error saying the memo does not match memo_type. memo_type should default to id in SEP-12 calls, right?

This sounds like a bug, thanks for finding.

yuriescl commented 1 year ago

This is the intended behavior

Got it, makes sense

This sounds like a bug, thanks for finding

Sure