[SURF-1836] feat(tag): support custom-domain script attribute - #72
Conversation
|
|
Can you remove the AI generated tests. I want to have a thoughtful approach to testing and this is a great place to develop good practices given the small size of this repo |
|
Addressed in c9c08b4: removed every test change introduced by this PR. The unchanged existing suites pass (12/12 Vitest and 5/5 unit tests), along with typecheck and build. |
| apiBaseUrl, | ||
| leadIdentifyApi: `${apiBaseUrl}/lead/identify`, | ||
| userJourneyTrackingApi: `${apiBaseUrl}/lead/track`, | ||
| surfaceDomains: Array.from(new Set([...SURFACE_DOMAINS, customOrigin])), |
There was a problem hiding this comment.
@greptile Any HTTPS data-custom-domain gets added to the postMessage allowlist here and has lead PII routed to it is the domain verified against the env's real custom domain somewhere, or do we just trust whatever the page passes?
There was a problem hiding this comment.
There's no server-side or environment-level verification — it's purely trusted from the page. normalizeCustomOrigin only validates that the attribute value is a well-formed HTTPS origin (HTTPS scheme, no credentials, no path/query/fragment). It does not cross-check the value against the site-id's actual configured custom domain.
This is a real attack surface to consider:
-
Exfiltration via a compromised/malicious page — Any page that embeds the Surface tag can redirect lead PII (
/lead/identify,/lead/track) to an arbitrary HTTPS server by settingdata-custom-domain="attacker.example.com". The tag will happily POST fingerprint + lead data there. -
postMessage spoofing — The attacker-controlled origin is added to
surfaceDomainsand trusted for inboundpostMessage. A malicious iframe from that origin could triggerSTORE_UPDATEflows, which then callidentifyLeadagainst the same attacker origin.
The practical risk depends on your threat model. If only your own team adds the <script> tag to customer pages (i.e., the page author is already trusted), the blast radius is limited to self-harm. But if customers self-serve by copying a snippet and the data-custom-domain value could be tampered (e.g., via a CMS injection, script injection into their page, or a misconfigured CDN), then an attacker can redirect PII collection to a domain they control.
A few mitigations to consider:
- Have the backend
/api/v1/lead/identifyendpoint verify that theOriginorRefererheader matches a known allowed domain for thatsite-id, so even if traffic is routed to Surface's infra the data is rejected for invalid environments. - Alternatively, return the allowed custom domain(s) for a given
site-idfrom a Surface-controlled endpoint at init time, then validate thedata-custom-domainattribute against that list before trusting it. - At minimum, document clearly that
data-custom-domainmust only be set to a domain verified and controlled by the Surface customer.
There was a problem hiding this comment.
@greptile is there a CI check that these built bundles match a fresh build from src/?
There was a problem hiding this comment.
Yes — the CI workflow has an explicit step for this:
- name: Bundle is up to date
run: |
pnpm run build
git diff --exit-code surface_tag.js surface_embed_v1.jsIt rebuilds from source and fails the job if surface_tag.js or surface_embed_v1.js differ from what's committed. So stale bundles will block the PR from merging.
| const hostname = window.location?.hostname ?? ""; | ||
| return SURFACE_DOMAINS.some((url) => new URL(url).hostname === hostname); | ||
| const origin = window.location?.origin ?? ""; | ||
| return this.surfaceDomains.includes(origin); |
There was a problem hiding this comment.
this switched from hostname matching to window.location.origin matching, is that equivalent in all cases @greptile
There was a problem hiding this comment.
Not quite equivalent — there are two edge cases where the behavior differs.
SURFACE_DOMAINS contains full origins ("https://forms.withsurface.com"), so surfaceDomains does too. The old code stripped both sides down to just the hostname before comparing; the new code compares the full origin (scheme + host + port).
Where they diverge:
-
HTTP — if
window.locationishttp://forms.withsurface.com, old code matches (hostname matches), new code doesn't (http://≠https://). Surface domains are HTTPS-only in production so this is unlikely to matter. -
Non-standard port — if
window.locationishttps://forms.withsurface.com:3000(local dev server),window.location.originis"https://forms.withsurface.com:3000", which won't match any entry insurfaceDomains. The old hostname comparison would still match. This is the more likely real-world gap.
The stricter check is correct for production — origin comparison is semantically right for security purposes. But it's worth confirming whether isCurrentOriginSurfaceDomain is ever exercised in a local-dev setup running on a non-standard port, since that scenario silently regresses.
| if (w.__SURFACE_OPEN_TRIGGERS_MAP) return w.__SURFACE_OPEN_TRIGGERS_MAP; | ||
|
|
||
| const sessionKey = SESSION_PREFIX + environmentId; | ||
| const sessionKey = `${SESSION_PREFIX}${config.apiBaseUrl}:${environmentId}`; |
There was a problem hiding this comment.
@greptile the session-cache key now includes the api base, so any key cached under the old format is orphaned after deploy, just a one-time refetch, or does anything rely on the old key??
There was a problem hiding this comment.
One-time refetch only. Nothing in the codebase reads or writes to the old key format after this deploy — sessionStorage under the old key (surface_open_triggers:<environmentId>) just becomes dead weight that expires when the tab closes. The new key (surface_open_triggers:<apiBaseUrl>:<environmentId>) will miss on first load, trigger a fresh fetch, and get written correctly.
No correctness issue: the cache is purely a perf optimization (CACHE_TTL_MS = 5 min), and the comments in the code explicitly treat it as best-effort. The worst case is one extra network call per tab on the first page load after deploy.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
Problem
The Surface tag hard-codes
forms.withsurface.comfor lead identification, journey tracking,external-form events, and open-trigger lookups. Environments whose forms are served from a
verified custom domain therefore still send tag traffic to the default Surface hostname, and
custom-domain iframes are not part of the tag's postMessage allowlist.
Changes
data-custom-domain="demo.example.com"script attribute.falls back to the current defaults for absent or invalid values.
through
https://<custom-domain>/api/v1.existing Surface origins.
SurfaceExternalForm({ serverBaseUrl })override.surface_tag.jsandsurface_embed_v1.js.Client usage
Without the attribute, existing integrations continue to use
https://forms.withsurface.com.Validation
pnpm typecheckpnpm test— 12/12 existing testspnpm test:unit— 5/5 existing unit testspnpm buildgit diff --check