From b67068027ab14f7782bbd2981c05e77056a88bca Mon Sep 17 00:00:00 2001 From: kai392 Date: Thu, 30 Jul 2026 03:23:14 +0800 Subject: [PATCH] fix(content-lane): first-hop source URL validation failure is a hard failure, not "passed" (#9669) checkOneSourceUrl's first-hop path mapped a `source_host_not_checked` validation failure (plain http:// or a private/loopback host that fails isSafeHttpUrl) to status "passed", while the redirect-loop path treats the identical outcome as a hard failure. So an unfetchable canonical source silently reported the whole item OK, and a report with only such a source came back "passed". Remove the `invalidProtocol` ternary so BOTH validator failure outcomes hard-fail on the first hop, identical to the redirect path. outcome/error still carry the validator's own values. downgradeInconclusiveSourceWarnings remains the only mechanism that makes a distribution-role source_host_not_checked non-blocking, so a distribution http:// source alongside a verifiable canonical is still relaxed -- via the correct mechanism, not by mislabelling it "passed". Tests: first-hop http:// and https-loopback are hard_failure with outcome source_host_not_checked; a canonical http:// with no verifiable canonical fails the report; a distribution http:// alongside a reachable primary canonical is downgraded to blocking:false. Updated the two prior tests that pinned the old "passed" behaviour. Closes #9669 Co-Authored-By: Claude Opus 4.8 --- src/review/content-lane/source-evidence.ts | 12 +++--- .../unit/content-lane-source-evidence.test.ts | 42 +++++++++++++------ 2 files changed, 35 insertions(+), 19 deletions(-) diff --git a/src/review/content-lane/source-evidence.ts b/src/review/content-lane/source-evidence.ts index 2f2aaf0b32..b2e1a5d9d0 100644 --- a/src/review/content-lane/source-evidence.ts +++ b/src/review/content-lane/source-evidence.ts @@ -417,14 +417,14 @@ async function checkOneSourceUrl( ): Promise { const validation = validateFetchableSourceUrl(item.url); if (!validation.ok) { - const invalidProtocol = validation.outcome === "invalid_url"; + // #9669: BOTH validator failure outcomes are a hard failure on the first hop, identical to the + // redirect-loop path -- a `source_host_not_checked` URL (plain http:// or a private/loopback host) used to + // fall through as "passed" here, so an unfetchable canonical source silently reported the whole item OK. + // A distribution-role http:// source is still made non-blocking, but only via the intended + // downgradeInconclusiveSourceWarnings mechanism, never by mislabelling it "passed" at this hop. return withSourceDefaults( item, - { - status: invalidProtocol ? "hard_failure" : "passed", - outcome: validation.outcome, - error: validation.error, - }, + { status: "hard_failure", outcome: validation.outcome, error: validation.error }, spec, ); } diff --git a/test/unit/content-lane-source-evidence.test.ts b/test/unit/content-lane-source-evidence.test.ts index 05305d91cf..144c8007a1 100644 --- a/test/unit/content-lane-source-evidence.test.ts +++ b/test/unit/content-lane-source-evidence.test.ts @@ -285,24 +285,40 @@ describe("checkSubmittedSourceEvidence — invalid / non-fetchable source URLs", expect(item?.outcome).toBe("invalid_url"); }); - it("treats a non-https (http) URL as a non-blocking 'passed' (source_host_not_checked)", async () => { - // validateFetchableSourceUrl: http passes the protocol check but fails isSafeHttpUrl (needs https), - // so the outcome is source_host_not_checked → checkOneSourceUrl maps non-invalid to status 'passed'. - const src = mdx({ githubUrl: "http://github.com/acme/x" }); + it("#9669: a non-https (http) canonical URL is a hard failure on the first hop and fails the report", async () => { + // validateFetchableSourceUrl: http passes the protocol check but fails isSafeHttpUrl (needs https), so the + // outcome is source_host_not_checked. Before #9669 the first hop mislabelled that "passed"; it is a + // hard_failure now, identical to the redirect path -- and a canonical hard-failure with no verifiable + // canonical to fall back on fails the whole report (today: "passed"). + const src = mdx({ githubUrl: "http://example.com/x" }); const report = await checkSubmittedSourceEvidence(src, fakeFetch({})); const item = report.urls.find((u) => u.field === "githubUrl"); - expect(item?.status).toBe("passed"); + expect(item?.status).toBe("hard_failure"); expect(item?.outcome).toBe("source_host_not_checked"); - expect(report.status).toBe("passed"); + expect(report.status).toBe("failed"); }); - it("treats an https loopback host as source_host_not_checked (SSRF guard), status passed", async () => { - const src = mdx({ githubUrl: "https://127.0.0.1/repo" }); + it("#9669: an https loopback host is a hard failure on the first hop (SSRF guard, source_host_not_checked)", async () => { + const src = mdx({ githubUrl: "https://127.0.0.1/x" }); const report = await checkSubmittedSourceEvidence(src, fakeFetch({})); const item = report.urls.find((u) => u.field === "githubUrl"); - expect(item?.status).toBe("passed"); + expect(item?.status).toBe("hard_failure"); expect(item?.outcome).toBe("source_host_not_checked"); }); + + it("#9669 REGRESSION: a distribution-role http:// source stays non-blocking via the downgrade when a verifiable canonical exists", async () => { + // githubUrl is a reachable PRIMARY canonical -> hasVerifiableCanonicalSource is true; packageUrl is a + // distribution field whose http:// value is source_host_not_checked -> hard_failure, then downgraded to + // blocking:false by downgradeInconclusiveSourceWarnings (the intended relaxation, via the correct mechanism). + const src = mdx({ githubUrl: "https://github.com/acme/x", packageUrl: "http://dist.example/pkg" }); + const spec = customSpec({ primaryCanonicalSourceFields: new Set(["githubUrl"]) }); + const report = await checkSubmittedSourceEvidence(src, fakeFetch({ "https://github.com/acme/x": 200 }), spec); + const dist = report.urls.find((u) => u.field === "packageUrl"); + expect(dist?.role).toBe("distribution"); + expect(dist?.status).toBe("hard_failure"); + expect(dist?.outcome).toBe("source_host_not_checked"); + expect(dist?.blocking).toBe(false); + }); }); describe("extractSubmittedSourceUrls — frontmatter parsing edge cases", () => { @@ -751,13 +767,13 @@ describe("checkOneSourceUrl branches", () => { expect(getCalls).toBe(0); }); - it("non-invalid validation failure → status 'passed' (invalidProtocol false branch)", async () => { - // source_host_not_checked (https loopback) is a non-invalid validation failure: invalidProtocol=false - // → status 'passed'. Distinct from invalid_url (which is hard_failure). + it("#9669: a non-invalid validation failure (source_host_not_checked) is now a hard failure — the invalidProtocol branch is gone", async () => { + // Both validateFetchableSourceUrl failure outcomes now hard-fail on the first hop, identical to the + // redirect-loop path; an https-loopback source_host_not_checked is no longer mislabelled "passed". const src = mdx({ githubUrl: "https://127.0.0.1/repo" }); const report = await checkSubmittedSourceEvidence(src, fakeFetch({})); const item = report.urls.find((u) => u.field === "githubUrl"); - expect(item?.status).toBe("passed"); + expect(item?.status).toBe("hard_failure"); expect(item?.outcome).toBe("source_host_not_checked"); });