diff --git a/workspace-server/internal/handlers/registry.go b/workspace-server/internal/handlers/registry.go index 740ed4cf..49c59d4d 100644 --- a/workspace-server/internal/handlers/registry.go +++ b/workspace-server/internal/handlers/registry.go @@ -3,6 +3,7 @@ package handlers import ( "context" "database/sql" + "encoding/json" "errors" "fmt" "log" @@ -1214,6 +1215,21 @@ func (h *RegistryHandler) UpdateCard(c *gin.Context) { return // response already written } + // Validate agent_card.url if present — prevents SSRF via transcript proxy + // (issue #2130). An attacker who compromises a workspace token could + // register a metadata endpoint as the agent_card url and trick the + // platform into forwarding the caller's bearer token to it. + var card struct { + URL string `json:"url"` + } + if err := json.Unmarshal(payload.AgentCard, &card); err == nil && card.URL != "" { + if err := isSafeURL(card.URL); err != nil { + log.Printf("UpdateCard: workspace %s agent_card url rejected: %v", payload.WorkspaceID, err) + c.JSON(http.StatusBadRequest, gin.H{"error": "workspace URL not allowed"}) + return + } + } + agentCardStr := string(payload.AgentCard) _, err := db.DB.ExecContext(c.Request.Context(), ` UPDATE workspaces SET agent_card = $2::jsonb, updated_at = now() WHERE id = $1 diff --git a/workspace-server/internal/handlers/registry_test.go b/workspace-server/internal/handlers/registry_test.go index f5c0336a..0b6bf9b0 100644 --- a/workspace-server/internal/handlers/registry_test.go +++ b/workspace-server/internal/handlers/registry_test.go @@ -780,6 +780,48 @@ func TestUpdateCard_DBError(t *testing.T) { } } +func TestUpdateCard_RejectsMetadataURL(t *testing.T) { + setupTestDB(t) + setupTestRedis(t) + setSSRFCheckForTest(true) + broadcaster := newTestBroadcaster() + handler := NewRegistryHandler(broadcaster) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + + body := `{"workspace_id":"ws-card","agent_card":{"name":"evil","url":"http://169.254.169.254/latest/meta-data/"}}` + c.Request = httptest.NewRequest("POST", "/registry/update-card", bytes.NewBufferString(body)) + c.Request.Header.Set("Content-Type", "application/json") + + handler.UpdateCard(c) + + if w.Code != http.StatusBadRequest { + t.Errorf("expected 400 for metadata URL in agent_card, got %d: %s", w.Code, w.Body.String()) + } +} + +func TestUpdateCard_RejectsNonHTTPScheme(t *testing.T) { + setupTestDB(t) + setupTestRedis(t) + setSSRFCheckForTest(true) + broadcaster := newTestBroadcaster() + handler := NewRegistryHandler(broadcaster) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + + body := `{"workspace_id":"ws-card","agent_card":{"name":"evil","url":"file:///etc/passwd"}}` + c.Request = httptest.NewRequest("POST", "/registry/update-card", bytes.NewBufferString(body)) + c.Request.Header.Set("Content-Type", "application/json") + + handler.UpdateCard(c) + + if w.Code != http.StatusBadRequest { + t.Errorf("expected 400 for file:// scheme in agent_card, got %d: %s", w.Code, w.Body.String()) + } +} + // TestRegister_GuardAgainstResurrectingRemovedRow verifies the #73 fix: // the ON CONFLICT UPSERT must carry a `WHERE status IS DISTINCT FROM 'removed'` // clause so that a late heartbeat from a workspace that was just deleted diff --git a/workspace-server/internal/handlers/transcript.go b/workspace-server/internal/handlers/transcript.go index 70484200..88ad2179 100644 --- a/workspace-server/internal/handlers/transcript.go +++ b/workspace-server/internal/handlers/transcript.go @@ -12,13 +12,10 @@ package handlers import ( "context" - "fmt" "io" "log" - "net" "net/http" "net/url" - "strings" "time" "git.moleculesai.app/molecule-ai/molecule-core/workspace-server/internal/db" @@ -60,17 +57,21 @@ func (h *TranscriptHandler) Get(c *gin.Context) { // workspaceURL comes from agent_card which is attacker-writable via // /registry/register — treat it as untrusted and validate before the - // outbound HTTP call to prevent SSRF (issue #272). + // outbound HTTP call to prevent SSRF (issue #272 / #2130). + // isSafeURL is the production policy used by A2A/MCP dispatch; it + // includes DNS resolution checks and blocks loopback/private/metadata + // targets that validateWorkspaceURL previously allowed. + if err := isSafeURL(workspaceURL); err != nil { + log.Printf("transcript: workspace %s URL rejected: %v", workspaceID, err) + c.JSON(http.StatusBadRequest, gin.H{"error": "workspace URL not allowed"}) + return + } + target, err := url.Parse(workspaceURL) if err != nil { c.JSON(http.StatusBadRequest, gin.H{"error": "invalid workspace URL"}) return } - if err := validateWorkspaceURL(target); err != nil { - log.Printf("transcript: workspace %s URL rejected: %v", workspaceID, err) - c.JSON(http.StatusBadRequest, gin.H{"error": "workspace URL not allowed"}) - return - } target.Path = "/transcript" // Don't forward the raw query string — an attacker-controlled caller @@ -122,57 +123,3 @@ func (h *TranscriptHandler) Get(c *gin.Context) { c.Data(resp.StatusCode, resp.Header.Get("Content-Type"), body) } -// validateWorkspaceURL enforces that the agent_card URL is safe to -// proxy to. agent_card is attacker-writable via /registry/register so -// any workspace-token holder could otherwise point the URL at cloud -// metadata (169.254.169.254), the Docker host, or other internal -// services reachable from the platform container. -// -// Policy: -// - scheme must be http or https (no file://, gopher://, ftp://, etc.) -// - host must be present -// - block cloud metadata endpoints (IMDS, GCP, Azure) -// - block link-local IPs (169.254/16 IPv4, fe80::/10 IPv6) -// - loopback is allowed — local dev runs workspaces on 127.0.0.1 -// - Docker internal hostnames (host.docker.internal, *.molecule-core-net) -// are allowed; the whole threat model assumes the platform already -// trusts peers on that network -func validateWorkspaceURL(u *url.URL) error { - if u.Scheme != "http" && u.Scheme != "https" { - return fmt.Errorf("unsupported scheme %q", u.Scheme) - } - host := u.Hostname() - if host == "" { - return fmt.Errorf("empty host") - } - - // Hostname blocklist (pre-IP-parse — these are usually resolved by - // the HTTP stack, not by us). - lower := strings.ToLower(host) - for _, banned := range []string{ - "metadata.google.internal", - "metadata.azure.com", - "metadata", - } { - if lower == banned { - return fmt.Errorf("metadata hostname blocked: %s", host) - } - } - - // IP-literal checks. - if ip := net.ParseIP(host); ip != nil { - // IMDS / cloud metadata. - if ip.String() == "169.254.169.254" { - return fmt.Errorf("cloud metadata endpoint blocked") - } - // Link-local: IPv4 169.254.0.0/16, IPv6 fe80::/10. - if ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() { - return fmt.Errorf("link-local address blocked: %s", host) - } - // IPv6 unique local fd00::/8 — used by some IMDS implementations. - if ip.To4() == nil && len(ip) == net.IPv6len && ip[0] == 0xfd { - return fmt.Errorf("IPv6 unique-local address blocked: %s", host) - } - } - return nil -} diff --git a/workspace-server/internal/handlers/transcript_test.go b/workspace-server/internal/handlers/transcript_test.go index 5407893b..1f1b9b3f 100644 --- a/workspace-server/internal/handlers/transcript_test.go +++ b/workspace-server/internal/handlers/transcript_test.go @@ -5,15 +5,12 @@ import ( "encoding/json" "net/http" "net/http/httptest" - "net/url" "testing" "github.com/DATA-DOG/go-sqlmock" "github.com/gin-gonic/gin" ) -// urlParse is a tiny wrapper so table-driven tests can keep their lines short. -func urlParse(s string) (*url.URL, error) { return url.Parse(s) } // expectWorkspaceURLLookup programs the sqlmock to answer the SELECT that // TranscriptHandler.Get issues for `agent_card->>'url'`. Tests call this @@ -122,6 +119,7 @@ func TestTranscript_ProxyPropagatesAllowlistedQueryParams(t *testing.T) { func TestTranscript_RejectsCloudMetadataIP(t *testing.T) { mock := setupTestDB(t) setupTestRedis(t) + setSSRFCheckForTest(true) h := NewTranscriptHandler() wsID := expectWorkspaceURLLookup(mock,"http://169.254.169.254/") @@ -139,6 +137,7 @@ func TestTranscript_RejectsCloudMetadataIP(t *testing.T) { func TestTranscript_RejectsNonHTTPScheme(t *testing.T) { mock := setupTestDB(t) setupTestRedis(t) + setSSRFCheckForTest(true) h := NewTranscriptHandler() wsID := expectWorkspaceURLLookup(mock,"file:///etc/passwd") @@ -156,6 +155,7 @@ func TestTranscript_RejectsNonHTTPScheme(t *testing.T) { func TestTranscript_RejectsMetadataHostname(t *testing.T) { mock := setupTestDB(t) setupTestRedis(t) + setSSRFCheckForTest(true) h := NewTranscriptHandler() wsID := expectWorkspaceURLLookup(mock,"http://metadata.google.internal/computeMetadata/v1/") @@ -173,6 +173,7 @@ func TestTranscript_RejectsMetadataHostname(t *testing.T) { func TestTranscript_RejectsLinkLocalIPv6(t *testing.T) { mock := setupTestDB(t) setupTestRedis(t) + setSSRFCheckForTest(true) h := NewTranscriptHandler() wsID := expectWorkspaceURLLookup(mock,"http://[fe80::1]/") @@ -187,41 +188,21 @@ func TestTranscript_RejectsLinkLocalIPv6(t *testing.T) { } } -// validateWorkspaceURL unit tests — pure function, no DB/Redis needed. -func TestValidateWorkspaceURL(t *testing.T) { - cases := []struct { - name string - raw string - wantErr bool - }{ - {"http localhost allowed (dev)", "http://127.0.0.1:8000", false}, - {"https public allowed", "https://agent.example.com", false}, - {"docker internal allowed", "http://host.docker.internal:8000", false}, - {"IMDS IP rejected", "http://169.254.169.254", true}, - {"GCP metadata hostname rejected", "http://metadata.google.internal", true}, - {"Azure metadata rejected", "http://metadata.azure.com", true}, - {"file scheme rejected", "file:///etc/passwd", true}, - {"gopher rejected", "gopher://internal:70/", true}, - {"IPv6 link-local rejected", "http://[fe80::1]", true}, - {"IPv4 link-local multicast rejected", "http://224.0.0.1", true}, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - u, parseErr := urlParse(tc.raw) - if parseErr != nil && !tc.wantErr { - t.Fatalf("parse error: %v", parseErr) - } - if parseErr != nil { - return // unparseable URLs are rejected upstream; not this function's job - } - err := validateWorkspaceURL(u) - if tc.wantErr && err == nil { - t.Errorf("expected error for %q, got nil", tc.raw) - } - if !tc.wantErr && err != nil { - t.Errorf("expected OK for %q, got %v", tc.raw, err) - } - }) +func TestTranscript_RejectsLoopbackURL(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + setSSRFCheckForTest(true) + h := NewTranscriptHandler() + + wsID := expectWorkspaceURLLookup(mock, "http://127.0.0.1:8080/") + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: wsID}} + c.Request = httptest.NewRequest("GET", "/workspaces/"+wsID+"/transcript", nil) + h.Get(c) + if w.Code != http.StatusBadRequest { + t.Errorf("expected 400 for loopback URL, got %d: %s", w.Code, w.Body.String()) } }