fix(auth): add PKCE to installed-app OAuth

CHANGELOG.md

@@ -21,6 +21,7 @@
 
 ### Fixed
 
+- Auth: bind browser, manual, remote, and account-manager OAuth exchanges with S256 PKCE; unfinished pre-PKCE manual flows must restart at step 1. (#693, #725) — thanks @TurboTheTurtle.
 - Docs: reset inherited text styles before applying Markdown find-replace formatting so leading bold spans and later inline styles stay paired correctly. (#735) — thanks @sebsnyk.
 - Docs: accept leading-dash Markdown list values in `docs cell-update --content` and reject nonempty Markdown that produces no editable cell text. (#733) — thanks @sebsnyk.
 - Docs: keep inline Markdown find-replace fragments inside their existing paragraph unless the replacement explicitly ends with a newline. (#736) — thanks @sebsnyk.

docs/auth-clients.md

@@ -64,6 +64,11 @@ Shows stored credential files plus any configured domain mappings.
 
 - Legacy `token:<email>` entries are copied to `token:default:<email>` the first time they are read.
 - Legacy `default_account` is still respected for the default client.
+- Browser, manual, remote, and account-manager authorization use S256 PKCE.
+  Manual state includes a short-lived verifier under the active `gog` config
+  directory. Keep the same `GOG_HOME` and `--client` between remote steps.
+- Manual or remote authorization started before v0.24.0 cannot be completed
+  after upgrading. Run step 1 again to generate a PKCE-bound URL.
 
 ## Workspace service accounts
 

docs/quickstart.md

@@ -64,6 +64,10 @@ refresh token in your OS keyring (Keychain on macOS, Secret Service on Linux,
 Credential Manager on Windows). Headless? Add `--manual` for a paste-the-URL
 flow, or `--remote --step 1`/`--step 2` for fully split server runs.
 
+Installed-app authorization uses S256 PKCE. Complete a manual or remote flow
+with the same `gog` home and client that generated its URL. After upgrading
+from a pre-PKCE release, restart any unfinished flow at step 1.
+
 Verify:
 
 ```bash

docs/spec.md

@@ -131,6 +131,10 @@ Implementation: `internal/secrets/store.go`.
 - Supports a remote/server-friendly 2-step manual flow:
   - Step 1 prints an auth URL (`gog auth add ... --remote --step 1`)
   - Step 2 exchanges the pasted redirect URL and requires `state` validation (`--remote --step 2 --auth-url ...`)
+  - Browser, manual, remote, and account-manager flows bind authorization
+    requests and token exchanges with S256 PKCE.
+  - Remote steps must share the same config home and OAuth client. Unfinished
+    pre-v0.24.0 flows must restart at step 1.
 - Refresh token issuance:
   - requests `access_type=offline`
   - supports `--force-consent` to force the consent prompt when Google doesn't return a refresh token
@@ -151,7 +155,7 @@ Scope selection note:
   - `credentials-<client>.json` (OAuth client id/secret; named clients)
 - State:
   - `state/gmail-watch/<account>.json` (Gmail watch state)
-  - `oauth-manual-state-<state>.json` (temporary manual OAuth state cache; expires quickly; no tokens)
+  - `oauth-manual-state-<state>.json` (temporary manual OAuth state and PKCE verifier cache; expires quickly; no tokens)
 - Secrets:
   - refresh tokens in keyring
 

internal/googleauth/accounts_server.go

@@ -58,7 +58,8 @@ type ManageServer struct {
 	fetchIdentity func(ctx context.Context, tok *oauth2.Token) (Identity, error)
 	oauthMu       sync.Mutex
 	oauthState    string
-	oauthStates   map[string]struct{}
+	oauthVerifier string
+	oauthStates   map[string]string
 	resultCh      chan error
 }
 
@@ -286,7 +287,8 @@ func (ms *ManageServer) handleAuthStart(w http.ResponseWriter, r *http.Request)
 		return
 	}
 
-	ms.addOAuthState(state)
+	codeVerifier := generateVerifierFn()
+	ms.addOAuthState(state, codeVerifier)
 
 	services := manageServices(ms.opts.Services)
 
@@ -306,7 +308,7 @@ func (ms *ManageServer) handleAuthStart(w http.ResponseWriter, r *http.Request)
 		Scopes:       scopes,
 	}
 
-	authURL := cfg.AuthCodeURL(state, authURLParams(ms.opts.ForceConsent, true)...)
+	authURL := cfg.AuthCodeURL(state, pkceAuthURLParams(ms.opts.ForceConsent, true, codeVerifier)...)
 	http.Redirect(w, r, authURL, http.StatusFound)
 }
 
@@ -340,7 +342,8 @@ func (ms *ManageServer) handleAuthUpgrade(w http.ResponseWriter, r *http.Request
 		return
 	}
 
-	ms.addOAuthState(state)
+	codeVerifier := generateVerifierFn()
+	ms.addOAuthState(state, codeVerifier)
 
 	// Use requested manage services (exclude Keep)
 	services := manageServices(ms.opts.Services)
@@ -364,7 +367,7 @@ func (ms *ManageServer) handleAuthUpgrade(w http.ResponseWriter, r *http.Request
 	// Always force consent for upgrades to ensure user sees all scopes
 	// Add login_hint to pre-select the account
 	authURL := cfg.AuthCodeURL(state,
-		append(authURLParams(true, true),
+		append(pkceAuthURLParams(true, true, codeVerifier),
 			oauth2.SetAuthURLParam("login_hint", email))...)
 
 	http.Redirect(w, r, authURL, http.StatusFound)
@@ -382,13 +385,21 @@ func (ms *ManageServer) handleOAuthCallback(w http.ResponseWriter, r *http.Reque
 		return
 	}
 
-	if !ms.consumeOAuthState(q.Get("state")) {
+	codeVerifier, ok := ms.consumeOAuthState(q.Get("state"))
+	if !ok {
 		w.WriteHeader(http.StatusBadRequest)
 		renderErrorPage(w, "State mismatch - possible CSRF attack. Please try again.")
 
 		return
 	}
 
+	if codeVerifier == "" {
+		w.WriteHeader(http.StatusBadRequest)
+		renderErrorPage(w, "Missing PKCE verifier. Please try again.")
+
+		return
+	}
+
 	code := q.Get("code")
 	if code == "" {
 		w.WriteHeader(http.StatusBadRequest)
@@ -428,7 +439,7 @@ func (ms *ManageServer) handleOAuthCallback(w http.ResponseWriter, r *http.Reque
 	ctx, cancel := context.WithTimeout(r.Context(), 30*time.Second)
 	defer cancel()
 
-	tok, err := cfg.Exchange(ctx, code)
+	tok, err := cfg.Exchange(ctx, code, oauth2.VerifierOption(codeVerifier))
 	if err != nil {
 		w.WriteHeader(http.StatusInternalServerError)
 		renderErrorPage(w, "Failed to exchange code for token: "+err.Error())
@@ -597,38 +608,48 @@ type defaultAccountDeleter interface {
 	DeleteDefaultAccount(client string) error
 }
 
-func (ms *ManageServer) addOAuthState(state string) {
+func (ms *ManageServer) addOAuthState(state string, codeVerifier string) {
 	ms.oauthMu.Lock()
 	defer ms.oauthMu.Unlock()
 
 	ms.oauthState = state
+	ms.oauthVerifier = codeVerifier
+
 	if ms.oauthStates == nil {
-		ms.oauthStates = make(map[string]struct{})
+		ms.oauthStates = make(map[string]string)
 	}
 
-	ms.oauthStates[state] = struct{}{}
+	ms.oauthStates[state] = codeVerifier
 }
 
-func (ms *ManageServer) consumeOAuthState(state string) bool {
+func (ms *ManageServer) consumeOAuthState(state string) (string, bool) {
 	ms.oauthMu.Lock()
 	defer ms.oauthMu.Unlock()
 
 	if ms.oauthStates != nil {
-		if _, ok := ms.oauthStates[state]; ok {
+		if codeVerifier, ok := ms.oauthStates[state]; ok {
 			delete(ms.oauthStates, state)
-			return true
+
+			if state == ms.oauthState {
+				ms.oauthState = ""
+				ms.oauthVerifier = ""
+			}
+
+			return codeVerifier, true
 		}
 
-		return false
+		return "", false
 	}
 
 	if state == "" || state != ms.oauthState {
-		return false
+		return "", false
 	}
 
 	ms.oauthState = ""
+	codeVerifier := ms.oauthVerifier
+	ms.oauthVerifier = ""
 
-	return true
+	return codeVerifier, true
 }
 
 func (ms *ManageServer) accountExists(email string) bool {

internal/googleauth/accounts_server_more_test.go

@@ -113,9 +113,10 @@ func TestManageServerHandleOAuthCallback_ReadCredsError(t *testing.T) {
 	t.Cleanup(func() { _ = ln.Close() })
 
 	ms := &ManageServer{
-		oauthState: "state1",
-		listener:   ln,
-		store:      &fakeStore{},
+		oauthState:    "state1",
+		oauthVerifier: testCodeVerifier,
+		listener:      ln,
+		store:         &fakeStore{},
 	}
 
 	rr := httptest.NewRecorder()
@@ -144,10 +145,11 @@ func TestManageServerHandleOAuthCallback_ScopesError(t *testing.T) {
 	t.Cleanup(func() { _ = ln.Close() })
 
 	ms := &ManageServer{
-		oauthState: "state1",
-		listener:   ln,
-		store:      &fakeStore{},
-		opts:       ManageServerOptions{Services: []Service{Service("nope")}},
+		oauthState:    "state1",
+		oauthVerifier: testCodeVerifier,
+		listener:      ln,
+		store:         &fakeStore{},
+		opts:          ManageServerOptions{Services: []Service{Service("nope")}},
 	}
 
 	rr := httptest.NewRecorder()
@@ -188,10 +190,11 @@ func TestManageServerHandleOAuthCallback_ExchangeError(t *testing.T) {
 	t.Cleanup(func() { _ = ln.Close() })
 
 	ms := &ManageServer{
-		oauthState: "state1",
-		listener:   ln,
-		store:      &fakeStore{},
-		opts:       ManageServerOptions{Services: []Service{ServiceGmail}},
+		oauthState:    "state1",
+		oauthVerifier: testCodeVerifier,
+		listener:      ln,
+		store:         &fakeStore{},
+		opts:          ManageServerOptions{Services: []Service{ServiceGmail}},
 	}
 
 	rr := httptest.NewRecorder()
@@ -237,10 +240,11 @@ func TestManageServerHandleOAuthCallback_MissingRefreshToken(t *testing.T) {
 	t.Cleanup(func() { _ = ln.Close() })
 
 	ms := &ManageServer{
-		oauthState: "state1",
-		listener:   ln,
-		store:      &fakeStore{},
-		opts:       ManageServerOptions{Services: []Service{ServiceGmail}},
+		oauthState:    "state1",
+		oauthVerifier: testCodeVerifier,
+		listener:      ln,
+		store:         &fakeStore{},
+		opts:          ManageServerOptions{Services: []Service{ServiceGmail}},
 	}
 
 	rr := httptest.NewRecorder()
@@ -287,9 +291,10 @@ func TestManageServerHandleOAuthCallback_FetchEmailError(t *testing.T) {
 	t.Cleanup(func() { _ = ln.Close() })
 
 	ms := &ManageServer{
-		oauthState: "state1",
-		listener:   ln,
-		store:      &fakeStore{},
+		oauthState:    "state1",
+		oauthVerifier: testCodeVerifier,
+		listener:      ln,
+		store:         &fakeStore{},
 		fetchIdentity: func(context.Context, *oauth2.Token) (Identity, error) {
 			return Identity{}, errTestStoreBoom
 		},