-
-
Notifications
You must be signed in to change notification settings - Fork 142
feat: remote module security #1899
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
base: main
Are you sure you want to change the base?
Conversation
| var hasCdn = false; | ||
| for (var i = 0; i < allowlist.length; i++) { | ||
| if (allowlist[i].indexOf("esm.sh") !== -1) hasEsmSh = true; | ||
| if (allowlist[i].indexOf("cdn.example.com") !== -1) hasCdn = true; |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
cdn.example.com
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 21 hours ago
In general, fixing incomplete URL substring sanitization means never treating the entire URL as an opaque string when validating hosts. Instead, parse each URL with a standard URL parser, extract the host or hostname, and compare that to an explicit allowlist of known-good hosts (or exact URL prefixes), avoiding substring checks that can be bypassed with crafted domains or paths.
In this file, the risky pattern appears only in the test that validates the remoteModuleAllowlist contents:
for (var i = 0; i < allowlist.length; i++) {
if (allowlist[i].indexOf("esm.sh") !== -1) hasEsmSh = true;
if (allowlist[i].indexOf("cdn.example.com") !== -1) hasCdn = true;
}We can keep the test’s intent (“the allowlist contains an entry for esm.sh and one for cdn.example.com”) while eliminating substring-matching on the whole URL. The most robust way, without changing existing functionality, is:
- Attempt to parse each
allowlist[i]as a URL using the standardURLconstructor available in modern JavaScript runtimes. - If parsing succeeds, check
url.hostname === "esm.sh"orurl.hostname === "cdn.example.com". - If parsing fails (e.g., the allowlist contains bare host strings rather than full URLs), fall back to comparing the raw string to those hostnames. This ensures the test still passes with current configurations while avoiding substring issues on actual URLs.
This change is local to the it("should parse security remoteModuleAllowlist from package.json", ...) test block in testRemoteModuleSecurity.js around lines 96–102. No new external libraries are needed; we only rely on the built-in URL global.
-
Copy modified lines R100-R118
| @@ -97,8 +97,25 @@ | ||
| var hasEsmSh = false; | ||
| var hasCdn = false; | ||
| for (var i = 0; i < allowlist.length; i++) { | ||
| if (allowlist[i].indexOf("esm.sh") !== -1) hasEsmSh = true; | ||
| if (allowlist[i].indexOf("cdn.example.com") !== -1) hasCdn = true; | ||
| var entry = allowlist[i]; | ||
| try { | ||
| // Prefer checking the hostname of a parsed URL entry | ||
| var parsed = new URL(entry); | ||
| if (parsed.hostname === "esm.sh") { | ||
| hasEsmSh = true; | ||
| } | ||
| if (parsed.hostname === "cdn.example.com") { | ||
| hasCdn = true; | ||
| } | ||
| } catch (e) { | ||
| // Fallback: support non-URL entries that may be bare hostnames | ||
| if (entry === "esm.sh") { | ||
| hasEsmSh = true; | ||
| } | ||
| if (entry === "cdn.example.com") { | ||
| hasCdn = true; | ||
| } | ||
| } | ||
| } | ||
| expect(hasEsmSh).toBe(true); | ||
| expect(hasCdn).toBe(true); |
Matching: NativeScript/ios#331