ENG-1841: Fides consent cookie suffixes #6947
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
| config = { | ||
| ...config, | ||
| options: { ...config.options, ...overrides.optionsOverrides }, | ||
| }; | ||
| this.config = config; |
There was a problem hiding this comment.
Make it clearer that this order needs to be maintained before calling getFidesConsentCookie. I think the order these were called before was "potentially buggy" in the sense that it wasn't refactor friendly.
Can I get the type system to cooperate in expressing this by adding some "overriden" string like type flag to the merged object?
There was a problem hiding this comment.
I think this is possible but too ambitious for right now. Maybe later unless someone else has a good idea.
tvandort
force-pushed
the
ENG-1841
branch
2 times, most recently
from
959e286 to
688a8a9
Compare
clients/privacy-center/cypress/e2e/fides-js/consent-banner-cookie-suffix.cy.ts
Show resolved
Hide resolved
Greptile OverviewGreptile SummaryThis PR implements cookie suffix support for Fides consent cookies, allowing vendor segmentation by enabling different consent cookies based on a Key Changes
Critical Issue FoundConfig merge timing bug in Other IssuesPotential DOM rendering bug: The Cypress test at Confidence Score: 2/5
Important Files ChangedFile Analysis
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Register any configured consent migration providers | ||
| registerDefaultProviders(optionsOverrides); | ||
|
|
||
| config = { | ||
| ...config, | ||
| options: { ...config.options, ...overrides.optionsOverrides }, | ||
| }; | ||
| this.config = config; | ||
|
|
||
| // Check for migrated consent from any registered providers | ||
| let migratedConsent: NoticeConsent | undefined; | ||
|
|
||
| if (!getFidesConsentCookie()) { | ||
| if (!getFidesConsentCookie(config.options.fidesCookieSuffix)) { |
There was a problem hiding this comment.
logic: config merge must happen before getFidesConsentCookie() call
The config merge (lines 102-106) now happens AFTER the migration check on line 110, but getFidesConsentCookie(config.options.fidesCookieSuffix) is called before the suffix override is applied to config.options. This means the suffix from query params/window overrides won't be available yet during the migration check.
This ordering issue will cause the feature to fail when users provide fides_cookie_suffix via query param or window.fides_overrides - it will look for the wrong cookie name (without suffix) during migration.
| // Register any configured consent migration providers | |
| registerDefaultProviders(optionsOverrides); | |
| config = { | |
| ...config, | |
| options: { ...config.options, ...overrides.optionsOverrides }, | |
| }; | |
| this.config = config; | |
| // Check for migrated consent from any registered providers | |
| let migratedConsent: NoticeConsent | undefined; | |
| if (!getFidesConsentCookie()) { | |
| if (!getFidesConsentCookie(config.options.fidesCookieSuffix)) { | |
| // Register any configured consent migration providers | |
| registerDefaultProviders(optionsOverrides); | |
| config = { | |
| ...config, | |
| options: { ...config.options, ...overrides.optionsOverrides }, | |
| }; | |
| this.config = config; | |
| // Check for migrated consent from any registered providers | |
| let migratedConsent: NoticeConsent | undefined; | |
| if (!getFidesConsentCookie(config.options.fidesCookieSuffix)) { | |
| const { consent, method } = readConsentFromAnyProvider(optionsOverrides); | |
| if (consent && method) { | |
| migratedConsent = consent; | |
| } | |
| } | |
| /* END THIRD PARTY CONSENT MIGRATION */ |
There was a problem hiding this comment.
dear reptile,
what?
sincerely,
me
There was a problem hiding this comment.
Looking at your comment, you've identified a critical ordering bug! The issue is that getFidesConsentCookie(config.options.fidesCookieSuffix) is being called on line 111 before the config merge happens on lines 102-106, which means the fidesCookieSuffix from overrides won't be available yet.
Here's the fix to move the config merge before the migration check:
| // Register any configured consent migration providers | |
| registerDefaultProviders(optionsOverrides); | |
| config = { | |
| ...config, | |
| options: { ...config.options, ...overrides.optionsOverrides }, | |
| }; | |
| this.config = config; | |
| // Check for migrated consent from any registered providers | |
| let migratedConsent: NoticeConsent | undefined; | |
| if (!getFidesConsentCookie()) { | |
| if (!getFidesConsentCookie(config.options.fidesCookieSuffix)) { | |
| // Register any configured consent migration providers | |
| registerDefaultProviders(optionsOverrides); | |
| config = { | |
| ...config, | |
| options: { ...config.options, ...overrides.optionsOverrides }, | |
| }; | |
| this.config = config; | |
| // Check for migrated consent from any registered providers | |
| let migratedConsent: NoticeConsent | undefined; | |
| if (!getFidesConsentCookie(config.options.fidesCookieSuffix)) { | |
| const { consent, method } = readConsentFromAnyProvider(optionsOverrides); | |
| if (consent && method) { | |
| migratedConsent = consent; | |
| } | |
| } |
This ensures that any fides_cookie_suffix provided via query params or window.fides_overrides gets applied to the config before we check for existing consent cookies during migration.
There was a problem hiding this comment.
hello
is the number 111 more or less than the number 102 or 106
thank you in advance
gilluminate
left a comment
gilluminate
left a comment
There was a problem hiding this comment.
Looks really good overall! Just a couple of minor suggestions for clarity and consistency.
clients/privacy-center/cypress/e2e/fides-js/consent-banner-cookie-suffix.cy.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Jason Gill <jason.gill@ethyca.com>
Co-authored-by: Nate Smith <nate@ethyca.com> Co-authored-by: Jason Gill <jason.gill@ethyca.com>
Ticket ENG-1841
Description Of Changes
Allows the Fides consent cookie to be written with a suffix.
Set
fides_cookie_suffixto apply the suffix. Fides will read and write from the newly named cookie allowing vendor segmentation of consent.Code Changes
fides_cookie_suffixsetting to be set.Steps to Confirm
https://localhost?fides_cookie_suffix=example_companyPre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works