fix(toolbar): match authorized URLs by exact origin#61020
Merged
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![hex-security-app[bot]](https://avatars.githubusercontent.com/in/2665233?s=60&v=4){kind=small}
| return true | ||
| } | ||
| // www-equivalence: compare hostnames with www. stripped | ||
| return stripWww(authorizedUrlParsed.hostname) === hostNormalized |
There was a problem hiding this comment.
In the updated checkUrlIsAuthorized, the www-equivalence fallback (line 223) compares only stripped hostnames — it never compares protocols. As a result, if https://example.com is in the authorized list, the check also passes for http://example.com and http://www.example.com:
- Exact-origin check (
protocol + '//' + host === urlWithoutPath) correctly fails for mismatched protocols. - Fallback:
stripWww(authorizedUrlParsed.hostname) === hostNormalizedsucceeds because both hostnames normalize toexample.com, ignoring the protocol difference.
This is directly exercised in iframedToolbarBrowserLogic.ts:256:
if (!values.checkUrlIsAuthorized(e.origin)) { ... }An attacker who can make the browser send a postMessage from an HTTP version of an authorized domain (trivially achievable via MITM on HTTP traffic) bypasses the origin guard entirely and can inject toolbar messages (PH_TOOLBAR_NAVIGATED, PH_TOOLBAR_INIT, PH_NEW_ACTION_CREATED) into the PostHog toolbar browser. While the individual message payloads are low-value, this makes the postMessage origin gate meaningless for any authorized HTTPS site whose HTTP counterpart is reachable or spoof-able.
Prompt To Fix With AI
In `checkUrlIsAuthorized`, the www-equivalence fallback on line 223 must also require the protocols to match before returning true. Change:
```ts
return stripWww(authorizedUrlParsed.hostname) === hostNormalized
```
to:
```ts
return (
authorizedUrlParsed.protocol === parsedUrl.protocol &&
stripWww(authorizedUrlParsed.hostname) === hostNormalized
)
```
This ensures that `http://example.com` cannot match an authorized entry of `https://example.com`, while still allowing `https://www.example.com` ↔ `https://example.com` equivalence. Add a test case to `checkUrlIsAuthorized` in `authorizedUrlListLogic.test.ts`:
```ts
{ url: 'http://example.com', authorized: ['https://example.com'], expected: false },
{ url: 'http://www.example.com', authorized: ['https://example.com'], expected: false },
{ url: 'https://www.example.com',authorized: ['https://example.com'], expected: true },
{ url: 'https://example.com', authorized: ['https://www.example.com'], expected: true },
```Severity: medium | Confidence: 75%
Contributor
|
Size Change: 0 B Total Size: 80.9 MB ℹ️ View Unchanged
|
Contributor
|
Reviews (1): Last reviewed commit: "fix(toolbar): match authorized URLs by e..." | Re-trigger Greptile |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
checkUrlIsAuthorized(shared by the toolbar, heatmaps, web analytics, and the postMessage origin check in the iframed toolbar browser) decided whether a URL belongs to a project's authorized list using a loose substring check plus an unanchored wildcard regex. As a result a URL could be considered authorized just by overlapping an authorized entry's string:https://example.com.evil.commatchedhttps://example.com(suffix)https://app.example.com.evil.commatchedhttps://*.example.com(the wildcard regex had no end anchor)https://example.comatchedhttps://example.com(prefix/substring)In each case the actual registrable domain is different from — and not controlled by the owner of — the authorized one.
Changes
Rewrite the matching so each authorized entry is evaluated exactly once:
https://*.example.com,http://localhost:*) are compiled to a regex anchored with^…$, so a*can no longer match a suffix of an unrelated origin.protocol://host) instead of substring, so a different domain can't be authorized by being a prefix/substring of an entry.Existing behavior is preserved: path is ignored,
www.equivalence still holds in both directions, and subdomain/port wildcards still match as before.How did you test this code?
I'm an agent (Claude Code). Automated tests only — no manual QA:
checkUrlIsAuthorizedtest block inauthorizedUrlListLogic.test.tscovering the legitimate matches (exact, path,www.both directions, subdomain wildcard, multi-level subdomain, port wildcard) and the suffix / prefix / unanchored-wildcard cases that must be rejected. Written test-first: the rejection cases failed against the old implementation and pass after the change.authorizedUrlListLogicsuite (55 tests, incl. existingfilterNotAuthorizedUrlscoverage) and the heatmaps logic suite — all green.🤖 Agent context
Authored by Claude Code as a follow-up to a separate change that hardened the site-preview iframe. While verifying that the site-preview authorization gate was sound, the underlying
checkUrlIsAuthorizedmatcher was found to accept origins that overlap an authorized entry's string. This PR tightens the matcher itself, which also benefits the other callers (notably thepostMessageevent-origin check iniframedToolbarBrowserLogic). Approach: kept the wildcard pattern construction identical and only added anchoring, and replaced the substring comparison with an origin comparison while preserving the existingwww.-equivalence semantics to avoid regressing legitimate matches.