Implemented fix for crash on unexpected QR code format

iKenndac / Tofu

An easy-to-use two-factor authentication app for iOS

https://tofuauth.com

ISC License

490 stars 64 forks source link

Implemented fix for crash on unexpected QR code format #41

Closed benbusby closed 3 years ago

benbusby commented 4 years ago

Hey again @calleerlandsson. Just noticed #40 and decided to take a quick stab at a fix for it. This seems to work for me -- the main changes are:

Added enforcement of 6/8 digit code length
Added rejection method (with error message) for attempts to add accounts that don't adhere to the current expected format

Let me know if there are any modifications needed!

calleluks commented 4 years ago

Hey @benbusby, thanks so much for working on this!

Overall the fix looks good. I a bit curious, though, where are we crashing when an account has 12 digits? Instead of preventing accounts with a different number of digits than 6 or 8, do you think we could fix that crash and thereby support 12 digit codes (at least when added via QR code)?

benbusby commented 4 years ago

Unfortunately I don't remember the exact details, but I vaguely recall it had something to do with the Password model using 32-bit integers for an operation (which might make sense for why it would crash in this case, since max UInt32 would still be less than any 12-digit number).

I can look into it some more later and see if there's a more elegant fix that would include support for 12 digit codes.

calleluks commented 3 years ago

Thanks again for your work on this @benbusby! I ended up solving this in a similar fashion in f43b08dce887226ff2f13dc0eb0b9b8a1bb2770e, 7a94ba60fbc414a7fe4f74810d45ab15e6adda1a and 1680d09e65962bf8089ec397b52a42efe6a59f43.