Remove the repo allowlist from the Forgejo admin broker config #64

New issue

Closed

opened 2026-10-25 16:37:24 +01:00 by omp-deck · 0 comments

omp-deck commented

2026-10-25 16:37:24 +01:00

Collaborator

Implementation: #65

Problem

The Forgejo admin broker (apps/broker, added in #55 / PR #56) only answers POST /permission for repositories listed in its config's repos allowlist. Any other repository gets 403 repository not allowed, which fails the trigger. The allowlist does nothing useful:

The agent's Forgejo account must already be a collaborator on a repository before it can act there.
Triggers only arrive from repositories with a configured webhook.

It only adds another thing to configure for every repository. It was never a deliberate requirement. It came from an early design idea about future mutating admin actions, and the broker only does a read-only permission lookup.

Remove it. After this change the broker config is {forgejoUrl, adminToken, webhookSecret}, and /permission answers for any owner/name. Existing config files may still contain repos, and so may the unfilled templates the entrypoint wrote earlier. The broker ignores that key silently: no log line, no error. The operator can delete it whenever they like.

Plan

1. Broker config: drop `repos`

In apps/broker/src/config.ts:

Remove the repos field and its doc comment from BrokerConfig.
In parseConfig, remove the repos validation and remove repos from the returned object. Unknown keys, including repos, are not read at all. That is how a leftover repos gets ignored.
Change isTemplate to o.forgejoUrl === "" && o.adminToken === "". Drop the repos condition so that both old templates (with "repos": []) and new ones (without it) count as unconfigured.
In the loadConfig doc comment, change "(empty forgejoUrl and adminToken, no repos)" to "(empty forgejoUrl and adminToken)".

2. Broker `/permission`: answer for every repository

In apps/broker/src/server.ts createHandler:

Delete const repos = new Set(cfg?.repos);.
Delete the if (!repos.has(repo.toLowerCase())) { … 403 … } block.
The rest stays as it is: the 400 shape check, 503 broker not configured when cfg === null, then fetchPermissionImpl, which returns 502 on failure or 200 {level}.

3. Tests

apps/broker/src/config.test.ts:
- Remove repos from VALID and from the expected object in "normalises a valid config".
- Remove the [{ repos: ["nope"] }, "repos"] row.
- In the template test, keep the old template with repos: [] (it must still return null), and also assert that { forgejoUrl: "", adminToken: "", webhookSecret: "a".repeat(64) } without repos returns null.
- Add a case: { ...VALID, repos: ["nope"] } loads, and the result equals the config without repos.
apps/broker/src/server.test.ts:
- Remove repos from CFG.
- Delete the test "a repo off the allowlist is 403 and never reaches Forgejo".
- Rename "returns the level for an allowlisted repo, case-insensitively" to "returns the level for any repo", using { repo: "other/x", user: "alice" }. Expect 200 with the stubbed level, and expect the stub to have been called with owner other and name x. Assert the call in the same shape the existing test uses for s.calls.
apps/broker/src/forgejo.test.ts: remove repos: [] from cfg.

4. Container entrypoint and deck client

docker/entrypoint.sh:
- Remove the "repos": [] line from the template heredoc, and the comma that ends the webhookSecret line before it, so the JSON stays valid.
- In the message that follows, change "fill in forgejoUrl, adminToken and repos as root" to "fill in forgejoUrl and adminToken as root".
apps/server/src/forgejo/broker-client.ts checkPermission: change the doc comment to /** \user`'s permission on `repo` (`owner/name`). */`.

5. Docs

docs/deployment.md broker section:
- Remove "repos": ["owner/name"] from the format example, and the comma after webhookSecret.
- Delete the bullet "repos is the allowlist the broker answers permission checks for."
docs/configuration.md:
- Line ~227: change "what is this user's permission on this allowlisted repository?" to "what is this user's permission on this repository?".
- Line ~245: delete the sentence "A repository missing from the broker's repos allowlist fails the trigger."

After these edits, grep -rniE "allowlist|\\brepos\\b" apps/broker docker docs/deployment.md docs/configuration.md apps/server/src/forgejo/broker-client.ts must not return any broker-allowlist hits. Hits for Forgejo API paths like /repos/… are expected.

Verification

From the repo root: bun run typecheck passes for all workspaces.
cd apps/broker && bun test passes, including the new cases: a leftover repos is ignored, a template without repos is unconfigured, and /permission for other/x returns 200.
cd apps/server && bun test passes (the deck side is unchanged apart from a comment).
sh -n docker/entrypoint.sh passes. Extract the template heredoc and run it through jq . to confirm it is valid JSON.
Broker smoke test: start the broker with a 0600 config that contains a leftover "repos": ["a/b"]. It starts. curl --unix-socket <sock> -X POST http://x/permission -d '{"repo":"other/x","user":"alice"}' no longer returns 403 repository not allowed: with no reachable Forgejo it returns 502, and with one it returns 200 {level}.

Implementation: #65 ## Problem The Forgejo admin broker (`apps/broker`, added in #55 / PR #56) only answers `POST /permission` for repositories listed in its config's `repos` allowlist. Any other repository gets `403 repository not allowed`, which fails the trigger. The allowlist does nothing useful: - The agent's Forgejo account must already be a collaborator on a repository before it can act there. - Triggers only arrive from repositories with a configured webhook. It only adds another thing to configure for every repository. It was never a deliberate requirement. It came from an early design idea about future *mutating* admin actions, and the broker only does a read-only permission lookup. Remove it. After this change the broker config is `{forgejoUrl, adminToken, webhookSecret}`, and `/permission` answers for any `owner/name`. Existing config files may still contain `repos`, and so may the unfilled templates the entrypoint wrote earlier. The broker ignores that key silently: no log line, no error. The operator can delete it whenever they like. ## Plan ### 1. Broker config: drop `repos` In `apps/broker/src/config.ts`: - Remove the `repos` field and its doc comment from `BrokerConfig`. - In `parseConfig`, remove the `repos` validation and remove `repos` from the returned object. Unknown keys, including `repos`, are not read at all. That is how a leftover `repos` gets ignored. - Change `isTemplate` to `o.forgejoUrl === "" && o.adminToken === ""`. Drop the `repos` condition so that both old templates (with `"repos": []`) and new ones (without it) count as unconfigured. - In the `loadConfig` doc comment, change "(empty `forgejoUrl` and `adminToken`, no `repos`)" to "(empty `forgejoUrl` and `adminToken`)". ### 2. Broker `/permission`: answer for every repository In `apps/broker/src/server.ts` `createHandler`: - Delete `const repos = new Set(cfg?.repos);`. - Delete the `if (!repos.has(repo.toLowerCase())) { … 403 … }` block. - The rest stays as it is: the 400 shape check, `503 broker not configured` when `cfg === null`, then `fetchPermissionImpl`, which returns 502 on failure or 200 `{level}`. ### 3. Tests - `apps/broker/src/config.test.ts`: - Remove `repos` from `VALID` and from the expected object in "normalises a valid config". - Remove the `[{ repos: ["nope"] }, "repos"]` row. - In the template test, keep the old template with `repos: []` (it must still return `null`), and also assert that `{ forgejoUrl: "", adminToken: "", webhookSecret: "a".repeat(64) }` without `repos` returns `null`. - Add a case: `{ ...VALID, repos: ["nope"] }` loads, and the result equals the config without `repos`. - `apps/broker/src/server.test.ts`: - Remove `repos` from `CFG`. - Delete the test "a repo off the allowlist is 403 and never reaches Forgejo". - Rename "returns the level for an allowlisted repo, case-insensitively" to "returns the level for any repo", using `{ repo: "other/x", user: "alice" }`. Expect 200 with the stubbed level, and expect the stub to have been called with owner `other` and name `x`. Assert the call in the same shape the existing test uses for `s.calls`. - `apps/broker/src/forgejo.test.ts`: remove `repos: []` from `cfg`. ### 4. Container entrypoint and deck client - `docker/entrypoint.sh`: - Remove the `"repos": []` line from the template heredoc, and the comma that ends the `webhookSecret` line before it, so the JSON stays valid. - In the message that follows, change "fill in forgejoUrl, adminToken and repos as root" to "fill in forgejoUrl and adminToken as root". - `apps/server/src/forgejo/broker-client.ts` `checkPermission`: change the doc comment to `/** \`user\`'s permission on \`repo\` (\`owner/name\`). */`. ### 5. Docs - `docs/deployment.md` broker section: - Remove `"repos": ["owner/name"]` from the format example, and the comma after `webhookSecret`. - Delete the bullet "`repos` is the allowlist the broker answers permission checks for." - `docs/configuration.md`: - Line ~227: change "what is this user's permission on this allowlisted repository?" to "what is this user's permission on this repository?". - Line ~245: delete the sentence "A repository missing from the broker's `repos` allowlist fails the trigger." After these edits, `grep -rniE "allowlist|\\brepos\\b" apps/broker docker docs/deployment.md docs/configuration.md apps/server/src/forgejo/broker-client.ts` must not return any broker-allowlist hits. Hits for Forgejo API paths like `/repos/…` are expected. ### Verification - From the repo root: `bun run typecheck` passes for all workspaces. - `cd apps/broker && bun test` passes, including the new cases: a leftover `repos` is ignored, a template without `repos` is unconfigured, and `/permission` for `other/x` returns 200. - `cd apps/server && bun test` passes (the deck side is unchanged apart from a comment). - `sh -n docker/entrypoint.sh` passes. Extract the template heredoc and run it through `jq .` to confirm it is valid JSON. - Broker smoke test: start the broker with a 0600 config that contains a leftover `"repos": ["a/b"]`. It starts. `curl --unix-socket <sock> -X POST http://x/permission -d '{"repo":"other/x","user":"alice"}'` no longer returns `403 repository not allowed`: with no reachable Forgejo it returns 502, and with one it returns 200 `{level}`.