diff --git a/workspace-server/internal/handlers/chat_files.go b/workspace-server/internal/handlers/chat_files.go index 91baf5ca5..24170bccf 100644 --- a/workspace-server/internal/handlers/chat_files.go +++ b/workspace-server/internal/handlers/chat_files.go @@ -196,10 +196,15 @@ func resolveWorkspaceForwardCreds(c *gin.Context, ctx context.Context, workspace c.JSON(http.StatusServiceUnavailable, gin.H{"error": "workspace url not registered yet"}) return "", "", false } - // Trust note: workspaces.url passes validateAgentURL at /registry/ - // register write time, blocking SSRF-shaped URLs. We rely on that - // upstream gate rather than re-validating here. Tracked at #2316 - // for follow-up: forward-time re-validation as defense-in-depth. + // Defense-in-depth for #2316: workspaces.url is validated at + // registration time, but the DB row can be stale/tampered and the + // SSRF policy can tighten. Re-validate immediately before attaching + // the inbound secret to an outbound forward. + if err := isSafeURL(wsURL); err != nil { + log.Printf("chat_files %s: unsafe workspace URL for %s rejected: %v", op, workspaceID, err) + c.JSON(http.StatusBadRequest, gin.H{"error": "workspace URL not allowed"}) + return "", "", false + } secret, healed, err := readOrLazyHealInboundSecret(ctx, workspaceID, "chat_files "+op) if err != nil { diff --git a/workspace-server/internal/handlers/chat_files_test.go b/workspace-server/internal/handlers/chat_files_test.go index 3f6e3edb8..7229f78ec 100644 --- a/workspace-server/internal/handlers/chat_files_test.go +++ b/workspace-server/internal/handlers/chat_files_test.go @@ -414,6 +414,56 @@ func TestChatUpload_WorkspaceUnreachable(t *testing.T) { } } +func TestChatUpload_RejectsMetadataWorkspaceURL(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + restore := setSSRFCheckForTest(true) + t.Cleanup(restore) + + wsID := "00000000-0000-0000-0000-000000000047" + 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.Fatalf("expected 400 for metadata workspace URL, got %d: %s", w.Code, w.Body.String()) + } + if !strings.Contains(w.Body.String(), "workspace URL not allowed") { + t.Errorf("expected unsafe URL error, got: %s", w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("sqlmock expectations not met: %v", err) + } +} + +func TestChatUpload_RejectsNonHTTPWorkspaceURL(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + restore := setSSRFCheckForTest(true) + t.Cleanup(restore) + + wsID := "00000000-0000-0000-0000-000000000048" + 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.Fatalf("expected 400 for non-HTTP workspace URL, got %d: %s", w.Code, w.Body.String()) + } + if !strings.Contains(w.Body.String(), "workspace URL not allowed") { + t.Errorf("expected unsafe URL error, got: %s", w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("sqlmock expectations not met: %v", err) + } +} + // TestChatUpload_BodyUnderCap_Forwards pins the lower edge of the new // 100 MB body cap (CTO 2026-05-19 directive on forensic a99ab0a1). // A multipart payload comfortably under the cap must reach the @@ -646,6 +696,54 @@ func TestChatDownload_NoInboundSecret_LazyHealFailure(t *testing.T) { } } +func TestChatDownload_RejectsMetadataWorkspaceURL(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + restore := setSSRFCheckForTest(true) + t.Cleanup(restore) + + wsID := "00000000-0000-0000-0000-000000000054" + 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.Fatalf("expected 400 for metadata workspace URL, got %d: %s", w.Code, w.Body.String()) + } + if !strings.Contains(w.Body.String(), "workspace URL not allowed") { + t.Errorf("expected unsafe URL error, got: %s", w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("sqlmock expectations not met: %v", err) + } +} + +func TestChatDownload_RejectsNonHTTPWorkspaceURL(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + restore := setSSRFCheckForTest(true) + t.Cleanup(restore) + + wsID := "00000000-0000-0000-0000-000000000055" + 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.Fatalf("expected 400 for non-HTTP workspace URL, got %d: %s", w.Code, w.Body.String()) + } + if !strings.Contains(w.Body.String(), "workspace URL not allowed") { + t.Errorf("expected unsafe URL error, got: %s", w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("sqlmock expectations not met: %v", err) + } +} + func TestChatDownload_ForwardsToWorkspace_HappyPath(t *testing.T) { mock := setupTestDB(t) setupTestRedis(t)