merge queue: checking main (ea8d6fb) and #3460 together

internal/generator/auth_oauth_pkce_test.go

@@ -0,0 +1,120 @@
+package generator
+
+import (
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/mvanhorn/cli-printing-press/v4/internal/spec"
+	"github.com/stretchr/testify/require"
+)
+
+func TestOAuthLoginUsesPKCEWhenNoClientSecret(t *testing.T) {
+	t.Parallel()
+
+	apiSpec := minimalSpec("oauth-pkce-login")
+	apiSpec.Auth = spec.AuthConfig{
+		Type:             "oauth2",
+		Header:           "Authorization",
+		Format:           "Bearer {token}",
+		OAuth2Grant:      spec.OAuth2GrantAuthorizationCode,
+		AuthorizationURL: "https://accounts.example.com/oauth/authorize",
+		TokenURL:         "https://accounts.example.com/oauth/token",
+		KeyURL:           "https://console.example.com/oauth",
+	}
+
+	outputDir := filepath.Join(t.TempDir(), "oauth-pkce-login-pp-cli")
+	require.NoError(t, New(apiSpec, outputDir).Generate())
+
+	authSrc, err := os.ReadFile(filepath.Join(outputDir, "internal", "cli", "auth.go"))
+	require.NoError(t, err)
+	auth := string(authSrc)
+
+	// Authorize request carries an S256 challenge on the secretless path.
+	require.Contains(t, auth, "codeVerifier, err = generatePKCEVerifier()")
+	require.Contains(t, auth, `params.Set("code_challenge", pkceCodeChallengeS256(codeVerifier))`)
+	require.Contains(t, auth, `params.Set("code_challenge_method", "S256")`)
+
+	// Token exchange authenticates with exactly one of client_secret or
+	// code_verifier (mutual exclusivity is validated behaviorally below);
+	// the bare authorization_code grant is gone.
+	require.Contains(t, auth, `tokenParams.Set("client_secret", clientSecret)`)
+	require.Contains(t, auth, `tokenParams.Set("code_verifier", codeVerifier)`)
+
+	// RFC 8252 loopback redirect: IP literal, never "localhost".
+	require.Contains(t, auth, `redirectURI := fmt.Sprintf("http://127.0.0.1:%d/callback"`)
+	require.NotContains(t, auth, `http://localhost:%d/callback`)
+
+	binPath := filepath.Join(outputDir, "oauth-pkce-login-pp-cli")
+	runGoCommand(t, outputDir, "build", "-o", binPath, "./cmd/oauth-pkce-login-pp-cli")
+
+	// Under verify-env the command prints the authorize URL it would launch;
+	// use that to assert live flag→params behavior without opening a browser
+	// or binding assumptions about the callback port.
+	runVerifyLogin := func(extraArgs ...string) string {
+		args := append([]string{"--config", filepath.Join(t.TempDir(), "config.toml"), "login", "--client-id", "test-client"}, extraArgs...)
+		cmd := exec.Command(binPath, args...)
+		cmd.Env = append(os.Environ(), "PRINTING_PRESS_VERIFY=1")
+		out, err := cmd.CombinedOutput()
+		require.NoError(t, err, "verify-env login failed: %s", string(out))
+		return string(out)
+	}
+
+	secretless := runVerifyLogin()
+	require.Contains(t, secretless, "would launch:")
+	require.Contains(t, secretless, "code_challenge=")
+	require.Contains(t, secretless, "code_challenge_method=S256")
+	require.Contains(t, secretless, "127.0.0.1%3A8085%2Fcallback")
+	require.NotContains(t, secretless, "localhost")
+
+	withSecret := runVerifyLogin("--client-secret", "test-secret")
+	require.Contains(t, withSecret, "would launch:")
+	require.NotContains(t, withSecret, "code_challenge")
+
+	const runtimeTest = `package cli
+
+import (
+	"strings"
+	"testing"
+)
+
+// Vector from RFC 7636 appendix B.
+func TestPKCECodeChallengeS256MatchesRFC7636Vector(t *testing.T) {
+	const verifier = "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk"
+	const want = "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM"
+	if got := pkceCodeChallengeS256(verifier); got != want {
+		t.Fatalf("pkceCodeChallengeS256() = %q, want %q", got, want)
+	}
+}
+
+func TestGeneratePKCEVerifierShapeAndUniqueness(t *testing.T) {
+	const unreserved = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~"
+	first, err := generatePKCEVerifier()
+	if err != nil {
+		t.Fatalf("generatePKCEVerifier() error = %v", err)
+	}
+	if len(first) != 43 {
+		t.Fatalf("verifier length = %d, want 43", len(first))
+	}
+	for _, r := range first {
+		if !strings.ContainsRune(unreserved, r) {
+			t.Fatalf("verifier contains %q, outside RFC 7636 unreserved set", r)
+		}
+	}
+	second, err := generatePKCEVerifier()
+	if err != nil {
+		t.Fatalf("generatePKCEVerifier() second call error = %v", err)
+	}
+	if first == second {
+		t.Fatalf("two verifiers are identical: %q", first)
+	}
+}
+`
+	require.NoError(t, os.WriteFile(filepath.Join(outputDir, "internal", "cli", "oauth_pkce_test.go"), []byte(runtimeTest), 0o644))
+	runGoCommand(t, outputDir, "test", "-race", "./internal/cli", "-run", "Test(PKCECodeChallengeS256|GeneratePKCEVerifier)")
+
+	require.Contains(t, auth, "http.NewRequestWithContext(cmd.Context(), http.MethodPost, tokenURL", "token exchange should be context-cancellable")
+	require.False(t, strings.Contains(auth, "http.PostForm(tokenURL"), "token exchange should use a client with its own timeout, not the shared default client")
+}

internal/generator/templates/auth.go.tmpl

@@ -7,6 +7,8 @@ import (
 	"bufio"
 	"context"
 	"crypto/rand"
+	"crypto/sha256"
+	"encoding/base64"
 	"encoding/hex"
 	"encoding/json"
 	"fmt"
@@ -153,6 +155,18 @@ func runOAuthLogin(cmd *cobra.Command, flags *rootFlags, clientID, clientSecret
 	}
 	state := hex.EncodeToString(stateBytes)
 
+	// RFC 7636 PKCE: with no client secret this is a public client, and a
+	// bare authorization_code grant reaches the token endpoint with no
+	// client authentication at all — providers reject that. When a secret
+	// is configured, keep the plain Authorization Code flow unchanged.
+	codeVerifier := ""
+	if clientSecret == "" {
+		codeVerifier, err = generatePKCEVerifier()
+		if err != nil {
+			return err
+		}
+	}
+
 	authURL := ""
 {{- if .Auth.AuthorizationURL}}
 	authURL = cfg.AuthorizationURL
@@ -166,6 +180,10 @@ func runOAuthLogin(cmd *cobra.Command, flags *rootFlags, clientID, clientSecret
 		"response_type": {"code"},
 		"state":         {state},
 	}
+	if codeVerifier != "" {
+		params.Set("code_challenge", pkceCodeChallengeS256(codeVerifier))
+		params.Set("code_challenge_method", "S256")
+	}
 {{- if eq $refreshMech.Kind "query"}}
 	params.Set({{printf "%q" $refreshMech.Key}}, {{printf "%q" $refreshMech.Value}})
 {{- end}}
@@ -184,7 +202,7 @@ func runOAuthLogin(cmd *cobra.Command, flags *rootFlags, clientID, clientSecret
 	// derived from the configured --port; the live flow below uses the actual
 	// bound port, which the OS assigns when --port 0 is passed.
 	if cliutil.IsVerifyEnv() {
-		params.Set("redirect_uri", fmt.Sprintf("http://localhost:%d/callback", port))
+		params.Set("redirect_uri", fmt.Sprintf("http://127.0.0.1:%d/callback", port))
 		fmt.Fprintf(w, "would launch: %s\n", authURL+"?"+params.Encode())
 		return nil
 	}
@@ -197,8 +215,10 @@ func runOAuthLogin(cmd *cobra.Command, flags *rootFlags, clientID, clientSecret
 
 	// Derive the redirect URI from the live listener address so an OS-assigned
 	// ephemeral port (--port 0) is reflected in both the auth request and the
-	// token exchange below.
-	redirectURI := fmt.Sprintf("http://localhost:%d/callback", listener.Addr().(*net.TCPAddr).Port)
+	// token exchange below. Use 127.0.0.1, not "localhost": RFC 8252 §7.3
+	// prescribes the loopback IP literal, and some providers reject
+	// "localhost" redirect URIs outright.
+	redirectURI := fmt.Sprintf("http://127.0.0.1:%d/callback", listener.Addr().(*net.TCPAddr).Port)
 	params.Set("redirect_uri", redirectURI)
 	fullURL := authURL + "?" + params.Encode()
 
@@ -261,9 +281,20 @@ func runOAuthLogin(cmd *cobra.Command, flags *rootFlags, clientID, clientSecret
 	}
 	if clientSecret != "" {
 		tokenParams.Set("client_secret", clientSecret)
+	} else {
+		tokenParams.Set("code_verifier", codeVerifier)
 	}
 
-	resp, err := http.PostForm(tokenURL, tokenParams)
+	// The browser-wait timeout above does not apply here; the token exchange
+	// is a plain server-to-server POST and gets its own short network timeout,
+	// while cmd.Context() keeps it cancellable from the terminal.
+	tokenReq, err := http.NewRequestWithContext(cmd.Context(), http.MethodPost, tokenURL, strings.NewReader(tokenParams.Encode()))
+	if err != nil {
+		return fmt.Errorf("building token request: %w", err)
+	}
+	tokenReq.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+	tokenClient := &http.Client{Timeout: 30 * time.Second}
+	resp, err := tokenClient.Do(tokenReq)
 	if err != nil {
 		return fmt.Errorf("exchanging code for token: %w", err)
 	}
@@ -446,6 +477,24 @@ func newAuthLogoutCmd(flags *rootFlags) *cobra.Command {
 	}
 }
 
+// generatePKCEVerifier returns a fresh RFC 7636 code verifier: 32 bytes of
+// CSPRNG output, base64url-encoded without padding (43 chars, within the
+// 43-128 range the RFC requires).
+func generatePKCEVerifier() (string, error) {
+	verifierBytes := make([]byte, 32)
+	if _, err := rand.Read(verifierBytes); err != nil {
+		return "", fmt.Errorf("generating PKCE verifier: %w", err)
+	}
+	return base64.RawURLEncoding.EncodeToString(verifierBytes), nil
+}
+
+// pkceCodeChallengeS256 derives the S256 code challenge for a verifier:
+// base64url(SHA-256(verifier)) without padding.
+func pkceCodeChallengeS256(verifier string) string {
+	sum := sha256.Sum256([]byte(verifier))
+	return base64.RawURLEncoding.EncodeToString(sum[:])
+}
+
 func openBrowser(url string) {
 	var cmd *exec.Cmd
 	switch runtime.GOOS {

testdata/golden/cases/generate-golden-api-oauth2-authcode/artifacts.txt

@@ -0,0 +1,19 @@
+# Locks the OAuth2 authorization_code + PKCE auth shape contract:
+#
+#   parser.go    → AuthConfig.OAuth2Grant=authorization_code,
+#                  AuthorizationURL + TokenURL set
+#   generator.go → auth.go.tmpl selected (browser loopback login)
+#   auth.go.tmpl → RFC 8252 loopback redirect (127.0.0.1, not localhost),
+#                  PKCE on the secretless path: generatePKCEVerifier /
+#                  pkceCodeChallengeS256 / code_challenge_method=S256 on
+#                  the authorize request, and exactly one of client_secret
+#                  or code_verifier on the token exchange
+#
+# If a refactor regresses template selection or drops the PKCE branch,
+# auth.go either disappears or loses the PKCE helpers and the harness
+# fails with a content diff.
+printing-press-oauth2-authcode/internal/cli/auth.go
+
+# client.go carries refreshAccessToken, which must keep the public-client
+# refresh shape (client_id always sent, client_secret only when present).
+printing-press-oauth2-authcode/internal/client/client.go

testdata/golden/cases/generate-golden-api-oauth2-authcode/command.txt

@@ -0,0 +1,2 @@
+set -euo pipefail
+"$BINARY" generate --spec testdata/golden/fixtures/golden-api-oauth2-authcode.yaml --output "$CASE_ACTUAL_DIR/printing-press-oauth2-authcode" --force --spec-url file://testdata/golden/fixtures/golden-api-oauth2-authcode.yaml --validate=false

testdata/golden/expected/generate-golden-api-oauth2-authcode/exit.txt

@@ -0,0 +1 @@
+0