fix(install): pass GitHub App manifest state as form field, not URL query

GitHub's App Manifest endpoint rejects `redirect_url` values that carry a query string with "invalid redirect_uri", leaving the web wizard stuck: the 10-minute pending-setup guard then blocked every retry for ten minutes. Move the CSRF state out of `redirect_url` and into a hidden `state` form field on the auto-submit — GitHub preserves it on the callback, matching the CLI's working Manifest flow. Drop the retry conflict so a fresh POST to /install/github/app/manifest always replaces the pending entry and mints a new state token; stale callbacks are already rejected by the existing state-match check on the redirect handler. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-08-28 05:27:41 +00:00 · 2026-04-21 14:21:33 -04:00 · 2026-04-21 14:21:33 -04:00 · 328bddea33
commit 328bddea33
parent 125a73aae3
8 changed files with 129 additions and 89 deletions
--- a/apps/fabro-web/app/install-app.tsx
+++ b/apps/fabro-web/app/install-app.tsx
@ -427,7 +427,11 @@ export default function InstallApp() {
                app_name:         appName.trim(),
                allowed_username: allowedUsername.trim(),
              });
-              submitGithubManifest(manifest.github_form_action, manifest.manifest);
+              submitGithubManifest(
+                manifest.github_form_action,
+                manifest.manifest,
+                manifest.state,
+              );
            } catch (error) {
              setSaveError(
                error instanceof Error ? error.message : "Failed to start GitHub setup.",
@ -1438,18 +1442,25 @@ function describeGithubAppOwner(
 function submitGithubManifest(
  formAction: string,
  manifest: Record<string, unknown>,
+  state: string,
 ): void {
  const form = document.createElement("form");
  form.method = "post";
  form.action = formAction;
  form.style.display = "none";

-  const input = document.createElement("input");
-  input.type = "hidden";
-  input.name = "manifest";
-  input.value = JSON.stringify(manifest);
+  const manifestInput = document.createElement("input");
+  manifestInput.type = "hidden";
+  manifestInput.name = "manifest";
+  manifestInput.value = JSON.stringify(manifest);
+  form.appendChild(manifestInput);
+
+  const stateInput = document.createElement("input");
+  stateInput.type = "hidden";
+  stateInput.name = "state";
+  stateInput.value = state;
+  form.appendChild(stateInput);

-  form.appendChild(input);
  document.body.appendChild(form);
  form.submit();
 }
--- a/docs/api-reference/fabro-api.yaml
+++ b/docs/api-reference/fabro-api.yaml
@ -2356,6 +2356,7 @@ components:
      required:
        - manifest
        - github_form_action
+        - state
      properties:
        manifest:
          type: object
@ -2363,6 +2364,13 @@ components:
        github_form_action:
          type: string
          format: uri
+        state:
+          description: |
+            CSRF token the browser must echo back to GitHub as a hidden
+            `state` form field alongside `manifest`. GitHub preserves it on
+            the redirect to `redirect_url` so the server can match the
+            callback to this pending install.
+          type: string

    InstallGithubSummary:
      description: Redacted summary of the GitHub install strategy selected during browser install.
--- a/lib/crates/fabro-server/src/install.rs
+++ b/lib/crates/fabro-server/src/install.rs
@ -646,40 +646,27 @@ async fn post_install_github_app_manifest(
    let Some(server) = pending_install.server.clone() else {
        return missing_step_response("server");
    };
-    let now = Instant::now();
-    if pending_install
-        .pending_github_app
-        .as_ref()
-        .is_some_and(|pending| pending.expires_at > now)
-    {
-        return install_error_response(
-            StatusCode::CONFLICT,
-            "GitHub App setup is already pending; finish it or wait for it to expire.",
-        );
-    }

    let state_token = generate_ephemeral_secret();
    let manifest = build_github_app_manifest(
        input.app_name.trim(),
-        &format!(
-            "{}/install/github/app/redirect?state={state_token}",
-            server.canonical_url
-        ),
+        &format!("{}/install/github/app/redirect", server.canonical_url),
        &format!("{}/auth/callback/github", server.canonical_url),
        &format!("{}/setup", server.canonical_url),
    );

    pending_install.pending_github_app = Some(PendingGithubApp {
-        state:            state_token,
+        state:            state_token.clone(),
        owner:            owner.clone(),
        app_name:         input.app_name.trim().to_string(),
        allowed_username: input.allowed_username.trim().to_string(),
-        expires_at:       now + Duration::from_mins(10),
+        expires_at:       Instant::now() + Duration::from_mins(10),
    });

    Json(serde_json::json!({
        "manifest": manifest,
        "github_form_action": owner.manifest_form_action(),
+        "state": state_token,
    }))
    .into_response()
 }
--- a/lib/crates/fabro-server/tests/it/api/install.rs
+++ b/lib/crates/fabro-server/tests/it/api/install.rs
@ -501,15 +501,15 @@ async fn app_install_finish_omits_dev_token_and_does_not_write_it() {
        "POST /install/github/app/manifest",
    )
    .await;
-    let redirect_url = manifest_body["manifest"]["redirect_url"]
+    let state = manifest_body["state"]
        .as_str()
-        .expect("redirect_url should be present");
-    let redirect_uri = fabro_http::Url::parse(redirect_url).unwrap();
-    let state = redirect_uri
-        .query_pairs()
-        .find(|(key, _)| key == "state")
-        .map(|(_, value)| value.into_owned())
-        .expect("state should be embedded in redirect_url");
+        .expect("state should be present on manifest response")
+        .to_owned();
+    assert_eq!(
+        manifest_body["manifest"]["redirect_url"],
+        "https://fabro.example.com/install/github/app/redirect",
+        "redirect_url must not carry a query string — GitHub rejects manifests whose redirect_url has one"
+    );

    let callback_response = app
        .clone()
@ -769,15 +769,15 @@ async fn github_app_manifest_round_trip_updates_install_session() {
        "https://fabro.example.com/auth/callback/github"
    );

-    let redirect_url = manifest_body["manifest"]["redirect_url"]
+    let state = manifest_body["state"]
        .as_str()
-        .expect("redirect_url should be present");
-    let redirect_uri = fabro_http::Url::parse(redirect_url).unwrap();
-    let state = redirect_uri
-        .query_pairs()
-        .find(|(key, _)| key == "state")
-        .map(|(_, value)| value.into_owned())
-        .expect("state should be embedded in redirect_url");
+        .expect("state should be present on manifest response")
+        .to_owned();
+    assert_eq!(
+        manifest_body["manifest"]["redirect_url"],
+        "https://fabro.example.com/install/github/app/redirect",
+        "redirect_url must not carry a query string — GitHub rejects manifests whose redirect_url has one"
+    );

    let callback_response = checked_response(
        app.clone()
@ -831,7 +831,7 @@ async fn github_app_manifest_round_trip_updates_install_session() {
 }

 #[tokio::test]
-async fn github_app_manifest_rejects_retry_while_pending_and_preserves_prior_token_strategy() {
+async fn github_app_manifest_retry_replaces_pending_and_preserves_prior_token_strategy() {
    let app = build_install_router(InstallAppState::for_test("test-install-token")).await;

    let server_response = app
@ -878,7 +878,7 @@ async fn github_app_manifest_rejects_retry_while_pending_and_preserves_prior_tok
    )
    .await;

-    let manifest_response = app
+    let first_response = app
        .clone()
        .oneshot(
            Request::builder()
@ -893,15 +893,70 @@ async fn github_app_manifest_rejects_retry_while_pending_and_preserves_prior_tok
        )
        .await
        .unwrap();
-    response_status(
-        manifest_response,
+    let first_body = response_json(
+        first_response,
        StatusCode::OK,
-        "POST /install/github/app/manifest",
+        "POST /install/github/app/manifest (initial)",
+    )
+    .await;
+    let first_state = first_body["state"]
+        .as_str()
+        .expect("state should be present on manifest response")
+        .to_owned();
+
+    let retry_response = app
+        .clone()
+        .oneshot(
+            Request::builder()
+                .method("POST")
+                .uri("/install/github/app/manifest")
+                .header("authorization", "Bearer test-install-token")
+                .header("content-type", "application/json")
+                .body(Body::from(
+                    r#"{"owner":{"kind":"personal"},"app_name":"Fabro Retry","allowed_username":"octocat"}"#,
+                ))
+                .unwrap(),
+        )
+        .await
+        .unwrap();
+    let retry_body = response_json(
+        retry_response,
+        StatusCode::OK,
+        "POST /install/github/app/manifest (retry)",
+    )
+    .await;
+    let retry_state = retry_body["state"]
+        .as_str()
+        .expect("state should be present on retry manifest response")
+        .to_owned();
+    assert_ne!(
+        first_state, retry_state,
+        "retry must mint a fresh state token so the old callback is invalidated"
+    );
+
+    // A late callback using the now-discarded first-attempt state must not
+    // complete the install.
+    let stale_callback = app
+        .clone()
+        .oneshot(
+            Request::builder()
+                .method("GET")
+                .uri(format!(
+                    "/install/github/app/redirect?code=stub-code&state={first_state}"
+                ))
+                .body(Body::empty())
+                .unwrap(),
+        )
+        .await
+        .unwrap();
+    response_status(
+        stale_callback,
+        StatusCode::FOUND,
+        "GET /install/github/app/redirect with stale state",
    )
    .await;

    let session_response = app
-        .clone()
        .oneshot(
            Request::builder()
                .method("GET")
@ -923,31 +978,6 @@ async fn github_app_manifest_rejects_retry_while_pending_and_preserves_prior_tok
            .iter()
            .any(|value| value == "github")
    );
-
-    let retry_response = app
-        .oneshot(
-            Request::builder()
-                .method("POST")
-                .uri("/install/github/app/manifest")
-                .header("authorization", "Bearer test-install-token")
-                .header("content-type", "application/json")
-                .body(Body::from(
-                    r#"{"owner":{"kind":"personal"},"app_name":"Fabro Retry","allowed_username":"octocat"}"#,
-                ))
-                .unwrap(),
-        )
-        .await
-        .unwrap();
-    let retry_body = response_json(
-        retry_response,
-        StatusCode::CONFLICT,
-        "POST /install/github/app/manifest",
-    )
-    .await;
-    assert_eq!(
-        retry_body["errors"][0]["detail"],
-        "GitHub App setup is already pending; finish it or wait for it to expire."
-    );
 }

 #[tokio::test]
@ -1020,15 +1050,15 @@ async fn github_app_redirect_rejects_invalid_or_missing_state_without_mutating_s
        "POST /install/github/app/manifest",
    )
    .await;
-    let redirect_url = manifest_body["manifest"]["redirect_url"]
+    let state = manifest_body["state"]
        .as_str()
-        .expect("redirect_url should be present");
-    let redirect_uri = fabro_http::Url::parse(redirect_url).unwrap();
-    let state = redirect_uri
-        .query_pairs()
-        .find(|(key, _)| key == "state")
-        .map(|(_, value)| value.into_owned())
-        .expect("state should be embedded in redirect_url");
+        .expect("state should be present on manifest response")
+        .to_owned();
+    assert_eq!(
+        manifest_body["manifest"]["redirect_url"],
+        "https://fabro.example.com/install/github/app/redirect",
+        "redirect_url must not carry a query string — GitHub rejects manifests whose redirect_url has one"
+    );

    let wrong_state_response = checked_response(
        app.clone()
@ -1181,15 +1211,15 @@ async fn github_app_redirect_exchange_failure_returns_to_wizard_and_keeps_pendin
        "POST /install/github/app/manifest",
    )
    .await;
-    let redirect_url = manifest_body["manifest"]["redirect_url"]
+    let state = manifest_body["state"]
        .as_str()
-        .expect("redirect_url should be present");
-    let redirect_uri = fabro_http::Url::parse(redirect_url).unwrap();
-    let state = redirect_uri
-        .query_pairs()
-        .find(|(key, _)| key == "state")
-        .map(|(_, value)| value.into_owned())
-        .expect("state should be embedded in redirect_url");
+        .expect("state should be present on manifest response")
+        .to_owned();
+    assert_eq!(
+        manifest_body["manifest"]["redirect_url"],
+        "https://fabro.example.com/install/github/app/redirect",
+        "redirect_url must not carry a query string — GitHub rejects manifests whose redirect_url has one"
+    );

    let callback_response = checked_response(
        app.clone()
--- a/lib/crates/fabro-spa/assets/assets/app.css
+++ b/lib/crates/fabro-spa/assets/assets/app.css
--- a/lib/crates/fabro-spa/assets/assets/entry-n6nbr3z5.js
+++ b/lib/crates/fabro-spa/assets/assets/entry-n6nbr3z5.js
--- a/lib/crates/fabro-spa/assets/index.html
+++ b/lib/crates/fabro-spa/assets/index.html
@ -58,7 +58,7 @@
    <script type="module" src="/assets/chunk-sadshphz.js"></script>
    <script type="module" src="/assets/chunk-pmthkscp.js"></script>
    <script type="module" src="/assets/chunk-v61ks9f7.js"></script>
-    <script type="module" src="/assets/entry-b79sap7r.js"></script>
+    <script type="module" src="/assets/entry-n6nbr3z5.js"></script>
    <script type="module" src="/assets/chunk-n1k68xa8.js"></script>
    <script type="module" src="/assets/chunk-rsph5pvm.js"></script>
    <script type="module" src="/assets/chunk-9t57pdty.js"></script>
--- a/lib/packages/fabro-api-client/src/models/install-github-app-manifest-response.ts
+++ b/lib/packages/fabro-api-client/src/models/install-github-app-manifest-response.ts
@ -20,5 +20,9 @@
 export interface InstallGithubAppManifestResponse {
    'manifest': { [key: string]: any; };
    'github_form_action': string;
+    /**
+     * CSRF token the browser must echo back to GitHub as a hidden `state` form field alongside `manifest`. GitHub preserves it on the redirect to `redirect_url` so the server can match the callback to this pending install. 
+     */
+    'state': string;
 }