Ignore invalid three-character country codes for FidesJS geolocation (e.g. "USA") #4877
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Passing run #7675 ↗︎
Details:
Review all test suite changes for PR #4877 ↗︎ |
||||||||||||||||||||
| .fides-acknowledge-button-container { | ||
| margin-bottom: 0px; | ||
| } |
There was a problem hiding this comment.
Unrelated, but this is invalid CSS and prints a console error in the privacy center unit tests if you look closely at them: https://github.com/ethyca/fides/actions/runs/9008947049/job/24752130402#step:8:18
There was a problem hiding this comment.
speaking of which, have we considered using sass for fides.js?
There was a problem hiding this comment.
Sure! The postprocessor we use in rollup supports lots of plugins 👍
There was a problem hiding this comment.
apparently this is supposedly valid now. https://caniuse.com/?search=css%20nesting
| * convenience. | ||
| */ | ||
| export const VALID_ISO_3166_LOCATION_REGEX = | ||
| /^(?:([a-z]{2})(-[a-z0-9]{1,3})?|(eea))$/i; |
There was a problem hiding this comment.
This regex is getting a bit more complicated but really only because of the () characters that add some visual noise. You can try it here to see more of an explanation: https://regex101.com/r/lYuAGj/2
There was a problem hiding this comment.
It looks like this:
^(?:...)$: outer, non-capturing group (that's the?:to ensure we test the whole string from ^ -> $([a-z]{2}): required 2-letter country code(-[a-z0-9]{1,3})?optional 1-3 alphanumeric region code|(eea)alternate support foreeaas a special-case match
| // DEFER: return geoLocation.country when BE supports filtering by just country | ||
| // see https://github.com/ethyca/fides/issues/3300 |
There was a problem hiding this comment.
This has been supported for ages, just fixing the comment
| @@ -86,7 +111,6 @@ describe("getGeolocation", () => { | |||
| const tests = [ | |||
| { input: { country: "US", region: undefined }, expected: "US" }, | |||
| { input: { country: "us", region: undefined }, expected: "us" }, | |||
| { input: { country: "USA", region: "NY" }, expected: "USA-NY" }, | |||
…ent-3-char-country-codes
…ent-3-char-country-codes
| const req = createRequest({ | ||
| url: "https://privacy.example.com/fides.js?geolocation=USA", | ||
| }); | ||
| const geolocation = await lookupGeolocation(req); | ||
| expect(geolocation).toBeNull(); |
There was a problem hiding this comment.
Is there not a reason to test USA-NY also, since that was removed above? Or do you think that's a redundant test?
There was a problem hiding this comment.
Nope, more tests are totally reasonable! If a second engineer thinks a test helps I always prefer to add it - modifying now...
|
Updated! |
Closes PROD-2063
Description Of Changes
This makes FidesJS and the Privacy Center slightly more defensive against invalid ISO 3166-2 codes that must use two-character country codes (e.g.
USvsUSA). Technically there are ISO 3166-1 alpha-3 codes that are three letter codes, but these are meant for other purposes (like passports) and aren't used along with the ISO 3166-2 subdivisions, etc.More importantly, the Fides API expects only two-letter country codes compliant with ISO 3166-1 alpha-2, so this ensures that FidesJS & Privacy Center code will match the validation used in the API 👍
However, we do support a special-case
EEAlocation code, so this ensures that is still considered valid!Code Changes
EEAcodesSteps to Confirm
fides-jsandprivacy-centerprojects/fides.js?geolocation=USAroute does not return ageolocationobject in the generated bundlePre-Merge Checklist
CHANGELOG.md