amadan
broker: a state-less callback arrival gets an "all set" page, not "bad state" (#533)
Hit live: installing the GitHub App on a second account directly on github.com bounces the member to the registered callback with a code but no state (our flow never started), and the broker answered with a bare "bad state" error — alarming, though the install itself succeeded. A human at that URL with no state is almost always exactly this, so say so: a small friendly page, CSP-locked, storing nothing. Real flows (state present) are untouched, and an unregistered origin still refuses. Also worth doing at the source: the GitHub App's "Request user authorization (OAuth) during installation" setting causes these bounces and our plain- authorize flow doesn't use it — unchecking it stops them entirely.
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2 files changed,
+32
−1
broker/broker_test.go+20 −0broker/main.go+12 −1
diff --git a/broker/broker_test.go b/broker/broker_test.go| index 45064e6..a31443c 100644 |
| --- a/broker/broker_test.go |
| +++ b/broker/broker_test.go |
| @@ -347,3 +347,23 @@ func TestRegistry(t *testing.T) { |
| t.Fatalf("registry = schema %d, %d services (want 1, 4)", out.Schema, len(out.Services)) |
| } |
| } |
| + |
| +// A state-less arrival at the callback is a human GitHub bounced here after an |
| +// on-github.com app install ("request OAuth during installation") — they get a |
| +// humane "all set" page, never a bare "bad state" error. |
| +func TestCallbackWithoutStateIsFriendly(t *testing.T) { |
| + ts := newTestBroker(t, "http://unused") |
| + resp, err := http.Get(ts.URL + "/oauth/github/callback?code=XYZ&setup_action=install&installation_id=1") |
| + if err != nil { |
| + t.Fatalf("callback: %v", err) |
| + } |
| + defer resp.Body.Close() |
| + if resp.StatusCode != http.StatusOK { |
| + t.Fatalf("got %d, want a 200 page", resp.StatusCode) |
| + } |
| + body := make([]byte, 512) |
| + n, _ := resp.Body.Read(body) |
| + if !strings.Contains(string(body[:n]), "All set") { |
| + t.Fatalf("page: %q", body[:n]) |
| + } |
| +} |
diff --git a/broker/main.go b/broker/main.go| index 4c48799..c81d50c 100644 |
| --- a/broker/main.go |
| +++ b/broker/main.go |
| @@ -268,7 +268,18 @@ func (s *Server) callback(w http.ResponseWriter, r *http.Request) { |
| state := q.Get("state") |
| dot := strings.IndexByte(state, '.') |
| if dot <= 0 { |
| - http.Error(w, "bad state", http.StatusBadRequest) |
| + // No (or malformed) state usually means GitHub sent someone here after |
| + // an app INSTALL made on github.com itself ("request OAuth during |
| + // installation" bounces to the registered callback with a code but no |
| + // state, since our flow never started). The install worked; a human is |
| + // looking at this page — say something humane instead of "bad state". |
| + w.Header().Set("Content-Security-Policy", "default-src 'none'; style-src 'unsafe-inline'") |
| + w.Header().Set("Content-Type", "text/html; charset=utf-8") |
| + w.WriteHeader(http.StatusOK) |
| + w.Write([]byte(`<!doctype html><meta charset="utf-8"><title>All set</title>` + |
| + `<body style="font:16px/1.5 system-ui;max-width:34em;margin:4rem auto;color:#333">` + |
| + `<h2>All set ✓</h2><p>If you were installing or updating an app on the provider's site, ` + |
| + `that worked. You can close this tab and head back to your chat — reconnect or refresh there if needed.</p>`)) |
| return |
| } |
| originBytes, err := base64.RawURLEncoding.DecodeString(state[:dot]) |