feat(lease): add GitHub App installation token lease backend#342
Conversation
Adds a new `github-app` lease backend that creates short-lived GitHub installation access tokens from a GitHub App's private key. Supports scoping tokens to specific permissions and repositories, custom env var names, and GitHub Enterprise via configurable api_base. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new github-app lease backend, which is a valuable addition. While the implementation is well-structured and includes comprehensive tests, a critical security vulnerability has been identified: the revoke_lease method is currently a no-op. This means that tokens are not invalidated on GitHub's side when a lease is revoked, leaving a window of risk for compromised tokens until they naturally expire (up to 1 hour). It is strongly recommended to implement proper revocation logic using GitHub's API. Furthermore, potential panics due to unchecked unwrap() calls should be addressed for robustness, and there's an opportunity to improve performance and code design by reducing cloning in the backend creation logic.
| if let Some(ref permissions) = self.permissions { | ||
| body.insert( | ||
| "permissions".to_string(), | ||
| serde_json::to_value(permissions).unwrap(), | ||
| ); | ||
| } | ||
| if let Some(ref repositories) = self.repositories { | ||
| body.insert( | ||
| "repositories".to_string(), | ||
| serde_json::to_value(repositories).unwrap(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
The use of .unwrap() on the result of serde_json::to_value could lead to a panic if serialization fails for any reason. While it's unlikely for these types, it's safer to handle the Result gracefully to prevent the application from crashing.
if let Some(ref permissions) = self.permissions {
body.insert(
"permissions".to_string(),
serde_json::to_value(permissions).map_err(|e| FnoxError::ProviderApiError {
provider: "GitHub App".to_string(),
details: format!("Failed to serialize permissions: {e}"),
hint: "This is likely an internal bug.".to_string(),
url: URL.to_string(),
})?,
);
}
if let Some(ref repositories) = self.repositories {
body.insert(
"repositories".to_string(),
serde_json::to_value(repositories).map_err(|e| FnoxError::ProviderApiError {
provider: "GitHub App".to_string(),
details: format!("Failed to serialize repositories: {e}"),
hint: "This is likely an internal bug.".to_string(),
url: URL.to_string(),
})?,
);
}| async fn revoke_lease(&self, lease_id: &str) -> Result<()> { | ||
| // Extract the token from cached credentials to revoke it. | ||
| // GitHub's revoke endpoint requires the token itself, not a lease ID. | ||
| // Since we use a generated lease_id (not the token), we can't revoke | ||
| // without the token value. The token auto-expires in ≤1 hour anyway. | ||
| tracing::debug!( | ||
| lease_id, | ||
| "GitHub App tokens auto-expire; skipping explicit revocation" | ||
| ); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
The revoke_lease implementation for the GitHub App backend is currently a no-op. While the lease is marked as revoked in the local fnox ledger (preventing further use by the tool), the actual installation access token remains valid on GitHub's servers until its natural expiration (which can be up to 1 hour). This means that if a token is compromised and a user attempts to revoke it, the attacker still has a window of opportunity to use the token.
To fix this, you should implement the revocation logic by calling GitHub's DELETE /installation/token endpoint. This requires the lease_id to be the installation token itself (or a way to retrieve it), and the revoke_lease method should use this token to authenticate the revocation request.
| } => Ok(Box::new(github_app::GitHubAppBackend::new( | ||
| app_id.clone(), | ||
| installation_id.clone(), | ||
| private_key_file.clone(), | ||
| env_var.clone(), | ||
| permissions.clone(), | ||
| repositories.clone(), | ||
| api_base.clone(), | ||
| ))), |
There was a problem hiding this comment.
There are multiple .clone() calls here to create the GitHubAppBackend. While this is consistent with other backend instantiations in this file, it's inefficient as it allocates new memory for each cloned value.
A more efficient approach would be to change the create_backend method to take self by value instead of by reference (&self). This would allow moving the configuration values into the GitHubAppBackend::new function without needing to clone them. This would be a broader refactoring but would improve performance and is a better design pattern for this kind of factory function.
Also sort and normalize dependency versions in Cargo.toml. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
bugbot run |
Greptile SummaryThe Strengths:
Known issue:
Core authentication, token lifecycle, and revocation logic are all sound. Confidence Score: 4/5
Last reviewed commit: eed87bd |
GitHub's REST API requires a User-Agent header and rejects requests without one with 403 Forbidden. Set it globally in http_client() so all backends (including the new github-app lease backend) include it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Implement real token revocation via DELETE /installation/token (lease_id is now the token itself, not a random ID) - Fix JWT lifetime: derive exp from iat (not now) to stay within GitHub's 600s limit when iat is backdated for clock drift - Fix iss claim: parse app_id to integer (GitHub requires numeric) - Fix repositories config: use bare names, not owner/repo paths - Make required_env_vars() return empty vec since the env var is optional when private_key_file is configured - Remove dead bash mock server code from test helper Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The lease_id field is never encrypted in the ledger and is visible in lease list output. Previously, the GitHub installation token was stored directly as the lease_id, bypassing credential encryption entirely. Fix: use a blake3 hash of the token as the lease_id (deterministic, non-secret), and extend the LeaseBackend::revoke_lease trait to accept an optional credentials map. The revoke command now decrypts cached credentials before passing them to the backend, so the GitHub App backend can authenticate DELETE /installation/token with the actual token from the encrypted credential store. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| hex = "0.4" | ||
| hkdf = "0.12" | ||
| indexmap = { version = "2", features = ["serde"] } | ||
| jsonwebtoken = { version = "10", features = ["aws_lc_rs"] } |
There was a problem hiding this comment.
Two jsonwebtoken versions now compile into the binary
Adding jsonwebtoken = { version = "10", … } alongside the existing jsonwebtoken 9.3.1 dependency (pulled in by the GCP IAM backend, visible in Cargo.lock) means both crates are compiled and linked. The jsonwebtoken crate links aws-lc-rs, so this doubles a large C-extension dependency.
If upgrading the GCP IAM backend to v10 is feasible, doing so would collapse the duplicate and keep compile times and binary size in check. If not, this is at minimum worth a comment noting the intentional dual-version situation.
…st ports - Return Ok(()) when credentials are unavailable in revoke_lease instead of erroring (expired tokens can't be revoked server-side anyway) - Use OS-assigned ports in tests to avoid conflicts in CI - Fix shfmt formatting Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
- Fix iss claim: JWT RFC 7519 defines iss as StringOrURI, so keep app_id as a string in the claims instead of parsing to u64 - Add fallback expiry of now + 1h when expires_at is missing or unparseable, preventing stale token reuse in the ledger - Add revocation test: mock server now handles DELETE, test creates a lease then revokes it and verifies success Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
## Summary - Adds a new `github-app` lease backend that creates short-lived GitHub installation access tokens from a GitHub App's private key - Supports scoping tokens to specific permissions and repositories - Configurable `env_var` (default: `GITHUB_TOKEN`), `api_base` for GitHub Enterprise - Tokens auto-expire in ≤1 hour (GitHub's hard limit), cached via existing lease ledger ### Example config ```toml [leases.github] type = "github-app" app_id = "12345" installation_id = "67890" private_key_file = "~/.config/fnox/github-app.pem" [leases.github.permissions] contents = "read" pull_requests = "write" repositories = ["my-org/my-repo"] ``` ## Test plan - [x] 9 bats tests covering: token creation (file + env var), `fnox exec` integration, custom env_var, permissions/repositories config, missing key error, lease list - [x] All tests use a mock HTTP server (no real GitHub credentials needed) - [x] Existing cargo + bats tests pass 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Adds new networked auth/crypto code for generating and revoking GitHub tokens and changes the `LeaseBackend::revoke_lease` API across all backends, so regressions could affect lease cleanup/revocation behavior. > > **Overview** > Adds a new **`github-app` lease backend** that creates GitHub App installation access tokens by signing a short-lived JWT with an RSA private key (from `FNOX_GITHUB_APP_PRIVATE_KEY` or `private_key_file`), supports optional permission/repository scoping and custom `api_base`, and generates non-secret `lease_id`s derived from a token hash. > > Updates lease revocation to pass **decrypted cached credentials** into `LeaseBackend::revoke_lease` (trait signature change applied across existing backends) so backends can revoke using the actual credential value; `lease cleanup` now explicitly skips passing creds for expired leases. Also sets a default `User-Agent` on the shared `reqwest` client. > > Extends config schema and docs to document the new backend, adds bats tests with a mock GitHub API server, and updates dependencies (`jsonwebtoken` v10 with `aws_lc_rs`, plus lockfile changes around `untrusted`). > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit eed87bd. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
### 🚀 Features - **(cloudflare)** add Cloudflare API token lease backend by [@jdx](https://github.com/jdx) in [#335](#335) - **(fido2)** bump demand to v2, mask PIN during typing by [@jdx](https://github.com/jdx) in [#334](#334) - **(init)** add -f as short alias for --force by [@jdx](https://github.com/jdx) in [#329](#329) - **(lease)** add --all flag, default to creating all leases by [@jdx](https://github.com/jdx) in [#337](#337) - **(lease)** add GitHub App installation token lease backend by [@jdx](https://github.com/jdx) in [#342](#342) ### 🐛 Bug Fixes - **(config)** fix directory locations to follow XDG spec by [@jdx](https://github.com/jdx) in [#336](#336) - **(exec)** use unix exec and exit silently on subprocess failure by [@jdx](https://github.com/jdx) in [#339](#339) - **(fido2)** remove duplicate touch prompt by [@jdx](https://github.com/jdx) in [#332](#332) - **(set)** write to lowest-priority existing config file by [@jdx](https://github.com/jdx) in [#331](#331) - **(tui)** skip providers requiring interactive auth by [@jdx](https://github.com/jdx) in [#333](#333) ### 🛡️ Security - **(ci)** retry lint step to handle transient pkl fetch failures by [@jdx](https://github.com/jdx) in [#341](#341) - **(mcp)** add MCP server for secret-gated AI agent access by [@jdx](https://github.com/jdx) in [#343](#343) - add guide for fnox sync by [@jdx](https://github.com/jdx) in [#328](#328) ### 🔍 Other Changes - share Rust cache across CI jobs by [@jdx](https://github.com/jdx) in [#340](#340)
Summary
github-applease backend that creates short-lived GitHub installation access tokens from a GitHub App's private keyenv_var(default:GITHUB_TOKEN),api_basefor GitHub EnterpriseExample config
Test plan
fnox execintegration, custom env_var, permissions/repositories config, missing key error, lease list🤖 Generated with Claude Code
Note
Medium Risk
Adds new networked auth/crypto code for generating and revoking GitHub tokens and changes the
LeaseBackend::revoke_leaseAPI across all backends, so regressions could affect lease cleanup/revocation behavior.Overview
Adds a new
github-applease backend that creates GitHub App installation access tokens by signing a short-lived JWT with an RSA private key (fromFNOX_GITHUB_APP_PRIVATE_KEYorprivate_key_file), supports optional permission/repository scoping and customapi_base, and generates non-secretlease_ids derived from a token hash.Updates lease revocation to pass decrypted cached credentials into
LeaseBackend::revoke_lease(trait signature change applied across existing backends) so backends can revoke using the actual credential value;lease cleanupnow explicitly skips passing creds for expired leases. Also sets a defaultUser-Agenton the sharedreqwestclient.Extends config schema and docs to document the new backend, adds bats tests with a mock GitHub API server, and updates dependencies (
jsonwebtokenv10 withaws_lc_rs, plus lockfile changes arounduntrusted).Written by Cursor Bugbot for commit eed87bd. This will update automatically on new commits. Configure here.