Files
multica/server/pkg/db/queries/skill.sql
Tom Qiao 1c91c2a3b2 security(db): scope DELETE/UpdateIssueStatus by workspace_id (defense-in-depth) (#3027)
* fix(security): scope DELETE/UpdateIssueStatus by workspace_id

Add workspace_id to the WHERE clause of DeleteIssue, DeleteComment,
DeleteProject, DeleteSkill, DeleteChatSession, and UpdateIssueStatus
as SQL-layer defense-in-depth.

Handler loaders (loadIssueForUser / loadSkillForUser / etc.) already
enforce workspace membership today, so this is not patching a known
live vuln. But the tenant invariant is currently a handler-layer
guarantee — a future loader bypass or a new caller skipping the
loader would be silently catastrophic. Making workspace_id part of
the SQL identity collapses the trust surface to the schema itself:
forging a sibling-workspace UUID becomes ErrNoRows instead of a
cross-tenant write.

Reference: incident #1661 (util.ParseUUID silent zero UUID returning
204 on a DELETE that matched zero rows) — same class of failure,
prevented at a different layer.

Scope:
- 5 DELETE queries: issue, comment, project, skill, chat_session
- 1 simple UPDATE: UpdateIssueStatus (2 narg, no SET ordering risk)
- All callers updated (handlers, service, runtime sweeper fallback)

Multi-narg UPDATE queries (UpdateIssue, UpdateProject, UpdateSkill,
UpdateComment, UpdateChatSession*) are deferred to a follow-up to
keep this change reviewable: each needs its narg pinning shifted
and per-caller verification.

sqlc was regenerated by hand (no local sqlc toolchain); CI's
backend job is the authoritative compile check.

* test(security): add workspace_scope_guard regression test

Locks in the SQL-layer tenant guard added in this PR. For each of the 6
scoped queries (DeleteIssue, DeleteComment, DeleteProject, DeleteSkill,
DeleteChatSession, UpdateIssueStatus), creates the resource in workspace
A, invokes the query with a foreign workspace UUID, and asserts the row
is untouched (0 rows affected with no error for :exec; pgx.ErrNoRows for
:one). A future refactor that drops the workspace_id arg from any of
these queries will now fail loudly instead of silently regressing.

Includes a sanity sub-test that the in-workspace path still mutates, so
a buggy guard that returns no-op for every call would not pass.

Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>

---------

Co-authored-by: Tom Qiao <tomqiaozc@users.noreply.github.com>
Co-authored-by: Claude Opus 4 <noreply@anthropic.com>
2026-05-22 14:39:47 +08:00

113 lines
3.3 KiB
SQL

-- Skill CRUD
-- name: ListSkillsByWorkspace :many
SELECT * FROM skill
WHERE workspace_id = $1
ORDER BY name ASC;
-- name: ListSkillSummariesByWorkspace :many
-- Same as ListSkillsByWorkspace but omits the SKILL.md `content` column. Used
-- by list endpoints (CLI table, web list page) where the body is never read;
-- shipping it everywhere blew up payload size on workspaces with many skills
-- and caused 15s CLI timeouts from high-latency regions (GH multica-ai/multica#2174).
SELECT id, workspace_id, name, description, config, created_by, created_at, updated_at
FROM skill
WHERE workspace_id = $1
ORDER BY name ASC;
-- name: GetSkill :one
SELECT * FROM skill
WHERE id = $1;
-- name: GetSkillInWorkspace :one
SELECT * FROM skill
WHERE id = $1 AND workspace_id = $2;
-- name: GetSkillByWorkspaceAndName :one
-- Used by agent-template materialization to implement find-or-create: when a
-- template references a skill by name that already exists in the workspace,
-- reuse the existing skill_id rather than INSERT (which would fail the
-- UNIQUE(workspace_id, name) constraint from migration 008).
SELECT * FROM skill
WHERE workspace_id = $1 AND name = $2;
-- name: CreateSkill :one
INSERT INTO skill (workspace_id, name, description, content, config, created_by)
VALUES ($1, $2, $3, $4, $5, $6)
RETURNING *;
-- name: UpdateSkill :one
UPDATE skill SET
name = COALESCE(sqlc.narg('name'), name),
description = COALESCE(sqlc.narg('description'), description),
content = COALESCE(sqlc.narg('content'), content),
config = COALESCE(sqlc.narg('config'), config),
updated_at = now()
WHERE id = $1
RETURNING *;
-- name: DeleteSkill :exec
-- Defense-in-depth: workspace_id is a SQL-layer tenant guard. See DeleteIssue.
DELETE FROM skill WHERE id = $1 AND workspace_id = $2;
-- Skill File CRUD
-- name: ListSkillFiles :many
SELECT * FROM skill_file
WHERE skill_id = $1
ORDER BY path ASC;
-- name: GetSkillFile :one
SELECT * FROM skill_file
WHERE id = $1;
-- name: UpsertSkillFile :one
INSERT INTO skill_file (skill_id, path, content)
VALUES ($1, $2, $3)
ON CONFLICT (skill_id, path) DO UPDATE SET
content = EXCLUDED.content,
updated_at = now()
RETURNING *;
-- name: DeleteSkillFile :exec
DELETE FROM skill_file WHERE id = $1;
-- name: DeleteSkillFilesBySkill :exec
DELETE FROM skill_file WHERE skill_id = $1;
-- Agent-Skill junction
-- name: ListAgentSkills :many
SELECT s.* FROM skill s
JOIN agent_skill ask ON ask.skill_id = s.id
WHERE ask.agent_id = $1
ORDER BY s.name ASC;
-- name: ListAgentSkillSummaries :many
-- Summary variant for the agent skills list endpoint — omits `content` for
-- the same reason as ListSkillSummariesByWorkspace.
SELECT s.id, s.workspace_id, s.name, s.description, s.config, s.created_by, s.created_at, s.updated_at
FROM skill s
JOIN agent_skill ask ON ask.skill_id = s.id
WHERE ask.agent_id = $1
ORDER BY s.name ASC;
-- name: AddAgentSkill :exec
INSERT INTO agent_skill (agent_id, skill_id)
VALUES ($1, $2)
ON CONFLICT DO NOTHING;
-- name: RemoveAgentSkill :exec
DELETE FROM agent_skill
WHERE agent_id = $1 AND skill_id = $2;
-- name: RemoveAllAgentSkills :exec
DELETE FROM agent_skill WHERE agent_id = $1;
-- name: ListAgentSkillsByWorkspace :many
SELECT ask.agent_id, s.id, s.name, s.description
FROM agent_skill ask
JOIN skill s ON s.id = ask.skill_id
WHERE s.workspace_id = $1
ORDER BY s.name ASC;