Skip to content

fix(api): require real GitHub write access for BYOK key writes (closes #674) - #682

Merged
JSONbored merged 1 commit into
mainfrom
fix/byok-key-write-permission
Jun 14, 2026
Merged

fix(api): require real GitHub write access for BYOK key writes (closes #674)#682
JSONbored merged 1 commit into
mainfrom
fix/byok-key-write-permission

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

The final BYOK security-audit follow-up — the one I deferred from #680, now done via real push-permission resolution.

Problem

The secret-bearing BYOK key writes (POST/DELETE /v1/repos/:owner/:repo/ai-key) were gated only by the maintainer scope, which admits a caller who authored a PR with author_association of OWNER/MEMBER/COLLABORATOR. So an org MEMBER (mere org membership, not repo write) or a read-only COLLABORATOR who opened a single PR could set/replace/delete a repo's provider key.

Fix

New requireRepoKeyWriteAccess gate on the key write routes: on top of the maintainer gate, a session caller must have real GitHub write access (admin/maintain/write) to the repo, resolved via the installation (getRepositoryCollaboratorPermission) rather than inferred from a PR association. Operators and server-to-server tokens are exempt. It fails closed (403 insufficient_repo_permission) when write access can't be verified.

Tests

  • repo admin → 200
  • read-only collaborator (in scope via a PR) → 403 insufficient_repo_permission (POST + DELETE); GET status still readable
  • operator → 200 without a push check
  • repo with no installation → 403

Verification

typecheck ✅ · test:coverage ✅ (97.01% branch, 1680 tests) · test:workers ✅ · git diff --check

Closes #674. Part of #525.

…#674)

The secret-bearing BYOK key writes (POST/DELETE /v1/repos/:owner/:repo/ai-key)
were gated only by the maintainer scope, which admits a caller who authored a PR
with author_association OWNER/MEMBER/COLLABORATOR — i.e. an org MEMBER (mere org
membership) or a read-only COLLABORATOR could set/replace/delete a repo's
provider key.

Add requireRepoKeyWriteAccess: on top of the maintainer gate, a session caller
must have real GitHub write access (admin/maintain/write) to the repo, resolved
via the installation (getRepositoryCollaboratorPermission) rather than inferred
from a PR association. Operators and server-to-server tokens are exempt; it fails
closed (403 insufficient_repo_permission) when write access can't be verified.
The non-secret ai-review config (PUT /ai-review) and the read-only GET /ai-key
status keep the existing maintainer gate.

This is the deferred follow-up from the BYOK security audit — done now via real
push-permission resolution so it does not lock out legitimate org maintainers
(who surface no OWNER author_association on org repos).

Tests: admin → 200; read-only collaborator (in scope via a PR) → 403; operator
bypasses the push check; no-installation → 403; GET status still readable.

Closes #674. Part of #525.
@ghost

ghost commented Jun 14, 2026

Copy link
Copy Markdown

Note

Gittensory Gate skipped

PR closed before full evaluation. No late first comment was created.

Signal Result Evidence Action
Gate result ⚠️ Skipped #682 is no longer open. No action.

💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

@ghost ghost added the gittensory:reviewed label Jun 14, 2026
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@ghost

ghost commented Jun 14, 2026

Copy link
Copy Markdown

gittensory · advisory review

Reviewed 2 changed file(s) — two independent AI reviewers.

Suggested action:Safe to merge — both reviewers found no blocking issues.

Reviewer A · gpt-oss-120b — recommends ✅ merge
The PR introduces a stricter permission check for BYOK key write operations, requiring real GitHub write access, and adds comprehensive tests covering owner, read‑only collaborator, operator, and edge cases. The implementation is clean, respects existing auth flow, and does not introduce public‑facing forbidden terms.

Suggestions

  • Consider adding a comment documenting the expected GitHub permission strings (e.g., "admin", "maintain", "write") for future maintainers.
  • Ensure the new REPO_KEY_WRITE_PERMISSIONS set is kept in sync if GitHub adds new permission levels.

Worth double-checking

  • If getRepositoryCollaboratorPermission changes its return type or values, the gate may misclassify permissions.
  • Repositories without an installation ID now return 403; ensure this aligns with intended UX.

Reviewer B · nemotron-3-120b-a12b — recommends ✅ merge
The PR enhances security for BYOK key writes by requiring real GitHub write access (via installation-based permission checks) for session users, while exempting operators and server-to-server tokens. It replaces the previous maintainer-only gate with a stricter check for POST/DELETE /ai-key endpoints. The changes are well-tested with new unit cases covering various permission scenarios.

Worth double-checking

  • Verify that getRepositoryCollaboratorPermission (unused in this diff but called) has proper SSRF guards as per project context.
  • Ensure no other endpoints modify BYOK keys that might require similar gating (only POST/DELETE /ai-key changed).

@ghost ghost added the gittensory-review label Jun 14, 2026
@JSONbored
JSONbored merged commit 36087c0 into main Jun 14, 2026
10 checks passed
@JSONbored
JSONbored deleted the fix/byok-key-write-permission branch June 14, 2026 03:48
@github-project-automation github-project-automation Bot moved this from Todo to Done in gittensory - v1 roadmap Jun 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

BYOK: tighten ai-key writes to repo owners (not org MEMBER / read COLLABORATOR)

1 participant