fix(security): OAuth state CSRF + log redaction + ownership & rate-limit hardening (#27)

Author: tirthpatell

.github/workflows/ci.yml

@@ -70,7 +70,7 @@ jobs:
 
     - name: Update coverage badge
       if: github.ref == 'refs/heads/main' && matrix.go-version == '1.24'
-      uses: schneegans/dynamic-badges-action@v1.7.0
+      uses: schneegans/dynamic-badges-action@v1.8.0
       with:
         auth: ${{ secrets.GIST_TOKEN }}
         gistID: 2c608589294aed9aa900256daeec0fd4
@@ -83,7 +83,7 @@ jobs:
 
     - name: Upload coverage to Codecov
       if: matrix.go-version == '1.24'
-      uses: codecov/codecov-action@v5
+      uses: codecov/codecov-action@v6
       with:
         files: coverage.out
         fail_ci_if_error: false

README.md

@@ -57,13 +57,21 @@ config := &threads.Config{
 
 client, err := threads.NewClient(config)
 
-// Get authorization URL
-authURL := client.GetAuthURL(config.Scopes)
-// Redirect user to authURL
+// Get authorization URL. Persist state in the user's session (signed cookie,
+// server-side session, etc.) — it MUST be compared against the state query
+// parameter on the OAuth callback to prevent CSRF / authorization-code
+// fixation (RFC 6749 §10.12, OAuth 2.0 Security BCP §4.7).
+authURL, state, err := client.GetAuthURL(config.Scopes)
+if err != nil {
+    log.Fatal(err)
+}
+// Redirect user to authURL; on the callback, read ?code=... and ?state=...
 
-// Exchange authorization code for token
+// Exchange authorization code for token. The client refuses the exchange if
+// the expected state does not match the state echoed on the callback
+// (constant-time compare).
 ctx := context.Background()
-err = client.ExchangeCodeForToken(ctx, "auth-code-from-callback")
+err = client.ExchangeCodeForToken(ctx, "auth-code-from-callback", state, receivedState)
 err = client.GetLongLivedToken(ctx) // Convert to long-lived token
 ```
 

auth.go

@@ -3,6 +3,7 @@ package threads
 import (
 	"context"
 	"crypto/rand"
+	"crypto/subtle"
 	"encoding/base64"
 	"encoding/json"
 	"fmt"
@@ -48,19 +49,26 @@ func generateState() (string, error) {
 	return base64.URLEncoding.EncodeToString(b), nil
 }
 
-// GetAuthURL generates the authorization URL for OAuth 2.0 flow.
-// Users should be redirected to this URL to grant permissions to your app.
-// If scopes are not provided, defaults to threads_basic and threads_content_publish.
-// Returns the complete authorization URL including all necessary parameters.
-func (c *Client) GetAuthURL(scopes []string) string {
+// GetAuthURL generates the authorization URL for the OAuth 2.0 flow along with
+// the random state parameter embedded in it. Callers MUST persist the returned
+// state in the user's session (e.g. a signed cookie) and pass it as
+// expectedState to ExchangeCodeForToken when the provider redirects back,
+// comparing it against the state echoed on the callback. This is required by
+// RFC 6749 §10.12 and OAuth 2.0 Security BCP §4.7 to prevent login/CSRF and
+// authorization-code fixation attacks.
+//
+// If scopes are not provided, defaults to threads_basic and
+// threads_content_publish. Returns an error if the system's secure random
+// source is unavailable; in that case no URL is returned so callers cannot
+// accidentally issue a flow with a guessable state (fail closed).
+func (c *Client) GetAuthURL(scopes []string) (authURL, state string, err error) {
 	if len(scopes) == 0 {
 		scopes = []string{"threads_basic", "threads_content_publish"}
 	}
 
-	state, err := generateState()
+	state, err = generateState()
 	if err != nil {
-		// If we can't generate state, use a simple timestamp-based fallback
-		state = fmt.Sprintf("state_%d", time.Now().Unix())
+		return "", "", err
 	}
 
 	params := url.Values{
@@ -71,18 +79,34 @@ func (c *Client) GetAuthURL(scopes []string) string {
 		"state":         {state},
 	}
 
-	authURL := fmt.Sprintf("https://www.threads.net/oauth/authorize?%s", params.Encode())
-	return authURL
+	authURL = fmt.Sprintf("https://www.threads.net/oauth/authorize?%s", params.Encode())
+	return authURL, state, nil
 }
 
 // ExchangeCodeForToken exchanges an authorization code for an access token.
-// This should be called after the user authorizes your app, and you receive the code
-// from the redirect URI callback. The resulting token is automatically stored
-// in the client and token storage.
-func (c *Client) ExchangeCodeForToken(ctx context.Context, code string) error {
+// This should be called after the user authorizes your app and the provider
+// redirects back with a code and state.
+//
+// expectedState is the state value that was returned by GetAuthURL and
+// persisted in the user's session; receivedState is the state query parameter
+// echoed on the callback. Both must be non-empty and must match (compared in
+// constant time). A mismatch indicates an OAuth CSRF / authorization-code
+// fixation attack and the exchange is refused before any network call.
+//
+// The resulting token is automatically stored in the client and token storage.
+func (c *Client) ExchangeCodeForToken(ctx context.Context, code, expectedState, receivedState string) error {
 	if code == "" {
 		return NewValidationError(400, "Authorization code is required", "Code parameter cannot be empty", "code")
 	}
+	if expectedState == "" {
+		return NewValidationError(400, "Expected state is required", "expectedState must be the value returned by GetAuthURL and persisted in the user's session", "expected_state")
+	}
+	if receivedState == "" {
+		return NewValidationError(400, "Received state is required", "receivedState must be the state query parameter echoed by the provider on the callback", "received_state")
+	}
+	if subtle.ConstantTimeCompare([]byte(expectedState), []byte(receivedState)) != 1 {
+		return NewAuthenticationError(400, "OAuth state mismatch", "The state parameter returned by the provider does not match the one issued by GetAuthURL; possible CSRF/code-fixation attempt")
+	}
 
 	data := url.Values{
 		"client_id":     {c.config.ClientID},

auth_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"strings"
 	"testing"
 	"time"
@@ -41,7 +42,7 @@ func TestExchangeCodeForToken_Success(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	err = client.ExchangeCodeForToken(context.Background(), "auth_code_123")
+	err = client.ExchangeCodeForToken(context.Background(), "auth_code_123", "state_abc", "state_abc")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -66,7 +67,7 @@ func TestExchangeCodeForToken_EmptyCode(t *testing.T) {
 	config.SetDefaults()
 	client, _ := NewClient(config)
 
-	err := client.ExchangeCodeForToken(context.Background(), "")
+	err := client.ExchangeCodeForToken(context.Background(), "", "state", "state")
 	if err == nil {
 		t.Fatal("expected error for empty code")
 	}
@@ -91,7 +92,7 @@ func TestExchangeCodeForToken_ServerError(t *testing.T) {
 	config.BaseURL = server.URL
 
 	client, _ := NewClient(config)
-	err := client.ExchangeCodeForToken(context.Background(), "bad_code")
+	err := client.ExchangeCodeForToken(context.Background(), "bad_code", "state", "state")
 	if err == nil {
 		t.Fatal("expected error for server error response")
 	}
@@ -116,7 +117,7 @@ func TestExchangeCodeForToken_NoExpiresIn(t *testing.T) {
 	config.BaseURL = server.URL
 
 	client, _ := NewClient(config)
-	err := client.ExchangeCodeForToken(context.Background(), "code")
+	err := client.ExchangeCodeForToken(context.Background(), "code", "state", "state")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -477,15 +478,26 @@ func TestGetAuthURL_ContainsRequiredParams(t *testing.T) {
 	config.SetDefaults()
 	client, _ := NewClient(config)
 
-	authURL := client.GetAuthURL([]string{"threads_basic"})
+	authURL, state, err := client.GetAuthURL([]string{"threads_basic"})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
 	if authURL == "" {
 		t.Fatal("expected non-empty auth URL")
 	}
+	if state == "" {
+		t.Fatal("expected non-empty state; callers cannot enforce CSRF protection without it")
+	}
 	for _, param := range []string{"client_id=my-app-id", "response_type=code", "scope=threads_basic"} {
 		if !strings.Contains(authURL, param) {
 			t.Errorf("expected auth URL to contain %q, got %s", param, authURL)
 		}
 	}
+	// The embedded state must be the state value returned to the caller,
+	// so the caller can compare it against the callback.
+	if !strings.Contains(authURL, "state="+url.QueryEscape(state)) {
+		t.Errorf("expected auth URL to embed the returned state %q, got %s", state, authURL)
+	}
 }
 
 func TestGetAuthURL_DefaultScopes(t *testing.T) {
@@ -497,12 +509,42 @@ func TestGetAuthURL_DefaultScopes(t *testing.T) {
 	config.SetDefaults()
 	client, _ := NewClient(config)
 
-	authURL := client.GetAuthURL(nil)
+	authURL, _, err := client.GetAuthURL(nil)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
 	if !strings.Contains(authURL, "threads_basic") {
 		t.Error("expected default scope threads_basic in auth URL")
 	}
 }
 
+// TestGetAuthURL_UniqueState: state must be unpredictable — a guessable
+// state neutralises CSRF protection.
+func TestGetAuthURL_UniqueState(t *testing.T) {
+	config := &Config{
+		ClientID:     "my-app-id",
+		ClientSecret: "secret",
+		RedirectURI:  "https://example.com/callback",
+	}
+	config.SetDefaults()
+	client, _ := NewClient(config)
+
+	_, s1, err := client.GetAuthURL(nil)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	_, s2, err := client.GetAuthURL(nil)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if s1 == s2 {
+		t.Fatal("GetAuthURL must produce a fresh state on each call")
+	}
+	if len(s1) < 32 {
+		t.Errorf("state looks too short to be high-entropy: len=%d", len(s1))
+	}
+}
+
 func TestExchangeCodeForToken_WithLogger(t *testing.T) {
 	handler := func(w http.ResponseWriter, r *http.Request) {
 		w.Header().Set("Content-Type", "application/json")
@@ -522,12 +564,86 @@ func TestExchangeCodeForToken_WithLogger(t *testing.T) {
 	config.BaseURL = server.URL
 
 	client, _ := NewClient(config)
-	err := client.ExchangeCodeForToken(context.Background(), "code")
+	err := client.ExchangeCodeForToken(context.Background(), "code", "state", "state")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
 }
 
+// TestExchangeCodeForToken_StateMismatch asserts the core CSRF protection:
+// when the state echoed on the callback does not match the state persisted
+// by the caller (from GetAuthURL), ExchangeCodeForToken must refuse the
+// exchange BEFORE hitting the token endpoint, so no attacker-controlled code
+// can be redeemed into the victim's session.
+func TestExchangeCodeForToken_StateMismatch(t *testing.T) {
+	called := false
+	handler := func(w http.ResponseWriter, r *http.Request) {
+		called = true
+		w.WriteHeader(200)
+	}
+	server := httptest.NewServer(http.HandlerFunc(handler))
+	t.Cleanup(server.Close)
+
+	config := &Config{
+		ClientID:     "test-id",
+		ClientSecret: "test-secret",
+		RedirectURI:  "https://example.com/callback",
+	}
+	config.SetDefaults()
+	config.BaseURL = server.URL
+
+	client, _ := NewClient(config)
+	err := client.ExchangeCodeForToken(context.Background(), "code", "expected-state", "attacker-chosen-state")
+	if err == nil {
+		t.Fatal("expected error for state mismatch")
+	}
+	if !IsAuthenticationError(err) {
+		t.Errorf("expected AuthenticationError (CSRF), got %T: %v", err, err)
+	}
+	if called {
+		t.Error("token endpoint must not be called when state mismatches")
+	}
+	if client.IsAuthenticated() {
+		t.Error("client must not become authenticated when state mismatches")
+	}
+}
+
+func TestExchangeCodeForToken_EmptyExpectedState(t *testing.T) {
+	config := &Config{
+		ClientID:     "test-id",
+		ClientSecret: "test-secret",
+		RedirectURI:  "https://example.com/callback",
+	}
+	config.SetDefaults()
+	client, _ := NewClient(config)
+
+	err := client.ExchangeCodeForToken(context.Background(), "code", "", "anything")
+	if err == nil {
+		t.Fatal("expected error when expectedState is empty (defeats CSRF check)")
+	}
+	if !IsValidationError(err) {
+		t.Errorf("expected ValidationError, got %T", err)
+	}
+}
+
+func TestExchangeCodeForToken_EmptyReceivedState(t *testing.T) {
+	config := &Config{
+		ClientID:     "test-id",
+		ClientSecret: "test-secret",
+		RedirectURI:  "https://example.com/callback",
+	}
+	config.SetDefaults()
+	client, _ := NewClient(config)
+
+	err := client.ExchangeCodeForToken(context.Background(), "code", "expected", "")
+	if err == nil {
+		t.Fatal("expected error when receivedState is empty")
+	}
+	if !IsValidationError(err) {
+		t.Errorf("expected ValidationError, got %T", err)
+	}
+}
+
 func TestGetLongLivedToken_NoToken(t *testing.T) {
 	config := &Config{
 		ClientID:     "test-id",