diff --git a/workspace-server/internal/handlers/request_store.go b/workspace-server/internal/handlers/request_store.go index 2be454d03..3712470dc 100644 --- a/workspace-server/internal/handlers/request_store.go +++ b/workspace-server/internal/handlers/request_store.go @@ -46,6 +46,10 @@ var ErrInvalidRequestKind = errors.New("request: kind must be 'task' or 'approva // author_type is outside the user/agent enum. Callers translate to HTTP 400. var ErrInvalidRequestParty = errors.New("request: type must be 'user' or 'agent'") +// ErrSelfResponse is returned by Respond when the responder is the same party +// as the requester (self-approval / self-rejection). Callers translate to HTTP 400. +var ErrSelfResponse = errors.New("request: responder cannot be the requester") + // CreateRequestInput is the set of fields Create needs. requester_* identifies // who raised it (for a per-workspace POST that is the calling agent); recipient_* // is who must act. Detail/OrgID/Priority are optional (zero values → NULL). @@ -387,6 +391,14 @@ func (s *RequestStore) Respond(ctx context.Context, id, action, responderType, r if err != nil { return RequestRow{}, err } + + // SECURITY: prevent self-approval / self-rejection. The requester must not + // be the same party as the responder — that would let an agent approve its + // own tasks or a user self-approve their own requests (RC 10416). + if responderType == req.RequesterType && responderID == req.RequesterID { + return RequestRow{}, ErrSelfResponse + } + status, err := actionToStatus(req.Kind, action) if err != nil { return RequestRow{}, err diff --git a/workspace-server/internal/handlers/requests.go b/workspace-server/internal/handlers/requests.go index 779b7c300..992932dc0 100644 --- a/workspace-server/internal/handlers/requests.go +++ b/workspace-server/internal/handlers/requests.go @@ -218,6 +218,19 @@ func (h *RequestsHandler) Get(c *gin.Context) { c.JSON(http.StatusInternalServerError, gin.H{"error": "query failed"}) return } + + // Workspace-token auth path: a workspace may read only a request it is a + // party to (requester or recipient). Cross-workspace read is forbidden + // (core#2542 full fix). + if workspaceID := c.Param("id"); workspaceID != "" { + isParty := (req.RequesterType == "agent" && req.RequesterID == workspaceID) || + (req.RecipientType == "agent" && req.RecipientID == workspaceID) + if !isParty { + c.JSON(http.StatusForbidden, gin.H{"error": "not a participant"}) + return + } + } + msgs, err := s.Messages(ctx, requestID) if err != nil { log.Printf("Get request messages error request=%s: %v", requestID, err) @@ -228,8 +241,14 @@ func (h *RequestsHandler) Get(c *gin.Context) { } // Respond handles POST /requests/:requestId/respond — a terminal action -// (done/rejected/approved), validated against the request's kind. responder -// identity comes from the body; the canvas/admin path defaults to 'user'. +// (done/rejected/approved), validated against the request's kind. +// +// REAL participant-binding (RC 10416 follow-up): on the workspace-token auth +// path (/workspaces/:id/requests/...), the responder is BOUND to the +// authenticated workspace — the body fields are ignored. This prevents a +// workspace from impersonating a user or a different agent when responding. +// The canvas/admin path continues to take responder from the body (defaults +// to 'user' on the canvas). // // @Summary Respond to a request (done / rejected / approved) // @Tags requests @@ -253,12 +272,41 @@ func (h *RequestsHandler) Respond(c *gin.Context) { return } - if _, err := h.store().Respond(ctx, requestID, body.Action, body.ResponderType, body.ResponderID); err != nil { + responderType := body.ResponderType + responderID := body.ResponderID + + // Workspace-token auth path: bind responder to the authenticated workspace. + if workspaceID := c.Param("id"); workspaceID != "" { + responderType = "agent" + responderID = workspaceID + } + + // Workspace-token auth path: verify the caller is the request's recipient. + // Cross-workspace respond is forbidden — an agent must not terminate a + // request that was not addressed to it (core#2542 full fix). + if workspaceID := c.Param("id"); workspaceID != "" { + reqRow, err := h.store().Get(ctx, requestID) + if err != nil { + if errors.Is(err, ErrRequestNotFound) { + c.JSON(http.StatusNotFound, gin.H{"error": "request not found or already resolved"}) + return + } + log.Printf("Respond authz error request=%s: %v", requestID, err) + c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to respond"}) + return + } + if reqRow.RecipientType != "agent" || reqRow.RecipientID != workspaceID { + c.JSON(http.StatusForbidden, gin.H{"error": "not the recipient"}) + return + } + } + + if _, err := h.store().Respond(ctx, requestID, body.Action, responderType, responderID); err != nil { if errors.Is(err, ErrRequestNotFound) { c.JSON(http.StatusNotFound, gin.H{"error": "request not found or already resolved"}) return } - if errors.Is(err, ErrInvalidRequestAction) || errors.Is(err, ErrInvalidRequestParty) { + if errors.Is(err, ErrInvalidRequestAction) || errors.Is(err, ErrInvalidRequestParty) || errors.Is(err, ErrSelfResponse) { c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) return } @@ -329,6 +377,26 @@ func (h *RequestsHandler) Cancel(c *gin.Context) { requestID := c.Param("requestId") ctx := c.Request.Context() + // Workspace-token auth path: verify the caller is the request's requester. + // Cross-workspace cancel is forbidden — an agent must not withdraw a + // request it did not raise (core#2542 full fix). + if workspaceID := c.Param("id"); workspaceID != "" { + reqRow, err := h.store().Get(ctx, requestID) + if err != nil { + if errors.Is(err, ErrRequestNotFound) { + c.JSON(http.StatusNotFound, gin.H{"error": "request not found or already resolved"}) + return + } + log.Printf("Cancel authz error request=%s: %v", requestID, err) + c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to cancel"}) + return + } + if reqRow.RequesterType != "agent" || reqRow.RequesterID != workspaceID { + c.JSON(http.StatusForbidden, gin.H{"error": "not the requester"}) + return + } + } + if err := h.store().Cancel(ctx, requestID); err != nil { if errors.Is(err, ErrRequestNotFound) { c.JSON(http.StatusNotFound, gin.H{"error": "request not found or already resolved"}) diff --git a/workspace-server/internal/handlers/requests_test.go b/workspace-server/internal/handlers/requests_test.go index 2e7101c4d..830e2f1e4 100644 --- a/workspace-server/internal/handlers/requests_test.go +++ b/workspace-server/internal/handlers/requests_test.go @@ -265,6 +265,91 @@ func TestRequests_Get_NotFound(t *testing.T) { } } +// TestRequests_Get_AgentPath_Requester_200 verifies that the requester workspace +// can read its own request on the workspace-token auth path. +func TestRequests_Get_AgentPath_Requester_200(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + handler := NewRequestsHandler(newTestBroadcaster()) + + mock.ExpectQuery("FROM requests WHERE id"). + WithArgs("req-1"). + WillReturnRows(oneRequestRow("req-1", "task", "ws-1", "agent", "ws-2", "pending")) + mock.ExpectQuery("FROM request_messages WHERE request_id"). + WithArgs("req-1"). + WillReturnRows(sqlmock.NewRows([]string{"id", "request_id", "author_type", "author_id", "body", "created_at"})) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{ + {Key: "requestId", Value: "req-1"}, + {Key: "id", Value: "ws-1"}, + } + c.Request = httptest.NewRequest("GET", "/", nil) + + handler.Get(c) + + if w.Code != http.StatusOK { + t.Errorf("expected 200 for requester read, got %d: %s", w.Code, w.Body.String()) + } +} + +// TestRequests_Get_AgentPath_Recipient_200 verifies that the recipient workspace +// can read a request addressed to it on the workspace-token auth path. +func TestRequests_Get_AgentPath_Recipient_200(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + handler := NewRequestsHandler(newTestBroadcaster()) + + mock.ExpectQuery("FROM requests WHERE id"). + WithArgs("req-1"). + WillReturnRows(oneRequestRow("req-1", "approval", "ws-1", "agent", "ws-2", "pending")) + mock.ExpectQuery("FROM request_messages WHERE request_id"). + WithArgs("req-1"). + WillReturnRows(sqlmock.NewRows([]string{"id", "request_id", "author_type", "author_id", "body", "created_at"})) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{ + {Key: "requestId", Value: "req-1"}, + {Key: "id", Value: "ws-2"}, + } + c.Request = httptest.NewRequest("GET", "/", nil) + + handler.Get(c) + + if w.Code != http.StatusOK { + t.Errorf("expected 200 for recipient read, got %d: %s", w.Code, w.Body.String()) + } +} + +// TestRequests_Get_AgentPath_NonParticipant_403 verifies that a workspace that +// is neither the requester nor the recipient gets 403 on the workspace-token +// auth path (core#2542 full fix — read-path org-scoping). +func TestRequests_Get_AgentPath_NonParticipant_403(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + handler := NewRequestsHandler(newTestBroadcaster()) + + mock.ExpectQuery("FROM requests WHERE id"). + WithArgs("req-1"). + WillReturnRows(oneRequestRow("req-1", "task", "ws-1", "agent", "ws-2", "pending")) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{ + {Key: "requestId", Value: "req-1"}, + {Key: "id", Value: "ws-3"}, + } + c.Request = httptest.NewRequest("GET", "/", nil) + + handler.Get(c) + + if w.Code != http.StatusForbidden { + t.Errorf("expected 403 for non-participant read, got %d: %s", w.Code, w.Body.String()) + } +} + // ---------- Respond (valid + invalid action-for-kind) ---------- func TestRequests_Respond_ApprovalApproved(t *testing.T) { @@ -371,6 +456,132 @@ func TestRequests_Respond_NotFound(t *testing.T) { } } +// TestRequests_Respond_SelfResponse_400 pins RC 10416: the requester must not +// be able to approve/reject/done their own request. +func TestRequests_Respond_SelfResponse_400(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + handler := NewRequestsHandler(newTestBroadcaster()) + + // Requester is agent ws-1; responder is also agent ws-1 → self-response. + mock.ExpectQuery("FROM requests WHERE id"). + WithArgs("req-1"). + WillReturnRows(oneRequestRow("req-1", "approval", "ws-1", "user", "", "pending")) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "requestId", Value: "req-1"}} + c.Request = httptest.NewRequest("POST", "/", bytes.NewBufferString(`{"action":"approved","responder_type":"agent","responder_id":"ws-1"}`)) + c.Request.Header.Set("Content-Type", "application/json") + + handler.Respond(c) + + if w.Code != http.StatusBadRequest { + t.Errorf("expected 400 for self-response, got %d: %s", w.Code, w.Body.String()) + } +} + +// TestRequests_Respond_AgentPath_BindsWorkspace verifies REAL participant-binding: +// on the workspace-token auth path the responder is forced to the URL workspace, +// ignoring any impersonation attempt in the body. +func TestRequests_Respond_AgentPath_BindsWorkspace(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + handler := NewRequestsHandler(newTestBroadcaster()) + + // Requester is agent ws-1; body claims self-response (ws-1), but the URL + // workspace is ws-2. Binding overrides the body, so responder = ws-2 and + // the call succeeds (not self-response). + // Authz Get in handler, then store Get inside Respond. + mock.ExpectQuery("FROM requests WHERE id"). + WithArgs("req-1"). + WillReturnRows(oneRequestRow("req-1", "approval", "ws-1", "agent", "ws-2", "pending")) + mock.ExpectQuery("FROM requests WHERE id"). + WithArgs("req-1"). + WillReturnRows(oneRequestRow("req-1", "approval", "ws-1", "agent", "ws-2", "pending")) + mock.ExpectExec("UPDATE requests SET status"). + WithArgs("approved", "agent", "ws-2", sqlmock.AnyArg()). + WillReturnResult(sqlmock.NewResult(0, 1)) + mock.ExpectExec("INSERT INTO structure_events"). + WillReturnResult(sqlmock.NewResult(0, 1)) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{ + {Key: "requestId", Value: "req-1"}, + {Key: "id", Value: "ws-2"}, + } + c.Request = httptest.NewRequest("POST", "/", bytes.NewBufferString(`{"action":"approved","responder_type":"agent","responder_id":"ws-1"}`)) + c.Request.Header.Set("Content-Type", "application/json") + + handler.Respond(c) + + if w.Code != http.StatusOK { + t.Errorf("expected 200 when bound responder differs from requester, got %d: %s", w.Code, w.Body.String()) + } +} + +// TestRequests_Respond_AgentPath_NotRecipient_403 verifies that an agent +// cannot respond to a request that was not addressed to it (core#2542). +func TestRequests_Respond_AgentPath_NotRecipient_403(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + handler := NewRequestsHandler(newTestBroadcaster()) + + // Requester is ws-1, recipient is ws-2; URL workspace is ws-1 (not recipient). + mock.ExpectQuery("FROM requests WHERE id"). + WithArgs("req-1"). + WillReturnRows(oneRequestRow("req-1", "approval", "ws-1", "agent", "ws-2", "pending")) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{ + {Key: "requestId", Value: "req-1"}, + {Key: "id", Value: "ws-1"}, + } + c.Request = httptest.NewRequest("POST", "/", bytes.NewBufferString(`{"action":"approved","responder_type":"agent","responder_id":"ws-2"}`)) + c.Request.Header.Set("Content-Type", "application/json") + + handler.Respond(c) + + if w.Code != http.StatusForbidden { + t.Errorf("expected 403 for non-recipient respond, got %d: %s", w.Code, w.Body.String()) + } +} + +// TestRequests_Respond_AgentPath_SelfResponse_400 verifies that the +// self-response guard still fires on the agent path when the request is +// self-addressed (requester == recipient) and the caller tries to respond. +func TestRequests_Respond_AgentPath_SelfResponse_400(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + handler := NewRequestsHandler(newTestBroadcaster()) + + // Self-addressed request: requester = recipient = ws-1. + // Authz Get in handler, then store Get inside Respond. + mock.ExpectQuery("FROM requests WHERE id"). + WithArgs("req-1"). + WillReturnRows(oneRequestRow("req-1", "approval", "ws-1", "agent", "ws-1", "pending")) + mock.ExpectQuery("FROM requests WHERE id"). + WithArgs("req-1"). + WillReturnRows(oneRequestRow("req-1", "approval", "ws-1", "agent", "ws-1", "pending")) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{ + {Key: "requestId", Value: "req-1"}, + {Key: "id", Value: "ws-1"}, + } + c.Request = httptest.NewRequest("POST", "/", bytes.NewBufferString(`{"action":"approved","responder_type":"agent","responder_id":"ws-1"}`)) + c.Request.Header.Set("Content-Type", "application/json") + + handler.Respond(c) + + if w.Code != http.StatusBadRequest { + t.Errorf("expected 400 for bound self-response, got %d: %s", w.Code, w.Body.String()) + } +} + // ---------- AddMessage → info_requested when recipient asks ---------- func TestRequests_AddMessage_RecipientFlipsInfoRequested(t *testing.T) { @@ -459,6 +670,33 @@ func TestRequests_Cancel_Success(t *testing.T) { } } +// TestRequests_Cancel_AgentPath_NotRequester_403 verifies that an agent +// cannot cancel a request it did not raise (core#2542). +func TestRequests_Cancel_AgentPath_NotRequester_403(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + handler := NewRequestsHandler(newTestBroadcaster()) + + // Requester is ws-1; URL workspace is ws-2 (not requester). + mock.ExpectQuery("FROM requests WHERE id"). + WithArgs("req-1"). + WillReturnRows(oneRequestRow("req-1", "task", "ws-1", "agent", "ws-2", "pending")) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{ + {Key: "requestId", Value: "req-1"}, + {Key: "id", Value: "ws-2"}, + } + c.Request = httptest.NewRequest("POST", "/", nil) + + handler.Cancel(c) + + if w.Code != http.StatusForbidden { + t.Errorf("expected 403 for non-requester cancel, got %d: %s", w.Code, w.Body.String()) + } +} + func TestRequests_Cancel_NotFound(t *testing.T) { mock := setupTestDB(t) setupTestRedis(t)