forked from molecule-ai/molecule-core
fix(security): C18 register ownership check, C20 DELETE auth gate
C18 — Workspace URL hijacking (CRITICAL, CONFIRMED LIVE): POST /registry/register now calls requireWorkspaceToken() before persisting anything. If the workspace has any live auth tokens, the caller must supply a valid Bearer token matching that workspace ID. First registration (no tokens yet) passes through — token is issued at end of this function (unchanged bootstrap contract). Mirrors the same pattern already applied to /registry/heartbeat and /registry/update-card. Attacker POC — overwriting Backend Engineer URL to http://attacker.example.com:9999/steal — now returns 401. C20 — Unauthenticated workspace deletion (CRITICAL, CONFIRMED LIVE): DELETE /workspaces/:id moved from bare router into AdminAuth group. Any valid workspace bearer token grants access (same fail-open bootstrap contract as /settings/secrets). Mass-deletion attack chain (C19 list → C20 delete all) requires auth for the DELETE step. POST /workspaces (create) also moved to AdminAuth to prevent unauthenticated workspace creation. C19 (GET /workspaces topology exposure) deferred — canvas browser has no bearer token; fix requires canvas service-token refactor. Tests: 2 new registry tests — C18 bootstrap (no tokens, passes through and issues token), C18 hijack blocked (has tokens, no bearer → 401). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
9ec566ad3d
commit
07bb730675
@ -71,6 +71,22 @@ func (h *RegistryHandler) Register(c *gin.Context) {
|
||||
}
|
||||
|
||||
ctx := c.Request.Context()
|
||||
|
||||
// C18: prevent workspace URL hijacking on re-registration.
|
||||
//
|
||||
// An attacker can overwrite any workspace's agent_card URL by calling
|
||||
// /registry/register with that workspace's ID and their own URL, redirecting
|
||||
// all A2A messages to their server.
|
||||
//
|
||||
// Fix: if this workspace already has any live auth tokens on file, the caller
|
||||
// must prove they own it by supplying a valid bearer token in Authorization.
|
||||
// First-ever registration (no tokens yet) is bootstrap-allowed — the token
|
||||
// is issued at the end of this function. This mirrors the same pattern used
|
||||
// for /registry/heartbeat and /registry/update-card.
|
||||
if err := h.requireWorkspaceToken(ctx, c, payload.ID); err != nil {
|
||||
return // 401 response already written by requireWorkspaceToken
|
||||
}
|
||||
|
||||
agentCardStr := string(payload.AgentCard)
|
||||
|
||||
// Upsert workspace: update url, agent_card, status if already exists.
|
||||
|
||||
@ -471,3 +471,96 @@ func TestValidateAgentURL(t *testing.T) {
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// ==================== C18 — Register ownership ====================
|
||||
|
||||
// TestRegister_C18_BootstrapAllowedNoTokens verifies that a workspace with NO
|
||||
// live tokens (i.e. first-ever registration) is allowed through without a bearer
|
||||
// token. This is the bootstrap path — the token is issued at the end of Register.
|
||||
func TestRegister_C18_BootstrapAllowedNoTokens(t *testing.T) {
|
||||
mock := setupTestDB(t)
|
||||
setupTestRedis(t)
|
||||
broadcaster := newTestBroadcaster()
|
||||
handler := NewRegistryHandler(broadcaster)
|
||||
|
||||
// requireWorkspaceToken → HasAnyLiveToken → COUNT(*) returns 0 (no tokens).
|
||||
mock.ExpectQuery("SELECT COUNT\\(\\*\\) FROM workspace_auth_tokens").
|
||||
WithArgs("ws-new").
|
||||
WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(0))
|
||||
|
||||
// Workspace upsert proceeds normally.
|
||||
mock.ExpectExec("INSERT INTO workspaces").
|
||||
WithArgs("ws-new", "ws-new", "http://localhost:9100", `{"name":"new-agent"}`).
|
||||
WillReturnResult(sqlmock.NewResult(0, 1))
|
||||
|
||||
mock.ExpectQuery("SELECT url FROM workspaces WHERE id").
|
||||
WithArgs("ws-new").
|
||||
WillReturnRows(sqlmock.NewRows([]string{"url"}).AddRow("http://localhost:9100"))
|
||||
|
||||
mock.ExpectExec("INSERT INTO structure_events").
|
||||
WillReturnResult(sqlmock.NewResult(0, 1))
|
||||
|
||||
// HasAnyLiveToken check for token issuance at end of Register.
|
||||
mock.ExpectQuery("SELECT COUNT\\(\\*\\) FROM workspace_auth_tokens").
|
||||
WithArgs("ws-new").
|
||||
WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(0))
|
||||
|
||||
// IssueToken INSERT.
|
||||
mock.ExpectExec("INSERT INTO workspace_auth_tokens").
|
||||
WithArgs("ws-new", sqlmock.AnyArg(), sqlmock.AnyArg()).
|
||||
WillReturnResult(sqlmock.NewResult(1, 1))
|
||||
|
||||
w := httptest.NewRecorder()
|
||||
c, _ := gin.CreateTestContext(w)
|
||||
c.Request = httptest.NewRequest("POST", "/registry/register",
|
||||
bytes.NewBufferString(`{"id":"ws-new","url":"http://localhost:9100","agent_card":{"name":"new-agent"}}`))
|
||||
c.Request.Header.Set("Content-Type", "application/json")
|
||||
|
||||
handler.Register(c)
|
||||
|
||||
if w.Code != http.StatusOK {
|
||||
t.Errorf("C18 bootstrap: expected 200, got %d: %s", w.Code, w.Body.String())
|
||||
}
|
||||
// Token should be present in response (first registration).
|
||||
var resp map[string]interface{}
|
||||
if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil {
|
||||
t.Fatalf("failed to parse response: %v", err)
|
||||
}
|
||||
if resp["auth_token"] == nil {
|
||||
t.Errorf("C18 bootstrap: expected auth_token in first-registration response, got %v", resp)
|
||||
}
|
||||
if err := mock.ExpectationsWereMet(); err != nil {
|
||||
t.Errorf("C18 bootstrap: unmet expectations: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
// TestRegister_C18_HijackBlockedNoBearer verifies the C18 attack is blocked:
|
||||
// when a workspace already has a live token, /register without a bearer → 401.
|
||||
func TestRegister_C18_HijackBlockedNoBearer(t *testing.T) {
|
||||
mock := setupTestDB(t)
|
||||
setupTestRedis(t)
|
||||
broadcaster := newTestBroadcaster()
|
||||
handler := NewRegistryHandler(broadcaster)
|
||||
|
||||
// HasAnyLiveToken returns 1 — workspace already has an active token.
|
||||
mock.ExpectQuery("SELECT COUNT\\(\\*\\) FROM workspace_auth_tokens").
|
||||
WithArgs("ws-victim").
|
||||
WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(1))
|
||||
|
||||
w := httptest.NewRecorder()
|
||||
c, _ := gin.CreateTestContext(w)
|
||||
// No Authorization header — simulates attacker with no credentials.
|
||||
c.Request = httptest.NewRequest("POST", "/registry/register",
|
||||
bytes.NewBufferString(`{"id":"ws-victim","url":"http://attacker.example.com:9999/steal","agent_card":{"name":"hijacked"}}`))
|
||||
c.Request.Header.Set("Content-Type", "application/json")
|
||||
|
||||
handler.Register(c)
|
||||
|
||||
if w.Code != http.StatusUnauthorized {
|
||||
t.Errorf("C18 hijack: expected 401, got %d: %s", w.Code, w.Body.String())
|
||||
}
|
||||
// The malicious URL must NOT have been persisted — no INSERT expectation was set.
|
||||
if err := mock.ExpectationsWereMet(); err != nil {
|
||||
t.Errorf("C18 hijack: unmet expectations: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
@ -62,12 +62,23 @@ func Setup(hub *ws.Hub, broadcaster *events.Broadcaster, prov *provisioner.Provi
|
||||
// Scrape with: curl http://localhost:8080/metrics
|
||||
r.GET("/metrics", metrics.Handler())
|
||||
|
||||
// Workspaces CRUD — bare /workspaces and /workspaces/:id (no sub-path), unauthenticated for canvas
|
||||
r.POST("/workspaces", wh.Create)
|
||||
// Workspaces read + position-patch — left open for the canvas browser frontend
|
||||
// which has no bearer token. C19 (GET /workspaces exposes topology) requires a
|
||||
// canvas service-token refactor and is tracked as a follow-up issue.
|
||||
r.GET("/workspaces", wh.List)
|
||||
r.GET("/workspaces/:id", wh.Get)
|
||||
r.PATCH("/workspaces/:id", wh.Update)
|
||||
r.DELETE("/workspaces/:id", wh.Delete)
|
||||
|
||||
// C20 + C18-adjacent: mutating workspace operations require any valid workspace
|
||||
// bearer token (AdminAuth — same fail-open bootstrap contract as global secrets).
|
||||
// Blocks: mass deletion (C20), unauthenticated workspace creation.
|
||||
// Canvas Create Workspace dialog passes through because no global tokens exist
|
||||
// on a fresh install; once any workspace is online the dialog requires auth.
|
||||
{
|
||||
wsAdmin := r.Group("", middleware.AdminAuth(db.DB))
|
||||
wsAdmin.POST("/workspaces", wh.Create)
|
||||
wsAdmin.DELETE("/workspaces/:id", wh.Delete)
|
||||
}
|
||||
|
||||
// A2A proxy — registered outside the auth group; already enforces CanCommunicate access control.
|
||||
r.POST("/workspaces/:id/a2a", wh.ProxyA2A)
|
||||
|
||||
Loading…
Reference in New Issue
Block a user