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: seek subdomain correctly #888

Merged
merged 14 commits into from
Nov 15, 2023
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
45 changes: 0 additions & 45 deletions src/__tests__/storage.js

This file was deleted.

84 changes: 84 additions & 0 deletions src/__tests__/storage.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
import { window } from '../../src/utils/globals'
import { resetSessionStorageSupported, seekFirstNonPublicSubDomain, sessionStore } from '../storage'

describe('sessionStore', () => {
describe('seekFirstNonPublicSubDomain', () => {
const mockDocumentDotCookie = {
value_: '',

get cookie() {
return this.value_
},

set cookie(value) {
//needs to refuse known public suffixes, like a browser would
// value arrives like dmn_chk_1699961248575=1;domain=.uk
const domain = value.split('domain=')
if (['.uk', '.com', '.au', '.com.au', '.co.uk'].includes(domain[1])) return
this.value_ += value + ';'
},
}
Comment on lines +6 to +20
Copy link
Member Author

Choose a reason for hiding this comment

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

@benjackwhite I couldn't get testcafe to test this. But I was having to attach the function to the window and then have testcafe eval it and who knows why it wasn't working.

Sometimes it wasn't present, others it returned the empty string, I think even sometimes it worked, but it was horrible.

So instead I have a fake cookie jar to test the logic with.

And then in the actual code...

test.each([
{
candidate: 'www.google.co.uk',
expected: 'google.co.uk',
},
{
candidate: 'www.google.com',
expected: 'google.com',
},
{
candidate: 'www.google.com.au',
expected: 'google.com.au',
},
{
candidate: 'localhost',
expected: '',
},
])(`%s subdomain check`, ({ candidate, expected }) => {
expect(seekFirstNonPublicSubDomain(candidate, mockDocumentDotCookie)).toEqual(expected)
})
})

it('stores objects as strings', () => {
sessionStore.set('foo', { bar: 'baz' })
expect(sessionStore.get('foo')).toEqual('{"bar":"baz"}')
})
it('stores and retrieves an object untouched', () => {
const obj = { bar: 'baz' }
sessionStore.set('foo', obj)
expect(sessionStore.parse('foo')).toEqual(obj)
})
it('stores and retrieves a string untouched', () => {
const str = 'hey hey'
sessionStore.set('foo', str)
expect(sessionStore.parse('foo')).toEqual(str)
})
it('returns null if the key does not exist', () => {
expect(sessionStore.parse('baz')).toEqual(null)
})
it('remove deletes an item from storage', () => {
const str = 'hey hey'
sessionStore.set('foo', str)
expect(sessionStore.parse('foo')).toEqual(str)
sessionStore.remove('foo')
expect(sessionStore.parse('foo')).toEqual(null)
})

describe('sessionStore.is_supported', () => {
beforeEach(() => {
// Reset the sessionStorageSupported before each test. Otherwise, we'd just be testing the cached value.
// eslint-disable-next-line no-unused-vars
resetSessionStorageSupported()
})
it('returns false if sessionStorage is undefined', () => {
const sessionStorage = (window as any).sessionStorage
delete (window as any).sessionStorage
expect(sessionStore.is_supported()).toEqual(false)
;(window as any).sessionStorage = sessionStorage
})
it('returns true by default', () => {
expect(sessionStore.is_supported()).toEqual(true)
})
})
})
72 changes: 63 additions & 9 deletions src/storage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,68 @@ import { DISTINCT_ID, SESSION_ID, SESSION_RECORDING_IS_SAMPLED } from './constan
import { _isNull, _isUndefined } from './utils/type-utils'
import { logger } from './utils/logger'

const Y1970 = 'Thu, 01 Jan 1970 00:00:00 GMT'

/**
* Browsers don't offer a way to check if something is a public suffix
* e.g. `.com.au`, `.io`, `.org.uk`
*
* But they do reject cookies set on public suffixes
* Setting a cookie on `.co.uk` would mean it was sent for every `.co.uk` site visited
*
* So, we can use this to check if a domain is a public suffix
* by trying to set a cookie on a subdomain of the provided hostname
* until the browser accepts it
*
* inspired by https://github.com/AngusFu/browser-root-domain
*/
export function seekFirstNonPublicSubDomain(hostname: string, cookieJar = document): string {
if (['localhost', '127.0.0.1'].includes(hostname)) return ''

const list = hostname.split('.')
let len = list.length
const key = 'dmn_chk_' + +new Date()
const R = new RegExp('(^|;)\\s*' + key + '=1')

while (len--) {
const candidate = list.slice(len).join('.')
const candidateCookieValue = key + '=1;domain=.' + candidate

// try to set cookie
cookieJar.cookie = candidateCookieValue

if (R.test(cookieJar.cookie)) {
// the cookie was accepted by the browser, remove the test cookie
cookieJar.cookie = candidateCookieValue + ';expires=' + Y1970
return candidate
}
}
return ''
}

const DOMAIN_MATCH_REGEX = /[a-z0-9][a-z0-9-]+\.[a-z]{2,}$/i
const originalCookieDomainFn = (hostname: string): string => {
const matches = hostname.match(DOMAIN_MATCH_REGEX)
return matches ? matches[0] : ''
}

export function chooseCookieDomain(hostname: string, cross_subdomain: boolean | undefined): string {
if (cross_subdomain) {
// NOTE: Could we use this for cross domain tracking?
let matchedSubDomain = seekFirstNonPublicSubDomain(hostname)

if (!matchedSubDomain) {
const originalMatch = originalCookieDomainFn(hostname)
if (originalMatch !== matchedSubDomain) {
logger.info('Warning: cookie subdomain discovery mismatch', originalMatch, matchedSubDomain)
}
matchedSubDomain = originalMatch
}
Comment on lines +58 to +64
Copy link
Member Author

Choose a reason for hiding this comment

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

... if the new mechanism returns no match we fall back to the original.

Copy link
Member Author

Choose a reason for hiding this comment

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

The other alternative I guess is to let people configure their subdomain.

And if that's present we use it without looking at location. Then folk that it's working for do nothing, everyone else has to configure it. That's yucky but safer

Copy link
Collaborator

Choose a reason for hiding this comment

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

Still somewhat worried about this code but its more of a gut feeling than anything I can actually see wrong with it :D
Let's try it


return matchedSubDomain ? '; domain=.' + matchedSubDomain : ''
}
return ''
}

// Methods partially borrowed from quirksmode.org/js/cookies.html
export const cookieStore: PersistentStore = {
Expand Down Expand Up @@ -44,17 +105,10 @@ export const cookieStore: PersistentStore = {

set: function (name, value, days, cross_subdomain, is_secure) {
try {
let cdomain = '',
expires = '',
let expires = '',
secure = ''

if (cross_subdomain) {
// NOTE: Could we use this for cross domain tracking?
const matches = document.location.hostname.match(DOMAIN_MATCH_REGEX),
domain = matches ? matches[0] : ''

cdomain = domain ? '; domain=.' + domain : ''
}
const cdomain = chooseCookieDomain(document.location.hostname, cross_subdomain)

if (days) {
const date = new Date()
Expand Down
2 changes: 2 additions & 0 deletions testcafe/e2e.spec.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
import { t } from 'testcafe'
import { retryUntilResults, queryAPI, initPosthog, captureLogger, staticFilesMock } from './helpers'

// eslint-disable-next-line no-undef
fixture('posthog.js capture')
.page('http://localhost:8000/playground/cypress-full/index.html')
.requestHooks(captureLogger, staticFilesMock)
.afterEach(async () => {
const browserLogs = await t.getBrowserConsoleMessages()
Object.keys(browserLogs).forEach((level) => {
browserLogs[level].forEach((line) => {
// eslint-disable-next-line no-console
console.log(`Browser ${level}:`, line)
})
})
Expand Down
12 changes: 10 additions & 2 deletions testcafe/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,14 @@ import fetch from 'node-fetch'

// NOTE: These tests are run against a dedicated test project in PostHog cloud
// but can be overridden to call a local API when running locally
// eslint-disable-next-line no-undef
const currentEnv = process.env
const {
POSTHOG_PROJECT_KEY,
POSTHOG_API_KEY,
POSTHOG_API_HOST = 'https://app.posthog.com',
POSTHOG_API_PROJECT = '11213',
} = process.env
} = currentEnv

const HEADERS = { Authorization: `Bearer ${POSTHOG_API_KEY}` }

Expand All @@ -26,18 +28,20 @@ export const captureLogger = RequestLogger(/ip=1/, {
export const staticFilesMock = RequestMock()
.onRequestTo(/array.full.js/)
.respond((req, res) => {
// eslint-disable-next-line no-undef
const arrayjs = fs.readFileSync(path.resolve(__dirname, '../dist/array.full.js'))
res.setBody(arrayjs)
})
.onRequestTo(/playground/)
.respond((req, res) => {
// eslint-disable-next-line no-undef
const html = fs.readFileSync(path.resolve(__dirname, '../playground/cypress-full/index.html'))
res.setBody(html)
})

export const initPosthog = (config) => {
return ClientFunction((configParams = {}) => {
var testSessionId = Math.round(Math.random() * 10000000000).toString()
const testSessionId = Math.round(Math.random() * 10000000000).toString()
Dismissed Show dismissed Hide dismissed
configParams.debug = true
window.posthog.init(configParams.api_key, configParams)
window.posthog.register({
Expand Down Expand Up @@ -70,6 +74,7 @@ export async function retryUntilResults(operation, target_results, limit = 6, de
if (results.length >= target_results) {
resolve(results)
} else {
// eslint-disable-next-line no-console
console.log(`Expected ${target_results} results, got ${results.length} (attempt ${count})`)
attempt(count + 1, resolve, reject)
}
Expand All @@ -78,6 +83,8 @@ export async function retryUntilResults(operation, target_results, limit = 6, de
}, delay)
}

// new Promise isn't supported in IE11, but we don't care in these tests
// eslint-disable-next-line compat/compat
return new Promise((...args) => attempt(0, ...args))
}

Expand All @@ -90,6 +97,7 @@ export async function queryAPI(testSessionId) {
const data = await response.text()

if (!response.ok) {
// eslint-disable-next-line no-console
console.error('Bad Response', response.status, data)
throw new Error('Bad Response')
}
Expand Down
Loading