From 878f0d34e0c87253168d5eaa490ab62bcc6df39a Mon Sep 17 00:00:00 2001 From: laithalenooz Date: Wed, 7 Oct 2026 16:19:43 +0300 Subject: [PATCH] fix(presets): make the burndown and burnup presets pass the query guard (#11) The "Burndown (active sprint)" and "Burnup (active sprint)" presets in the frontend, and the burndown preset in the paca-dashboard-builder skill, used UNION ALL and repeated {{project_id}} inside nested subqueries. The guard rejects both (compound SELECTs are forbidden because a second branch escapes the project filter; the placeholder must appear once, as a top-level equality), so these presets failed the moment anyone used them. They now select the most recently started active sprint in the outer query and produce one row per day from its start date to today with generate_series, counting scope and completed tasks with correlated subqueries. The skill's safety rules now state the single top-level placeholder rule, the compound-SELECT ban and the subquery restriction for tables with sensitive columns. backend/presets_test.go runs every preset shipped in presets.ts and in the skill through validateQuery, so an invalid preset fails the build. Fixes #11 --- backend/presets_test.go | 62 ++++++++++++++++++++++++++ frontend/src/presets.ts | 62 +++++++++++--------------- skills/paca-dashboard-builder/SKILL.md | 31 ++++++------- 3 files changed, 100 insertions(+), 55 deletions(-) create mode 100644 backend/presets_test.go diff --git a/backend/presets_test.go b/backend/presets_test.go new file mode 100644 index 0000000..2dce246 --- /dev/null +++ b/backend/presets_test.go @@ -0,0 +1,62 @@ +package main + +import ( + "os" + "regexp" + "strings" + "testing" +) + +// Every SQL preset shipped to users (the frontend's "insert a preset" list) and +// to agents (the paca-dashboard-builder skill) must pass the same guard a saved +// panel goes through, or it fails the moment someone uses it. + +var ( + presetBlock = regexp.MustCompile("(?s)\\{\\s*id: \"([^\"]+)\".*?scopes: \\[([^\\]]*)\\].*?query: `(.*?)`") + presetConst = regexp.MustCompile("(?s)const ([A-Z_]+) = `(.*?)`;") + skillSQLBlock = regexp.MustCompile("(?s)```sql\\n(.*?)```") + placeholderUsed = "{{project_id}}" +) + +func TestFrontendPresetsPassTheQueryGuard(t *testing.T) { + src, err := os.ReadFile("../frontend/src/presets.ts") + if err != nil { + t.Fatal(err) + } + text := string(src) + for _, m := range presetConst.FindAllStringSubmatch(text, -1) { + text = strings.ReplaceAll(text, "${"+m[1]+"}", m[2]) + } + presets := presetBlock.FindAllStringSubmatch(text, -1) + if len(presets) == 0 { + t.Fatal("found no presets in presets.ts; the parser is out of date") + } + for _, m := range presets { + id, scopes, query := m[1], m[2], m[3] + projectScoped := strings.Contains(scopes, "project") || strings.Contains(scopes, "integration") + if _, err := validateQuery(query, projectScoped); err != nil { + t.Errorf("preset %q is rejected by the query guard: %v", id, err) + } + } +} + +func TestSkillPresetsPassTheQueryGuard(t *testing.T) { + src, err := os.ReadFile("../skills/paca-dashboard-builder/SKILL.md") + if err != nil { + t.Fatal(err) + } + blocks := skillSQLBlock.FindAllStringSubmatch(string(src), -1) + if len(blocks) == 0 { + t.Fatal("found no ```sql blocks in SKILL.md; the parser is out of date") + } + for _, m := range blocks { + query := strings.TrimSpace(m[1]) + // Admin presets are the ones written without the placeholder; the guard + // rejects the placeholder in admin scope and requires it in project scope. + projectScoped := strings.Contains(query, placeholderUsed) + if _, err := validateQuery(query, projectScoped); err != nil { + first := strings.SplitN(query, "\n", 2)[0] + t.Errorf("skill preset starting %q is rejected by the query guard: %v", first, err) + } + } +} diff --git a/frontend/src/presets.ts b/frontend/src/presets.ts index 92d32eb..ee3eff7 100644 --- a/frontend/src/presets.ts +++ b/frontend/src/presets.ts @@ -7,14 +7,14 @@ * forbidden keywords) — see the dev notes in that file before adding more. * * Burndown/burnup scoping: rather than making the user hand-edit a sprint - * UUID into the query text, both sprint-based presets resolve "the active - * sprint" via a correlated subquery on sprints.status = 'active'. If a - * project runs multiple concurrent active sprints (schema allows it, see - * database-schema.md), the subquery's own filter still yields a single - * chart per query invocation — the query returns one row per day across - * whichever active sprint(s) match; if that's ever ambiguous for a given - * project, the user can copy the preset text and hand-tune the WHERE - * clause (e.g. add `AND sprint_id = ''` for a specific sprint). + * UUID into the query text, both sprint-based presets pick the project's most + * recently started active sprint and produce one row per day from its start + * date to today (generate_series), counting tasks with correlated subqueries. + * That shape is forced by the guard: compound SELECTs (UNION/INTERSECT/EXCEPT) + * are forbidden, and {{project_id}} must appear exactly once, as a top-level + * equality filter — so the sprint is selected in the outer query, not in a + * nested one. To chart a specific sprint instead, replace the start_date + * condition with `AND s.id = ''`. */ import type { ChartType, DashboardScopeKind, PanelType } from "./types"; @@ -30,7 +30,15 @@ export interface QueryPreset { query: string; } -const ACTIVE_SPRINT = `(SELECT id FROM sprints WHERE project_id = {{project_id}} AND status = 'active' ORDER BY start_date DESC LIMIT 1)`; +// One row per day of the project's most recently started active sprint. +const ACTIVE_SPRINT_DAYS = `FROM sprints s +CROSS JOIN generate_series(s.start_date::date, CURRENT_DATE, INTERVAL '1 day') AS d +WHERE s.project_id = {{project_id}} AND s.status = 'active' + AND s.start_date = (SELECT MAX(s2.start_date) FROM sprints s2 WHERE s2.project_id = s.project_id AND s2.status = 'active') +ORDER BY d`; +const SCOPE_BY_DAY = `(SELECT COUNT(*) FROM tasks t WHERE t.sprint_id = s.id AND t.deleted_at IS NULL AND t.created_at::date <= d::date)`; +const DONE_BY_DAY = `(SELECT COUNT(*) FROM tasks t JOIN task_statuses ts ON ts.id = t.status_id + WHERE t.sprint_id = s.id AND ts.category = 'done' AND t.deleted_at IS NULL AND t.updated_at::date <= d::date)`; export const QUERY_PRESETS: QueryPreset[] = [ { @@ -40,19 +48,10 @@ export const QUERY_PRESETS: QueryPreset[] = [ panelType: "chart", chartType: "line", scopes: ["project", "integration"], - query: `SELECT to_char(day, 'YYYY-MM-DD') AS day, SUM(delta) OVER (ORDER BY day::date) AS remaining_tasks -FROM ( - SELECT created_at::date AS day, 1 AS delta - FROM tasks - WHERE project_id = {{project_id}} AND sprint_id = ${ACTIVE_SPRINT} AND deleted_at IS NULL - UNION ALL - SELECT t.updated_at::date AS day, -1 AS delta - FROM tasks t - JOIN task_statuses ts ON ts.id = t.status_id - WHERE t.project_id = {{project_id}} AND t.sprint_id = ${ACTIVE_SPRINT} - AND ts.category = 'done' AND t.deleted_at IS NULL -) events -ORDER BY day`, + query: `SELECT to_char(d::date, 'YYYY-MM-DD') AS day, + ${SCOPE_BY_DAY} + - ${DONE_BY_DAY} AS remaining_tasks +${ACTIVE_SPRINT_DAYS}`, }, { id: "burnup-active-sprint", @@ -61,21 +60,10 @@ ORDER BY day`, panelType: "chart", chartType: "line", scopes: ["project", "integration"], - query: `SELECT to_char(day, 'YYYY-MM-DD') AS day, - SUM(scope_delta) OVER (ORDER BY day::date) AS total_scope, - SUM(done_delta) OVER (ORDER BY day::date) AS completed -FROM ( - SELECT created_at::date AS day, 1 AS scope_delta, 0 AS done_delta - FROM tasks - WHERE project_id = {{project_id}} AND sprint_id = ${ACTIVE_SPRINT} AND deleted_at IS NULL - UNION ALL - SELECT t.updated_at::date AS day, 0 AS scope_delta, 1 AS done_delta - FROM tasks t - JOIN task_statuses ts ON ts.id = t.status_id - WHERE t.project_id = {{project_id}} AND t.sprint_id = ${ACTIVE_SPRINT} - AND ts.category = 'done' AND t.deleted_at IS NULL -) events -ORDER BY day`, + query: `SELECT to_char(d::date, 'YYYY-MM-DD') AS day, + ${SCOPE_BY_DAY} AS total_scope, + ${DONE_BY_DAY} AS completed +${ACTIVE_SPRINT_DAYS}`, }, { id: "tasks-by-status", diff --git a/skills/paca-dashboard-builder/SKILL.md b/skills/paca-dashboard-builder/SKILL.md index 764b455..c4c6cd9 100644 --- a/skills/paca-dashboard-builder/SKILL.md +++ b/skills/paca-dashboard-builder/SKILL.md @@ -58,7 +58,9 @@ Every panel tool needs a `viewId`. Get one first: - Exactly one statement, and it must start with `SELECT` or `WITH`. No `INSERT`/`UPDATE`/`DELETE`/`DROP`/`ALTER`/`CREATE`/`GRANT`/`TRUNCATE`/`COPY`/`CALL`/`EXPLAIN`/`VACUUM`, and nothing after a trailing `;`. - No `information_schema`/`pg_catalog`/`pg_*` introspection, `dblink`, `lo_*`. -- **Project and integration-scope queries must contain the literal placeholder `{{project_id}}`** somewhere in a `WHERE`/`ON`/`HAVING` clause (e.g. `WHERE t.project_id = {{project_id}}`) — this is what scopes the query to the caller's own project; the guard substitutes it with a bound `$1` server-side. Never write `$1` yourself — it's reserved and will be rejected. +- **Project and integration-scope queries must contain the literal placeholder `{{project_id}}` exactly once, as a top-level equality filter against a `project_id` column** (e.g. `WHERE t.project_id = {{project_id}}`), never inside a subquery and never next to a bare `OR` — this is what scopes the query to the caller's own project; the guard substitutes it with a bound `$1` server-side. Select per-project rows in the outer query and keep subqueries correlated to it instead of repeating the placeholder. Never write `$1` yourself — it's reserved and will be rejected. +- No compound SELECTs: `UNION`, `INTERSECT` and `EXCEPT` are rejected (a second branch would escape the project filter). Build time series with `generate_series` and correlated `COUNT(*)` subqueries instead — see the burndown preset. +- A query that reads a table with sensitive columns (e.g. `agents`, `users`) may use subqueries only as `IN`/`EXISTS`/`ANY`/`ALL`/`SOME` operands; join those tables at the top level instead. - **Admin-scope queries must NOT contain `{{project_id}}`** — they're intentionally cross-project. - A `LIMIT 500` is appended automatically if the query doesn't already declare one — don't rely on getting more than 500 rows back. - This is a pattern-based guard, not a full SQL parser: prefer straightforward SELECTs over cleverness that might trip a keyword match. @@ -147,24 +149,17 @@ GROUP BY s.name, s.start_date ORDER BY s.start_date ``` -**Burndown (active sprint)** (line chart) — remaining open tasks per day: +**Burndown (active sprint)** (line chart) — remaining open tasks per day of the most recently started active sprint: ```sql -SELECT to_char(day, 'YYYY-MM-DD') AS day, SUM(delta) OVER (ORDER BY day::date) AS remaining_tasks -FROM ( - SELECT created_at::date AS day, 1 AS delta - FROM tasks - WHERE project_id = {{project_id}} - AND sprint_id = (SELECT id FROM sprints WHERE project_id = {{project_id}} AND status = 'active' ORDER BY start_date DESC LIMIT 1) - AND deleted_at IS NULL - UNION ALL - SELECT t.updated_at::date AS day, -1 AS delta - FROM tasks t - JOIN task_statuses ts ON ts.id = t.status_id - WHERE t.project_id = {{project_id}} - AND t.sprint_id = (SELECT id FROM sprints WHERE project_id = {{project_id}} AND status = 'active' ORDER BY start_date DESC LIMIT 1) - AND ts.category = 'done' AND t.deleted_at IS NULL -) events -ORDER BY day +SELECT to_char(d::date, 'YYYY-MM-DD') AS day, + (SELECT COUNT(*) FROM tasks t WHERE t.sprint_id = s.id AND t.deleted_at IS NULL AND t.created_at::date <= d::date) + - (SELECT COUNT(*) FROM tasks t JOIN task_statuses ts ON ts.id = t.status_id + WHERE t.sprint_id = s.id AND ts.category = 'done' AND t.deleted_at IS NULL AND t.updated_at::date <= d::date) AS remaining_tasks +FROM sprints s +CROSS JOIN generate_series(s.start_date::date, CURRENT_DATE, INTERVAL '1 day') AS d +WHERE s.project_id = {{project_id}} AND s.status = 'active' + AND s.start_date = (SELECT MAX(s2.start_date) FROM sprints s2 WHERE s2.project_id = s.project_id AND s2.status = 'active') +ORDER BY d ``` **Admin: tasks by project** (bar chart, instance-wide):