feat: Forgejo admin broker for the admin token and webhook secret

omp-deck commented

2026-10-21 17:35:04 +02:00

Collaborator

Adds apps/broker, a separate process that holds the Forgejo site-admin token and the webhook secret under its own UID. It answers two read-only requests on a unix socket: verify-webhook and permission. The deck checks webhook signatures through it and no longer stores or exposes the webhook secret (API route, UI generator and DB row removed). The dispatcher checks every trigger author's permission through the broker instead of the payload, and reviewerCanWrite is gone. The Docker image now starts as root, and its entrypoint runs the broker as UID 999 and the deck as OMP_DECK_UID:OMP_DECK_GID with HOME=/data.

Decisions

The plan asks to move verifyForgejoSignature's tests, but none existed; the broker has new signature tests instead (comment).
When the broker is unreachable, the webhook's delivery-log entry reuses the not_configured outcome (detail webhook broker unavailable: …), so migration 012's CHECK constraint stays unchanged.
The Justfile is gitignored, so its run recipe change is local only and not in this PR (comment).
docker-compose.yml now bind-mounts /data and passes OMP_DECK_UID/OMP_DECK_GID; its root-owned named volume would have been unwritable by the deck. .dockerignore and .gitignore exclude /data (same comment).
The entrypoint derives the socket directory from OMP_BROKER_SOCKET instead of hard-coding /run/omp-broker.

Verification

bun run typecheck: all workspaces pass.
apps/broker: bun test 15 pass.
apps/server: full bun test 859 pass, including the converted dispatcher tests and the webhook route tests against a stub broker socket.
Broker process smoke test: refuses to start on a 0644 config with must have mode 0600 (is 0644); the socket is 0666; permission for a repo off the allowlist → 403 repository not allowed; verify-webhook with a correct HMAC → {"valid":true}; the socket is removed on SIGTERM. The admin-token permission lookup against a real Forgejo was not exercised (no site-admin token here).
docker/entrypoint.sh: sh -n passes; run as non-root it exits 1 with the root-required message. The Docker build and in-container checks were not run (no Docker in this environment).

Closes #55

Adds `apps/broker`, a separate process that holds the Forgejo site-admin token and the webhook secret under its own UID. It answers two read-only requests on a unix socket: `verify-webhook` and `permission`. The deck checks webhook signatures through it and no longer stores or exposes the webhook secret (API route, UI generator and DB row removed). The dispatcher checks every trigger author's permission through the broker instead of the payload, and `reviewerCanWrite` is gone. The Docker image now starts as root, and its entrypoint runs the broker as UID 999 and the deck as `OMP_DECK_UID:OMP_DECK_GID` with `HOME=/data`. ## Decisions - The plan asks to move `verifyForgejoSignature`'s tests, but none existed; the broker has new signature tests instead ([comment](https://git.sclu1034.dev/lucas/omp-deck/issues/55#issuecomment-16543)). - When the broker is unreachable, the webhook's delivery-log entry reuses the `not_configured` outcome (detail `webhook broker unavailable: …`), so migration 012's CHECK constraint stays unchanged. - The Justfile is gitignored, so its `run` recipe change is local only and not in this PR ([comment](https://git.sclu1034.dev/lucas/omp-deck/pulls/56#issuecomment-16553)). - `docker-compose.yml` now bind-mounts `/data` and passes `OMP_DECK_UID`/`OMP_DECK_GID`; its root-owned named volume would have been unwritable by the deck. `.dockerignore` and `.gitignore` exclude `/data` (same comment). - The entrypoint derives the socket directory from `OMP_BROKER_SOCKET` instead of hard-coding `/run/omp-broker`. ## Verification - `bun run typecheck`: all workspaces pass. - `apps/broker`: `bun test` 15 pass. - `apps/server`: full `bun test` 859 pass, including the converted dispatcher tests and the webhook route tests against a stub broker socket. - Broker process smoke test: refuses to start on a 0644 config with `must have mode 0600 (is 0644)`; the socket is 0666; `permission` for a repo off the allowlist → 403 `repository not allowed`; `verify-webhook` with a correct HMAC → `{"valid":true}`; the socket is removed on SIGTERM. The admin-token permission lookup against a real Forgejo was not exercised (no site-admin token here). - `docker/entrypoint.sh`: `sh -n` passes; run as non-root it exits 1 with the root-required message. The Docker build and in-container checks were **not run** (no Docker in this environment). Closes #55

omp-deck referenced this pull request

2026-10-21 17:35:07 +02:00

Forgejo admin broker: isolate admin token and webhook secret from the agent #55

omp-deck commented

2026-10-21 17:39:44 +02:00

Author

Collaborator

Container step decisions and deviations from the plan:

Justfile not in the PR. .gitignore ignores Justfile ("kept out of the repo as a personal convenience file"), so the run recipe change exists only in the local working tree and can't be committed: --user and -e HOME=/data are removed, -e OMP_DECK_UID=$(id -u) -e OMP_DECK_GID=$(id -g) are added, and the comment explains the broker config setup.
docker-compose.yml updated (the plan didn't cover it). With the new entrypoint, its root-owned named volume at /data/omp-agent would have been unwritable by the deck UID. It now bind-mounts ${OMP_DECK_DATA:-./data} as /data (the deck's HOME, so .omp/agent lives there), passes OMP_DECK_UID/OMP_DECK_GID, and drops OMP_AGENT_DIR.
.dockerignore added with /data, and /data added to .gitignore. Compose's default data dir sits in the build context, and its broker/ subdirectory is readable only by UID 999, which would make sending the context fail.
The broker's socket directory comes from dirname $OMP_BROKER_SOCKET instead of the hard-coded /run/omp-broker, so overriding the socket path keeps working.
Not verified: Docker isn't available in this environment, so the image build and the in-container checks didn't run. Only sh -n and the non-root guard were exercised.

Container step decisions and deviations from the plan: - **Justfile not in the PR.** `.gitignore` ignores `Justfile` ("kept out of the repo as a personal convenience file"), so the `run` recipe change exists only in the local working tree and can't be committed: `--user` and `-e HOME=/data` are removed, `-e OMP_DECK_UID=$(id -u) -e OMP_DECK_GID=$(id -g)` are added, and the comment explains the broker config setup. - **docker-compose.yml updated** (the plan didn't cover it). With the new entrypoint, its root-owned named volume at `/data/omp-agent` would have been unwritable by the deck UID. It now bind-mounts `${OMP_DECK_DATA:-./data}` as `/data` (the deck's HOME, so `.omp/agent` lives there), passes `OMP_DECK_UID`/`OMP_DECK_GID`, and drops `OMP_AGENT_DIR`. - **`.dockerignore` added with `/data`, and `/data` added to `.gitignore`.** Compose's default data dir sits in the build context, and its `broker/` subdirectory is readable only by UID 999, which would make sending the context fail. - The broker's socket directory comes from `dirname $OMP_BROKER_SOCKET` instead of the hard-coded `/run/omp-broker`, so overriding the socket path keeps working. - **Not verified:** Docker isn't available in this environment, so the image build and the in-container checks didn't run. Only `sh -n` and the non-root guard were exercised.

lucas commented

2026-10-22 16:01:10 +02:00

Owner

Right now, the user has to manually create the config file, right? Can't we give the broker a route to set its settings, but it only works on init?
That would alleviate the issue that an agent could overwrite the tokens, because no one can call it a second time. And it would still make the happy path easier because most people would likely just configure things once and rarely need to edit afterwards.

Right now, the user has to manually create the config file, right? Can't we give the broker a route to set its settings, but it only works on init? That would alleviate the issue that an agent could overwrite the tokens, because no one can call it a second time. And it would still make the happy path easier because most people would likely just configure things once and rarely need to edit afterwards.

omp-deck commented

2026-10-22 16:01:12 +02:00

Author

Collaborator

omp-deck failed to handle this event: an internal error occurred.

Details are in omp-deck under webhook delivery 1104699d-7f89-4cd6-83c4-72116bf5da2f.

omp-deck failed to handle this event: an internal error occurred. Details are in omp-deck under webhook delivery `1104699d-7f89-4cd6-83c4-72116bf5da2f`.

omp-deck referenced this pull request

2026-10-22 17:16:29 +02:00

Trigger checkout fails across filesystems: git clone --local → EXDEV #57

omp-deck referenced this pull request

2026-10-22 17:17:27 +02:00

Let trigger checkouts fall back to copying across filesystems #58

lucas commented

2026-10-22 17:24:02 +02:00

Owner

Please answer the previous question

omp-deck commented

2026-10-22 17:24:05 +02:00

Author

Collaborator

This repository isn't connected to an omp-deck workspace, so I can't answer here. Register a checkout of it as a workspace in omp-deck.

omp-deck referenced this pull request

2026-10-22 17:58:45 +02:00

Forgejo: workspace index caches failed origin lookups as "not connected" #59

lucas commented

2026-10-22 18:04:32 +02:00

Owner

Please answer the previous question

omp-deck commented

2026-10-22 18:05:05 +02:00

Author

Collaborator

Sorry for the missed reply. Yes, right now the operator has to create $OMP_DECK_DATA_DIR/broker/config.json by hand, owned by 999 with mode 600. If it's missing, entrypoint.sh refuses to start, and loadConfig would exit anyway.

An init-only route would stop the config from being overwritten, but it opens a race for the first write. The socket is 0666, and server.ts says the broker can't tell the deck from the agent because they run as the same UID. If the broker starts unconfigured and exposes /init, any agent session that runs before the user finishes setup can claim it. That attacker could then:

set a webhookSecret they know and forge webhook deliveries that start sessions, or
point forgejoUrl at a server they control, so /permission returns whatever they want.

After that, the "only once" rule protects the attacker's config. On a fresh install the window may be short. But the deck is usable before Forgejo is set up, so agents can run before setup happens, and the window stays open until the user configures it.

Three ways to keep the easy setup without that race:

One-time setup code (closest to your idea). If the broker starts unconfigured, it generates a random code, prints it only to its own stdout (the container logs), and /init requires that code. The deck/agent UID can't read the broker's stdout or /proc entries, so only someone with access to the container logs can finish setup. After a successful init the code is discarded and the route is gone. The Settings panel could offer a form: "paste the code from docker logs".
Root-side CLI. docker exec -it omp-deck omp-broker init asks for the URL, token and repos, generates the webhook secret, and writes the file with the right owner and mode. There's no network surface and no race, and editing later is the same command. The trade-off is that setup happens in the terminal, not the UI.
Keep the file, but generate it. The entrypoint creates the broker dir and a template config with a random webhookSecret already filled in, so the user only adds the token, URL and repos. This is the smallest change but still needs manual editing.

Either way, the broker needs a "running but unconfigured" state: /verify-webhook returns valid: false and /permission returns 503 until it's configured, and the entrypoint no longer dies when the config is missing.

I'd go with (1), with (2) as the way to fix mistakes, because "can only be called once" means a typo otherwise requires deleting the file by hand. Should I implement (1), (2), or both in this PR?

Sorry for the missed reply. Yes, right now the operator has to create `$OMP_DECK_DATA_DIR/broker/config.json` by hand, owned by 999 with mode 600. If it's missing, `entrypoint.sh` refuses to start, and `loadConfig` would exit anyway. An init-only route would stop the config from being **overwritten**, but it opens a race for the **first write**. The socket is `0666`, and `server.ts` says the broker can't tell the deck from the agent because they run as the same UID. If the broker starts unconfigured and exposes `/init`, any agent session that runs before the user finishes setup can claim it. That attacker could then: - set a `webhookSecret` they know and forge webhook deliveries that start sessions, or - point `forgejoUrl` at a server they control, so `/permission` returns whatever they want. After that, the "only once" rule protects the attacker's config. On a fresh install the window may be short. But the deck is usable before Forgejo is set up, so agents can run before setup happens, and the window stays open until the user configures it. Three ways to keep the easy setup without that race: 1. **One-time setup code (closest to your idea).** If the broker starts unconfigured, it generates a random code, prints it only to its own stdout (the container logs), and `/init` requires that code. The deck/agent UID can't read the broker's stdout or `/proc` entries, so only someone with access to the container logs can finish setup. After a successful init the code is discarded and the route is gone. The Settings panel could offer a form: "paste the code from `docker logs`". 2. **Root-side CLI.** `docker exec -it omp-deck omp-broker init` asks for the URL, token and repos, generates the webhook secret, and writes the file with the right owner and mode. There's no network surface and no race, and editing later is the same command. The trade-off is that setup happens in the terminal, not the UI. 3. **Keep the file, but generate it.** The entrypoint creates the broker dir and a template config with a random `webhookSecret` already filled in, so the user only adds the token, URL and repos. This is the smallest change but still needs manual editing. Either way, the broker needs a "running but unconfigured" state: `/verify-webhook` returns `valid: false` and `/permission` returns 503 until it's configured, and the entrypoint no longer dies when the config is missing. I'd go with (1), with (2) as the way to fix mistakes, because "can only be called once" means a typo otherwise requires deleting the file by hand. Should I implement (1), (2), or both in this PR?

lucas commented

2026-10-22 18:07:59 +02:00

Owner

Let's go with #3 then

omp-deck commented

2026-10-22 18:08:16 +02:00

Author

Collaborator

Option 3 it is. I can't push from this session, so here's the change I'll make. One decision below needs your input first.

docker/entrypoint.sh

If $BROKER_CONFIG is missing, write a template as the broker UID (umask 077, so 999:999 with mode 600):
```
{ "forgejoUrl": "", "adminToken": "", "webhookSecret": "<64 hex chars from /dev/urandom>", "repos": [] }
```
An existing file is never touched, and the ownership and mode checks stay as they are.
Print the path and the next steps to stderr: fill in forgejoUrl, adminToken and repos, then put the generated webhookSecret into the Forgejo webhook. The secret itself is never printed.

Docs: docs/deployment.md drops the manual mkdir/chown/chmod steps. They're replaced by "start once, then edit the generated file as root" (docker exec -u 0 -it omp-deck vi … or sudo on the host) and "restart after editing".

Decision needed: what happens while the template is still unfilled?

(a) Exit after writing it, with a clear message. The current check-and-die behaviour stays, so nothing else changes, but the deck doesn't run until Forgejo is configured. Anyone who doesn't use Forgejo integration would be blocked.
(b) Start in an "unconfigured" mode. The broker recognises the empty template, logs a warning, and answers /verify-webhook with valid: false and /permission with 503. The deck runs normally and only the Forgejo integration stays off until the next restart with a filled-in config. This is more code (a new state in config.ts/index.ts plus tests), but it fits a deck where Forgejo is optional.

I'd go with (b), since Forgejo is opt-in everywhere else in the deck. Which do you want?

Option 3 it is. I can't push from this session, so here's the change I'll make. One decision below needs your input first. **`docker/entrypoint.sh`** - If `$BROKER_CONFIG` is missing, write a template as the broker UID (`umask 077`, so 999:999 with mode 600): ```json { "forgejoUrl": "", "adminToken": "", "webhookSecret": "<64 hex chars from /dev/urandom>", "repos": [] } ``` An existing file is never touched, and the ownership and mode checks stay as they are. - Print the path and the next steps to stderr: fill in `forgejoUrl`, `adminToken` and `repos`, then put the generated `webhookSecret` into the Forgejo webhook. The secret itself is never printed. **Docs:** `docs/deployment.md` drops the manual `mkdir`/`chown`/`chmod` steps. They're replaced by "start once, then edit the generated file as root" (`docker exec -u 0 -it omp-deck vi …` or `sudo` on the host) and "restart after editing". **Decision needed:** what happens while the template is still unfilled? - **(a) Exit after writing it**, with a clear message. The current check-and-die behaviour stays, so nothing else changes, but the deck doesn't run until Forgejo is configured. Anyone who doesn't use Forgejo integration would be blocked. - **(b) Start in an "unconfigured" mode.** The broker recognises the empty template, logs a warning, and answers `/verify-webhook` with `valid: false` and `/permission` with 503. The deck runs normally and only the Forgejo integration stays off until the next restart with a filled-in config. This is more code (a new state in `config.ts`/`index.ts` plus tests), but it fits a deck where Forgejo is optional. I'd go with (b), since Forgejo is opt-in everywhere else in the deck. Which do you want?

omp-deck referenced this pull request

2026-10-23 10:02:43 +02:00

PR sessions on agent-authored PRs can't push or edit #61

lucas commented

2026-10-23 12:07:44 +02:00

Owner

You should now have write access.

omp-deck commented

2026-10-23 12:07:53 +02:00

Author

Collaborator

Thanks. Before I implement option 3, I need your answer on the open question in my last comment: while the generated template is still unfilled, should the deck

(a) exit after writing the template (the current check-and-die behaviour, which blocks deployments that don't use Forgejo), or
(b) start with the broker in an "unconfigured" mode (/verify-webhook returns valid: false, /permission returns 503, and the deck otherwise runs normally)?

I recommend (b). Reply with (a) or (b) and I'll implement it, run the checks and push.

Thanks. Before I implement option 3, I need your answer on the open question in my last comment: while the generated template is still unfilled, should the deck - **(a)** exit after writing the template (the current check-and-die behaviour, which blocks deployments that don't use Forgejo), or - **(b)** start with the broker in an "unconfigured" mode (`/verify-webhook` returns `valid: false`, `/permission` returns 503, and the deck otherwise runs normally)? I recommend (b). Reply with (a) or (b) and I'll implement it, run the checks and push.

lucas commented

2026-10-23 12:10:32 +02:00

Owner

b) please

omp-deck commented

2026-10-23 12:11:52 +02:00

Author

Collaborator

Implemented option 3(b) in 692bdc4.

docker/entrypoint.sh: if broker/config.json is missing, the entrypoint writes a template as UID 999 with umask 077, so the file is 999:999 with mode 600. The template has empty forgejoUrl/adminToken, repos: [], and a 64-hex-char webhookSecret from /dev/urandom. The entrypoint then prints the path and next steps to stderr; the secret isn't printed. An existing file is never touched, and the owner/mode checks still apply.
Broker: loadConfig returns null for the exact unfilled template. In that case the broker logs a warning and runs unconfigured: /verify-webhook returns {valid:false} and /permission returns 503 broker not configured without contacting Forgejo. A partially filled template still fails validation, so the container exits. I made that choice on purpose: a typo should be loud, not leave Forgejo silently off.
Docs: deployment.md replaces the manual install steps with "start once, edit as root (docker exec -u 0 -it omp-deck vi …), restart". configuration.md is updated to match.
Tests: a loadConfig test checks that the template is treated as unconfigured and that a partially filled one is invalid. A handler test checks the unconfigured responses.

Verification:

bun test apps/broker: 17 pass.
sh -n docker/entrypoint.sh passes.
I ran the template-writing snippet (with as_broker replaced by plain sh, since this shell isn't root) and got mode 600 and a 64-char secret.
bun run typecheck couldn't run because tsc isn't installed in this checkout.
Docker still isn't available here, so the in-container flow is untested.

Separately, Forgejo now reports the PR as not mergeable against main. Do you want me to rebase it onto main?

Implemented option 3(b) in 692bdc4. - **`docker/entrypoint.sh`**: if `broker/config.json` is missing, the entrypoint writes a template as UID 999 with `umask 077`, so the file is 999:999 with mode 600. The template has empty `forgejoUrl`/`adminToken`, `repos: []`, and a 64-hex-char `webhookSecret` from `/dev/urandom`. The entrypoint then prints the path and next steps to stderr; the secret isn't printed. An existing file is never touched, and the owner/mode checks still apply. - **Broker**: `loadConfig` returns `null` for the exact unfilled template. In that case the broker logs a warning and runs unconfigured: `/verify-webhook` returns `{valid:false}` and `/permission` returns 503 `broker not configured` without contacting Forgejo. A partially filled template still fails validation, so the container exits. I made that choice on purpose: a typo should be loud, not leave Forgejo silently off. - **Docs**: `deployment.md` replaces the manual `install` steps with "start once, edit as root (`docker exec -u 0 -it omp-deck vi …`), restart". `configuration.md` is updated to match. - **Tests**: a `loadConfig` test checks that the template is treated as unconfigured and that a partially filled one is invalid. A handler test checks the unconfigured responses. Verification: - `bun test apps/broker`: 17 pass. - `sh -n docker/entrypoint.sh` passes. - I ran the template-writing snippet (with `as_broker` replaced by plain `sh`, since this shell isn't root) and got mode 600 and a 64-char secret. - `bun run typecheck` **couldn't run** because `tsc` isn't installed in this checkout. - Docker still isn't available here, so the in-container flow is untested. Separately, Forgejo now reports the PR as **not mergeable** against `main`. Do you want me to rebase it onto `main`?

lucas commented

2026-10-23 14:53:50 +02:00

Owner

Can you also fix the file conflicts?

omp-deck force-pushed 55-forgejo-admin-broker-isolate-admin-token-and-webhook-secret-from-the-agent from 692bdc4d17 to c45d72769d

2026-10-23 14:55:44 +02:00

Compare

omp-deck commented

2026-10-23 14:55:46 +02:00

Author

Collaborator

I rebased the branch onto main and force-pushed it (head is now c45d727). Two files had conflicts, both from main's new readPull / own-PR handling:

dispatcher.ts: I kept the broker permission check for every trigger author. reviewerCanWrite and the payload permissions check are gone, as before. The own-PR variable is now agentPr, matching main, so main's new reviewRequest && agentPr notice still works.
dispatcher.test.ts: I kept main's pull and pullReads fake fields and removed canWrite and reviewChecks.

All checks below pass:

bun run typecheck: all workspaces pass. I had to run bun install --frozen-lockfile first.
apps/server: bun test passes, 866 tests.
apps/broker: bun test passes, 17 tests.

The migration numbering is still sequential: main ends at 015, and this PR adds 016.

I rebased the branch onto `main` and force-pushed it (head is now c45d727). Two files had conflicts, both from `main`'s new `readPull` / own-PR handling: - **`dispatcher.ts`**: I kept the broker permission check for every trigger author. `reviewerCanWrite` and the payload `permissions` check are gone, as before. The own-PR variable is now `agentPr`, matching `main`, so `main`'s new `reviewRequest && agentPr` notice still works. - **`dispatcher.test.ts`**: I kept `main`'s `pull` and `pullReads` fake fields and removed `canWrite` and `reviewChecks`. All checks below pass: - `bun run typecheck`: all workspaces pass. I had to run `bun install --frozen-lockfile` first. - `apps/server`: `bun test` passes, 866 tests. - `apps/broker`: `bun test` passes, 17 tests. The migration numbering is still sequential: `main` ends at 015, and this PR adds 016.

lucas merged commit d261fc3665 into main

2026-10-23 15:20:49 +02:00

lucas deleted branch 55-forgejo-admin-broker-isolate-admin-token-and-webhook-secret-from-the-agent

2026-10-23 15:20:49 +02:00

lucas referenced this pull request from a commit

2026-10-23 15:20:50 +02:00

Merge pull request 'feat: Forgejo admin broker for the admin token and webhook secret' (#56) from 55-forgejo-admin-broker-isolate-admin-token-and-webhook-secret-from-the-agent into main

omp-deck referenced this pull request

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

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

Rows
Columns

feat: Forgejo admin broker for the admin token and webhook secret #56

Decisions

Verification