From ea70edd14838b6868ab69d2bad87f2e9c3a918b3 Mon Sep 17 00:00:00 2001 From: Bretton Date: Wed, 18 Feb 2026 20:06:54 -0800 Subject: [PATCH] feat(oauth): add web frontend dev proxy and generalize redirect URI handling Refactors the mobile-only OAuth redirect URI system into a configurable allowlist that supports both mobile apps (custom schemes + Universal Links) and web clients (HTTPS redirects). Adds Caddy-based reverse proxy for web frontend development, enabling same-origin cookie sharing between Vite frontend and Coves backend. Changes: - Refactor isAllowedMobileRedirectURI() into OAuthHandler.isAllowedRedirectURI() with BuildAllowedRedirectURIs() builder for the configurable allowlist - Add smart redirect: HTTPS clients get direct HTTP redirect, custom scheme clients get the intermediate redirect page - Add Caddyfile.dev reverse proxy config (Vite :5173 + Coves :8081 on :8080) - Add scripts/web-dev-run.sh for combined backend+frontend dev startup - Add Makefile targets: run-web, web-proxy, web-proxy-bg, web-proxy-stop - Auto-run db-migrate before server start in make run and make run-web - Update OAuth client comments to clarify ATProto loopback client_id spec - Add PAR request debug logging in dev auth resolver - Update all security tests to use OAuthHandler instance methods Co-Authored-By: Claude Opus 4.6 --- Caddyfile.dev | 41 +++ Makefile | 37 ++ cmd/server/main.go | 1 + internal/atproto/oauth/client.go | 7 +- internal/atproto/oauth/dev_auth_resolver.go | 11 +- internal/atproto/oauth/handlers.go | 83 +++-- internal/atproto/oauth/handlers_security.go | 27 +- .../atproto/oauth/handlers_security_test.go | 319 ++++++++++-------- internal/atproto/oauth/handlers_test.go | 4 +- scripts/web-dev-run.sh | 77 +++++ 10 files changed, 415 insertions(+), 192 deletions(-) create mode 100644 Caddyfile.dev create mode 100755 scripts/web-dev-run.sh diff --git a/Caddyfile.dev b/Caddyfile.dev new file mode 100644 index 0000000..6c19d9a --- /dev/null +++ b/Caddyfile.dev @@ -0,0 +1,41 @@ +# Coves Web Development Reverse Proxy +# Combines Vite frontend (5173) and Coves backend (8081) on single origin (8080) +# This enables OAuth cookies to work correctly across frontend/backend +# +# Usage: +# make web-dev # Starts proxy (also starts dev stack if needed) +# Or manually: caddy run --config Caddyfile.dev +# +# Access at: http://localhost:8080 + +:8080 { + # OAuth routes -> Coves backend + handle /oauth/* { + reverse_proxy 127.0.0.1:8081 + } + + # XRPC API routes -> Coves backend + handle /xrpc/* { + reverse_proxy 127.0.0.1:8081 + } + + # OAuth client metadata -> Coves backend + handle /oauth-client-metadata.json { + reverse_proxy 127.0.0.1:8081 + } + + # Image proxy routes -> Coves backend + handle /img/* { + reverse_proxy 127.0.0.1:8081 + } + + # API routes (if any) -> Coves backend + handle /api/* { + reverse_proxy 127.0.0.1:8081 + } + + # Everything else -> Vite dev server (frontend) + handle { + reverse_proxy localhost:5173 + } +} diff --git a/Makefile b/Makefile index c7ade29..8cc5cd0 100644 --- a/Makefile +++ b/Makefile @@ -265,6 +265,7 @@ build-dev: ## Build the Coves server with dev mode (includes localhost OAuth res @echo "$(GREEN)✓ Build complete: ./server (with dev tags)$(RESET)" run: ## Run the Coves server with dev environment (requires database running) + @make db-migrate @./scripts/dev-run.sh ##@ Cleanup @@ -333,6 +334,42 @@ ngrok-up: ## Start ngrok tunnels (for iOS or WiFi testing - requires paid plan f ngrok-down: ## Stop all ngrok tunnels @./scripts/stop-ngrok.sh +##@ Web Frontend Development + +run-web: ## Run Coves backend configured for web frontend dev (OAuth via :8080 proxy) + @make db-migrate + @./scripts/web-dev-run.sh + +web-proxy: ## Start Caddy reverse proxy for web frontend dev (combines Vite + Coves on :8080) + @echo "$(CYAN)Starting web development proxy...$(RESET)" + @echo "" + @echo "$(YELLOW)Prerequisites:$(RESET)" + @echo " 1. Coves backend running on :8081 (make run)" + @echo " 2. Vite frontend running on :5173 (cd frontend && npm run dev)" + @echo "" + @command -v caddy >/dev/null 2>&1 || { echo "$(RED)Error: Caddy not installed. Install with:$(RESET)"; \ + echo " Ubuntu/Debian: sudo apt install caddy"; \ + echo " macOS: brew install caddy"; \ + echo " Or see: https://caddyserver.com/docs/install"; \ + exit 1; } + @echo "$(GREEN)Starting Caddy on http://localhost:8080$(RESET)" + @echo " Backend routes (/oauth/*, /xrpc/*, /api/*) -> 127.0.0.1:8081" + @echo " Frontend routes (everything else) -> localhost:5173" + @echo "" + @echo "$(CYAN)Access your app at: http://localhost:8080$(RESET)" + @echo "$(CYAN)Press Ctrl+C to stop$(RESET)" + @echo "" + @caddy run --config Caddyfile.dev + +web-proxy-bg: ## Start Caddy proxy in background + @command -v caddy >/dev/null 2>&1 || { echo "$(RED)Error: Caddy not installed$(RESET)"; exit 1; } + @caddy start --config Caddyfile.dev + @echo "$(GREEN)✓ Caddy proxy started in background on http://localhost:8080$(RESET)" + +web-proxy-stop: ## Stop background Caddy proxy + @caddy stop 2>/dev/null || echo "$(YELLOW)Caddy not running$(RESET)" + @echo "$(GREEN)✓ Caddy proxy stopped$(RESET)" + ##@ Utilities validate-lexicon: ## Validate all Lexicon schemas diff --git a/cmd/server/main.go b/cmd/server/main.go index 1d4ad57..d5bb5fb 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -202,6 +202,7 @@ func main() { isDevMode := os.Getenv("IS_DEV_ENV") == "true" pdsURL := os.Getenv("PDS_URL") // For dev mode: resolve handles via local PDS + oauthConfig := &oauth.OAuthConfig{ PublicURL: os.Getenv("APPVIEW_PUBLIC_URL"), SealSecret: oauthSealSecret, diff --git a/internal/atproto/oauth/client.go b/internal/atproto/oauth/client.go index 263cdd9..9e10e3c 100644 --- a/internal/atproto/oauth/client.go +++ b/internal/atproto/oauth/client.go @@ -86,9 +86,10 @@ func NewOAuthClient(config *OAuthConfig, store oauth.ClientAuthStore) (*OAuthCli // Create indigo client config var clientConfig oauth.ClientConfig if config.DevMode { - // Dev mode: loopback with HTTP - // IMPORTANT: Use 127.0.0.1 instead of localhost per RFC 8252 - PDS rejects localhost - // The callback URL must match the APPVIEW_PUBLIC_URL from .env.dev + // Dev mode: loopback OAuth client + // Per ATProto OAuth spec: client_id base MUST be "http://localhost" (not 127.0.0.1) + // The redirect_uri in the query params CAN use 127.0.0.1 with port + // Format: http://localhost?redirect_uri=http%3A%2F%2F127.0.0.1%3A8081%2Foauth%2Fcallback&scope=atproto callbackURL := config.PublicURL + "/oauth/callback" clientConfig = oauth.NewLocalhostConfig(callbackURL, config.Scopes) slog.Info("dev mode: OAuth client configured", diff --git a/internal/atproto/oauth/dev_auth_resolver.go b/internal/atproto/oauth/dev_auth_resolver.go index 30f74e0..585fdc6 100644 --- a/internal/atproto/oauth/dev_auth_resolver.go +++ b/internal/atproto/oauth/dev_auth_resolver.go @@ -257,11 +257,20 @@ func (r *DevAuthResolver) StartDevAuthFlow(ctx context.Context, client *OAuthCli slog.Debug("dev mode: got auth server metadata", "issuer", authMeta.Issuer, "authorization_endpoint", authMeta.AuthorizationEndpoint, - "token_endpoint", authMeta.TokenEndpoint) + "token_endpoint", authMeta.TokenEndpoint, + "par_endpoint", authMeta.PushedAuthorizationRequestEndpoint) + + slog.Debug("dev mode: PAR request details", + "client_id", client.ClientApp.Config.ClientID, + "callback_url", client.ClientApp.Config.CallbackURL, + "scopes", client.Config.Scopes) // Send auth request (PAR) using indigo's method info, err := client.ClientApp.SendAuthRequest(ctx, authMeta, client.Config.Scopes, identifier) if err != nil { + slog.Error("dev mode: PAR request failed", + "error", err, + "client_id", client.ClientApp.Config.ClientID) return "", fmt.Errorf("auth request failed: %w", err) } diff --git a/internal/atproto/oauth/handlers.go b/internal/atproto/oauth/handlers.go index 3bcd32a..a8ceb67 100644 --- a/internal/atproto/oauth/handlers.go +++ b/internal/atproto/oauth/handlers.go @@ -156,12 +156,13 @@ type UserIndexer interface { // OAuthHandler handles OAuth-related HTTP endpoints type OAuthHandler struct { - client *OAuthClient - store oauth.ClientAuthStore - mobileStore MobileOAuthStore // For server-side CSRF validation - userIndexer UserIndexer // For indexing users after OAuth login - devResolver *DevHandleResolver // For dev mode: resolve handles via local PDS - devAuthResolver *DevAuthResolver // For dev mode: bypass HTTPS validation for localhost OAuth + client *OAuthClient + store oauth.ClientAuthStore + mobileStore MobileOAuthStore // For server-side CSRF validation + userIndexer UserIndexer // For indexing users after OAuth login + devResolver *DevHandleResolver // For dev mode: resolve handles via local PDS + devAuthResolver *DevAuthResolver // For dev mode: bypass HTTPS validation for localhost OAuth + allowedRedirectURIs map[string]bool // Combined allowlist for mobile + external OAuth clients } // OAuthHandlerOption is a functional option for configuring OAuthHandler @@ -178,8 +179,9 @@ func WithUserIndexer(indexer UserIndexer) OAuthHandlerOption { // NewOAuthHandler creates a new OAuth handler func NewOAuthHandler(client *OAuthClient, store oauth.ClientAuthStore, opts ...OAuthHandlerOption) *OAuthHandler { handler := &OAuthHandler{ - client: client, - store: store, + client: client, + store: store, + allowedRedirectURIs: BuildAllowedRedirectURIs(), } // Apply functional options @@ -209,6 +211,15 @@ func NewOAuthHandler(client *OAuthClient, store oauth.ClientAuthStore, opts ...O return handler } +// isAllowedRedirectURI checks if a redirect URI is in the configured allowlist. +// This includes both the base mobile redirect URIs and any configured external client URIs. +// +// SECURITY: Uses exact string matching - no wildcards or pattern matching. +// The URI must match exactly as configured in the allowlist. +func (h *OAuthHandler) isAllowedRedirectURI(redirectURI string) bool { + return h.allowedRedirectURIs[redirectURI] +} + // HandleClientMetadata serves the OAuth client metadata document // GET /oauth-client-metadata.json func (h *OAuthHandler) HandleClientMetadata(w http.ResponseWriter, r *http.Request) { @@ -354,9 +365,10 @@ func (h *OAuthHandler) HandleMobileLogin(w http.ResponseWriter, r *http.Request) } // SECURITY FIX 1: Validate redirect_uri against allowlist - if !isAllowedMobileRedirectURI(mobileRedirectURI) { - slog.Warn("rejected unauthorized mobile redirect URI", "scheme", extractScheme(mobileRedirectURI)) - http.Error(w, "invalid redirect_uri: scheme not allowed", http.StatusBadRequest) + // Uses configurable allowlist that includes both mobile deep links and external client URIs + if !h.isAllowedRedirectURI(mobileRedirectURI) { + slog.Warn("rejected unauthorized redirect URI", "scheme", extractScheme(mobileRedirectURI)) + http.Error(w, "invalid redirect_uri: not in allowlist", http.StatusBadRequest) return } @@ -769,24 +781,27 @@ func (h *OAuthHandler) handleWebCallback(w http.ResponseWriter, r *http.Request, http.Redirect(w, r, redirectURL, http.StatusFound) } -// handleMobileCallback handles the mobile OAuth callback flow +// handleMobileCallback handles the mobile OAuth callback flow. +// This handles both mobile deep links (custom schemes like social.coves://) and +// Universal Links (https:// URLs verified via .well-known). func (h *OAuthHandler) handleMobileCallback(w http.ResponseWriter, r *http.Request, sessData *oauth.ClientSessionData, mobileRedirectURIEncoded, csrfToken, verifiedHandle string) { - // Decode the mobile redirect URI - mobileRedirectURI, err := url.QueryUnescape(mobileRedirectURIEncoded) + // Decode the redirect URI + redirectURI, err := url.QueryUnescape(mobileRedirectURIEncoded) if err != nil { - slog.Error("failed to decode mobile redirect URI", "error", err) - http.Error(w, "invalid mobile redirect URI", http.StatusBadRequest) + slog.Error("failed to decode redirect URI", "error", err) + http.Error(w, "invalid redirect URI", http.StatusBadRequest) return } // SECURITY FIX 1: Re-validate redirect URI against allowlist - if !isAllowedMobileRedirectURI(mobileRedirectURI) { - slog.Error("mobile callback attempted with unauthorized redirect URI", "scheme", extractScheme(mobileRedirectURI)) + // Uses configurable allowlist that includes both mobile deep links and external client URIs + if !h.isAllowedRedirectURI(redirectURI) { + slog.Error("callback attempted with unauthorized redirect URI", "scheme", extractScheme(redirectURI)) http.Error(w, "invalid redirect URI", http.StatusBadRequest) return } - // Seal the session data for mobile + // Seal the session data sealedToken, err := h.client.SealSession( sessData.AccountDID.String(), sessData.SessionID, @@ -807,25 +822,37 @@ func (h *OAuthHandler) handleMobileCallback(w http.ResponseWriter, r *http.Reque } } - // Clear all mobile cookies to prevent reuse (defense in depth) + // Clear all mobile/external cookies to prevent reuse (defense in depth) clearMobileCookies(w) - // Build deep link with sealed token - deepLink := fmt.Sprintf("%s?token=%s&did=%s&session_id=%s", - mobileRedirectURI, + // Build redirect URL with sealed token + callbackURL := fmt.Sprintf("%s?token=%s&did=%s&session_id=%s", + redirectURI, url.QueryEscape(sealedToken), url.QueryEscape(sessData.AccountDID.String()), url.QueryEscape(sessData.SessionID), ) if handle != "" { - deepLink += "&handle=" + url.QueryEscape(handle) + callbackURL += "&handle=" + url.QueryEscape(handle) } - // Log mobile redirect (sanitized - no token or session ID to avoid leaking credentials) - slog.Info("redirecting to mobile app", "did", sessData.AccountDID, "handle", handle) + // Determine redirect type based on scheme + parsedURI, parseErr := url.Parse(redirectURI) + isWebClient := parseErr == nil && (parsedURI.Scheme == "http" || parsedURI.Scheme == "https") + if isWebClient { + // HTTPS Universal Links or web clients get a direct HTTP redirect. + // The OS intercepts Universal Links and opens the app; no intermediate page needed. + slog.Info("redirecting via HTTP", "did", sessData.AccountDID, "handle", handle, "host", parsedURI.Host) + http.Redirect(w, r, callbackURL, http.StatusFound) + return + } + + // Mobile app with custom scheme (e.g., social.coves://) // Serve intermediate page that redirects to the app // This prevents the browser from showing a stale PDS page after the custom scheme redirect + slog.Info("redirecting to mobile app", "did", sessData.AccountDID, "handle", handle) + w.Header().Set("Content-Type", "text/html; charset=utf-8") w.Header().Set("Cache-Control", "no-store, no-cache, must-revalidate") @@ -833,14 +860,14 @@ func (h *OAuthHandler) handleMobileCallback(w http.ResponseWriter, r *http.Reque DeepLink string Handle string }{ - DeepLink: deepLink, + DeepLink: callbackURL, Handle: handle, } if err := mobileCallbackTemplate.Execute(w, data); err != nil { slog.Error("failed to render mobile callback template", "error", err) // Fallback to direct redirect if template fails - http.Redirect(w, r, deepLink, http.StatusFound) + http.Redirect(w, r, callbackURL, http.StatusFound) } } diff --git a/internal/atproto/oauth/handlers_security.go b/internal/atproto/oauth/handlers_security.go index 79dcd02..46741c2 100644 --- a/internal/atproto/oauth/handlers_security.go +++ b/internal/atproto/oauth/handlers_security.go @@ -9,7 +9,8 @@ import ( "net/url" ) -// allowedMobileRedirectURIs contains the EXACT allowed redirect URIs for mobile apps. +// baseMobileRedirectURIs contains the EXACT allowed redirect URIs for mobile apps. +// These are always allowed regardless of configuration. // // Per atproto OAuth spec (https://atproto.com/specs/oauth#mobile-clients): // - Custom URL schemes are allowed for native mobile apps @@ -23,7 +24,7 @@ import ( // Universal Links provide stronger security guarantees but require: // - iOS: Verified via /.well-known/apple-app-site-association // - Android: Verified via /.well-known/assetlinks.json -var allowedMobileRedirectURIs = map[string]bool{ +var baseMobileRedirectURIs = map[string]bool{ // Custom scheme per atproto spec (reverse-domain of coves.social) "social.coves:/callback": true, "social.coves://callback": true, // Some platforms add double slash @@ -33,18 +34,18 @@ var allowedMobileRedirectURIs = map[string]bool{ "https://coves.social/app/oauth/callback": true, } -// isAllowedMobileRedirectURI validates that the redirect URI is in the exact allowlist. -// SECURITY: Exact URI matching prevents token theft by rogue apps. +// BuildAllowedRedirectURIs returns a copy of the allowed mobile redirect URIs. +// The allowlist uses exact URI matching to prevent token theft. // -// Per atproto OAuth spec, custom schemes must match the client_id hostname -// in reverse-domain order (social.coves for coves.social), which provides -// some protection as malicious apps would need to know the specific scheme. -// -// Universal Links (https://) provide stronger security as they're cryptographically -// bound to the app via .well-known verification files. -func isAllowedMobileRedirectURI(redirectURI string) bool { - // Normalize and check exact match - return allowedMobileRedirectURIs[redirectURI] +// For web frontends like Kelp, use a reverse proxy (Vite proxy in dev, nginx in prod) +// to serve from the same origin as Coves, allowing HTTP-only cookies to work. +func BuildAllowedRedirectURIs() map[string]bool { + // Return a copy of base mobile URIs + allowed := make(map[string]bool, len(baseMobileRedirectURIs)) + for uri := range baseMobileRedirectURIs { + allowed[uri] = true + } + return allowed } // extractScheme extracts the scheme from a URI for logging purposes diff --git a/internal/atproto/oauth/handlers_security_test.go b/internal/atproto/oauth/handlers_security_test.go index e64f89e..f05952a 100644 --- a/internal/atproto/oauth/handlers_security_test.go +++ b/internal/atproto/oauth/handlers_security_test.go @@ -5,74 +5,11 @@ import ( "net/http/httptest" "testing" + "github.com/bluesky-social/indigo/atproto/auth/oauth" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -// TestIsAllowedMobileRedirectURI tests the mobile redirect URI allowlist with EXACT URI matching -// Only Universal Links (HTTPS) are allowed - custom schemes are blocked for security -func TestIsAllowedMobileRedirectURI(t *testing.T) { - tests := []struct { - name string - uri string - expected bool - }{ - { - name: "allowed - Universal Link", - uri: "https://coves.social/app/oauth/callback", - expected: true, - }, - { - name: "rejected - custom scheme coves-app (vulnerable to interception)", - uri: "coves-app://oauth/callback", - expected: false, - }, - { - name: "rejected - custom scheme coves (vulnerable to interception)", - uri: "coves://oauth/callback", - expected: false, - }, - { - name: "rejected - evil scheme", - uri: "evil://callback", - expected: false, - }, - { - name: "rejected - http (not secure)", - uri: "http://example.com/callback", - expected: false, - }, - { - name: "rejected - https different domain", - uri: "https://example.com/callback", - expected: false, - }, - { - name: "rejected - https coves.social wrong path", - uri: "https://coves.social/wrong/path", - expected: false, - }, - { - name: "rejected - invalid URI", - uri: "not a uri", - expected: false, - }, - { - name: "rejected - empty string", - uri: "", - expected: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := isAllowedMobileRedirectURI(tt.uri) - assert.Equal(t, tt.expected, result, - "isAllowedMobileRedirectURI(%q) = %v, want %v", tt.uri, result, tt.expected) - }) - } -} - // TestExtractScheme tests the scheme extraction function func TestExtractScheme(t *testing.T) { tests := []struct { @@ -122,53 +59,6 @@ func TestGenerateCSRFToken(t *testing.T) { assert.Greater(t, len(token1), 40, "CSRF token should be reasonably long (32 bytes base64 encoded)") } -// TestHandleMobileLogin_RedirectURIValidation tests that HandleMobileLogin validates redirect URIs -func TestHandleMobileLogin_RedirectURIValidation(t *testing.T) { - // Note: This is a unit test for the validation logic only. - // Full integration tests with OAuth flow are in tests/integration/oauth_e2e_test.go - - tests := []struct { - name string - redirectURI string - expectedLog string - expectedStatus int - }{ - { - name: "allowed - Universal Link", - redirectURI: "https://coves.social/app/oauth/callback", - expectedStatus: http.StatusBadRequest, // Will fail at StartAuthFlow (no OAuth client setup) - }, - { - name: "rejected - custom scheme coves-app (insecure)", - redirectURI: "coves-app://oauth/callback", - expectedStatus: http.StatusBadRequest, - expectedLog: "rejected unauthorized mobile redirect URI", - }, - { - name: "rejected evil scheme", - redirectURI: "evil://callback", - expectedStatus: http.StatusBadRequest, - expectedLog: "rejected unauthorized mobile redirect URI", - }, - { - name: "rejected http", - redirectURI: "http://evil.com/callback", - expectedStatus: http.StatusBadRequest, - expectedLog: "scheme not allowed", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Test the validation function directly - result := isAllowedMobileRedirectURI(tt.redirectURI) - if tt.expectedLog != "" { - assert.False(t, result, "Should reject %s", tt.redirectURI) - } - }) - } -} - // TestHandleCallback_CSRFValidation tests that HandleCallback validates CSRF tokens for mobile flow func TestHandleCallback_CSRFValidation(t *testing.T) { // This is a conceptual test structure. Full implementation would require: @@ -210,41 +100,6 @@ func TestHandleCallback_CSRFValidation(t *testing.T) { }) } -// TestHandleMobileCallback_RevalidatesRedirectURI tests that handleMobileCallback re-validates the redirect URI -func TestHandleMobileCallback_RevalidatesRedirectURI(t *testing.T) { - // This is a critical security test: even if an attacker somehow bypasses the initial check, - // the callback handler should re-validate the redirect URI before redirecting. - - tests := []struct { - name string - redirectURI string - shouldPass bool - }{ - { - name: "allowed - Universal Link", - redirectURI: "https://coves.social/app/oauth/callback", - shouldPass: true, - }, - { - name: "blocked - custom scheme (insecure)", - redirectURI: "coves-app://oauth/callback", - shouldPass: false, - }, - { - name: "blocked - evil scheme", - redirectURI: "evil://callback", - shouldPass: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := isAllowedMobileRedirectURI(tt.redirectURI) - assert.Equal(t, tt.shouldPass, result) - }) - } -} - // TestGenerateMobileRedirectBinding tests the binding token generation // The binding now includes the CSRF token for proper double-submit validation func TestGenerateMobileRedirectBinding(t *testing.T) { @@ -475,3 +330,175 @@ func TestConstantTimeCompare(t *testing.T) { }) } } + +// TestBuildAllowedRedirectURIs tests the mobile redirect URI allowlist builder +func TestBuildAllowedRedirectURIs(t *testing.T) { + t.Run("includes all base mobile URIs", func(t *testing.T) { + allowed := BuildAllowedRedirectURIs() + + // All base mobile URIs should be included + assert.True(t, allowed["social.coves:/callback"], "should include social.coves:/callback") + assert.True(t, allowed["social.coves://callback"], "should include social.coves://callback") + assert.True(t, allowed["social.coves:/oauth/callback"], "should include social.coves:/oauth/callback") + assert.True(t, allowed["social.coves://oauth/callback"], "should include social.coves://oauth/callback") + assert.True(t, allowed["https://coves.social/app/oauth/callback"], "should include Universal Link") + + // Should have exactly 5 base mobile URIs + assert.Len(t, allowed, 5, "should have exactly 5 base mobile URIs") + }) + + t.Run("rejects URIs not in allowlist", func(t *testing.T) { + allowed := BuildAllowedRedirectURIs() + + // Should reject URIs not in the list + assert.False(t, allowed["http://evil.com/callback"], "should reject evil.com") + assert.False(t, allowed["http://localhost:5173/callback"], "should reject localhost") + assert.False(t, allowed["evil://steal"], "should reject evil scheme") + }) + + t.Run("returns copy not reference to base URIs", func(t *testing.T) { + allowed1 := BuildAllowedRedirectURIs() + allowed2 := BuildAllowedRedirectURIs() + + // Modifying one should not affect the other + allowed1["test://modified"] = true + assert.False(t, allowed2["test://modified"], "modifications should not affect other copies") + }) +} + +// ============================================================================= +// Mobile OAuth Redirect URI Integration Tests +// ============================================================================= + +// createTestOAuthHandler creates a minimal OAuthHandler for testing. +// This uses a memory store and minimal configuration suitable for unit tests. +func createTestOAuthHandler(t *testing.T) *OAuthHandler { + t.Helper() + + config := &OAuthConfig{ + PublicURL: "https://coves.social", + Scopes: []string{"atproto"}, + DevMode: true, // Dev mode to avoid real PDS calls + AllowPrivateIPs: true, + SealSecret: "MTIzNDU2Nzg5MDEyMzQ1Njc4OTAxMjM0NTY3ODkwMTI=", // base64 encoded 32 bytes + } + + client, err := NewOAuthClient(config, oauth.NewMemStore()) + require.NoError(t, err) + + handler := NewOAuthHandler(client, oauth.NewMemStore()) + return handler +} + +// TestOAuthHandler_isAllowedRedirectURI tests the OAuthHandler.isAllowedRedirectURI() method. +// This is a critical security test (severity 9/10). +func TestOAuthHandler_isAllowedRedirectURI(t *testing.T) { + t.Run("accepts base mobile URIs", func(t *testing.T) { + handler := createTestOAuthHandler(t) + + // Base mobile URIs should be accepted + baseMobileURIs := []string{ + "social.coves:/callback", + "social.coves://callback", + "social.coves:/oauth/callback", + "social.coves://oauth/callback", + "https://coves.social/app/oauth/callback", + } + + for _, uri := range baseMobileURIs { + assert.True(t, handler.isAllowedRedirectURI(uri), + "should accept base mobile URI: %s", uri) + } + }) + + t.Run("rejects URIs not in allowlist", func(t *testing.T) { + handler := createTestOAuthHandler(t) + + // These URIs should be rejected + rejectedURIs := []string{ + "http://localhost:5173/callback", // Localhost (use Vite proxy instead) + "http://localhost:3000/callback", // Localhost + "http://evil.com/callback", // Evil domain + "https://example.com/oauth", // Random HTTPS + "https://coves.social/wrong/path", // Right domain, wrong path + "evil://steal", // Evil custom scheme + "coves-app://callback", // Old/wrong custom scheme + "coves://oauth/callback", // Wrong custom scheme (not reverse-domain) + "", // Empty + "not-a-uri", // Invalid URI + } + + for _, uri := range rejectedURIs { + assert.False(t, handler.isAllowedRedirectURI(uri), + "should reject URI not in allowlist: %s", uri) + } + }) +} + +// TestHandleMobileLogin_MobileURIs tests that HandleMobileLogin properly +// accepts/rejects mobile redirect URIs. (severity 9/10) +func TestHandleMobileLogin_MobileURIs(t *testing.T) { + t.Run("accepts mobile Universal Link", func(t *testing.T) { + handler := createTestOAuthHandler(t) + + req := httptest.NewRequest(http.MethodGet, + "/oauth/mobile/login?handle=test.user&redirect_uri=https://coves.social/app/oauth/callback", nil) + rec := httptest.NewRecorder() + + handler.HandleMobileLogin(rec, req) + + // Should NOT get "invalid redirect_uri" error + body := rec.Body.String() + assert.NotContains(t, body, "invalid redirect_uri", + "mobile Universal Link should be accepted") + }) + + t.Run("rejects localhost URI", func(t *testing.T) { + handler := createTestOAuthHandler(t) + + req := httptest.NewRequest(http.MethodGet, + "/oauth/mobile/login?handle=test.user&redirect_uri=http://localhost:5173/callback", nil) + rec := httptest.NewRecorder() + + handler.HandleMobileLogin(rec, req) + + // Should get "invalid redirect_uri" error + assert.Equal(t, http.StatusBadRequest, rec.Code) + assert.Contains(t, rec.Body.String(), "invalid redirect_uri", + "localhost URI should be rejected (use Vite proxy for dev)") + }) + + t.Run("rejects evil URI", func(t *testing.T) { + handler := createTestOAuthHandler(t) + + req := httptest.NewRequest(http.MethodGet, + "/oauth/mobile/login?handle=test.user&redirect_uri=http://evil.com/callback", nil) + rec := httptest.NewRecorder() + + handler.HandleMobileLogin(rec, req) + + // Should get "invalid redirect_uri" error + assert.Equal(t, http.StatusBadRequest, rec.Code) + assert.Contains(t, rec.Body.String(), "invalid redirect_uri", + "evil URI should be rejected") + }) +} + +// TestMobileURIs_OnlyMobileAllowed tests that only mobile URIs are allowed. +func TestMobileURIs_OnlyMobileAllowed(t *testing.T) { + t.Run("only mobile URIs work", func(t *testing.T) { + handler := createTestOAuthHandler(t) + + // Mobile URIs should work + assert.True(t, handler.isAllowedRedirectURI("social.coves:/callback"), + "mobile custom scheme should work") + assert.True(t, handler.isAllowedRedirectURI("https://coves.social/app/oauth/callback"), + "mobile Universal Link should work") + + // Localhost URIs should NOT work (use Vite proxy for dev) + assert.False(t, handler.isAllowedRedirectURI("http://localhost:5173/callback"), + "localhost should be rejected") + assert.False(t, handler.isAllowedRedirectURI("http://127.0.0.1:3000/callback"), + "127.0.0.1 should be rejected") + }) +} diff --git a/internal/atproto/oauth/handlers_test.go b/internal/atproto/oauth/handlers_test.go index a8c9e9e..0506a68 100644 --- a/internal/atproto/oauth/handlers_test.go +++ b/internal/atproto/oauth/handlers_test.go @@ -174,6 +174,8 @@ func TestParseSessionToken(t *testing.T) { // TestIsMobileRedirectURI tests mobile redirect URI validation with EXACT URI matching // Per atproto spec, custom schemes must match client_id hostname in reverse-domain order func TestIsMobileRedirectURI(t *testing.T) { + handler := createTestOAuthHandler(t) + tests := []struct { uri string expected bool @@ -199,7 +201,7 @@ func TestIsMobileRedirectURI(t *testing.T) { for _, tt := range tests { t.Run(tt.uri, func(t *testing.T) { - result := isAllowedMobileRedirectURI(tt.uri) + result := handler.isAllowedRedirectURI(tt.uri) assert.Equal(t, tt.expected, result) }) } diff --git a/scripts/web-dev-run.sh b/scripts/web-dev-run.sh new file mode 100755 index 0000000..0d9fcc9 --- /dev/null +++ b/scripts/web-dev-run.sh @@ -0,0 +1,77 @@ +#!/bin/bash +# Web frontend development server runner +# Starts both Coves backend AND Vite frontend, uses Caddy proxy on port 8080 +# +# Usage: make run-web (or ./scripts/web-dev-run.sh) + +set -a # automatically export all variables +source .env.dev +set +a + +# Override for web frontend development +# OAuth callback needs to use the proxy port (8080) for cookie sharing +# MUST use 127.0.0.1 (not localhost) per RFC 8252 - PDS rejects localhost in redirect_uri +export APPVIEW_PUBLIC_URL="http://127.0.0.1:8080" + +# Resolve paths relative to this script (no hardcoded absolute paths) +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +COVES_DIR="$(cd "$SCRIPT_DIR/.." && pwd)" + +# Frontend location (override with KELP_DIR env var) +KELP_DIR="${KELP_DIR:-$COVES_DIR/../kelp}" + +# Cleanup function +cleanup() { + echo "" + echo "🛑 Shutting down..." + # Kill the Vite process if it's running + if [ -n "$VITE_PID" ]; then + kill $VITE_PID 2>/dev/null + wait $VITE_PID 2>/dev/null + fi + exit 0 +} + +# Set up trap for clean shutdown +trap cleanup SIGINT SIGTERM + +echo "🌐 Starting Coves WEB FRONTEND development environment..." +echo "" +echo " IS_DEV_ENV: $IS_DEV_ENV" +echo " PLC_DIRECTORY_URL: $PLC_DIRECTORY_URL" +echo " APPVIEW_PUBLIC_URL: $APPVIEW_PUBLIC_URL (via Caddy proxy)" +echo " PDS_URL: $PDS_URL" +echo " Build tags: dev" +echo "" + +# Check if kelp directory exists +if [ ! -d "$KELP_DIR" ]; then + echo "❌ Frontend directory not found: $KELP_DIR" + exit 1 +fi + +# Start Vite in background +echo "🚀 Starting Vite frontend (kelp) on :5173..." +cd "$KELP_DIR" && npm run dev & +VITE_PID=$! + +# Give Vite a moment to start +sleep 2 + +# Return to Coves directory +cd "$COVES_DIR" + +echo "" +echo "🚀 Starting Coves backend on :8081..." +echo "" +echo " ⚠️ Make sure Caddy proxy is running: make web-proxy" +echo " 📍 Access your app at: http://localhost:8080" +echo "" +echo " Press Ctrl+C to stop both services" +echo "" + +# Run Go server (foreground - will block) +go run -tags dev ./cmd/server + +# If Go server exits, clean up Vite +cleanup -- 2.51.2