fix(tools/git): pr-review.sh _belongs case-insensitive repo-slug compare #877

2026-07-23T17:34:57Z

jason.woltje commented

2026-07-23 17:34:57 +00:00

Bug (#875, post-#866 follow-up)

#866 hardened pr-review.sh's comment-persistence read-back into a full-path
_belongs() check that pins scheme+host+port ORIGIN and the FULL path, to
block look-alike-host and same-host decoy-prefix attacks. That hardening left
a case-sensitivity bug:

_origin_and_path(url) lowercases scheme+host but returns
parsed.path.rstrip("/") WITHOUT lowercasing the path.
expected_pr_path embeds the repo slug from EXPECTED_REPO_SLUG
(${GITEA_API_BASE##*/repos/}), which can be MIXED-CASE (e.g.
USC/uconnect).
Gitea canonicalizes the returned pull_request_url slug to LOWERCASE.

So _belongs compared the path CASE-SENSITIVELY, and a successful comment
POST (201) FALSE-NEGATIVED on a mixed-case-slug repo ("did not land on a pull
request"). This fails CLOSED on a real success (safe — no false-success) but
breaks legitimate mixed-case-slug repos.

Fix

Lowercase both path and expected_path inside the _belongs comparison
only. The origin == base_origin tuple check (scheme+host+port pin from
#866) is untouched, _origin_and_path still lowercases scheme+host as
before, and the path match is still a FULL-path comparison (no
endswith/suffix relaxation) — so the look-alike-host and same-host
decoy-prefix protections from #866 are preserved. Repo owner/repo slugs are
case-insensitive and the rest of the comment path (/pulls/<number>) is
numeric, so lowercasing this specific comparison is safe.

Diff is scoped to the one _belongs function; no other pr-review.sh logic
changed.

Tests (red-first)

Added a new regression case in test-pr-review-gitea-comment.sh:
comment-mixed-case-slug — a MIXED-CASE EXPECTED_REPO_SLUG
(USC/uconnect) against a Gitea-canonicalized lowercase
pull_request_url (.../usc/uconnect/pulls/123).

Verified FAILS against pre-fix code (Error: ... claimed PR comment did not land on a pull request (kind=pulls) on this provider/repo).
Verified PASSES after the fix.

The existing #866 security regression cases (look-alike host, same-host
decoy prefix, wrong owner, wrong repo, wrong kind/issue_url) all still
REJECT — confirmed unaffected by this change.

Gates

shellcheck on both changed files: no new findings (pre-existing SC1091
info on source is present identically on origin/main).
pnpm --filter @mosaicstack/mosaic run test:framework-shell: all suites
pass, including pr-review.sh REST review + comment create/read-back regression passed.
eslint/prettier could not be run in this checkout (node_modules/tool
binaries not installed, pnpm install fails with an environment-level
EACCES on the shared pnpm store) — pre-existing environment condition,
unrelated to this diff, and out of scope since only shell files changed
(ESLint here targets src TypeScript only).
Pre-push hook (pnpm typecheck && pnpm lint && pnpm format:check) was not
blocked on push; packages/mosaic/framework/tools/orchestrator/README.md
is already clean on this branch (identical to origin/main, via #872), so
no action was needed there.

Closes #875

## Bug (#875, post-#866 follow-up) #866 hardened `pr-review.sh`'s comment-persistence read-back into a full-path `_belongs()` check that pins scheme+host+port ORIGIN and the FULL path, to block look-alike-host and same-host decoy-prefix attacks. That hardening left a case-sensitivity bug: - `_origin_and_path(url)` lowercases scheme+host but returns `parsed.path.rstrip("/")` WITHOUT lowercasing the path. - `expected_pr_path` embeds the repo slug from `EXPECTED_REPO_SLUG` (`${GITEA_API_BASE##*/repos/}`), which can be MIXED-CASE (e.g. `USC/uconnect`). - Gitea canonicalizes the returned `pull_request_url` slug to LOWERCASE. So `_belongs` compared the path CASE-SENSITIVELY, and a successful comment POST (201) FALSE-NEGATIVED on a mixed-case-slug repo ("did not land on a pull request"). This fails CLOSED on a real success (safe — no false-success) but breaks legitimate mixed-case-slug repos. ## Fix Lowercase both `path` and `expected_path` inside the `_belongs` comparison only. The `origin == base_origin` tuple check (scheme+host+port pin from #866) is untouched, `_origin_and_path` still lowercases scheme+host as before, and the path match is still a FULL-path comparison (no endswith/suffix relaxation) — so the look-alike-host and same-host decoy-prefix protections from #866 are preserved. Repo owner/repo slugs are case-insensitive and the rest of the comment path (`/pulls/<number>`) is numeric, so lowercasing this specific comparison is safe. Diff is scoped to the one `_belongs` function; no other pr-review.sh logic changed. ## Tests (red-first) Added a new regression case in `test-pr-review-gitea-comment.sh`: `comment-mixed-case-slug` — a MIXED-CASE `EXPECTED_REPO_SLUG` (`USC/uconnect`) against a Gitea-canonicalized lowercase `pull_request_url` (`.../usc/uconnect/pulls/123`). - Verified FAILS against pre-fix code (`Error: ... claimed PR comment did not land on a pull request (kind=pulls) on this provider/repo`). - Verified PASSES after the fix. The existing #866 security regression cases (look-alike host, same-host decoy prefix, wrong owner, wrong repo, wrong kind/issue_url) all still REJECT — confirmed unaffected by this change. ## Gates - `shellcheck` on both changed files: no new findings (pre-existing SC1091 info on `source` is present identically on `origin/main`). - `pnpm --filter @mosaicstack/mosaic run test:framework-shell`: all suites pass, including `pr-review.sh REST review + comment create/read-back regression passed`. - `eslint`/`prettier` could not be run in this checkout (`node_modules`/tool binaries not installed, `pnpm install` fails with an environment-level `EACCES` on the shared pnpm store) — pre-existing environment condition, unrelated to this diff, and out of scope since only shell files changed (ESLint here targets `src` TypeScript only). - Pre-push hook (`pnpm typecheck && pnpm lint && pnpm format:check`) was not blocked on push; `packages/mosaic/framework/tools/orchestrator/README.md` is already clean on this branch (identical to `origin/main`, via #872), so no action was needed there. Closes #875

jason.woltje added 1 commit 2026-07-23 17:34:58 +00:00

fix(tools/git): pr-review.sh _belongs case-insensitive repo-slug compare

ci/woodpecker/pr/ci Pipeline was successful

Details

9d8d58ae75

#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

Ghost approved these changes 2026-07-23 17:53:26 +00:00

Ghost left a comment

Gate-16 APPROVE -- stamped by Mos (id-11)

Registered under the distinct Mos bot (id-11): PR author jason.woltje(id2) != approver Mos(id11). Commit-author = mosaic-coder (the (c) durable builder identity, != reviewer) -> Finding-0 does NOT apply (clean). Registers the independent reviewer's APPROVE; MS-LEAD prepared/relayed, Mos stamps+merges.

VERDICT: APPROVE @ 9d8d58ae75 (Closes #875 -- the post-#866 follow-up).

Independent review, all 6 PASS: (1) fix = pr-review.sh _belongs() line 214 path==expected_path -> path.lower()==expected_path.lower() (resolves mixed-case-slug false-negative); (2) SECURITY PRESERVED -- origin(scheme+host+port) pin unchanged, FULL-path equality unchanged, wrong-kind reject unchanged; whole-path lowering safe (namespaces case-insensitive-unique, numeric /pulls/, no case-only decoy constructible; #866 decoy tests still reject); (3) red-first reproduced (revert line -> exact false-negative, restore -> pass); (4) scope 2 files +29/-1; (5) firewall PASS w/ tracked observation (reuses the PRE-EXISTING pervasive USC/uconnect fixture, ~30+ instances since PR#538, verify-sanitized.sh explicitly defers third-party-host refs as a separate tracked follow-up -- not new, not a blocker); (6) state open/mergeable/base main, CI terminal-green wp1982 @head.

6-CHECK re-verified by Mos @9d8d58ae75dee95ac4e7596d8beae733bc563c9a: open/not-merged/mergeable/base main; CI success @exact head; reviewed==CI==head; independent APPROVE author!=reviewer no REQUEST-CHANGES; body 'Closes #875'; queue-guard clear. Firewall follow-up (third-party-host fixture) swept into the existing tracked item, not a #877 blocker.

## Gate-16 APPROVE -- stamped by Mos (id-11) Registered under the distinct Mos bot (id-11): PR author jason.woltje(id2) != approver Mos(id11). Commit-author = mosaic-coder (the (c) durable builder identity, != reviewer) -> Finding-0 does NOT apply (clean). Registers the independent reviewer's APPROVE; MS-LEAD prepared/relayed, Mos stamps+merges. VERDICT: APPROVE @ 9d8d58ae75dee95ac4e7596d8beae733bc563c9a (Closes #875 -- the post-#866 follow-up). Independent review, all 6 PASS: (1) fix = pr-review.sh _belongs() line 214 path==expected_path -> path.lower()==expected_path.lower() (resolves mixed-case-slug false-negative); (2) SECURITY PRESERVED -- origin(scheme+host+port) pin unchanged, FULL-path equality unchanged, wrong-kind reject unchanged; whole-path lowering safe (namespaces case-insensitive-unique, numeric /pulls/<n>, no case-only decoy constructible; #866 decoy tests still reject); (3) red-first reproduced (revert line -> exact false-negative, restore -> pass); (4) scope 2 files +29/-1; (5) firewall PASS w/ tracked observation (reuses the PRE-EXISTING pervasive USC/uconnect fixture, ~30+ instances since PR#538, verify-sanitized.sh explicitly defers third-party-host refs as a separate tracked follow-up -- not new, not a blocker); (6) state open/mergeable/base main, CI terminal-green wp1982 @head. 6-CHECK re-verified by Mos @9d8d58ae75dee95ac4e7596d8beae733bc563c9a: open/not-merged/mergeable/base main; CI success @exact head; reviewed==CI==head; independent APPROVE author!=reviewer no REQUEST-CHANGES; body 'Closes #875'; queue-guard clear. Firewall follow-up (third-party-host fixture) swept into the existing tracked item, not a #877 blocker.

Ghost merged commit 8504216964 into main

2026-07-23 17:53:27 +00:00

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: mosaicstack/stack#877