Conversation
🦋 Changeset detectedLatest commit: e8266dc The changes in this PR will be included in the next version bump. This PR includes changesets to release 112 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (typeof document !== 'undefined') { | ||
| let script = document.createElement('script'); | ||
| script.src = asset.url + '?t=' + Date.now(); | ||
| script.src = appendBundleVersion(asset.url); |
Check failure
Code scanning / CodeQL
Client-side cross-site scripting High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 days ago
To fix this, we should prevent arbitrary, attacker-controlled URLs from being used as script.src. The safest general approach is to validate or normalize asset.url before using it, restricting it to expected forms (for example, only same-origin URLs, or only relative paths under a known base) and rejecting or ignoring anything else. This keeps the existing HMR behavior for valid bundle URLs while blocking injection of external or JavaScript pseudo-URLs.
The best minimal fix in this snippet is to introduce a small helper that “sanitizes” or validates URLs coming from HMR assets, and then use that helper wherever we load scripts via asset.url (in document.createElement('script') and in the worker import/importScripts paths). Specifically, inside hmr-runtime.ts we can add a sanitizeAssetUrl function near appendBundleVersion that (1) rejects dangerous URL schemes like javascript: and data:, and (2) ensures the URL is either relative or same-origin (e.g., by constructing a URL object with location.origin as base and checking the origin). If the URL fails validation, the function can throw or cause the download to reject, preventing script execution. Then we update the uses at lines 476, 497, and 506 to call sanitizeAssetUrl(asset.url) before passing it to appendBundleVersion. No external dependencies are needed; we can use the built-in URL class and location/self.location where available.
Concretely:
- In
packages/runtimes/hmr/src/loaders/hmr-runtime.ts, aboveappendBundleVersion, definefunction sanitizeAssetUrl(rawUrl: string): string { ... }that:- Trims whitespace.
- If
typeof location !== 'undefined', buildsnew URL(rawUrl, location.href)and:- Rejects if
url.protocolis nothttp:orhttps:. - Optionally rejects if
url.origin !== location.origin(to enforce same-origin). - Returns
url.href(a normalized absolute URL).
- Rejects if
- If
locationis not available (e.g., worker context), falls back to a conservative check that disallowsjavascript:anddata:prefixes and returns the original string.
- Change:
script.src = appendBundleVersion(asset.url);
to
script.src = appendBundleVersion(sanitizeAssetUrl(asset.url));__parcel__import__(appendBundleVersion(asset.url))
to usesanitizeAssetUrl(asset.url)similarly.__parcel__importScripts__(appendBundleVersion(asset.url))
likewise.
This keeps all existing functionality for valid HMR asset URLs but ensures that untrusted or cross-origin URLs cannot silently be executed.
There was a problem hiding this comment.
Doesn't look like a legit issue to me.
There was a problem hiding this comment.
@mattcompiles do you reckon we can dismiss this? I think this is the only failing CodeQL check.
| if (typeof document !== 'undefined') { | ||
| let script = document.createElement('script'); | ||
| script.src = asset.url + '?t=' + Date.now(); | ||
| script.src = appendBundleVersion(asset.url); |
Check warning
Code scanning / CodeQL
Client-side URL redirect Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 days ago
In general, to fix untrusted URL usage in client-side redirects or script loads, you restrict navigation or loading to either (1) same-origin paths or (2) a small, explicit allowlist of trusted origins/paths. You avoid using fully user-controlled URLs directly in window.location, script.src, img.src, or similar sinks without validation.
For this specific case, we should ensure that asset.url cannot cause navigation or loading from an untrusted origin. The least invasive change, without altering overall HMR behavior, is to make appendBundleVersion normalize the URL so that only relative URLs (or same-origin URLs) are allowed, and to prevent protocol-relative or absolute cross-origin URLs. We only touch the shown file. The best approach here is:
-
Add a small helper to sanitize/normalize the asset URL.
- Reject
javascript:and other dangerous schemes. - If the URL is absolute and same-origin, allow it.
- If it is relative (starts with
/or no scheme), resolve it againstlocation.originwhenwindow/locationare available. - If we cannot safely resolve or check, conservatively return the input unchanged (to minimize functional impact where no browser context is present, e.g. workers), but we keep the main protection for the document case where
script.srcis used.
- Reject
-
Modify
appendBundleVersion(url: string)so it first passesurlthrough this sanitizer before appending the version query parameter. This way:- The browser case (
document.createElement('script')on line 475–490) will use a sanitized URL. - Worker cases still go through
appendBundleVersionand thus benefit where possible.
- The browser case (
Concretely:
-
In
packages/runtimes/hmr/src/loaders/hmr-runtime.ts, aboveappendBundleVersion, definesanitizeBundleUrl(or similarly named) that:- Returns the original
urlearly iftypeof window === 'undefined'ortypeof location === 'undefined', to avoid changing non-browser contexts. - Trims whitespace.
- URL-decodes via
decodeURIfor comparison (defensively in try/catch). - Rejects / blocks URLs that start with
javascript:,data:, or other non-http(s) schemes. - For absolute URLs (
new URL(url, location.href)), ensuresparsed.origin === location.origin; otherwise, returns a safe fallback (e.g.location.origin+ pathname) or justlocation.origindepending on what we can preserve safely. To avoid altering functionality too much, we keep the path if same-origin, but if cross-origin we instead returnlocation.origin+ the original pathname or a default known-safe path (here, we can just strip origin and keep pathname if possible). - For relative URLs, returns the relative form unchanged (but normalized).
- Returns the original
-
Inside
appendBundleVersion(url: string), callconst safeUrl = sanitizeBundleUrl(url);and then perform all further logic usingsafeUrlinstead of the originalurl.
This keeps existing behavior for the normal, expected asset URLs (relative or same-origin), and prevents script loads from attacker-controlled cross-origin or dangerous-scheme URLs when the HMR server is compromised.
| @@ -117,18 +117,60 @@ | ||
| return nextVersion; | ||
| } | ||
|
|
||
| function sanitizeBundleUrl(url: string): string { | ||
| // Only enforce strict checks in a browser-like environment where we control script.src. | ||
| if (typeof window === 'undefined' || typeof location === 'undefined') { | ||
| return url; | ||
| } | ||
|
|
||
| let trimmed = url.trim(); | ||
| // Basic scheme protection: disallow dangerous schemes like javascript: or data: | ||
| let lower = trimmed.toLowerCase(); | ||
| if ( | ||
| lower.startsWith('javascript:') || | ||
| lower.startsWith('data:') || | ||
| lower.startsWith('vbscript:') | ||
| ) { | ||
| // Block entirely by returning a safe no-op relative URL. | ||
| return '/'; | ||
| } | ||
|
|
||
| try { | ||
| // Resolve against current location to inspect origin. | ||
| let resolved = new URL(trimmed, location.href); | ||
|
|
||
| // If the original specifies a different origin explicitly, do not allow it. | ||
| if (resolved.origin !== location.origin) { | ||
| // Strip the foreign origin but keep the path if present. | ||
| return resolved.pathname + resolved.search + resolved.hash; | ||
| } | ||
|
|
||
| // Same-origin URLs are allowed as-is (normalized form). | ||
| return resolved.pathname + resolved.search + resolved.hash; | ||
| } catch { | ||
| // If URL construction fails, fall back to the original string. | ||
| return trimmed; | ||
| } | ||
| } | ||
|
|
||
| function appendBundleVersion(url: string) { | ||
| const safeUrl = sanitizeBundleUrl(url); | ||
| // @ts-expect-error TS2304 | ||
| if (!HMR_ENABLE_BUNDLE_VERSION) { | ||
| return url + (url.includes('?') ? '&' : '?') + 't=' + Date.now(); | ||
| return ( | ||
| safeUrl + | ||
| (safeUrl.includes('?') ? '&' : '?') + | ||
| 't=' + | ||
| Date.now() | ||
| ); | ||
| } | ||
|
|
||
| let version = getBundleVersion(); | ||
| if (version == null) { | ||
| return url; | ||
| return safeUrl; | ||
| } | ||
|
|
||
| return url + (url.includes('?') ? '&' : '?') + 't=' + version; | ||
| return safeUrl + (safeUrl.includes('?') ? '&' : '?') + 't=' + version; | ||
| } | ||
|
|
||
| function getHostname() { |
| assetFromValue(asset, options), | ||
| ]), | ||
| ), | ||
| changedAssets: skipChangedAssets |
There was a problem hiding this comment.
The changed-assets code is making all asset URLs involved in one HMR update share a single version token, instead of each URL generating its own Date.now() query string.
Before:
- every refreshed asset URL did its own ?t=${Date.now()}
- that meant one HMR cycle could produce slightly different timestamps for JS, CSS, new URL(...) assets, worker imports, and so on
- if multiple related assets were refreshed together, they weren’t guaranteed to point at the same logical update version
Now:
- when Atlaspack receives and handles an HMR update, it bumps globalThis.__parcel__hmrBundleVersion once
- every asset refresh during that HMR cycle appends ?t=
- the next HMR cycle bumps it again
In hmr-runtime.ts, that shared token is used for:
- CSS link replacement
- JS hot-update script loads
- worker/module import paths during HMR
- In JSRuntime.ts, the same token is used for runtime-generated asset URLs, like new URL(...)-style bundle references.
So the practical effect is:
- one update cycle gets one cache-busting token
- subsequent update cycles get a new token
- all refreshed assets stay in sync within a cycle
- you avoid subtle mismatches caused by a bunch of independent Date.now() calls firing a few milliseconds apart
There was a problem hiding this comment.
I don't see how changedAssets is related to the URLs though. Assets are modules or files, where-as the URLs are related to bundles. I think some systems, like incremental symbol prop will break if you just blank the changed assets param.
I don't see how changedAssets is related to this change at all. Can you try testing your change without this code and see if it still works?
There was a problem hiding this comment.
thanks! Looks like it still works so I removed these changes.
📊 Type Coverage ReportCoverage Comparison
Files with Most Type Issues (Top 15)
This report was generated by the Type Coverage GitHub Action |
📊 Benchmark Results🎉 Performance improvements detected! 📊 Benchmark ResultsOverall Performance
🔍 Detailed Phase AnalysisThree.js Real Repository (JS)
Three.js Real Repository (V3)
💾 Unified Memory AnalysisThree.js Real Repository (JS) Memory Statistics
Sample Counts: JS: 14, Native: 246 Three.js Real Repository (V3) Memory Statistics
Sample Counts: JS: 14, Native: 239 🖥️ Environment
|
alexzherdev
force-pushed
the
azherdev/hmr-timestamps
branch
2 times, most recently
from
24f411a to
a0e3dec
Compare
alexzherdev
force-pushed
the
azherdev/hmr-timestamps
branch
from
a0e3dec to
82438b3
Compare
alexzherdev
force-pushed
the
azherdev/hmr-timestamps
branch
from
82438b3 to
e8266dc
Compare
| }, | ||
| ); | ||
|
|
||
| it.v2( |
There was a problem hiding this comment.
Any reason you marked this as v2?
3 participants
Motivation
Timestamp-based cache busting is linked to Chrome devtools crashing in Confluence due to the accumulation of duplicate copies of source code parsed out of sourcemaps.
Changes
Codex generated description:
Checklist
docs/folder