mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-09-06 08:18:58 +00:00
Detect GitHub App visibility mismatch during repo init (#99)
This PR detects when a GitHub App's visibility will prevent installation
on a cross-owner repository during `fabro repo init`. Previously, when
the app wasn't installed, users saw only a generic "install at" URL with
no indication of why the install link might not work—particularly
confusing when the repo belongs to a different owner than the app and
the app is private.
Two new functions are added to `fabro-github`: `get_authenticated_app()`
fetches the app's metadata (slug and owner) via the authenticated `GET
/app` endpoint, and `is_app_public()` probes `GET /apps/{slug}` without
authentication to determine visibility (public apps return 200, private
ones return 404). In `init.rs`, when the app is not installed, we now
compare the app owner against the repo owner and, if they differ and the
app is private, display a targeted warning explaining that the app must
be made public along with a direct link to the settings page. All new
checks are best-effort—failures are silently ignored so the existing
flow is unaffected.
The PR also introduces a `GITHUB_API_BASE_URL` constant to replace
hardcoded URL strings and adds five unit tests covering the new
functions: successful app info retrieval, auth failure handling,
public/private app detection, and verification that the visibility check
sends no `Authorization` header.
### Fabro Details
<details>
<summary>Ran 10 stages in 18m 26s for $4.40</summary>
| Stage | Duration | Cost | Retries |
|---|---|---|---|
| start | 0s | – | 0 |
| toolchain | 0s | – | 0 |
| preflight_compile | 1m 13s | – | 0 |
| preflight_lint | 13s | – | 0 |
| implement | 3m 23s | $0.94 | 0 |
| simplify_opus | 5m 17s | $1.44 | 0 |
| simplify_gemini | 2m 52s | $0.92 | 0 |
| simplify_gpt | 3m 24s | $1.10 | 0 |
| verify | 1m 25s | – | 0 |
| fmt | 1s | – | 0 |
| **Total** | **18m 26s** | **$4.40** | **0** |
</details>
<details>
<summary>Ran <code>ImplementAndSimplify.fabro</code> (13 nodes and 16
edges)</summary>
```dot
digraph ImplementAndSimplify {
graph [
goal="Implement and simplify",
model_stylesheet="
* { backend: api; model: claude-opus-4-6;}
"
]
rankdir=LR
start [shape=Mdiamond, label="Start"]
exit [shape=Msquare, label="Exit"]
toolchain [label="Toolchain", shape=parallelogram, script="command -v cargo >/dev/null || { curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y && sudo ln -sf $HOME/.cargo/bin/* /usr/local/bin/; }; cargo --version 2>&1", max_retries=0]
preflight_compile [label="Preflight Compile", shape=parallelogram, script="cargo check -q --workspace 2>&1", max_retries=0]
preflight_lint [label="Preflight Lint", shape=parallelogram, script="cargo clippy -q --workspace -- -D warnings 2>&1", max_retries=0]
fix_lints [label="Fix Lints", prompt="The preflight lint step failed. Read the build output from context and fix all clippy lint warnings.", max_visits=3]
implement [label="Implement", prompt="Read the plan file referenced in the goal and implement every step. Make all the code changes described in the plan. Use red/green TDD."]
simplify_opus [label="Simplify (Opus)", prompt="@prompts/simplify.md"]
simplify_gemini [label="Simplify (Gemini)", prompt="@prompts/simplify.md", model="gemini-3.1-pro-preview-customtools"]
simplify_gpt [label="Simplify (GPT-54)", prompt="@prompts/simplify.md", model="gpt-54"]
verify [label="Verify", shape=parallelogram, script="cargo clippy -q --workspace -- -D warnings 2>&1 && cargo nextest run --cargo-quiet --workspace --status-level fail 2>&1", goal_gate=true, retry_target="fixup"]
fixup [label="Fixup", prompt="The verify step failed. Read the build output from context and fix all clippy lint warnings and test failures.", max_visits=3]
fmt [label="Format", shape=parallelogram, script="cargo fmt --all 2>&1", max_retries=0]
start -> toolchain
toolchain -> preflight_compile [condition="outcome=success"]
toolchain -> exit
preflight_compile -> preflight_lint [condition="outcome=success"]
preflight_compile -> exit
preflight_lint -> implement [condition="outcome=success"]
preflight_lint -> fix_lints
fix_lints -> preflight_lint
implement -> simplify_opus -> simplify_gemini -> simplify_gpt -> verify
verify -> fmt [condition="outcome=success"]
verify -> fixup
fixup -> verify
fmt -> exit
}
```
</details>
⚒️ Generated with [Fabro](https://fabro.sh)
---------
Co-authored-by: Fabro <noreply@fabro.sh>
This commit is contained in:
parent
e658f9705e
commit
d5bbc12261
2 changed files with 220 additions and 3 deletions
|
|
@ -237,8 +237,14 @@ async fn check_github_app_installation() {
|
|||
|
||||
let client = reqwest::Client::new();
|
||||
|
||||
match fabro_github::check_app_installed(&client, &jwt, &owner, &repo, "https://api.github.com")
|
||||
.await
|
||||
match fabro_github::check_app_installed(
|
||||
&client,
|
||||
&jwt,
|
||||
&owner,
|
||||
&repo,
|
||||
fabro_github::GITHUB_API_BASE_URL,
|
||||
)
|
||||
.await
|
||||
{
|
||||
Ok(true) => {
|
||||
let green = console::Style::new().green();
|
||||
|
|
@ -254,6 +260,42 @@ async fn check_github_app_installation() {
|
|||
};
|
||||
|
||||
let yellow = console::Style::new().yellow();
|
||||
|
||||
// Best-effort: warn if the app is private and the repo belongs to a different owner.
|
||||
if let Ok(app_info) = fabro_github::get_authenticated_app(
|
||||
&client,
|
||||
&jwt,
|
||||
fabro_github::GITHUB_API_BASE_URL,
|
||||
)
|
||||
.await
|
||||
{
|
||||
let cross_owner = !app_info.owner.login.eq_ignore_ascii_case(&owner);
|
||||
let is_private = cross_owner
|
||||
&& fabro_github::is_app_public(
|
||||
&client,
|
||||
&app_info.slug,
|
||||
fabro_github::GITHUB_API_BASE_URL,
|
||||
)
|
||||
.await
|
||||
== Ok(false);
|
||||
|
||||
if is_private {
|
||||
eprintln!(
|
||||
"\n {} GitHub App \"{}\" is private but this repo belongs to a different owner ({}).",
|
||||
yellow.apply_to("!"),
|
||||
app_info.slug,
|
||||
owner
|
||||
);
|
||||
eprintln!(
|
||||
" The app must be made public before it can be installed outside {}.",
|
||||
app_info.owner.login
|
||||
);
|
||||
eprintln!(
|
||||
" Update visibility at: https://github.com/settings/apps/{}",
|
||||
app_info.slug
|
||||
);
|
||||
}
|
||||
}
|
||||
eprintln!(
|
||||
"\n {} GitHub App is not installed for {owner}/{repo}",
|
||||
yellow.apply_to("!")
|
||||
|
|
@ -275,7 +317,7 @@ async fn check_github_app_installation() {
|
|||
&jwt,
|
||||
&owner,
|
||||
&repo,
|
||||
"https://api.github.com",
|
||||
fabro_github::GITHUB_API_BASE_URL,
|
||||
)
|
||||
.await
|
||||
{
|
||||
|
|
|
|||
|
|
@ -37,6 +37,19 @@ pub struct PullRequestRef {
|
|||
pub ref_name: String,
|
||||
}
|
||||
|
||||
/// Owner information for a GitHub App.
|
||||
#[derive(Debug, Clone, Deserialize)]
|
||||
pub struct AppOwner {
|
||||
pub login: String,
|
||||
}
|
||||
|
||||
/// Information about a GitHub App from the authenticated `/app` endpoint.
|
||||
#[derive(Debug, Clone, Deserialize)]
|
||||
pub struct AppInfo {
|
||||
pub slug: String,
|
||||
pub owner: AppOwner,
|
||||
}
|
||||
|
||||
/// Credentials for authenticating as a GitHub App.
|
||||
#[derive(Clone, Debug)]
|
||||
pub struct GitHubAppCredentials {
|
||||
|
|
@ -524,6 +537,70 @@ pub async fn check_app_installed(
|
|||
}
|
||||
}
|
||||
|
||||
/// Fetch information about the authenticated GitHub App.
|
||||
///
|
||||
/// Uses the App JWT to call `GET /app` and returns the app's slug and owner.
|
||||
pub async fn get_authenticated_app(
|
||||
client: &reqwest::Client,
|
||||
jwt: &str,
|
||||
base_url: &str,
|
||||
) -> Result<AppInfo, String> {
|
||||
let url = format!("{base_url}/app");
|
||||
let resp = client
|
||||
.get(&url)
|
||||
.header("Authorization", format!("Bearer {jwt}"))
|
||||
.header("Accept", "application/vnd.github+json")
|
||||
.header("User-Agent", "fabro")
|
||||
.send()
|
||||
.await
|
||||
.map_err(|e| format!("Failed to fetch GitHub App info: {e}"))?;
|
||||
|
||||
match resp.status().as_u16() {
|
||||
200 => {}
|
||||
401 => {
|
||||
return Err("GitHub App authentication failed. \
|
||||
Check that app_id and GITHUB_APP_PRIVATE_KEY are correct."
|
||||
.to_string())
|
||||
}
|
||||
status => {
|
||||
return Err(format!(
|
||||
"Unexpected status {status} fetching GitHub App info"
|
||||
))
|
||||
}
|
||||
}
|
||||
|
||||
resp.json::<AppInfo>()
|
||||
.await
|
||||
.map_err(|e| format!("Failed to parse GitHub App info: {e}"))
|
||||
}
|
||||
|
||||
/// Check whether a GitHub App is publicly visible.
|
||||
///
|
||||
/// Calls `GET /apps/{slug}` **without** authentication. Public apps return 200,
|
||||
/// private apps return 404 to unauthenticated requests.
|
||||
pub async fn is_app_public(
|
||||
client: &reqwest::Client,
|
||||
slug: &str,
|
||||
base_url: &str,
|
||||
) -> Result<bool, String> {
|
||||
let url = format!("{base_url}/apps/{slug}");
|
||||
let resp = client
|
||||
.get(&url)
|
||||
.header("Accept", "application/vnd.github+json")
|
||||
.header("User-Agent", "fabro")
|
||||
.send()
|
||||
.await
|
||||
.map_err(|e| format!("Failed to check GitHub App visibility: {e}"))?;
|
||||
|
||||
match resp.status().as_u16() {
|
||||
200 => Ok(true),
|
||||
404 => Ok(false),
|
||||
status => Err(format!(
|
||||
"Unexpected status {status} checking GitHub App visibility"
|
||||
)),
|
||||
}
|
||||
}
|
||||
|
||||
/// Resolve git clone credentials for a GitHub repository.
|
||||
///
|
||||
/// Returns `(username, password)` for authenticated cloning.
|
||||
|
|
@ -1718,6 +1795,104 @@ mod tests {
|
|||
);
|
||||
}
|
||||
|
||||
// -----------------------------------------------------------------------
|
||||
// get_authenticated_app
|
||||
// -----------------------------------------------------------------------
|
||||
|
||||
#[tokio::test]
|
||||
async fn get_authenticated_app_success() {
|
||||
let mut server = mockito::Server::new_async().await;
|
||||
|
||||
server
|
||||
.mock("GET", "/app")
|
||||
.match_header("Authorization", "Bearer test-jwt")
|
||||
.with_status(200)
|
||||
.with_body(r#"{"slug": "my-fabro-app", "owner": {"login": "my-org"}}"#)
|
||||
.create_async()
|
||||
.await;
|
||||
|
||||
let client = reqwest::Client::new();
|
||||
let info = get_authenticated_app(&client, "test-jwt", &server.url())
|
||||
.await
|
||||
.unwrap();
|
||||
assert_eq!(info.slug, "my-fabro-app");
|
||||
assert_eq!(info.owner.login, "my-org");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn get_authenticated_app_auth_failure() {
|
||||
let mut server = mockito::Server::new_async().await;
|
||||
|
||||
server
|
||||
.mock("GET", "/app")
|
||||
.with_status(401)
|
||||
.create_async()
|
||||
.await;
|
||||
|
||||
let client = reqwest::Client::new();
|
||||
let result = get_authenticated_app(&client, "bad-jwt", &server.url()).await;
|
||||
assert!(result.is_err());
|
||||
assert!(
|
||||
result.unwrap_err().contains("authentication failed"),
|
||||
"expected auth error"
|
||||
);
|
||||
}
|
||||
|
||||
// -----------------------------------------------------------------------
|
||||
// is_app_public
|
||||
// -----------------------------------------------------------------------
|
||||
|
||||
#[tokio::test]
|
||||
async fn is_app_public_returns_true_on_200() {
|
||||
let mut server = mockito::Server::new_async().await;
|
||||
|
||||
server
|
||||
.mock("GET", "/apps/my-fabro-app")
|
||||
.with_status(200)
|
||||
.with_body(r#"{"slug": "my-fabro-app"}"#)
|
||||
.create_async()
|
||||
.await;
|
||||
|
||||
let client = reqwest::Client::new();
|
||||
let result = is_app_public(&client, "my-fabro-app", &server.url()).await;
|
||||
assert_eq!(result.unwrap(), true);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn is_app_public_returns_false_on_404() {
|
||||
let mut server = mockito::Server::new_async().await;
|
||||
|
||||
server
|
||||
.mock("GET", "/apps/my-private-app")
|
||||
.with_status(404)
|
||||
.create_async()
|
||||
.await;
|
||||
|
||||
let client = reqwest::Client::new();
|
||||
let result = is_app_public(&client, "my-private-app", &server.url()).await;
|
||||
assert_eq!(result.unwrap(), false);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn is_app_public_no_auth_header() {
|
||||
let mut server = mockito::Server::new_async().await;
|
||||
|
||||
// Verify the request does NOT include an Authorization header
|
||||
let mock = server
|
||||
.mock("GET", "/apps/my-app")
|
||||
.match_header("Authorization", mockito::Matcher::Missing)
|
||||
.with_status(200)
|
||||
.with_body(r#"{"slug": "my-app"}"#)
|
||||
.create_async()
|
||||
.await;
|
||||
|
||||
let client = reqwest::Client::new();
|
||||
let result = is_app_public(&client, "my-app", &server.url()).await;
|
||||
assert_eq!(result.unwrap(), true);
|
||||
|
||||
mock.assert_async().await;
|
||||
}
|
||||
|
||||
// -----------------------------------------------------------------------
|
||||
// get_pull_request
|
||||
// -----------------------------------------------------------------------
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue