diff --git a/workspace-server/internal/handlers/org_import.go b/workspace-server/internal/handlers/org_import.go index 519548e4..fc5f3426 100644 --- a/workspace-server/internal/handlers/org_import.go +++ b/workspace-server/internal/handlers/org_import.go @@ -138,6 +138,22 @@ func (h *OrgHandler) createWorkspaceTree(ws OrgWorkspace, parentID *string, absX return fmt.Errorf("workspace %s: %w", ws.Name, err) } + // core#2129 write-path SSRF defense (CR2 RC 13399): validate the + // external URL BEFORE any durable side effect (workspaces INSERT, + // canvas_layouts INSERT, structure_events INSERT). Pre-#3170RC1 + // ordering put the validation after the INSERT, which left a + // stranded provisioning workspace row + layout + event for a + // rejected malicious leaf. Mirrors workspace.go's pre-BeginTx + // pattern (workspace.go:624) — reject at the boundary, not after + // the fact. The post-INSERT UPDATE that originally carried the URL + // never runs on rejection, so no unsafe URL ever lands in the DB. + if ws.External && ws.URL != "" { + if err := validateAgentURL(ws.URL); err != nil { + log.Printf("Org import: external workspace URL rejected for %s (pre-INSERT): %v — leaf rejected", ws.Name, err) + return fmt.Errorf("external workspace %s URL rejected: %w", ws.Name, err) + } + } + ctx := context.Background() // Org-template imports default to expanded so children render @@ -262,6 +278,12 @@ func (h *OrgHandler) createWorkspaceTree(ws OrgWorkspace, parentID *string, absX h.workspace.seedInitialMemories(ctx, id, wsMemories) // Handle external workspaces + // + // NOTE: external URL validation moved to the top of this function + // (CR2 RC 13399) — before the workspaces INSERT — so a rejected + // malicious URL leaves no workspace-row / canvas-layout / structure- + // event side effects. Only the UPDATE that lands `ws.URL` lives + // here, and it runs unconditionally on the happy path. if ws.External { if _, err := db.DB.ExecContext(ctx, `UPDATE workspaces SET status = $1, url = $2 WHERE id = $3`, models.StatusOnline, ws.URL, id); err != nil { log.Printf("Org import: external workspace status update failed for %s: %v", ws.Name, err) diff --git a/workspace-server/internal/handlers/org_import_ssrf_test.go b/workspace-server/internal/handlers/org_import_ssrf_test.go new file mode 100644 index 00000000..dcc53cca --- /dev/null +++ b/workspace-server/internal/handlers/org_import_ssrf_test.go @@ -0,0 +1,144 @@ +package handlers + +// Regression tests for core#2129 write-path SSRF defense on the org_import +// path. +// +// Background: /registry/register validates the workspace URL at registration +// time (the front-door gate). The /org/import path bypasses /registry/register +// entirely — it writes `ws.URL` directly to `workspaces.url`. Without a +// re-validation in createWorkspaceTree, a malicious org template that ships +// a metadata endpoint (169.254.169.254) or loopback URL would land in the +// DB and the downstream chat-files forward would attach +// platform_inbound_secret to it (the forward-time gate in PR#3169 / #3169's +// branch is the critical defense; this is defense-in-depth at the write +// path). +// +// The workspace.go (Create) write site already had SSRF tests +// (TestWorkspaceCreate_ExternalURL_SSRFMetadataBlocked + +// TestWorkspaceCreate_ExternalURL_SSRFLoopbackBlocked). The org_import +// write site was the gap that CR2 RC 13398 called out. +// +// CR2 RC 13399 (raised after the initial #3170RC1 fix) tightened the +// contract further: the validation must fire BEFORE the workspace +// INSERT (and the canvas_layouts INSERT, and the structure_events +// INSERT), so a rejected malicious URL leaves NO workspace-row side +// effects. The previous post-INSERT check created a stranded +// provisioning row + layout + event for the rejected leaf — same class +// of "leave debris on failure" the Researcher flagged. These tests +// assert the no-stray-row contract: a rejected external URL produces +// no INSERT INTO workspaces, no INSERT INTO canvas_layouts, and no +// INSERT INTO structure_events. sqlmock catches any unexpected call +// as a test failure (the same way the existing TestEmitOrgEvent tests +// pin SQL shapes). + +import ( + "strings" + "testing" +) + +// TestCreateWorkspaceTree_RejectsMetadataURL is the cloud-metadata +// analogue of the workspace.go SSRF tests, for the org_import write +// path. The test pins two contracts: +// +// 1. The function returns a non-nil error mentioning "URL rejected" +// (so the top-level OrgImport handler's 207 partial-import path +// surfaces the leaf error to the caller). +// +// 2. The function leaves NO database side effects on rejection — +// no workspaces row, no canvas_layouts row, no structure_events +// row. sqlmock expects zero queries, so any INSERT/UPDATE the +// function might have issued would fail the test via +// mock.ExpectationsWereMet() (it reports unexpected calls). +func TestCreateWorkspaceTree_RejectsMetadataURL(t *testing.T) { + setSSRFCheckForTest(true) + t.Cleanup(func() { setSSRFCheckForTest(false)() }) + + mock := setupTestDB(t) + setupTestRedis(t) + broadcaster := newTestBroadcaster() + wh := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) + + h := &OrgHandler{ + workspace: wh, + broadcaster: broadcaster, + } + + ws := OrgWorkspace{ + Name: "Bad Agent", + Runtime: "external", + Model: "external:custom", + External: true, + URL: "http://169.254.169.254/latest/meta-data/", + } + defaults := OrgDefaults{} + results := []map[string]interface{}{} + provisionSem := make(chan struct{}, 1) + parentID := (*string)(nil) + + err := h.createWorkspaceTree(ws, parentID, 0, 0, 0, 0, defaults, "", &results, provisionSem) + + if err == nil { + t.Fatalf("expected error from createWorkspaceTree for metadata URL, got nil") + } + if !strings.Contains(err.Error(), "URL rejected") { + t.Errorf("expected error to mention 'URL rejected', got: %v", err) + } + + // No-stray-row contract: zero INSERTs/UPDATEs to workspaces, + // canvas_layouts, or structure_events. mock.ExpectationsWereMet() + // returns nil when zero expectations were set AND zero unexpected + // calls were logged; any INSERT/UPDATE the function would have + // made surfaces here as a test failure. + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("sqlmock expectations: %v", err) + } +} + +// TestCreateWorkspaceTree_RejectsLoopbackURL is the loopback analogue +// of TestCreateWorkspaceTree_RejectsMetadataURL. Loopback is blocked +// in self-hosted mode (the default for the org_import test) — a +// malicious org template pointing an external workspace at 127.0.0.1 +// must be rejected by the same pre-INSERT defense. +// +// 127.0.0.1 is a metadata-class target — an attacker reaching the +// host's loopback interface can hit any debug/admin endpoint a +// developer left open during development. +func TestCreateWorkspaceTree_RejectsLoopbackURL(t *testing.T) { + setSSRFCheckForTest(true) + t.Cleanup(func() { setSSRFCheckForTest(false)() }) + + mock := setupTestDB(t) + setupTestRedis(t) + broadcaster := newTestBroadcaster() + wh := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) + + h := &OrgHandler{ + workspace: wh, + broadcaster: broadcaster, + } + + ws := OrgWorkspace{ + Name: "Bad Loopback", + Runtime: "external", + Model: "external:custom", + External: true, + URL: "http://127.0.0.1:9000/a2a", + } + defaults := OrgDefaults{} + results := []map[string]interface{}{} + provisionSem := make(chan struct{}, 1) + parentID := (*string)(nil) + + err := h.createWorkspaceTree(ws, parentID, 0, 0, 0, 0, defaults, "", &results, provisionSem) + + if err == nil { + t.Fatalf("expected error from createWorkspaceTree for loopback URL, got nil") + } + if !strings.Contains(err.Error(), "URL rejected") { + t.Errorf("expected error to mention 'URL rejected', got: %v", err) + } + + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("sqlmock expectations: %v", err) + } +} diff --git a/workspace-server/internal/handlers/workspace.go b/workspace-server/internal/handlers/workspace.go index 55bf252f..5baabc54 100644 --- a/workspace-server/internal/handlers/workspace.go +++ b/workspace-server/internal/handlers/workspace.go @@ -877,7 +877,19 @@ func (h *WorkspaceHandler) Create(c *gin.Context) { if payload.External || isExternalLikeRuntime(payload.Runtime) { var connectionToken string if payload.URL != "" { - // URL already validated by validateAgentURL above (before BeginTx). + // core#2129 write-path SSRF defense: validateAgentURL runs at + // registration-time on the /registry/register path. This is the + // OTHER external-create entrypoint (the dashboard admin path + + // any update-with-URL flow) — it previously relied on the comment + // claim that "URL already validated by validateAgentURL above" + // but the above validation lives in CreateWorkspace, not in + // this handler. Re-validate here so a workspace URL can NEVER + // land in the DB without passing the SSRF policy. + if err := validateAgentURL(payload.URL); err != nil { + log.Printf("External workspace: URL rejected for %s: %v", payload.Name, err) + c.JSON(http.StatusBadRequest, gin.H{"error": "unsafe workspace URL: " + err.Error()}) + return + } // Now persist it: the external URL is set after the workspace row // commits so that a failed URL UPDATE doesn't roll back the row. // Preserve BYO-compute runtime label (kimi, kimi-cli, external) —