bitwarden / mobile

Retired Bitwarden mobile app for iOS and Android (MAUI/Xamarin).
https://bitwarden.com
GNU General Public License v3.0
140 stars 21 forks source link

using a space at the beginning of otpauth:// generate a wrong OTP #2122

Open qKw2CBlAbP0nMElXrwC9So7UtfhZmvOxr4fLlq5 opened 2 years ago

qKw2CBlAbP0nMElXrwC9So7UtfhZmvOxr4fLlq5 commented 2 years ago

Steps To Reproduce

Bitwarden Android, web interface and Firefox's extension are affected

  1. Open Bitwarden
  2. clone a working entry that has a valid OTP
  3. edit the cloned entry putting a space at the beginning of otpauth://... like this: " otpauth://..."
  4. save

Expected Result

two possible results:

  1. a space before otpauth:// is wrong, bitwarden should not generate any OTP but display a warning on the wrong OTP field
  2. bitwarden deals with wrong characters like spaces before otpauth:// and generate a working OTP

Actual Result

a wrong OTP is generated

Screenshots or Videos

No response

Additional Context

I found it while copying and pasting the otpauth:// line. Bitwarden interface's extension is small and it's not easy to immediately find what is wrong

Operating System

Android

Operating System Version

10

Device

No response

Build Version

2022.9.1 (5047)

Beta

tangowithfoxtrot commented 1 year ago

Thanks, @luca-e075e!

I've confirmed the behavior and marked this as being reproducible internally.

xm0V06piUTf7bIS8C5za2WyTYrnkh35ESqYclAy commented 1 year ago

Is someone working on this? Can i work on this?

djsmith85 commented 1 year ago

@jayg2309 Thank you for your interest in contributing.

As a starting guide please have a look at our Contribution Guidelines. These will get you started with setting up your development environment and how to proceed with your contribution.

Please reference this issue when you create a pull request.

xm0V06piUTf7bIS8C5za2WyTYrnkh35ESqYclAy commented 1 year ago

Okay thankyou

SAXJoeyyPyV9dV0jjHM3iRfnnnUGxdo5cbOIATF commented 1 year ago

Is this sill an open issue? I was not able to reproduce it, as the new OTP is generated correctly after adding a space. Otherwise I am happy to help solve the issue :)

X0yhyFu7ZU7vjLibDW1lnv57gpRcdOdo8Zg7aRZ commented 12 months ago

I tested it on my iPhone and in the Chrome extension. In Both cases the issue is still happening. It is important that its an OTP that begins with otpauth://. @flooxo if you want to fix it, feel free to do that or I try to do this.

X0yhyFu7ZU7vjLibDW1lnv57gpRcdOdo8Zg7aRZ commented 11 months ago

I will wait some days and if you don't respond to this, I will fix this issue. I already know how to fix this, but if you want to fix it, I let you fix this.

SAXJoeyyPyV9dV0jjHM3iRfnnnUGxdo5cbOIATF commented 11 months ago

OK, thanks for the note. Feel free to fix it if you already know where in the code :)

X0yhyFu7ZU7vjLibDW1lnv57gpRcdOdo8Zg7aRZ commented 11 months ago

Ok, but I have to get the project up and running first on my PC.

SAXJoeyyPyV9dV0jjHM3iRfnnnUGxdo5cbOIATF commented 11 months ago

If you give me a hint where in the code something should be changed, I can do it too (it just saves time if i don't have to find out myself first). Whatever suits you best :)

X0yhyFu7ZU7vjLibDW1lnv57gpRcdOdo8Zg7aRZ commented 11 months ago

I try if the setup on my PC is easy, if it isn't easy I give you a hint so you can do it

X0yhyFu7ZU7vjLibDW1lnv57gpRcdOdo8Zg7aRZ commented 11 months ago

@flooxo Do you think its a good idea to remove the space at saving and at generation. At generating for existing entries and saving for new entries.

SAXJoeyyPyV9dV0jjHM3iRfnnnUGxdo5cbOIATF commented 11 months ago

Yeah, I guess the expected behavior when cloning an entry with an otp is to still have a valid otp path, right? So I would suggest to just trim the whitespaces

X0yhyFu7ZU7vjLibDW1lnv57gpRcdOdo8Zg7aRZ commented 11 months ago

I can't get xamarin running. @flooxo you can solve this issue. In this method the key must be trimmed because the StartwWith check don't work https://github.com/bitwarden/mobile/blob/793c5fef6f64b3c75b801874750d5ac26bcd5a9f/src/Core/Services/TotpService.cs#L21

You can also search for the code where a entry get saved and trim the string before saving

SAXJoeyyPyV9dV0jjHM3iRfnnnUGxdo5cbOIATF commented 11 months ago

Ok, thanks, I've already looked at that method. I'll have to take a closer look at where the key is saved, because wouldn't it be a better solution if it was already saved correctly beforehand?

X0yhyFu7ZU7vjLibDW1lnv57gpRcdOdo8Zg7aRZ commented 11 months ago

Yes, but I think trim it at the generation is also necessary for keys which are already stored wrong.

X0yhyFu7ZU7vjLibDW1lnv57gpRcdOdo8Zg7aRZ commented 11 months ago

@flooxo would you also try to fix this in the browser extensions? If not I can try it.

I think it's a good idea to create an new issue there and attach the link of this issue, so anyone has the context to the issue.

SAXJoeyyPyV9dV0jjHM3iRfnnnUGxdo5cbOIATF commented 11 months ago

Sure, i'll give it a try. Thanks :)