Yatekii / qrbill

2 stars 3 forks source link

Cannot create ISO 11649 references longer than 19 characters #4

Closed jacg closed 3 months ago

jacg commented 3 months ago

The maximum permitted ISO 11649 reference length is 25 characters, but here creation fails if the length exceeds 19 characters.

This happens because the implementation tries to parse the concatenated base 36 digit-values into a single u64 and all those digits represent an integer which exceeds u64::MAX.

I have added some tests demonstrating this problem here: https://github.com/jacg/qrbill/commit/0ef1c078063fcea0ff6a5326915f36144aff9119.

I had some code I hacked together a while ago, for creating ISO 11649 references. There is a proof of concept of using this code to replace the buggy ISO 11649 code here: https://github.com/jacg/qrbill/commit/f26244ffe2d94def6564370a871bed3cccd1182c.

There are some significant differences in the requirements / assumptions between your version and mine:

The motivation for my choices was to make it as easy as possible for the user to create the references from high-level information, without having to bother with such low-level details. But I appreciate that you may wish to have strict limitations on the input and delegate pre-verification and pre-sanitization to Someone Else. It would be no problem for me to use my code to generate the pre-sanitized input to feed into your strict garbage-rejecting interface.

However, that still leaves the bug that this issue brings to attention. [Edit: but there is the trivial fix: #5] [Further edit: the trivial fix doesn't fix it fully. See #18.]

I would be happy to donate my code to your project, in order to fix this bug, but the quickest way (essentially, as shown in the POC liked above) would make the interface more tolerant of dodgy inputs. If you prefer the stricter interface, I could try to adapt my code to that, but it would take me longer to complete it, and I might not get around to completing it for a while.

Perhaps it would make sense to have both my convenient massage-the-input-into-something-acceptable interface next to your strict accept-no-nonsense interface.

jacg commented 3 months ago

Hah! Of course, the bug is fixed trivially by parsing into u128 instead of u64: #5 !

jacg commented 3 weeks ago

the bug is fixed trivially by parsing into u128

Nope! It merely kicks the can a little further down the road: u128 is not large enough to accept all valid inputs.

PR coming up with: