Fix handling of some ISO-3166 geolocation edge cases in Privacy Center /fides.js endpoint #4858
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
NevilleS
changed the title
NevilleS
changed the title
Passing run #7594 ↗︎
Details:
Review all test suite changes for PR #4858 ↗︎ |
|||||||||||||||
|
@NevilleS this all looks good, but we also do a validation on the |
| // 3) Separated by a dash (e.g. "US-CA") | ||
| const VALID_ISO_3166_LOCATION_REGEX = /^\w{2,3}(-\w{2,3})?$/; | ||
| const VALID_ISO_3166_LOCATION_REGEX = /^\w{2,3}(-\w{1,3})?$/; |
There was a problem hiding this comment.
nit: technically the \w metacharacter in regex would match some values that are invalid, e.g. _ and also country codes that starts with a digit. For the country part only (the region part is good, of course)
This is how the regex was already built, so it doesn't make anything worse as written. It's also of minimal risk IMO, but I just wanted to call it out.
The fix would be to replace your \w metacharacters with a set: [A-Za-z]
There was a problem hiding this comment.
Might also worth adding it to the unit tests 😄
There was a problem hiding this comment.
Fair- however, the true spec is "alphanumeric" characters (at least for the region codes, aka subdivisions), where numeric codes are definitely allowed. However, not just all "non-whitespace" characters!
There's some wiggle room in this regex to be more permissive, but tightening it up here might make sense...
I'll definitely add some test cases and then decide
There was a problem hiding this comment.
Er my last comment is a bit backwards, low reading comprehension there - but yes, I do think I should reject numeric country codes, that's just another subtle footgun!
There was a problem hiding this comment.
OK, updated! I think it's preferable to be defensive here and not overly permissive - the change to discard invalid region headers is there as a safety hatch, but I think it's good to be strict about the ?geolocation= param since that's a user-facing API
…r invalid numeric country codes, valid three-letter countries, etc.
I don't think it was ever really needed on those pages... removing for consistency, so that our tests don't silently discard that and hide potential issues...! Pushed up and re-running. |
There was a problem hiding this comment.
Approved without looking too closely at the validation specifics since @RobertKeyser is the expert there.
…r /fides.js endpoint (#4858)
Closes PROD-2015
Description Of Changes
This fixes a subtle error where the Privacy Center would consider some valid ISO-3166 codes as invalid if they had single-character regions; for example,
SE-Owould be rejected. The fix here is trivial (accept 1-3 characters!) but this PR also adds additional test coverage for confidence, and also adds an extra defensive check to fallback to just the country header if an invalid CloudFront region header is detected.Code Changes
geolocation.tsSteps to Confirm
Pre-Merge Checklist
CHANGELOG.md