Skip to content

Conversation

@NathanWalker
Copy link
Contributor

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
' can be anywhere in the URL, and arbitrary hosts may come before or after it.

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:

  1. Attempt to parse each allowlist[i] as a URL using the standard URL constructor available in modern JavaScript runtimes.
  2. If parsing succeeds, check url.hostname === "esm.sh" or url.hostname === "cdn.example.com".
  3. 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.


Suggested changeset 1
test-app/app/src/main/assets/app/tests/testRemoteModuleSecurity.js

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/test-app/app/src/main/assets/app/tests/testRemoteModuleSecurity.js b/test-app/app/src/main/assets/app/tests/testRemoteModuleSecurity.js
--- a/test-app/app/src/main/assets/app/tests/testRemoteModuleSecurity.js
+++ b/test-app/app/src/main/assets/app/tests/testRemoteModuleSecurity.js
@@ -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);
EOF
@@ -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);
Copilot is powered by AI and may make mistakes. Always verify output.
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