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

Allow localhost URLs when detecting write key #38

Merged
merged 1 commit into from
Jan 31, 2025

Conversation

nunofgs
Copy link
Collaborator

@nunofgs nunofgs commented Jan 30, 2025

Useful for local testing.

This PR allows http:// URLs to be used when extracting the CDN. I considered possible security implications but quickly dismissed it because:

  • This is a client-side script and can be manipulated by customers. All bets are off in that sense.
  • Even if someone succeeds in using http:// for serving this file, the write key is considered public.
  • For customers that are going the self-serve option and hosting the analytics.min.js file themselves, allowing http:// URLs is useful for local testing.
    • There's an argument to be made that we should encourage customers to serve their pages under https:// in this use-case by enforcing it. Ultimately because this is a client-side concern I don't think it's a problem

@richdawe-cio richdawe-cio self-requested a review January 31, 2025 10:32
@@ -1,7 +1,7 @@
import { embeddedWriteKey } from './embedded-write-key'

const analyticsScriptRegex =
/(https:\/\/[\w.-]+)\/(?:analytics\.js\/v1|v1\/analytics-js\/snippet)\/[\w-:]+\/(analytics\.(?:min)\.js)/
/(https?:\/\/[\w.\-:]+)\/(?:analytics\.js\/v1|v1\/analytics-js\/snippet)\/[\w\-:]+\/(analytics\.(?:min)\.js)/
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Escaped the - in the latter part of the regex because it wasn't a range. We intended to allow a dash -

[\w\-:]

Choose a reason for hiding this comment

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

This is not relevant to your PR, but I'm not really sure what the (?:min) bit at the end is doing. Non-capturing match on min seems pointless.

I wonder if it's supposed to be optionally matching .min in the path, in which case:

(analytics\.(?:min)\.js)

should probably be:

(?:analytics(\.min)?\.js)

@richdawe-cio
Copy link

Useful for local testing.

This PR allows http:// URLs to be used when extracting the CDN. I considered possible security implications but quickly dismissed it because:

Just to confirm that our CDN redirects HTTP to HTTPS:

bash-5.2$ curl -v -o /dev/null http://cdp-eu.customer.io/v1/analytics-js/snippet/REDACTED/analytics.min.js 2>&1 | grep -i -E '(HTTP/|location)'
> GET /v1/analytics-js/snippet/REDACTED/analytics.min.js HTTP/1.1
< HTTP/1.1 301 Moved Permanently
< Location: https://cdp-eu.customer.io:443/v1/analytics-js/snippet/REDACTED/analytics.min.js

bash-5.2$ curl -v -o /dev/null http://cdp.customer.io/v1/analytics-js/snippet/REDACTED/analytics.min.js 2>&1 | grep -i -E '(HTTP/|location)'
> GET /v1/analytics-js/snippet/REDACTED/analytics.min.js HTTP/1.1
< HTTP/1.1 301 Moved Permanently
< Location: https://cdp.customer.io:443/v1/analytics-js/snippet/REDACTED/analytics.min.js

@@ -1,7 +1,7 @@
import { embeddedWriteKey } from './embedded-write-key'

const analyticsScriptRegex =
/(https:\/\/[\w.-]+)\/(?:analytics\.js\/v1|v1\/analytics-js\/snippet)\/[\w-:]+\/(analytics\.(?:min)\.js)/
/(https?:\/\/[\w.\-:]+)\/(?:analytics\.js\/v1|v1\/analytics-js\/snippet)\/[\w\-:]+\/(analytics\.(?:min)\.js)/

Choose a reason for hiding this comment

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

This is not relevant to your PR, but I'm not really sure what the (?:min) bit at the end is doing. Non-capturing match on min seems pointless.

I wonder if it's supposed to be optionally matching .min in the path, in which case:

(analytics\.(?:min)\.js)

should probably be:

(?:analytics(\.min)?\.js)

@nunofgs nunofgs merged commit 74f90a5 into main Jan 31, 2025
1 check passed
@nunofgs nunofgs deleted the allow-localhost-urls-for-analytics-js branch January 31, 2025 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants