workspace-server/internal/handlers/chat_files.go

@@ -98,6 +98,31 @@ func NewChatFilesHandler(t *TemplatesHandler) *ChatFilesHandler {
 			// surfaces "connection too slow" — distinct from the file-size
 			// pre-flight that returns immediately before any network I/O.
 			Timeout: 1200 * time.Second,
+			// core#2129 RC 13395/CR2: disable redirects entirely. A safe
+			// workspace URL that 302s to a metadata/loopback/non-http
+			// target would otherwise be followed by the default client
+			// and the platform_inbound_secret would be forwarded to the
+			// unsafe target. ErrUseLastResponse surfaces the 3xx to the
+			// caller; we never chase. Mirrors the transcript-proxy
+			// pattern (transcript.go:73). The dial-time IP guard below
+			// is the belt-and-suspenders for any future code that
+			// re-enables redirects.
+			CheckRedirect: func(req *http.Request, via []*http.Request) error {
+				return http.ErrUseLastResponse
+			},
+			// Dial-time IP guard: safeDialer.Control inspects the
+			// POST-resolution IP on every dial and rejects if it
+			// violates isSafeURL. Catches DNS rebinding (where the
+			// preflight lookup returned a safe IP and the dial resolves
+			// to a different one) and any code that bypasses the
+			// redirect guard. Same pattern as transcript-proxy
+			// (transcript.go:91-98).
+			Transport: &http.Transport{
+				DialContext:         safeDialer().DialContext,
+				MaxIdleConns:        100,
+				IdleConnTimeout:     90 * time.Second,
+				TLSHandshakeTimeout: 5 * time.Second,
+			},
 		},
 	}
 }

workspace-server/internal/handlers/chat_files_test.go

@@ -22,6 +22,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"strings"
+	"sync/atomic"
 	"testing"
 
 	"github.com/DATA-DOG/go-sqlmock"
@@ -815,3 +816,196 @@ func TestChatDownload_404FromWorkspacePropagated(t *testing.T) {
 		t.Errorf("expected 404 propagated, got %d", w.Code)
 	}
 }
+
+// SSRF defense-in-depth tests for #2316 — forward-time URL validation in
+// resolveWorkspaceForwardCreds. Upload + Download share the helper; both
+// paths are pinned so a partial regression (one handler drifting away from
+// the shared helper) fails in CI.
+
+func TestChatUpload_RejectsMetadataURL(t *testing.T) {
+	mock := setupTestDB(t)
+	setupTestRedis(t)
+	setSSRFCheckForTest(true)
+
+	wsID := "00000000-0000-0000-0000-000000000060"
+	expectURL(mock, wsID, "http://169.254.169.254/latest/meta-data/")
+
+	h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil))
+	body, ct := uploadFixture(t)
+	c, w := makeUploadRequest(t, wsID, body, ct)
+	h.Upload(c)
+
+	if w.Code != http.StatusBadRequest {
+		t.Errorf("expected 400 for metadata URL in upload, got %d: %s", w.Code, w.Body.String())
+	}
+}
+
+func TestChatUpload_RejectsNonHTTPScheme(t *testing.T) {
+	mock := setupTestDB(t)
+	setupTestRedis(t)
+	setSSRFCheckForTest(true)
+
+	wsID := "00000000-0000-0000-0000-000000000061"
+	expectURL(mock, wsID, "file:///etc/passwd")
+
+	h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil))
+	body, ct := uploadFixture(t)
+	c, w := makeUploadRequest(t, wsID, body, ct)
+	h.Upload(c)
+
+	if w.Code != http.StatusBadRequest {
+		t.Errorf("expected 400 for file:// scheme in upload, got %d: %s", w.Code, w.Body.String())
+	}
+}
+
+func TestChatDownload_RejectsMetadataURL(t *testing.T) {
+	mock := setupTestDB(t)
+	setupTestRedis(t)
+	setSSRFCheckForTest(true)
+
+	wsID := "00000000-0000-0000-0000-000000000062"
+	expectURL(mock, wsID, "http://169.254.169.254/latest/meta-data/")
+
+	h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil))
+	c, w := makeDownloadRequest(t, wsID, "/workspace/foo.txt")
+	h.Download(c)
+
+	if w.Code != http.StatusBadRequest {
+		t.Errorf("expected 400 for metadata URL in download, got %d: %s", w.Code, w.Body.String())
+	}
+}
+
+func TestChatDownload_RejectsNonHTTPScheme(t *testing.T) {
+	mock := setupTestDB(t)
+	setupTestRedis(t)
+	setSSRFCheckForTest(true)
+
+	wsID := "00000000-0000-0000-0000-000000000063"
+	expectURL(mock, wsID, "file:///etc/passwd")
+
+	h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil))
+	c, w := makeDownloadRequest(t, wsID, "/workspace/foo.txt")
+	h.Download(c)
+
+	if w.Code != http.StatusBadRequest {
+		t.Errorf("expected 400 for file:// scheme in download, got %d: %s", w.Code, w.Body.String())
+	}
+}
+// ---------- core#2129 RC 13395/CR2: redirect + DNS-rebind bypass ----------
+
+// safeWorkspaceURLServer is a tiny upstream that 302-redirects every request
+// to the metadata endpoint. Used to prove the chat-files client refuses
+// to follow a redirect to a blocked target (RC 13395).
+//
+// Returns (workspaceURL, metadataURL, metadataHitCount) so each test can
+// assert the metadata target got the expected number of hits (0 for the
+// redirect-blocked cases). The metadata counter is the load-bearing
+// signal — w.Code can't be used for POST-based 3xx responses because
+// Go's http.Redirect omits the body for non-GET/HEAD, so a recorder-
+// based check would not detect the 302 in those cases.
+func safeWorkspaceURLServer(t *testing.T) (workspaceURL string, metadataURL string, metadataHits *int32) {
+	t.Helper()
+	var hits int32
+	metadata := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		atomic.AddInt32(&hits, 1)
+		w.WriteHeader(http.StatusOK)
+		_, _ = w.Write([]byte("metadata"))
+	}))
+	t.Cleanup(metadata.Close)
+
+	safe := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		http.Redirect(w, r, metadata.URL+"/latest/meta-data/", http.StatusFound)
+	}))
+	t.Cleanup(safe.Close)
+	return safe.URL, metadata.URL, &hits
+}
+
+// TestChatDownload_RedirectToMetadata_NotFollowed is the RC 13395 regression:
+// a SAFE workspace URL (the one that passes isSafeURL on the front-door
+// check) that 302-redirects to a metadata target must NOT be followed.
+// The chat-files client returns the 3xx to the caller without chasing —
+// the platform_inbound_secret bearer is never attached to the unsafe
+// redirect target.
+func TestChatDownload_RedirectToMetadata_NotFollowed(t *testing.T) {
+	mock := setupTestDB(t)
+	setupTestRedis(t)
+	// Disable the front-door isSafeURL check so the test URL passes the
+	// workspace-URL gate — this test focuses on the REDIRECT guard,
+	// not the front-door (which the existing TestChatDownload_RejectsMetadataWorkspaceURL
+	// already covers). The redirect guard MUST still refuse to follow
+	// the 302 to the metadata target even when the front-door is happy.
+	restore := setSSRFCheckForTest(false)
+	t.Cleanup(restore)
+
+	safeURL, _, metadataHits := safeWorkspaceURLServer(t)
+	wsID := "00000000-0000-0000-0000-000000000060"
+	expectURL(mock, wsID, safeURL)
+	// The secret IS read (the redirect guard fires AFTER the secret is
+	// attached — the guard is on the http.Client, not on the secret
+	// fetch). We mock the secret so the test can reach the HTTP call.
+	expectInboundSecret(mock, wsID, "tok-redirect-test")
+
+	h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil))
+	c, w := makeDownloadRequest(t, wsID, "/workspace/foo.txt")
+	h.Download(c)
+
+	// 302 from the safe URL — surfaced to the caller without chasing.
+	// The default client would follow to the metadata target; our
+	// client uses CheckRedirect: http.ErrUseLastResponse to refuse.
+	if w.Code != http.StatusFound {
+		t.Errorf("expected 302 (redirect surfaced, not followed), got %d: %s", w.Code, w.Body.String())
+	}
+	// And the metadata target was NOT hit (it would have returned 200 +
+	// the body "metadata" if the client had followed the redirect).
+	if strings.Contains(w.Body.String(), "metadata") {
+		t.Errorf("response body looks like the metadata target was fetched: %s", w.Body.String())
+	}
+	// Authoritative signal: the metadata server got zero hits.
+	if got := atomic.LoadInt32(metadataHits); got != 0 {
+		t.Errorf("metadata target was hit %d times — the client followed the redirect", got)
+	}
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Errorf("sqlmock expectations not met: %v", err)
+	}
+}
+
+// TestChatUpload_RedirectToMetadata_NotFollowed is the same regression
+// for the upload path. A SAFE workspace URL that 302-redirects to
+// metadata must NOT be followed (a 302 from a POST would have the
+// http.Client follow by default; ErrUseLastResponse refuses).
+//
+// Note: we can't assert w.Code on a POST-based 3xx response because
+// Go's http.Redirect omits the body for non-GET/HEAD, so a recorder-
+// based check (which only sees the status when a body byte is written)
+// would not detect the 302. The metadata-hit counter is the load-bearing
+// signal — if the client followed, the metadata server would have been
+// hit.
+func TestChatUpload_RedirectToMetadata_NotFollowed(t *testing.T) {
+	mock := setupTestDB(t)
+	setupTestRedis(t)
+	// Same setup as the download test: disable the front-door so the
+	// redirect guard is the gate under test.
+	restore := setSSRFCheckForTest(false)
+	t.Cleanup(restore)
+
+	safeURL, _, metadataHits := safeWorkspaceURLServer(t)
+	wsID := "00000000-0000-0000-0000-000000000061"
+	expectURL(mock, wsID, safeURL)
+	// The secret IS read (redirect guard is on the http.Client, not on
+	// the secret fetch).
+	expectInboundSecret(mock, wsID, "tok-redirect-test-upload")
+
+	h := NewChatFilesHandler(NewTemplatesHandler(t.TempDir(), nil, nil))
+	c, _ := makeUploadRequest(t, wsID, bytes.NewBufferString("hi"), "application/octet-stream")
+	h.Upload(c)
+
+	// Authoritative signal: the metadata server got zero hits.
+	// If the client had followed the 302, this counter would be 1.
+	if got := atomic.LoadInt32(metadataHits); got != 0 {
+		t.Errorf("metadata target was hit %d times — the client followed the redirect", got)
+	}
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Errorf("sqlmock expectations not met: %v", err)
+	}
+}
+