Fail closed approval gate authorization #1

Merged

stephen merged 1 commit from fix/fail-closed-approver-permissions into main

2026-07-05 22:40:03 +00:00

stephen commented

2026-07-05 22:32:02 +00:00

Owner

Security fix: fail-open -> fail-closed change to a production deploy gate.

Confirmed finding before fixing:

src/lib/command.js previously said empty approvers meant anyone except the run actor, and implemented if (approvers.length) return approvers.includes(login); if (!allowSelf && login === actor) return false; return true;.
src/main.js accepted comments with only isAllowed(login, approvers, ctx.actor, allowSelf), so a matching login string was enough and repo permission was never verified.

Change:

Empty approvers now fails closed: no comment or relay approval is accepted without an explicit allowlist.
Listed approvers are checked against Forgejo GET /repos/{owner}/{repo}/collaborators/{login}/permission; only write, admin, or owner permissions may approve/deny.
Existing self-approval protection remains an additional check: the run actor is rejected unless explicitly allowlisted and allow-self-approval is enabled.
README/action metadata now document that callers must configure approvers and that listed approvers also need write/admin repo permission.

Tests:

make test covers empty approvers reject, listed non-collaborator/read-only login reject, listed write collaborator accept, admin deny, and run actor self-approval rejection.
make all passed (node --check, shellcheck, offline tests; shfmt/yq not installed so skipped as before).

Caller impact:

In this repo, .forgejo/workflows/test.yml does not call the action, and the README example already sets approvers. I found no in-repo caller relying on the empty-approvers default.
Any external workflow currently using this deploy gate with no approvers must add an explicit allowlist; that is intentional for the fail-closed security change.

Security fix: fail-open -> fail-closed change to a production deploy gate. Confirmed finding before fixing: - `src/lib/command.js` previously said empty approvers meant anyone except the run actor, and implemented `if (approvers.length) return approvers.includes(login); if (!allowSelf && login === actor) return false; return true;`. - `src/main.js` accepted comments with only `isAllowed(login, approvers, ctx.actor, allowSelf)`, so a matching login string was enough and repo permission was never verified. Change: - Empty `approvers` now fails closed: no comment or relay approval is accepted without an explicit allowlist. - Listed approvers are checked against Forgejo `GET /repos/{owner}/{repo}/collaborators/{login}/permission`; only `write`, `admin`, or `owner` permissions may approve/deny. - Existing self-approval protection remains an additional check: the run actor is rejected unless explicitly allowlisted and `allow-self-approval` is enabled. - README/action metadata now document that callers must configure `approvers` and that listed approvers also need write/admin repo permission. Tests: - `make test` covers empty approvers reject, listed non-collaborator/read-only login reject, listed write collaborator accept, admin deny, and run actor self-approval rejection. - `make all` passed (`node --check`, `shellcheck`, offline tests; shfmt/yq not installed so skipped as before). Caller impact: - In this repo, `.forgejo/workflows/test.yml` does not call the action, and the README example already sets `approvers`. I found no in-repo caller relying on the empty-approvers default. - Any external workflow currently using this deploy gate with no `approvers` must add an explicit allowlist; that is intentional for the fail-closed security change.

stephen added 1 commit

2026-07-05 22:32:02 +00:00

Fail closed approval gate authorization

test / unit (pull_request) Successful in 21s

Details

4ce036fbe6

stephen commented

2026-07-05 22:39:05 +00:00

Author

Owner

Adversarial review: APPROVE (merge)

I did not author this. Reviewed the branch fix/fail-closed-approver-permissions vs main, read src/lib/command.js and src/main.js, and ran the suite (14/14 green locally, including the two new cases). The fail-open is closed and the permission check is real. All five verification points hold.

1. Empty approvers now fails closed — confirmed

command.js isAllowed gained if (!approvers.length) return false; as the first list check, so an empty allowlist admits nobody. With fail-on-timeout defaulting true, an unconfigured gate simply times out and fails the step. The integration test empty approvers fail closed (a valid alice:write /approve with no INPUT_APPROVERS) exits non-zero. The old "anyone except the run actor" branch is gone.

Both decision paths now go through isAuthorizedApprover = isAllowed(...) && hasDeployPermission(...). hasDeployPermission calls GET /repos/{repo}/collaborators/{login}/permission and accepts only write/admin/owner (permissionAllowsDeploy), rejecting read/none. Allowlist and permission are ANDed: a listed login without write/admin is rejected (test listed non-collaborator is rejected, mallory:read), and a login with permission but not on the list is rejected by isAllowed before any API call (and with an empty list, rejected outright). login is encodeURIComponent-ed; repo is the run's own trusted repo.

3. Not-the-run-actor guard intact — confirmed

if (!allowSelf && login === actor) return false; is preserved. Test self-approval blocked by default (releaser:admin, listed) exits non-zero.

4. No new bypass — checked

API-error fail-open? No. api.get is _throwing (throws on any non-2xx), and hasDeployPermission's catch returns false. A 404 (non-collaborator), 403 (token can't read collaborators), or 5xx all reject. Every error path is fail-closed.
Relay fast-path previously accepted the app's decision unconditionally (if (d) decision = d); it is now gated by the same isAuthorizedApprover, so a relay-supplied approver must also be a permitted collaborator. This closes a second bypass beyond the reported finding.
Response shape: permissionAllowsDeploy handles both the Forgejo {permission: "..."} object and a bare string, and treats missing/unknown as not-allowed.
No path injection (encoded login, trusted repo).

5. Tests — adequate

command.test.js covers empty-approvers → false (with and without allowSelf) and listed-self gating. run.sh adds empty approvers fail closed and listed non-collaborator is rejected, and updates the existing approve/deny/self cases to seed MOCK_PERMISSIONS. 14/14 pass.

Minor, non-blocking follow-ups (not merge blockers)

The allowlist match approvers.includes(login) is case-sensitive, but Forgejo logins are case-insensitive. A case-mismatched legitimate approver is silently rejected (fail-closed, so safe, but a config footgun) — consider normalizing case on both sides.
The gate now hard-requires the workflow token to have collaborator-permission read scope; without it, every approval fails closed. Worth one README line so operators aren't surprised.
No explicit test for the permission-API-error → fail-closed path or the relay permission-check path; the code is clearly correct on both, but a case each would lock in the behavior.

Verdict: APPROVE for merge. Correct fail-open→fail-closed conversion, real permission enforcement, fail-closed on every error path, self-approval guard intact, relay bypass also closed, tests green. The three notes above are optional hardening.

## Adversarial review: APPROVE (merge) I did not author this. Reviewed the branch `fix/fail-closed-approver-permissions` vs `main`, read `src/lib/command.js` and `src/main.js`, and ran the suite (14/14 green locally, including the two new cases). The fail-open is closed and the permission check is real. All five verification points hold. ### 1. Empty approvers now fails closed — confirmed `command.js` `isAllowed` gained `if (!approvers.length) return false;` as the first list check, so an empty allowlist admits nobody. With `fail-on-timeout` defaulting true, an unconfigured gate simply times out and fails the step. The integration test `empty approvers fail closed` (a valid `alice:write` `/approve` with no `INPUT_APPROVERS`) exits non-zero. The old "anyone except the run actor" branch is gone. ### 2. Real repo permission, not a login-string match — confirmed Both decision paths now go through `isAuthorizedApprover = isAllowed(...) && hasDeployPermission(...)`. `hasDeployPermission` calls `GET /repos/{repo}/collaborators/{login}/permission` and accepts only `write`/`admin`/`owner` (`permissionAllowsDeploy`), rejecting `read`/`none`. Allowlist and permission are ANDed: a listed login without write/admin is rejected (test `listed non-collaborator is rejected`, `mallory:read`), and a login with permission but not on the list is rejected by `isAllowed` before any API call (and with an empty list, rejected outright). `login` is `encodeURIComponent`-ed; `repo` is the run's own trusted repo. ### 3. Not-the-run-actor guard intact — confirmed `if (!allowSelf && login === actor) return false;` is preserved. Test `self-approval blocked by default` (`releaser:admin`, listed) exits non-zero. ### 4. No new bypass — checked - **API-error fail-open?** No. `api.get` is `_throwing` (throws on any non-2xx), and `hasDeployPermission`'s `catch` returns `false`. A 404 (non-collaborator), 403 (token can't read collaborators), or 5xx all reject. Every error path is fail-closed. - **Relay fast-path** previously accepted the app's decision unconditionally (`if (d) decision = d`); it is now gated by the same `isAuthorizedApprover`, so a relay-supplied `approver` must also be a permitted collaborator. This closes a second bypass beyond the reported finding. - **Response shape**: `permissionAllowsDeploy` handles both the Forgejo `{permission: "..."}` object and a bare string, and treats missing/unknown as not-allowed. - No path injection (encoded login, trusted repo). ### 5. Tests — adequate `command.test.js` covers empty-approvers → false (with and without `allowSelf`) and listed-self gating. `run.sh` adds `empty approvers fail closed` and `listed non-collaborator is rejected`, and updates the existing approve/deny/self cases to seed `MOCK_PERMISSIONS`. 14/14 pass. ### Minor, non-blocking follow-ups (not merge blockers) - The allowlist match `approvers.includes(login)` is case-sensitive, but Forgejo logins are case-insensitive. A case-mismatched legitimate approver is silently rejected (fail-closed, so safe, but a config footgun) — consider normalizing case on both sides. - The gate now hard-requires the workflow token to have collaborator-permission read scope; without it, every approval fails closed. Worth one README line so operators aren't surprised. - No explicit test for the permission-API-error → fail-closed path or the relay permission-check path; the code is clearly correct on both, but a case each would lock in the behavior. Verdict: **APPROVE** for merge. Correct fail-open→fail-closed conversion, real permission enforcement, fail-closed on every error path, self-approval guard intact, relay bypass also closed, tests green. The three notes above are optional hardening.

stephen merged commit fe26932cfc into main

2026-07-05 22:40:03 +00:00

stephen referenced this pull request from a commit

2026-07-05 22:40:03 +00:00

Fail closed approval gate authorization (#1)