fix(tools/git): pr-review.sh _belongs case-insensitive repo-slug compare
All checks were successful
ci/woodpecker/pr/ci Pipeline was successful
All checks were successful
ci/woodpecker/pr/ci Pipeline was successful
#866 hardened the comment-persistence read-back into a full-path _belongs() check that pins scheme+host+port origin plus the full path, to block look-alike-host and same-host decoy-prefix attacks. That check compared the path case-sensitively: EXPECTED_REPO_SLUG is taken verbatim from GITEA_API_BASE and can be mixed-case (e.g. "USC/uconnect"), while Gitea canonicalizes the returned pull_request_url's owner/repo segment to lowercase. A successful 201 write on a mixed-case-slug repo therefore false-negatived ("did not land on a pull request") even though the comment genuinely landed — fail-closed on a real success, not a false success, but broken for legitimate mixed-case-slug repos. Lowercase both sides of the path comparison inside _belongs (path and expected_path) while leaving the origin tuple comparison, _origin_and_path's scheme+host lowercasing, and the full (non-suffix) path match untouched — the look-alike-host and decoy-prefix protections from #866 are unchanged; only the repo-slug segment's case sensitivity is relaxed, which is safe because Gitea repo slugs are case-insensitive and the remainder of the path is numeric. Adds a red-first regression case (comment-mixed-case-slug) modeling a mixed-case EXPECTED_REPO_SLUG against a Gitea-canonicalized lowercase pull_request_url, which fails against pre-fix code and passes after. Closes #875
This commit is contained in:
@@ -203,7 +203,15 @@ try:
|
|||||||
if not url:
|
if not url:
|
||||||
return False
|
return False
|
||||||
origin, path = _origin_and_path(url)
|
origin, path = _origin_and_path(url)
|
||||||
return origin == base_origin and path == expected_path
|
# Repo owner/repo slugs are case-insensitive (Gitea canonicalizes the
|
||||||
|
# pull_request_url slug to lowercase on return), while EXPECTED_REPO_SLUG
|
||||||
|
# is taken verbatim from GITEA_API_BASE and may be mixed-case. The
|
||||||
|
# remainder of the path (".../pulls/<number>") is numeric, so lowercasing
|
||||||
|
# the whole path for this comparison only relaxes case, not identity: the
|
||||||
|
# origin tuple (scheme+host+port) above still pins the provider host, and
|
||||||
|
# the path is still compared in FULL (no endswith/suffix match), so the
|
||||||
|
# look-alike-host and same-host decoy-prefix protections are unchanged.
|
||||||
|
return origin == base_origin and path.lower() == expected_path.lower()
|
||||||
|
|
||||||
if comment.get("id") != expected_id:
|
if comment.get("id") != expected_id:
|
||||||
raise ValueError("read-back id does not match the created id")
|
raise ValueError("read-back id does not match the created id")
|
||||||
|
|||||||
@@ -406,6 +406,13 @@ 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-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
|
||||||
|
# pull_request_url's owner/repo segment to LOWERCASE. Model that here by
|
||||||
|
# lowercasing only the slug path, independent of the (possibly mixed-case)
|
||||||
|
# web_base the wrapper was configured with.
|
||||||
|
pr_url = f"{_origin}{_slug.lower()}/pulls/123"
|
||||||
record = {
|
record = {
|
||||||
"id": 456,
|
"id": 456,
|
||||||
"body": body,
|
"body": body,
|
||||||
@@ -868,6 +875,19 @@ for bad_mode in comment-url-wrong-host comment-url-wrong-owner comment-url-wrong
|
|||||||
assert_no_temp_leak "$bad_mode"
|
assert_no_temp_leak "$bad_mode"
|
||||||
done
|
done
|
||||||
|
|
||||||
|
# Case 15b (#875): a MIXED-CASE repo slug (as embedded verbatim in
|
||||||
|
# GITEA_API_BASE, e.g. "USC/uconnect") must still verify when Gitea returns the
|
||||||
|
# comment's pull_request_url with its owner/repo segment canonicalized to
|
||||||
|
# LOWERCASE ("usc/uconnect"). This is a legitimate, unforged provider response —
|
||||||
|
# not a spoof — so `_belongs` must accept it (case-insensitive slug compare)
|
||||||
|
# while still requiring the origin (scheme+host+port) and the rest of the path
|
||||||
|
# to match in full. Pre-#875-fix this fails closed on a real success
|
||||||
|
# (false-negative); post-fix it verifies.
|
||||||
|
run_review comment-mixed-case-slug comment durable-body https://git.mosaicstack.dev \
|
||||||
|
https://git.mosaicstack.dev/USC/uconnect.git USC/uconnect
|
||||||
|
grep -q 'Added and verified comment on Gitea PR #123' "$OUTPUT_FILE"
|
||||||
|
assert_no_temp_leak "comment-mixed-case-slug"
|
||||||
|
|
||||||
# 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
|
||||||
|
|||||||
Reference in New Issue
Block a user