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

Escape RegExp characters in e-mail in keyCheck() function #92

Closed
kylekatarnls opened this issue Jul 23, 2020 · 6 comments · Fixed by #93
Closed

Escape RegExp characters in e-mail in keyCheck() function #92

kylekatarnls opened this issue Jul 23, 2020 · 6 comments · Fixed by #93

Comments

@kylekatarnls
Copy link
Contributor

kylekatarnls commented Jul 23, 2020

Hello!

By looking at the code at https://github.com/ProtonMail/pmcrypto/blob/master/lib/key/check.js#L18 I assume that if info.user.userId ends with "<jackyblack@foo.com>" and email is "jack.black@foo.com", it would match as . is any character in regular expression.

I think this would trigger unwanted behaviors.

If my analyse sounds correct to you, you can assign this issue to me and I will propose a PR to escape this email variable.

@twiss
Copy link
Member

twiss commented Jul 23, 2020

Hey 👋 Good catch! Yeah, a PR would be welcome. Instead of escaping the regex, you could also just check directly whether the string contains the email (e.g. using indexOf or lastIndexOf).

@kylekatarnls
Copy link
Contributor Author

kylekatarnls commented Jul 23, 2020

I guess pmcrypto aims to be compatible out of the box with IE, but with a String.prototype polyfill, we could use endsWith which would be the more appropriate tool IMO.

@kylekatarnls
Copy link
Contributor Author

I see Array.prototype.includes is used in check.js, so I assume String.prototype.endsWith can be used too as it's the same range of compatibility.

@twiss
Copy link
Member

twiss commented Jul 23, 2020

Unfortunately we can't, because while array.includes is polyfilled in openpgp.js, a dependency of pmcrypto (https://github.com/openpgpjs/openpgpjs/blob/e29de76dc1d86a25b3fe6d20c7dc21c78e8c9ec2/src/polyfills.js#L27-L29); string.endsWidth isn't. We could of course add it there or here, but for just a single use I think it's probably not really worth it .-.

@kylekatarnls
Copy link
Contributor Author

kylekatarnls commented Jul 23, 2020

No problem, .substr() will do the job. I think it's faster than indexOf has this last one would read the whole string.

@twiss
Copy link
Member

twiss commented Jul 23, 2020

👍 Good point :) Thanks for the PR! I will look at it soon(TM).

@twiss twiss closed this as completed in #93 Jul 30, 2020
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 a pull request may close this issue.

2 participants