Skip to content
This repository has been archived by the owner on Jun 17, 2022. It is now read-only.

Update color-password.pipe.js to handle Unicode/Emoji correctly accross platforms. #354

Merged
merged 1 commit into from Apr 22, 2021

Conversation

gryffs
Copy link
Contributor

@gryffs gryffs commented Apr 22, 2021

Original PR on Desktop:

Issues this PR is related to:

The issue is associated with an angular pipe ColorPasswordPipe used for sanitizing HTML and colorizing the password. Unicode characters do not play well with this Pipe because of issues with Unicode, ie: "💩".length === 2, and how it parsed through a basic string. I've updated the Pipe to use Array.from because it is a simple approach that will keep Unicode characters properly grouped. I've also set the type for emoji in case there are any desires to implement specific styling.

I've noticed that this issue does not exist on Web because Web does not implement the ColorPasswordPipe (maybe it should be implemented?). I confirmed locally that this fix works both on browser and Desktop.

Copy link
Contributor

@cscharf cscharf left a comment

Choose a reason for hiding this comment

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

Thanks @gryffs , looks good!

@Pipe({ name: 'colorPassword' })
export class ColorPasswordPipe implements PipeTransform {
transform(password: string) {
// Regex Unicode property escapes for checking if emoji in passwords.
const regexpEmojiPresentation = /\p{Emoji_Presentation}/gu;
Copy link
Contributor

Choose a reason for hiding this comment

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

crazy new world emojis... tested and verified using

/\p{Emoji_Presentation}/gu.test('sadf435😟😡🥺retgstt4R#$%^$%ETYERGSERTYRHD🥰FStgfdfsdgsfg');

@cscharf cscharf merged commit b6f1029 into bitwarden:master Apr 22, 2021
@cscharf
Copy link
Contributor

cscharf commented Apr 22, 2021

@gryffs , would you mind updating jslib submodule to desktop and browser when you get a chance, that is the final step to pull these changes in there.

@gryffs gryffs deleted the password-unicode branch April 23, 2021 03:24
@eliykat
Copy link
Member

eliykat commented Jun 3, 2021

Hey @gryffs, this fix has been passed by our QA team for the Angular clients (desktop, browser and web). Thanks again for the PR.

However, QA noticed that the issue (bitwarden/desktop#768) is still present in mobile. Android displays "???" instead of the emoji, and iOS crashes. Would you be interested in submitting a PR for mobile? Totally up to you - I'm happy to do it if it's not in your wheelhouse or you don't have time.

@gryffs
Copy link
Contributor Author

gryffs commented Jun 4, 2021

@eliykat thanks for reaching out! I'm currently on vacation and can't look at this until the 18th. If it can wait, I'll happily work through it, but if you need to get this issue fully resolved, please feel free to make the changes. Thanks again!

@eliykat
Copy link
Member

eliykat commented Jun 6, 2021

@gryffs Thanks! I think it can wait until then if you're still willing to help out. (and of course, no work on holidays!) Let me know how you go.

eliykat added a commit that referenced this pull request Jul 5, 2021
…ly accross platforms. (#354)"

This reverts commit b6f1029.
Reason: incompatible with FF <= 77
eliykat added a commit that referenced this pull request Jul 5, 2021
… correctly accross platforms. (#354)""

This reverts commit 4a0b264.
eliykat added a commit that referenced this pull request Jul 6, 2021
…424)

* Revert "Update color-password.pipe.js to handle Unicode/Emoji correctly accross platforms. (#354)"

This reverts commit b6f1029.
Reason: incompatible with FF <= 77

* Revert "Revert "Update color-password.pipe.js to handle Unicode/Emoji correctly accross platforms. (#354)""

This reverts commit 4a0b264.

* Transpile unicode property escape regex

For compatibility with <= FF 77 and other older browsers

* Fix linting
eliykat added a commit that referenced this pull request Jul 6, 2021
…424)

* Revert "Update color-password.pipe.js to handle Unicode/Emoji correctly accross platforms. (#354)"

This reverts commit b6f1029.
Reason: incompatible with FF <= 77

* Revert "Revert "Update color-password.pipe.js to handle Unicode/Emoji correctly accross platforms. (#354)""

This reverts commit 4a0b264.

* Transpile unicode property escape regex

For compatibility with <= FF 77 and other older browsers

* Fix linting
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants