Skip to content

fix(github-app): the manifest grants the writes the runs actually do - #144

Merged
debuggingfuture merged 1 commit into
mainfrom
manifest-grants-match-the-writes
Aug 22, 2026
Merged

fix(github-app): the manifest grants the writes the runs actually do#144
debuggingfuture merged 1 commit into
mainfrom
manifest-grants-match-the-writes

Conversation

@debuggingfuture

Copy link
Copy Markdown
Member

What

Both manifest copies — the Worker literal (apps/dispatcher/src/routes/github.ts) and the committed mirror (infra/github-app-manifest.json) — go from

   checks:        write
-  contents:      read
+  contents:      write
   deployments:   read
+  issues:        write
   metadata:      read
   pull_requests: write

Why

pull_requests: write only covers opening a PR once a branch exists. Creating the branch and committing the files is contents, so at read every openDraftPullRequest returns 403 unauthorized — and that is the one artifact every proposing run exists to produce (spec-drift-pr, org-spec-audit, self-heal-pr, release-notes). issues was absent entirely, so a fresh install could not file a question at all.

The live fractalboxdev install was raised to contents: write + issues: write on 2026-08-16, after org-spec-audit produced four real questions against hakiri and could not file them. This brings the manifest that provisions every other install into line.

Why it survived review

Neither cause is visible in a diff:

  • contents: read reads as a virtuous least-privilege default. It is the shape of a correct decision; nothing about it announces that the whole Act path is downstream of it.
  • It could not turn anything red. A failed PR write is deliberately never fatal, so the run completed green with its output discarded. A write the App can never do, on a path that never reports failure, is silence indistinguishable from a quiet week.

That is the same shape as an unset config key resolving an empty estate — the third time a degrading read/write has bitten this repo.

members: read is deliberately not added

The live App carries it; the manifest never has. The only membership check here is deploy-authz, which reads its groups from Cloudflare Access's GitHub IdP via get-identity — never from an App token. Propagating it would make every new install request a permission nothing uses. It should probably come off the live install too, but revoking a grant on a live App is its own change.

One gap this leaves open

github-manifest-parity.test.ts asserts the two committed copies match each other, and never compares either to the live App. So App-vs-manifest drift is silent in both directions — which is exactly how the live install and the manifest came to disagree about two permissions with every check green. Closing that needs a credentialed check outside unit tests; not attempted here.

Test

github-manifest-parity.test.ts + github.test.ts — 23 passed.

Both manifest copies declared `contents: read` and no `issues` at all, so a
fresh install of this App could not open a pull request or file an issue —
which is to say it could not produce the one artifact every proposing run
exists to produce.

`pull_requests: write` only covers opening a PR once a branch exists. Creating
the branch and committing the files is `contents`, and at `read` every
`openDraftPullRequest` returns 403. The live `fractalboxdev` install was raised
to `contents: write` + `issues: write` on 2026-08-16 after `org-spec-audit`
produced four real questions and could not file them; this brings the manifest
that provisions every OTHER install into line with what the code does.

Two things made this survive review for so long, and both are worth stating
because neither is visible in a diff:

`contents: read` reads as a virtuous least-privilege default. It is the shape
of a correct decision. Nothing about it announces that the repo's entire Act
path is downstream of it.

And the failure could not turn anything red. A failed PR write is deliberately
never fatal — a notice or proposal that cannot be delivered must not fail a run
that already did its work — so the run completed green with its output
discarded. A write the App can never perform, on a path that never reports
failure, produces silence indistinguishable from a quiet week. That is the
same shape as an unset config key resolving an empty estate, and it is the
third time this repo has been bitten by a read or write that degrades instead
of failing.

`members: read` is deliberately NOT added. The live App carries it, but the
only membership check in this codebase — `deploy-authz` — reads its groups
from Cloudflare Access's GitHub IdP via `get-identity`, never from an App
token. Propagating it would make every new install request a permission
nothing uses. It should probably come off the live App too, but revoking a
grant on a live install is its own change.

Worth knowing for next time: the parity test asserts the two committed copies
match EACH OTHER and never compares either to the live App, so App-vs-manifest
drift is silent in both directions. That is how the live install and the
manifest disagreed about two permissions with every check green.

@flaredispatch-fractalboxdev flaredispatch-fractalboxdev Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI code review — 💬 Comment

Risk tier: full · 0 critical · 2 warnings · 0 suggestions

Reviewers: security ⚠️ · performance ⚠️ · code-quality ⚠️ · documentation ⚠️ · release-management ⚠️ · compliance 2 · agents-md ⚠️

1. ⚠️ Warning — GitHub App requests broad repository write permissions

📍 apps/dispatcher/src/routes/github.ts:88-101

Changing the default installation permissions to 'contents: write' and 'issues: write' grants every installation write access to repository files and issue data. These are high-impact privileges; confirm the app is restricted to repositories where it is trusted, and document or enforce the intended scope rather than broadly expanding the default permission set.

2. ⚠️ Warning — Manifest-wide write access increases installation blast radius

📍 infra/github-app-manifest.json:9-15

The production manifest now requests both 'contents: write' and 'issues: write', so all existing and new installations must grant broad repository mutation capabilities. Validate that this permission escalation has been approved and that deployments handle existing installations requiring permission re-authorization; otherwise the app may fail closed or receive more access than compliance policy permits.

📋 View full logs & reviewed diff ↗

@debuggingfuture
debuggingfuture merged commit 8dd6a8f into main Aug 22, 2026
5 checks passed
@debuggingfuture
debuggingfuture deleted the manifest-grants-match-the-writes branch August 22, 2026 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant