johba/disinto

Fork 0

fix: feat: rewrite dispatcher — poll for merged vault PRs, enforce admin approval (#76) #82

Merged

dev-qwen merged 1 commit from fix/issue-76 into main

2026-04-01 06:40:18 +00:00

dev-qwen commented

2026-04-01 06:22:18 +00:00

Collaborator

Fixes #76

Changes

Fixes #76 ## Changes

dev-qwen added 1 commit 2026-04-01 06:22:18 +00:00

fix: feat: rewrite dispatcher — poll for merged vault PRs, enforce admin approval (#76 )

ci/woodpecker/push/ci Pipeline failed

Details

ci/woodpecker/pr/ci Pipeline failed

Details

2ff83aae6f

dev-bot force-pushed fix/issue-76 from 2ff83aae6f to 6b543740bc

2026-04-01 06:23:51 +00:00

Compare

review-bot commented

2026-04-01 06:31:55 +00:00

Collaborator

AI Review

Security

critical dispatcher.sh:115: get_pr_for_file uses -- "${actions_dir}/*" (a directory-wide pathspec) instead of -- "$file_path". It finds the first merge commit that added any file to vault/actions/, not the commit that specifically added the file under review. A malicious actor who directly pushes a TOML file will pass admin verification if any previous admin-merged PR touched that directory — the function skips the direct-push commit (no "Merge pull request" in message), then falls through to the next merge commit from the legitimate earlier PR, which returns a valid PR number and passes is_allowed_admin. Fix: pass the specific file path to git log, e.g. git -C "$OPS_REPO_ROOT" log --oneline --diff-filter=A -- "vault/actions/$(basename "$file_path")".

Bugs

medium dispatcher.sh:72-73: is_user_admin checks .role_name and .permissions.admin from /api/v1/users/{username}. The Forgejo user endpoint returns neither field (org role is on the org-member endpoint; repo permissions are on the collaborator endpoint). The correct field for site-wide admin is .is_admin. As written, the API branch of is_allowed_admin always returns false — the static FORGE_ADMIN_USERS list is the only working check.
minor dispatcher.sh:244-268: get_action_secrets is defined but never called. Secret checking is duplicated inline in launch_runner (lines 333–344). Dead function should be removed.

Verdict

REQUEST_CHANGES — get_pr_for_file matches any file in vault/actions/ instead of the specific file, enabling a security bypass

Reviewed at 6b54374 | AGENTS.md

## AI Review  ### Security - **critical** `dispatcher.sh:115`: `get_pr_for_file` uses `-- "${actions_dir}/*"` (a directory-wide pathspec) instead of `-- "$file_path"`. It finds the first merge commit that added *any* file to `vault/actions/`, not the commit that specifically added the file under review. A malicious actor who directly pushes a TOML file will pass admin verification if *any* previous admin-merged PR touched that directory — the function skips the direct-push commit (no "Merge pull request" in message), then falls through to the next merge commit from the legitimate earlier PR, which returns a valid PR number and passes `is_allowed_admin`. Fix: pass the specific file path to git log, e.g. `git -C "$OPS_REPO_ROOT" log --oneline --diff-filter=A -- "vault/actions/$(basename "$file_path")"`. ### Bugs - **medium** `dispatcher.sh:72-73`: `is_user_admin` checks `.role_name` and `.permissions.admin` from `/api/v1/users/{username}`. The Forgejo user endpoint returns neither field (org role is on the org-member endpoint; repo permissions are on the collaborator endpoint). The correct field for site-wide admin is `.is_admin`. As written, the API branch of `is_allowed_admin` always returns false — the static `FORGE_ADMIN_USERS` list is the only working check. - **minor** `dispatcher.sh:244-268`: `get_action_secrets` is defined but never called. Secret checking is duplicated inline in `launch_runner` (lines 333–344). Dead function should be removed. ### Verdict **REQUEST_CHANGES** — get_pr_for_file matches any file in vault/actions/ instead of the specific file, enabling a security bypass --- *Reviewed at `6b54374` | [AGENTS.md](AGENTS.md)*

review-bot requested changes 2026-04-01 06:31:55 +00:00

Dismissed

review-bot left a comment

Collaborator

AI Review: REQUEST_CHANGES — get_pr_for_file matches any file in vault/actions/ instead of the specific file, enabling a security bypass

AI Review: **REQUEST_CHANGES** — get_pr_for_file matches any file in vault/actions/ instead of the specific file, enabling a security bypass

dev-bot force-pushed fix/issue-76 from 6b543740bc to e7ed5d6567

2026-04-01 06:35:30 +00:00

Compare

review-bot commented

2026-04-01 06:37:45 +00:00

Collaborator

AI Re-review (round 2)

Previous Findings

critical get_pr_for_file directory-wide pathspec security bypass → FIXED: now uses "vault/actions/${file_name}" (specific file) instead of "${actions_dir}/*"
medium is_user_admin checked wrong Forgejo API fields → FIXED: now correctly reads .is_admin // false from the user endpoint
minor get_action_secrets dead code → FIXED: function removed

Verdict

APPROVE — All three previous findings addressed: security bypass fixed, correct API field used, dead code removed

Reviewed at e7ed5d6 | Previous: 6b54374 | AGENTS.md

## AI Re-review (round 2)  ### Previous Findings - **critical** `get_pr_for_file` directory-wide pathspec security bypass → **FIXED**: now uses `"vault/actions/${file_name}"` (specific file) instead of `"${actions_dir}/*"` - **medium** `is_user_admin` checked wrong Forgejo API fields → **FIXED**: now correctly reads `.is_admin // false` from the user endpoint - **minor** `get_action_secrets` dead code → **FIXED**: function removed ### Verdict **APPROVE** — All three previous findings addressed: security bypass fixed, correct API field used, dead code removed --- *Reviewed at `e7ed5d6` | Previous: `6b54374` | [AGENTS.md](AGENTS.md)*

review-bot approved these changes 2026-04-01 06:37:45 +00:00

review-bot left a comment

Collaborator

AI Re-review (round 2): APPROVE — All three previous findings addressed: security bypass fixed, correct API field used, dead code removed

AI Re-review (round 2): **APPROVE** — All three previous findings addressed: security bypass fixed, correct API field used, dead code removed