Repository navigation
fix: reject non-http URL schemes on the anonymous check instead of mangling them #83
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
Changes from 1 commit
c24a4b3
3824675
a928f19
03a0621
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2721,8 +2721,15 @@ function absolute(value, base) { | |
|
|
||
| export function normalizeUrl(input) { | ||
| const trimmed = String(input || "").trim(); | ||
| const explicitScheme = /^([a-z][a-z0-9+.-]*):\/\//i.exec(trimmed); | ||
| if (explicitScheme && !/^https?$/i.test(explicitScheme[1])) { | ||
|
Comment on lines
+2724
to
+2725
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The scheme check only recognizes forms containing Useful? React with 👍 / 👎. |
||
| throw new Error(`Unsupported URL scheme "${explicitScheme[1]}".`); | ||
| } | ||
| const withProtocol = /^https?:\/\//i.test(trimmed) ? trimmed : `https://${trimmed}`; | ||
|
Comment on lines
+2724
to
2728
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- target context ---'
sed -n '2670,2775p' shared/audit-engine.js
printf '%s\n' '--- relevant symbols and call sites ---'
rg -n -C 3 'explicitScheme|withProtocol|new URL|DNS|dns|audit' shared/audit-engine.js
printf '%s\n' '--- related tests and documentation ---'
rg -n -i -C 3 'example\.com:8080|unsupported URL scheme|javascript:|mailto:|data:|URL validation|audit-engine' . \
-g '!node_modules' -g '!dist' -g '!build'Repository: nish3451/seo-fix-kit Length of output: 50376 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- audit-engine URL tests ---'
rg -n -C 5 'normalizeUrl|parseAuditCompetitorUrls|publicAuditUrlStatus|competitorUrls' shared/audit-engine.test.mjs
printf '%s\n' '--- URL guard implementation ---'
sed -n '2770,2830p' shared/audit-engine.js
printf '%s\n' '--- standalone behavior probe ---'
node - <<'JS'
const inputs = [
"javascript:/alert(1)",
"mailto:/hello",
"data:/text/plain",
"example.com:8080/page",
"javascript:123",
"javascript:123/page",
"https://example.com/page",
"ftp://example.com/page"
];
const currentScheme = /^([a-z][a-z0-9+.-]*):\\/\\//i;
const currentHttp = /^https?:\\/\\//i;
const proposedHostPort = /^[a-z0-9.-]+:\\d+(?:[/?#]|$)/i;
const proposedScheme = /^([a-z][a-z0-9+.-]*):/i;
for (const input of inputs) {
const trimmed = input.trim();
const currentMatch = currentScheme.exec(trimmed);
const currentRejected = Boolean(currentMatch && !/^https?$/i.test(currentMatch[1]));
const currentValue = currentRejected ? null : (currentHttp.test(trimmed) ? trimmed : `https://${trimmed}`);
let currentParsed = null;
try {
currentParsed = currentValue && new URL(currentValue);
} catch {}
const proposedMatch = proposedScheme.exec(trimmed);
const proposedRejected = Boolean(
proposedMatch &&
!proposedHostPort.test(trimmed) &&
!/^https?$/i.test(proposedMatch[1])
);
const proposedValue = proposedRejected ? null : (currentHttp.test(trimmed) ? trimmed : `https://${trimmed}`);
let proposedParsed = null;
try {
proposedParsed = proposedValue && new URL(proposedValue);
} catch {}
console.log(JSON.stringify({
input,
current: currentParsed && { href: currentParsed.href, protocol: currentParsed.protocol, hostname: currentParsed.hostname, port: currentParsed.port },
proposed: proposedRejected ? { rejected: true } : proposedParsed && { href: proposedParsed.href, protocol: proposedParsed.protocol, hostname: proposedParsed.hostname, port: proposedParsed.port }
}));
}
JSRepository: nish3451/seo-fix-kit Length of output: 4512 🏁 Script executed: #!/bin/bash
set -eu
node - <<'JS'
const inputs = [
"javascript:/alert(1)",
"mailto:/hello",
"data:/text/plain",
"example.com:8080/page",
"javascript:123",
"javascript:123/page",
"https://example.com/page",
"ftp://example.com/page"
];
const currentScheme = /^([a-z][a-z0-9+.-]*):\/\//i;
const currentHttp = /^https?:\/\//i;
const proposedHostPort = /^[a-z0-9.-]+:\d+(?:[/?#]|$)/i;
const proposedScheme = /^([a-z][a-z0-9+.-]*):/i;
for (const input of inputs) {
const trimmed = input.trim();
const currentMatch = currentScheme.exec(trimmed);
const currentRejected = Boolean(currentMatch && !/^https?$/i.test(currentMatch[1]));
const currentValue = currentRejected
? null
: (currentHttp.test(trimmed) ? trimmed : `https://${trimmed}`);
let currentParsed = null;
try {
currentParsed = currentValue && new URL(currentValue);
} catch {}
const proposedMatch = proposedScheme.exec(trimmed);
const proposedRejected = Boolean(
proposedMatch &&
!proposedHostPort.test(trimmed) &&
!/^https?$/i.test(proposedMatch[1])
);
const proposedValue = proposedRejected
? null
: (currentHttp.test(trimmed) ? trimmed : `https://${trimmed}`);
let proposedParsed = null;
try {
proposedParsed = proposedValue && new URL(proposedValue);
} catch {}
console.log(JSON.stringify({
input,
current: currentRejected
? { rejected: true, reason: "scheme check" }
: currentParsed && {
href: currentParsed.href,
protocol: currentParsed.protocol,
hostname: currentParsed.hostname,
port: currentParsed.port
},
proposed: proposedRejected
? { rejected: true, reason: "scheme check" }
: proposedParsed && {
href: proposedParsed.href,
protocol: proposedParsed.protocol,
hostname: proposedParsed.hostname,
port: proposedParsed.port
}
}));
}
JSRepository: nish3451/seo-fix-kit Length of output: 1805 Reject non-HTTP schemes before adding The 🧰 Tools🪛 OpenGrep (1.26.0)[ERROR] 2724-2724: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead. (coderabbit.command-injection.exec-js) 🤖 Prompt for AI Agents |
||
| const url = new URL(withProtocol); | ||
| if (url.username || url.password) { | ||
| throw new Error("URLs with embedded credentials are not supported."); | ||
| } | ||
| url.hash = ""; | ||
| return url.href; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| import assert from "node:assert/strict"; | ||
| import test from "node:test"; | ||
| import { createAuditEngine, isThrottledResource } from "./audit-engine.js"; | ||
| import { createAuditEngine, isThrottledResource, normalizeUrl } from "./audit-engine.js"; | ||
| import { buildBacklinkAudit } from "./backlink-audit.js"; | ||
| import { buildCrawlInventory } from "./crawl-inventory.js"; | ||
| import { buildLocalSeoAudit } from "./local-seo-audit.js"; | ||
|
|
@@ -759,3 +759,20 @@ test("rendered-vs-static guards stay truthful for the live script-rendered fixtu | |
| `expected an internal-links guard, got: ${guardTitles.join(" | ")}` | ||
| ); | ||
| }); | ||
|
|
||
| test("normalizeUrl adds https to scheme-less input and keeps valid http(s) URLs intact", () => { | ||
| assert.equal(normalizeUrl("example.com/about?q=1"), "https://example.com/about?q=1"); | ||
| assert.equal(normalizeUrl(" example.com "), "https://example.com/"); | ||
| assert.equal(normalizeUrl("//example.com/about"), "https://example.com/about"); | ||
| assert.equal(normalizeUrl("https://example.com/a?b=c#frag"), "https://example.com/a?b=c"); | ||
| assert.equal(normalizeUrl("HTTP://EXAMPLE.COM/a"), "http://example.com/a"); | ||
| assert.equal(normalizeUrl("example.com:8080/page"), "https://example.com:8080/page"); | ||
| }); | ||
|
|
||
| test("normalizeUrl rejects non-http schemes and embedded credentials instead of mangling them", () => { | ||
| assert.throws(() => normalizeUrl("ftp://example.com"), /Unsupported URL scheme "ftp"/); | ||
| assert.throws(() => normalizeUrl("javascript://example.com/x"), /Unsupported URL scheme "javascript"/); | ||
| assert.throws(() => normalizeUrl("mailto:hello@example.com"), /embedded credentials/i); | ||
| assert.throws(() => normalizeUrl("https://user:pass@example.com/"), /embedded credentials/i); | ||
| assert.throws(() => normalizeUrl("https://user@example.com/"), /embedded credentials/i); | ||
| }); | ||
|
Comment on lines
+772
to
+778
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- candidate files ---'
fd -i 'audit-engine|normalize' . | head -80
printf '%s\n' '--- normalizeUrl definitions and uses ---'
rg -n -C 8 'normalizeUrl|Unsupported URL scheme|embedded credentials' . --glob '!node_modules' --glob '!dist' --glob '!build'Repository: nish3451/seo-fix-kit Length of output: 29231 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- files ---'
git ls-files | rg '(^|/)(shared/)?audit-engine|normalize|package.json'
printf '%s\n' '--- relevant source ---'
rg -n -C 12 'function normalizeUrl|const normalizeUrl|normalizeUrl\s*=|Unsupported URL scheme|embedded credentials' . \
--glob '!node_modules' --glob '!dist' --glob '!build'Repository: nish3451/seo-fix-kit Length of output: 9796 🏁 Script executed: #!/bin/bash
set -e
rg -n -C 12 'normalizeUrl|Unsupported URL scheme|embedded credentials' . \
--glob '!node_modules' --glob '!dist' --glob '!build'Repository: nish3451/seo-fix-kit Length of output: 40170 🏁 Script executed: #!/bin/bash
set -e
node <<'JS'
function currentNormalizeUrl(input) {
const trimmed = String(input || "").trim();
const explicitScheme = /^([a-z][a-z0-9+.-]*):\/\//i.exec(trimmed);
if (explicitScheme && !/^https?$/i.test(explicitScheme[1])) {
throw new Error(`Unsupported URL scheme "${explicitScheme[1]}".`);
}
const withProtocol = /^https?:\/\//i.test(trimmed) ? trimmed : `https://${trimmed}`;
const url = new URL(withProtocol);
if (url.username || url.password) {
throw new Error("URLs with embedded credentials are not supported.");
}
url.hash = "";
return url.href;
}
for (const value of [
"ftp://example.com",
"javascript://example.com/x",
"mailto:hello@example.com",
"mailto:/hello",
"javascript:/alert(1)",
"https://user:pass@example.com/",
"https://user@example.com/"
]) {
try {
console.log(JSON.stringify({ value, result: currentNormalizeUrl(value) }));
} catch (error) {
console.log(JSON.stringify({ value, error: error.message }));
}
}
JSRepository: nish3451/seo-fix-kit Length of output: 746 Reject explicit non-HTTP schemes before adding 🤖 Prompt for AI Agents |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fresh evidence in this revision is that the new scheme matcher still requires
://:normalizeUrl("tel:1234")currently returnshttps://tel:1234/, whichpublicAuditUrlStatusaccepts. Consequently,/api/public-checkproceeds through quota accounting and attempts a browser audit instead of returning the intended 400 response; detect the leadingscheme:form regardless of whether it includes slashes.Useful? React with 👍 / 👎.