diff --git a/internal/atproto/identity/factory.go b/internal/atproto/identity/factory.go --- a/internal/atproto/identity/factory.go +++ b/internal/atproto/identity/factory.go @@ -3,6 +3,7 @@ import ( "database/sql" "net/http" + "os" "time" ) @@ -13,10 +14,24 @@ PLCURL string CacheTTL time.Duration } -// DefaultConfig returns a configuration with sensible defaults +// DefaultConfig returns a configuration with sensible defaults. +// +// PLCURL honors PLC_DIRECTORY_URL when set, falling back to the production +// directory otherwise. This matters for tests: the Makefile exports .env.dev +// (PLC_DIRECTORY_URL=http://localhost:3002), so every resolver built from +// DefaultConfig resolves against the local PLC instead of quietly issuing +// lookups against production for DIDs that only exist locally. +// +// Callers that deliberately need the production directory - resolving real +// Bluesky handles, for instance - must set PLCURL explicitly after calling +// this, as cmd/server does for its read-only Bluesky resolver. func DefaultConfig() Config { + plcURL := os.Getenv("PLC_DIRECTORY_URL") + if plcURL == "" { + plcURL = "https://plc.directory" + } return Config{ - PLCURL: "https://plc.directory", + PLCURL: plcURL, CacheTTL: 24 * time.Hour, // Cache for 24 hours HTTPClient: &http.Client{Timeout: 10 * time.Second}, } diff --git a/internal/atproto/oauth/client.go b/internal/atproto/oauth/client.go --- a/internal/atproto/oauth/client.go +++ b/internal/atproto/oauth/client.go @@ -42,6 +42,15 @@ if config == nil { return nil, fmt.Errorf("config is required") } + // PLCURL must be explicit. An empty value used to fall through to indigo's + // default directory, which is the production plc.directory - so a test or a + // misconfigured deploy that simply forgot the field would silently resolve + // identities against production. Callers must name the directory they mean: + // "https://plc.directory" in production, the local PLC in dev and tests. + if config.PLCURL == "" { + return nil, fmt.Errorf("PLCURL is required (use the local PLC directory in dev/test, \"https://plc.directory\" in production)") + } + // Validate seal secret var sealSecret []byte if config.SealSecret != "" { @@ -132,25 +141,21 @@ // Override the default HTTP client with our SSRF-safe client // This protects against SSRF attacks via malicious PDS URLs, DID documents, and JWKS URIs clientApp.Client = NewSSRFSafeHTTPClient(config.AllowPrivateIPs) - // Override the directory if a custom PLC URL is configured - // This is necessary for local development with a local PLC directory - if config.PLCURL != "" { - // Use SSRF-safe HTTP client for PLC directory requests - httpClient := NewSSRFSafeHTTPClient(config.AllowPrivateIPs) - baseDir := &identity.BaseDirectory{ - PLCURL: config.PLCURL, - HTTPClient: *httpClient, - UserAgent: "Coves/1.0", - } - // Wrap in cache directory for better performance - // Use pointer since CacheDirectory methods have pointer receivers - cacheDir := identity.NewCacheDirectory(baseDir, 100_000, time.Hour*24, time.Minute*2, time.Minute*5) - clientApp.Dir = &cacheDir - // Log the PLC URL being used for OAuth directory resolution - fmt.Printf("🔐 OAuth client directory configured with PLC URL: %s (AllowPrivateIPs: %v)\n", config.PLCURL, config.AllowPrivateIPs) - } else { - fmt.Println("⚠️ OAuth client using DEFAULT PLC directory (production plc.directory)") + // Always override the directory so resolution goes to the configured PLC + // rather than indigo's default. Use SSRF-safe HTTP client for PLC requests. + httpClient := NewSSRFSafeHTTPClient(config.AllowPrivateIPs) + baseDir := &identity.BaseDirectory{ + PLCURL: config.PLCURL, + HTTPClient: *httpClient, + UserAgent: "Coves/1.0", } + // Wrap in cache directory for better performance + // Use pointer since CacheDirectory methods have pointer receivers + cacheDir := identity.NewCacheDirectory(baseDir, 100_000, time.Hour*24, time.Minute*2, time.Minute*5) + clientApp.Dir = &cacheDir + slog.Info("OAuth client directory configured", + "plc_url", config.PLCURL, + "allow_private_ips", config.AllowPrivateIPs) return &OAuthClient{ ClientApp: clientApp, diff --git a/internal/atproto/oauth/handlers_error_redirect_test.go b/internal/atproto/oauth/handlers_error_redirect_test.go --- a/internal/atproto/oauth/handlers_error_redirect_test.go +++ b/internal/atproto/oauth/handlers_error_redirect_test.go @@ -106,6 +106,7 @@ config := &OAuthConfig{ PublicURL: "https://coves.social", Scopes: []string{"atproto"}, + PLCURL: testPLCURL, DevMode: devMode, AllowPrivateIPs: devMode, SealSecret: "MTIzNDU2Nzg5MDEyMzQ1Njc4OTAxMjM0NTY3ODkwMTI=", // base64 encoded 32 bytes diff --git a/internal/atproto/oauth/handlers_security_test.go b/internal/atproto/oauth/handlers_security_test.go --- a/internal/atproto/oauth/handlers_security_test.go +++ b/internal/atproto/oauth/handlers_security_test.go @@ -378,6 +378,7 @@ config := &OAuthConfig{ PublicURL: "https://coves.social", Scopes: []string{"atproto"}, + PLCURL: testPLCURL, DevMode: true, // Dev mode to avoid real PDS calls AllowPrivateIPs: true, SealSecret: "MTIzNDU2Nzg5MDEyMzQ1Njc4OTAxMjM0NTY3ODkwMTI=", // base64 encoded 32 bytes @@ -416,16 +417,16 @@ 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 + "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 { diff --git a/internal/atproto/oauth/handlers_test.go b/internal/atproto/oauth/handlers_test.go --- a/internal/atproto/oauth/handlers_test.go +++ b/internal/atproto/oauth/handlers_test.go @@ -13,12 +13,20 @@ "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +// testPLCURL is the PLC directory these unit tests configure their OAuth clients +// with. It is deliberately an unroutable address rather than a real directory: +// these tests never need to resolve an identity, so any resolution attempt is a +// bug and should fail with a connection error instead of silently reaching the +// production plc.directory. OAuthConfig.PLCURL is required, so it must be set. +const testPLCURL = "http://127.0.0.1:1" + // TestHandleClientMetadata tests the client metadata endpoint func TestHandleClientMetadata(t *testing.T) { // Create a test OAuth client configuration config := &OAuthConfig{ PublicURL: "https://coves.social", Scopes: []string{"atproto"}, + PLCURL: testPLCURL, DevMode: false, AllowPrivateIPs: false, SealSecret: "MTIzNDU2Nzg5MDEyMzQ1Njc4OTAxMjM0NTY3ODkwMTI=", // base64 encoded 32 bytes @@ -63,6 +71,7 @@ func TestHandleLogin(t *testing.T) { config := &OAuthConfig{ PublicURL: "https://coves.social", Scopes: []string{"atproto"}, + PLCURL: testPLCURL, DevMode: true, // Use dev mode to avoid real PDS calls AllowPrivateIPs: true, // Allow private IPs in dev mode SealSecret: "MTIzNDU2Nzg5MDEyMzQ1Njc4OTAxMjM0NTY3ODkwMTI=", @@ -102,6 +111,7 @@ func TestHandleMobileLogin(t *testing.T) { config := &OAuthConfig{ PublicURL: "https://coves.social", Scopes: []string{"atproto"}, + PLCURL: testPLCURL, DevMode: true, AllowPrivateIPs: true, SealSecret: "MTIzNDU2Nzg5MDEyMzQ1Njc4OTAxMjM0NTY3ODkwMTI=", @@ -212,6 +222,7 @@ func TestSealAndUnsealSessionData(t *testing.T) { config := &OAuthConfig{ PublicURL: "https://coves.social", Scopes: []string{"atproto"}, + PLCURL: testPLCURL, DevMode: false, AllowPrivateIPs: false, SealSecret: "MTIzNDU2Nzg5MDEyMzQ1Njc4OTAxMjM0NTY3ODkwMTI=", @@ -257,6 +268,7 @@ baseConfig := func() *OAuthConfig { return &OAuthConfig{ PublicURL: "https://coves.social", Scopes: []string{"atproto"}, + PLCURL: testPLCURL, DevMode: false, AllowPrivateIPs: false, SealSecret: "MTIzNDU2Nzg5MDEyMzQ1Njc4OTAxMjM0NTY3ODkwMTI=", @@ -328,6 +340,7 @@ t.Run("public client returns empty JWKS", func(t *testing.T) { config := &OAuthConfig{ PublicURL: "https://coves.social", Scopes: []string{"atproto"}, + PLCURL: testPLCURL, DevMode: false, AllowPrivateIPs: false, SealSecret: "MTIzNDU2Nzg5MDEyMzQ1Njc4OTAxMjM0NTY3ODkwMTI=", @@ -358,6 +371,7 @@ t.Run("confidential client returns JWKS with public key", func(t *testing.T) { config := &OAuthConfig{ PublicURL: "https://coves.social", Scopes: []string{"atproto"}, + PLCURL: testPLCURL, DevMode: false, AllowPrivateIPs: false, SealSecret: "MTIzNDU2Nzg5MDEyMzQ1Njc4OTAxMjM0NTY3ODkwMTI=", @@ -436,6 +450,7 @@ func TestConfidentialClientWithDevMode(t *testing.T) { config := &OAuthConfig{ PublicURL: "http://127.0.0.1:8081", Scopes: []string{"atproto"}, + PLCURL: testPLCURL, DevMode: true, // Dev mode enabled AllowPrivateIPs: true, SealSecret: "MTIzNDU2Nzg5MDEyMzQ1Njc4OTAxMjM0NTY3ODkwMTI=", @@ -464,6 +479,7 @@ func TestSessionTTLsForConfidentialClient(t *testing.T) { config := &OAuthConfig{ PublicURL: "https://coves.social", Scopes: []string{"atproto"}, + PLCURL: testPLCURL, DevMode: false, AllowPrivateIPs: false, SealSecret: "MTIzNDU2Nzg5MDEyMzQ1Njc4OTAxMjM0NTY3ODkwMTI=", diff --git a/internal/validation/lexicon_test.go b/internal/validation/lexicon_test.go --- a/internal/validation/lexicon_test.go +++ b/internal/validation/lexicon_test.go @@ -1,6 +1,7 @@ package validation import ( + "strings" "testing" ) @@ -27,11 +28,14 @@ if err != nil { t.Fatalf("Failed to create validator: %v", err) } - // Valid profile + // Valid profile. Note social.coves.actor.profile has no required fields: + // every property is optional, matching app.bsky.actor.profile. A handle is + // NOT part of the record - it lives in the DID document - so this record + // carries only profile presentation fields. validProfile := map[string]interface{}{ "$type": "social.coves.actor.profile", - "handle": "test.example.com", "displayName": "Test User", + "description": "A test bio", "createdAt": "2024-01-01T00:00:00Z", } @@ -39,14 +43,50 @@ if err := validator.ValidateActorProfile(validProfile); err != nil { t.Errorf("Valid profile failed validation: %v", err) } - // Invalid profile - missing required field - invalidProfile := map[string]interface{}{ - "$type": "social.coves.actor.profile", - "displayName": "Test User", + // A profile with no fields at all is valid, precisely because the schema + // requires nothing. Asserting this pins the "required: []" decision so a + // future schema change that reintroduces a required field fails loudly here. + minimalProfile := map[string]interface{}{ + "$type": "social.coves.actor.profile", } - if err := validator.ValidateActorProfile(invalidProfile); err == nil { - t.Error("Invalid profile passed validation when it should have failed") + if err := validator.ValidateActorProfile(minimalProfile); err != nil { + t.Errorf("Minimal profile failed validation: %v", err) + } + + // Invalid profiles - since no field is required, the enforceable failures + // are constraint violations on the fields that ARE present. + invalidProfiles := map[string]map[string]interface{}{ + "wrong $type": { + "$type": "social.coves.community.post", + "displayName": "Test User", + }, + "displayName over maxLength": { + "$type": "social.coves.actor.profile", + "displayName": strings.Repeat("a", 641), + }, + "displayName over maxGraphemes": { + "$type": "social.coves.actor.profile", + "displayName": strings.Repeat("é", 65), + }, + "description over maxLength": { + "$type": "social.coves.actor.profile", + "description": strings.Repeat("a", 2561), + }, + "createdAt not a datetime": { + "$type": "social.coves.actor.profile", + "createdAt": "January 1st, 2024", + }, + "displayName wrong JSON type": { + "$type": "social.coves.actor.profile", + "displayName": 12345, + }, + } + + for name, profile := range invalidProfiles { + if err := validator.ValidateActorProfile(profile); err == nil { + t.Errorf("Invalid profile (%s) passed validation when it should have failed", name) + } } } diff --git a/tests/integration/feed_test.go b/tests/integration/feed_test.go --- a/tests/integration/feed_test.go +++ b/tests/integration/feed_test.go @@ -937,7 +937,11 @@ embedMap, ok := feedPost.Post.Embed.(map[string]interface{}) require.True(t, ok, "Embed should be a map") - assert.Equal(t, "social.coves.embed.external", embedMap["$type"], "Embed type should be external") + // The AppView rewrites the thumb blob ref into a URL, so the served shape no + // longer matches the record schema. Per the postView embed union in + // social/coves/community/post/defs.json, it must therefore declare the #view + // variant on the wire rather than the record type. + assert.Equal(t, "social.coves.embed.external#view", embedMap["$type"], "Transformed embed should be declared as the external view type") external, ok := embedMap["external"].(map[string]interface{}) require.True(t, ok, "External should be a map") diff --git a/tests/integration/oauth_helpers.go b/tests/integration/oauth_helpers.go --- a/tests/integration/oauth_helpers.go +++ b/tests/integration/oauth_helpers.go @@ -71,9 +71,9 @@ config = &oauth.OAuthConfig{ PublicURL: "http://localhost:3000", // Test server callback URL SealSecret: sealSecretB64, // For sealing mobile tokens Scopes: []string{"atproto"}, - DevMode: false, // Production mode for HTTPS PDS - AllowPrivateIPs: false, // No private IPs in production mode - PLCURL: "", // Use default PLC directory (plc.directory) + DevMode: false, // Production mode for HTTPS PDS + AllowPrivateIPs: false, // No private IPs in production mode + PLCURL: "https://plc.directory", // READ-ONLY: resolving DIDs that already exist on the production directory } t.Logf("🌐 OAuth client configured for production PDS: %s", pdsURL) } else { diff --git a/tests/integration/oauth_session_fixation_test.go b/tests/integration/oauth_session_fixation_test.go --- a/tests/integration/oauth_session_fixation_test.go +++ b/tests/integration/oauth_session_fixation_test.go @@ -8,6 +8,7 @@ "encoding/base64" "net/http" "net/http/httptest" "net/url" + "strings" "testing" "time" @@ -106,15 +107,13 @@ // But we're testing that even if it succeeded, the mobile redirect // validation would prevent the attack handler.HandleCallback(rec, req) - // Step 4: Verify the attack was prevented - // The handler should reject the request due to missing binding - // Since ProcessCallback will fail first (no real OAuth code), we expect - // a 400 error, but the important thing is it doesn't redirect to evil://steal - - assert.NotEqual(t, http.StatusFound, rec.Code, - "Should not redirect when ProcessCallback fails") - assert.NotContains(t, rec.Header().Get("Location"), "evil://", - "Should never redirect to attacker's URI") + // Step 4: Verify the attack was prevented. + // ProcessCallback fails first (no real OAuth code), so the flow ends on + // the first-party error target. The important thing is that the planted + // cookie never steers the redirect to evil://steal - qualifying a mobile + // flow requires server-side data keyed by the OAuth state, which the + // attacker cannot plant. + assertCallbackEndedSafely(t, rec, attackerRedirectURI, "evil://") }) t.Run("legitimate mobile flow - with valid binding", func(t *testing.T) { @@ -171,11 +170,11 @@ rec := httptest.NewRecorder() handler.HandleCallback(rec, req) - // This will also fail at ProcessCallback (no real OAuth code) - // but we're verifying the binding validation logic is in place - // In a real integration test with PDS, this would succeed - assert.NotEqual(t, http.StatusFound, rec.Code, - "Should not redirect when ProcessCallback fails (expected in mock test)") + // This also fails at ProcessCallback (no real OAuth code), so the flow + // ends on the first-party error target rather than in the app: a mobile + // redirect requires server-side data for this OAuth state, and this mock + // callback never created one. Cookies alone are not sufficient. + assertCallbackEndedSafely(t, rec) }) t.Run("binding mismatch - attacker tries wrong binding", func(t *testing.T) { @@ -296,10 +295,48 @@ rec := httptest.NewRecorder() handler.HandleCallback(rec, req) // Should fail because hash(attackerCSRF + redirectURI) != hash(originalCSRF + redirectURI) - // This is the key security fix - CSRF token VALUE is now validated - assert.NotEqual(t, http.StatusFound, rec.Code, - "Should not redirect when CSRF token doesn't match binding") + // This is the key security fix - CSRF token VALUE is now validated, so + // the swapped cookie cannot steer the callback into the app. + assertCallbackEndedSafely(t, rec, redirectURI) }) +} + +// assertCallbackEndedSafely asserts that a callback which failed to produce a +// session ended the flow without leaking anything to an attacker. +// +// A failed ProcessCallback no longer renders a raw 400: HandleCallback answers +// with a first-party 302 to /?oauth_error= so the user lands back +// in the app instead of on a dead-end error page. So the status code proves +// nothing on its own - the DESTINATION is the security property. A mobile +// redirect is only ever chosen from SERVER-SIDE data keyed by the OAuth state, +// which a planted cookie cannot create, so an attacker-supplied URI must never +// appear here. +func assertCallbackEndedSafely(t *testing.T, rec *httptest.ResponseRecorder, forbiddenURIs ...string) { + t.Helper() + + location := rec.Header().Get("Location") + + for _, forbidden := range forbiddenURIs { + assert.NotContains(t, location, forbidden, + "Should never redirect to attacker-controlled URI %q", forbidden) + } + + if location != "" { + // Must be first-party: a relative path, not an absolute URL to some + // other origin or a custom deep-link scheme. + assert.True(t, strings.HasPrefix(location, "/") && !strings.HasPrefix(location, "//"), + "Failed callback should redirect to a first-party path, got %q", location) + assert.Contains(t, location, "oauth_error=", + "Failed callback should redirect to the generic OAuth error target, got %q", location) + } + + // Whatever the outcome, no session material may ride along on the wire. + for _, credential := range []string{"access_token", "session_id", "sealed", "did:plc:"} { + assert.NotContains(t, location, credential, + "Redirect target must not carry session material (%s), got %q", credential, location) + } + assert.NotContains(t, rec.Body.String(), "access-token", + "Response body must not echo an access token") } // generateMobileRedirectBindingForTest generates a binding for testing diff --git a/tests/integration/user_test.go b/tests/integration/user_test.go --- a/tests/integration/user_test.go +++ b/tests/integration/user_test.go @@ -197,8 +197,20 @@ }) // Test 4: Resolve handle to DID (using real handle) t.Run("Resolve Handle to DID", func(t *testing.T) { - // Test with a real atProto handle - did, err := userService.ResolveHandleToDID(ctx, "bretton.dev") + // bretton.dev is a real handle registered on the PRODUCTION PLC directory, + // so this subtest needs its own resolver pinned there. The suite-wide + // resolver above uses identity.DefaultConfig(), which follows + // PLC_DIRECTORY_URL to the local PLC - correct for every other test, but + // it 404s on handles that only exist upstream. + // + // READ-ONLY: ResolveHandleToDID performs HTTP GET lookups only. It never + // registers or mutates anything on the production directory. + productionPLCConfig := identity.DefaultConfig() + productionPLCConfig.PLCURL = "https://plc.directory" + productionResolver := identity.NewResolver(db, productionPLCConfig) + productionUserService := users.NewUserService(userRepo, productionResolver, "http://localhost:3001", nil, "") + + did, err := productionUserService.ResolveHandleToDID(ctx, "bretton.dev") if err != nil { t.Fatalf("Failed to resolve handle bretton.dev: %v", err) }