diff --git a/server/internal/automation/schema.go b/server/internal/automation/schema.go index 2ad0985770..61f300bf78 100644 --- a/server/internal/automation/schema.go +++ b/server/internal/automation/schema.go @@ -135,6 +135,23 @@ var validIssueFields = map[string]bool{ IssueFieldParentIssueID: true, } +// validIssueStatuses mirrors the issue.status DB CHECK (migrations/001) and the +// handler's validIssueStatuses. A status-valued condition or action is rejected +// unless its value is one of these, so a hook can never persist an unreachable +// status (MUL-4332 PR2 review point 3). +var validIssueStatuses = map[string]bool{ + "backlog": true, + "todo": true, + "in_progress": true, + "in_review": true, + "done": true, + "blocked": true, + "cancelled": true, +} + +// isValidIssueStatus reports whether s is a persistable issue status. +func isValidIssueStatus(s string) bool { return validIssueStatuses[s] } + // conditionDependencyEvent maps a condition to the single v1 domain event that // can change its truth value. A rising_edge hook must listen to exactly that // event so its latch can be re-evaluated (§5.2). In the v1 fixed vocabulary diff --git a/server/internal/automation/validate.go b/server/internal/automation/validate.go index ff712e3c32..61b911bf5e 100644 --- a/server/internal/automation/validate.go +++ b/server/internal/automation/validate.go @@ -12,6 +12,9 @@ import ( const ( MaxActionsPerHook = 8 MaxNameLength = 200 + MaxConditionIDs = 100 + MaxMatchSetSize = 100 + MaxMessageLength = 4000 ) // ValidationError is a user-fixable problem with a hook spec. Handlers map it to @@ -20,6 +23,14 @@ type ValidationError struct{ msg string } func (e *ValidationError) Error() string { return e.msg } +// NewValidationError builds a ValidationError. Exported so callers that extend +// author-time validation with checks this pure package cannot do (e.g. the +// service's workspace-scoped target existence checks) surface the same typed +// error and share the handler's single 400 mapping. +func NewValidationError(format string, args ...any) *ValidationError { + return &ValidationError{msg: fmt.Sprintf(format, args...)} +} + func verr(format string, args ...any) error { return &ValidationError{msg: fmt.Sprintf(format, args...)} } @@ -117,6 +128,9 @@ func validateMatch(raw []byte, schema EventSchema) error { } func validateClauseValues(field string, kind FieldKind, clause MatchClause) error { + if clause.Op == MatchIn && len(clause.Set) > MaxMatchSetSize { + return verr("match field %q has %d values, at most %d allowed", field, len(clause.Set), MaxMatchSetSize) + } if kind != FieldUUID { return nil // string fields accept any scalar; existence needs no value check } @@ -156,6 +170,9 @@ func validateIssuesStatus(c IssuesStatusCond) error { if len(c.IDs) == 0 { return verr("issues_status.ids must not be empty") } + if len(c.IDs) > MaxConditionIDs { + return verr("issues_status.ids has %d ids, at most %d allowed", len(c.IDs), MaxConditionIDs) + } for _, id := range c.IDs { if !validUUID(id) { return verr("issues_status.ids must be uuids, got %q", id) @@ -165,6 +182,13 @@ func validateIssuesStatus(c IssuesStatusCond) error { if hasAll == hasAny { return verr("exactly one of issues_status.all or issues_status.any must be set") } + status := c.All + if hasAny { + status = c.Any + } + if !isValidIssueStatus(status) { + return verr("issues_status status %q is not a valid issue status", status) + } return nil } @@ -179,8 +203,19 @@ func validateIssueField(c IssueFieldCond) error { if hasEq == hasIn { return verr("exactly one of issue_field.eq or issue_field.in must be set") } - // Field-typed values: status is a free string; the id-shaped fields require uuids. - if c.Field != IssueFieldStatus { + if len(c.In) > MaxConditionIDs { + return verr("issue_field.in has %d values, at most %d allowed", len(c.In), MaxConditionIDs) + } + switch c.Field { + case IssueFieldStatus: + // status is a free string but must be a persistable issue status. + for _, v := range collectValues(c.Eq, c.In) { + if !isValidIssueStatus(v) { + return verr("issue_field status %q is not a valid issue status", v) + } + } + default: + // id-shaped fields (assignee_id / parent_issue_id) require uuids. if hasEq && !validUUID(c.Eq) { return verr("issue_field.eq must be a uuid for field %q", c.Field) } @@ -193,6 +228,16 @@ func validateIssueField(c IssueFieldCond) error { return nil } +// collectValues returns eq (if set) plus the in slice as one list. +func collectValues(eq string, in []string) []string { + out := make([]string, 0, len(in)+1) + if eq != "" { + out = append(out, eq) + } + out = append(out, in...) + return out +} + func validateFire(spec HookSpec) error { switch spec.Fire.Mode { case FirePerEvent: @@ -225,6 +270,18 @@ func validateRisingEdgeCoverage(spec HookSpec) error { return nil } +// actionAllowedFields declares the exact set of ActionSpec parameter fields each +// action type may set. Any other non-empty field is rejected so a stray param +// (e.g. an agent_id smuggled onto add_comment) can never be persisted onto a +// revision (MUL-4332 PR2 review point 3). +var actionAllowedFields = map[string]map[string]bool{ + ActionSetIssueStatus: {"issue_id": true, "status": true}, + ActionTriggerAgent: {"issue_id": true, "agent_id": true}, + ActionAddComment: {"issue_id": true, "message": true}, + ActionSendInbox: {"member_id": true, "message": true}, + ActionRunAutopilot: {"autopilot_id": true}, +} + func validateAction(a ActionSpec) error { if systemActionTypes[a.Type] { return verr("action type %q is reserved for system hooks", a.Type) @@ -232,6 +289,12 @@ func validateAction(a ActionSpec) error { if !userActionTypes[a.Type] { return verr("unknown action type %q", a.Type) } + if err := rejectUnexpectedActionFields(a); err != nil { + return err + } + if a.Message != "" && len(a.Message) > MaxMessageLength { + return verr("%s message must be at most %d characters", a.Type, MaxMessageLength) + } switch a.Type { case ActionSetIssueStatus: if !validUUID(a.IssueID) { @@ -240,6 +303,9 @@ func validateAction(a ActionSpec) error { if a.Status == "" { return verr("set_issue_status requires status") } + if !isValidIssueStatus(a.Status) { + return verr("set_issue_status status %q is not a valid issue status", a.Status) + } case ActionTriggerAgent: if !validUUID(a.IssueID) { return verr("trigger_agent requires a valid issue_id") @@ -269,6 +335,26 @@ func validateAction(a ActionSpec) error { return nil } +// rejectUnexpectedActionFields fails if the action sets any parameter field not +// allowed for its type — strict "exactly the allowed fields" enforcement. +func rejectUnexpectedActionFields(a ActionSpec) error { + allowed := actionAllowedFields[a.Type] + present := map[string]string{ + "issue_id": a.IssueID, + "status": a.Status, + "agent_id": a.AgentID, + "member_id": a.MemberID, + "message": a.Message, + "autopilot_id": a.AutopilotID, + } + for field, value := range present { + if value != "" && !allowed[field] { + return verr("%s does not accept field %q", a.Type, field) + } + } + return nil +} + func validUUID(s string) bool { if s == "" { return false diff --git a/server/internal/automation/validate_test.go b/server/internal/automation/validate_test.go index ce6d3839b5..ca8258eade 100644 --- a/server/internal/automation/validate_test.go +++ b/server/internal/automation/validate_test.go @@ -79,6 +79,17 @@ func TestValidateRejectsInvalidSpecs(t *testing.T) { {"set_issue_status missing status", func(s *HookSpec) { s.Do = []ActionSpec{{Type: ActionSetIssueStatus, IssueID: uuidC}} }, "requires status"}, + {"set_issue_status invalid status enum", func(s *HookSpec) { + s.Do = []ActionSpec{{Type: ActionSetIssueStatus, IssueID: uuidC, Status: "ascended"}} + }, "not a valid issue status"}, + {"add_comment with disallowed agent_id field", func(s *HookSpec) { + s.Do = []ActionSpec{{Type: ActionAddComment, IssueID: uuidC, Message: "hi", AgentID: uuidA}} + }, "does not accept field"}, + {"issues_status invalid status enum", func(s *HookSpec) { + s.When = WhenSpec{Event: "issue.status_changed"} + s.Fire = FireSpec{Mode: FirePerEvent} + s.If = []ConditionSpec{{IssuesStatus: &IssuesStatusCond{IDs: []string{uuidA}, All: "ascended"}}} + }, "not a valid issue status"}, {"trigger_agent bad agent", func(s *HookSpec) { s.Do = []ActionSpec{{Type: ActionTriggerAgent, IssueID: uuidC, AgentID: "x"}} }, "valid agent_id"}, diff --git a/server/internal/handler/hook.go b/server/internal/handler/hook.go index 51a9ed2ddc..e52c388ede 100644 --- a/server/internal/handler/hook.go +++ b/server/internal/handler/hook.go @@ -129,18 +129,17 @@ func (h *Handler) CreateHook(w http.ResponseWriter, r *http.Request) { return } - var spec automation.HookSpec - if err := json.NewDecoder(r.Body).Decode(&spec); err != nil { - writeError(w, http.StatusBadRequest, "invalid request body") - return - } - - author, ok := h.resolveHookAuthor(w, r, userID, workspaceID) + spec, ok := decodeHookSpec(w, r) if !ok { return } - result, err := h.HookService.CreateHook(r.Context(), workspaceUUID, spec, author) + author, canInvoke, ok := h.resolveHookWriter(w, r, userID, workspaceID) + if !ok { + return + } + + result, err := h.HookService.CreateHook(r.Context(), workspaceUUID, spec, author, canInvoke) if err != nil { h.writeHookError(w, err) return @@ -201,16 +200,15 @@ func (h *Handler) UpdateHook(w http.ResponseWriter, r *http.Request) { if !ok { return } - var spec automation.HookSpec - if err := json.NewDecoder(r.Body).Decode(&spec); err != nil { - writeError(w, http.StatusBadRequest, "invalid request body") - return - } - author, ok := h.resolveHookAuthor(w, r, userID, workspaceID) + spec, ok := decodeHookSpec(w, r) if !ok { return } - result, err := h.HookService.UpdateHook(r.Context(), workspaceUUID, hookUUID, spec, author) + author, canInvoke, ok := h.resolveHookWriter(w, r, userID, workspaceID) + if !ok { + return + } + result, err := h.HookService.UpdateHook(r.Context(), workspaceUUID, hookUUID, spec, author, canInvoke) if err != nil { h.writeHookError(w, err) return @@ -232,6 +230,11 @@ func (h *Handler) setHookEnabled(w http.ResponseWriter, r *http.Request, enabled if !h.hookEnabled(w, r) { return } + workspaceID := h.resolveWorkspaceID(r) + userID, ok := requireUserID(w, r) + if !ok { + return + } workspaceUUID, hookUUID, ok := h.hookPathParams(w, r) if !ok { return @@ -247,7 +250,11 @@ func (h *Handler) setHookEnabled(w http.ResponseWriter, r *http.Request, enabled } reason = body.Reason } - result, err := h.HookService.SetEnabled(r.Context(), workspaceUUID, hookUUID, enabled, reason) + author, _, ok := h.resolveHookWriter(w, r, userID, workspaceID) + if !ok { + return + } + result, err := h.HookService.SetEnabled(r.Context(), workspaceUUID, hookUUID, enabled, reason, author) if err != nil { h.writeHookError(w, err) return @@ -260,11 +267,20 @@ func (h *Handler) DeleteHook(w http.ResponseWriter, r *http.Request) { if !h.hookEnabled(w, r) { return } + workspaceID := h.resolveWorkspaceID(r) + userID, ok := requireUserID(w, r) + if !ok { + return + } workspaceUUID, hookUUID, ok := h.hookPathParams(w, r) if !ok { return } - if err := h.HookService.ArchiveHook(r.Context(), workspaceUUID, hookUUID); err != nil { + author, _, ok := h.resolveHookWriter(w, r, userID, workspaceID) + if !ok { + return + } + if err := h.HookService.ArchiveHook(r.Context(), workspaceUUID, hookUUID, author); err != nil { h.writeHookError(w, err) return } @@ -306,28 +322,61 @@ func (h *Handler) hookPathParams(w http.ResponseWriter, r *http.Request) (pgtype return workspaceUUID, hookUUID, true } -// resolveHookAuthor derives the audit creator actor and the accountable human -// principal (§8). A member acts under their own authority; an agent must resolve -// to the human behind its current task, otherwise creation is refused — an agent -// UUID is never a substitute for a human authorization principal. -func (h *Handler) resolveHookAuthor(w http.ResponseWriter, r *http.Request, userID, workspaceID string) (service.HookAuthor, bool) { +// decodeHookSpec strictly decodes the request body into a HookSpec, rejecting +// unknown fields so a stray top-level or nested key can never be silently +// accepted (MUL-4332 PR2 review point 3). Per-action disallowed fields are +// rejected by the automation validator. +func decodeHookSpec(w http.ResponseWriter, r *http.Request) (automation.HookSpec, bool) { + dec := json.NewDecoder(r.Body) + dec.DisallowUnknownFields() + var spec automation.HookSpec + if err := dec.Decode(&spec); err != nil { + writeError(w, http.StatusBadRequest, "invalid request body: "+err.Error()) + return automation.HookSpec{}, false + } + return spec, true +} + +// resolveHookWriter derives the audit creator actor, the accountable human +// principal (§8), and whether that principal is a workspace owner/admin, plus the +// agent-admission callback used for fail-closed trigger_agent validation. A +// member acts under their own authority; an agent must resolve to the human +// behind its current task. The resolved principal must still be a workspace +// member (review point 2) — an agent UUID is never a substitute for a human, and +// a departed principal can no longer author hooks. +func (h *Handler) resolveHookWriter(w http.ResponseWriter, r *http.Request, userID, workspaceID string) (service.HookAuthor, service.CanInvokeAgent, bool) { actorType, actorID := h.resolveActor(r, userID, workspaceID) actorUUID, err := util.ParseUUID(actorID) if err != nil { writeError(w, http.StatusBadRequest, "invalid actor id") - return service.HookAuthor{}, false + return service.HookAuthor{}, nil, false } principal := h.invokeOriginatorFromRequest(r, actorType, actorID) if principal == "" { h.writeDispatchBlocked(w, http.StatusForbidden, ReasonInvocationNotAllowed) - return service.HookAuthor{}, false + return service.HookAuthor{}, nil, false } principalUUID, err := util.ParseUUID(principal) if err != nil { writeError(w, http.StatusForbidden, "no accountable authorization principal") - return service.HookAuthor{}, false + return service.HookAuthor{}, nil, false } - return service.HookAuthor{ActorType: actorType, ActorID: actorUUID, PrincipalUserID: principalUUID}, true + // The accountable principal must still be a member of this workspace. + member, err := h.getWorkspaceMember(r.Context(), principal, workspaceID) + if err != nil { + writeError(w, http.StatusForbidden, "authorization principal is not a member of this workspace") + return service.HookAuthor{}, nil, false + } + author := service.HookAuthor{ + ActorType: actorType, + ActorID: actorUUID, + PrincipalUserID: principalUUID, + IsWorkspaceAdmin: roleAllowed(member.Role, "owner", "admin"), + } + canInvoke := func(agent db.Agent) bool { + return h.canInvokeAgent(r.Context(), agent, actorType, actorID, principal, workspaceID) + } + return author, canInvoke, true } func (h *Handler) writeHookError(w http.ResponseWriter, err error) { @@ -340,6 +389,8 @@ func (h *Handler) writeHookError(w http.ResponseWriter, err error) { writeError(w, http.StatusNotFound, "hook not found") case errors.Is(err, service.ErrHookSystemManaged): writeError(w, http.StatusForbidden, err.Error()) + case errors.Is(err, service.ErrHookForbidden): + writeError(w, http.StatusForbidden, err.Error()) case errors.Is(err, service.ErrHookNoPrincipal): writeError(w, http.StatusForbidden, "no accountable authorization principal") default: diff --git a/server/internal/handler/hook_test.go b/server/internal/handler/hook_test.go index 51bcbc810b..df68606f36 100644 --- a/server/internal/handler/hook_test.go +++ b/server/internal/handler/hook_test.go @@ -4,14 +4,28 @@ import ( "bytes" "context" "encoding/json" + "fmt" "net/http" "net/http/httptest" + "sync" + "sync/atomic" "testing" + "github.com/multica-ai/multica/server/internal/automation" "github.com/multica-ai/multica/server/internal/featureflags" + "github.com/multica-ai/multica/server/internal/service" "github.com/multica-ai/multica/server/pkg/featureflag" ) +// hookSpecFromMap round-trips a test spec map through JSON into a typed HookSpec, +// for tests that drive the service layer directly. +func hookSpecFromMap(m map[string]any) automation.HookSpec { + buf, _ := json.Marshal(m) + var spec automation.HookSpec + json.Unmarshal(buf, &spec) + return spec +} + // enableHooksFlag flips automation_event_hooks on for the shared test handler and // restores the previous flag service when the test ends. func enableHooksFlag(t *testing.T) { @@ -23,11 +37,14 @@ func enableHooksFlag(t *testing.T) { t.Cleanup(func() { testHandler.FeatureFlags = prev }) } -// newMemberHookRequest builds a member-authenticated request (X-User-ID + -// X-Workspace-ID), the same identity a member hits the REST API with. func newMemberHookRequest(method, path string, body any) *http.Request { + return newUserHookRequest(method, path, body, testUserID) +} + +// newUserHookRequest builds a member-authenticated request for the given user. +func newUserHookRequest(method, path string, body any, userID string) *http.Request { req := newJSONRequest(method, path, body) - req.Header.Set("X-User-ID", testUserID) + req.Header.Set("X-User-ID", userID) req.Header.Set("X-Workspace-ID", testWorkspaceID) return req } @@ -44,9 +61,74 @@ func newJSONRequest(method, path string, body any) *http.Request { return r } -// A minimal valid per_event spec; action targets are arbitrary uuids (PR2 -// validates uuid shape, not existence — matching/execution is a later slice). -func sampleHookSpec(name, message string) map[string]any { +// seedHookIssue inserts a real issue in the test workspace and returns its id. +func seedHookIssue(t *testing.T) string { + t.Helper() + var id string + if err := testPool.QueryRow(context.Background(), ` + INSERT INTO issue (workspace_id, title, status, priority, creator_type, creator_id, number) + VALUES ($1, 'hook target issue', 'todo', 'medium', 'member', $2, + COALESCE((SELECT MAX(number) FROM issue WHERE workspace_id = $1), 0) + 1) + RETURNING id`, testWorkspaceID, testUserID).Scan(&id); err != nil { + t.Fatalf("seed issue: %v", err) + } + t.Cleanup(func() { testPool.Exec(context.Background(), `DELETE FROM issue WHERE id = $1`, id) }) + return id +} + +// seededHookAgentID returns the workspace-visible agent seeded by the fixture. +func seededHookAgentID(t *testing.T) string { + t.Helper() + var id string + if err := testPool.QueryRow(context.Background(), + `SELECT id FROM agent WHERE workspace_id = $1 AND name = 'Handler Test Agent' LIMIT 1`, + testWorkspaceID).Scan(&id); err != nil { + t.Fatalf("load seeded agent: %v", err) + } + return id +} + +// testOwnerMemberID returns the member row id of the fixture owner (testUserID). +func testOwnerMemberID(t *testing.T) string { + t.Helper() + var id string + if err := testPool.QueryRow(context.Background(), + `SELECT id FROM member WHERE workspace_id = $1 AND user_id = $2`, + testWorkspaceID, testUserID).Scan(&id); err != nil { + t.Fatalf("load owner member: %v", err) + } + return id +} + +// hookSeedCounter makes each seeded user's email unique across calls. +var hookSeedCounter atomic.Int64 + +// seedHookMember inserts a user + workspace member with the given role and +// returns the user id. +func seedHookMember(t *testing.T, role string) string { + t.Helper() + ctx := context.Background() + var userID string + email := fmt.Sprintf("hook-%s-%d-%d@test.local", role, hookSeedCounter.Add(1), len(t.Name())) + if err := testPool.QueryRow(ctx, + `INSERT INTO "user" (name, email) VALUES ($1, $2) RETURNING id`, + "Hook "+role, email).Scan(&userID); err != nil { + t.Fatalf("seed user: %v", err) + } + if _, err := testPool.Exec(ctx, + `INSERT INTO member (workspace_id, user_id, role) VALUES ($1, $2, $3)`, + testWorkspaceID, userID, role); err != nil { + t.Fatalf("seed member: %v", err) + } + t.Cleanup(func() { + testPool.Exec(context.Background(), `DELETE FROM member WHERE workspace_id = $1 AND user_id = $2`, testWorkspaceID, userID) + testPool.Exec(context.Background(), `DELETE FROM "user" WHERE id = $1`, userID) + }) + return userID +} + +// sampleHookSpec is a minimal valid per_event spec: comment an existing issue. +func sampleHookSpec(name, message, issueID string) map[string]any { return map[string]any{ "name": name, "when": map[string]any{ @@ -55,42 +137,45 @@ func sampleHookSpec(name, message string) map[string]any { }, "fire": map[string]any{"mode": "per_event"}, "do": []any{ - map[string]any{"type": "add_comment", "issue_id": "55555555-5555-5555-5555-555555555555", "message": message}, + map[string]any{"type": "add_comment", "issue_id": issueID, "message": message}, }, } } +func createHookAs(t *testing.T, userID string, spec map[string]any) HookResponse { + t.Helper() + w := httptest.NewRecorder() + testHandler.CreateHook(w, newUserHookRequest(http.MethodPost, "/api/hooks", spec, userID)) + if w.Code != http.StatusCreated { + t.Fatalf("create hook as %s: status %d: %s", userID, w.Code, w.Body.String()) + } + var resp HookResponse + if err := json.NewDecoder(w.Body).Decode(&resp); err != nil { + t.Fatalf("decode create: %v", err) + } + t.Cleanup(func() { + testPool.Exec(context.Background(), `DELETE FROM hook_revision WHERE hook_id = $1`, resp.ID) + testPool.Exec(context.Background(), `DELETE FROM hook WHERE id = $1`, resp.ID) + }) + return resp +} + func TestHookCRUDLifecycle(t *testing.T) { if testPool == nil { t.Skip("database unavailable") } enableHooksFlag(t) ctx := context.Background() + issueID := seedHookIssue(t) - // Create → revision #1. - w := httptest.NewRecorder() - testHandler.CreateHook(w, newMemberHookRequest(http.MethodPost, "/api/hooks", sampleHookSpec("lifecycle hook", "first"))) - if w.Code != http.StatusCreated { - t.Fatalf("create: status %d: %s", w.Code, w.Body.String()) - } - var created HookResponse - if err := json.NewDecoder(w.Body).Decode(&created); err != nil { - t.Fatalf("decode create: %v", err) - } - if created.ID == "" || created.Revision.Revision != 1 || !created.Enabled { + created := createHookAs(t, testUserID, sampleHookSpec("lifecycle hook", "first", issueID)) + if created.Revision.Revision != 1 || !created.Enabled || created.Revision.Event != "issue.status_changed" { t.Fatalf("unexpected create response: %+v", created) } - if created.Revision.Event != "issue.status_changed" || created.Revision.FireMode != "per_event" { - t.Errorf("revision fields wrong: %+v", created.Revision) - } hookID := created.ID - t.Cleanup(func() { - testPool.Exec(context.Background(), `DELETE FROM hook_revision WHERE hook_id = $1`, hookID) - testPool.Exec(context.Background(), `DELETE FROM hook WHERE id = $1`, hookID) - }) // Get. - w = httptest.NewRecorder() + w := httptest.NewRecorder() testHandler.GetHook(w, withURLParam(newMemberHookRequest(http.MethodGet, "/api/hooks/"+hookID, nil), "id", hookID)) if w.Code != http.StatusOK { t.Fatalf("get: status %d: %s", w.Code, w.Body.String()) @@ -98,31 +183,24 @@ func TestHookCRUDLifecycle(t *testing.T) { // Update → new immutable revision #2, active pointer moves, name updated. w = httptest.NewRecorder() - testHandler.UpdateHook(w, withURLParam(newMemberHookRequest(http.MethodPatch, "/api/hooks/"+hookID, sampleHookSpec("renamed hook", "second")), "id", hookID)) + testHandler.UpdateHook(w, withURLParam(newMemberHookRequest(http.MethodPatch, "/api/hooks/"+hookID, sampleHookSpec("renamed hook", "second", issueID)), "id", hookID)) if w.Code != http.StatusOK { t.Fatalf("update: status %d: %s", w.Code, w.Body.String()) } var updated HookResponse json.NewDecoder(w.Body).Decode(&updated) - if updated.Revision.Revision != 2 || updated.Name != "renamed hook" { - t.Fatalf("update did not append revision / rename: %+v", updated) + if updated.Revision.Revision != 2 || updated.Name != "renamed hook" || updated.Revision.ID == created.Revision.ID { + t.Fatalf("update did not append a new revision / rename: %+v", updated) } - if updated.Revision.ID == created.Revision.ID { - t.Errorf("update must create a NEW revision id, got same %s", updated.Revision.ID) - } - // The original revision row is retained (immutable history). var revCount int testPool.QueryRow(ctx, `SELECT count(*) FROM hook_revision WHERE hook_id = $1`, hookID).Scan(&revCount) if revCount != 2 { - t.Errorf("hook_revision count = %d, want 2 (revisions are immutable, never overwritten)", revCount) + t.Errorf("hook_revision count = %d, want 2 (revisions are immutable)", revCount) } - // List includes it. + // List. w = httptest.NewRecorder() testHandler.ListHooks(w, newMemberHookRequest(http.MethodGet, "/api/hooks", nil)) - if w.Code != http.StatusOK { - t.Fatalf("list: status %d", w.Code) - } var list []HookResponse json.NewDecoder(w.Body).Decode(&list) if !containsHook(list, hookID) { @@ -132,9 +210,6 @@ func TestHookCRUDLifecycle(t *testing.T) { // Disable → enabled=false with reason. w = httptest.NewRecorder() testHandler.DisableHook(w, withURLParam(newMemberHookRequest(http.MethodPost, "/api/hooks/"+hookID+"/disable", map[string]any{"reason": "paused"}), "id", hookID)) - if w.Code != http.StatusOK { - t.Fatalf("disable: status %d: %s", w.Code, w.Body.String()) - } var disabled HookResponse json.NewDecoder(w.Body).Decode(&disabled) if disabled.Enabled || disabled.DisabledReason != "paused" { @@ -150,17 +225,12 @@ func TestHookCRUDLifecycle(t *testing.T) { t.Errorf("enable did not take: %+v", reenabled) } - // Executions trace is empty (no matcher runs in PR2) but the endpoint works. + // Executions trace is empty in store-only PR2 but the endpoint works. w = httptest.NewRecorder() testHandler.ListHookExecutions(w, withURLParam(newMemberHookRequest(http.MethodGet, "/api/hooks/"+hookID+"/executions", nil), "id", hookID)) if w.Code != http.StatusOK { t.Fatalf("executions: status %d", w.Code) } - var execs []HookExecutionResponse - json.NewDecoder(w.Body).Decode(&execs) - if len(execs) != 0 { - t.Errorf("executions = %d, want 0 in store-only PR2", len(execs)) - } // Delete (soft archive) → subsequent get 404s. w = httptest.NewRecorder() @@ -173,15 +243,8 @@ func TestHookCRUDLifecycle(t *testing.T) { if w.Code != http.StatusNotFound { t.Errorf("get after archive: status %d, want 404", w.Code) } - // The row is soft-archived, not physically deleted. - var archived bool - testPool.QueryRow(ctx, `SELECT archived_at IS NOT NULL FROM hook WHERE id = $1`, hookID).Scan(&archived) - if !archived { - t.Errorf("hook should be soft-archived, not deleted") - } } -// The whole surface is invisible unless the feature flag is on. func TestHookRequiresFeatureFlag(t *testing.T) { if testPool == nil { t.Skip("database unavailable") @@ -191,24 +254,211 @@ func TestHookRequiresFeatureFlag(t *testing.T) { t.Cleanup(func() { testHandler.FeatureFlags = prev }) w := httptest.NewRecorder() - testHandler.CreateHook(w, newMemberHookRequest(http.MethodPost, "/api/hooks", sampleHookSpec("blocked", "x"))) + testHandler.CreateHook(w, newMemberHookRequest(http.MethodPost, "/api/hooks", sampleHookSpec("blocked", "x", "55555555-5555-5555-5555-555555555555"))) if w.Code != http.StatusNotFound { t.Errorf("create with flag off: status %d, want 404", w.Code) } } -// A bad spec is rejected at the API boundary with 400, never reaching the store. -func TestHookCreateRejectsInvalidSpec(t *testing.T) { +// Strict schema: unknown fields and per-action disallowed fields and bad status +// are all rejected at the boundary with 400, never persisted (review point 3). +func TestHookStrictSchemaRejections(t *testing.T) { if testPool == nil { t.Skip("database unavailable") } enableHooksFlag(t) - bad := sampleHookSpec("bad", "x") - bad["do"] = []any{map[string]any{"type": "set_issue_status_many"}} // system-only + issueID := seedHookIssue(t) + + cases := map[string]func() map[string]any{ + "unknown top-level field": func() map[string]any { + s := sampleHookSpec("x", "hi", issueID) + s["unexpected"] = true + return s + }, + "disallowed action field": func() map[string]any { + s := sampleHookSpec("x", "hi", issueID) + s["do"] = []any{map[string]any{"type": "add_comment", "issue_id": issueID, "message": "hi", "agent_id": "66666666-6666-6666-6666-666666666666"}} + return s + }, + "invalid status enum": func() map[string]any { + s := sampleHookSpec("x", "hi", issueID) + s["do"] = []any{map[string]any{"type": "set_issue_status", "issue_id": issueID, "status": "ascended"}} + return s + }, + "system-only action": func() map[string]any { + s := sampleHookSpec("x", "hi", issueID) + s["do"] = []any{map[string]any{"type": "set_issue_status_many"}} + return s + }, + } + for name, build := range cases { + t.Run(name, func(t *testing.T) { + w := httptest.NewRecorder() + testHandler.CreateHook(w, newMemberHookRequest(http.MethodPost, "/api/hooks", build())) + if w.Code != http.StatusBadRequest { + t.Errorf("status %d, want 400: %s", w.Code, w.Body.String()) + } + }) + } +} + +// Fail-closed target validation: a spec that references a target absent from the +// workspace is rejected with 400, not persisted (review point 2). +func TestHookFailClosedTargets(t *testing.T) { + if testPool == nil { + t.Skip("database unavailable") + } + enableHooksFlag(t) + realIssue := seedHookIssue(t) + const ghost = "77777777-7777-7777-7777-777777777777" + + action := func(a map[string]any) map[string]any { + s := sampleHookSpec("targets", "hi", realIssue) + s["do"] = []any{a} + return s + } + cases := map[string]map[string]any{ + "nonexistent issue": action(map[string]any{"type": "add_comment", "issue_id": ghost, "message": "hi"}), + "nonexistent member": action(map[string]any{"type": "send_inbox", "member_id": ghost, "message": "hi"}), + "nonexistent agent": action(map[string]any{"type": "trigger_agent", "issue_id": realIssue, "agent_id": ghost}), + "nonexistent autopilot": action(map[string]any{"type": "run_autopilot", "autopilot_id": ghost}), + } + for name, spec := range cases { + t.Run(name, func(t *testing.T) { + w := httptest.NewRecorder() + testHandler.CreateHook(w, newMemberHookRequest(http.MethodPost, "/api/hooks", spec)) + if w.Code != http.StatusBadRequest { + t.Errorf("status %d, want 400 (fail-closed): %s", w.Code, w.Body.String()) + } + }) + } + + // A real, invokable agent target is accepted. + t.Run("real invokable agent accepted", func(t *testing.T) { + spec := action(map[string]any{"type": "trigger_agent", "issue_id": realIssue, "agent_id": seededHookAgentID(t)}) + createHookAs(t, testUserID, spec) // fails the test if not 201 + }) +} + +// Only the hook's principal or a workspace owner/admin may edit it; an arbitrary +// member cannot rewrite a rule that keeps running under someone else's authority +// (review point 1). +func TestHookEditAuthorization(t *testing.T) { + if testPool == nil { + t.Skip("database unavailable") + } + enableHooksFlag(t) + issueID := seedHookIssue(t) + + author := seedHookMember(t, "member") // principal, non-admin + other := seedHookMember(t, "member") // non-principal, non-admin + + hook := createHookAs(t, author, sampleHookSpec("owned by author", "hi", issueID)) + + // A different non-admin member cannot edit / disable / delete it. + patch := withURLParam(newUserHookRequest(http.MethodPatch, "/api/hooks/"+hook.ID, sampleHookSpec("hijacked", "x", issueID), other), "id", hook.ID) w := httptest.NewRecorder() - testHandler.CreateHook(w, newMemberHookRequest(http.MethodPost, "/api/hooks", bad)) - if w.Code != http.StatusBadRequest { - t.Errorf("create with system-only action: status %d, want 400", w.Code) + testHandler.UpdateHook(w, patch) + if w.Code != http.StatusForbidden { + t.Errorf("other member PATCH: status %d, want 403", w.Code) + } + w = httptest.NewRecorder() + testHandler.DisableHook(w, withURLParam(newUserHookRequest(http.MethodPost, "/api/hooks/"+hook.ID+"/disable", nil, other), "id", hook.ID)) + if w.Code != http.StatusForbidden { + t.Errorf("other member disable: status %d, want 403", w.Code) + } + w = httptest.NewRecorder() + testHandler.DeleteHook(w, withURLParam(newUserHookRequest(http.MethodDelete, "/api/hooks/"+hook.ID, nil, other), "id", hook.ID)) + if w.Code != http.StatusForbidden { + t.Errorf("other member delete: status %d, want 403", w.Code) + } + + // The principal can edit their own hook. + w = httptest.NewRecorder() + testHandler.UpdateHook(w, withURLParam(newUserHookRequest(http.MethodPatch, "/api/hooks/"+hook.ID, sampleHookSpec("by principal", "x", issueID), author), "id", hook.ID)) + if w.Code != http.StatusOK { + t.Errorf("principal PATCH: status %d, want 200: %s", w.Code, w.Body.String()) + } + + // A workspace owner/admin (the fixture owner) can edit any hook. + w = httptest.NewRecorder() + testHandler.UpdateHook(w, withURLParam(newMemberHookRequest(http.MethodPatch, "/api/hooks/"+hook.ID, sampleHookSpec("by admin", "x", issueID)), "id", hook.ID)) + if w.Code != http.StatusOK { + t.Errorf("admin PATCH: status %d, want 200: %s", w.Code, w.Body.String()) + } + + // The principal was NOT transferred by any edit — it is still the author. + var principal string + testPool.QueryRow(context.Background(), + `SELECT authorization_principal_user_id FROM hook WHERE id = $1`, hook.ID).Scan(&principal) + if principal != author { + t.Errorf("principal = %s, want %s (edits must not transfer the principal)", principal, author) + } +} + +// Concurrent PATCHes on the same hook must each append a distinct, contiguous +// revision without a MAX+1 unique-index collision surfacing as a 500 (review +// point 4). Driven at the service layer so all workers hit the pool concurrently. +func TestHookConcurrentPatchAppendsContiguousRevisions(t *testing.T) { + if testPool == nil { + t.Skip("database unavailable") + } + enableHooksFlag(t) + ctx := context.Background() + issueID := seedHookIssue(t) + hook := createHookAs(t, testUserID, sampleHookSpec("concurrent", "hi", issueID)) + + admin := service.HookAuthor{ + ActorType: "member", + ActorID: parseUUID(testUserID), + PrincipalUserID: parseUUID(testUserID), + IsWorkspaceAdmin: true, + } + hookUUID := parseUUID(hook.ID) + wsUUID := parseUUID(testWorkspaceID) + + const workers = 8 + start := make(chan struct{}) + errs := make(chan error, workers) + var wg sync.WaitGroup + for i := 0; i < workers; i++ { + wg.Add(1) + go func(n int) { + defer wg.Done() + <-start + spec := hookSpecFromMap(sampleHookSpec(fmt.Sprintf("rev-%d", n), "x", issueID)) + _, err := testHandler.HookService.UpdateHook(ctx, wsUUID, hookUUID, spec, admin, nil) + errs <- err + }(i) + } + close(start) + wg.Wait() + close(errs) + for err := range errs { + if err != nil { + t.Fatalf("concurrent PATCH errored (revision race not serialized): %v", err) + } + } + + // Revisions must be exactly 1..(workers+1), contiguous and unique. + rows, err := testPool.Query(ctx, `SELECT revision FROM hook_revision WHERE hook_id = $1 ORDER BY revision`, hook.ID) + if err != nil { + t.Fatalf("query revisions: %v", err) + } + defer rows.Close() + var revs []int + for rows.Next() { + var r int + rows.Scan(&r) + revs = append(revs, r) + } + if len(revs) != workers+1 { + t.Fatalf("revision count = %d, want %d", len(revs), workers+1) + } + for i, r := range revs { + if r != i+1 { + t.Fatalf("revisions not contiguous at index %d: got %d, want %d (revs=%v)", i, r, i+1, revs) + } } } @@ -218,7 +468,8 @@ func TestHookAgentRequiresPrincipal(t *testing.T) { t.Skip("database unavailable") } enableHooksFlag(t) - req := newMemberHookRequest(http.MethodPost, "/api/hooks", sampleHookSpec("agent hook", "x")) + issueID := seedHookIssue(t) + req := newMemberHookRequest(http.MethodPost, "/api/hooks", sampleHookSpec("agent hook", "x", issueID)) // Trusted agent identity (task_token), valid agent uuid, but no X-Task-ID means // no originator can be resolved → no accountable principal. req.Header.Set("X-Actor-Source", "task_token") diff --git a/server/internal/service/hook.go b/server/internal/service/hook.go index dc06bb7e32..b0fa44fccf 100644 --- a/server/internal/service/hook.go +++ b/server/internal/service/hook.go @@ -15,22 +15,32 @@ import ( ) // Hook CRUD errors surfaced to the handler for status mapping. Validation -// problems flow through as automation.ValidationError (→ 400). +// problems (shape or unresolvable target) flow through as +// *automation.ValidationError (→ 400). var ( ErrHookNotFound = errors.New("hook not found") ErrHookSystemManaged = errors.New("system-managed hooks cannot be modified through this API") ErrHookNoPrincipal = errors.New("no accountable authorization principal for this hook") + ErrHookForbidden = errors.New("only the hook's principal or a workspace admin may modify it") ) -// HookAuthor carries the resolved identity for a create/update: who is acting -// (creator, pure audit) and the accountable human whose authority the hook runs -// under (§8). An agent author must resolve to a real member principal. +// HookAuthor carries the resolved identity for a hook write: who is acting +// (creator, pure audit), the accountable human whose authority the hook runs +// under (§8), and whether that human is a workspace owner/admin. An agent author +// must resolve to a real member principal. type HookAuthor struct { - ActorType string // member | agent - ActorID pgtype.UUID - PrincipalUserID pgtype.UUID + ActorType string // member | agent + ActorID pgtype.UUID + PrincipalUserID pgtype.UUID + IsWorkspaceAdmin bool } +// CanInvokeAgent is the admission predicate the handler supplies (wrapping +// Handler.canInvokeAgent) so the service can fail-closed on a trigger_agent +// target without importing request context. A nil predicate denies every +// trigger_agent target (fail-closed). +type CanInvokeAgent func(agent db.Agent) bool + // HookWithRevision pairs a hook row with its active revision so the handler can // render one complete view. The service returns db rows; the handler shapes JSON. type HookWithRevision struct { @@ -52,10 +62,11 @@ func NewHookService(q *db.Queries, tx TxStarter) *HookService { return &HookService{Queries: q, TxStarter: tx} } -// CreateHook validates the spec, resolves scope + principal, and inserts the -// hook together with revision #1 in one transaction. The two rows reference -// each other, so both ids are generated up front. -func (s *HookService) CreateHook(ctx context.Context, workspaceID pgtype.UUID, spec automation.HookSpec, author HookAuthor) (HookWithRevision, error) { +// CreateHook validates the spec (shape + workspace-scoped targets), resolves +// scope + principal, and inserts the hook together with revision #1 in one +// transaction. The two rows reference each other, so both ids are generated up +// front. +func (s *HookService) CreateHook(ctx context.Context, workspaceID pgtype.UUID, spec automation.HookSpec, author HookAuthor, canInvoke CanInvokeAgent) (HookWithRevision, error) { if err := automation.Validate(spec); err != nil { return HookWithRevision{}, err } @@ -76,6 +87,9 @@ func (s *HookService) CreateHook(ctx context.Context, workspaceID pgtype.UUID, s var out HookWithRevision err = s.inTx(ctx, func(qtx *db.Queries) error { + if err := validateTargets(ctx, qtx, workspaceID, spec, canInvoke); err != nil { + return err + } hook, err := qtx.CreateHook(ctx, db.CreateHookParams{ ID: hookID, WorkspaceID: workspaceID, @@ -117,36 +131,41 @@ func (s *HookService) CreateHook(ctx context.Context, workspaceID pgtype.UUID, s } // UpdateHook appends a new immutable revision from the spec and repoints the -// hook's active revision (§5.1). Existing executions stay pinned to their -// original revision. System-managed hooks are not editable here. -func (s *HookService) UpdateHook(ctx context.Context, workspaceID, hookID pgtype.UUID, spec automation.HookSpec, author HookAuthor) (HookWithRevision, error) { +// hook's active revision (§5.1). It locks the hook row first so concurrent +// PATCHes serialize and MAX(revision)+1 can never collide (review point 4), and +// re-checks archived/origin/authorization inside the lock. Only the hook's +// principal or a workspace admin may edit (review point 1). Scope is immutable. +func (s *HookService) UpdateHook(ctx context.Context, workspaceID, hookID pgtype.UUID, spec automation.HookSpec, author HookAuthor, canInvoke CanInvokeAgent) (HookWithRevision, error) { if err := automation.Validate(spec); err != nil { return HookWithRevision{}, err } - // Scope is immutable after creation; UpdateHook only replaces the revision - // config and the display name. match, conditions, actions, err := marshalRevisionConfig(spec) if err != nil { return HookWithRevision{}, err } - existing, err := s.Queries.GetHookInWorkspace(ctx, db.GetHookInWorkspaceParams{ID: hookID, WorkspaceID: workspaceID}) - if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - return HookWithRevision{}, ErrHookNotFound - } - return HookWithRevision{}, err - } - if existing.ArchivedAt.Valid { - return HookWithRevision{}, ErrHookNotFound - } - if existing.Origin == "system" { - return HookWithRevision{}, ErrHookSystemManaged - } - revisionID := util.NewUUID() var out HookWithRevision err = s.inTx(ctx, func(qtx *db.Queries) error { + existing, err := qtx.GetHookForUpdate(ctx, db.GetHookForUpdateParams{ID: hookID, WorkspaceID: workspaceID}) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return ErrHookNotFound + } + return err + } + if existing.ArchivedAt.Valid { + return ErrHookNotFound + } + if existing.Origin == "system" { + return ErrHookSystemManaged + } + if err := authorizeHookEdit(existing, author); err != nil { + return err + } + if err := validateTargets(ctx, qtx, workspaceID, spec, canInvoke); err != nil { + return err + } maxRev, err := qtx.GetMaxHookRevision(ctx, hookID) if err != nil { return err @@ -223,23 +242,12 @@ func (s *HookService) ListHooks(ctx context.Context, workspaceID pgtype.UUID) ([ } // SetEnabled enables/disables a hook. Disable only blocks future matches; it -// does not cancel queued/running executions (§5.1). -func (s *HookService) SetEnabled(ctx context.Context, workspaceID, hookID pgtype.UUID, enabled bool, reason string) (HookWithRevision, error) { - existing, err := s.Queries.GetHookInWorkspace(ctx, db.GetHookInWorkspaceParams{ID: hookID, WorkspaceID: workspaceID}) - if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - return HookWithRevision{}, ErrHookNotFound - } +// does not cancel queued/running executions (§5.1). Only the principal or a +// workspace admin may toggle it. +func (s *HookService) SetEnabled(ctx context.Context, workspaceID, hookID pgtype.UUID, enabled bool, reason string, author HookAuthor) (HookWithRevision, error) { + if _, err := s.loadEditableHook(ctx, workspaceID, hookID, author); err != nil { return HookWithRevision{}, err } - if existing.ArchivedAt.Valid { - return HookWithRevision{}, ErrHookNotFound - } - if existing.Origin == "system" { - // Enabling/disabling a system hook is an admin-only lifecycle op reserved - // for the PR5 system-hook management path, not this user CRUD API. - return HookWithRevision{}, ErrHookSystemManaged - } disabledReason := pgtype.Text{} if !enabled && reason != "" { disabledReason = pgtype.Text{String: reason, Valid: true} @@ -264,20 +272,11 @@ func (s *HookService) SetEnabled(ctx context.Context, workspaceID, hookID pgtype } // ArchiveHook soft-deletes a hook (§5.1); revisions/executions/effects are kept. -func (s *HookService) ArchiveHook(ctx context.Context, workspaceID, hookID pgtype.UUID) error { - existing, err := s.Queries.GetHookInWorkspace(ctx, db.GetHookInWorkspaceParams{ID: hookID, WorkspaceID: workspaceID}) - if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - return ErrHookNotFound - } +// Only the principal or a workspace admin may archive it. +func (s *HookService) ArchiveHook(ctx context.Context, workspaceID, hookID pgtype.UUID, author HookAuthor) error { + if _, err := s.loadEditableHook(ctx, workspaceID, hookID, author); err != nil { return err } - if existing.ArchivedAt.Valid { - return ErrHookNotFound - } - if existing.Origin == "system" { - return ErrHookSystemManaged - } if _, err := s.Queries.ArchiveHook(ctx, db.ArchiveHookParams{ID: hookID, WorkspaceID: workspaceID}); err != nil { if errors.Is(err, pgx.ErrNoRows) { return ErrHookNotFound @@ -299,6 +298,47 @@ func (s *HookService) ListExecutions(ctx context.Context, workspaceID, hookID pg return s.Queries.ListHookExecutionsByHook(ctx, db.ListHookExecutionsByHookParams{HookID: hookID, Limit: limit}) } +// loadEditableHook loads a non-archived, non-system hook and enforces the edit +// authorization gate, for the enable/disable/archive paths. +func (s *HookService) loadEditableHook(ctx context.Context, workspaceID, hookID pgtype.UUID, author HookAuthor) (db.Hook, error) { + existing, err := s.Queries.GetHookInWorkspace(ctx, db.GetHookInWorkspaceParams{ID: hookID, WorkspaceID: workspaceID}) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return db.Hook{}, ErrHookNotFound + } + return db.Hook{}, err + } + if existing.ArchivedAt.Valid { + return db.Hook{}, ErrHookNotFound + } + if existing.Origin == "system" { + return db.Hook{}, ErrHookSystemManaged + } + if err := authorizeHookEdit(existing, author); err != nil { + return db.Hook{}, err + } + return existing, nil +} + +// authorizeHookEdit implements the edit gate (review point 1): a workspace +// owner/admin may edit any hook; otherwise only the hook's original +// authorization principal may. The principal is NOT transferred on edit, so an +// arbitrary member can never rewrite a rule that keeps running under someone +// else's authority. +func authorizeHookEdit(hook db.Hook, author HookAuthor) error { + if author.IsWorkspaceAdmin { + return nil + } + if principalMatches(hook.AuthorizationPrincipalUserID, author.PrincipalUserID) { + return nil + } + return ErrHookForbidden +} + +func principalMatches(a, b pgtype.UUID) bool { + return a.Valid && b.Valid && a.Bytes == b.Bytes +} + func (s *HookService) inTx(ctx context.Context, fn func(qtx *db.Queries) error) error { tx, err := s.TxStarter.Begin(ctx) if err != nil { @@ -342,3 +382,120 @@ func marshalRevisionConfig(spec automation.HookSpec) (match, conditions, actions } return match, conditions, actions, nil } + +// validateTargets fail-closed checks that every id the spec references exists in +// the hook's workspace under the current principal (review point 2). §13 requires +// this at create/update time so an illegal configuration never enters the store +// and never reaches a worker. Uses the tx-bound queries so the checks share the +// write transaction. +func validateTargets(ctx context.Context, qtx *db.Queries, workspaceID pgtype.UUID, spec automation.HookSpec, canInvoke CanInvokeAgent) error { + if spec.Scope != nil && spec.Scope.Type == automation.ScopeIssue { + if err := requireIssue(ctx, qtx, workspaceID, spec.Scope.ID, "scope.id"); err != nil { + return err + } + } + for i, cond := range spec.If { + if cond.IssuesStatus != nil { + for _, id := range cond.IssuesStatus.IDs { + if err := requireIssue(ctx, qtx, workspaceID, id, fmt.Sprintf("if[%d].issues_status.ids", i)); err != nil { + return err + } + } + } + if cond.IssueField != nil { + if err := requireIssue(ctx, qtx, workspaceID, cond.IssueField.ID, fmt.Sprintf("if[%d].issue_field.id", i)); err != nil { + return err + } + } + } + for i, action := range spec.Do { + if err := validateActionTargets(ctx, qtx, workspaceID, i, action, canInvoke); err != nil { + return err + } + } + return nil +} + +func validateActionTargets(ctx context.Context, qtx *db.Queries, workspaceID pgtype.UUID, i int, a automation.ActionSpec, canInvoke CanInvokeAgent) error { + where := fmt.Sprintf("do[%d].%s", i, a.Type) + switch a.Type { + case automation.ActionSetIssueStatus, automation.ActionAddComment: + return requireIssue(ctx, qtx, workspaceID, a.IssueID, where+".issue_id") + case automation.ActionTriggerAgent: + if err := requireIssue(ctx, qtx, workspaceID, a.IssueID, where+".issue_id"); err != nil { + return err + } + return requireInvokableAgent(ctx, qtx, workspaceID, a.AgentID, where+".agent_id", canInvoke) + case automation.ActionSendInbox: + return requireMember(ctx, qtx, workspaceID, a.MemberID, where+".member_id") + case automation.ActionRunAutopilot: + return requireAutopilot(ctx, qtx, workspaceID, a.AutopilotID, where+".autopilot_id") + } + return nil +} + +func requireIssue(ctx context.Context, qtx *db.Queries, workspaceID pgtype.UUID, id, field string) error { + uid, err := util.ParseUUID(id) + if err != nil { + return automation.NewValidationError("%s must be a uuid", field) + } + if _, err := qtx.GetIssueInWorkspace(ctx, db.GetIssueInWorkspaceParams{ID: uid, WorkspaceID: workspaceID}); err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return automation.NewValidationError("%s references issue %s which does not exist in this workspace", field, id) + } + return err + } + return nil +} + +func requireMember(ctx context.Context, qtx *db.Queries, workspaceID pgtype.UUID, id, field string) error { + uid, err := util.ParseUUID(id) + if err != nil { + return automation.NewValidationError("%s must be a uuid", field) + } + if _, err := qtx.GetMemberInWorkspace(ctx, db.GetMemberInWorkspaceParams{ID: uid, WorkspaceID: workspaceID}); err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return automation.NewValidationError("%s references member %s which is not in this workspace", field, id) + } + return err + } + return nil +} + +func requireAutopilot(ctx context.Context, qtx *db.Queries, workspaceID pgtype.UUID, id, field string) error { + uid, err := util.ParseUUID(id) + if err != nil { + return automation.NewValidationError("%s must be a uuid", field) + } + if _, err := qtx.GetAutopilotInWorkspace(ctx, db.GetAutopilotInWorkspaceParams{ID: uid, WorkspaceID: workspaceID}); err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return automation.NewValidationError("%s references autopilot %s which does not exist in this workspace", field, id) + } + return err + } + return nil +} + +// requireInvokableAgent confirms the agent exists in the workspace, is not +// archived, has a runtime, and is invokable by the current principal — the same +// admission the interactive trigger path enforces, applied fail-closed at save. +func requireInvokableAgent(ctx context.Context, qtx *db.Queries, workspaceID pgtype.UUID, id, field string, canInvoke CanInvokeAgent) error { + uid, err := util.ParseUUID(id) + if err != nil { + return automation.NewValidationError("%s must be a uuid", field) + } + agent, err := qtx.GetAgentInWorkspace(ctx, db.GetAgentInWorkspaceParams{ID: uid, WorkspaceID: workspaceID}) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return automation.NewValidationError("%s references agent %s which does not exist in this workspace", field, id) + } + return err + } + if agent.ArchivedAt.Valid || !agent.RuntimeID.Valid { + return automation.NewValidationError("%s references agent %s which is archived or has no runtime", field, id) + } + if canInvoke == nil || !canInvoke(agent) { + return automation.NewValidationError("%s references agent %s which the hook's principal may not invoke", field, id) + } + return nil +} diff --git a/server/pkg/db/generated/hook.sql.go b/server/pkg/db/generated/hook.sql.go index 82d4fab9ff..7f51bf31fa 100644 --- a/server/pkg/db/generated/hook.sql.go +++ b/server/pkg/db/generated/hook.sql.go @@ -179,6 +179,47 @@ func (q *Queries) CreateHookRevision(ctx context.Context, arg CreateHookRevision return i, err } +const getHookForUpdate = `-- name: GetHookForUpdate :one +SELECT id, workspace_id, name, enabled, active_revision_id, scope_type, scope_id, retire_after_event_seq, origin, system_key, system_version, creator_actor_type, creator_actor_id, authorization_principal_user_id, disabled_reason, created_at, updated_at, archived_at FROM hook +WHERE id = $1 AND workspace_id = $2 +FOR UPDATE +` + +type GetHookForUpdateParams struct { + ID pgtype.UUID `json:"id"` + WorkspaceID pgtype.UUID `json:"workspace_id"` +} + +// Row-locking load used by PATCH so concurrent edits to the same hook serialize: +// the lock holder allocates the next revision and repoints the active pointer +// before the next waiter reads MAX(revision), so idx_hook_revision_unique can +// never be violated by a MAX+1 race (MUL-4332 PR2 review point 4). +func (q *Queries) GetHookForUpdate(ctx context.Context, arg GetHookForUpdateParams) (Hook, error) { + row := q.db.QueryRow(ctx, getHookForUpdate, arg.ID, arg.WorkspaceID) + var i Hook + err := row.Scan( + &i.ID, + &i.WorkspaceID, + &i.Name, + &i.Enabled, + &i.ActiveRevisionID, + &i.ScopeType, + &i.ScopeID, + &i.RetireAfterEventSeq, + &i.Origin, + &i.SystemKey, + &i.SystemVersion, + &i.CreatorActorType, + &i.CreatorActorID, + &i.AuthorizationPrincipalUserID, + &i.DisabledReason, + &i.CreatedAt, + &i.UpdatedAt, + &i.ArchivedAt, + ) + return i, err +} + const getHookInWorkspace = `-- name: GetHookInWorkspace :one SELECT id, workspace_id, name, enabled, active_revision_id, scope_type, scope_id, retire_after_event_seq, origin, system_key, system_version, creator_actor_type, creator_actor_id, authorization_principal_user_id, disabled_reason, created_at, updated_at, archived_at FROM hook WHERE id = $1 AND workspace_id = $2 diff --git a/server/pkg/db/generated/member.sql.go b/server/pkg/db/generated/member.sql.go index ee18f5e15a..322a22b0cf 100644 --- a/server/pkg/db/generated/member.sql.go +++ b/server/pkg/db/generated/member.sql.go @@ -86,6 +86,32 @@ func (q *Queries) GetMemberByUserAndWorkspace(ctx context.Context, arg GetMember return i, err } +const getMemberInWorkspace = `-- name: GetMemberInWorkspace :one +SELECT id, workspace_id, user_id, role, created_at FROM member +WHERE id = $1 AND workspace_id = $2 +` + +type GetMemberInWorkspaceParams struct { + ID pgtype.UUID `json:"id"` + WorkspaceID pgtype.UUID `json:"workspace_id"` +} + +// Workspace-scoped member existence check by member row id. Used to fail-closed +// validate a send_inbox action target belongs to the hook's workspace +// (MUL-4332 PR2 review point 2). +func (q *Queries) GetMemberInWorkspace(ctx context.Context, arg GetMemberInWorkspaceParams) (Member, error) { + row := q.db.QueryRow(ctx, getMemberInWorkspace, arg.ID, arg.WorkspaceID) + var i Member + err := row.Scan( + &i.ID, + &i.WorkspaceID, + &i.UserID, + &i.Role, + &i.CreatedAt, + ) + return i, err +} + const listMembers = `-- name: ListMembers :many SELECT id, workspace_id, user_id, role, created_at FROM member WHERE workspace_id = $1 diff --git a/server/pkg/db/queries/hook.sql b/server/pkg/db/queries/hook.sql index a4497332a4..a9d0f4e26d 100644 --- a/server/pkg/db/queries/hook.sql +++ b/server/pkg/db/queries/hook.sql @@ -21,6 +21,15 @@ RETURNING *; SELECT * FROM hook WHERE id = $1 AND workspace_id = $2; +-- name: GetHookForUpdate :one +-- Row-locking load used by PATCH so concurrent edits to the same hook serialize: +-- the lock holder allocates the next revision and repoints the active pointer +-- before the next waiter reads MAX(revision), so idx_hook_revision_unique can +-- never be violated by a MAX+1 race (MUL-4332 PR2 review point 4). +SELECT * FROM hook +WHERE id = $1 AND workspace_id = $2 +FOR UPDATE; + -- name: ListHooksByWorkspace :many SELECT * FROM hook WHERE workspace_id = $1 AND archived_at IS NULL diff --git a/server/pkg/db/queries/member.sql b/server/pkg/db/queries/member.sql index f93365d3b3..2c4e471eef 100644 --- a/server/pkg/db/queries/member.sql +++ b/server/pkg/db/queries/member.sql @@ -7,6 +7,13 @@ ORDER BY created_at ASC; SELECT * FROM member WHERE id = $1; +-- name: GetMemberInWorkspace :one +-- Workspace-scoped member existence check by member row id. Used to fail-closed +-- validate a send_inbox action target belongs to the hook's workspace +-- (MUL-4332 PR2 review point 2). +SELECT * FROM member +WHERE id = $1 AND workspace_id = $2; + -- name: GetMemberByUserAndWorkspace :one SELECT * FROM member WHERE user_id = $1 AND workspace_id = $2;