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

fix(mosaic): harden Gitea pr merge fallback #521

Closed
jason.woltje wants to merge 0 commits from fix/t_301e4e3b-pr-merge-gitea-empty-uid into main
pull from: fix/t_301e4e3b-pr-merge-gitea-empty-uid
merge into: mosaicstack:main
Conversation 2 Commits - Files Changed -
jason.woltje commented 2026-05-22 21:04:40 +00:00
Owner
Copy Link

Problem: pr-merge.sh preferred tea pr merge whenever a host tea login existed. On Gitea, tea identity preflight can succeed while non-interactive tea merge fails with the known empty identity error, permanently blocking a valid squash merge.

Change:

  • Preserve policy gates: base main, squash-only, queue guard before real merges.
  • Preserve existing authenticated Gitea API merge fallback for no tea login.
  • When tea login exists, fall back to authenticated Gitea API merge only when tea output matches the known empty uid/name user failure class.
  • Unknown tea failures remain blocking and print the tea error; they do not silently API-merge.

Exact fallback trigger:

  • user does not exist [uid: 0, name: ]
  • equivalent empty uid/name wording from Gitea/tea for the same known failure class

Tests/evidence:

  • bash -n packages/mosaic/framework/tools/git/pr-merge.sh
  • shellcheck -x packages/mosaic/framework/tools/git/pr-merge.sh packages/mosaic/framework/tools/git/test-pr-merge-gitea-empty-uid.sh
  • bash packages/mosaic/framework/tools/git/test-pr-merge-gitea-empty-uid.sh
  • pnpm typecheck
  • pnpm lint
  • pnpm format:check
  • pnpm --filter @mosaicstack/mosaic test
  • Full pnpm test was attempted and hit out-of-scope gateway DB setup failure in apps/gateway/src/tests/cross-user-isolation.test.ts: Postgres relation messages does not exist. No gateway/DB files were modified.
  • /home/hermes/.config/mosaic/tools/git/ci-queue-wait.sh --purpose push completed before push.

Tracking:

Problem: pr-merge.sh preferred tea pr merge whenever a host tea login existed. On Gitea, tea identity preflight can succeed while non-interactive tea merge fails with the known empty identity error, permanently blocking a valid squash merge. Change: - Preserve policy gates: base main, squash-only, queue guard before real merges. - Preserve existing authenticated Gitea API merge fallback for no tea login. - When tea login exists, fall back to authenticated Gitea API merge only when tea output matches the known empty uid/name user failure class. - Unknown tea failures remain blocking and print the tea error; they do not silently API-merge. Exact fallback trigger: - user does not exist [uid: 0, name: ] - equivalent empty uid/name wording from Gitea/tea for the same known failure class Tests/evidence: - bash -n packages/mosaic/framework/tools/git/pr-merge.sh - shellcheck -x packages/mosaic/framework/tools/git/pr-merge.sh packages/mosaic/framework/tools/git/test-pr-merge-gitea-empty-uid.sh - bash packages/mosaic/framework/tools/git/test-pr-merge-gitea-empty-uid.sh - pnpm typecheck - pnpm lint - pnpm format:check - pnpm --filter @mosaicstack/mosaic test - Full pnpm test was attempted and hit out-of-scope gateway DB setup failure in apps/gateway/src/__tests__/cross-user-isolation.test.ts: Postgres relation messages does not exist. No gateway/DB files were modified. - /home/hermes/.config/mosaic/tools/git/ci-queue-wait.sh --purpose push completed before push. Tracking: - Kanban implementation task: t_301e4e3b - Parent Kanban task: t_245ad9d8 - Issue: #520
jason.woltje added 1 commit 2026-05-22 21:04:41 +00:00
fix(mosaic): harden Gitea pr merge fallback (#520)
Some checks failed
ci/woodpecker/push/ci Pipeline failed
Details
ci/woodpecker/pr/ci Pipeline failed
Details
893dd19efb
jason.woltje commented 2026-05-22 21:11:57 +00:00
Author
Owner
Copy Link

DO-178B Review: PR #521 — fix(mosaic): harden Gitea pr merge fallback

Software Level: C (Mosaic delivery wrapper/business-critical tooling)
Reviewer: ultron
Base: 755df9079e
Head reviewed: 893dd19efb
Scope: packages/mosaic/framework/tools/git/pr-merge.sh, packages/mosaic/framework/tools/git/test-pr-merge-gitea-empty-uid.sh, docs/scratchpads/t_301e4e3b-pr-merge-gitea-empty-uid.md

Objective summary:
1 Requirements Traceability: PASS — PR body links t_245ad9d8/t_301e4e3b and issue #520.
2 Derived Requirements: PASS — API fallback safety behavior documented.
3 Configuration Management: PASS — branch targets main; head pinned above; scoped files only.
4 Functional Correctness: PASS for Gitea fallback — known empty uid path falls back; arbitrary tea failure blocks.
5 Structural Coverage: PASS for focused shell harness; broader coverage not applicable to shell wrapper.
6 Security Analysis: FAIL — eval injection remains in GitHub path of touched wrapper.
7 Data Integrity: PASS — merge action remains guarded by squash/main/queue checks before execution.
8 Robustness: PASS — fallback handles no-login and known tea identity failure; unknown failures fail closed.
9 Code Quality: FAIL — eval-based command construction remains in pr-merge.sh.
10 Framework Compliance: PASS — Mosaic squash/main/queue policies preserved for Gitea path.
11 Regression Protection: PASS for focused checks; full pnpm test failure documented as out-of-scope gateway DB setup.
12 Hands-Off Verification: PASS — diff is limited to declared wrapper/test/scratchpad scope.

Findings:

🔴 [BLOCKER] packages/mosaic/framework/tools/git/pr-merge.sh:193-195 — GitHub merge path still builds a command string with unvalidated PR_NUMBER and executes it via eval.

The review scope explicitly required verifying that shell quoting avoids eval-style injection risks. The Gitea path was fixed to quote arguments, but the same touched wrapper still has:

CMD="gh pr merge $PR_NUMBER --squash"
[[ "$DELETE_BRANCH" == true ]] && CMD="$CMD --delete-branch"
eval "$CMD"

A mocked GitHub-platform probe with -n '123; touch <sentinel> #' executed the injected command before reporting success. This is a command injection risk in a privileged delivery wrapper and fails the required shell-quoting check.

Remediation:

  • Validate PR_NUMBER before any metadata lookup/merge, e.g. [[ "$PR_NUMBER" =~ ^[0-9]+$ ]] || exit 1.
  • Replace the GitHub eval path with an argv array, e.g. cmd=(gh pr merge "$PR_NUMBER" --squash); [[ "$DELETE_BRANCH" == true ]] && cmd+=(--delete-branch); "${cmd[@]}".
  • Add a focused regression proving metacharacters in -n/--number are rejected and do not execute on both GitHub and Gitea paths.

Verification evidence run by reviewer:

  • bash -n packages/mosaic/framework/tools/git/pr-merge.sh
  • bash -n packages/mosaic/framework/tools/git/test-pr-merge-gitea-empty-uid.sh
  • shellcheck -x packages/mosaic/framework/tools/git/pr-merge.sh packages/mosaic/framework/tools/git/test-pr-merge-gitea-empty-uid.sh — PASS
  • bash packages/mosaic/framework/tools/git/test-pr-merge-gitea-empty-uid.sh — PASS
  • Focused guard probe: non-main base exits before tea/API merge; non-squash method exits before external calls — PASS
  • Focused injection probe: GitHub eval path executed injected sentinel command — FAIL
  • Codex automated review wrapper attempted but unavailable due 401 Unauthorized (/home/hermes/.config/mosaic/tools/codex/codex-code-review.sh -b origin/main -o ...). Independent subagent review completed and agreed the Gitea-specific behavior is acceptable but GitHub eval is a material residual issue.

Verdict: REQUEST CHANGES. Do not merge until the eval injection path is removed or PR_NUMBER is strictly validated before command construction. Tuesday should not proceed to CI/queue-guard/merge gate yet.

## DO-178B Review: PR #521 — fix(mosaic): harden Gitea pr merge fallback Software Level: C (Mosaic delivery wrapper/business-critical tooling) Reviewer: ultron Base: 755df9079e4d1d697856c89d4952407dfa1b554a Head reviewed: 893dd19efbe6a6434f3ae9e040ef9d4c25034fc6 Scope: packages/mosaic/framework/tools/git/pr-merge.sh, packages/mosaic/framework/tools/git/test-pr-merge-gitea-empty-uid.sh, docs/scratchpads/t_301e4e3b-pr-merge-gitea-empty-uid.md Objective summary: 1 Requirements Traceability: PASS — PR body links t_245ad9d8/t_301e4e3b and issue #520. 2 Derived Requirements: PASS — API fallback safety behavior documented. 3 Configuration Management: PASS — branch targets main; head pinned above; scoped files only. 4 Functional Correctness: PASS for Gitea fallback — known empty uid path falls back; arbitrary tea failure blocks. 5 Structural Coverage: PASS for focused shell harness; broader coverage not applicable to shell wrapper. 6 Security Analysis: FAIL — eval injection remains in GitHub path of touched wrapper. 7 Data Integrity: PASS — merge action remains guarded by squash/main/queue checks before execution. 8 Robustness: PASS — fallback handles no-login and known tea identity failure; unknown failures fail closed. 9 Code Quality: FAIL — eval-based command construction remains in pr-merge.sh. 10 Framework Compliance: PASS — Mosaic squash/main/queue policies preserved for Gitea path. 11 Regression Protection: PASS for focused checks; full pnpm test failure documented as out-of-scope gateway DB setup. 12 Hands-Off Verification: PASS — diff is limited to declared wrapper/test/scratchpad scope. Findings: 🔴 [BLOCKER] packages/mosaic/framework/tools/git/pr-merge.sh:193-195 — GitHub merge path still builds a command string with unvalidated PR_NUMBER and executes it via eval. The review scope explicitly required verifying that shell quoting avoids eval-style injection risks. The Gitea path was fixed to quote arguments, but the same touched wrapper still has: CMD="gh pr merge $PR_NUMBER --squash" [[ "$DELETE_BRANCH" == true ]] && CMD="$CMD --delete-branch" eval "$CMD" A mocked GitHub-platform probe with `-n '123; touch <sentinel> #'` executed the injected command before reporting success. This is a command injection risk in a privileged delivery wrapper and fails the required shell-quoting check. Remediation: - Validate `PR_NUMBER` before any metadata lookup/merge, e.g. `[[ "$PR_NUMBER" =~ ^[0-9]+$ ]] || exit 1`. - Replace the GitHub eval path with an argv array, e.g. `cmd=(gh pr merge "$PR_NUMBER" --squash); [[ "$DELETE_BRANCH" == true ]] && cmd+=(--delete-branch); "${cmd[@]}"`. - Add a focused regression proving metacharacters in `-n/--number` are rejected and do not execute on both GitHub and Gitea paths. Verification evidence run by reviewer: - `bash -n packages/mosaic/framework/tools/git/pr-merge.sh` - `bash -n packages/mosaic/framework/tools/git/test-pr-merge-gitea-empty-uid.sh` - `shellcheck -x packages/mosaic/framework/tools/git/pr-merge.sh packages/mosaic/framework/tools/git/test-pr-merge-gitea-empty-uid.sh` — PASS - `bash packages/mosaic/framework/tools/git/test-pr-merge-gitea-empty-uid.sh` — PASS - Focused guard probe: non-main base exits before tea/API merge; non-squash method exits before external calls — PASS - Focused injection probe: GitHub eval path executed injected sentinel command — FAIL - Codex automated review wrapper attempted but unavailable due 401 Unauthorized (`/home/hermes/.config/mosaic/tools/codex/codex-code-review.sh -b origin/main -o ...`). Independent subagent review completed and agreed the Gitea-specific behavior is acceptable but GitHub eval is a material residual issue. Verdict: REQUEST CHANGES. Do not merge until the eval injection path is removed or PR_NUMBER is strictly validated before command construction. Tuesday should not proceed to CI/queue-guard/merge gate yet.
jason.woltje added 1 commit 2026-05-22 21:24:48 +00:00
fix(mosaic): reject unsafe pr merge numbers (#520)
Some checks failed
ci/woodpecker/push/ci Pipeline failed
Details
ci/woodpecker/pr/ci Pipeline failed
Details
19fc6d549e
jason.woltje force-pushed fix/t_301e4e3b-pr-merge-gitea-empty-uid from 19fc6d549e to 59b611ba8a 2026-06-11 18:23:34 +00:00 Compare
jason.woltje commented 2026-06-11 18:24:31 +00:00
Author
Owner
Copy Link

Closing as superseded by #524 (rollup 821e19d, merged 2026-05-26). Verified during orchestration rebase: both commits (893dd19 harden Gitea fallback, 19fc6d5 reject unsafe PR numbers) are already on main, which is a strict superset (adds basic_auth fallback, HTTP-code tracking, --dry-run). The api_url-unbound bug is FIXED on main (pr-merge.sh declares+assigns api_url before use). NOTE: the bug still reproduces from the installed framework copy on hosts (v0.0.30, 2026-05-06) which predates #524 — the real remaining action is upgrading the deployed framework, not merging this PR. — orchestrator (mos)

Closing as **superseded by #524** (rollup 821e19d, merged 2026-05-26). Verified during orchestration rebase: both commits (893dd19 harden Gitea fallback, 19fc6d5 reject unsafe PR numbers) are already on main, which is a strict superset (adds basic_auth fallback, HTTP-code tracking, --dry-run). The api_url-unbound bug is FIXED on main (pr-merge.sh declares+assigns api_url before use). NOTE: the bug still reproduces from the *installed* framework copy on hosts (v0.0.30, 2026-05-06) which predates #524 — the real remaining action is upgrading the deployed framework, not merging this PR. — orchestrator (mos)
jason.woltje closed this pull request 2026-06-11 18:24:31 +00:00
Some checks failed
ci/woodpecker/push/ci Pipeline was canceled
Details
ci/woodpecker/push/publish Pipeline was successful
Details
ci/woodpecker/pr/ci Pipeline was successful
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
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
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: mosaicstack/stack#521
Delete Branch "fix/t_301e4e3b-pr-merge-gitea-empty-uid"

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?