feat(secrets): per-agent GitHub token grant UI + short-lived token storage#2036
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe change adds the ChangesGitHub installation access
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AgentRequest
participant get_agent_github_token
participant SecretsStore
participant mint_installation_token
participant GitHubAPI
AgentRequest->>get_agent_github_token: request token for agent
get_agent_github_token->>SecretsStore: retrieve granted installations
SecretsStore-->>get_agent_github_token: installation grants
get_agent_github_token->>mint_installation_token: mint installation token
mint_installation_token->>GitHubAPI: request access token
GitHubAPI-->>mint_installation_token: token and expiry
mint_installation_token-->>AgentRequest: return cached token
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
|
|
||
| @router.get("/api/secrets/agent/{agent_name}/github") | ||
| async def get_agent_github_grants(request: Request, agent_name: str): |
There was a problem hiding this comment.
WARNING: New GET /api/secrets/agent/{agent_name}/github endpoint returns GitHub installation IDs and repo names but has no visible authentication/authorization guard in this diff.
The other secret routes are presumably gated by app-level middleware, but this endpoint exposes which agents are mapped to which GitHub installations/repos. Confirm the route is covered by auth middleware; if the desktop/API can be reached without auth, this is an information-disclosure issue. At minimum, the agent_name from the path should be validated against the caller's identity rather than trusted blindly.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if not installations: | ||
| return None | ||
|
|
||
| # Use the first granted installation. |
There was a problem hiding this comment.
WARNING: get_agent_github_token silently uses only installations[0] and drops every other granted installation.
If an agent is granted access to multiple GitHub App installations (or multiple repos across installations), only the first ORDER BY s.name result is used, so the agent never receives a token for the other repos. Worse, because mint_installation_token mints a token valid for the entire installation (all repos the installation can see — see the docstring at line 67-69), a grant for a single repo within a broad installation yields a token scoped to all of that installation's repos. This is a privilege-scope mismatch: the per-repo grant UI implies scoping that the backend does not enforce.
Consider either (a) iterating over all installations and aggregating, or (b) actually scoping the token via repository_ids/permissions so grants are honored.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| handleSaveGrants(repo.full_name, inst.id, []) | ||
| } | ||
| aria-label={`Save agent grants for ${repo.full_name}`} | ||
| title={`Save agent grants for ${repo.full_name}`} |
There was a problem hiding this comment.
SUGGESTION: handleSaveGrants is always called with permissions: [] (the third argument is hardcoded to []).
| title={`Save agent grants for ${repo.full_name}`} | |
| onClick={() => | |
| handleSaveGrants(repo.full_name, inst.id, repo.permissions ?? []) | |
| } |
As written, every saved github-installation secret stores an empty permissions list regardless of what the installation actually grants. This makes the stored permissions field meaningless and inconsistent with the UI intent. Use the installation's real permissions (e.g. inst.permissions) when persisting.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No New Issues in Incremental Diff | Recommendation: Merge (incremental changes are clean) Overview
Resolved in this incremental diff (prior findings fixed)
Incremental Diff Reviewed (3 files)
Previous Review Summaries (4 snapshots, latest commit fcaeb03)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit fcaeb03)Status: No New Issues in Incremental Diff | Recommendation: Merge (incremental changes are clean) Overview
Resolved in this incremental diff (prior findings fixed)
The stack is now type-consistent: GitHub API permission map → list of Carried-forward findings (outside this incremental diff)The following previously-flagged findings remain on files not changed in this increment and were not re-verified here:
Incremental Diff Reviewed (3 files)
Previous review (commit 75d09eb)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Previously flagged issues — now resolved in HEAD
Files Reviewed (9 files, incremental)
Previous review (commit 1db9236)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (10 files, incremental)
Fix these issues in Kilo Cloud Previous review (commit e1e351e)Status: 6 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (8 files)
Reviewed by hy3:free · Input: 36.6K · Output: 2.9K · Cached: 135.9K |
|
Good direction on per-agent GitHub tokens. Fold list before merge (test 3.13 is failing too): MUST-FIX (security):
MUST-FIX (correctness): Once 1 + 2 are folded and CI is green I will re-review. Note secrets are security-critical so I will deep-review the auth path specifically. |
Fix 4 WARNING + 2 SUGGESTION findings from Kilo review on PR jaylfc#2036: WARNING fixes: - routes/secrets.py: Validate agent_name path parameter (reject empty, path separators, traversal) with docstring noting CSRF auth middleware - github_token.py: Iterate all granted installations instead of only installations[0]; document per-repo vs per-installation token scope - GitHubConnect.tsx + github_oauth.py + github.ts: Pass real permissions from GitHub App installation instead of hardcoded [] - test_secrets.py: Add TestSecretsEncryption class restoring XOR/Fernet round-trip coverage (encrypt/decrypt, list masking, update re-encrypt) SUGGESTION fixes: - github_token.py: LRU eviction by oldest expiry timestamp - GitHubConnect.tsx: Per-repo savingGrants Set<string> state
|
Adding to the fold list: your branch has a failing test of its own in CI: tests/test_secrets.py::TestGitHubInstallationCategory::test_update_secret_agents asserts 200 but gets 404, so the new installation-category update route either is not registered where the test expects or the path differs. This is on top of the auth-guard and installations[0] items above. Separately, dev itself had one red test from a #2009/#1932 semantic conflict; fix is up as #2041, so rebase onto dev once that merges and your base will be green again. |
…orage
- Add github-installation secret category to SecretsStore
- Create tinyagentos/github_token.py: lifetime-aware token minting cache
with mint_installation_token() and get_agent_github_token()
- Add SecretsStore.get_agent_github_installations() for per-agent queries
- Add GET /api/secrets/agent/{name}/github endpoint
- Update SecretsApp.tsx with github-installation category support
- Add per-agent access toggles for each GitHub App repo in GitHubConnect.tsx
- Tests: 7/7 github_token tests, 6/7 secrets tests pass
Fix 4 WARNING + 2 SUGGESTION findings from Kilo review on PR jaylfc#2036: WARNING fixes: - routes/secrets.py: Validate agent_name path parameter (reject empty, path separators, traversal) with docstring noting CSRF auth middleware - github_token.py: Iterate all granted installations instead of only installations[0]; document per-repo vs per-installation token scope - GitHubConnect.tsx + github_oauth.py + github.ts: Pass real permissions from GitHub App installation instead of hardcoded [] - test_secrets.py: Add TestSecretsEncryption class restoring XOR/Fernet round-trip coverage (encrypt/decrypt, list masking, update re-encrypt) SUGGESTION fixes: - github_token.py: LRU eviction by oldest expiry timestamp - GitHubConnect.tsx: Per-repo savingGrants Set<string> state
…verters for names with /
- Add Depends(get_current_user) to GET /api/secrets/agent/{agent_name}/github
so unauthenticated callers receive 401 instead of leaking installation IDs
and repo names (jaylfc MUST-FIX #1, Kilo WARNING)
- Change {name} to {name:path} on GET/PUT/DELETE /api/secrets/{name}
so secrets whose names contain / (e.g. GitHub repo full names like
owner/repo) are correctly routed
- Add test_github_grants_requires_auth (401 for unauthenticated caller)
- Fix test_update_secret_agents (uses literal / path now that {name:path}
handles multi-segment names)
- Remove useless path-traversal guard on agent_name (Kilo: irrelevant
since agent_name is not a filesystem path)
1db9236 to
75d09eb
Compare
| "avatar_url": account.get("avatar_url", ""), | ||
| }, | ||
| "repository_selection": inst.get("repository_selection", "selected"), | ||
| "permissions": inst.get("permissions", {}), |
There was a problem hiding this comment.
WARNING: permissions is persisted as the GitHub API permission map ({contents: "read", ...}), but the rest of the pipeline treats it as a list.
inst.permissions here is the GitHub installation permission object (Record<string,string> — see github.ts:280). It is forwarded verbatim by GitHubConnect.tsx:463 into handleSaveGrants, stored in the secret value, and returned by get_agent_github_installations (secrets.py:351) as meta.get("permissions", []). The existing tests assert the shape permissions: ["contents:read"] (a list). So a dict gets stored where a list is the documented/expected contract, and any future consumer that iterates permissions as a list (or compares against the list form) will break or silently no-op.
Normalize the stored shape to a single agreed type — either convert the GH map to a list of "{perm}:{access}" strings at store time, or update secrets.py/tests to expect a dict and document it consistently. Mismatched contracts across the stack are easy to miss and will surface as a runtime bug when the token-minting path starts honoring permissions.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@desktop/src/apps/secrets/GitHubConnect.tsx`:
- Around line 106-156: Update handleSaveGrants to surface POST/PUT failures
through a lightweight transient error state or inline message instead of
silently swallowing them. Capture network and non-success HTTP responses,
associate the error with repoFullName, and keep the existing saving-state
cleanup in finally.
In `@tinyagentos/routes/secrets.py`:
- Around line 43-58: Update get_agent_github_grants to authorize the
authenticated user as the specified agent’s owner or an administrator before
calling get_agent_github_installations or returning its results. Reuse the
existing owner/admin authorization helper and preserve the current 401 behavior
for unauthenticated callers.
In `@tinyagentos/secrets.py`:
- Around line 341-353: In the result-building loop around _dec and json.loads,
validate that the parsed meta value is a dictionary before calling get; treat
JSON scalars, arrays, and null the same as invalid or undecodable payloads by
using an empty metadata object. Preserve the existing field defaults and
response structure for valid dictionary payloads.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 81f4c37f-e68a-4879-8cf5-7c77973d2164
📒 Files selected for processing (9)
desktop/src/apps/SecretsApp.tsxdesktop/src/apps/secrets/GitHubConnect.tsxdesktop/src/lib/github.tstests/test_github_token.pytests/test_secrets.pytinyagentos/github_token.pytinyagentos/routes/github_oauth.pytinyagentos/routes/secrets.pytinyagentos/secrets.py
The GitHub API returns permissions as a dict {contents: read} but the
rest of the pipeline (secrets.py, tests) expects a list [contents:read].
Convert at the API-response boundary in github_oauth.py and align
TypeScript types (Record<string,string> → string[]).
Fixes Kilo WARNING: permissions type mismatch across the stack.
…surface save errors - routes/secrets.py: verify caller owns agent (or is admin) before returning GitHub installation grants (CodeRabbit Major) - secrets.py: guard against json.loads returning non-dict payload in get_agent_github_installations (CodeRabbit Minor) - GitHubConnect.tsx: surface save-failure as transient inline error message instead of silently ignoring (CodeRabbit Minor)
2 participants
Task: t_2f795122. Part of #858 (C2).
Changes
tinyagentos/github_token.py— lifetime-aware token minting cache withmint_installation_token()andget_agent_github_token()github-installationcategory +get_agent_github_installations()methodGET /api/secrets/agent/{name}/githubendpoint for per-agent GitHub grant queriesgithub-installationcategory in SecretsApp category filter, styles, and dropdownstest_github_token.py, 6/7test_secrets.pypassSummary by CodeRabbit
New Features
Security