fix: seek subdomain correctly #888
Conversation
|
Size Change: +2.2 kB (0%) Total Size: 734 kB
ℹ️ View Unchanged
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| 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 + ';' | ||
| }, | ||
| } |
There was a problem hiding this comment.
@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...
| if (!matchedSubDomain) { | ||
| const originalMatch = originalCookieDomainFn(hostname) | ||
| if (originalMatch !== matchedSubDomain) { | ||
| logger.info('Warning: cookie subdomain discovery mismatch', originalMatch, matchedSubDomain) | ||
| } | ||
| matchedSubDomain = originalMatch | ||
| } |
There was a problem hiding this comment.
... if the new mechanism returns no match we fall back to the original.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
| if (!matchedSubDomain) { | ||
| const originalMatch = originalCookieDomainFn(hostname) | ||
| if (originalMatch !== matchedSubDomain) { | ||
| logger.info('Warning: cookie subdomain discovery mismatch', originalMatch, matchedSubDomain) | ||
| } | ||
| matchedSubDomain = originalMatch | ||
| } |
There was a problem hiding this comment.
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
We relied on a regex for detecting subdomain in order to choose what cookie domain to set for PostHog storage.
This worked fine for
example.comorwww.example.combut it didn't work forexample.co.ukorexample.com.aubecause it would grab the first two parts and so try to set on the public suffixco.ukorcom.auA lot of suggested solutions on line involve keeping a list of public suffixes, or making DNS lookups to validate values which wouldn't fit here.
But... browsers reject cookies set on public suffixes - despite not offering an API to directly check if something is a public suffix.
So, we can split the current hostname up and then progressively try to set a cookie until the browser accepts it.
For
a.long.domain.someone.is.using.com.auwe would check:.aurejected by browser.com.aurejected by browser.using.com.auaccepted by browserWe call this method a lot and the browser can't maintain in-memory variables and navigate between sub-domains, so I also cache the result
I manually tested the code in the developer console of firefox, chrome, and safari