From fbe83fc41910b1d1915b5e65ef616b6dd0846057 Mon Sep 17 00:00:00 2001 From: sumit Date: Fri, 11 Sep 2026 10:02:52 +0530 Subject: [PATCH 1/2] fix(security): bind public dashboard queries to their published shapes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit POST /api/public/dashboards/{token}/query took the SQL to run as a caller-supplied body field and never compared it against the dashboard being shared. The only check was validateReadOnlySql, which asks whether a statement reads — not whether this dashboard was published to run it. The endpoint is permitAll via /public/**, so a link created to publish one chart granted anonymous, unauthenticated read of every table on that connection, paginable to completion. Share tokens are 192-bit so this was never brute-forceable; the exposure is to whoever receives or forwards a link, which is exactly the population a share link is meant to be safe for. An exact string match could not be the fix. The agent builds interactive dashboards whose SQL is interpolated at runtime — SKILL.md states there is no placeholder convention — so exact matching would break public links while the author's own view kept working, failing only for the audience. DashboardQueryShapeService instead matches by shape: the statement with its literals replaced by placeholders, via the QueryNormalizer that already backs QueryFingerprintService. A date-range change shares a shape; a different table, column or predicate does not. Shapes are extracted statically from the stored artifact, so there is no capture step and no partly-captured set, and a test pins that the extracted shape equals the shape of the SQL issued at runtime. Unmatched shapes fail closed and the artifact renders its existing per-widget error, so one widget degrades alone. Structure smuggled inside a string literal is refused because the normalizer's '[^']*' rule does not model SQL's '' escape: the payload splits into a different number of placeholders, so the shape changes. The imprecision fails in the safe direction. It remains one layer — the read-only guard, setReadOnly(true), the row cap and the is_public re-check all still apply. Also re-checks hasActivePolicy per query. Enabling a share is refused while a policy is active, but nothing re-checked afterwards, so a link created before a policy was added stayed live and unprotected — "public-share" has no policy row, so resolveEffectivePolicy returns none() and column protections and redaction never ran. Re-checked for the same reason is_public is. Verified: 13 tests fail to compile before the service exists, pass after, and 5 fail when matches() is stubbed to return true. 50 tests green across the related suites, mvn compile clean. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 24 +++ .../controller/PublicDashboardController.java | 28 +++ .../service/DashboardQueryShapeService.java | 105 +++++++++++ .../DashboardQueryShapeServiceTest.java | 166 ++++++++++++++++++ ...26-09-11-public-dashboard-arbitrary-sql.md | 149 ++++++++++++++++ ...1-public-dashboard-query-binding-design.md | 135 ++++++++++++++ 6 files changed, 607 insertions(+) create mode 100644 backend/src/main/java/com/dbaagent/service/DashboardQueryShapeService.java create mode 100644 backend/src/test/java/com/dbaagent/service/DashboardQueryShapeServiceTest.java create mode 100644 docs/security/2026-09-11-public-dashboard-arbitrary-sql.md create mode 100644 docs/superpowers/specs/2026-09-11-public-dashboard-query-binding-design.md diff --git a/CLAUDE.md b/CLAUDE.md index c4ef6ef..22849a9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -261,6 +261,30 @@ Dashboards are **generated by the embedded DeepSQL Agent acting as a coding agen - **Rendering + data access**: `DashboardArtifact.jsx` renders the HTML in a **sandboxed iframe** (`sandbox="allow-scripts"`, opaque origin + a strict CSP — no external network). The artifact fetches data only through an injected `deepsql.query(sql)` bridge that `postMessage`s to the parent; the parent calls **`POST /api/dashboards/query`** (`DashboardQueryController`), which is **read-only twice over** (`McpSqlGuardService.validateReadOnlySql` + `QueryExecutionContext.api` = `READ_ONLY_ONLY`) and access-scoped via `assertCanReadConnectionContent`. So the agent's code has full creative freedom while every query stays guarded and sandboxed. The bridge also auto-sizes the iframe and forwards runtime errors. - Generation endpoints unchanged (`POST /api/dashboards/generate` + `/generate/stream`). `DashboardBuilder.js`/`DashboardInputs.js` remain only because `tabs/Core/PreviewTab.js` still uses them — the dashboard *creation* path no longer touches them. - **Sharing**: both share types render a standalone read-only `DashboardViewer` (title + `DashboardArtifact` with an injected `queryFn`). Internal link `/dashboard-view/:id` (auth) uses the authed broker; public link `/share/dashboard/:token` (permitAll) uses `PublicDashboardController` (`GET /api/public/dashboards/{token}` + `/query`), which resolves only while `saved_dashboards.is_public` is true (revoke = flip it) and runs read-only + connection-scoped. `share_token`/`is_public` are set only via `POST|DELETE /api/saved-dashboards/{id}/share` (access-checked), never a general update. `ShareMenu.jsx` drives the UI. The public query path has its own nginx `dashq` limiter. +- **A public share token is not a licence to run any SQL.** `POST /api/public/dashboards/{token}/query` + took the SQL as a body field and never compared it against the dashboard being shared — only + `validateReadOnlySql`, which asks whether a statement *reads*, not whether this dashboard was + published to run it. A link shared to show one chart therefore granted **anonymous read of every + table on the connection** (`SELECT * FROM users`), paginable to completion. Tokens are 192-bit so + this was never brute-forceable; the exposure is to whoever receives or forwards a link. + `DashboardQueryShapeService` now binds each public query to a **shape** extracted from the + dashboard's own stored artifact: the statement with literals replaced by placeholders, via the + existing `QueryNormalizer`. Shape-matching rather than exact-matching is load-bearing — the agent + builds interactive dashboards whose SQL is interpolated at runtime (`SKILL.md`: "There is no + placeholder convention"), so an exact match would break public links while the author's own view + kept working. Literals vary freely; tables, columns and predicates do not. Extraction is static, + from `dashboard_config`, so there is no capture step and no partly-captured set. **Unmatched + shapes fail closed** and the artifact renders its existing per-widget error, so one widget + degrades alone. Note the normalizer's `'[^']*'` rule does not model SQL's `''` escape, which is + *why* structure smuggled inside a literal is refused: it splits into a different number of + placeholders, so the shape changes. The imprecision fails safe — but it is one layer, not the + only one, and the read-only guard, `setReadOnly(true)` and the row cap all still apply. +- **Revoking a chat-access policy has to reach an already-issued share link.** Enabling a public + share is refused while the connection has an active policy (`SavedDashboardController:81`), but + nothing re-checked afterwards — so a link created *before* a policy was added stayed live and + unprotected, because `"public-share"` has no policy row and `resolveEffectivePolicy` returns + `none()`, meaning column protections and redaction never ran. `PublicDashboardController` now + re-checks `hasActivePolicy` per query, for the same reason it re-checks `is_public`. - **Organization** (search/folders/favorites): `SavedDashboardController`'s search/folder/favorite endpoints existed for a while with no UI consumer. `DashboardsHome.jsx` now wires all of it — a search box (client-side filter over name/description), folder chips derived from `GET /connection/{id}/folders` with a per-card "move to folder" popover (`PUT /saved-dashboards/{id}` with `folder: ""` to clear — `updateDashboard` treats `null` as "field omitted" so blank is the explicit clear signal, same convention as `setSharePassword`), and a favorite star toggle (`POST /{id}/favorite`) with optimistic UI update. - **Clone**: `POST /saved-dashboards/{id}/clone` (`SavedDashboardService.cloneDashboard`) duplicates a dashboard's config/chat/tags/folder into a fresh row — not shared, not favorited. Exposed as a copy icon on each `DashboardsHome.jsx` card. - **Version history**: every real overwrite of `dashboardConfig` (agent build via `completeBuildTurn`, manual Source-tab edit via `updateDashboard`, or a restore) snapshots the *previous* config into `dashboard_versions` (`V113__create_dashboard_versions.sql`) before overwriting, tagged with a trigger (`AGENT_BUILD`/`MANUAL_EDIT`/`RESTORE`) — capped at 50 snapshots per dashboard, oldest pruned first. `GET /{id}/versions` lists them newest-first; `POST /{id}/versions/{versionId}/restore` swaps a snapshot back in as current (itself snapshotting whatever was live, so a restore is undoable too) and **dedupes**: after a restore, the restored row plus any other row with byte-identical `dashboard_config` are deleted, since that content is now "Current," not history — otherwise a restore-edit-restore cycle piles up an alternating chain of duplicate snapshots. `DashboardWorkspace.jsx`'s History panel shows a lightweight diff summary per entry (title/widget-count/size delta computed client-side, not a real line diff — the agent rewrites large chunks even for small logical changes) plus a Preview modal that renders that version's HTML live via `DashboardArtifact`. diff --git a/backend/src/main/java/com/dbaagent/controller/PublicDashboardController.java b/backend/src/main/java/com/dbaagent/controller/PublicDashboardController.java index a481d27..0e7bc8b 100644 --- a/backend/src/main/java/com/dbaagent/controller/PublicDashboardController.java +++ b/backend/src/main/java/com/dbaagent/controller/PublicDashboardController.java @@ -4,6 +4,8 @@ import com.dbaagent.model.QueryRequest; import com.dbaagent.model.QueryResult; import com.dbaagent.model.SavedDashboard; +import com.dbaagent.service.ConnectionChatAccessPolicyService; +import com.dbaagent.service.DashboardQueryShapeService; import com.dbaagent.service.McpSqlGuardService; import com.dbaagent.service.QueryExecutionContext; import com.dbaagent.service.QueryExecutorService; @@ -18,6 +20,7 @@ import java.util.List; import java.util.Map; +import java.util.Set; import java.util.Optional; /** @@ -43,6 +46,8 @@ public class PublicDashboardController { private final SavedDashboardService savedDashboardService; private final ObjectMapper objectMapper; private final McpSqlGuardService sqlGuardService; + private final DashboardQueryShapeService queryShapeService; + private final ConnectionChatAccessPolicyService policyService; private final QueryExecutorService queryExecutorService; private Optional publicDashboard(String token) { @@ -101,6 +106,29 @@ public ResponseEntity query(@PathVariable String token, @RequestBody PublicQu if (!guard.ok()) { return ResponseEntity.badRequest().body(Map.of("success", false, "error", guard.reason())); } + // Read-only is not enough on an anonymous path: it asks whether the statement reads, + // not whether this dashboard was published to run it. Without the shape check below, a + // link shared to show one chart accepted "SELECT * FROM users" just as happily. + // A policy added AFTER the link was shared must take effect on it. Enabling a share is + // refused while a policy is active (SavedDashboardController), but nothing re-checked + // afterwards, so a link created before the policy stayed live and unprotected — + // "public-share" has no policy row, so resolveEffectivePolicy returns none() and + // column protections and redaction never run. Re-checked here for the same reason + // is_public is: revocation has to reach an already-issued link. + if (policyService.hasActivePolicy(found.get().getConnectionId())) { + log.info("Public dashboard query refused for token {}: connection has an active policy", token); + return ResponseEntity.status(HttpStatus.FORBIDDEN).body(Map.of( + "success", false, + "error", "This dashboard is no longer available publicly.")); + } + Set publishedShapes = + queryShapeService.extractShapes(found.get().getDashboardConfig()); + if (!queryShapeService.matches(publishedShapes, request.sql())) { + log.info("Public dashboard query refused for token {}: shape not published", token); + return ResponseEntity.badRequest().body(Map.of( + "success", false, + "error", "This query is not part of the shared dashboard.")); + } int limit = request.limit() == null ? DEFAULT_LIMIT : Math.max(1, Math.min(request.limit(), MAX_LIMIT)); try { QueryRequest qr = new QueryRequest(); diff --git a/backend/src/main/java/com/dbaagent/service/DashboardQueryShapeService.java b/backend/src/main/java/com/dbaagent/service/DashboardQueryShapeService.java new file mode 100644 index 0000000..ca94df4 --- /dev/null +++ b/backend/src/main/java/com/dbaagent/service/DashboardQueryShapeService.java @@ -0,0 +1,105 @@ +package com.dbaagent.service; + +import com.dbaagent.util.QueryNormalizer; +import org.springframework.stereotype.Service; + +import java.util.LinkedHashSet; +import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +/** + * Binds the queries a public dashboard may run to the ones its artifact actually contains. + * + *

{@code POST /api/public/dashboards/{token}/query} takes the SQL as a request body field. + * Checking only that the statement reads is not enough: it answers "is this a select" when the + * question is "is this a query this dashboard was published to run". Without that second check + * a link shared to show one chart grants anonymous read of the whole connection. + * + *

Exact string matching cannot be the answer. Dashboards are interactive by design — a date + * picker re-queries with new bounds on every change ({@code dashboard-design/SKILL.md}), so the + * exact string is not knowable at publish time. Matching would then fail only on the public + * link while the author's own view kept working, which is the worst shape a regression can take. + * + *

So queries are matched by shape: the statement with its literals replaced by + * placeholders, via the same {@link QueryNormalizer} that backs + * {@link QueryFingerprintService}. Two queries differing only in a date range share a shape; + * two naming different tables or columns do not. + * + *

This is one layer, not the only one. {@code validateReadOnlySql}, + * {@code connection.setReadOnly(true)}, the row cap and the {@code is_public} re-check all still + * apply. That matters because {@code QueryNormalizer} was written for analytics grouping, where + * a collision is a cosmetic nuisance rather than a vulnerability. + */ +@Service +public class DashboardQueryShapeService { + + /** + * The first argument of a {@code deepsql.query(...)} call, in each quoting style the agent + * emits — backtick, double and single. Escaped quotes are consumed so a literal containing + * the delimiter does not end the match early. + */ + private static final Pattern QUERY_CALL = Pattern.compile( + "deepsql\\s*\\.\\s*query\\s*\\(\\s*" + + "(`(?:[^`\\\\]|\\\\.)*`" + + "|\"(?:[^\"\\\\]|\\\\.)*\"" + + "|'(?:[^'\\\\]|\\\\.)*')", + Pattern.DOTALL); + + /** + * A JS template interpolation. Replaced with a quoted placeholder before normalizing, so the + * interpolated value is treated as the literal it becomes at runtime: {@code '${from}'} + * already sits inside quotes in the artifact, and a bare {@code ${n}} still has to normalize + * to the same placeholder the runtime's numeric literal produces. + */ + private static final Pattern INTERPOLATION = Pattern.compile("\\$\\{[^}]*\\}"); + + /** Extracts the shape of every query the artifact can issue. */ + public Set extractShapes(String artifactHtml) { + Set shapes = new LinkedHashSet<>(); + if (artifactHtml == null || artifactHtml.isBlank()) { + return shapes; + } + Matcher calls = QUERY_CALL.matcher(artifactHtml); + while (calls.find()) { + String shape = shapeOf(unwrapJsLiteral(calls.group(1))); + if (!shape.isBlank()) { + shapes.add(shape); + } + } + return shapes; + } + + /** The shape of one SQL statement: its literals replaced by placeholders. */ + public String shapeOf(String sql) { + if (sql == null || sql.isBlank()) { + return ""; + } + return QueryNormalizer.normalize(sql); + } + + /** + * Whether {@code sql} matches a published shape. Fails closed: an empty shape set, a blank + * statement, or any shape that was not extracted is refused. + */ + public boolean matches(Set publishedShapes, String sql) { + if (publishedShapes == null || publishedShapes.isEmpty() || sql == null || sql.isBlank()) { + return false; + } + String shape = shapeOf(sql); + return !shape.isBlank() && publishedShapes.contains(shape); + } + + /** + * Strips the surrounding quotes from a JS string literal and collapses interpolations. + * + *

An interpolation becomes {@code '?'} — a quoted placeholder — so that + * {@code BETWEEN '${from}' AND '${to}'} yields the same shape as the runtime statement + * {@code BETWEEN '2026-01-01' AND '2026-03-01'}. The surrounding quotes already present in + * the artifact are left in place and normalized away with it. + */ + private String unwrapJsLiteral(String literal) { + String body = literal.substring(1, literal.length() - 1); + return INTERPOLATION.matcher(body).replaceAll("?"); + } +} diff --git a/backend/src/test/java/com/dbaagent/service/DashboardQueryShapeServiceTest.java b/backend/src/test/java/com/dbaagent/service/DashboardQueryShapeServiceTest.java new file mode 100644 index 0000000..683ff8d --- /dev/null +++ b/backend/src/test/java/com/dbaagent/service/DashboardQueryShapeServiceTest.java @@ -0,0 +1,166 @@ +package com.dbaagent.service; + +import org.junit.jupiter.api.Test; + +import java.util.Set; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * The security contract for public dashboard sharing. + * + *

{@code POST /api/public/dashboards/{token}/query} took the SQL to run as a body field and + * never compared it against the dashboard being shared — only {@code validateReadOnlySql}, + * which asks whether a statement reads, not whether this dashboard was ever meant to run it. + * A link published to show one chart therefore granted anonymous read of every table on the + * connection. + * + *

An exact string match cannot be the fix: the agent writes interactive dashboards whose + * SQL is interpolated at runtime (a date picker changes the string on every use), so exact + * matching would break public links while the author's own view kept working. These tests pin + * the shape-matching behaviour that closes the hole while keeping interactivity: literals vary + * freely, structure does not. + */ +class DashboardQueryShapeServiceTest { + + private final DashboardQueryShapeService service = new DashboardQueryShapeService(); + + private static final String PUBLISHED = + "SELECT COUNT(*) FROM public.properties p WHERE p.created_at BETWEEN '2026-01-01' AND '2026-03-01'"; + + private Set shapesOf(String artifactHtml) { + return service.extractShapes(artifactHtml); + } + + private boolean allows(Set shapes, String sql) { + return service.matches(shapes, sql); + } + + // ── extraction ──────────────────────────────────────────────────────────── + + @Test + void extractsQueriesFromTemplateLiteralsWithInterpolation() { + Set shapes = shapesOf(""" + + """); + + assertThat(shapes).hasSize(1); + assertThat(allows(shapes, PUBLISHED)).isTrue(); + } + + @Test + void extractsQueriesFromEveryQuotingStyleTheAgentEmits() { + Set shapes = shapesOf(""" + + """); + + assertThat(shapes).hasSize(3); + assertThat(allows(shapes, "SELECT a FROM public.t1")).isTrue(); + assertThat(allows(shapes, "SELECT b FROM public.t2")).isTrue(); + assertThat(allows(shapes, "SELECT c FROM public.t3")).isTrue(); + } + + @Test + void artifactWithNoQueriesYieldsNoShapes() { + assertThat(shapesOf("

a static dashboard

")).isEmpty(); + } + + // ── interactivity must survive ──────────────────────────────────────────── + + @Test + void allowsTheSameQueryWithADifferentDateRange() { + Set shapes = Set.of(service.shapeOf(PUBLISHED)); + + assertThat(allows(shapes, + "SELECT COUNT(*) FROM public.properties p WHERE p.created_at BETWEEN '2025-06-01' AND '2025-09-30'")) + .isTrue(); + } + + @Test + void allowsWhitespaceAndCaseVariationsOfThePublishedQuery() { + Set shapes = Set.of(service.shapeOf(PUBLISHED)); + + assertThat(allows(shapes, + "select count(*)\n FROM public.PROPERTIES p\tWHERE p.created_at between '2026-01-01' and '2026-03-01'")) + .isTrue(); + } + + // ── exfiltration must be refused ────────────────────────────────────────── + + @Test + void refusesAnUnrelatedTableScan() { + assertThat(allows(Set.of(service.shapeOf(PUBLISHED)), "SELECT * FROM users")).isFalse(); + } + + @Test + void refusesTheSameShapeAgainstADifferentTable() { + assertThat(allows(Set.of(service.shapeOf(PUBLISHED)), + "SELECT COUNT(*) FROM public.users p WHERE p.created_at BETWEEN '2026-01-01' AND '2026-03-01'")) + .isFalse(); + } + + @Test + void refusesTheSameShapeSelectingADifferentColumn() { + assertThat(allows(Set.of(service.shapeOf(PUBLISHED)), + "SELECT p.password FROM public.properties p WHERE p.created_at BETWEEN '2026-01-01' AND '2026-03-01'")) + .isFalse(); + } + + @Test + void refusesAnAppendedPredicate() { + assertThat(allows(Set.of(service.shapeOf(PUBLISHED)), + PUBLISHED + " OR 1=1")) + .isFalse(); + } + + /** + * The normalizer's {@code '[^']*'} rule does not model SQL's {@code ''} escape, so a payload + * smuggling structure inside a literal splits into a different number of placeholders than + * the published query has. The shape changes and the query is refused — the imprecision + * fails in the safe direction, which is the property worth pinning. + */ + @Test + void refusesStructureSmuggledInsideAStringLiteral() { + assertThat(allows(Set.of(service.shapeOf(PUBLISHED)), + "SELECT COUNT(*) FROM public.properties p WHERE p.created_at " + + "BETWEEN '2026-01-01' AND '2026-03-01 '' UNION SELECT password FROM users --'")) + .isFalse(); + } + + // ── fail closed ─────────────────────────────────────────────────────────── + + @Test + void refusesEveryQueryWhenNoShapesWereExtracted() { + assertThat(allows(Set.of(), PUBLISHED)).isFalse(); + } + + @Test + void refusesBlankAndNullSql() { + Set shapes = Set.of(service.shapeOf(PUBLISHED)); + + assertThat(allows(shapes, null)).isFalse(); + assertThat(allows(shapes, " ")).isFalse(); + } + + /** + * Publish-time extraction and query-time normalization must agree, or every public + * dashboard breaks. This is the seam between the two halves of the feature, so it is + * asserted directly rather than only implied by the tests above. + */ + @Test + void extractedShapeEqualsTheShapeOfTheQueryActuallyIssuedAtRuntime() { + Set extracted = shapesOf( + ""); + + assertThat(extracted).containsExactly(service.shapeOf(PUBLISHED)); + } +} diff --git a/docs/security/2026-09-11-public-dashboard-arbitrary-sql.md b/docs/security/2026-09-11-public-dashboard-arbitrary-sql.md new file mode 100644 index 0000000..2c0488e --- /dev/null +++ b/docs/security/2026-09-11-public-dashboard-arbitrary-sql.md @@ -0,0 +1,149 @@ +# A public share link granted full database read + +*Found 2026-09-10 in a repository-wide security audit. Severity: critical.* + +## What was wrong + +`PublicDashboardController.query` took the SQL to run as a caller-supplied body field: + +```java +public record PublicQueryRequest(String sql, Integer limit) { } +``` + +and never compared it against the dashboard being shared. The only check was +`validateReadOnlySql`, which asks whether a statement *reads* — not whether it is a query +this dashboard was ever published to run. + +The endpoint is `permitAll` (via `/public/**` in `SecurityConfig`), so a link created to +publish one chart granted **anonymous, unauthenticated read of every table on that +connection**, paginable to completion: + +```bash +curl -X POST https://host/api/public/dashboards/$TOKEN/query \ + -H 'Content-Type: application/json' \ + -d '{"sql":"SELECT * FROM information_schema.tables","limit":5000}' + +curl -X POST https://host/api/public/dashboards/$TOKEN/query \ + -H 'Content-Type: application/json' \ + -d '{"sql":"SELECT * FROM users","limit":5000}' +``` + +Share tokens are 24 bytes of `SecureRandom` — 192 bits, so this was never brute-forceable. +The exposure is to whoever receives or forwards a link, which is exactly the population a +share link is supposed to be safe for. The class javadoc called this "the accepted trade-off +of a public BI link", but the trade-off as documented is *view the dashboard*; the +implemented behaviour was *read the whole connection*. + +## Why an exact match could not be the fix + +Public dashboards are interactive by design. `agent/skills/dashboard-design/SKILL.md` +instructs the agent to build date pickers and dropdowns that re-query on change (`:148`), +sharing state via `window.__dateRange`, and states plainly (`:54`): + +> There is **no placeholder convention**. You write normal SQL strings in JS and pass the +> finished string to `deepsql.query`. + +So the exact string is not knowable at publish time — every date-range change produces a new +one. `PublicDashboardPage` also supports kiosk auto-refresh, so queries must stay repeatable. + +Exact matching would therefore break interactive dashboards **only on the public link**: +working for the author, failing for the audience. That is the worst shape a regression can +take, so it was rejected. + +## The fix: match the shape, not the text + +`DashboardQueryShapeService` extracts every query the artifact can issue, normalizes each to +a *shape* — the statement with literals replaced by placeholders, via the existing +`QueryNormalizer` that already backs `QueryFingerprintService` — and requires an incoming +query to match one of them. + +Two queries differing only in a date range share a shape. Two naming different tables or +columns do not. + +| Incoming query | Result | +|---|---| +| Same query, different date range | allowed — interactivity preserved | +| Whitespace, newline and case variations | allowed | +| `SELECT * FROM users` | refused | +| Same shape, different table | refused | +| Same shape, different column | refused | +| Same query with `OR 1=1` appended | refused | +| Escaped-quote `UNION` smuggled inside a literal | refused | + +The last row is worth recording, because it is refused for a non-obvious reason. The payload +`'2026-03-01 '' UNION SELECT password FROM users --'` normalizes to `… between ? and ??` — +**two** placeholders, not one — because the `'[^']*'` rule does not model SQL's `''` escape +and so splits the literal differently than the database would. The shape changes, the +fingerprint misses, the query is refused. **The imprecision fails in the safe direction:** any +attempt to smuggle structure through a literal perturbs the shape. + +### Extraction is static, at share time + +`saved_dashboards.dashboard_config` stores the artifact as one self-contained HTML document +with its queries embedded in ` + """; + + @Test + void extractsSqlAssignedToAVariableBeforeTheCall() { + Set shapes = shapesOf(REAL_ARTIFACT); + + assertThat(shapes).hasSize(1); + assertThat(allows(shapes, + "SELECT COALESCE(SUM(public.orders.total_amount), 0) AS total_sales, COUNT(*) AS order_count " + + "FROM public.orders WHERE public.orders.created_at >= '2026-01-01' " + + "AND public.orders.created_at < ('2026-03-01'::date + INTERVAL '1 day')")) + .isTrue(); + } + + @Test + void resolvesEveryVariableNameTheAgentUses() { + Set shapes = shapesOf(""" + + """); + + assertThat(shapes).hasSize(3); + assertThat(allows(shapes, "SELECT a FROM public.t1")).isTrue(); + assertThat(allows(shapes, "SELECT b FROM public.t2")).isTrue(); + assertThat(allows(shapes, "SELECT c FROM public.t3")).isTrue(); + } + + @Test + void stillRefusesExfiltrationFromAVariableBackedArtifact() { + Set shapes = shapesOf(REAL_ARTIFACT); + + assertThat(allows(shapes, "SELECT * FROM public.customers")).isFalse(); + assertThat(allows(shapes, "SELECT * FROM users")).isFalse(); + } + + /** + * An argument that cannot be resolved to a literal must be reported, not skipped. Skipping + * it would publish a shape set missing one of the dashboard's own queries, which then fails + * closed at runtime — a widget broken for the audience only, with nothing to indicate why. + */ + @Test + void reportsAnArgumentItCannotResolveRatherThanSkippingIt() { + String unresolvable = """ + + """; + + assertThat(service.hasUnresolvableQuery(unresolvable)).isTrue(); + assertThat(service.hasUnresolvableQuery(REAL_ARTIFACT)).isFalse(); + } + + @Test + void anArtifactWithNoQueriesAtAllHasNothingUnresolvable() { + assertThat(service.hasUnresolvableQuery("

static

")).isFalse(); + } + + /** + * Widgets are separate {@code + + + """); + + assertThat(shapes).hasSize(3); + assertThat(allows(shapes, "SELECT COUNT(*) FROM public.orders")).isTrue(); + assertThat(allows(shapes, "SELECT COUNT(*) FROM public.customers")).isTrue(); + assertThat(allows(shapes, "SELECT SUM(amount) FROM public.payments")).isTrue(); + } + + /** + * {@code saved_dashboards.dashboard_config} does not hold raw HTML. It holds the broker's + * envelope — {@code {"version":3,"renderMode":"artifact","title":...,"html":"This was missed by three rounds of green tests because the probe that checked real + * artifacts unescaped the dump by hand first, so the harness was more forgiving than the + * production path. Caught only by calling the real endpoint against the real row. + */ + @Test + void extractsFromTheStoredEnvelopeNotJustRawHtml() { + String stored = """ + {"version":3,"renderMode":"artifact","title":"Sales overview", "html":"\n"} + """; + + Set shapes = shapesOf(stored); + + assertThat(shapes).hasSize(1); + assertThat(allows(shapes, "SELECT COUNT(*) FROM public.orders")).isTrue(); + assertThat(allows(shapes, "SELECT * FROM public.customers")).isFalse(); + } + + @Test + void stillHandlesARawHtmlArtifactWithNoEnvelope() { + assertThat(shapesOf("")) + .hasSize(1); + } + // ── fail closed ─────────────────────────────────────────────────────────── @Test diff --git a/docs/security/2026-09-11-public-dashboard-arbitrary-sql.md b/docs/security/2026-09-11-public-dashboard-arbitrary-sql.md index 2c0488e..0f6b910 100644 --- a/docs/security/2026-09-11-public-dashboard-arbitrary-sql.md +++ b/docs/security/2026-09-11-public-dashboard-arbitrary-sql.md @@ -139,6 +139,39 @@ is deliberately one layer among several. cd backend && mvn test -Dtest=DashboardQueryShapeServiceTest ``` +## What hands-on QA against the real stack found + +Three defects survived a green unit suite and were caught only by running the fix against the +live database and a real browser. All three shared one cause: the fixtures encoded assumptions +about the agent's output instead of its actual output. + +1. **Real artifacts assign the SQL to a variable.** Every call site in this database — 18 of 18, + across the names `query`, `sql`, `trendQuery`, `totalQuery` — does + `const sql = \`...\`; await deepsql.query(sql)`. The first extractor matched only a literal + argument, so it returned an empty set and, failing closed, refused **every** query on **every** + existing public dashboard. Executed against a real 33KB artifact: 8 call sites, 0 shapes. +2. **Widgets are separate scopes that reuse the same variable name.** One dashboard has nine + `