Fix: get the top-level hostname #4935
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Passing run #8019 ↗︎
Details:
Review all test suite changes for PR #4935 ↗︎ |
||||||||||||||||||||
gilluminate
force-pushed
the
PROD-2126-top-level-cookie-domain
branch
from
b34e7aa
to
e41d864
Compare
NevilleS
approved these changes
May 30, 2024
NevilleS
reviewed
May 30, 2024
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
If this exits and fails to save a cookie, what should we do?
There was a problem hiding this comment.
I wondered about throwing an error, but I thought the only real world scenario where this happens is if the browser is blocking 1st party cookies. Should we do something a little more invasive, like a browser alert to let the user know it's not going to work?
There was a problem hiding this comment.
Actually, I propose we solve that separately. fides.js should probably check to see if cookies can be written prior to showing a banner at all. We should be thoughtful about how we want to handle it in that initialization phase.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes PROD-2126
Description Of Changes
IP addresses and top level hostnames that included multiple TLDs (like
co.uk) were not working for setting a Fides Cookie.This fix update cookie setter to guarantee highest level domain allowed to set cookie. It relies on browser's own methods to determine viable domain.
Code Changes
Steps to Confirm
/etc/hostson your local machine to include new line:127.0.0.1 privacy.example.co.ukhttp://privacy.example.co.uk:3001/fides-js-demo.htmlexample.co.ukhttp://127.0.0.1:3001/fides-js-demo.htmland ensure that works.(domains like
www.fides.comandlocalhostshould continue working with this enhancement in place)Pre-Merge Checklist
CHANGELOG.md