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

Ensure that an OTP's issuer and label values are escaped correctly #391

Merged
merged 3 commits into from
Apr 7, 2024

Conversation

josh-
Copy link
Contributor

@josh- josh- commented Mar 8, 2022

Summary

This PR fixes/implements the following bugs/features:

  • Creating an OTP with an issuer/label that contains a space or other restricted characters does not properly escape the issuer/label

What existing problem does the pull request solve?**

Currently, if you create an OTP with an issuer that contains a space, for example:

var oneTimePassword = new OneTimePassword {
	Issuer = "Google Google",
	Label = "test@google.com",
	Secret = "pwq6 5q55"
};

and then call ToString on that oneTimePassword, that will return:

otpauth://totp/Google Google:test@google.com?secret=pwq65q55&issuer=Google%20Google

(note the first "Google Google" is not escaped, like the second one in the issuer parameter)

which then looks like this in Google Authenticator after scanning the resulting QR code:

File

This PR resolves this issue by correctly escaping both instances of the issuer in the URL, which then appears correctly in Google Authenticator:

otpauth://totp/Google%20Google:test@google.com?secret=pwq65q55&issuer=Google%20Google

Test plan

Test cases have been added.

Closing issues

N/A

@prvacy
Copy link

prvacy commented Mar 9, 2022

I discovered the same problem yesterday, thank you for the fix! Also, MS Authenticator does not support unescaped URLs, so that fix is really important.

@tom-156842
Copy link

Thanks for this fix, it addresses issue #375.

I suggest also escaping the Label value, since an email address can contain / and ? characters.

@josh-
Copy link
Contributor Author

josh- commented Mar 15, 2022

Thanks for this fix, it addresses issue #375.

I suggest also escaping the Label value, since an email address can contain / and ? characters.

Thanks @tom-156842, done 👍

@josh- josh- changed the title Ensure that an OTP's issuer is correctly escaped when it contains a space Ensure that an OTP's issuer and label values are escaped correctly Mar 15, 2022
@hdocsek
Copy link

hdocsek commented Oct 31, 2022

@codebude Do you have any update on when this fix will be released?

Copy link

@doggy8088 doggy8088 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@codebude

@Shane32
Copy link
Contributor

Shane32 commented Apr 6, 2024

All changes here look good. Generated QR codes scan properly with Google Authenticator. Tests have already been added 👍

@codebude codebude merged commit 56ace43 into codebude:master Apr 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants