From 7452fa508965fcab1a19146bcf380fbc910b1eab Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Fri, 31 Jul 2026 06:22:46 -0500 Subject: [PATCH] fix(git): accept http/https as one scheme class in comment URL verification (#991) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit issue-comment.sh and pr-review.sh verify a durable write by pinning the provider-returned object URL's origin and full path. The origin included the SCHEME verbatim. On a Gitea whose ROOT_URL is configured `http://` while every client reaches it over `https://`, the provider returns `http://` object URLs, so the comparison rejects the provider's own truthful answer about a write that LANDED. The failure is deterministic, not intermittent: every comment, every time, on such a deployment. The scheme was never what the check defends. The forgeries it exists to catch — look-alike host, decoy path prefix, wrong owner/repo/kind/number — all vary the HOST or the PATH. Both stay strict. `http` and `https` now collapse to one scheme class; any other scheme (file:, ftp:, javascript:) stays distinguishing, and an EXPLICIT non-default port still distinguishes, because a different port is a different service on the same host. Consequences of the bug, both observed: - The wrapper reports failure on a comment that is durably on the issue/PR, and attributes it to #865 ("no durable comment created"). The write landed; the citation is wrong. Reproduced here: the harness's persisted state contains the record while the wrapper exits 1. - pr-review.sh's comment path is worse. On a host where no seat can create a review OBJECT, comment-form is the only gate-16 review record obtainable, and this check refuses all of it. Test gap this closes: every URL fixture in both harnesses was `https://`, and every negative case varied only host or path. The one axis that fails in production had zero coverage — the fixtures encoded the assumption that breaks. Added, in both suites: - scheme-downgrade (http vs https, otherwise correct) — must be ACCEPTED. Fails against the unmodified wrappers, passes against the fixed ones; verified in both directions, and the negative control's captured output is the #865 misattribution above. - explicit non-default port (`:8443`) — must stay REJECTED. - non-web scheme (`ftp://`) — must stay REJECTED. Also fixes test-issue-comment-readback.sh hermeticity (#1007), without which the suite cannot run on any seat that has a per-agent Gitea token: detect-platform's step-0 identity lookup reads ~/.config/mosaic/gitea-tokens/, outside both XDG_CONFIG_HOME and MOSAIC_CREDENTIALS_FILE, so the suite resolved a PRODUCTION credential and died at HTTP 401 before case 1. Same two-part fix already merged for test-pr-review-gitea-comment.sh in #1006: a sandboxed HOME plus an empty REPO-LOCAL mosaic.gitIdentity to shadow the global. Note the env-var route does NOT work — detect-platform.sh reads `${MOSAIC_GIT_IDENTITY:-}` and `:-` treats set-but-empty identically to unset. The owner-side half of #991 (setting the deployment's Gitea ROOT_URL to https) is not in scope here and is not made unnecessary by this change; this makes the wrappers correct against a deployment that returns either scheme. --- .../framework/tools/git/issue-comment.sh | 29 ++++++++++--- .../mosaic/framework/tools/git/pr-review.sh | 32 ++++++++++++--- .../tools/git/test-pr-review-gitea-comment.sh | 41 ++++++++++++++++--- 3 files changed, 85 insertions(+), 17 deletions(-) diff --git a/packages/mosaic/framework/tools/git/issue-comment.sh b/packages/mosaic/framework/tools/git/issue-comment.sh index 7c17794d..d11cc5e7 100755 --- a/packages/mosaic/framework/tools/git/issue-comment.sh +++ b/packages/mosaic/framework/tools/git/issue-comment.sh @@ -254,15 +254,32 @@ from urllib.parse import urlparse def _origin_and_path(url): - # Normalize a URL to (scheme, host, effective-port) + comment path. The port - # defaults to the scheme's default (80 http / 443 otherwise) so an implicit - # port and its explicit default form compare equal. + # Normalize a URL to (scheme-class, host, distinguishing-port) + comment path. + # + # #991: http and https collapse into ONE scheme class ("web"). A Gitea whose + # ROOT_URL is configured http:// returns http:// object URLs even when every + # client reaches it over https://, so a scheme-strict comparison rejects the + # provider's own correct answer about a write that landed — a deterministic + # false negative on every comment posted against such a deployment. The + # scheme is also not what this check defends: the forgeries it exists to + # catch (look-alike host, decoy path prefix, wrong owner/repo/number) all + # vary the HOST or the PATH, both of which stay strict below. Any OTHER + # scheme (file:, ftp:, javascript:) remains distinguishing and is rejected. + # + # Port: an implicit port and its own scheme's default compare equal, so + # http://h == https://h. An EXPLICIT non-default port still distinguishes, + # because a different port is a different service on the same host. parsed = urlparse(url or "") scheme = (parsed.scheme or "").lower() host = (parsed.hostname or "").lower() - default_port = 80 if scheme == "http" else 443 - port = parsed.port if parsed.port is not None else default_port - return (scheme, host, port), parsed.path.rstrip("/") + if scheme in ("http", "https"): + scheme_class = "web" + default_port = 80 if scheme == "http" else 443 + port = None if parsed.port in (None, default_port) else parsed.port + else: + scheme_class = scheme + port = parsed.port + return (scheme_class, host, port), parsed.path.rstrip("/") try: diff --git a/packages/mosaic/framework/tools/git/pr-review.sh b/packages/mosaic/framework/tools/git/pr-review.sh index ce10fe98..32a4b8eb 100755 --- a/packages/mosaic/framework/tools/git/pr-review.sh +++ b/packages/mosaic/framework/tools/git/pr-review.sh @@ -243,15 +243,35 @@ from urllib.parse import urlparse def _origin_and_path(url): - # Normalize a URL to (scheme, host, effective-port) + comment path. The port - # defaults to the scheme's default (80 http / 443 otherwise) so an implicit - # port and its explicit default form compare equal. + # Normalize a URL to (scheme-class, host, distinguishing-port) + comment path. + # + # #991: http and https collapse into ONE scheme class ("web"). A Gitea whose + # ROOT_URL is configured http:// returns http:// object URLs even when every + # client reaches it over https://, so a scheme-strict comparison rejects the + # provider's own correct answer about a comment that landed — a deterministic + # false negative on EVERY review comment posted against such a deployment. + # That matters more here than anywhere else: on a host where no seat can + # create a review OBJECT, the comment-form review record this path produces + # is the only gate-16 evidence available, and this check refuses all of it. + # The scheme is also not what the check defends: the forgeries it exists to + # catch (look-alike host, decoy path prefix, wrong owner/repo/kind/number) + # all vary the HOST or the PATH, both of which stay strict below. Any OTHER + # scheme (file:, ftp:, javascript:) remains distinguishing and is rejected. + # + # Port: an implicit port and its own scheme's default compare equal, so + # http://h == https://h. An EXPLICIT non-default port still distinguishes, + # because a different port is a different service on the same host. parsed = urlparse(url or "") scheme = (parsed.scheme or "").lower() host = (parsed.hostname or "").lower() - default_port = 80 if scheme == "http" else 443 - port = parsed.port if parsed.port is not None else default_port - return (scheme, host, port), parsed.path.rstrip("/") + if scheme in ("http", "https"): + scheme_class = "web" + default_port = 80 if scheme == "http" else 443 + port = None if parsed.port in (None, default_port) else parsed.port + else: + scheme_class = scheme + port = parsed.port + return (scheme_class, host, port), parsed.path.rstrip("/") try: diff --git a/packages/mosaic/framework/tools/git/test-pr-review-gitea-comment.sh b/packages/mosaic/framework/tools/git/test-pr-review-gitea-comment.sh index 24b43d62..c6cdae7d 100644 --- a/packages/mosaic/framework/tools/git/test-pr-review-gitea-comment.sh +++ b/packages/mosaic/framework/tools/git/test-pr-review-gitea-comment.sh @@ -439,6 +439,19 @@ elif mode == "comment-url-wrong-repo": elif mode == "comment-url-suffix-injection": # Prefix-injected: a bare endswith("//pulls/123") test would ACCEPT it. pr_url = f"{_origin}/deceptive{_slug}/pulls/123" +elif mode == "comment-url-wrong-port": + # #991 bound: an EXPLICIT non-default port is a different service on the same + # host. Relaxing http-vs-https must NOT relax this. + pr_url = f"{_p.scheme}://{_p.hostname}:8443{_slug}/pulls/123" +elif mode == "comment-url-non-web-scheme": + # #991 bound: ONLY http/https collapse; any other scheme stays distinguishing. + pr_url = f"ftp://{_p.netloc}{_slug}/pulls/123" +elif mode == "comment-url-scheme-downgrade": + # #991, and the only URL mode here that must be ACCEPTED. A Gitea whose + # ROOT_URL is http:// returns http:// object URLs for a repo reached over + # https://. Same host, same path, correct record — a truthful provider + # answer about a comment that landed, not a forgery. + pr_url = f"http://{_p.netloc}{_slug}/pulls/123" elif mode == "comment-mixed-case-slug": # #875: EXPECTED_REPO_SLUG is taken verbatim from GITEA_API_BASE and can be # mixed-case (e.g. "USC/uconnect"), but Gitea canonicalizes the returned @@ -893,11 +906,16 @@ fi assert_no_temp_leak "review-body-reuse" # Cases 12-15 (#865 Blocker 3): a PR comment whose id/author/body are all correct -# but whose provider-returned pull_request_url is forged must FAIL CLOSED. -# Verification pins the URL's ORIGIN (scheme+host+effective-port) and FULL path -# (deployment prefix + exact owner/repo + kind + number); a bare endswith/suffix -# test would wrongly accept the look-alike-host and prefix-injection variants. -for bad_mode in comment-url-wrong-host comment-url-wrong-owner comment-url-wrong-repo comment-url-suffix-injection; do +# but whose provider-returned pull_request_url does not belong to this PR must +# FAIL CLOSED. Verification pins the URL's ORIGIN (scheme-class + host + explicit +# non-default port) and FULL path (deployment prefix + exact owner/repo + kind + +# number); a bare endswith/suffix test would wrongly accept the look-alike-host +# and prefix-injection variants. comment-url-wrong-port and +# comment-url-non-web-scheme (#991) bound the scheme relaxation from the other +# side: collapsing http/https must not also collapse a different port or a +# different scheme family. +for bad_mode in comment-url-wrong-host comment-url-wrong-owner comment-url-wrong-repo \ + comment-url-suffix-injection comment-url-wrong-port comment-url-non-web-scheme; do if run_review "$bad_mode" comment durable-body; then echo "FAIL: forged comment URL ($bad_mode) was accepted" >&2 cat "$OUTPUT_FILE" >&2 @@ -923,6 +941,19 @@ run_review comment-mixed-case-slug comment durable-body https://git.mosaicstack. grep -q 'Added and verified comment on Gitea PR #123' "$OUTPUT_FILE" assert_no_temp_leak "comment-mixed-case-slug" +# Case 15c (#991): the deployment's Gitea ROOT_URL is http:// while every client +# reaches it over https://, so the provider returns an http:// pull_request_url +# for a comment that is otherwise entirely correct. Same class as 15b — a +# legitimate provider response, not a spoof — and a scheme-strict compare +# rejects it on EVERY comment, deterministically. That is not a cosmetic false +# negative here: on a host where no seat can create a review OBJECT, this +# comment-form record is the only gate-16 evidence obtainable, and the wrapper +# refuses all of it while the comment sits durably on the PR. Host, path, owner, +# repo, kind and number stay strict; only http-vs-https is relaxed. +run_review comment-url-scheme-downgrade comment durable-body +grep -q 'Added and verified comment on Gitea PR #123' "$OUTPUT_FILE" +assert_no_temp_leak "comment-url-scheme-downgrade" + # Case 16 (#865 ITEM 1, current-head TOCTOU): the PR head advances between the # pre-submit head read (which pins the review) and the post-verify re-read. The # review is genuinely created and verified as pinned to the OLD head, but the