-
Notifications
You must be signed in to change notification settings - Fork 84
ENG-1933: Add support for cookie deletion using wildcards #7047
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Greptile OverviewGreptile SummaryAdds support for deleting cookies using wildcard patterns (e.g., Key changes:
Issue found: The regex pattern uses Confidence Score: 3/5
Important Files ChangedFile Analysis
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 files reviewed, 2 comments
clients/fides-js/src/lib/cookie.ts
Outdated
| ); | ||
| Object.keys(cookies.get()).forEach((name) => { | ||
| if (pattern.test(name)) { | ||
| cookies.remove(name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regular cookies are removed with { path: cookie.path ?? "/", domain: domainToUse } and optionally subdomain removal. Wildcard cookies are removed with no options at all. This could fail to remove cookies set on specific paths or domains.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering how this works.. I guess I'll dig into the library some more to see what the actual behavior is, i.e., are attributes required to remove the cookies or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can probably just use removeCookiesFromBrowser from a few lines up :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, js-cookie remove() function is a thin wrapper around document.cookie = "foo=..." with a past-due expiration which requires (name, path, domain) to match, but the library doesn't return the attributes and so is no help at all. I think the best way to handle this will depend on the backend support which may or may not provide the required attributes. Since that's TBD, we can pause here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"pause here" as in stop reviewing this PR, or "pause here" as in this is ok for now?
|
@gilluminate ready for another look! The cookies on the notice that are coming from the website monitor will have the |
gilluminate
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with one very minor nitpick to prefer arrow functions
Co-authored-by: Jason Gill <[email protected]>
Ticket ENG-1933
Description Of Changes
This PR adds support for deleting cookies using wildcards which will typically be the form of
prefix[id]or rarely[id]suffix. TheremoveCookiesFromBrowserfunction will now detect these, turn them into a regex pattern and use it to match cookies to be deleted. The attributes are ignored in this implementation sincejs-cookiedoesn't return them when getting all of the cookies, but this could be worked around with some additional complexity.For these changes to be useful, there needs to be some backend support to return the cookies with wildcards in the first place. Not clear if that's already in place or just being planned - need to track this down.The changes to Fides are pending - will link to a ticket from here when one is available.Code Changes
removeCookiesFromBrowserfunctionPre-Merge Checklist
CHANGELOG.mdupdatedpotential for performance impact or unexpected regression) that should be flagged