Skip to content
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

Fix: get the top-level hostname #4935

Merged
merged 8 commits into from
May 31, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ The types of changes are:
### Fixed
- Fixed an issue where the test integration action failed for the Zendesk integration [#4929](https://github.com/ethyca/fides/pull/4929)

### Fixed
- Fixed an issue where the consent cookie could not be set on multi-level root domain (e.g. co.uk, co.jp) [#4935](https://github.com/ethyca/fides/pull/4935)

## [2.37.0](https://github.com/ethyca/fides/compare/2.36.0...2.37.0)

### Added
Expand Down
33 changes: 29 additions & 4 deletions clients/fides-js/__tests__/lib/cookie.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,17 @@ mockUuid.v4.mockReturnValue(MOCK_UUID);
const mockGetCookie = jest.fn((): string | undefined => "mockGetCookie return");
const mockSetCookie = jest.fn(
/* eslint-disable-next-line @typescript-eslint/no-unused-vars */
(name: string, value: string, attributes: object, encoding: object) =>
`mock setCookie return (value=${value})`
(name: string, value: string, attributes: object, encoding: object) => {
// Simulate that browsers will not write cookies to known top-level public domains like "com" or "co.uk"
if (
["com", "ca", "org", "uk", "co.uk", "in", "co.in", "jp", "co.jp"].indexOf(
(attributes as { domain: string }).domain
) > -1
) {
return undefined;
}
return `mock setCookie return (value=${value})`;
}
);
const mockRemoveCookie = jest.fn(
/* eslint-disable-next-line @typescript-eslint/no-unused-vars */
Expand Down Expand Up @@ -249,6 +258,18 @@ describe("saveFidesCookie", () => {
{ url: "https://www.another.com", expected: "another.com" },
{ url: "https://privacy.bigco.ca", expected: "bigco.ca" },
{ url: "https://privacy.subdomain.example.org", expected: "example.org" },
{
url: "https://privacy.subdomain.example.co.uk",
expected: "example.co.uk",
},
gilluminate marked this conversation as resolved.
Show resolved Hide resolved
{
url: "https://example.co.in",
expected: "example.co.in",
},
{
url: "https://example.co.jp",
expected: "example.co.jp",
},
])(
"calculates the root domain from the hostname ($url)",
({ url, expected }) => {
Expand All @@ -259,8 +280,12 @@ describe("saveFidesCookie", () => {
});
const cookie: FidesCookie = getOrMakeFidesCookie();
saveFidesCookie(cookie);
expect(mockSetCookie.mock.calls).toHaveLength(1);
expect(mockSetCookie.mock.calls[0][2]).toHaveProperty("domain", expected);
const numCalls = expected.split(".").length;
expect(mockSetCookie.mock.calls).toHaveLength(numCalls);
expect(mockSetCookie.mock.calls[numCalls - 1][2]).toHaveProperty(
"domain",
expected
);
}
);

Expand Down
47 changes: 26 additions & 21 deletions clients/fides-js/src/lib/cookie.ts
Original file line number Diff line number Diff line change
Expand Up @@ -191,15 +191,11 @@ export const getOrMakeFidesCookie = (
/**
* Save the given Fides cookie to the browser using the current root domain.
*
* This calculates the root domain by using the last two parts of the hostname:
* This calculates the root domain by using the last parts of the hostname:
* privacy.example.co.uk -> example.co.uk
* privacy.example.com -> example.com
* example.com -> example.com
* localhost -> localhost
*
* NOTE: This won't handle second-level domains like co.uk:
* privacy.example.co.uk -> co.uk # ERROR
*
* (see https://github.com/ethyca/fides/issues/2072)
*/
export const saveFidesCookie = (
cookie: FidesCookie,
Expand All @@ -215,26 +211,35 @@ export const saveFidesCookie = (
// eslint-disable-next-line no-param-reassign
cookie.fides_meta.updatedAt = updatedAt;

// Write the cookie to the root domain
const rootDomain = window.location.hostname.split(".").slice(-2).join(".");

let encodedCookie: string = JSON.stringify(cookie);
if (base64Cookie) {
encodedCookie = base64_encode(encodedCookie);
}

setCookie(
CONSENT_COOKIE_NAME,
encodedCookie,
{
// An explicit path ensures this is always set to the entire domain.
path: "/",
// An explicit domain allows subdomains to access the cookie.
domain: rootDomain,
expires: CONSENT_COOKIE_MAX_AGE_DAYS,
},
CODEC
);
const hostnameParts = window.location.hostname.split(".");
let topViableDomain = "";
for (let i = 1; i <= hostnameParts.length; i += 1) {
// This loop guarantees to get the top-level hostname because that's the smallest one browsers will let you set cookies in. We test a given suffix for whether we are able to set cookies, if not we try the next suffix until we find the one that works.
topViableDomain = hostnameParts.slice(-i).join(".");
const c = setCookie(
CONSENT_COOKIE_NAME,
encodedCookie,
{
// An explicit path ensures this is always set to the entire domain.
path: "/",
// An explicit domain allows subdomains to access the cookie.
domain: topViableDomain,
expires: CONSENT_COOKIE_MAX_AGE_DAYS,
},
CODEC
);
if (c) {
const cookieString = getCookieByName(CONSENT_COOKIE_NAME);
if (cookieString) {
break;
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this exits and fails to save a cookie, what should we do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};

/**
Expand Down
Loading