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: heroku subdomain check #842

Merged
merged 3 commits into from
Oct 23, 2023
Merged

fix: heroku subdomain check #842

merged 3 commits into from
Oct 23, 2023

Conversation

pauldambra
Copy link
Member

resolves https://github.com/PostHog/posthog-js/security/code-scanning/3

We checked for an herokuapp.com domain without caring about the position of the match within the hostname.

So, we would have treated appname.herokuapp.com and appname.herokuapp.com.intercepting-domain.io as the same.

Well, not any more!

@pauldambra pauldambra added the bump patch Bump patch version when this PR gets merged label Oct 21, 2023
// split and slice isn't a great way to match arbitrary domains,
// but it's good enough for ensuring we only match herokuapp.com when it is the TLD
// for the hostname
return hostname.split('.').slice(-2).join('.').indexOf('herokuapp.com') === -1

Check failure

Code scanning / CodeQL

Incomplete URL substring sanitization High

'
herokuapp.com
' can be anywhere in the URL, and arbitrary hosts may come before or after it.
@github-actions
Copy link

github-actions bot commented Oct 21, 2023

Size Change: +41 B (0%)

Total Size: 725 kB

Filename Size Change
dist/array.full.js 177 kB +11 B (0%)
dist/array.js 118 kB +10 B (0%)
dist/es.js 118 kB +10 B (0%)
dist/module.js 118 kB +10 B (0%)
ℹ️ View Unchanged
Filename Size
dist/recorder-v2.js 95 kB
dist/recorder.js 58.3 kB
dist/surveys.js 39.6 kB

compressed-size-action

Copy link
Collaborator

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

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

Other than the alert still showing up for this PR - this looks correct to me.

@pauldambra pauldambra merged commit 719b3a8 into master Oct 23, 2023
13 checks passed
@pauldambra pauldambra deleted the fix/sub-domain-check branch October 23, 2023 08:43
@neilkakkar
Copy link
Collaborator

hmm, I'm not really clear about the original functionality. Is it something like:

By default, we want to share super properties across subdomains, unless we're on herokuapp, which makes sense since those apps are all independent.

A URL that fails the current default is something like app.mysite-herokuapp.com. Not a big deal since these are defaults anyway and don't present a real security risk over this setting(I think), I'd prefer if appname.herokuapp.com.intercepting-domain.io resolves to true then mysite-herokuapp.com does too. (consider it the nittiest of nit 😝 ).

Overall, if my understanding is correct, I feel like defaulting to false whenever we see herokuapp is much more straightforward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump patch Bump patch version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants