mosaicstack/stack
1
0
Fork 0
Code Issues 85 Pull Requests Actions Packages Projects Releases 15 Wiki Activity

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

Merged
Mos merged 1 commits from fix/875-belongs-case-insensitive-slug into main 2026-07-23 17:53:27 +00:00
Conversation 1 Commits 1 Files Changed 2 +29 -1
jason.woltje commented 2026-07-23 17:34:57 +00:00
Owner
Copy Link

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
All checks were successful
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
Mos approved these changes 2026-07-23 17:53:26 +00:00
Mos left a comment
First-time contributor
Copy Link

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.
Mos merged commit 8504216964 into main 2026-07-23 17:53:27 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
Mos
Labels
Clear labels
fleet-enhancement
Fleet-wide reliability/speed/cost improvements surfaced by the nightly enhance pass
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
jason.woltje
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: mosaicstack/stack#877
Delete Branch "fix/875-belongs-case-insensitive-slug"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?