diff --git a/workspace-server/internal/handlers/chat_files.go b/workspace-server/internal/handlers/chat_files.go index 91baf5ca5..e232e7a4d 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: re-validate the URL at forward time even though + // registration-time validation already ran. The DB value could have + // been tampered with or the registration-time policy could have + // changed since the row was written (#2316). + if err := isSafeURL(wsURL); err != nil { + log.Printf("chat_files %s: workspace %s URL 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..27d8047d8 100644 --- a/workspace-server/internal/handlers/chat_files_test.go +++ b/workspace-server/internal/handlers/chat_files_test.go @@ -717,3 +717,78 @@ 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()) + } +}