From 2efee0c6e5914fbc61612a4888a727ed0a84b3b2 Mon Sep 17 00:00:00 2001 From: Vignesh Date: Fri, 17 Jul 2026 20:21:53 +0530 Subject: [PATCH 1/2] fix(security): SSRF protection for approval callback_url (#435) The approval callback_url field was accepted without SSRF validation and dispatched via a plain http.Client, allowing an attacker to use the control plane as a proxy to internal services (cloud metadata, RFC-1918, loopback). Changes: - Validate callback_url at registration time using services.ValidateWebhookURL() to reject private/internal targets (localhost, 169.254.x, 10.x, 172.16.x, 192.168.x, ::1) before the URL is persisted. - Replace the plain http.Client in notifyApprovalCallback with services.NewSSRFSafeClient() which enforces DNS-rebinding-safe private-IP blocking at dial time. - Add comprehensive test coverage for both registration rejection and runtime transport enforcement. Fixes #435 --- .../internal/handlers/execute_approval.go | 14 ++ .../handlers/execute_approval_ssrf_test.go | 211 ++++++++++++++++++ .../internal/handlers/webhook_approval.go | 5 +- .../handlers/webhook_approval_test.go | 6 + 4 files changed, 235 insertions(+), 1 deletion(-) create mode 100644 control-plane/internal/handlers/execute_approval_ssrf_test.go diff --git a/control-plane/internal/handlers/execute_approval.go b/control-plane/internal/handlers/execute_approval.go index c089443b7..8e7eba8c6 100644 --- a/control-plane/internal/handlers/execute_approval.go +++ b/control-plane/internal/handlers/execute_approval.go @@ -8,6 +8,7 @@ import ( "github.com/Agent-Field/agentfield/control-plane/internal/events" "github.com/Agent-Field/agentfield/control-plane/internal/logger" + "github.com/Agent-Field/agentfield/control-plane/internal/services" "github.com/Agent-Field/agentfield/control-plane/pkg/types" "github.com/gin-gonic/gin" @@ -111,6 +112,19 @@ func (c *approvalController) handleRequestApproval(ctx *gin.Context) { return } + // SSRF protection: validate callback_url before storing it. Reject + // URLs targeting private/internal addresses (cloud metadata, localhost, + // RFC-1918) to prevent the control plane from being used as a proxy. + if req.CallbackURL != "" { + if err := services.ValidateWebhookURL(req.CallbackURL); err != nil { + ctx.JSON(http.StatusBadRequest, gin.H{ + "error": "invalid_callback_url", + "message": fmt.Sprintf("callback_url rejected: %v", err), + }) + return + } + } + now := time.Now().UTC() statusReason := "waiting_for_approval" approvalStatus := "pending" diff --git a/control-plane/internal/handlers/execute_approval_ssrf_test.go b/control-plane/internal/handlers/execute_approval_ssrf_test.go new file mode 100644 index 000000000..27a6a4be1 --- /dev/null +++ b/control-plane/internal/handlers/execute_approval_ssrf_test.go @@ -0,0 +1,211 @@ +package handlers + +import ( + "bytes" + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/Agent-Field/agentfield/control-plane/internal/services" + "github.com/Agent-Field/agentfield/control-plane/pkg/types" + + "github.com/gin-gonic/gin" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// --------------------------------------------------------------------------- +// SSRF protection tests for approval callback_url (issue #435) +// +// These tests are NOT marked t.Parallel() because they interact with the +// global services.SetWebhookAllowedHosts state which other tests in this +// package also mutate. Running them sequentially avoids races. +// --------------------------------------------------------------------------- + +func setupRunningExecution(t *testing.T, store *testExecutionStorage, execID, agentID string) { + t.Helper() + now := time.Now().UTC() + require.NoError(t, store.CreateExecutionRecord(context.Background(), &types.Execution{ + ExecutionID: execID, + RunID: "run-1", + AgentNodeID: agentID, + Status: types.ExecutionStatusRunning, + StartedAt: now, + CreatedAt: now, + })) + require.NoError(t, store.StoreWorkflowExecution(context.Background(), &types.WorkflowExecution{ + ExecutionID: execID, + WorkflowID: "wf-1", + RunID: ptr("run-1"), + AgentNodeID: agentID, + Status: types.ExecutionStatusRunning, + StartedAt: now, + })) +} + +func TestRequestApprovalHandler_RejectsPrivateCallbackURL(t *testing.T) { + gin.SetMode(gin.TestMode) + + // Clear global allowlist to ensure SSRF validation is strict. + services.SetWebhookAllowedHosts(nil) + t.Cleanup(func() { services.SetWebhookAllowedHosts(nil) }) + + tests := []struct { + name string + callbackURL string + wantReject bool + }{ + { + name: "localhost is rejected", + callbackURL: "http://localhost:8080/callback", + wantReject: true, + }, + { + name: "127.0.0.1 is rejected", + callbackURL: "http://127.0.0.1:9000/callback", + wantReject: true, + }, + { + name: "RFC-1918 10.x is rejected", + callbackURL: "http://10.0.0.1/callback", + wantReject: true, + }, + { + name: "RFC-1918 172.16.x is rejected", + callbackURL: "http://172.16.0.5:3000/callback", + wantReject: true, + }, + { + name: "RFC-1918 192.168.x is rejected", + callbackURL: "http://192.168.1.1/callback", + wantReject: true, + }, + { + name: "AWS metadata endpoint is rejected", + callbackURL: "http://169.254.169.254/latest/meta-data/", + wantReject: true, + }, + { + name: "IPv6 loopback is rejected", + callbackURL: "http://[::1]:8080/callback", + wantReject: true, + }, + { + name: "empty callback_url is allowed (optional field)", + callbackURL: "", + wantReject: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + agent := &types.AgentNode{ID: "agent-ssrf"} + store := newTestExecutionStorage(agent) + setupRunningExecution(t, store, "exec-ssrf", "agent-ssrf") + + router := gin.New() + router.POST("/api/v1/agents/:node_id/executions/:execution_id/request-approval", + AgentScopedRequestApprovalHandler(store)) + + payload := map[string]any{ + "approval_request_id": "req-ssrf-test", + "approval_request_url": "https://hub.example.com/review/req-ssrf-test", + } + if tc.callbackURL != "" { + payload["callback_url"] = tc.callbackURL + } + body, _ := json.Marshal(payload) + + req := httptest.NewRequest(http.MethodPost, "/api/v1/agents/agent-ssrf/executions/exec-ssrf/request-approval", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + resp := httptest.NewRecorder() + router.ServeHTTP(resp, req) + + if tc.wantReject { + assert.Equal(t, http.StatusBadRequest, resp.Code, + "expected rejection for callback_url=%q", tc.callbackURL) + var result map[string]any + require.NoError(t, json.Unmarshal(resp.Body.Bytes(), &result)) + assert.Equal(t, "invalid_callback_url", result["error"], + "error code should be invalid_callback_url for %q", tc.callbackURL) + } else { + assert.Equal(t, http.StatusOK, resp.Code, + "expected success for callback_url=%q, got body: %s", tc.callbackURL, resp.Body.String()) + } + }) + } +} + +func TestRequestApprovalHandler_PublicCallbackURL_Allowed(t *testing.T) { + gin.SetMode(gin.TestMode) + + // Use a real httptest server so the URL resolves to 127.0.0.1 which we + // explicitly allowlist. This avoids external DNS lookups in CI. + callbackServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer callbackServer.Close() + + // Allowlist loopback so the httptest URL passes SSRF validation. + services.SetWebhookAllowedHosts([]string{"127.0.0.1"}) + t.Cleanup(func() { services.SetWebhookAllowedHosts(nil) }) + + agent := &types.AgentNode{ID: "agent-cb"} + store := newTestExecutionStorage(agent) + setupRunningExecution(t, store, "exec-cb", "agent-cb") + + router := gin.New() + router.POST("/api/v1/agents/:node_id/executions/:execution_id/request-approval", + AgentScopedRequestApprovalHandler(store)) + + body, _ := json.Marshal(map[string]any{ + "approval_request_id": "req-cb-1", + "approval_request_url": "https://hub.example.com/review/req-cb-1", + "callback_url": callbackServer.URL + "/hooks/approval", + }) + + req := httptest.NewRequest(http.MethodPost, "/api/v1/agents/agent-cb/executions/exec-cb/request-approval", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + resp := httptest.NewRecorder() + router.ServeHTTP(resp, req) + + require.Equal(t, http.StatusOK, resp.Code, "body: %s", resp.Body.String()) + + // Verify the callback URL was stored + wfExec, err := store.GetWorkflowExecution(context.Background(), "exec-cb") + require.NoError(t, err) + require.NotNil(t, wfExec) + require.NotNil(t, wfExec.ApprovalCallbackURL) + assert.Equal(t, callbackServer.URL+"/hooks/approval", *wfExec.ApprovalCallbackURL) +} + +func TestNotifyApprovalCallback_UsesSSRFSafeClient(t *testing.T) { + // Verify that notifyApprovalCallback uses the SSRF-safe client by + // attempting to POST to a private IP (127.0.0.1). Without the allowlist + // the SSRF-safe transport rejects it at dial time. We clear the + // allowlist to ensure strict validation. + gin.SetMode(gin.TestMode) + + services.SetWebhookAllowedHosts(nil) + t.Cleanup(func() { services.SetWebhookAllowedHosts(nil) }) + + ctrl := &webhookApprovalController{ + store: newTestExecutionStorage(&types.AgentNode{ID: "agent-1"}), + webhookSecret: "test-secret", + } + + // Call synchronously. The SSRF-safe client will reject 127.0.0.1 at + // dial time (no actual TCP connection), so this returns quickly. + // If it used a plain http.Client instead, it would attempt a real + // connection to 127.0.0.1:80 which would either connect or timeout. + ctrl.notifyApprovalCallback( + "http://127.0.0.1:1/ssrf-test", + "exec-1", "approved", "running", "", nil, "req-1", + ) + + // If we reach here without hanging, the SSRF-safe client properly + // rejected the private IP at the transport level. +} diff --git a/control-plane/internal/handlers/webhook_approval.go b/control-plane/internal/handlers/webhook_approval.go index 8e9d23fb0..af4eb11e8 100644 --- a/control-plane/internal/handlers/webhook_approval.go +++ b/control-plane/internal/handlers/webhook_approval.go @@ -13,6 +13,7 @@ import ( "time" "github.com/Agent-Field/agentfield/control-plane/internal/events" + "github.com/Agent-Field/agentfield/control-plane/internal/services" "github.com/Agent-Field/agentfield/control-plane/internal/logger" "github.com/Agent-Field/agentfield/control-plane/pkg/types" @@ -547,7 +548,9 @@ func (c *webhookApprovalController) notifyApprovalCallback(callbackURL, executio return } - client := &http.Client{Timeout: 5 * time.Second} + // Use SSRF-safe client to prevent the control plane from being used as + // a proxy to internal services via a malicious callback_url (CWE-918). + client := services.NewSSRFSafeClient(5 * time.Second) resp, err := client.Post(callbackURL, "application/json", bytes.NewReader(body)) if err != nil { logger.Logger.Warn().Err(err).Str("callback_url", callbackURL).Str("execution_id", executionID).Msg("approval callback delivery failed") diff --git a/control-plane/internal/handlers/webhook_approval_test.go b/control-plane/internal/handlers/webhook_approval_test.go index 3817392af..c682b10c1 100644 --- a/control-plane/internal/handlers/webhook_approval_test.go +++ b/control-plane/internal/handlers/webhook_approval_test.go @@ -14,6 +14,7 @@ import ( "testing" "time" + "github.com/Agent-Field/agentfield/control-plane/internal/services" "github.com/Agent-Field/agentfield/control-plane/pkg/types" "github.com/gin-gonic/gin" @@ -626,6 +627,11 @@ func TestApprovalWebhook_HaxSignatureRejectsNonNumericTimestamp(t *testing.T) { func TestApprovalWebhook_CallbackNotification(t *testing.T) { gin.SetMode(gin.TestMode) + // Allow loopback for httptest server — the SSRF-safe client blocks + // private IPs by default; tests need the allowlist to reach 127.0.0.1. + services.SetWebhookAllowedHosts([]string{"127.0.0.1"}) + t.Cleanup(func() { services.SetWebhookAllowedHosts(nil) }) + agent := &types.AgentNode{ID: "agent-1"} store := newTestExecutionStorage(agent) From 992c0012bd0c49b433c871b6490f0f303e5a089a Mon Sep 17 00:00:00 2001 From: Vignesh Date: Sat, 18 Jul 2026 23:50:29 +0530 Subject: [PATCH 2/2] fix(test): allowlist loopback in webhook helper test for SSRF-safe client The existing TestExecuteReasonerAndWebhookHelpersCoverage test calls notifyApprovalCallback against a httptest server (127.0.0.1). After switching to NewSSRFSafeClient (#435), the SSRF transport rejects loopback as a private IP, causing the test to hang waiting for callbacks that never arrive. Fix: set services.SetWebhookAllowedHosts([]string{"127.0.0.1"}) before the callback calls so the httptest server is reachable. --- .../internal/handlers/coverage_handlers_90_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/control-plane/internal/handlers/coverage_handlers_90_test.go b/control-plane/internal/handlers/coverage_handlers_90_test.go index 196bf9ab8..13de08ec9 100644 --- a/control-plane/internal/handlers/coverage_handlers_90_test.go +++ b/control-plane/internal/handlers/coverage_handlers_90_test.go @@ -14,6 +14,7 @@ import ( "github.com/Agent-Field/agentfield/control-plane/internal/events" "github.com/Agent-Field/agentfield/control-plane/internal/server/middleware" + "github.com/Agent-Field/agentfield/control-plane/internal/services" "github.com/Agent-Field/agentfield/control-plane/internal/storage" "github.com/Agent-Field/agentfield/control-plane/pkg/types" "github.com/gin-gonic/gin" @@ -497,6 +498,12 @@ func TestExecuteReasonerAndWebhookHelpersCoverage(t *testing.T) { ctrl := &webhookApprovalController{} response := `{"decision":"approved"}` + + // Allowlist loopback so the httptest server is reachable via the + // SSRF-safe client used by notifyApprovalCallback (#435). + services.SetWebhookAllowedHosts([]string{"127.0.0.1"}) + t.Cleanup(func() { services.SetWebhookAllowedHosts(nil) }) + ctrl.notifyApprovalCallback(server.URL, "exec-1", "approved", "running", "ok", &response, "req-1") ctrl.notifyApprovalCallback(server.URL+"/fail", "exec-2", "rejected", "cancelled", "", nil, "req-2") ctrl.notifyApprovalCallback("http://127.0.0.1:1", "exec-3", "approved", "running", "", nil, "req-3")