lucas/omp-deck

Watch

Fork 0

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

New issue

Closed

opened 2026-10-21 17:28:34 +02:00 by omp-deck · 1 comment

omp-deck commented

2026-10-21 17:28:34 +02:00

Collaborator

Implementation: #56

Problem

Review triggers (#45 / PR #51) need the reviewer's permission, which needs a repo-admin or site-admin token (GET /repos/{o}/{r}/collaborators/{user}/permission). The agent runs in the deck's process as the same UID, so bash can read anything the deck holds (DB, .env, /proc/<pid>/environ, memory). An admin token in the deck means admin for anyone who can trigger the agent, or for injected text. The webhook secret has the same problem today: it sits in app_settings and is returned by GET /api/forgejo/config.

Goal: a small broker process under a dedicated UID in the same container owns the site-admin token and the webhook secret. The deck (and the agent, same UID) can only call two named, narrow, read-only actions:

verify-webhook: is this body + signature valid HMAC for the secret?
permission: what is user's permission on repo (none/read/write/admin)?

Design rule: the broker can't tell the deck from the agent, so every action must be safe for anyone to call. No generic API proxy; any future mutating action must enforce its safety inside the broker. The Forgejo write token stays in the deck (accepted for now).

Decisions:

Transport: HTTP over one unix socket in a broker-owned directory, connectable by anyone. The deck dials it; only the broker UID can create that path, so the deck knows it's talking to the broker.
Webhook ingress stays at POST /api/forgejo/webhook on the deck; the deck asks the broker to verify and the broker never listens on TCP.
Fail closed: broker unreachable → webhook 503, permission check fails the trigger. No in-process fallback.
The dispatcher checks every trigger's author via the broker instead of trusting repository.permissions from the payload; reviewerCanWrite and its 403 fallback are deleted.
Config: $OMP_DECK_DATA_DIR/broker/config.json (JSON, 0600, broker UID, dir 0700), edited by the operator. The deck UI shows the secret as "managed by the broker" and loses the generator; the API route and the forgejo.webhook_secret row are removed.
Container: no more docker run --user. Starts as root; tini → docker/entrypoint.sh drops privileges per process with setpriv: broker as fixed 999:999, deck as OMP_DECK_UID:OMP_DECK_GID (default 1000:1000) with HOME=/data, bind-mountable (as a whole or per subdirectory). The Justfile run recipe is updated to match.
The deck's local HTTP API exposes the write token? Checked: no (hasToken only). It exposes the webhook secret; that goes away here.

Plan

Context

Add a new workspace app apps/broker (Bun, no runtime deps), wire the deck to it for webhook verification and permission checks, remove the webhook secret from the deck entirely, and change the Docker image to run both processes under separate UIDs. End state: nothing the deck UID can read contains the admin token or the webhook secret.

Approach

1. Broker app `apps/broker`

Independent of all later steps; build it first.

apps/broker/package.json: {"name": "@omp-deck/broker", "private": true, "type": "module", "scripts": {"start": "bun src/index.ts", "typecheck": "tsc --noEmit", "test": "bun test"}}, devDependency typescript matching root; tsconfig.json extending ../../tsconfig.base.json (copy apps/server/tsconfig.json's shape). No dependency on @omp-deck/server; do not import from apps/server.
src/config.ts: export interface BrokerConfig { forgejoUrl: string; adminToken: string; webhookSecret: string; repos: string[] } and export function loadConfig(path: string): BrokerConfig.
- Path: env OMP_BROKER_CONFIG, else ${OMP_DECK_DATA_DIR}/broker/config.json; if neither env set, throw OMP_BROKER_CONFIG or OMP_DECK_DATA_DIR must be set.
- Before reading: fs.statSync; throw unless stat.uid === process.getuid() and (stat.mode & 0o777) === 0o600. Messages: <path> must be owned by uid <n> / <path> must have mode 0600 (is 0<octal>). Also stat the parent dir: owner must be process.getuid() and mode & 0o077 must be 0, else <dir> must be owned by uid <n> with no group/other access.
- Validate: forgejoUrl a http(s):// URL (strip trailing /), adminToken non-empty string, webhookSecret string of ≥ 32 chars, repos array of owner/name strings (compare lowercased). Any violation → throw invalid broker config: <field> …. Missing file → throw broker config not found at <path>.
src/signature.ts: move verifyForgejoSignature from apps/server/src/forgejo/webhook.ts (L33-36 area: 64-hex regex + timingSafeEqual over HMAC-SHA256 of the raw body) verbatim, and move its tests from apps/server/src/forgejo/webhook.test.ts to apps/broker/src/signature.test.ts. Delete the function and its tests from the server (step 3 removes the only caller).
src/forgejo.ts: export type PermissionLevel = "none" | "read" | "write" | "admin" and export async function fetchPermission(cfg, owner, repo, user): Promise<{ ok: true; level: PermissionLevel } | { ok: false; error: string }> using plain fetch (no shared client exists outside apps/server) to ${forgejoUrl}/api/v1/repos/${enc(owner)}/${enc(repo)}/collaborators/${enc(user)}/permission, header Authorization: token <adminToken>, AbortSignal.timeout(10_000). Map permission: owner/admin → admin, write → write, read → read, anything else → none. HTTP 404 → {ok: true, level: "none"} (unknown user). Other non-2xx → {ok:false, error: \forgejo ${status}`}; network error → {ok:false, error: `forgejo unreachable: ${message}`}`. Never put the token or response body in errors.
src/server.ts: export function createHandler(cfg: BrokerConfig, fetchPermissionImpl = fetchPermission): (req: Request) => Promise<Response> (injectable for tests). Routes, all JSON, request body capped at 2 MiB (check content-length and actual length; over → 413 {error:"too large"}):
- POST /verify-webhook body { signature: string | null, body: string /* base64 */ } → 200 {"valid": boolean}. Bad shape → 400 {error:"invalid request"}.
- POST /permission body { repo: "owner/name", user: string } → 200 {"level": PermissionLevel}; repo not in cfg.repos (case-insensitive) → 403 {"error":"repository not allowed"}; fetch failure → 502 {"error": <error>}; bad shape → 400.
- Anything else → 404.
- Log one line per request: route, repo/user for permission, result; never bodies, signatures, or tokens.
src/index.ts: load config (on error: log message, process.exit(1)); socket path env OMP_BROKER_SOCKET, default /run/omp-broker/broker.sock; fs.rmSync(path, {force: true}); Bun.serve({ unix: path, fetch: handler }); then fs.chmodSync(path, 0o666); log broker listening on <path>. SIGTERM/SIGINT → server.stop(true), remove socket, exit 0. The config is read once at start; changing it needs a restart (documented).
Tests apps/broker/src/server.test.ts (bun:test): valid/invalid signature round-trip, 403 for non-allowlisted repo, level mapping incl. owner → admin, 404 → none, 502 on fake failure, 400 on bad shape. config.test.ts: mode 0644 rejected, wrong-owner can't be produced without root so only test mode + dir mode + validation errors.

2. Deck broker client

New apps/server/src/forgejo/broker-client.ts:
- export type PermissionLevel = "none" | "read" | "write" | "admin"; export type BrokerResult<T> = { ok: true; data: T } | { ok: false; error: string }.
- export function brokerSocketPath(): string → process.env.OMP_BROKER_SOCKET?.trim() || "/run/omp-broker/broker.sock".
- export async function verifyWebhookSignature(body: Uint8Array, signature: string | undefined): Promise<BrokerResult<boolean>> and export async function checkPermission(repo: string, user: string): Promise<BrokerResult<PermissionLevel>>, both fetch("http://broker/<route>", { unix: brokerSocketPath(), method: "POST", headers: {"content-type": "application/json"}, body, signal: AbortSignal.timeout(10_000) }). Connection error → {ok:false, error: \broker unavailable: ${message}`}; non-200 → {ok:false, error: `broker ${status}: ${json.error ?? ""}`}; malformed JSON reply → {ok:false, error:"broker returned an invalid response"}`.
Add OMP_BROKER_SOCKET to apps/server/src/env-schema.ts next to OMP_DECK_DATA_DIR (valueType path, sensitive false, restartRequired false — read per call) and to docs/configuration.md.

3. Webhook route verifies via the broker; secret removed from the deck

Depends on 2.

apps/server/src/routes-forgejo-webhook.ts: delete the readWebhookSecret() lookup. Keep the !cfg 503 path with its existing record (not_configured, forgejo webhook not configured). Replace verifyForgejoSignature(body, signature, secret) with await verifyWebhookSignature(body, signature):
- !res.ok → log.warn, record(c, { outcome: "not_configured", httpStatus: 503, verified: false, detail: clip(\webhook broker unavailable: ${res.error}`, HEADER_TEXT_MAX), bodyBytes }), return c.json({ error: "webhook broker unavailable" }, 503). Reuse the not_configured` outcome so migration 012's CHECK constraint and the protocol union stay unchanged.
- res.data === false → today's 401 branch; change the detail text "signature does not match the deck's webhook secret" to "signature does not match the broker's webhook secret".
- Update the module doc comment: authenticity rests on the HMAC, checked by the broker.
apps/server/src/forgejo/config.ts: remove KEY_WEBHOOK_SECRET, webhookSecret from Stored/readStored/readForgejoConfigPublic, readWebhookSecret, generateWebhookSecret, the randomBytes import, the secret line in clearForgejoConfig, and the webhook-secret paragraph of the module doc (replace with one sentence: the webhook secret lives in the broker's config, not here).
packages/protocol/src/index.ts ForgejoConfigResponse: delete webhookSecret.
apps/server/src/routes-forgejo.ts: delete POST /forgejo/webhook-secret (route L152-156), its import and its line in the header comment; update the DELETE header line to drop "and webhook secret".
New migration apps/server/src/db/migrations/016-drop-forgejo-webhook-secret.sql: header comment (the secret moved to the broker config) and DELETE FROM app_settings WHERE key = 'forgejo.webhook_secret';.
apps/web/src/lib/forgejo-api.ts: delete generateWebhookSecret. apps/web/src/components/settings/ForgejoPanel.tsx: delete generateSecret, the "secret" busy state, and the Secret block's generate/show UI (L256-276); in its place render label Secret with text Managed by the webhook broker: the "webhookSecret" in <data dir>/broker/config.json. Change the instructions bullet to Secret = the broker's webhookSecret. Remove now-unused imports (KeyRound if unused).
Tests: routes-forgejo-webhook.test.ts — replace secret seeding with a broker stub: in the test, start Bun.serve({ unix: <tmp>/broker.sock, fetch }) answering /verify-webhook with {valid} computed by an HMAC over a test secret, and set OMP_BROKER_SOCKET; replace the "no secret stored" test with "broker unreachable: 503, logged as not_configured" (point OMP_BROKER_SOCKET at a missing path). config.test.ts — delete the two generateWebhookSecret tests, drop webhookSecret from expectations, rowCount after save + clear becomes 3. forgejo-trigger-tools.test.ts needs no change.

4. Dispatcher checks every author via the broker

Depends on 2; independent of 3.

apps/server/src/forgejo/dispatcher.ts: add to the deps interface (next to agentUsername, ~L80) checkPermission: (repo: string, user: string) => Promise<BrokerResult<PermissionLevel>>;. In gate() replace the whole if (reviewSubmitted) { … } else { … } block (L312-333) with:
- if reviewSubmitted && !ownPr → drop "review on a pull request not authored by the agent" (unchanged).
- const perm = await this.deps.checkPermission(row.repo, row.commentAuthor); !perm.ok → await this.fail(job, \could not check the author's permission: ${perm.error}`, "internal", null); return null;`.
- perm.data !== "write" && perm.data !== "admin" → drop \author lacks write access (${perm.data})``.
- Keep the sender≠author edit check (L305-309) as is. Rewrite the comment at L299-304: the sender check stops edits by someone else; permission comes from the broker for the trigger's author, never from repository.permissions in the payload.
- Remove repository?.permissions from PayloadFields (L99) and update the module doc at L19-20 to say "checked through the webhook broker".
apps/server/src/forgejo/dispatch-api.ts: delete reviewerCanWrite (interface L21-30, impl L132-157), RawTeam, WRITE_PERMISSIONS.
apps/server/src/index.ts (~L109-115): pass checkPermission from ./forgejo/broker-client.ts.
dispatcher.test.ts: delete canWrite/reviewChecks/reviewerCanWrite from FakeApi; add harness fields permission: BrokerResult<PermissionLevel> = { ok: true, data: "write" } and permissionChecks: string[], wired into deps.checkPermission. Convert payload-permission tests (L441, L503, L521) to set h.permission (read → reason author lacks write access (read)); replace "payload without permissions fails closed" with "broker failure fails the trigger" asserting status failed and reason containing could not check the author's permission. Convert review tests at L1100/L1110 the same way. Add one test: payload claims push: true but broker says read → dropped.

5. Container: two UIDs via entrypoint

Independent of 2-4 at build level; ship together.

Dockerfile runtime stage: add tini to the apt list; add COPY apps/broker/package.json apps/broker/ in both stages next to the other workspace manifests (frozen lockfile needs the full graph); COPY apps/broker apps/broker and COPY docker/entrypoint.sh /usr/local/bin/omp-deck-entrypoint (+ chmod 0755); ENV OMP_DECK_DATA_DIR=/data/omp-deck OMP_BROKER_SOCKET=/run/omp-broker/broker.sock; replace CMD with ENTRYPOINT ["/usr/bin/tini", "--", "/usr/local/bin/omp-deck-entrypoint"]. No USER, no VOLUME. Update the header run example: drop --user, use -v <host-dir>:/data, mention -e OMP_DECK_UID/OMP_DECK_GID. Regenerate bun.lock with bun install.
docker/entrypoint.sh (#!/bin/sh, set -eu), in order:
1. [ "$(id -u)" = 0 ] || { echo "omp-deck: the container must start as root (do not pass --user); set OMP_DECK_UID/OMP_DECK_GID instead" >&2; exit 1; }.
2. DECK_UID=${OMP_DECK_UID:-1000}, DECK_GID=${OMP_DECK_GID:-1000}; refuse 0 or 999 for either: omp-deck: OMP_DECK_UID/GID must not be 0 or 999.
3. mkdir -p /data. If /data is not a mount point (! mountpoint -q /data, util-linux; e.g. only subdirectories are bind-mounted, as in the Justfile) and is owned by uid 0, chown "$DECK_UID:$DECK_GID" /data (that directory only, never recursive). Then, if stat -c %u /data ≠ DECK_UID → echo "omp-deck: /data must be owned by $DECK_UID:$DECK_GID (chown the bind-mounted host dir)"; exit 1. Never chown a mount or anything under /data.
4. As the deck UID: setpriv --reuid=$DECK_UID --regid=$DECK_GID --clear-groups mkdir -p "$OMP_DECK_DATA_DIR".
5. Broker dir B="$OMP_DECK_DATA_DIR/broker": if absent, mkdir "$B", chown 999:999 "$B", chmod 0700 "$B". Then verify: owner of $B is 999 and mode 700, and $B/config.json exists, is owned by 999 and mode 600; otherwise exit 1 with omp-deck: <path> must be owned by 999:999 with mode <m>; create the broker config as described in docs/deployment.md. (A dir planted by the deck UID can't be owned by 999, so this blocks config substitution.)
6. mkdir -p /run/omp-broker && chown 999:999 /run/omp-broker && chmod 0755 /run/omp-broker.
7. Start broker in background: setpriv --reuid=999 --regid=999 --clear-groups env HOME=/nonexistent bun /app/apps/broker/src/index.ts & BROKER=$!. Wait up to 10 s (poll [ -S "$OMP_BROKER_SOCKET" ] every 0.2 s); on timeout or broker exit → kill it, exit 1 with omp-deck: broker did not start.
8. Start deck: cd /app/apps/server && setpriv --reuid=$DECK_UID --regid=$DECK_GID --clear-groups env HOME=/data bun src/index.ts & DECK=$!. trap 'kill -TERM $DECK $BROKER 2>/dev/null' TERM INT. Loop wait -n is not POSIX sh: use while kill -0 $DECK 2>/dev/null && kill -0 $BROKER 2>/dev/null; do sleep 1; done, then kill the survivor, wait, exit with status 1 if the broker died first, else the deck's status. The deck's self-restart (scheduleRestart in apps/server/src/index.ts, spawns a detached child then process.exit(0)) therefore ends the container, which matches today's behavior where the deck is PID 1; rely on the container restart policy.
The broker uses no OMP_DECK_UID-owned paths except reading through $OMP_DECK_DATA_DIR/broker (traversal through /data needs x for others: document chmod o+x on the host dir if it's 0700).
docs/deployment.md Docker section: new run example, the broker config file format ({"forgejoUrl": "...", "adminToken": "...", "webhookSecret": "<openssl rand -hex 32>", "repos": ["owner/name"]}), how to create it (sudo install -o 999 -g 999 -m 0700 -d <host>/omp-deck/broker and sudo install -o 999 -g 999 -m 0600 config.json …), that the admin token must belong to a site admin, that config changes need a container restart, and the --user removal. docs/configuration.md Forgejo section (L198-227): webhook secret now in the broker config; for local dev run OMP_DECK_DATA_DIR=… OMP_BROKER_SOCKET=/tmp/omp-broker.sock bun apps/broker/src/index.ts alongside the deck (same UID is fine for dev).
Justfile run recipe (L23-49): delete --user $(id -u):$(id -g) and -e HOME=/data (the entrypoint sets HOME); add -e OMP_DECK_UID=$(id -u) and -e OMP_DECK_GID=$(id -g). Keep every bind mount and env var, including -e OMP_DECK_DATA_DIR=/data/.config/omp-deck, so the broker config lives on the host at $HOME/.config/omp-deck/broker/config.json. Update the recipe comment: the container starts as root and the entrypoint runs the deck as your host UID/GID and the broker as UID 999; add one comment line that the broker config must exist first (sudo install -o 999 -g 999 -m 0700 -d ~/.config/omp-deck/broker, then sudo install -o 999 -g 999 -m 0600 config.json ~/.config/omp-deck/broker/). No new recipe.

Critical files & anchors

apps/server/src/forgejo/dispatcher.ts gate() L268-340 — permission gate rewrite; keep sender≠author check.
apps/server/src/routes-forgejo-webhook.ts L106-134 — secret lookup and HMAC replaced by broker call.
apps/server/src/forgejo/config.ts — all webhook-secret code removed.
apps/server/src/db/migrations/012-forgejo-deliveries.sql L17 — outcome CHECK; why broker-unavailable reuses not_configured.
Dockerfile L28-35, L85-109 — manifests copied per workspace; entrypoint replaces CMD.

Verification

cd /workspace/omp-deck && bun install && bun run typecheck passes; bun test in apps/server and apps/broker pass.
Grep webhookSecret|webhook_secret|reviewerCanWrite|repository.permissions|verifyForgejoSignature under apps/server, apps/web, packages returns only the migration 016 file.
Local end-to-end (same UID): mkdir -m 700 /tmp/dd/broker with config.json (0600) holding a test secret, the real instance URL, a site-admin token and repo lucas/omp-deck; run broker with OMP_DECK_DATA_DIR=/tmp/dd OMP_BROKER_SOCKET=/tmp/b.sock. Then:
- curl --unix-socket /tmp/b.sock -XPOST http://b/permission -d '{"repo":"lucas/omp-deck","user":"lucas"}' → {"level":"admin"}; "repo":"other/x" → 403 repository not allowed.
- chmod 644 config.json → broker refuses to start with the mode error.
- Deck running with the same OMP_BROKER_SOCKET: POST a body signed with the test secret to /api/forgejo/webhook → accepted (202) or ignored (200); wrong signature → 401; broker stopped → 503 and the Settings → Forgejo delivery log shows webhook broker unavailable: ….
Docker: docker build -t omp-deck .; mkdir data && sudo chown 1000:1000 data, create data/omp-deck/broker/config.json per docs; docker run --rm -p 127.0.0.1:8787:8787 -v $PWD/data:/data omp-deck. Check: docker exec <c> ps -o user,args shows broker as 999 and deck as 1000; docker exec -u 1000 <c> cat /data/omp-deck/broker/config.json → permission denied; --user 1000 run exits with the root-required message; a bind-mounted data dir owned by root → exits with the /data must be owned message and the host dir's owner is unchanged. A Forgejo review on an agent-authored PR by a write collaborator starts a session; by a read-only user is dropped with author lacks write access (read).
Justfile: with ~/.config/omp-deck/broker/config.json created as in the recipe comment, just run starts the container; docker exec omp-deck ps -o user,args shows the deck under your host UID and the broker under 999; files the deck writes under ~/.omp-deck stay owned by your host user.
UI: Settings → Forgejo shows "Managed by the webhook broker" and no generate button; GET /api/forgejo/config has no webhookSecret.

Assumptions & contingencies

setpriv ships in the oven/bun:1.3.14 Debian base (util-linux). If which setpriv fails in the image, add util-linux to the apt list.
Bun's fetch unix option and Bun.serve({ unix }) are used for the socket. If fetch with unix misbehaves on Bun 1.3.14, use node:http request({ socketPath }) in broker-client.ts instead; the wire contract stays the same.
Existing users who ran with --user and a different HOME must move their data under the /data mount; no automatic migration of old paths.

Implementation: #56 ## Problem Review triggers (#45 / PR #51) need the reviewer's permission, which needs a repo-admin or site-admin token (`GET /repos/{o}/{r}/collaborators/{user}/permission`). The agent runs in the deck's process as the same UID, so bash can read anything the deck holds (DB, `.env`, `/proc/<pid>/environ`, memory). An admin token in the deck means admin for anyone who can trigger the agent, or for injected text. The webhook secret has the same problem today: it sits in `app_settings` and is returned by `GET /api/forgejo/config`. Goal: a small broker process under a dedicated UID in the same container owns the **site-admin token** and the **webhook secret**. The deck (and the agent, same UID) can only call two named, narrow, read-only actions: - `verify-webhook`: is this body + signature valid HMAC for the secret? - `permission`: what is `user`'s permission on `repo` (none/read/write/admin)? Design rule: the broker can't tell the deck from the agent, so every action must be safe for anyone to call. No generic API proxy; any future mutating action must enforce its safety inside the broker. The Forgejo **write** token stays in the deck (accepted for now). Decisions: - Transport: HTTP over one unix socket in a broker-owned directory, connectable by anyone. The deck dials it; only the broker UID can create that path, so the deck knows it's talking to the broker. - Webhook ingress stays at `POST /api/forgejo/webhook` on the deck; the deck asks the broker to verify and the broker never listens on TCP. - Fail closed: broker unreachable → webhook 503, permission check fails the trigger. No in-process fallback. - The dispatcher checks every trigger's author via the broker instead of trusting `repository.permissions` from the payload; `reviewerCanWrite` and its 403 fallback are deleted. - Config: `$OMP_DECK_DATA_DIR/broker/config.json` (JSON, 0600, broker UID, dir 0700), edited by the operator. The deck UI shows the secret as "managed by the broker" and loses the generator; the API route and the `forgejo.webhook_secret` row are removed. - Container: no more `docker run --user`. Starts as root; `tini` → `docker/entrypoint.sh` drops privileges per process with `setpriv`: broker as fixed `999:999`, deck as `OMP_DECK_UID:OMP_DECK_GID` (default `1000:1000`) with `HOME=/data`, bind-mountable (as a whole or per subdirectory). The `Justfile` `run` recipe is updated to match. - The deck's local HTTP API exposes the write token? Checked: no (`hasToken` only). It exposes the webhook secret; that goes away here. ## Plan ### Context Add a new workspace app `apps/broker` (Bun, no runtime deps), wire the deck to it for webhook verification and permission checks, remove the webhook secret from the deck entirely, and change the Docker image to run both processes under separate UIDs. End state: nothing the deck UID can read contains the admin token or the webhook secret. ### Approach #### 1. Broker app `apps/broker` Independent of all later steps; build it first. - `apps/broker/package.json`: `{"name": "@omp-deck/broker", "private": true, "type": "module", "scripts": {"start": "bun src/index.ts", "typecheck": "tsc --noEmit", "test": "bun test"}}`, devDependency `typescript` matching root; `tsconfig.json` extending `../../tsconfig.base.json` (copy `apps/server/tsconfig.json`'s shape). No dependency on `@omp-deck/server`; do not import from `apps/server`. - `src/config.ts`: `export interface BrokerConfig { forgejoUrl: string; adminToken: string; webhookSecret: string; repos: string[] }` and `export function loadConfig(path: string): BrokerConfig`. - Path: env `OMP_BROKER_CONFIG`, else `${OMP_DECK_DATA_DIR}/broker/config.json`; if neither env set, throw `OMP_BROKER_CONFIG or OMP_DECK_DATA_DIR must be set`. - Before reading: `fs.statSync`; throw unless `stat.uid === process.getuid()` and `(stat.mode & 0o777) === 0o600`. Messages: `<path> must be owned by uid <n>` / `<path> must have mode 0600 (is 0<octal>)`. Also stat the parent dir: owner must be `process.getuid()` and mode `& 0o077` must be 0, else `<dir> must be owned by uid <n> with no group/other access`. - Validate: `forgejoUrl` a `http(s)://` URL (strip trailing `/`), `adminToken` non-empty string, `webhookSecret` string of ≥ 32 chars, `repos` array of `owner/name` strings (compare lowercased). Any violation → throw `invalid broker config: <field> …`. Missing file → throw `broker config not found at <path>`. - `src/signature.ts`: move `verifyForgejoSignature` from `apps/server/src/forgejo/webhook.ts` (L33-36 area: 64-hex regex + `timingSafeEqual` over HMAC-SHA256 of the raw body) verbatim, and move its tests from `apps/server/src/forgejo/webhook.test.ts` to `apps/broker/src/signature.test.ts`. Delete the function and its tests from the server (step 3 removes the only caller). - `src/forgejo.ts`: `export type PermissionLevel = "none" | "read" | "write" | "admin"` and `export async function fetchPermission(cfg, owner, repo, user): Promise<{ ok: true; level: PermissionLevel } | { ok: false; error: string }>` using plain `fetch` (no shared client exists outside apps/server) to `${forgejoUrl}/api/v1/repos/${enc(owner)}/${enc(repo)}/collaborators/${enc(user)}/permission`, header `Authorization: token <adminToken>`, `AbortSignal.timeout(10_000)`. Map `permission`: `owner`/`admin` → `admin`, `write` → `write`, `read` → `read`, anything else → `none`. HTTP 404 → `{ok: true, level: "none"}` (unknown user). Other non-2xx → `{ok:false, error: \`forgejo ${status}\`}`; network error → `{ok:false, error: \`forgejo unreachable: ${message}\`}`. Never put the token or response body in errors. - `src/server.ts`: `export function createHandler(cfg: BrokerConfig, fetchPermissionImpl = fetchPermission): (req: Request) => Promise<Response>` (injectable for tests). Routes, all JSON, request body capped at 2 MiB (check `content-length` and actual length; over → 413 `{error:"too large"}`): - `POST /verify-webhook` body `{ signature: string | null, body: string /* base64 */ }` → `200 {"valid": boolean}`. Bad shape → 400 `{error:"invalid request"}`. - `POST /permission` body `{ repo: "owner/name", user: string }` → `200 {"level": PermissionLevel}`; repo not in `cfg.repos` (case-insensitive) → `403 {"error":"repository not allowed"}`; fetch failure → `502 {"error": <error>}`; bad shape → 400. - Anything else → 404. - Log one line per request: route, repo/user for permission, result; never bodies, signatures, or tokens. - `src/index.ts`: load config (on error: log message, `process.exit(1)`); socket path env `OMP_BROKER_SOCKET`, default `/run/omp-broker/broker.sock`; `fs.rmSync(path, {force: true})`; `Bun.serve({ unix: path, fetch: handler })`; then `fs.chmodSync(path, 0o666)`; log `broker listening on <path>`. SIGTERM/SIGINT → `server.stop(true)`, remove socket, exit 0. The config is read once at start; changing it needs a restart (documented). - Tests `apps/broker/src/server.test.ts` (bun:test): valid/invalid signature round-trip, 403 for non-allowlisted repo, level mapping incl. `owner` → `admin`, 404 → `none`, 502 on fake failure, 400 on bad shape. `config.test.ts`: mode 0644 rejected, wrong-owner can't be produced without root so only test mode + dir mode + validation errors. #### 2. Deck broker client - New `apps/server/src/forgejo/broker-client.ts`: - `export type PermissionLevel = "none" | "read" | "write" | "admin"`; `export type BrokerResult<T> = { ok: true; data: T } | { ok: false; error: string }`. - `export function brokerSocketPath(): string` → `process.env.OMP_BROKER_SOCKET?.trim() || "/run/omp-broker/broker.sock"`. - `export async function verifyWebhookSignature(body: Uint8Array, signature: string | undefined): Promise<BrokerResult<boolean>>` and `export async function checkPermission(repo: string, user: string): Promise<BrokerResult<PermissionLevel>>`, both `fetch("http://broker/<route>", { unix: brokerSocketPath(), method: "POST", headers: {"content-type": "application/json"}, body, signal: AbortSignal.timeout(10_000) })`. Connection error → `{ok:false, error: \`broker unavailable: ${message}\`}`; non-200 → `{ok:false, error: \`broker ${status}: ${json.error ?? ""}\`}`; malformed JSON reply → `{ok:false, error:"broker returned an invalid response"}`. - Add `OMP_BROKER_SOCKET` to `apps/server/src/env-schema.ts` next to `OMP_DECK_DATA_DIR` (valueType `path`, sensitive false, restartRequired false — read per call) and to `docs/configuration.md`. #### 3. Webhook route verifies via the broker; secret removed from the deck Depends on 2. - `apps/server/src/routes-forgejo-webhook.ts`: delete the `readWebhookSecret()` lookup. Keep the `!cfg` 503 path with its existing record (`not_configured`, `forgejo webhook not configured`). Replace `verifyForgejoSignature(body, signature, secret)` with `await verifyWebhookSignature(body, signature)`: - `!res.ok` → `log.warn`, `record(c, { outcome: "not_configured", httpStatus: 503, verified: false, detail: clip(\`webhook broker unavailable: ${res.error}\`, HEADER_TEXT_MAX), bodyBytes })`, return `c.json({ error: "webhook broker unavailable" }, 503)`. Reuse the `not_configured` outcome so migration 012's CHECK constraint and the protocol union stay unchanged. - `res.data === false` → today's 401 branch; change the detail text `"signature does not match the deck's webhook secret"` to `"signature does not match the broker's webhook secret"`. - Update the module doc comment: authenticity rests on the HMAC, checked by the broker. - `apps/server/src/forgejo/config.ts`: remove `KEY_WEBHOOK_SECRET`, `webhookSecret` from `Stored`/`readStored`/`readForgejoConfigPublic`, `readWebhookSecret`, `generateWebhookSecret`, the `randomBytes` import, the secret line in `clearForgejoConfig`, and the webhook-secret paragraph of the module doc (replace with one sentence: the webhook secret lives in the broker's config, not here). - `packages/protocol/src/index.ts` `ForgejoConfigResponse`: delete `webhookSecret`. - `apps/server/src/routes-forgejo.ts`: delete `POST /forgejo/webhook-secret` (route L152-156), its import and its line in the header comment; update the DELETE header line to drop "and webhook secret". - New migration `apps/server/src/db/migrations/016-drop-forgejo-webhook-secret.sql`: header comment (the secret moved to the broker config) and `DELETE FROM app_settings WHERE key = 'forgejo.webhook_secret';`. - `apps/web/src/lib/forgejo-api.ts`: delete `generateWebhookSecret`. `apps/web/src/components/settings/ForgejoPanel.tsx`: delete `generateSecret`, the `"secret"` busy state, and the Secret block's generate/show UI (L256-276); in its place render label `Secret` with text `Managed by the webhook broker: the "webhookSecret" in <data dir>/broker/config.json.` Change the instructions bullet to `Secret = the broker's webhookSecret`. Remove now-unused imports (`KeyRound` if unused). - Tests: `routes-forgejo-webhook.test.ts` — replace secret seeding with a broker stub: in the test, start `Bun.serve({ unix: <tmp>/broker.sock, fetch })` answering `/verify-webhook` with `{valid}` computed by an HMAC over a test secret, and set `OMP_BROKER_SOCKET`; replace the "no secret stored" test with "broker unreachable: 503, logged as not_configured" (point `OMP_BROKER_SOCKET` at a missing path). `config.test.ts` — delete the two `generateWebhookSecret` tests, drop `webhookSecret` from expectations, `rowCount` after save + clear becomes 3. `forgejo-trigger-tools.test.ts` needs no change. #### 4. Dispatcher checks every author via the broker Depends on 2; independent of 3. - `apps/server/src/forgejo/dispatcher.ts`: add to the deps interface (next to `agentUsername`, ~L80) `checkPermission: (repo: string, user: string) => Promise<BrokerResult<PermissionLevel>>;`. In `gate()` replace the whole `if (reviewSubmitted) { … } else { … }` block (L312-333) with: - if `reviewSubmitted && !ownPr` → drop `"review on a pull request not authored by the agent"` (unchanged). - `const perm = await this.deps.checkPermission(row.repo, row.commentAuthor);` `!perm.ok` → `await this.fail(job, \`could not check the author's permission: ${perm.error}\`, "internal", null); return null;`. - `perm.data !== "write" && perm.data !== "admin"` → drop `\`author lacks write access (${perm.data})\``. - Keep the sender≠author edit check (L305-309) as is. Rewrite the comment at L299-304: the sender check stops edits by someone else; permission comes from the broker for the trigger's author, never from `repository.permissions` in the payload. - Remove `repository?.permissions` from `PayloadFields` (L99) and update the module doc at L19-20 to say "checked through the webhook broker". - `apps/server/src/forgejo/dispatch-api.ts`: delete `reviewerCanWrite` (interface L21-30, impl L132-157), `RawTeam`, `WRITE_PERMISSIONS`. - `apps/server/src/index.ts` (~L109-115): pass `checkPermission` from `./forgejo/broker-client.ts`. - `dispatcher.test.ts`: delete `canWrite`/`reviewChecks`/`reviewerCanWrite` from `FakeApi`; add harness fields `permission: BrokerResult<PermissionLevel> = { ok: true, data: "write" }` and `permissionChecks: string[]`, wired into `deps.checkPermission`. Convert payload-permission tests (L441, L503, L521) to set `h.permission` (`read` → reason `author lacks write access (read)`); replace "payload without permissions fails closed" with "broker failure fails the trigger" asserting status `failed` and reason containing `could not check the author's permission`. Convert review tests at L1100/L1110 the same way. Add one test: payload claims `push: true` but broker says `read` → dropped. #### 5. Container: two UIDs via entrypoint Independent of 2-4 at build level; ship together. - `Dockerfile` runtime stage: add `tini` to the apt list; add `COPY apps/broker/package.json apps/broker/` in both stages next to the other workspace manifests (frozen lockfile needs the full graph); `COPY apps/broker apps/broker` and `COPY docker/entrypoint.sh /usr/local/bin/omp-deck-entrypoint` (+ `chmod 0755`); `ENV OMP_DECK_DATA_DIR=/data/omp-deck OMP_BROKER_SOCKET=/run/omp-broker/broker.sock`; replace `CMD` with `ENTRYPOINT ["/usr/bin/tini", "--", "/usr/local/bin/omp-deck-entrypoint"]`. No `USER`, no `VOLUME`. Update the header run example: drop `--user`, use `-v <host-dir>:/data`, mention `-e OMP_DECK_UID/OMP_DECK_GID`. Regenerate `bun.lock` with `bun install`. - `docker/entrypoint.sh` (`#!/bin/sh`, `set -eu`), in order: 1. `[ "$(id -u)" = 0 ] || { echo "omp-deck: the container must start as root (do not pass --user); set OMP_DECK_UID/OMP_DECK_GID instead" >&2; exit 1; }`. 2. `DECK_UID=${OMP_DECK_UID:-1000}`, `DECK_GID=${OMP_DECK_GID:-1000}`; refuse `0` or `999` for either: `omp-deck: OMP_DECK_UID/GID must not be 0 or 999`. 3. `mkdir -p /data`. If `/data` is not a mount point (`! mountpoint -q /data`, util-linux; e.g. only subdirectories are bind-mounted, as in the `Justfile`) and is owned by uid 0, `chown "$DECK_UID:$DECK_GID" /data` (that directory only, never recursive). Then, if `stat -c %u /data` ≠ `DECK_UID` → `echo "omp-deck: /data must be owned by $DECK_UID:$DECK_GID (chown the bind-mounted host dir)"; exit 1`. Never chown a mount or anything under `/data`. 4. As the deck UID: `setpriv --reuid=$DECK_UID --regid=$DECK_GID --clear-groups mkdir -p "$OMP_DECK_DATA_DIR"`. 5. Broker dir `B="$OMP_DECK_DATA_DIR/broker"`: if absent, `mkdir "$B"`, `chown 999:999 "$B"`, `chmod 0700 "$B"`. Then verify: owner of `$B` is 999 and mode `700`, and `$B/config.json` exists, is owned by 999 and mode `600`; otherwise exit 1 with `omp-deck: <path> must be owned by 999:999 with mode <m>; create the broker config as described in docs/deployment.md`. (A dir planted by the deck UID can't be owned by 999, so this blocks config substitution.) 6. `mkdir -p /run/omp-broker && chown 999:999 /run/omp-broker && chmod 0755 /run/omp-broker`. 7. Start broker in background: `setpriv --reuid=999 --regid=999 --clear-groups env HOME=/nonexistent bun /app/apps/broker/src/index.ts & BROKER=$!`. Wait up to 10 s (poll `[ -S "$OMP_BROKER_SOCKET" ]` every 0.2 s); on timeout or broker exit → kill it, exit 1 with `omp-deck: broker did not start`. 8. Start deck: `cd /app/apps/server && setpriv --reuid=$DECK_UID --regid=$DECK_GID --clear-groups env HOME=/data bun src/index.ts & DECK=$!`. `trap 'kill -TERM $DECK $BROKER 2>/dev/null' TERM INT`. Loop `wait -n` is not POSIX sh: use `while kill -0 $DECK 2>/dev/null && kill -0 $BROKER 2>/dev/null; do sleep 1; done`, then kill the survivor, `wait`, exit with status 1 if the broker died first, else the deck's status. The deck's self-restart (`scheduleRestart` in `apps/server/src/index.ts`, spawns a detached child then `process.exit(0)`) therefore ends the container, which matches today's behavior where the deck is PID 1; rely on the container restart policy. - The broker uses no `OMP_DECK_UID`-owned paths except reading through `$OMP_DECK_DATA_DIR/broker` (traversal through `/data` needs `x` for others: document `chmod o+x` on the host dir if it's 0700). - `docs/deployment.md` Docker section: new run example, the broker config file format (`{"forgejoUrl": "...", "adminToken": "...", "webhookSecret": "<openssl rand -hex 32>", "repos": ["owner/name"]}`), how to create it (`sudo install -o 999 -g 999 -m 0700 -d <host>/omp-deck/broker` and `sudo install -o 999 -g 999 -m 0600 config.json …`), that the admin token must belong to a site admin, that config changes need a container restart, and the `--user` removal. `docs/configuration.md` Forgejo section (L198-227): webhook secret now in the broker config; for local dev run `OMP_DECK_DATA_DIR=… OMP_BROKER_SOCKET=/tmp/omp-broker.sock bun apps/broker/src/index.ts` alongside the deck (same UID is fine for dev). - `Justfile` `run` recipe (L23-49): delete `--user $(id -u):$(id -g)` and `-e HOME=/data` (the entrypoint sets HOME); add `-e OMP_DECK_UID=$(id -u)` and `-e OMP_DECK_GID=$(id -g)`. Keep every bind mount and env var, including `-e OMP_DECK_DATA_DIR=/data/.config/omp-deck`, so the broker config lives on the host at `$HOME/.config/omp-deck/broker/config.json`. Update the recipe comment: the container starts as root and the entrypoint runs the deck as your host UID/GID and the broker as UID 999; add one comment line that the broker config must exist first (`sudo install -o 999 -g 999 -m 0700 -d ~/.config/omp-deck/broker`, then `sudo install -o 999 -g 999 -m 0600 config.json ~/.config/omp-deck/broker/`). No new recipe. ### Critical files & anchors - `apps/server/src/forgejo/dispatcher.ts` `gate()` L268-340 — permission gate rewrite; keep sender≠author check. - `apps/server/src/routes-forgejo-webhook.ts` L106-134 — secret lookup and HMAC replaced by broker call. - `apps/server/src/forgejo/config.ts` — all webhook-secret code removed. - `apps/server/src/db/migrations/012-forgejo-deliveries.sql` L17 — outcome CHECK; why broker-unavailable reuses `not_configured`. - `Dockerfile` L28-35, L85-109 — manifests copied per workspace; entrypoint replaces CMD. ### Verification - `cd /workspace/omp-deck && bun install && bun run typecheck` passes; `bun test` in `apps/server` and `apps/broker` pass. - Grep `webhookSecret|webhook_secret|reviewerCanWrite|repository.permissions|verifyForgejoSignature` under `apps/server`, `apps/web`, `packages` returns only the migration 016 file. - Local end-to-end (same UID): `mkdir -m 700 /tmp/dd/broker` with `config.json` (0600) holding a test secret, the real instance URL, a site-admin token and repo `lucas/omp-deck`; run broker with `OMP_DECK_DATA_DIR=/tmp/dd OMP_BROKER_SOCKET=/tmp/b.sock`. Then: - `curl --unix-socket /tmp/b.sock -XPOST http://b/permission -d '{"repo":"lucas/omp-deck","user":"lucas"}'` → `{"level":"admin"}`; `"repo":"other/x"` → 403 `repository not allowed`. - `chmod 644 config.json` → broker refuses to start with the mode error. - Deck running with the same `OMP_BROKER_SOCKET`: POST a body signed with the test secret to `/api/forgejo/webhook` → accepted (202) or ignored (200); wrong signature → 401; broker stopped → 503 and the Settings → Forgejo delivery log shows `webhook broker unavailable: …`. - Docker: `docker build -t omp-deck .`; `mkdir data && sudo chown 1000:1000 data`, create `data/omp-deck/broker/config.json` per docs; `docker run --rm -p 127.0.0.1:8787:8787 -v $PWD/data:/data omp-deck`. Check: `docker exec <c> ps -o user,args` shows broker as 999 and deck as 1000; `docker exec -u 1000 <c> cat /data/omp-deck/broker/config.json` → permission denied; `--user 1000` run exits with the root-required message; a bind-mounted data dir owned by root → exits with the `/data must be owned` message and the host dir's owner is unchanged. A Forgejo review on an agent-authored PR by a write collaborator starts a session; by a read-only user is dropped with `author lacks write access (read)`. - Justfile: with `~/.config/omp-deck/broker/config.json` created as in the recipe comment, `just run` starts the container; `docker exec omp-deck ps -o user,args` shows the deck under your host UID and the broker under 999; files the deck writes under `~/.omp-deck` stay owned by your host user. - UI: Settings → Forgejo shows "Managed by the webhook broker" and no generate button; `GET /api/forgejo/config` has no `webhookSecret`. ### Assumptions & contingencies - `setpriv` ships in the `oven/bun:1.3.14` Debian base (util-linux). If `which setpriv` fails in the image, add `util-linux` to the apt list. - Bun's `fetch` `unix` option and `Bun.serve({ unix })` are used for the socket. If `fetch` with `unix` misbehaves on Bun 1.3.14, use `node:http` `request({ socketPath })` in `broker-client.ts` instead; the wire contract stays the same. - Existing users who ran with `--user` and a different HOME must move their data under the `/data` mount; no automatic migration of old paths.

lucas added the

labels

2026-10-21 17:30:21 +02:00

omp-deck added

and removed

labels

2026-10-21 17:31:10 +02:00

omp-deck commented

2026-10-21 17:32:46 +02:00

Author

Collaborator

Deviation from the plan (step 1): the plan says to move verifyForgejoSignature's tests from apps/server/src/forgejo/webhook.test.ts, but that file has none. I copied the function into apps/broker/src/signature.ts and covered it with new tests in apps/broker/src/server.test.ts (valid, wrong secret, missing and tampered signature). The server copy is deleted once the webhook route switches to the broker.

Deviation from the plan (step 1): the plan says to move `verifyForgejoSignature`'s tests from `apps/server/src/forgejo/webhook.test.ts`, but that file has none. I copied the function into `apps/broker/src/signature.ts` and covered it with new tests in `apps/broker/src/server.test.ts` (valid, wrong secret, missing and tampered signature). The server copy is deleted once the webhook route switches to the broker.

omp-deck referenced this issue from a commit

2026-10-21 17:34:59 +02:00

feat(broker): add Forgejo admin broker app

omp-deck referenced this issue from a commit

2026-10-21 17:34:59 +02:00

feat(forgejo): verify webhook signatures through the broker

omp-deck referenced this issue from a commit

2026-10-21 17:34:59 +02:00

feat(db): drop the stored Forgejo webhook secret

omp-deck referenced this issue from a pull request that will close it,

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

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