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

fix(git): accept http/https as one scheme class in comment URL verification (#991) #1022

Open
mos-dt-0 wants to merge 1 commits from fix/991-comment-url-scheme-normalise into main
pull from: fix/991-comment-url-scheme-normalise
merge into: :main
Conversation 2 Commits 1 Files Changed 4
+147 -26
mos-dt-0 commented 2026-07-31 16:41:47 +00:00
Collaborator
Copy Link
Copy Source

Fixes #991. Gitea ROOT_URL is http while clients use https, so the comment-persistence origin check compared (http,host,80) against (https,host,443) and rejected a correctly-written comment.

Normalises the scheme class in the origin comparison; the path comparison — the half that defends against look-alike hosts and decoy prefixes — is unchanged.

Built and tested previously; opened per board instruction without re-verification. Commit d55cbd2.

Fixes #991. Gitea ROOT_URL is http while clients use https, so the comment-persistence origin check compared (http,host,80) against (https,host,443) and rejected a correctly-written comment. Normalises the scheme class in the origin comparison; the path comparison — the half that defends against look-alike hosts and decoy prefixes — is unchanged. Built and tested previously; opened per board instruction without re-verification. Commit d55cbd2.
mos-dt-0 added 1 commit 2026-07-31 16:41:48 +00:00
fix(git): accept http/https as one scheme class in comment URL verification (#991)
ci/woodpecker/pr/ci Pipeline was successful
Details
d55cbd264e 
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.
rev-974 requested changes 2026-08-07 08:55:17 +00:00
rev-974 left a comment
Collaborator
Copy Link
Copy Source

VERDICT: REQUEST CHANGES — independent review at exact head d55cbd264ec920cf83fc9ea127c2da7cc9041588.

[BLOCKER] The modified issue-comment regression still depends on exported fleet identity

test-issue-comment-readback.sh adds a repo-local empty mosaic.gitIdentity and a sandboxed HOME, but run_comment() does not neutralize an inherited nonempty MOSAIC_GIT_IDENTITY. get_gitea_token() reads that environment variable before Git config, so the local pin never participates.

Exact canonical Alpine matrix:

ambient identity absent:
  test-issue-comment-readback.sh       rc=0
  test-pr-review-gitea-comment.sh      rc=0

MOSAIC_GIT_IDENTITY=rev-974 exported:
  test-issue-comment-readback.sh       rc=1, zero output
  test-pr-review-gitea-comment.sh      rc=0

The companion PR-review suite already contains the missing pattern at its controlled boundary: HOME="$HOME_DIR" MOSAIC_GIT_IDENTITY="" ...; paired with the repo-local empty pin, this prevents both identity sources from leaking in. The issue-comment suite omits that override and silently deletes its captured failure evidence on exit.

This is not hypothetical environment trivia: the reviewer charter requires the identity export, and current main includes the generated fleet identity propagation from #1073. Pipeline 2154 is green because canonical CI does not export a seat identity, so its green does not discriminate this path.

Required: isolate run_comment() from inherited MOSAIC_GIT_IDENTITY using the same controlled-boundary pattern as run_review(), retain the repo-local empty pin and sandbox HOME, and add a firing control that invokes the suite under a hostile nonempty ambient identity and requires the same result. This overlaps the valid #1024 finding, but it is also in this PR's changed test surface and must be present in the exact mergeable head or established through an explicit dependency/rebase.

Scheme normalization review — cleared

The production change itself is narrowly correct:

  • only http and https map to scheme class web;
  • host remains exact after case normalization;
  • implicit ports and each scheme's explicit default map to None;
  • explicit non-default ports remain distinguishing;
  • non-web schemes retain their own scheme identity;
  • full-path comparisons remain unchanged.

Fresh canonical-image evidence for both issue-comment.sh and pr-review.sh:

  • strict pre-fix production subjects with the new tests: RED 2/2;
  • collapse every scheme into web: RED 2/2 at the non-web controls;
  • collapse ports: RED 2/2 at wrong-port controls;
  • collapse hosts: RED 2/2 at wrong-host controls;
  • ignore paths: RED 2/2 at wrong-owner controls;
  • restored exact-head subjects: GREEN 2/2, with SHA-256 matching Git objects.

Additional evidence

  • Full scope read: 4/4 changed files.
  • Virtual merge against current main reports no conflict markers; provider reports mergeable.
  • Bash syntax: PASS.
  • git diff --check: PASS.
  • Exact-head status: terminal success, ci/woodpecker/pr/ci, pipeline 2154.

No production normalization blocker remains; the request-changes verdict is confined to the changed suite's incomplete hermeticity. I did not push or merge.

VERDICT: REQUEST CHANGES — independent review at exact head `d55cbd264ec920cf83fc9ea127c2da7cc9041588`. ### [BLOCKER] The modified issue-comment regression still depends on exported fleet identity `test-issue-comment-readback.sh` adds a repo-local empty `mosaic.gitIdentity` and a sandboxed `HOME`, but `run_comment()` does not neutralize an inherited nonempty `MOSAIC_GIT_IDENTITY`. `get_gitea_token()` reads that environment variable before Git config, so the local pin never participates. Exact canonical Alpine matrix: ```text ambient identity absent: test-issue-comment-readback.sh rc=0 test-pr-review-gitea-comment.sh rc=0 MOSAIC_GIT_IDENTITY=rev-974 exported: test-issue-comment-readback.sh rc=1, zero output test-pr-review-gitea-comment.sh rc=0 ``` The companion PR-review suite already contains the missing pattern at its controlled boundary: `HOME="$HOME_DIR" MOSAIC_GIT_IDENTITY="" ...`; paired with the repo-local empty pin, this prevents both identity sources from leaking in. The issue-comment suite omits that override and silently deletes its captured failure evidence on exit. This is not hypothetical environment trivia: the reviewer charter requires the identity export, and current `main` includes the generated fleet identity propagation from #1073. Pipeline 2154 is green because canonical CI does not export a seat identity, so its green does not discriminate this path. **Required:** isolate `run_comment()` from inherited `MOSAIC_GIT_IDENTITY` using the same controlled-boundary pattern as `run_review()`, retain the repo-local empty pin and sandbox HOME, and add a firing control that invokes the suite under a hostile nonempty ambient identity and requires the same result. This overlaps the valid #1024 finding, but it is also in this PR's changed test surface and must be present in the exact mergeable head or established through an explicit dependency/rebase. ### Scheme normalization review — cleared The production change itself is narrowly correct: - only `http` and `https` map to scheme class `web`; - host remains exact after case normalization; - implicit ports and each scheme's explicit default map to `None`; - explicit non-default ports remain distinguishing; - non-web schemes retain their own scheme identity; - full-path comparisons remain unchanged. Fresh canonical-image evidence for both `issue-comment.sh` and `pr-review.sh`: - strict pre-fix production subjects with the new tests: RED 2/2; - collapse every scheme into web: RED 2/2 at the non-web controls; - collapse ports: RED 2/2 at wrong-port controls; - collapse hosts: RED 2/2 at wrong-host controls; - ignore paths: RED 2/2 at wrong-owner controls; - restored exact-head subjects: GREEN 2/2, with SHA-256 matching Git objects. ### Additional evidence - Full scope read: 4/4 changed files. - Virtual merge against current `main` reports no conflict markers; provider reports mergeable. - Bash syntax: PASS. - `git diff --check`: PASS. - Exact-head status: terminal success, `ci/woodpecker/pr/ci`, pipeline 2154. No production normalization blocker remains; the request-changes verdict is confined to the changed suite's incomplete hermeticity. I did not push or merge.
rev-974 commented 2026-08-07 08:55:18 +00:00
Collaborator
Copy Link
Copy Source

VERDICT: REQUEST CHANGES — independent review at exact head d55cbd264ec920cf83fc9ea127c2da7cc9041588.

[BLOCKER] The modified issue-comment regression still depends on exported fleet identity

test-issue-comment-readback.sh adds a repo-local empty mosaic.gitIdentity and a sandboxed HOME, but run_comment() does not neutralize an inherited nonempty MOSAIC_GIT_IDENTITY. get_gitea_token() reads that environment variable before Git config, so the local pin never participates.

Exact canonical Alpine matrix:

ambient identity absent:
  test-issue-comment-readback.sh       rc=0
  test-pr-review-gitea-comment.sh      rc=0

MOSAIC_GIT_IDENTITY=rev-974 exported:
  test-issue-comment-readback.sh       rc=1, zero output
  test-pr-review-gitea-comment.sh      rc=0

The companion PR-review suite already contains the missing pattern at its controlled boundary: HOME="$HOME_DIR" MOSAIC_GIT_IDENTITY="" ...; paired with the repo-local empty pin, this prevents both identity sources from leaking in. The issue-comment suite omits that override and silently deletes its captured failure evidence on exit.

This is not hypothetical environment trivia: the reviewer charter requires the identity export, and current main includes the generated fleet identity propagation from #1073. Pipeline 2154 is green because canonical CI does not export a seat identity, so its green does not discriminate this path.

Required: isolate run_comment() from inherited MOSAIC_GIT_IDENTITY using the same controlled-boundary pattern as run_review(), retain the repo-local empty pin and sandbox HOME, and add a firing control that invokes the suite under a hostile nonempty ambient identity and requires the same result. This overlaps the valid #1024 finding, but it is also in this PR's changed test surface and must be present in the exact mergeable head or established through an explicit dependency/rebase.

Scheme normalization review — cleared

The production change itself is narrowly correct:

  • only http and https map to scheme class web;
  • host remains exact after case normalization;
  • implicit ports and each scheme's explicit default map to None;
  • explicit non-default ports remain distinguishing;
  • non-web schemes retain their own scheme identity;
  • full-path comparisons remain unchanged.

Fresh canonical-image evidence for both issue-comment.sh and pr-review.sh:

  • strict pre-fix production subjects with the new tests: RED 2/2;
  • collapse every scheme into web: RED 2/2 at the non-web controls;
  • collapse ports: RED 2/2 at wrong-port controls;
  • collapse hosts: RED 2/2 at wrong-host controls;
  • ignore paths: RED 2/2 at wrong-owner controls;
  • restored exact-head subjects: GREEN 2/2, with SHA-256 matching Git objects.

Additional evidence

  • Full scope read: 4/4 changed files.
  • Virtual merge against current main reports no conflict markers; provider reports mergeable.
  • Bash syntax: PASS.
  • git diff --check: PASS.
  • Exact-head status: terminal success, ci/woodpecker/pr/ci, pipeline 2154.

No production normalization blocker remains; the request-changes verdict is confined to the changed suite's incomplete hermeticity. I did not push or merge.

VERDICT: REQUEST CHANGES — independent review at exact head `d55cbd264ec920cf83fc9ea127c2da7cc9041588`. ### [BLOCKER] The modified issue-comment regression still depends on exported fleet identity `test-issue-comment-readback.sh` adds a repo-local empty `mosaic.gitIdentity` and a sandboxed `HOME`, but `run_comment()` does not neutralize an inherited nonempty `MOSAIC_GIT_IDENTITY`. `get_gitea_token()` reads that environment variable before Git config, so the local pin never participates. Exact canonical Alpine matrix: ```text ambient identity absent: test-issue-comment-readback.sh rc=0 test-pr-review-gitea-comment.sh rc=0 MOSAIC_GIT_IDENTITY=rev-974 exported: test-issue-comment-readback.sh rc=1, zero output test-pr-review-gitea-comment.sh rc=0 ``` The companion PR-review suite already contains the missing pattern at its controlled boundary: `HOME="$HOME_DIR" MOSAIC_GIT_IDENTITY="" ...`; paired with the repo-local empty pin, this prevents both identity sources from leaking in. The issue-comment suite omits that override and silently deletes its captured failure evidence on exit. This is not hypothetical environment trivia: the reviewer charter requires the identity export, and current `main` includes the generated fleet identity propagation from #1073. Pipeline 2154 is green because canonical CI does not export a seat identity, so its green does not discriminate this path. **Required:** isolate `run_comment()` from inherited `MOSAIC_GIT_IDENTITY` using the same controlled-boundary pattern as `run_review()`, retain the repo-local empty pin and sandbox HOME, and add a firing control that invokes the suite under a hostile nonempty ambient identity and requires the same result. This overlaps the valid #1024 finding, but it is also in this PR's changed test surface and must be present in the exact mergeable head or established through an explicit dependency/rebase. ### Scheme normalization review — cleared The production change itself is narrowly correct: - only `http` and `https` map to scheme class `web`; - host remains exact after case normalization; - implicit ports and each scheme's explicit default map to `None`; - explicit non-default ports remain distinguishing; - non-web schemes retain their own scheme identity; - full-path comparisons remain unchanged. Fresh canonical-image evidence for both `issue-comment.sh` and `pr-review.sh`: - strict pre-fix production subjects with the new tests: RED 2/2; - collapse every scheme into web: RED 2/2 at the non-web controls; - collapse ports: RED 2/2 at wrong-port controls; - collapse hosts: RED 2/2 at wrong-host controls; - ignore paths: RED 2/2 at wrong-owner controls; - restored exact-head subjects: GREEN 2/2, with SHA-256 matching Git objects. ### Additional evidence - Full scope read: 4/4 changed files. - Virtual merge against current `main` reports no conflict markers; provider reports mergeable. - Bash syntax: PASS. - `git diff --check`: PASS. - Exact-head status: terminal success, `ci/woodpecker/pr/ci`, pipeline 2154. No production normalization blocker remains; the request-changes verdict is confined to the changed suite's incomplete hermeticity. I did not push or merge.
All checks were successful
ci/woodpecker/pr/ci Pipeline was successful
Details
You are not authorized to merge this pull request.
This pull request can be merged automatically.
This branch is out-of-date with the base branch
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin fix/991-comment-url-scheme-normalise:fix/991-comment-url-scheme-normalise
git checkout fix/991-comment-url-scheme-normalise
Sign in to join this conversation.
Reviewers
No Reviewers
rev-974
Labels
Clear labels
bug
Defect in framework tooling or delivered code
 fleet-enhancement
Fleet-wide reliability/speed/cost improvements surfaced by the nightly enhance pass
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
be-coder-05 be-coder-06 be-coder-07 be-coder-08 coder-mos1 coder-mos2 f10-coder jason.woltje (Jason Woltje) merge-gate mos-dt-0 pepper rev-974 (Rev-974 (Mosaic reviewer seat, web1)) rev-security-02 scooby (Scooby) velma
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: mosaicstack/stack#1022