Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix error from personal key validation unexpected characters #7070

Merged
merged 1 commit into from
Oct 5, 2022

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Oct 3, 2022

🛠 Summary of changes

Fixes an error which occurs when the user enters an invalid (non-alphanumeric) character in the personal key step of the identity verification flow.

The base32-decode library used in @18f/identity-personal-key-input throws an error when encountering an unexpected character. Rather than letting these errors throw unhandled, catch them and treat them as equivalent to an invalid entry.

NewRelic: https://onenr.io/0bRmopb8BQy

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • rspec './spec/features/idv/steps/confirmation_step_spec.rb[1:1:2:2]' passes (fails on main with changes to spec)

@aduth
Copy link
Contributor Author

aduth commented Oct 3, 2022

Hm, based on the error in NewRelic seemingly not specifying the character (and there are errors which do), I wonder if this is caused by the user using whitespace as delimiters for code fragments. It seems like we should be tolerant to that, similar to how we're tolerant to the optional use of dashes?

e.g. 1234-ABCD-5678-EFGH and 1234ABCD5678EFGH and 1234 ABCD 5678 EFGH would all be considered valid.

@aduth
Copy link
Contributor Author

aduth commented Oct 3, 2022

Hm, based on the error in NewRelic seemingly not specifying the character (and there are errors which do), I wonder if this is caused by the user using whitespace as delimiters for code fragments. It seems like we should be tolerant to that, similar to how we're tolerant to the optional use of dashes?

e.g. 1234-ABCD-5678-EFGH and 1234ABCD5678EFGH and 1234 ABCD 5678 EFGH would all be considered valid.

This is a bit trickier to implement than I'd expected, due to a combination of (a) the field being limited to 16 characters and (b) the Cleave.js formatting allowing a single delimiter (not either spaces or dashes). We probably need to prevent the user from entering certain characters, but this too isn't straightforward with Cleave.js (related issue).

I confirmed that this error--while noisy in NewRelic--does not appear to have an impact on the user-facing validation, and invalid characters are treated as equivalent to an invalid submission (error message appears as expected). So without that urgency, I'll plan to revert this back to draft to try to come up with a more robust handling of characters.

@aduth aduth marked this pull request as draft October 3, 2022 13:37
changelog: Bug Fixes, Identity Verification, Fix error when entering invalid characters for personal key
@aduth aduth force-pushed the aduth-personal-key-invalid-char branch from 585b5cc to 9d84938 Compare October 4, 2022 17:20
@aduth
Copy link
Contributor Author

aduth commented Oct 4, 2022

I'll plan to revert this back to draft to try to come up with a more robust handling of characters.

On second thought, I'm not going to spend much effort enhancing this behavior if the plan is to remove the confirmation modal altogether (started in #6821). The changes here should silence the errors in NewRelic at least.

@aduth aduth marked this pull request as ready for review October 4, 2022 17:20
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

@aduth aduth merged commit c75f075 into main Oct 5, 2022
@aduth aduth deleted the aduth-personal-key-invalid-char branch October 5, 2022 17:52
jskinne3 pushed a commit that referenced this pull request Oct 12, 2022
changelog: Bug Fixes, Identity Verification, Fix error when entering invalid characters for personal key
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants