From c40b4315268740ef0cbbd952bf675b73ac3e4ba9 Mon Sep 17 00:00:00 2001 From: Karibhou Date: Tue, 27 Oct 2026 20:28:12 +0000 Subject: [PATCH 1/5] =?UTF-8?q?=E2=9C=A8=20feat(hub):=20add=20S256=20PKCE?= =?UTF-8?q?=20verifier=20cookie=20to=20the=20Hub=20GitHub=20login?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit startProviderLogin now mints a 32-byte code_verifier for the GitHub (non-OIDC) branch, stores it in a separate host-scoped HttpOnly/Secure/Lax cookie with the state TTL, and adds code_challenge (S256) and code_challenge_method=S256 to the authorize URL. The callback reads and clears the cookie on every outcome, fails closed with 400 on a missing or malformed verifier before any token exchange, and passes the verifier to exchangeGitHubLogin, which now sends a form-encoded POST body instead of URL query parameters. The state nonce format, OIDC providers and the device flow are unchanged. Hive-Run: hivecommons/hive#14633 Hive-Plan: hivecommons/hive#14598 (hub-auth hardening RFC) — PKCE child Hive-Spec: hivecommons/hive#14598#proposal-1 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01HRH6kiVuagmg4z6UnoDtj6 Signed-off-by: Karibhou --- changelog.d/security-14633-hub-github-pkce.md | 1 + src/pkg/hub/oauth.go | 116 ++++++- src/pkg/hub/oauth_pkce_test.go | 309 ++++++++++++++++++ src/pkg/hub/oauth_provision_coverage_test.go | 2 + 4 files changed, 424 insertions(+), 4 deletions(-) create mode 100644 changelog.d/security-14633-hub-github-pkce.md create mode 100644 src/pkg/hub/oauth_pkce_test.go diff --git a/changelog.d/security-14633-hub-github-pkce.md b/changelog.d/security-14633-hub-github-pkce.md new file mode 100644 index 0000000000..c679e9b966 --- /dev/null +++ b/changelog.d/security-14633-hub-github-pkce.md @@ -0,0 +1 @@ +- The Hub's GitHub browser login now uses PKCE (S256): the authorize redirect carries a `code_challenge`, the matching `code_verifier` lives in a short-lived single-use HttpOnly cookie, and the token exchange is a form-encoded POST carrying it, so client secret, code and verifier no longer appear in the request URL. A callback with a missing or malformed verifier is rejected with 400 before any code exchange or session is minted; logins started before the upgrade must be restarted (#14633). diff --git a/src/pkg/hub/oauth.go b/src/pkg/hub/oauth.go index cc43f8167f..f5efbdbcfe 100644 --- a/src/pkg/hub/oauth.go +++ b/src/pkg/hub/oauth.go @@ -2,7 +2,9 @@ package hub import ( cryptoRand "crypto/rand" + "crypto/sha256" "crypto/subtle" + "encoding/base64" "encoding/hex" "encoding/json" "fmt" @@ -601,8 +603,29 @@ func (s *HubServer) startProviderLogin(w http.ResponseWriter, r *http.Request, p // scope is deliberate — the hub only needs WHO is logging in, and GitHub // serves /user's public profile (including "login") unscoped. Do NOT add a // scope without a feature that needs it. - authURL := fmt.Sprintf("%s?client_id=%s&scope=&redirect_uri=%s&state=%s", - p.AuthorizeURL, p.ClientID, oauthRedirectURI(), state) + // + // PKCE (RFC 7636, S256): mint a per-login code_verifier, keep it in its + // own short-lived host-scoped cookie (mirroring the state cookie), and + // send only its S256 challenge. The callback must present the verifier + // at token exchange, so an authorization code intercepted on its way + // back cannot be redeemed by anyone without this browser's cookie. + verifier, err := oauthPKCEVerifier() + if err != nil { + s.logger.Warn("OAuth: cannot mint PKCE verifier", "error", err) + http.Error(w, "login unavailable", http.StatusInternalServerError) + return + } + http.SetCookie(w, &http.Cookie{ + Name: oauthPKCECookieName, + Value: verifier, + Path: "/", + MaxAge: int(oauthStateTTL.Seconds()), + HttpOnly: true, + Secure: true, + SameSite: http.SameSiteLaxMode, + }) + authURL := fmt.Sprintf("%s?client_id=%s&scope=&redirect_uri=%s&state=%s&code_challenge=%s&code_challenge_method=S256", + p.AuthorizeURL, p.ClientID, oauthRedirectURI(), state, pkceS256Challenge(verifier)) http.Redirect(w, r, authURL, http.StatusTemporaryRedirect) return } @@ -718,6 +741,11 @@ text-decoration:none;font-size:16px;font-weight:600;transition:background .12s,b } func (s *HubServer) handleOAuthCallback(w http.ResponseWriter, r *http.Request) { + // PKCE verifier: read once and clear immediately, on every outcome, so it is + // single-use whether this callback succeeds or is rejected below. + pkceVerifier := s.oauthPKCEVerifierFromCookie(r) + s.clearOAuthPKCECookie(w) + code := r.URL.Query().Get("code") if code == "" { http.Error(w, "missing code", http.StatusBadRequest) @@ -790,7 +818,16 @@ func (s *HubServer) handleOAuthCallback(w http.ResponseWriter, r *http.Request) displayName = claims.Name s.logger.Info("audit: hub OIDC login", "provider", p.Name, "user", canonicalID) } else { - login, avatar, token, ok := s.exchangeGitHubLogin(w, code) + // Fail closed BEFORE the exchange: a missing or malformed verifier means + // this browser never started a PKCE login (or already used it), and the + // code must not be redeemed. In-flight logins from before PKCE land here + // too and are asked to start again. + if !validPKCEVerifier(pkceVerifier) { + s.logger.Warn("OAuth: rejected callback with missing or malformed PKCE verifier") + http.Error(w, "invalid login state — please start the login again", http.StatusBadRequest) + return + } + login, avatar, token, ok := s.exchangeGitHubLogin(w, code, pkceVerifier) if !ok { return // exchangeGitHubLogin already wrote the error } @@ -931,7 +968,11 @@ func (s *HubServer) parseCallbackState(r *http.Request) (provider, redirect stri // login, avatar, and user access token. On any failure it writes the HTTP error // and returns ok=false. This is the original callback logic, factored out so the // dispatcher can share the surrounding cookie/persistence code with OIDC. -func (s *HubServer) exchangeGitHubLogin(w http.ResponseWriter, code string) (login, avatar, token string, ok bool) { +// +// The exchange is a form-encoded POST: the client secret, the one-time code and +// the PKCE code_verifier travel in the body, never in the URL where request +// logs could retain them. +func (s *HubServer) exchangeGitHubLogin(w http.ResponseWriter, code, codeVerifier string) (login, avatar, token string, ok bool) { clientID := os.Getenv("HIVE_HUB_OAUTH_CLIENT_ID") clientSecret := os.Getenv("HIVE_HUB_OAUTH_CLIENT_SECRET") @@ -942,6 +983,7 @@ func (s *HubServer) exchangeGitHubLogin(w http.ResponseWriter, code string) (log form.Set("client_secret", clientSecret) form.Set("code", code) form.Set("redirect_uri", oauthRedirectURI()) + form.Set("code_verifier", codeVerifier) tokenReq, _ := http.NewRequest("POST", defaultGHTokenURL, strings.NewReader(form.Encode())) tokenReq.Header.Set("Content-Type", "application/x-www-form-urlencoded") tokenReq.Header.Set("Accept", "application/json") @@ -1417,8 +1459,74 @@ const ( // oauthStateNonceBytes is the entropy behind the nonce. 32 bytes is far // beyond guessing and matches the other random values minted in this package. oauthStateNonceBytes = 32 + + // oauthPKCECookieName holds the per-login PKCE code_verifier (RFC 7636) for + // the GitHub browser login. Separate from the state cookie so the state + // nonce format is untouched; host-scoped and single-use like it. + oauthPKCECookieName = "hive_oauth_pkce" + + // oauthPKCEVerifierBytes is the entropy behind the code_verifier. 32 bytes + // base64url-encode to 43 characters, the RFC 7636 minimum length. + oauthPKCEVerifierBytes = 32 ) +// oauthPKCEVerifier mints a PKCE code_verifier: 32 random bytes, base64url +// without padding (43 unreserved characters). +func oauthPKCEVerifier() (string, error) { + buf := make([]byte, oauthPKCEVerifierBytes) + if _, err := io.ReadFull(cryptoRand.Reader, buf); err != nil { + return "", err + } + return base64.RawURLEncoding.EncodeToString(buf), nil +} + +// pkceS256Challenge returns BASE64URL(SHA256(verifier)), the S256 code_challenge. +func pkceS256Challenge(verifier string) string { + sum := sha256.Sum256([]byte(verifier)) + return base64.RawURLEncoding.EncodeToString(sum[:]) +} + +// validPKCEVerifier reports whether v is a well-formed RFC 7636 code_verifier: +// 43–128 characters from the unreserved set [A-Za-z0-9-._~]. +func validPKCEVerifier(v string) bool { + if len(v) < 43 || len(v) > 128 { + return false + } + for i := 0; i < len(v); i++ { + c := v[i] + switch { + case c >= 'A' && c <= 'Z', c >= 'a' && c <= 'z', c >= '0' && c <= '9', + c == '-', c == '.', c == '_', c == '~': + default: + return false + } + } + return true +} + +// oauthPKCEVerifierFromCookie returns the PKCE verifier this browser was issued +// at login start, or "" if absent. +func (s *HubServer) oauthPKCEVerifierFromCookie(r *http.Request) string { + c, err := r.Cookie(oauthPKCECookieName) + if err != nil { + return "" + } + return c.Value +} + +// clearOAuthPKCECookie expires the PKCE verifier, making it single-use. +func (s *HubServer) clearOAuthPKCECookie(w http.ResponseWriter) { + http.SetCookie(w, &http.Cookie{ + Name: oauthPKCECookieName, + Value: "", + Path: "/", + MaxAge: -1, + HttpOnly: true, + Secure: true, + SameSite: http.SameSiteLaxMode, + }) +} + // oauthStateNonce mints an unguessable login nonce. func oauthStateNonce() (string, error) { buf := make([]byte, oauthStateNonceBytes) diff --git a/src/pkg/hub/oauth_pkce_test.go b/src/pkg/hub/oauth_pkce_test.go new file mode 100644 index 0000000000..384231f854 --- /dev/null +++ b/src/pkg/hub/oauth_pkce_test.go @@ -0,0 +1,309 @@ +package hub + +import ( + "crypto/sha256" + "encoding/base64" + "log/slog" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "sync/atomic" + "testing" + + "github.com/hivecommons/hive/pkg/auth" +) + +// addPKCECookie gives a callback request the PKCE verifier cookie a real +// browser would carry after startProviderLogin, and returns the verifier. +func addPKCECookie(t *testing.T, req *http.Request) string { + t.Helper() + v, err := oauthPKCEVerifier() + if err != nil { + t.Fatalf("mint PKCE verifier: %v", err) + } + req.AddCookie(&http.Cookie{Name: oauthPKCECookieName, Value: v}) + return v +} + +// pkceCookieCleared reports whether the response expires the PKCE cookie. +func pkceCookieCleared(rec *httptest.ResponseRecorder) bool { + for _, c := range rec.Result().Cookies() { + if c.Name == oauthPKCECookieName && c.MaxAge < 0 && c.Value == "" { + return true + } + } + return false +} + +// fakeGitHubTokenEndpoint points the token and user endpoints at local +// servers and counts token-exchange requests. onToken inspects each one. +func fakeGitHubTokenEndpoint(t *testing.T, onToken func(r *http.Request)) *atomic.Int32 { + t.Helper() + var hits atomic.Int32 + tok := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hits.Add(1) + if onToken != nil { + onToken(r) + } + _, _ = w.Write([]byte(`{"access_token":"gho_pkce"}`)) + })) + usr := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`{"login":"octocat","avatar_url":"https://x/a.png"}`)) + })) + oldTok, oldUsr := defaultGHTokenURL, defaultGHUserURL + defaultGHTokenURL, defaultGHUserURL = tok.URL, usr.URL + t.Cleanup(func() { + defaultGHTokenURL, defaultGHUserURL = oldTok, oldUsr + tok.Close() + usr.Close() + }) + return &hits +} + +func TestPKCE_VerifierShapeAndChallenge(t *testing.T) { + v, err := oauthPKCEVerifier() + if err != nil { + t.Fatal(err) + } + if len(v) != 43 || !validPKCEVerifier(v) { + t.Fatalf("verifier %q: want a valid 43-char RFC 7636 verifier", v) + } + sum := sha256.Sum256([]byte(v)) + if got, want := pkceS256Challenge(v), base64.RawURLEncoding.EncodeToString(sum[:]); got != want { + t.Fatalf("challenge = %q, want %q", got, want) + } + // RFC 7636 appendix B test vector. + if got := pkceS256Challenge("dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk"); got != "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM" { + t.Fatalf("RFC 7636 vector challenge = %q", got) + } + for _, bad := range []string{"", "short", strings.Repeat("a", 42), strings.Repeat("a", 129), strings.Repeat("a", 42) + "!", strings.Repeat("a", 42) + "%"} { + if validPKCEVerifier(bad) { + t.Errorf("validPKCEVerifier(%q) = true, want false", bad) + } + } +} + +func TestPKCE_GitHubAuthorizeRedirectCarriesS256Challenge(t *testing.T) { + s := &HubServer{ + authProviders: auth.NewRegistry(&auth.Provider{ + Name: "github", + DisplayName: "GitHub", + AuthorizeURL: "https://github.com/login/oauth/authorize", + ClientID: "test-client-id", + }), + } + req := httptest.NewRequest(http.MethodGet, "/login/github", nil) + req.SetPathValue("provider", "github") + rec := httptest.NewRecorder() + s.handleProviderLogin(rec, req) + + if rec.Code != http.StatusTemporaryRedirect { + t.Fatalf("status = %d, want 307", rec.Code) + } + var pkce *http.Cookie + for _, c := range rec.Result().Cookies() { + if c.Name == oauthPKCECookieName { + pkce = c + } + } + if pkce == nil { + t.Fatalf("no %s cookie set", oauthPKCECookieName) + } + if !pkce.HttpOnly || !pkce.Secure || pkce.SameSite != http.SameSiteLaxMode || pkce.Domain != "" || pkce.Path != "/" { + t.Errorf("PKCE cookie must be HttpOnly+Secure+Lax, host-scoped, Path=/; got %+v", pkce) + } + if pkce.MaxAge != int(oauthStateTTL.Seconds()) { + t.Errorf("PKCE cookie MaxAge = %d, want %d", pkce.MaxAge, int(oauthStateTTL.Seconds())) + } + if !validPKCEVerifier(pkce.Value) { + t.Fatalf("PKCE cookie value %q is not a valid verifier", pkce.Value) + } + + loc, err := url.Parse(rec.Header().Get("Location")) + if err != nil { + t.Fatal(err) + } + q := loc.Query() + if got := q.Get("code_challenge"); got != pkceS256Challenge(pkce.Value) { + t.Errorf("code_challenge = %q, want S256 of the cookie verifier", got) + } + if got := q.Get("code_challenge_method"); got != "S256" { + t.Errorf("code_challenge_method = %q, want S256", got) + } + if _, ok := q["scope"]; !ok || q.Get("scope") != "" { + t.Errorf("scope must stay present and empty, got %q (present=%v)", q.Get("scope"), ok) + } + if strings.Contains(loc.RawQuery, pkce.Value) { + t.Error("the verifier itself must never appear in the authorize URL") + } +} + +func TestPKCE_OIDCAuthorizeHasNoPKCECookie(t *testing.T) { + f := newFakeOIDCProvider(t) + defer f.close() + s := &HubServer{logger: slog.Default()} + s.authProviders = auth.NewRegistry(&auth.Provider{ + Name: "google", + DisplayName: "Google", + IsOIDC: true, + Issuer: f.issuer, + ClientID: "hub-oidc-client", + Scopes: []string{"openid", "email", "profile"}, + }) + req := httptest.NewRequest(http.MethodGet, "/api/auth/login/google", nil) + req.SetPathValue("provider", "google") + rec := httptest.NewRecorder() + s.handleProviderLogin(rec, req) + if rec.Code != http.StatusTemporaryRedirect { + t.Fatalf("status = %d, want 307", rec.Code) + } + if strings.Contains(rec.Header().Get("Location"), "code_challenge") { + t.Error("OIDC authorize URL must be unchanged: no code_challenge") + } + for _, c := range rec.Result().Cookies() { + if c.Name == oauthPKCECookieName { + t.Fatal("OIDC login must be unchanged: no PKCE cookie") + } + } +} + +func TestPKCE_CallbackSendsVerifierInFormBody(t *testing.T) { + cleanup := helperSetupTempDirs(t) + defer cleanup() + + var gotMethod, gotCT, gotVerifier, gotRawQuery string + hits := fakeGitHubTokenEndpoint(t, func(r *http.Request) { + gotMethod, gotCT, gotRawQuery = r.Method, r.Header.Get("Content-Type"), r.URL.RawQuery + _ = r.ParseForm() + gotVerifier = r.PostForm.Get("code_verifier") + }) + + s := &HubServer{logger: slog.Default(), hubSecret: testHubSecret} + req := newCallbackRequestWithState(t, "abc") + verifier := s.oauthPKCEVerifierFromCookie(req) + rec := httptest.NewRecorder() + s.handleOAuthCallback(rec, req) + + if rec.Code != http.StatusTemporaryRedirect { + t.Fatalf("status = %d, want 307 (body=%s)", rec.Code, rec.Body.String()) + } + if hits.Load() != 1 { + t.Fatalf("token endpoint hits = %d, want 1", hits.Load()) + } + if gotMethod != http.MethodPost || !strings.HasPrefix(gotCT, "application/x-www-form-urlencoded") { + t.Errorf("token request = %s %q, want form-encoded POST", gotMethod, gotCT) + } + if gotVerifier != verifier { + t.Errorf("code_verifier = %q, want the cookie verifier %q", gotVerifier, verifier) + } + if gotRawQuery != "" { + t.Errorf("token request URL must carry no parameters, got %q", gotRawQuery) + } + if !pkceCookieCleared(rec) { + t.Error("PKCE cookie must be cleared on success") + } +} + +func TestPKCE_CallbackRejectsBadVerifierBeforeExchange(t *testing.T) { + cases := map[string]func(*http.Request){ + "missing": func(*http.Request) {}, + "empty": func(r *http.Request) { r.AddCookie(&http.Cookie{Name: oauthPKCECookieName, Value: ""}) }, + "malformed": func(r *http.Request) { r.AddCookie(&http.Cookie{Name: oauthPKCECookieName, Value: "not a verifier"}) }, + "too short": func(r *http.Request) { + r.AddCookie(&http.Cookie{Name: oauthPKCECookieName, Value: strings.Repeat("a", 42)}) + }, + } + for name, addCookie := range cases { + t.Run(name, func(t *testing.T) { + cleanup := helperSetupTempDirs(t) + defer cleanup() + hits := fakeGitHubTokenEndpoint(t, nil) + + nonce, _ := oauthStateNonce() + req := httptest.NewRequest(http.MethodGet, "/api/auth/callback?code=abc&state="+url.QueryEscape(nonce+oauthStateSeparator+"/dashboard"), nil) + req.AddCookie(&http.Cookie{Name: oauthStateCookieName, Value: nonce}) + addCookie(req) + + s := &HubServer{logger: slog.Default(), hubSecret: testHubSecret} + rec := httptest.NewRecorder() + s.handleOAuthCallback(rec, req) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400", rec.Code) + } + if hits.Load() != 0 { + t.Fatalf("token endpoint hits = %d, want 0", hits.Load()) + } + for _, c := range rec.Result().Cookies() { + if c.Name == "hive_hub_user" && c.Value != "" { + t.Fatal("no session may be minted") + } + } + if loadSaaSUser("octocat") != nil { + t.Fatal("no user record may be written") + } + if !pkceCookieCleared(rec) { + t.Error("PKCE cookie must be cleared on failure") + } + }) + } +} + +func TestPKCE_CallbackRejectsBadStateBeforeExchangeAndClearsVerifier(t *testing.T) { + cleanup := helperSetupTempDirs(t) + defer cleanup() + hits := fakeGitHubTokenEndpoint(t, nil) + + req := httptest.NewRequest(http.MethodGet, "/api/auth/callback?code=abc&state="+url.QueryEscape("attacker-nonce"+oauthStateSeparator+"/dashboard"), nil) + req.AddCookie(&http.Cookie{Name: oauthStateCookieName, Value: "victim-nonce"}) + addPKCECookie(t, req) + + s := &HubServer{logger: slog.Default(), hubSecret: testHubSecret} + rec := httptest.NewRecorder() + s.handleOAuthCallback(rec, req) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400", rec.Code) + } + if hits.Load() != 0 { + t.Fatalf("token endpoint hits = %d, want 0", hits.Load()) + } + if !pkceCookieCleared(rec) { + t.Error("PKCE cookie must be cleared when state is rejected") + } +} + +// TestPKCE_ReplayedCallbackIsRejected replays a completed callback the way a +// browser would after the first response: the state and PKCE cookies have been +// expired, so the second attempt carries neither and must not reach the exchange. +func TestPKCE_ReplayedCallbackIsRejected(t *testing.T) { + cleanup := helperSetupTempDirs(t) + defer cleanup() + hits := fakeGitHubTokenEndpoint(t, nil) + + s := &HubServer{logger: slog.Default(), hubSecret: testHubSecret} + first := newCallbackRequestWithState(t, "abc") + rec := httptest.NewRecorder() + s.handleOAuthCallback(rec, first) + if rec.Code != http.StatusTemporaryRedirect || hits.Load() != 1 { + t.Fatalf("first callback: status=%d hits=%d, want 307 and 1", rec.Code, hits.Load()) + } + + // Same URL; the state cookie is resent (a stale copy) but the PKCE cookie + // was cleared by the first response. + replay := httptest.NewRequest(http.MethodGet, first.URL.String(), nil) + for _, c := range first.Cookies() { + if c.Name != oauthPKCECookieName { + replay.AddCookie(c) + } + } + rec2 := httptest.NewRecorder() + s.handleOAuthCallback(rec2, replay) + if rec2.Code != http.StatusBadRequest { + t.Fatalf("replay status = %d, want 400", rec2.Code) + } + if hits.Load() != 1 { + t.Fatalf("replay reached the token endpoint (hits=%d)", hits.Load()) + } +} diff --git a/src/pkg/hub/oauth_provision_coverage_test.go b/src/pkg/hub/oauth_provision_coverage_test.go index 3ef0f04356..05b32c348e 100644 --- a/src/pkg/hub/oauth_provision_coverage_test.go +++ b/src/pkg/hub/oauth_provision_coverage_test.go @@ -89,6 +89,7 @@ func TestHandleOAuthCallbackSuccess(t *testing.T) { } req := httptest.NewRequest(http.MethodGet, "/api/auth/callback?code=abc&state="+url.QueryEscape(nonce+oauthStateSeparator+"/my-hives"), nil) req.AddCookie(&http.Cookie{Name: oauthStateCookieName, Value: nonce}) + addPKCECookie(t, req) s.handleOAuthCallback(rec, req) if rec.Code != http.StatusTemporaryRedirect { t.Fatalf("success status = %d, want 307 (body=%s)", rec.Code, rec.Body.String()) @@ -344,6 +345,7 @@ func newCallbackRequestWithState(t *testing.T, code string) *http.Request { } req := httptest.NewRequest(http.MethodGet, "/api/auth/callback?code="+code+"&state="+url.QueryEscape(nonce+oauthStateSeparator+"/dashboard"), nil) req.AddCookie(&http.Cookie{Name: oauthStateCookieName, Value: nonce}) + addPKCECookie(t, req) return req } From d4a66a4d983d80f9e4fadf9148b0b2df34029e32 Mon Sep 17 00:00:00 2001 From: Karibhou Date: Tue, 27 Oct 2026 20:35:55 +0000 Subject: [PATCH 2/5] =?UTF-8?q?=F0=9F=93=9D=20docs(api):=20re-anchor=20Hub?= =?UTF-8?q?=20OAuth=20route=20citations=20after=20PKCE=20change?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The PKCE additions shifted the route registrations in pkg/hub/oauth.go down by two lines, so the five Hub auth rows in api-reference.md cited stale lines and the citation check failed. Rewritten with check-api-reference-citations.sh --fix. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Karibhou --- src/docs/api-reference.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/docs/api-reference.md b/src/docs/api-reference.md index 743365dd82..7ca9469016 100644 --- a/src/docs/api-reference.md +++ b/src/docs/api-reference.md @@ -746,11 +746,11 @@ always resolved server-side from the validated token. | Method | Path | Auth | Purpose | Source | |---|---|---|---|---| -| `GET` | `/login` | Hub handler-specific | Login | `pkg/hub/oauth.go:69` | -| `GET` | `/login/{provider}` | Hub handler-specific | Per-Provider Login (github or OIDC provider) | `pkg/hub/oauth.go:72` | -| `GET` | `/api/auth/callback` | Hub handler-specific | OAuth Callback | `pkg/hub/oauth.go:73` | -| `GET` | `/api/auth/user` | Hub handler-specific | Auth User | `pkg/hub/oauth.go:74` | -| `POST` | `/api/auth/logout` | Hub handler-specific | Logout | `pkg/hub/oauth.go:75` | +| `GET` | `/login` | Hub handler-specific | Login | `pkg/hub/oauth.go:71` | +| `GET` | `/login/{provider}` | Hub handler-specific | Per-Provider Login (github or OIDC provider) | `pkg/hub/oauth.go:74` | +| `GET` | `/api/auth/callback` | Hub handler-specific | OAuth Callback | `pkg/hub/oauth.go:75` | +| `GET` | `/api/auth/user` | Hub handler-specific | Auth User | `pkg/hub/oauth.go:76` | +| `POST` | `/api/auth/logout` | Hub handler-specific | Logout | `pkg/hub/oauth.go:77` | | `GET` | `/api/openrouter/connect/start` | Hub auth | Hub Open Router Start | `pkg/hub/openrouter.go:26` | | `GET` | `/api/openrouter/qr` | Hub auth | Hub Open Router QR | `pkg/hub/openrouter.go:27` | | `GET` | `/api/openrouter/models` | Hub auth | Hub Open Router Models | `pkg/hub/openrouter.go:28` | From 233160aaa3987a05a2e52d0cad3b555a780a74f1 Mon Sep 17 00:00:00 2001 From: Karibhou Date: Wed, 28 Oct 2026 10:00:08 +0000 Subject: [PATCH 3/5] =?UTF-8?q?=F0=9F=90=9B=20fix(hub):=20verify=20PKCE=20?= =?UTF-8?q?exchange=20retains=20redirect=20URI?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Karibhou --- changelog.d/security-14633-hub-github-pkce.md | 2 +- src/pkg/hub/oauth_pkce_test.go | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/changelog.d/security-14633-hub-github-pkce.md b/changelog.d/security-14633-hub-github-pkce.md index c679e9b966..e30ff9a423 100644 --- a/changelog.d/security-14633-hub-github-pkce.md +++ b/changelog.d/security-14633-hub-github-pkce.md @@ -1 +1 @@ -- The Hub's GitHub browser login now uses PKCE (S256): the authorize redirect carries a `code_challenge`, the matching `code_verifier` lives in a short-lived single-use HttpOnly cookie, and the token exchange is a form-encoded POST carrying it, so client secret, code and verifier no longer appear in the request URL. A callback with a missing or malformed verifier is rejected with 400 before any code exchange or session is minted; logins started before the upgrade must be restarted (#14633). +- The Hub's GitHub browser login now uses PKCE (S256): as defence-in-depth to bind the authorization code to the browser that started the login, the authorize redirect carries a `code_challenge`, the matching `code_verifier` lives in a short-lived single-use HttpOnly cookie, and the token exchange is a form-encoded POST carrying it, so client secret, code and verifier no longer appear in the request URL. A callback with a missing or malformed verifier is rejected with 400 before any code exchange or session is minted; logins started before the upgrade must be restarted (#14633). diff --git a/src/pkg/hub/oauth_pkce_test.go b/src/pkg/hub/oauth_pkce_test.go index 384231f854..e7a9270bfe 100644 --- a/src/pkg/hub/oauth_pkce_test.go +++ b/src/pkg/hub/oauth_pkce_test.go @@ -172,11 +172,12 @@ func TestPKCE_CallbackSendsVerifierInFormBody(t *testing.T) { cleanup := helperSetupTempDirs(t) defer cleanup() - var gotMethod, gotCT, gotVerifier, gotRawQuery string + var gotMethod, gotCT, gotVerifier, gotRedirectURI, gotRawQuery string hits := fakeGitHubTokenEndpoint(t, func(r *http.Request) { gotMethod, gotCT, gotRawQuery = r.Method, r.Header.Get("Content-Type"), r.URL.RawQuery _ = r.ParseForm() gotVerifier = r.PostForm.Get("code_verifier") + gotRedirectURI = r.PostForm.Get("redirect_uri") }) s := &HubServer{logger: slog.Default(), hubSecret: testHubSecret} @@ -197,6 +198,9 @@ func TestPKCE_CallbackSendsVerifierInFormBody(t *testing.T) { if gotVerifier != verifier { t.Errorf("code_verifier = %q, want the cookie verifier %q", gotVerifier, verifier) } + if gotRedirectURI != oauthRedirectURI() || gotRedirectURI == "" { + t.Errorf("redirect_uri = %q, want %q", gotRedirectURI, oauthRedirectURI()) + } if gotRawQuery != "" { t.Errorf("token request URL must carry no parameters, got %q", gotRawQuery) } From 05b08d64cc9ca043d0e5420f0b192d5885c14903 Mon Sep 17 00:00:00 2001 From: Karibhou Date: Wed, 28 Oct 2026 10:00:27 +0000 Subject: [PATCH 4/5] =?UTF-8?q?=F0=9F=93=9D=20docs(hub):=20qualify=20PKCE?= =?UTF-8?q?=20security=20guarantee?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Karibhou --- src/pkg/hub/oauth.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/pkg/hub/oauth.go b/src/pkg/hub/oauth.go index f5efbdbcfe..587ed76f74 100644 --- a/src/pkg/hub/oauth.go +++ b/src/pkg/hub/oauth.go @@ -607,8 +607,8 @@ func (s *HubServer) startProviderLogin(w http.ResponseWriter, r *http.Request, p // PKCE (RFC 7636, S256): mint a per-login code_verifier, keep it in its // own short-lived host-scoped cookie (mirroring the state cookie), and // send only its S256 challenge. The callback must present the verifier - // at token exchange, so an authorization code intercepted on its way - // back cannot be redeemed by anyone without this browser's cookie. + // at token exchange, binding the authorization code to the browser that + // started the login as defence-in-depth. verifier, err := oauthPKCEVerifier() if err != nil { s.logger.Warn("OAuth: cannot mint PKCE verifier", "error", err) From 2b44f9045a475f59bc7eecf7ce77e5c8225f7acf Mon Sep 17 00:00:00 2001 From: Karibhou <157208370+Karibhou@users.noreply.github.com> Date: Wed, 28 Oct 2026 12:04:27 +0100 Subject: [PATCH 5/5] =?UTF-8?q?=F0=9F=90=9B=20fix(hub):=20verify=20PKCE=20?= =?UTF-8?q?in=20form=20exchange=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Karibhou <157208370+Karibhou@users.noreply.github.com> --- changelog.d/security-14633-hub-github-pkce.md | 2 +- src/pkg/hub/oauth_provision_coverage_test.go | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/changelog.d/security-14633-hub-github-pkce.md b/changelog.d/security-14633-hub-github-pkce.md index e30ff9a423..68c2b7adae 100644 --- a/changelog.d/security-14633-hub-github-pkce.md +++ b/changelog.d/security-14633-hub-github-pkce.md @@ -1 +1 @@ -- The Hub's GitHub browser login now uses PKCE (S256): as defence-in-depth to bind the authorization code to the browser that started the login, the authorize redirect carries a `code_challenge`, the matching `code_verifier` lives in a short-lived single-use HttpOnly cookie, and the token exchange is a form-encoded POST carrying it, so client secret, code and verifier no longer appear in the request URL. A callback with a missing or malformed verifier is rejected with 400 before any code exchange or session is minted; logins started before the upgrade must be restarted (#14633). +- The Hub's GitHub browser login now uses PKCE (S256) as defence-in-depth, binding the authorization code to the browser that started the login: the authorize redirect carries a `code_challenge`, the matching `code_verifier` lives in a short-lived single-use HttpOnly cookie and is sent in the token-exchange form body. A callback with a missing or malformed verifier is rejected with 400 before any code exchange or session is minted; logins started before the upgrade must be restarted (#14633). diff --git a/src/pkg/hub/oauth_provision_coverage_test.go b/src/pkg/hub/oauth_provision_coverage_test.go index 05b32c348e..36d2f154ba 100644 --- a/src/pkg/hub/oauth_provision_coverage_test.go +++ b/src/pkg/hub/oauth_provision_coverage_test.go @@ -479,7 +479,8 @@ func TestExchangeGitHubLoginSendsFormPost(t *testing.T) { s := &HubServer{logger: slog.Default()} rec := httptest.NewRecorder() - if _, _, _, ok := s.exchangeGitHubLogin(rec, "code-789"); !ok { + verifier := strings.Repeat("v", 43) + if _, _, _, ok := s.exchangeGitHubLogin(rec, "code-789", verifier); !ok { t.Fatalf("exchange failed: %d %s", rec.Code, rec.Body.String()) } if gotMethod != http.MethodPost { @@ -488,7 +489,7 @@ func TestExchangeGitHubLoginSendsFormPost(t *testing.T) { if gotQuery != "" { t.Errorf("token request URL query = %q, want empty", gotQuery) } - for _, secret := range []string{"cid-123", "sekret-456", "code-789"} { + for _, secret := range []string{"cid-123", "sekret-456", "code-789", verifier} { if strings.Contains(gotRawURL, secret) { t.Errorf("token request URL %q leaks %q", gotRawURL, secret) } @@ -504,6 +505,7 @@ func TestExchangeGitHubLoginSendsFormPost(t *testing.T) { "client_secret": "sekret-456", "code": "code-789", "redirect_uri": oauthRedirectURI(), + "code_verifier": verifier, } for k, v := range want { if got := gotForm.Get(k); got != v {