fix(git): accept http/https as one scheme class in comment URL verification (#991)
ci/woodpecker/pr/ci Pipeline was successful
ci/woodpecker/pr/ci Pipeline was successful
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:
@@ -41,7 +41,13 @@
|
||||
# that other host's credential cross-host;
|
||||
# 10. leaves NO temp files behind (POST/GET bodies + metadata) on either the
|
||||
# success or the failure path — nested function-scoped RETURN traps do not
|
||||
# clobber each other and every scratch file is removed on all exit paths.
|
||||
# clobber each other and every scratch file is removed on all exit paths;
|
||||
# 11. (#991) ACCEPTS a record whose provider-returned URL differs from the repo
|
||||
# remote ONLY in http-vs-https — the standing state on a deployment whose
|
||||
# Gitea ROOT_URL is misconfigured — while still REJECTING a different
|
||||
# explicit port and a non-web scheme. Every prior fixture here was https://
|
||||
# and every negative varied only host or path, so the one axis that fails
|
||||
# in production had zero coverage: the fixtures encoded the assumption.
|
||||
|
||||
set -euo pipefail
|
||||
|
||||
@@ -61,15 +67,29 @@ STATE_FILE="$WORK_DIR/comments.json"
|
||||
# A dedicated scratch dir the wrapper is pointed at via TMPDIR, so the leak
|
||||
# check can assert every POST/GET body + metadata temp file is cleaned up.
|
||||
TMP_SCRATCH="$WORK_DIR/scratch"
|
||||
# #1007: a sandboxed HOME. detect-platform.sh's step-0 per-agent identity lookup
|
||||
# reads ~/.config/mosaic/gitea-tokens/<identity>, which is OUTSIDE both
|
||||
# XDG_CONFIG_HOME and MOSAIC_CREDENTIALS_FILE — so on any seat that has a real
|
||||
# per-agent token the suite resolves a PRODUCTION credential and dies at the
|
||||
# authenticated-identity read (HTTP 401) before reaching case 1. Pointing HOME
|
||||
# at the work dir keeps that lookup inside the sandbox.
|
||||
HOME_DIR="$WORK_DIR/home"
|
||||
|
||||
cleanup() {
|
||||
rm -rf "$WORK_DIR"
|
||||
}
|
||||
trap cleanup EXIT
|
||||
|
||||
mkdir -p "$REPO_DIR" "$BIN_DIR" "$XDG_DIR" "$TMP_SCRATCH"
|
||||
mkdir -p "$REPO_DIR" "$BIN_DIR" "$XDG_DIR" "$TMP_SCRATCH" "$HOME_DIR"
|
||||
git -C "$REPO_DIR" init -q
|
||||
git -C "$REPO_DIR" remote add origin https://git.mosaicstack.dev/mosaicstack/stack.git
|
||||
# #1007: shadow any GLOBAL `mosaic.gitIdentity` with an empty REPO-LOCAL value.
|
||||
# A repo-local key shadows the global and reads back empty at rc=0, which is the
|
||||
# only way to make step 0 fall through inside a sandbox. Note the env-var route
|
||||
# (MOSAIC_GIT_IDENTITY="") does NOT work: detect-platform.sh reads it with
|
||||
# `${MOSAIC_GIT_IDENTITY:-}`, and `:-` treats set-but-empty identically to
|
||||
# unset, so setting it empty is silently the same as not setting it at all.
|
||||
git -C "$REPO_DIR" config mosaic.gitIdentity ""
|
||||
|
||||
ISSUE_NUMBER=7
|
||||
REPO_SLUG="mosaicstack/stack"
|
||||
@@ -266,6 +286,19 @@ elif mode == "url-wrong-repo":
|
||||
elif mode == "url-suffix-injection":
|
||||
# Prefix-injected: a bare endswith("/<slug>/issues/7") test would ACCEPT this.
|
||||
issue_url = f"https://git.mosaicstack.dev/deceptive/{repo}/issues/7"
|
||||
elif mode == "url-scheme-downgrade":
|
||||
# #991, and the ONLY one of these modes that must be ACCEPTED. A Gitea whose
|
||||
# ROOT_URL is http:// returns http:// object URLs for a repo cloned over
|
||||
# https://. Same host, same path, correct record — the provider is telling
|
||||
# the truth about a write that landed.
|
||||
issue_url = f"http://git.mosaicstack.dev/{repo}/issues/7"
|
||||
elif mode == "url-wrong-port":
|
||||
# An EXPLICIT non-default port is a different service on the same host and
|
||||
# must stay distinguishing — collapsing the scheme must not collapse this.
|
||||
issue_url = f"https://git.mosaicstack.dev:8443/{repo}/issues/7"
|
||||
elif mode == "url-non-web-scheme":
|
||||
# Only http/https collapse. Any other scheme stays distinguishing.
|
||||
issue_url = f"ftp://git.mosaicstack.dev/{repo}/issues/7"
|
||||
record = {
|
||||
"id": new_id,
|
||||
"body": body,
|
||||
@@ -365,6 +398,7 @@ run_comment() {
|
||||
(
|
||||
cd "$REPO_DIR"
|
||||
PATH="$BIN_DIR:$PATH" \
|
||||
HOME="$HOME_DIR" \
|
||||
TMPDIR="$TMP_SCRATCH" \
|
||||
XDG_CONFIG_HOME="$XDG_DIR" \
|
||||
MOSAIC_CREDENTIALS_FILE="$CREDENTIALS_FILE" \
|
||||
@@ -545,13 +579,17 @@ if grep -q " $ACTING_LOGIN\$" "$AUTH_LOG"; then
|
||||
fi
|
||||
assert_no_temp_leak "cross-host"
|
||||
|
||||
# Cases 7-10 (#865 Blocker 3): the created record's id/author/body are all
|
||||
# correct, but its provider-returned issue_url is forged. Verification pins the
|
||||
# URL's ORIGIN (scheme+host+effective-port) and its FULL path (deployment prefix
|
||||
# + exact owner/repo + kind + number), so each forgery must FAIL CLOSED. A bare
|
||||
# endswith/suffix test would wrongly accept the look-alike-host and
|
||||
# prefix-injection variants.
|
||||
for bad_mode in url-wrong-host url-wrong-owner url-wrong-repo url-suffix-injection; do
|
||||
# Cases 7-12 (#865 Blocker 3): the created record's id/author/body are all
|
||||
# correct, but its provider-returned issue_url does not belong to this issue on
|
||||
# this provider/repo. Verification pins the URL's ORIGIN (scheme-class + host +
|
||||
# explicit non-default port) and its FULL path (deployment prefix + exact
|
||||
# owner/repo + kind + number), so each must FAIL CLOSED. A bare endswith/suffix
|
||||
# test would wrongly accept the look-alike-host and prefix-injection variants.
|
||||
# url-wrong-port and 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 url-wrong-host url-wrong-owner url-wrong-repo url-suffix-injection \
|
||||
url-wrong-port url-non-web-scheme; do
|
||||
if run_comment "$bad_mode"; then
|
||||
echo "FAIL: forged comment URL ($bad_mode) was accepted" >&2
|
||||
cat "$OUTPUT_FILE" >&2
|
||||
@@ -568,4 +606,19 @@ done
|
||||
# issue_url (already exercised by Case 1's fresh-success), so the tightened check
|
||||
# is not rejecting genuine writes.
|
||||
|
||||
# Case 13 (#991): the deployment's ROOT_URL is http:// while the repo remote is
|
||||
# https://, so the provider returns an http:// issue_url for a record that is
|
||||
# otherwise entirely correct. This is not a forgery — it is the provider's own
|
||||
# truthful answer about a write that landed — and a scheme-strict comparison
|
||||
# rejects it deterministically, on EVERY comment, converting a successful write
|
||||
# into a reported failure. It must be ACCEPTED. Host, path, owner, repo, kind
|
||||
# and number all remain strict; only the http/https distinction is relaxed.
|
||||
run_comment url-scheme-downgrade
|
||||
grep -q 'Added and verified comment on Gitea issue #7 (comment ID 1)' "$OUTPUT_FILE"
|
||||
# The write really happened and was read back by exact id — this case passes
|
||||
# through the same POST/GET chain as case 1, not around it.
|
||||
grep -q "^POST $API_BASE/issues/7/comments$" "$CURL_LOG"
|
||||
grep -q "^GET $API_BASE/issues/comments/1$" "$CURL_LOG"
|
||||
assert_no_temp_leak "url-scheme-downgrade"
|
||||
|
||||
echo "issue-comment.sh REST create + exact-id read-back regression passed"
|
||||
|
||||
Reference in New Issue
Block a user