Skip to content
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

Scanning QR code with certain characters in memo fails #1678

Closed
Tomas-M opened this issue Nov 14, 2024 · 7 comments
Closed

Scanning QR code with certain characters in memo fails #1678

Tomas-M opened this issue Nov 14, 2024 · 7 comments
Labels
invalid This doesn't seem right

Comments

@Tomas-M
Copy link

Tomas-M commented Nov 14, 2024

When I use zashi wallet for Android and I scan a QR code while sending money, it works most of the time, but fails when the memo includes certain characters in certain order.

For example, using memo as A<B or B>A works fine, but using memo as A<>B fails with message Error parsing query parameters.

So, if the following text is in QR code, it works:
zcash:u19spl3y4zu73twemxrzm33tm3eefepecv4zdssn0hfd4tjaqpgmlcm9nhyjqlvaytwpknqjqctvdscjmg47ex20j03cu4gx3zmy26y2hunpenvw083dmtlq4y7re5rwsygpteq57wwllr3zhs4rw43j5puxgrcqdq4f9dd38qksl4f9p2hc7x3kj582zdjxsnj8urmnc3msfjw72kej0?amount=0.01&memo=QT5C

If the following text is in QR code, it works still:
zcash:u19spl3y4zu73twemxrzm33tm3eefepecv4zdssn0hfd4tjaqpgmlcm9nhyjqlvaytwpknqjqctvdscjmg47ex20j03cu4gx3zmy26y2hunpenvw083dmtlq4y7re5rwsygpteq57wwllr3zhs4rw43j5puxgrcqdq4f9dd38qksl4f9p2hc7x3kj582zdjxsnj8urmnc3msfjw72kej0?amount=0.01&memo=QTxC

But If the following text is in QR code, it fails
zcash:u19spl3y4zu73twemxrzm33tm3eefepecv4zdssn0hfd4tjaqpgmlcm9nhyjqlvaytwpknqjqctvdscjmg47ex20j03cu4gx3zmy26y2hunpenvw083dmtlq4y7re5rwsygpteq57wwllr3zhs4rw43j5puxgrcqdq4f9dd38qksl4f9p2hc7x3kj582zdjxsnj8urmnc3msfjw72kej0?amount=0.01&memo=QTw+Qg

To test, you can scan this QR code, which works:

works

And try this one which fails:

fails

I would expect that both QR codes would be readable, so when scanning the failed QR code I would expect it fills in the memo A<B>C

It is not a problem of < and > only, there are also problems with other characters, for example question mark followed by a letter seems to have problems, $ and % may have also some problems, etc. I assume there are more cases which will fail. I would expect that all ascii characters will be parsed correctly in all times.

  • App Version: 1.2.1 (760)
  • Android Version: 14
  • Device: (if applies) Pixel 6a
@Tomas-M Tomas-M added the bug Something isn't working label Nov 14, 2024
@HonzaR
Copy link
Collaborator

HonzaR commented Nov 14, 2024

Hi @Tomas-M, Thank you for providing all the details about the issue! We'll check it and get back to you soon.

@HonzaR
Copy link
Collaborator

HonzaR commented Nov 14, 2024

We can reproduce the described issue. Zashi prompts users with this error handling in such a case:

@Tomas-M
Copy link
Author

Tomas-M commented Nov 14, 2024

I just noticed this ZIP
https://zips.z.cash/zip-0321

It looks like there is something different in base64 encoding, and my payment URL with + sign in memo (base64 encoded) is invalid.

The payment URL should be
zcash:u19spl3y4zu73twemxrzm33tm3eefepecv4zdssn0hfd4tjaqpgmlcm9nhyjqlvaytwpknqjqctvdscjmg47ex20j03cu4gx3zmy26y2hunpenvw083dmtlq4y7re5rwsygpteq57wwllr3zhs4rw43j5puxgrcqdq4f9dd38qksl4f9p2hc7x3kj582zdjxsnj8urmnc3msfjw72kej0?amount=0.01&memo=QTw-Qg

However the iOS zashi app accepts it even with +. Not sure why.
Anyway, this issue should probably be closed, sorry, it was my mistake.

@HonzaR
Copy link
Collaborator

HonzaR commented Nov 19, 2024

@Tomas-M Thanks for the update. We'll check the related Zip321 URI logic.

@daira
Copy link

daira commented Nov 20, 2024

The spec is clear:

"base64url" is defined as in [4] with padding omitted. (Note that this uses a different alphabet to the usual base64; the values 62 and 63 in the alphabet are encoded as - and _ respectively. Implementations MUST NOT accept the characters +, /, and = that occur only in the usual base64.)

[4] RFC 4648 section 5: Base64 Encoding with URL and Filename Safe Alphabet

If Zashi iOS accepts +, that's a bug relative to ZIP 321 (filed as Electric-Coin-Company/zashi-ios#1420).

In general we do not follow the Postel-era RFC "Be generous in what you accept." dictum when writing or implementing Zcash specs —or at least we don't intend to— because it demonstrably results in significant security and interoperability problems over the long term. For discussion of this see RFC 9413: Maintaining Robust Protocols, originally drafted with less weasel-wording and under the better title "The Harmful Consequences of the Robustness Principle" (which ruffled a few political feathers with the IETF old-timers, sigh).

Among other benefits, such as easier security analysis, that practice was intended to prevent situations such as that encountered in this issue: someone testing on one implementation (Zashi iOS) and then getting inconsistent behaviour on another (Zashi Android). In this case interoperability problems could also occur if filename-unsafe characters are mangled by some transmission channels, or inconsistently converted to/from a QR code. If we accepted the wider set of characters then we'd have to test all those cases (or whatever subset of them we were able to think of), and would be imposing a similar testing burden on other implementors.

@daira daira changed the title Scaning QR code with certain characters in memo fails Scanning QR code with certain characters in memo fails Nov 20, 2024
@daira daira added invalid This doesn't seem right and removed bug Something isn't working labels Nov 20, 2024
@daira daira closed this as completed Nov 20, 2024
@Tomas-M
Copy link
Author

Tomas-M commented Nov 20, 2024

@daira Thank you for clarification regarding your view on the dictum. Considering this, the iOS Zashi wallet does not follow this, since it perfectly accepts memo with pure base64 encoding (buggy relative to ZIP 321) even without the replaced + / =
You can test by scanning the QR code above. Furthermore it has a bug complaining that valid address is invalid while scanning QR in iOS Zashi wallet, I've already opened an issue for that, but that is another problem.

@daira
Copy link

daira commented Nov 20, 2024

@daira Thank you for clarification regarding your view on the dictum. Considering this, the iOS Zashi wallet does not follow this, since it perfectly accepts memo with pure base64 encoding (buggy relative to ZIP 321) even without the replaced + / =
You can test by scanning the QR code above.

Filed as zashi-ios#1420, thanks.

Furthermore it has a bug complaining that valid address is invalid while scanning QR in iOS Zashi wallet, I've already opened an issue for that, but that is another problem.

For reference this is zashi-ios#1410.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

3 participants