address OAuth PR review: conditional init, lint fixes, refresh concurrency

Author: capcom6

.env.example

@@ -279,14 +279,15 @@ OAUTH__CLIENT_ID=
 # Default: (empty - OAuth connection disabled)
 OAUTH__CLIENT_SECRET=
 
-# OAuth Token Encryption Key (required when OAuth connection is enabled)
+# OAuth Token Encryption Key (required only when OAuth connection is enabled)
 # Purpose: AES key used to encrypt the Bitbucket access and refresh tokens at
 #   rest in the oauth_tokens table (AES-GCM, 12-byte random nonce per value).
 # Format: 32 bytes (AES-256) encoded as hex or standard base64. Examples:
-#   base64: OAUTH__TOKEN_ENCRYPTION_KEY=<64-char base64 string>
+#   base64: OAUTH__TOKEN_ENCRYPTION_KEY=AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=
 #   hex:    OAUTH__TOKEN_ENCRYPTION_KEY=<64-char hex string>
-# Default: (empty - the service fails to start if OAuth connection is used)
+# Default: (empty - OAuth disabled)
 # SECURITY: Must be kept secret and unique per environment. Store in a secrets
 #   manager, never commit it. Rotating the key renders all existing stored
-#   tokens unreadable (treat as a disconnect).
+#   tokens unreadable (treat as a disconnect). When empty, the OAuth
+#   connection stays disabled and startup does not fail.
 OAUTH__TOKEN_ENCRYPTION_KEY=

internal/config/config.go

@@ -64,9 +64,9 @@ type webhooksConfig struct {
 }
 
 type oauthConfig struct {
-	ClientID             string `koanf:"client_id"`
-	ClientSecret         string `koanf:"client_secret"`
-	TokenEncryptionKey   string `koanf:"token_encryption_key"`
+	ClientID           string `koanf:"client_id"`
+	ClientSecret       string `koanf:"client_secret"`
+	TokenEncryptionKey string `koanf:"token_encryption_key"`
 }
 
 type Config struct {
@@ -142,8 +142,9 @@ func Default() Config {
 			},
 		},
 		OAuth: oauthConfig{
-			ClientID:     "",
-			ClientSecret: "",
+			ClientID:           "",
+			ClientSecret:       "",
+			TokenEncryptionKey: "",
 		},
 	}
 }

internal/config/module.go

@@ -84,9 +84,9 @@ func Module() fx.Option {
 			},
 			func(cfg Config) oauth.Config {
 				return oauth.Config{
-					ClientID:            cfg.OAuth.ClientID,
-					ClientSecret:        cfg.OAuth.ClientSecret,
-					TokenEncryptionKey:  cfg.OAuth.TokenEncryptionKey,
+					ClientID:           cfg.OAuth.ClientID,
+					ClientSecret:       cfg.OAuth.ClientSecret,
+					TokenEncryptionKey: cfg.OAuth.TokenEncryptionKey,
 				}
 			},
 		),

internal/db/migrations/20260825050007_oauth_tokens.sql

@@ -5,6 +5,7 @@ CREATE TABLE `oauth_tokens` (
     `user_id` BIGINT UNSIGNED NOT NULL,
     `access_token` TEXT NOT NULL,
     `refresh_token` TEXT NOT NULL,
+    `token_fingerprint` VARCHAR(64) NOT NULL,
     `scopes` VARCHAR(255) NOT NULL,
     `expires_at` DATETIME NOT NULL,
     `created_at` DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP,

internal/oauth/config.go

@@ -11,3 +11,10 @@ type Config struct {
 	// encrypt the stored OAuth access and refresh tokens at rest. Required.
 	TokenEncryptionKey string
 }
+
+// Enabled reports whether OAuth is fully configured. All of the Bitbucket
+// client credentials and the token encryption key must be present; otherwise
+// the OAuth connection stays disabled and startup must not fail.
+func (c Config) Enabled() bool {
+	return c.ClientID != "" && c.ClientSecret != "" && c.TokenEncryptionKey != ""
+}

internal/oauth/crypto.go

@@ -52,8 +52,8 @@ func NewEncryptorFromConfig(encoded string) (*Encryptor, error) {
 		return nil, fmt.Errorf("%w: encryption key is required", ErrInvalidKey)
 	}
 	for _, dec := range []func(string) ([]byte, error){
-		func(s string) ([]byte, error) { return base64.StdEncoding.DecodeString(s) },
 		hex.DecodeString,
+		func(s string) ([]byte, error) { return base64.StdEncoding.DecodeString(s) },
 	} {
 		if raw, derr := dec(encoded); derr == nil {
 			if enc, nerr := NewEncryptor(raw); nerr == nil {
@@ -86,7 +86,7 @@ func (e *Encryptor) Decrypt(ciphertext string) (string, error) {
 	}
 	raw, err := base64.StdEncoding.DecodeString(ciphertext)
 	if err != nil {
-		return "", fmt.Errorf("%w: %v", ErrInvalidCiphertext, err)
+		return "", fmt.Errorf("%w: %w", ErrInvalidCiphertext, err)
 	}
 	ns := e.gcm.NonceSize()
 	if len(raw) < ns {
@@ -95,7 +95,7 @@ func (e *Encryptor) Decrypt(ciphertext string) (string, error) {
 	nonce, ct := raw[:ns], raw[ns:]
 	plain, err := e.gcm.Open(nil, nonce, ct, nil)
 	if err != nil {
-		return "", fmt.Errorf("%w: %v", ErrInvalidCiphertext, err)
+		return "", fmt.Errorf("%w: %w", ErrInvalidCiphertext, err)
 	}
 	return string(plain), nil
 }

internal/oauth/crypto_test.go

@@ -1,13 +1,16 @@
-package oauth
+package oauth_test
 
 import (
+	"encoding/hex"
 	"strings"
 	"testing"
+
+	"github.com/bit-issues/backend/internal/oauth"
 )
 
-func testEncryptor(t *testing.T) *Encryptor {
+func testEncryptor(t *testing.T) *oauth.Encryptor {
 	t.Helper()
-	enc, err := NewEncryptorFromConfig("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=") // 32-byte base64
+	enc, err := oauth.NewEncryptorFromConfig("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=") // 32-byte base64
 	if err != nil {
 		t.Fatalf("failed to build encryptor: %v", err)
 	}
@@ -64,23 +67,55 @@ func TestEncryptor_TamperFails(t *testing.T) {
 	} else {
 		tampered = "A" + tampered[1:]
 	}
-	if _, err := enc.Decrypt(tampered); err == nil {
+	if _, derr := enc.Decrypt(tampered); derr == nil {
 		t.Fatalf("expected decryption failure on tampered ciphertext")
 	}
 }
 
 func TestNewEncryptorFromConfig_Errors(t *testing.T) {
-	if _, err := NewEncryptorFromConfig(""); err == nil {
+	if _, err := oauth.NewEncryptorFromConfig(""); err == nil {
 		t.Fatalf("expected error for empty key")
 	}
-	if _, err := NewEncryptorFromConfig("not-a-valid-key!!"); err == nil {
+	if _, err := oauth.NewEncryptorFromConfig("not-a-valid-key!!"); err == nil {
 		t.Fatalf("expected error for invalid key encoding")
 	}
-	if _, err := NewEncryptor([]byte("tooshort")); err == nil {
+	if _, err := oauth.NewEncryptor([]byte("tooshort")); err == nil {
 		t.Fatalf("expected error for short raw key")
 	}
 	// Hex 32-byte key is accepted.
-	if _, err := NewEncryptorFromConfig("000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f"); err != nil {
+	hexKey := "000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f"
+	if _, err := oauth.NewEncryptorFromConfig(hexKey); err != nil {
 		t.Fatalf("valid hex key rejected: %v", err)
 	}
 }
+
+func TestNewEncryptorFromConfig_HexKeyPrecedence(t *testing.T) {
+	// A 32-char hex string is also valid base64; ensure it is interpreted as
+	// raw hex bytes (16 bytes -> AES-128), not as 24 base64-decoded bytes.
+	hexKey := "000102030405060708090a0b0c0d0e0f"
+	enc, err := oauth.NewEncryptorFromConfig(hexKey)
+	if err != nil {
+		t.Fatalf("hex key rejected: %v", err)
+	}
+
+	raw, derr := hex.DecodeString(hexKey)
+	if derr != nil {
+		t.Fatalf("internal: hex decode failed: %v", derr)
+	}
+	direct, derr := oauth.NewEncryptor(raw)
+	if derr != nil {
+		t.Fatalf("internal: direct encryptor failed: %v", derr)
+	}
+
+	ct, eerr := enc.Encrypt("secret")
+	if eerr != nil {
+		t.Fatalf("encrypt failed: %v", eerr)
+	}
+	got, derr := direct.Decrypt(ct)
+	if derr != nil {
+		t.Fatalf("direct decrypt failed: %v", derr)
+	}
+	if got != "secret" {
+		t.Fatalf("hex key not interpreted as raw bytes: got %q want %q", got, "secret")
+	}
+}

internal/oauth/models.go

@@ -11,12 +11,13 @@ type tokenModel struct {
 	bun.BaseModel `bun:"table:oauth_tokens,alias:ot"`
 	db.TimedModel
 
-	ID           int64     `bun:"id,pk,autoincrement"`
-	UserID       int64     `bun:"user_id"`
-	AccessToken  string    `bun:"access_token,notnull"`
-	RefreshToken string    `bun:"refresh_token,notnull"`
-	Scopes       string    `bun:"scopes,notnull"`
-	ExpiresAt    time.Time `bun:"expires_at"`
+	ID               int64     `bun:"id,pk,autoincrement"`
+	UserID           int64     `bun:"user_id"`
+	AccessToken      string    `bun:"access_token,notnull"`
+	RefreshToken     string    `bun:"refresh_token,notnull"`
+	TokenFingerprint string    `bun:"token_fingerprint,notnull"`
+	Scopes           string    `bun:"scopes,notnull"`
+	ExpiresAt        time.Time `bun:"expires_at"`
 }
 
 func (t *tokenModel) toDomain() *Token {
@@ -39,11 +40,12 @@ func newTokenModel(userID int64, token *Token) *tokenModel {
 			UpdatedAt: token.UpdatedAt,
 		},
 
-		ID:           0,
-		UserID:       userID,
-		AccessToken:  token.AccessToken,
-		RefreshToken: token.RefreshToken,
-		Scopes:       token.Scopes,
-		ExpiresAt:    token.ExpiresAt,
+		ID:               0,
+		UserID:           userID,
+		AccessToken:      token.AccessToken,
+		RefreshToken:     token.RefreshToken,
+		TokenFingerprint: fingerprint(token.RefreshToken),
+		Scopes:           token.Scopes,
+		ExpiresAt:        token.ExpiresAt,
 	}
 }

internal/oauth/module.go

@@ -13,6 +13,11 @@ func Module() fx.Option {
 		logger.WithNamedLogger("oauth"),
 		fx.Provide(
 			func(cfg Config) (*Encryptor, error) {
+				if !cfg.Enabled() {
+					// OAuth is not configured: keep the connection disabled
+					// instead of failing startup over a missing encryption key.
+					return nil, nil
+				}
 				return NewEncryptorFromConfig(cfg.TokenEncryptionKey)
 			},
 			fx.Private,

internal/oauth/repository.go

@@ -2,7 +2,9 @@ package oauth
 
 import (
 	"context"
+	"crypto/sha256"
 	"database/sql"
+	"encoding/hex"
 	"errors"
 	"fmt"
 
@@ -34,7 +36,7 @@ func (r *Repository) Upsert(ctx context.Context, userID int64, token *Token) err
 	model.AccessToken = encAccess
 	model.RefreshToken = encRefresh
 
-	if _, err := r.db.NewInsert().Model(model).On("DUPLICATE KEY UPDATE").Exec(ctx); err != nil {
+	if _, err = r.db.NewInsert().Model(model).On("DUPLICATE KEY UPDATE").Exec(ctx); err != nil {
 		return fmt.Errorf("failed to upsert token: %w", err)
 	}
 
@@ -46,12 +48,11 @@ func (r *Repository) Upsert(ctx context.Context, userID int64, token *Token) err
 // no row matched, which happens if the token was deleted (or replaced) while
 // the refresh request was in flight.
 func (r *Repository) Update(ctx context.Context, userID int64, currentRefreshToken string, token *Token) (bool, error) {
-	// The stored refresh token is ciphertext, so the optimistic-concurrency
-	// match must compare against the encrypted current value.
-	encCurrentRefresh, err := r.enc.Encrypt(currentRefreshToken)
-	if err != nil {
-		return false, fmt.Errorf("failed to encrypt current refresh token: %w", err)
-	}
+	// The stored refresh token is ciphertext with a fresh random nonce per
+	// value, so it is never stable across calls. Use a deterministic fingerprint
+	// of the plaintext refresh token for optimistic concurrency instead.
+	currentFingerprint := fingerprint(currentRefreshToken)
+
 	encAccess, err := r.enc.Encrypt(token.AccessToken)
 	if err != nil {
 		return false, fmt.Errorf("failed to encrypt access token: %w", err)
@@ -65,11 +66,12 @@ func (r *Repository) Update(ctx context.Context, userID int64, currentRefreshTok
 		Model((*tokenModel)(nil)).
 		Set("access_token = ?", encAccess).
 		Set("refresh_token = ?", encRefresh).
+		Set("token_fingerprint = ?", fingerprint(token.RefreshToken)).
 		Set("scopes = ?", token.Scopes).
 		Set("expires_at = ?", token.ExpiresAt).
 		Set("updated_at = ?", token.UpdatedAt).
 		Where("user_id = ?", userID).
-		Where("refresh_token = ?", encCurrentRefresh).
+		Where("token_fingerprint = ?", currentFingerprint).
 		Exec(ctx)
 	if err != nil {
 		return false, fmt.Errorf("failed to update token: %w", err)
@@ -118,3 +120,11 @@ func (r *Repository) Delete(ctx context.Context, userID int64) error {
 	}
 	return nil
 }
+
+// fingerprint returns a stable SHA-256 hex digest of the plaintext refresh
+// token. Because AES-GCM seals with a random nonce, the stored ciphertext is
+// not suitable for optimistic-concurrency checks; the fingerprint is.
+func fingerprint(s string) string {
+	sum := sha256.Sum256([]byte(s))
+	return hex.EncodeToString(sum[:])
+}