Loosen the Forgejo trigger write-access gate (owners, openers, private-repo conversations) #214

New issue

Closed

opened 2026-11-03 11:07:37 +01:00 by omp-deck · 0 comments

omp-deck commented

2026-11-03 11:07:37 +01:00

Collaborator

Problem

Every Forgejo trigger (comment/@mention, label, issue opened, review request, submitted review) is dropped with author lacks write access (<perm>) unless the broker reports write/admin for the trigger author. This should be loosened in three ways:

The repository owner is always allowed.
The user who opened the issue (or PR) the trigger is on is always allowed.
On a private repository, a user with read access is allowed, but only when the trigger leads to a read-only session (a forgejo_conversation session). They must never reach a planning, implementer or pull-request session.

Plan

Context

The gate is in apps/server/src/forgejo/dispatcher.ts, ~L623–643: checkPermission(row.repo, row.commentAuthor) followed by the perm.data !== "write" && perm.data !== "admin" drop. The local author (L623–626) is the PR opener for PR-scoped triggers and payload.issue.user.login otherwise; for comments on PRs, payload.issue is the PR. api, owner and repo are in scope there (used at L678).

The PR-author gates for auto-review (~L926) and the CI watchdog (~L973) stay unchanged. They check the PR author, who is always the opener, so exempting openers there would make the require_write_permissions repo config meaningless.

Facts behind the private-repo rule:

Session kinds are chosen in resolveSession (~L1700–1725). Any trigger on a non-PR issue with no mapped session gets forgejo_conversation.
An existing mapped session for the issue is reused (getForgejoSession(row.repo, row.number) → reuseMapped), and that session can be a forgejo_implementer (implementer sessions are mapped from their issue).
If reuse fails, a new session replaces the mapping.
forgejo_conversation's tool set (apps/server/src/bridge/session-policy.ts, CONVERSATION_TOOL_NAMES ~L157–185) has no edit, write, bash or git tools. It can still post comments and create issues; this is accepted.
PR conversations become forgejo_pull_request, which can push when othersPrsPush is set, so PRs are excluded.

Approach

Owner/opener exemption. In dispatcher.ts, just before the permission block (L633), add:
```
const triggerAuthor = row.commentAuthor.toLowerCase();
const repoOwner = row.repo.split("/")[0]?.toLowerCase() ?? "";
const trusted = triggerAuthor === repoOwner || (typeof author === "string" && triggerAuthor === author.toLowerCase());
```
Take the owner from row.repo, never from the payload. For an org-owned repo the exemption never matches, which is intended. Add !trusted && to the block's condition so trusted authors skip checkPermission entirely.
Repo visibility lookup. Add the following to the DispatchApi interface in apps/server/src/forgejo/dispatch-api.ts:
```
/** Whether the repository is private. */
readRepoPrivate(owner: string, repo: string, signal?: AbortSignal): Promise<ApiResult<boolean>>;
```
Implement it in createDispatchApi next to readIssue: client.json<ApiRepo>("GET", repoApiPath(owner, repo), { signal }), then return { ...res, data: res.data.private === true } on success, or res on failure. Mirror how readIssue returns a mapped ApiResult; import ApiRepo from ./tools/api-types.ts. The repo-level fetchVisibility in tools/index.ts is bound to a tool context, so it can't be reused here. Leave it unchanged. The webhook payload's repository.private is not trusted.
Private-repo read exemption. In the permission block, replace the plain drop on non-write permission with the following. If perm.data === "read" and await this.readOnlyAllowed(job, row, reason) returns true, continue (do not drop). Otherwise drop with the existing reason string author lacks write access (${perm.data}). Add the private method:
```
/** Whether a `read` author may still trigger: a conversation turn on an issue of a private repo that can only reach a conversation session. */
private async readOnlyAllowed(job: Job, row: ForgejoTriggerRow, reason: string): Promise<boolean>
```
(Use the row type that job.row already has; reuse whatever type name the file uses.) It returns true only if all of the following hold:
- CONVERSATION_REASONS.has(reason), which excludes labels, issue-opened, review requests and submitted reviews.
- !row.isPull.
- No session is mapped, or the mapped one is a conversation: const mapped = getForgejoSession(row.repo, row.number) is null, or getSessionTrigger(mapped) returns a trigger with kind === "forgejo_conversation". If getSessionTrigger throws or returns null, return false. Otherwise resolveSession could replace an implementer mapping with a new session.
- api.readRepoPrivate(job.owner, job.repo) returns { ok: true, data: true }. Any API failure returns false (the author is dropped) after a log.debug naming the repo and the error.
Call the visibility API last, so it only runs when the other conditions already pass.
Comment. Replace the comment above the block (L628–632) with text that keeps the existing rationale and adds: the repo owner and the issue/PR opener are always allowed; on private repos, read authors may hold a conversation on an issue, because that session has no write tools.
Fakes. Add readRepoPrivate to FakeApi in apps/server/src/forgejo/dispatcher.test.ts (L97). It is backed by a field repoPrivate: ApiResult<boolean> = ok(false) and records calls in repoPrivateReads: string[] ("owner/repo"). The as unknown as DispatchApi fakes in bridge/session-policy.test.ts and forgejo/plan-issue.test.ts need no change.
UI text. In apps/web/src/components/settings/ForgejoPanel.tsx (~L255), change "from authors with write access" to "from authors with write access, the repository owner, or the issue's opener (on private repositories, anyone with read access may converse on issues)".

Critical files & anchors

apps/server/src/forgejo/dispatcher.ts: the permission block ~L613–643; resolveSession ~L1684; reuseMapped ~L1834. getForgejoSession and getSessionTrigger are already imported (L73, L106).
apps/server/src/forgejo/dispatch-api.ts: the DispatchApi interface (L15) and readIssue impl (~L332).
apps/server/src/forgejo/dispatcher.test.ts: enqueue() (L461) supports author, issueAuthor (default carol), repo and isPull. The permission is set with h.permission = { ok: true, data: "read" } (see L802–809). The mapped-session setup is shown in "the issue's existing conversation session is reused" (~L664).

Verification

Add tests near "a sender without write access is dropped" (L652). Unless stated otherwise, each uses h.permission = { ok: true, data: "read" } and author alice:

enqueue({ author: "carol", issueAuthor: "carol" }) → not dropped, a session is created, h.permissionChecks is empty.
enqueue({ author: "lucas" }) on lucas/omp-deck → not dropped, no permission check.
enqueue({ author: "Carol", issueAuthor: "carol" }) → not dropped (case-insensitive).
h.api.repoPrivate = ok(true), plain enqueue() → not dropped, and the created session's trigger kind is forgejo_conversation.
repoPrivate = ok(true), enqueue({ isPull: true }) → dropped author lacks write access (read), and repoPrivateReads is empty.
repoPrivate = ok(true), with an implementer session mapped to issue 7 → dropped with the same reason, and the mapping is unchanged.
repoPrivate = ok(true), enqueueLabel(...) → dropped.
repoPrivate = { ok: false, status: 500, message: "boom" }, plain enqueue() → dropped with the same reason.
repoPrivate = ok(false) → dropped (the existing behaviour).
h.permission = { ok: true, data: "none" } with repoPrivate = ok(true) → dropped.

Existing tests expecting author lacks write access (read) must still pass, because the default repoPrivate is ok(false) and their authors are neither owner nor opener. If a fixture's opener equals its author, change the fixture's opener, not the assertion.

Run: cd apps/server && bun test src/forgejo/dispatcher.test.ts.

## Problem Every Forgejo trigger (comment/@mention, label, issue opened, review request, submitted review) is dropped with `author lacks write access (<perm>)` unless the broker reports `write`/`admin` for the trigger author. This should be loosened in three ways: - The repository owner is always allowed. - The user who opened the issue (or PR) the trigger is on is always allowed. - On a private repository, a user with `read` access is allowed, but only when the trigger leads to a read-only session (a `forgejo_conversation` session). They must never reach a planning, implementer or pull-request session. ## Plan ### Context The gate is in `apps/server/src/forgejo/dispatcher.ts`, ~L623–643: `checkPermission(row.repo, row.commentAuthor)` followed by the `perm.data !== "write" && perm.data !== "admin"` drop. The local `author` (L623–626) is the PR opener for PR-scoped triggers and `payload.issue.user.login` otherwise; for comments on PRs, `payload.issue` is the PR. `api`, `owner` and `repo` are in scope there (used at L678). The PR-author gates for auto-review (~L926) and the CI watchdog (~L973) stay unchanged. They check the PR author, who is always the opener, so exempting openers there would make the `require_write_permissions` repo config meaningless. Facts behind the private-repo rule: - Session kinds are chosen in `resolveSession` (~L1700–1725). Any trigger on a non-PR issue with no mapped session gets `forgejo_conversation`. - An existing mapped session for the issue is reused (`getForgejoSession(row.repo, row.number)` → `reuseMapped`), and that session can be a `forgejo_implementer` (implementer sessions are mapped from their issue). - If reuse fails, a new session *replaces the mapping*. - `forgejo_conversation`'s tool set (`apps/server/src/bridge/session-policy.ts`, `CONVERSATION_TOOL_NAMES` ~L157–185) has no edit, write, bash or git tools. It can still post comments and create issues; this is accepted. - PR conversations become `forgejo_pull_request`, which can push when `othersPrsPush` is set, so PRs are excluded. ### Approach 1. **Owner/opener exemption.** In `dispatcher.ts`, just before the permission block (L633), add: ```ts const triggerAuthor = row.commentAuthor.toLowerCase(); const repoOwner = row.repo.split("/")[0]?.toLowerCase() ?? ""; const trusted = triggerAuthor === repoOwner || (typeof author === "string" && triggerAuthor === author.toLowerCase()); ``` Take the owner from `row.repo`, never from the payload. For an org-owned repo the exemption never matches, which is intended. Add `!trusted &&` to the block's condition so trusted authors skip `checkPermission` entirely. 2. **Repo visibility lookup.** Add the following to the `DispatchApi` interface in `apps/server/src/forgejo/dispatch-api.ts`: ```ts /** Whether the repository is private. */ readRepoPrivate(owner: string, repo: string, signal?: AbortSignal): Promise<ApiResult<boolean>>; ``` Implement it in `createDispatchApi` next to `readIssue`: `client.json<ApiRepo>("GET", repoApiPath(owner, repo), { signal })`, then return `{ ...res, data: res.data.private === true }` on success, or `res` on failure. Mirror how `readIssue` returns a mapped `ApiResult`; import `ApiRepo` from `./tools/api-types.ts`. The repo-level `fetchVisibility` in `tools/index.ts` is bound to a tool context, so it can't be reused here. Leave it unchanged. The webhook payload's `repository.private` is not trusted. 3. **Private-repo read exemption.** In the permission block, replace the plain drop on non-write permission with the following. If `perm.data === "read"` and `await this.readOnlyAllowed(job, row, reason)` returns true, continue (do not drop). Otherwise drop with the existing reason string `author lacks write access (${perm.data})`. Add the private method: ```ts /** Whether a `read` author may still trigger: a conversation turn on an issue of a private repo that can only reach a conversation session. */ private async readOnlyAllowed(job: Job, row: ForgejoTriggerRow, reason: string): Promise<boolean> ``` (Use the row type that `job.row` already has; reuse whatever type name the file uses.) It returns true only if all of the following hold: - `CONVERSATION_REASONS.has(reason)`, which excludes labels, issue-opened, review requests and submitted reviews. - `!row.isPull`. - No session is mapped, or the mapped one is a conversation: `const mapped = getForgejoSession(row.repo, row.number)` is `null`, or `getSessionTrigger(mapped)` returns a trigger with `kind === "forgejo_conversation"`. If `getSessionTrigger` throws or returns `null`, return false. Otherwise `resolveSession` could replace an implementer mapping with a new session. - `api.readRepoPrivate(job.owner, job.repo)` returns `{ ok: true, data: true }`. Any API failure returns false (the author is dropped) after a `log.debug` naming the repo and the error. Call the visibility API last, so it only runs when the other conditions already pass. 4. **Comment.** Replace the comment above the block (L628–632) with text that keeps the existing rationale and adds: the repo owner and the issue/PR opener are always allowed; on private repos, `read` authors may hold a conversation on an issue, because that session has no write tools. 5. **Fakes.** Add `readRepoPrivate` to `FakeApi` in `apps/server/src/forgejo/dispatcher.test.ts` (L97). It is backed by a field `repoPrivate: ApiResult<boolean> = ok(false)` and records calls in `repoPrivateReads: string[]` (`"owner/repo"`). The `as unknown as DispatchApi` fakes in `bridge/session-policy.test.ts` and `forgejo/plan-issue.test.ts` need no change. 6. **UI text.** In `apps/web/src/components/settings/ForgejoPanel.tsx` (~L255), change "from authors with write access" to "from authors with write access, the repository owner, or the issue's opener (on private repositories, anyone with read access may converse on issues)". ### Critical files & anchors - `apps/server/src/forgejo/dispatcher.ts`: the permission block ~L613–643; `resolveSession` ~L1684; `reuseMapped` ~L1834. `getForgejoSession` and `getSessionTrigger` are already imported (L73, L106). - `apps/server/src/forgejo/dispatch-api.ts`: the `DispatchApi` interface (L15) and `readIssue` impl (~L332). - `apps/server/src/forgejo/dispatcher.test.ts`: `enqueue()` (L461) supports `author`, `issueAuthor` (default `carol`), `repo` and `isPull`. The permission is set with `h.permission = { ok: true, data: "read" }` (see L802–809). The mapped-session setup is shown in "the issue's existing conversation session is reused" (~L664). ### Verification Add tests near "a sender without write access is dropped" (L652). Unless stated otherwise, each uses `h.permission = { ok: true, data: "read" }` and author `alice`: - `enqueue({ author: "carol", issueAuthor: "carol" })` → not dropped, a session is created, `h.permissionChecks` is empty. - `enqueue({ author: "lucas" })` on `lucas/omp-deck` → not dropped, no permission check. - `enqueue({ author: "Carol", issueAuthor: "carol" })` → not dropped (case-insensitive). - `h.api.repoPrivate = ok(true)`, plain `enqueue()` → not dropped, and the created session's trigger kind is `forgejo_conversation`. - `repoPrivate = ok(true)`, `enqueue({ isPull: true })` → dropped `author lacks write access (read)`, and `repoPrivateReads` is empty. - `repoPrivate = ok(true)`, with an implementer session mapped to issue 7 → dropped with the same reason, and the mapping is unchanged. - `repoPrivate = ok(true)`, `enqueueLabel(...)` → dropped. - `repoPrivate = { ok: false, status: 500, message: "boom" }`, plain `enqueue()` → dropped with the same reason. - `repoPrivate = ok(false)` → dropped (the existing behaviour). - `h.permission = { ok: true, data: "none" }` with `repoPrivate = ok(true)` → dropped. Existing tests expecting `author lacks write access (read)` must still pass, because the default `repoPrivate` is `ok(false)` and their authors are neither owner nor opener. If a fixture's opener equals its author, change the fixture's opener, not the assertion. Run: `cd apps/server && bun test src/forgejo/dispatcher.test.ts`.

omp-deck added the

Agent

In Progress

label

2026-11-03 11:07:50 +01:00

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

2026-11-03 11:10:53 +01:00

Loosen the Forgejo trigger write-access gate #215