From e3ec2a259fe5ebcadde876ca4640b6cb8139c6d3 Mon Sep 17 00:00:00 2001 From: "Molecule AI Code Reviewer (2)" Date: Tue, 2 Jun 2026 20:33:24 +0000 Subject: [PATCH 1/4] fix(security): harden transcript URL validation --- .../internal/handlers/registry.go | 12 +++ .../internal/handlers/registry_test.go | 51 +++++++++++++ .../internal/handlers/transcript.go | 60 +-------------- .../internal/handlers/transcript_test.go | 73 +++++++++---------- 4 files changed, 97 insertions(+), 99 deletions(-) diff --git a/workspace-server/internal/handlers/registry.go b/workspace-server/internal/handlers/registry.go index 9d7ffd58d..0a2563853 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" @@ -873,6 +874,17 @@ func (h *RegistryHandler) UpdateCard(c *gin.Context) { return // response already written } + 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 ed7f90467..dd98c9622 100644 --- a/workspace-server/internal/handlers/registry_test.go +++ b/workspace-server/internal/handlers/registry_test.go @@ -627,6 +627,57 @@ func TestUpdateCard_DBError(t *testing.T) { } } +func TestUpdateCard_RejectsMetadataURL(t *testing.T) { + t.Setenv("MOLECULE_ENV", "production") + restore := setSSRFCheckForTest(true) + defer restore() + mock := setupTestDB(t) + setupTestRedis(t) + broadcaster := newTestBroadcaster() + handler := NewRegistryHandler(broadcaster) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + + body := `{"workspace_id":"ws-card","agent_card":{"name":"bad","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 status 400, got %d: %s", w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + +func TestUpdateCard_RejectsNonHTTPScheme(t *testing.T) { + restore := setSSRFCheckForTest(true) + defer restore() + mock := setupTestDB(t) + setupTestRedis(t) + broadcaster := newTestBroadcaster() + handler := NewRegistryHandler(broadcaster) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + + body := `{"workspace_id":"ws-card","agent_card":{"name":"bad","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 status 400, got %d: %s", w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + // 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 704842009..2920f3b7e 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" @@ -66,7 +63,7 @@ func (h *TranscriptHandler) Get(c *gin.Context) { c.JSON(http.StatusBadRequest, gin.H{"error": "invalid workspace URL"}) return } - if err := validateWorkspaceURL(target); err != nil { + if err := isSafeURL(target.String()); err != nil { log.Printf("transcript: workspace %s URL rejected: %v", workspaceID, err) c.JSON(http.StatusBadRequest, gin.H{"error": "workspace URL not allowed"}) return @@ -121,58 +118,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 5407893bd..6a2c9e470 100644 --- a/workspace-server/internal/handlers/transcript_test.go +++ b/workspace-server/internal/handlers/transcript_test.go @@ -5,16 +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 // instead of inserting real rows (we use sqlmock — there's no DB). @@ -50,6 +46,7 @@ func TestTranscript_WorkspaceNotFound(t *testing.T) { } func TestTranscript_ProxyForwardsAndReturnsBody(t *testing.T) { + allowLoopbackForTest(t) mock := setupTestDB(t) setupTestRedis(t) h := NewTranscriptHandler() @@ -90,6 +87,7 @@ func TestTranscript_ProxyForwardsAndReturnsBody(t *testing.T) { } func TestTranscript_ProxyPropagatesAllowlistedQueryParams(t *testing.T) { + allowLoopbackForTest(t) mock := setupTestDB(t) setupTestRedis(t) h := NewTranscriptHandler() @@ -115,11 +113,14 @@ func TestTranscript_ProxyPropagatesAllowlistedQueryParams(t *testing.T) { } // SSRF regression tests — see issue #272. agent_card->>'url' is attacker- -// writable via /registry/register so validateWorkspaceURL must reject +// writable via /registry/register so the production SSRF policy must reject // link-local / cloud-metadata / non-http(s) targets before the outbound // HTTP call fires. func TestTranscript_RejectsCloudMetadataIP(t *testing.T) { + t.Setenv("MOLECULE_ENV", "production") + restore := setSSRFCheckForTest(true) + defer restore() mock := setupTestDB(t) setupTestRedis(t) h := NewTranscriptHandler() @@ -137,6 +138,8 @@ func TestTranscript_RejectsCloudMetadataIP(t *testing.T) { } func TestTranscript_RejectsNonHTTPScheme(t *testing.T) { + restore := setSSRFCheckForTest(true) + defer restore() mock := setupTestDB(t) setupTestRedis(t) h := NewTranscriptHandler() @@ -154,6 +157,8 @@ func TestTranscript_RejectsNonHTTPScheme(t *testing.T) { } func TestTranscript_RejectsMetadataHostname(t *testing.T) { + restore := setSSRFCheckForTest(true) + defer restore() mock := setupTestDB(t) setupTestRedis(t) h := NewTranscriptHandler() @@ -171,6 +176,9 @@ func TestTranscript_RejectsMetadataHostname(t *testing.T) { } func TestTranscript_RejectsLinkLocalIPv6(t *testing.T) { + t.Setenv("MOLECULE_ENV", "production") + restore := setSSRFCheckForTest(true) + defer restore() mock := setupTestDB(t) setupTestRedis(t) h := NewTranscriptHandler() @@ -187,45 +195,28 @@ 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) { + t.Setenv("MOLECULE_ENV", "production") + restore := setSSRFCheckForTest(true) + defer restore() + mock := setupTestDB(t) + setupTestRedis(t) + h := NewTranscriptHandler() + + wsID := expectWorkspaceURLLookup(mock, "http://127.0.0.1:8000/") + + 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 target, got %d: %s", w.Code, w.Body.String()) } } func TestTranscript_UnreachableWorkspaceReturns502(t *testing.T) { + allowLoopbackForTest(t) mock := setupTestDB(t) setupTestRedis(t) h := NewTranscriptHandler() @@ -255,6 +246,7 @@ func TestTranscript_UnreachableWorkspaceReturns502(t *testing.T) { // req.Header.Set("Authorization", c.GetHeader("Authorization")) // This test verifies the fix and acts as a regression guard. func TestTranscript_ForwardsAuthHeader(t *testing.T) { + allowLoopbackForTest(t) mock := setupTestDB(t) setupTestRedis(t) h := NewTranscriptHandler() @@ -308,6 +300,7 @@ func TestTranscript_ForwardsAuthHeader(t *testing.T) { // request. The workspace will return 401 in this case, which the proxy // faithfully relays — no silent upgrade of privilege. func TestTranscript_NoAuthHeader_PassesThrough(t *testing.T) { + allowLoopbackForTest(t) mock := setupTestDB(t) setupTestRedis(t) h := NewTranscriptHandler() -- 2.52.0 From 352c47d028a8b99d1cea8a2184b1ac23f7d43cc8 Mon Sep 17 00:00:00 2001 From: "Molecule AI Dev Engineer A (Kimi)" Date: Fri, 5 Jun 2026 11:37:35 +0000 Subject: [PATCH 2/4] fix(test): reorder SSRF enable after setupTestDB so tests run with guard ON (internal#807) setupTestDB unconditionally calls setSSRFCheckForTest(false) and registers t.Cleanup(restore). When the new SSRF regression tests called setSSRFCheckForTest(true) *before* setupTestDB, the DB setup overwrote the flag back to false, so the tests ran with SSRF disabled and got 404/502 instead of the expected 400 BadRequest. Fix: move setSSRFCheckForTest(true) to AFTER setupTestDB and switch from defer restore() to t.Cleanup(restore) so the two restore functions compose correctly in LIFO order. Affected tests:\n- TestTranscript_RejectsCloudMetadataIP\n- TestTranscript_RejectsNonHTTPScheme\n- TestTranscript_RejectsMetadataHostname\n- TestTranscript_RejectsLinkLocalIPv6\n- TestTranscript_RejectsLoopbackURL\n- TestUpdateCard_RejectsMetadataURL\n- TestUpdateCard_RejectsNonHTTPScheme\n Co-Authored-By: Claude Opus 4.7 --- .../internal/handlers/registry_test.go | 8 ++++---- .../internal/handlers/transcript_test.go | 20 +++++++++---------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/workspace-server/internal/handlers/registry_test.go b/workspace-server/internal/handlers/registry_test.go index dd98c9622..51c955200 100644 --- a/workspace-server/internal/handlers/registry_test.go +++ b/workspace-server/internal/handlers/registry_test.go @@ -629,9 +629,9 @@ func TestUpdateCard_DBError(t *testing.T) { func TestUpdateCard_RejectsMetadataURL(t *testing.T) { t.Setenv("MOLECULE_ENV", "production") - restore := setSSRFCheckForTest(true) - defer restore() mock := setupTestDB(t) + restore := setSSRFCheckForTest(true) + t.Cleanup(restore) setupTestRedis(t) broadcaster := newTestBroadcaster() handler := NewRegistryHandler(broadcaster) @@ -654,9 +654,9 @@ func TestUpdateCard_RejectsMetadataURL(t *testing.T) { } func TestUpdateCard_RejectsNonHTTPScheme(t *testing.T) { - restore := setSSRFCheckForTest(true) - defer restore() mock := setupTestDB(t) + restore := setSSRFCheckForTest(true) + t.Cleanup(restore) setupTestRedis(t) broadcaster := newTestBroadcaster() handler := NewRegistryHandler(broadcaster) diff --git a/workspace-server/internal/handlers/transcript_test.go b/workspace-server/internal/handlers/transcript_test.go index 6a2c9e470..807d7901e 100644 --- a/workspace-server/internal/handlers/transcript_test.go +++ b/workspace-server/internal/handlers/transcript_test.go @@ -119,9 +119,9 @@ func TestTranscript_ProxyPropagatesAllowlistedQueryParams(t *testing.T) { func TestTranscript_RejectsCloudMetadataIP(t *testing.T) { t.Setenv("MOLECULE_ENV", "production") - restore := setSSRFCheckForTest(true) - defer restore() mock := setupTestDB(t) + restore := setSSRFCheckForTest(true) + t.Cleanup(restore) setupTestRedis(t) h := NewTranscriptHandler() @@ -138,9 +138,9 @@ func TestTranscript_RejectsCloudMetadataIP(t *testing.T) { } func TestTranscript_RejectsNonHTTPScheme(t *testing.T) { - restore := setSSRFCheckForTest(true) - defer restore() mock := setupTestDB(t) + restore := setSSRFCheckForTest(true) + t.Cleanup(restore) setupTestRedis(t) h := NewTranscriptHandler() @@ -157,9 +157,9 @@ func TestTranscript_RejectsNonHTTPScheme(t *testing.T) { } func TestTranscript_RejectsMetadataHostname(t *testing.T) { - restore := setSSRFCheckForTest(true) - defer restore() mock := setupTestDB(t) + restore := setSSRFCheckForTest(true) + t.Cleanup(restore) setupTestRedis(t) h := NewTranscriptHandler() @@ -177,9 +177,9 @@ func TestTranscript_RejectsMetadataHostname(t *testing.T) { func TestTranscript_RejectsLinkLocalIPv6(t *testing.T) { t.Setenv("MOLECULE_ENV", "production") - restore := setSSRFCheckForTest(true) - defer restore() mock := setupTestDB(t) + restore := setSSRFCheckForTest(true) + t.Cleanup(restore) setupTestRedis(t) h := NewTranscriptHandler() @@ -197,9 +197,9 @@ func TestTranscript_RejectsLinkLocalIPv6(t *testing.T) { func TestTranscript_RejectsLoopbackURL(t *testing.T) { t.Setenv("MOLECULE_ENV", "production") - restore := setSSRFCheckForTest(true) - defer restore() mock := setupTestDB(t) + restore := setSSRFCheckForTest(true) + t.Cleanup(restore) setupTestRedis(t) h := NewTranscriptHandler() -- 2.52.0 From 2f585ab183f585791edf405517cba2bfec9af0fe Mon Sep 17 00:00:00 2001 From: "Molecule AI Dev Engineer A (Kimi)" Date: Fri, 5 Jun 2026 19:03:52 +0000 Subject: [PATCH 3/4] fix(tests): stop setupTestDB from unconditionally disabling SSRF guard Issue #807: setupTestDB called setSSRFCheckForTest(false) for every test, which silently overrode the SSRF guard in regression tests that explicitly enable it (transcript_test.go and registry_test.go). The later call to setSSRFCheckForTest(true) in those tests was being masked. Change setupTestDB to preserve (not mutate) the existing SSRF state across the test boundary. Tests that genuinely need SSRF disabled can call setSSRFCheckForTest(false) inline. Co-Authored-By: Claude Opus 4.7 --- workspace-server/internal/handlers/handlers_test.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/workspace-server/internal/handlers/handlers_test.go b/workspace-server/internal/handlers/handlers_test.go index 2ee8ed4b4..dc9aa697f 100644 --- a/workspace-server/internal/handlers/handlers_test.go +++ b/workspace-server/internal/handlers/handlers_test.go @@ -94,11 +94,12 @@ func setupTestDB(t *testing.T) sqlmock.Sqlmock { mockDB.Close() }) - // Disable SSRF checks for the duration of this test only. Restore - // the previous state via t.Cleanup so that TestIsSafeURL_* tests - // (which run with SSRF enabled) are not affected by state leak. - restore := setSSRFCheckForTest(false) - t.Cleanup(restore) + // Preserve SSRF state across this test. Individual tests that need + // SSRF disabled can call setSSRFCheckForTest(false) themselves. + // This prevents setupTestDB from silently overriding SSRF guards in + // regression tests that assert the guard is active (issue #807). + prevSSRF := ssrfCheckEnabled + t.Cleanup(func() { ssrfCheckEnabled = prevSSRF }) // The wsauth.platform_inbound_secret cache (#189) is package-level // state in another package — without a reset between tests, a -- 2.52.0 From db3908964b89077bebcda37d01366a980206097b Mon Sep 17 00:00:00 2001 From: "Molecule AI Dev Engineer A (Kimi)" Date: Mon, 15 Jun 2026 21:09:45 +0000 Subject: [PATCH 4/4] fix(security): validate agent_card.url in Register + block redirect SSRF in transcript proxy (#2132) - RegistryHandler.Register now validates the embedded agent_card.url with isSafeURL before persisting the reconciled card. Previously only UpdateCard checked it, so a re-registration could plant a metadata URL. - TranscriptHandler now sets http.Client.CheckRedirect to re-run isSafeURL on every redirect hop. Prevents an attacker from redirecting a safe workspace URL to an internal/metadata target and stealing the bearer token. - Add tests: Register rejects bad embedded URL; transcript proxy blocks 302 -> internal host. Addresses CR2 + Researcher REQUEST_CHANGES on #2132. --- .../internal/handlers/registry.go | 17 ++++++++++ .../internal/handlers/registry_test.go | 34 +++++++++++++++++++ .../internal/handlers/transcript.go | 21 +++++++++++- .../internal/handlers/transcript_test.go | 34 +++++++++++++++++++ 4 files changed, 105 insertions(+), 1 deletion(-) diff --git a/workspace-server/internal/handlers/registry.go b/workspace-server/internal/handlers/registry.go index 0a2563853..ec3112abc 100644 --- a/workspace-server/internal/handlers/registry.go +++ b/workspace-server/internal/handlers/registry.go @@ -364,6 +364,23 @@ func (h *RegistryHandler) Register(c *gin.Context) { } agentCardStr := string(reconciledCard) + // SSRF guard: the embedded agent_card.url is attacker-writable and is + // used by the transcript proxy regardless of delivery mode. Reject the + // same unsafe targets that UpdateCard rejects. Empty URL is allowed + // (poll-mode workspaces may omit it). #2130 #2132 + { + var cardURL struct { + URL string `json:"url"` + } + if jsonErr := json.Unmarshal(reconciledCard, &cardURL); jsonErr == nil && cardURL.URL != "" { + if err := isSafeURL(cardURL.URL); err != nil { + log.Printf("Registry register: workspace %s agent_card url rejected: %v", payload.ID, err) + c.JSON(http.StatusBadRequest, gin.H{"error": "workspace URL not allowed"}) + return + } + } + } + // urlForUpsert: poll-mode workspaces don't need a URL. Empty input // becomes NULL via sql.NullString so the row's URL stays clean (the // CASE below also preserves an existing provisioner-set URL, which diff --git a/workspace-server/internal/handlers/registry_test.go b/workspace-server/internal/handlers/registry_test.go index 51c955200..22e380da4 100644 --- a/workspace-server/internal/handlers/registry_test.go +++ b/workspace-server/internal/handlers/registry_test.go @@ -678,6 +678,40 @@ func TestUpdateCard_RejectsNonHTTPScheme(t *testing.T) { } } +// TestRegister_RejectsBadEmbeddedURL verifies that the agent_card.url field +// is validated in Register, not just UpdateCard. The URL is attacker-writable +// and used by the transcript proxy, so a metadata/loopback target must be +// rejected before the row is persisted. #2132 +func TestRegister_RejectsBadEmbeddedURL(t *testing.T) { + mock := setupTestDB(t) + restore := setSSRFCheckForTest(true) + t.Cleanup(restore) + setupTestRedis(t) + broadcaster := newTestBroadcaster() + handler := NewRegistryHandler(broadcaster) + + // Bootstrap path — no live tokens. + mock.ExpectQuery("SELECT COUNT\\(\\*\\) FROM workspace_auth_tokens"). + WithArgs("ws-bad-card-url"). + WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(0)) + // resolveDeliveryMode: no row yet, default push. + mock.ExpectQuery(`SELECT delivery_mode, runtime FROM workspaces WHERE id`). + WithArgs("ws-bad-card-url"). + WillReturnError(sql.ErrNoRows) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + body := `{"id":"ws-bad-card-url","url":"http://example.com","agent_card":{"name":"bad","url":"http://169.254.169.254/latest/meta-data/"}}` + c.Request = httptest.NewRequest("POST", "/registry/register", bytes.NewBufferString(body)) + c.Request.Header.Set("Content-Type", "application/json") + + handler.Register(c) + + if w.Code != http.StatusBadRequest { + t.Errorf("expected 400 for bad embedded agent_card.url, 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 2920f3b7e..2b5287333 100644 --- a/workspace-server/internal/handlers/transcript.go +++ b/workspace-server/internal/handlers/transcript.go @@ -12,6 +12,7 @@ package handlers import ( "context" + "errors" "io" "log" "net/http" @@ -22,6 +23,10 @@ import ( "github.com/gin-gonic/gin" ) +// errTranscriptRedirectBlocked is returned by the transcript proxy's +// CheckRedirect when a redirect target fails the SSRF policy. +var errTranscriptRedirectBlocked = errors.New("redirect target blocked by SSRF policy") + // TranscriptHandler proxies /workspaces/:id/transcript to the workspace agent. type TranscriptHandler struct { httpClient *http.Client @@ -29,7 +34,21 @@ type TranscriptHandler struct { func NewTranscriptHandler() *TranscriptHandler { return &TranscriptHandler{ - httpClient: &http.Client{Timeout: 15 * time.Second}, + httpClient: &http.Client{ + Timeout: 15 * time.Second, + CheckRedirect: func(req *http.Request, via []*http.Request) error { + // Go's default client follows up to 10 redirects and copies + // headers (including Authorization) to each hop. A redirect to + // an internal/metadata target would bypass the front-door + // isSafeURL check because that check only inspected the first + // URL. Re-validate every hop. #2130 #2132 + if err := isSafeURL(req.URL.String()); err != nil { + log.Printf("transcript: redirect to %s rejected: %v", req.URL.String(), err) + return errTranscriptRedirectBlocked + } + return nil + }, + }, } } diff --git a/workspace-server/internal/handlers/transcript_test.go b/workspace-server/internal/handlers/transcript_test.go index 807d7901e..19fae49d2 100644 --- a/workspace-server/internal/handlers/transcript_test.go +++ b/workspace-server/internal/handlers/transcript_test.go @@ -215,6 +215,40 @@ func TestTranscript_RejectsLoopbackURL(t *testing.T) { } } +// TestTranscript_BlocksRedirectToInternalHost verifies that a 302 redirect to +// an SSRF-forbidden target is not followed. The initial URL is safe (a test +// server), but the redirect target points to cloud metadata. Without the +// per-hop check the Authorization bearer would be forwarded to the metadata +// endpoint. #2132 +func TestTranscript_BlocksRedirectToInternalHost(t *testing.T) { + allowLoopbackForTest(t) + mock := setupTestDB(t) + restore := setSSRFCheckForTest(true) + t.Cleanup(restore) + setupTestRedis(t) + h := NewTranscriptHandler() + + // First server is a safe-looking target that redirects to IMDS. + stub := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Location", "http://169.254.169.254/latest/meta-data/") + w.WriteHeader(http.StatusFound) + })) + defer stub.Close() + + wsID := expectWorkspaceURLLookup(mock, stub.URL) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: wsID}} + c.Request = httptest.NewRequest("GET", "/workspaces/"+wsID+"/transcript", nil) + c.Request.Header.Set("Authorization", "Bearer test-token") + h.Get(c) + + if w.Code != http.StatusBadGateway { + t.Errorf("expected 502 when redirect is blocked, got %d: %s", w.Code, w.Body.String()) + } +} + func TestTranscript_UnreachableWorkspaceReturns502(t *testing.T) { allowLoopbackForTest(t) mock := setupTestDB(t) -- 2.52.0