2026-06-23 12:02:40 +00:00
3 changed files with 179 additions and 1 deletions
@@ -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)
@@ -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)
+	}
+}
@@ -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) —