fix(a2a): isSafeURL guard inside dispatchA2A (closes #1483)

#1483 flagged that dispatchA2A() doesn't call isSafeURL internally —
the guard exists only at the caller level (resolveAgentURL at
a2a_proxy.go:424). The primary call path through proxyA2ARequest
is safe today, but if any future code path ever calls dispatchA2A
directly without going through resolveAgentURL, the SSRF check
would be silently bypassed.

This adds the one-line defense-in-depth guard the issue prescribed:

  if err := isSafeURL(agentURL); err != nil {
      return nil, nil, &proxyDispatchBuildError{err: err}
  }

Wrapping as *proxyDispatchBuildError preserves the existing caller
error-classification path — the same shape that maps to 500 elsewhere.

Adds TestDispatchA2A_RejectsUnsafeURL pinning the contract:
re-enables SSRF for the test (setupTestDB disables it for normal
unit tests), passes a metadata IP, asserts the build error returns
and cancel is nil so no resource is leaked.

The 4 existing dispatchA2A unit tests use setupTestDB → SSRF
disabled, so they continue passing unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Hongming Wang 2026-04-26 07:18:58 -07:00
parent a8c9644618
commit fd891a147e
2 changed files with 43 additions and 0 deletions

View File

@ -483,6 +483,16 @@ func normalizeA2APayload(body []byte) ([]byte, string, *proxyA2AError) {
// canvas (callerID == "") = 5 min, agent-to-agent = 30 min. Callers can
// override via the X-Timeout header (applied to ctx upstream in ProxyA2A).
func (h *WorkspaceHandler) dispatchA2A(ctx context.Context, agentURL string, body []byte, callerID string) (*http.Response, context.CancelFunc, error) {
// #1483 SSRF defense-in-depth: the primary call path through
// proxyA2ARequest → resolveAgentURL already validates via isSafeURL
// (a2a_proxy.go:424), but adding the check here closes the gap for
// any future code path that calls dispatchA2A directly without
// going through resolveAgentURL. Wrapping as proxyDispatchBuildError
// keeps the caller's error-classification path unchanged — the same
// shape it already produces a 500 for.
if err := isSafeURL(agentURL); err != nil {
return nil, nil, &proxyDispatchBuildError{err: err}
}
forwardCtx := context.WithoutCancel(ctx)
var cancel context.CancelFunc
if _, hasDeadline := ctx.Deadline(); !hasDeadline {

View File

@ -1155,6 +1155,39 @@ func TestDispatchA2A_ContextDeadline_NoCancelAdded(t *testing.T) {
}
}
// TestDispatchA2A_RejectsUnsafeURL is the #1483 defense-in-depth
// regression. setupTestDB disables SSRF for normal tests so existing
// dispatchA2A unit tests can hit httptest.NewServer (loopback) — we
// re-enable it here to verify the new in-function isSafeURL guard.
// Production callers go through resolveAgentURL which already
// validates; this test pins that dispatchA2A is now safe even when
// called directly by a future caller that skips resolveAgentURL.
func TestDispatchA2A_RejectsUnsafeURL(t *testing.T) {
setupTestDB(t)
setupTestRedis(t)
restoreSSRF := setSSRFCheckForTest(true)
t.Cleanup(restoreSSRF)
handler := NewWorkspaceHandler(newTestBroadcaster(), nil, "http://localhost:8080", t.TempDir())
// Cloud metadata IP — must be rejected before any HTTP call goes out.
_, cancel, err := handler.dispatchA2A(
context.Background(),
"http://169.254.169.254/latest/meta-data/",
[]byte(`{}`),
"",
)
if cancel != nil {
cancel()
t.Error("cancel must be nil when the URL is rejected pre-request")
}
if err == nil {
t.Fatal("expected SSRF rejection error, got nil")
}
if _, ok := err.(*proxyDispatchBuildError); !ok {
t.Errorf("expected *proxyDispatchBuildError (caller maps to 500), got %T: %v", err, err)
}
}
// --- handleA2ADispatchError ---
func TestHandleA2ADispatchError_ContextDeadline(t *testing.T) {