Add canEncode validator for use in callback urls and bearer tokens#5920
Add canEncode validator for use in callback urls and bearer tokens#5920whpearson wants to merge 2 commits into
Conversation
| r"(?:#[\w\-._~%!$&'()*+,;=:@/?]*)?$", | ||
| message="Must be a valid https URL", | ||
| ), | ||
| CanEncode(message="Urls have to be capable of being encoded in latin-1"), |
There was a problem hiding this comment.
| CanEncode(message="Urls have to be capable of being encoded in latin-1"), | |
| CanEncode(message="URL cannot include Unicode characters"), |
There was a problem hiding this comment.
I don‘t know how much we care about technical correctness here but URLs can contain Unicode characters. Unicode contains basically all characters, including boring ones like a or 1.
When people try to use emoji in text messages we don’t get into the weeds about character sets. Instead we tell them which characters are the problem:
notifications-admin/app/main/validators.py
Lines 166 to 171 in 06fe9c3
There was a problem hiding this comment.
Unicode characters have to be percent encoded, so perhaps that should be the correct error message? Happy to highlight the characters that should be.
There was a problem hiding this comment.
Yeah, if the error message was something like
🤪 and ŵ must be percent-encoded in URLs
that would be better
There was a problem hiding this comment.
"You cannot use the following characters in URIs. These characters, ∆ or 📲, might be mis-encoded."
I kept close to the sms error. What do you think @quis @karlchillmaid ?
There was a problem hiding this comment.
How about this?
You cannot use ∆ in a web address. You must use percent encoding if you want to include this character in a URL.
You cannot use ∆ or 📲 in a web address. You must use percent encoding if you want to include these characters in a URL.
There was a problem hiding this comment.
I went with calling it a web address in in both places (for ease of coding/consistency)
There was a problem hiding this comment.
Removed the repetition now. So it is consistent with @karlchillmaid 's advice (in slack)
705b164 to
a7a9d8e
Compare
0609e2a to
be3a3b9
Compare
be3a3b9 to
0df34e1
Compare
To validate that we can encode with latin-1. This is needed to stop unicode errors we are seeing in notifications-api when attempting to send to these callback urls. Edited the validator to return a list of characters that cannot be validated. With @karlchillmaid for content work
0df34e1 to
efb4252
Compare
| assert mock_field.error_summary_messages == ["No sequences in %s please"] | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
These tests are good, but they only check that the CanEncode validator works as expected. They don’t test that it’s being used on the bearer token field.
There was a problem hiding this comment.
Pushed up a version that checks whether CallbackForms have the CanEncode validators attached to the fields. I'd rather not test the functionality twice (to avoid duplication of effort if it changed).
I couldn't find examples of this kind of code (our forms tests are quite small), so pointers on how to do it properly appreciated.
To avoid it getting left off.
To validate that we can encode with latin-1.
This is needed to stop unicode errors we are seeing in notifications-api when attempting
to send to these callback urls