fix(git): accept http/https as one scheme class in comment URL verification (#991)
ci/woodpecker/pr/ci Pipeline failed

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/<identity>, 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.
This commit is contained in:
Jason Woltje
2026-08-11 18:20:42 -05:00
parent 540ec5b6ef
commit 7452fa5089
3 changed files with 85 additions and 17 deletions
@@ -254,15 +254,32 @@ from urllib.parse import urlparse
def _origin_and_path(url): def _origin_and_path(url):
# Normalize a URL to (scheme, host, effective-port) + comment path. The port # Normalize a URL to (scheme-class, host, distinguishing-port) + comment path.
# defaults to the scheme's default (80 http / 443 otherwise) so an implicit #
# port and its explicit default form compare equal. # #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 "") parsed = urlparse(url or "")
scheme = (parsed.scheme or "").lower() scheme = (parsed.scheme or "").lower()
host = (parsed.hostname or "").lower() host = (parsed.hostname or "").lower()
if scheme in ("http", "https"):
scheme_class = "web"
default_port = 80 if scheme == "http" else 443 default_port = 80 if scheme == "http" else 443
port = parsed.port if parsed.port is not None else default_port port = None if parsed.port in (None, default_port) else parsed.port
return (scheme, host, port), parsed.path.rstrip("/") else:
scheme_class = scheme
port = parsed.port
return (scheme_class, host, port), parsed.path.rstrip("/")
try: try:
@@ -243,15 +243,35 @@ from urllib.parse import urlparse
def _origin_and_path(url): def _origin_and_path(url):
# Normalize a URL to (scheme, host, effective-port) + comment path. The port # Normalize a URL to (scheme-class, host, distinguishing-port) + comment path.
# defaults to the scheme's default (80 http / 443 otherwise) so an implicit #
# port and its explicit default form compare equal. # #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 "") parsed = urlparse(url or "")
scheme = (parsed.scheme or "").lower() scheme = (parsed.scheme or "").lower()
host = (parsed.hostname or "").lower() host = (parsed.hostname or "").lower()
if scheme in ("http", "https"):
scheme_class = "web"
default_port = 80 if scheme == "http" else 443 default_port = 80 if scheme == "http" else 443
port = parsed.port if parsed.port is not None else default_port port = None if parsed.port in (None, default_port) else parsed.port
return (scheme, host, port), parsed.path.rstrip("/") else:
scheme_class = scheme
port = parsed.port
return (scheme_class, host, port), parsed.path.rstrip("/")
try: try:
@@ -439,6 +439,19 @@ elif mode == "comment-url-wrong-repo":
elif mode == "comment-url-suffix-injection": elif mode == "comment-url-suffix-injection":
# Prefix-injected: a bare endswith("/<slug>/pulls/123") test would ACCEPT it. # Prefix-injected: a bare endswith("/<slug>/pulls/123") test would ACCEPT it.
pr_url = f"{_origin}/deceptive{_slug}/pulls/123" 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": elif mode == "comment-mixed-case-slug":
# #875: EXPECTED_REPO_SLUG is taken verbatim from GITEA_API_BASE and can be # #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 # mixed-case (e.g. "USC/uconnect"), but Gitea canonicalizes the returned
@@ -893,11 +906,16 @@ fi
assert_no_temp_leak "review-body-reuse" assert_no_temp_leak "review-body-reuse"
# Cases 12-15 (#865 Blocker 3): a PR comment whose id/author/body are all correct # 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. # but whose provider-returned pull_request_url does not belong to this PR must
# Verification pins the URL's ORIGIN (scheme+host+effective-port) and FULL path # FAIL CLOSED. Verification pins the URL's ORIGIN (scheme-class + host + explicit
# (deployment prefix + exact owner/repo + kind + number); a bare endswith/suffix # non-default port) and FULL path (deployment prefix + exact owner/repo + kind +
# test would wrongly accept the look-alike-host and prefix-injection variants. # number); a bare endswith/suffix test would wrongly accept the look-alike-host
for bad_mode in comment-url-wrong-host comment-url-wrong-owner comment-url-wrong-repo comment-url-suffix-injection; do # 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 if run_review "$bad_mode" comment durable-body; then
echo "FAIL: forged comment URL ($bad_mode) was accepted" >&2 echo "FAIL: forged comment URL ($bad_mode) was accepted" >&2
cat "$OUTPUT_FILE" >&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" grep -q 'Added and verified comment on Gitea PR #123' "$OUTPUT_FILE"
assert_no_temp_leak "comment-mixed-case-slug" 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 # 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 # 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 # review is genuinely created and verified as pinned to the OLD head, but the