fix(api): close admin auth bypass, scope runner tokens, gate repo access - #60
Merged
Conversation
Security fixes from the backend audit: - admin: ENVIRONMENT defaults to "development", where the login accepted ANY password — a deploy that forgets the variable exposed user rows and BYOK ciphertext at /admin. The bypass is gone, comparison is constant-time, and the panel refuses to mount without ADMIN_PASSWORD. BYOK/user credential columns are now excluded from the edit forms, not just the detail view. - containers: the runner received an installation token scoped to every repo and every App permission, injected as an env var into a container running Claude Code over attacker-controllable PR content. It is now narrowed to the repo under review with read-only scopes; the API keeps minting its own unscoped token for posting comments. - containers: session creation only checked installation membership, so any org member could start a session on any repo the installation covered and stream its diff back. verify_repo_access now asks GitHub with the caller's own token. - installations: the two read-only routes required org-admin, so ordinary members saw an installation in the list and got 403 opening it. They now use member-level access; admin stays on BYOK and settings. - config: GITHUB_APP_PRIVATE_KEY accepts base64-encoded PEM, which every deployment guide already told operators to use — .env cannot hold multi-line values. Raw PEM keeps working. - alembic: drop the six pre-pivot comprehension tables. Migrations built a 12-table schema while tests built 6, and the next autogenerate would have emitted these drops itself. - routers: replace `user=Depends(get_current_user)` with a `CurrentUser` Annotated alias — removes 19 noqa: B008 and gives `user` a real type. RUF100 is enabled so dead noqa cannot accumulate again. - tests: hand-written GitHubDouble (httpx.MockTransport, routed by URL) replaces unittest.mock patching in four files; new tests cover admin login, PEM normalization, repo access and token scoping. 282 -> 308 tests, coverage 77% -> 79%, ruff and mypy clean.
|
helPRs session created for this PR. Skill: |
mariuspruvot
added a commit
that referenced
this pull request
Aug 1, 2026
FERNET_KEY encrypts the GitHub and Claude credentials in the database, and replacing it made every one of them unreadable — so in practice it could not be replaced, which is a bad property for a key whose exposure is the whole reason it exists. The crypto helpers now take an ordered keyset and go through MultiFernet: encrypt with the first key, decrypt with whichever one matches. A new FERNET_KEY_FALLBACKS setting holds retired keys, so a rotation is a deploy rather than an outage. Fallbacks are validated at boot alongside the primary key. A malformed retired key must not surface at first decrypt — the row that would break is exactly the credential the rotation is trying not to lose. `helprs.scripts.rotate_credentials` finishes the job by re-encrypting stored rows under the primary key, using MultiFernet.rotate (no plaintext needed). Without it the fallback list grows forever and a "retired" key stays as sensitive as the live one. It rewrites in a single transaction, is safe to re-run, and a row no configured key can read is reported and left intact rather than overwritten. The keyset is typed `list[str]` rather than `Sequence[str]` deliberately: `str` satisfies `Sequence[str]`, so a caller still passing a single key would have type-checked cleanly. mypy caught all six call sites this way. `settings.fernet_keys` is a plain property, not a computed_field — the latter would put every key back into model_dump() output and undo the SecretStr work from #65. Docs: a rotation runbook in self-hosting.md, the new setting in .env.example, and the stale "in development mode, any password is accepted" line next to ADMIN_PASSWORD removed — that bypass was fixed in #60.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Security and correctness fixes from the backend audit. Each item was verified against the code before changing it.
Security
Admin panel was unauthenticated by default.
ENVIRONMENTdefaults to"development", and the login accepted any password in that mode — so a self-hoster who never set the variable exposed GitHub users, installations and BYOK ciphertext at/admin. The bypass is removed, comparison is constant-time, and the panel refuses to mount whenADMIN_PASSWORDis unset. Writing a test for it caught a second bug in my own fix:compare_digest("", "")isTrue, so an unset password would have authenticated an empty field.Runner containers got an over-privileged GitHub token.
mint_installation_tokensent norepositories/permissions, so the token carried every permission the App holds on every repo — injected as a plain env var into a container runningclaude --dangerously-skip-permissionsover attacker-controllable PR content with open egress. It is now narrowed to the repo under review withcontents/metadata/pull_requests: read. The API still mints its own unscoped token when it needs to write.No per-repo authorization.
create_container_sessionchecked installation membership only, neverrepo_full_name— any org member could start a session on any repo the installation covered and stream its diff back, bypassing GitHub's per-repo permissions.verify_repo_accessnow asks GitHub with the caller's own token.Credential columns were editable in SQLAdmin.
column_details_exclude_listonly hides fields on the detail page; the edit form rendered the Fernet ciphertext in a writable input. Addedform_excluded_columnson both views andcan_edit = Falseon BYOK.Correctness
.envcannot hold multi-line values) but no decoding existed — following the docs broke all GitHub App auth. Raw PEM keeps working.alembic upgrade headbuilt a 12-table schema whilecreate_allbuilt 6, so tests never ran against the deployed schema, and the next--autogeneratewould have emitted these drops as a surprise destructive diff.Cleanup
CurrentUser = Annotated[GitHubUser, Depends(get_current_user)]replacesuser=Depends(...)on every route: 19noqa: B008gone anduserhas a real type instead of implicitAny. Same for the webhook body.RUF100enabled so dead noqa cannot accumulate. Note for future audits:ruff check --select RUF100on the command line replaces the rule selection, which makes every noqa look dead — that trap nearly cost us the B008 suppressions.unittest.mockonto a hand-writtenGitHubDouble(httpx.MockTransport, routed by URL) that records requests, so tests can assert on what was actually sent.Verification
ruff check+ruff format --checkclean on src/tests/alembic,mypyclean