Repository navigation
fix: reject non-http URL schemes on the anonymous check instead of mangling them - #83
Conversation
…ngling them The public one-page check normalized any non-http scheme into garbage instead of rejecting it: ftp://example.com became https://ftp//example.com (422 with a cryptic net::ERR_CONNECTION_RESET error) and mailto:hello@example.com silently audited example.com with a mangled displayed URL. The shared normalizeUrl also accepted embedded credentials. Every surface that uses normalizeUrl (public check, audits, developer API, local server) now rejects unsupported schemes and credential-bearing URLs with the standard invalid-URL error. The /check form input type=url also blocked scheme-less entries ("example.com") client-side with no message even though the server normalizes them; the input now accepts plain text with inputmode=url so the server's normalization runs. - normalizeUrl throws on explicit non-http(s) schemes and embedded credentials - /check input is type=text inputmode=url, pinned in route tests - unit pins for normalizeUrl and route pins for validatePublicCheckUrl - live-promise spot-check gains a scheme-rejection probe (red against the currently deployed Worker, green after deploy)
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
nish3451 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Warning Review limit reached
Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR hardens URL normalization, changes the public-check field to text input, expands validation tests for unsupported schemes and credentials, and adds a live FTP rejection spot-check. ChangesPublic URL validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c24a4b3208
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const explicitScheme = /^([a-z][a-z0-9+.-]*):\/\//i.exec(trimmed); | ||
| if (explicitScheme && !/^https?$/i.test(explicitScheme[1])) { |
There was a problem hiding this comment.
The scheme check only recognizes forms containing ://, but URI schemes do not universally use that delimiter. For example, tel:1234 bypasses this check, is normalized to https://tel:1234/, and passes publicAuditUrlStatus, so the anonymous check still attempts a browser render and returns a runtime error instead of the intended 400 response. Detect the scheme from the leading name: portion before adding HTTPS.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shared/audit-engine.js`:
- Around line 2724-2728: Update the URL normalization logic around
explicitScheme and withProtocol to detect any non-HTTP scheme using the full
scheme: prefix, including single-slash and no-slash forms, before prepending
https://. Preserve host:port inputs such as example.com:8080/page, but ensure
the exception does not allow scheme-like values such as javascript:123.
In `@shared/audit-engine.test.mjs`:
- Around line 772-778: Update normalizeUrl’s scheme detection to reject explicit
non-HTTP schemes, including mailto and single-slash forms, before prepending
https://; emit Unsupported URL scheme "<scheme>" for these inputs. Preserve
example.com:8080/page as a host with a port, and extend the normalizeUrl tests
with mailto:, mailto:/hello, and javascript:/alert(1) cases.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f036c2cc-de0b-47eb-a074-24e0858ffd82
📒 Files selected for processing (6)
scripts/live-promise-spot-check.mjsscripts/live-promise-spot-check.test.mjsshared/audit-engine.jsshared/audit-engine.test.mjsworker/routes/public-check.jsworker/routes/public-check.test.mjs
| 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}`; |
There was a problem hiding this comment.
🎯 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 https://.
The scheme:// check lets javascript:/alert(1), mailto:/hello, and data:/text/plain become HTTPS URLs for hosts named javascript, mailto, and data. Apply the check to the full scheme: prefix. Keep the example.com:8080/page host-port form, but do not let the exception admit scheme-like inputs such as javascript:123.
🧰 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
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shared/audit-engine.js` around lines 2724 - 2728, Update the URL
normalization logic around explicitScheme and withProtocol to detect any
non-HTTP scheme using the full scheme: prefix, including single-slash and
no-slash forms, before prepending https://. Preserve host:port inputs such as
example.com:8080/page, but ensure the exception does not allow scheme-like
values such as javascript:123.
| 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); | ||
| }); |
There was a problem hiding this comment.
🎯 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 https://. Extend scheme detection to cover mailto: and single-slash forms, while preserving example.com:8080/page as a host with a port. Expect Unsupported URL scheme "mailto" and add cases for mailto:/hello and javascript:/alert(1).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shared/audit-engine.test.mjs` around lines 772 - 778, Update normalizeUrl’s
scheme detection to reject explicit non-HTTP schemes, including mailto and
single-slash forms, before prepending https://; emit Unsupported URL scheme
"<scheme>" for these inputs. Preserve example.com:8080/page as a host with a
port, and extend the normalizeUrl tests with mailto:, mailto:/hello, and
javascript:/alert(1) cases.
There was a problem hiding this comment.
nish3451 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
nish3451 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a928f19ff7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| export function normalizeUrl(input) { | ||
| const trimmed = String(input || "").trim(); | ||
| const explicitScheme = /^([a-z][a-z0-9+.-]*):\/\//i.exec(trimmed); |
There was a problem hiding this comment.
Fresh evidence in this revision is that the new scheme matcher still requires ://: normalizeUrl("tel:1234") currently returns https://tel:1234/, which publicAuditUrlStatus accepts. Consequently, /api/public-check proceeds through quota accounting and attempts a browser audit instead of returning the intended 400 response; detect the leading scheme: form regardless of whether it includes slashes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
nish3451 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
What
The anonymous one-page check at /check accepted any URL prefix and mangled non-http schemes into garbage instead of rejecting them:
ftp://example.com-> normalized tohttps://ftp//example.com-> the public check 422s with a crypticnet::ERR_CONNECTION_RESETerrormailto:hello@example.com-> normalized tohttps://mailto:hello@example.com/and silently audits example.com with a mangled displayed URLhttps://user:pass@example.com/-> accepted with embedded credentialsVerified live against the deployed Worker (pre-fix):
POST /api/public-check {"url":"ftp://example.com"}->{"error": "net::ERR_CONNECTION_RESET at https://ftp//example.com"}(422)POST /api/public-check {"url":"mailto:test@example.com"}->{"ok": true, "checkedUrl": "https://mailto:test@example.com/", "finalUrl": "https://example.com/", ...}(silently audits the wrong host)Fix
shared/audit-engine.jsnormalizeUrl(used by public check, audits, developer API, local server, competitor/backlink/keyword/local inputs) now throws on explicit non-http(s) schemes and on URLs carrying embedded credentials; every caller already maps that throw toEnter a valid public website URL.(400)/checkform input changedtype="url"->type="text"(withinputmode="url"): Chrome silently blocked scheme-less entries likeexample.comclient-side withPlease enter a URL.even though the server normalizes them finePins
normalizeUrl(scheme-less, protocol-relative, http(s) intact, ftp/javascript/mailto/credentials rejected)validatePublicCheckUrl+ the form input attributesVerification
npm run checkfully green (all suites + build)shared/audit-engine.test.mjs,worker/routes/public-check.test.mjs,scripts/live-promise-spot-check.test.mjs(42/42)Summary by CodeRabbit
Bug Fixes
Tests