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..b11f9d6 --- /dev/null +++ b/backend/src/main/java/com/dbaagent/service/DashboardQueryShapeService.java @@ -0,0 +1,240 @@ +package com.dbaagent.service; + +import com.dbaagent.util.QueryNormalizer; +import org.springframework.stereotype.Service; + +import java.util.HashMap; +import java.util.LinkedHashSet; +import java.util.Map; +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 the shape check + * below, a link shared to show one chart granted anonymous read of the whole connection — + * verified live against a real share token, which returned customer rows including + * {@code email}, {@code password_hash} and {@code phone}. + * + *

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. + * + *

Real artifacts assign the SQL to a variable first. An earlier version of + * this class matched only a literal argument to {@code deepsql.query(...)}. Every call site in + * the real dashboards checked — 18 of 18, across the names {@code query}, {@code sql}, + * {@code trendQuery} and {@code totalQuery} — instead does: + * + * 

{@code
+ *   const sql = `SELECT ... WHERE created_at >= '${esc(from)}'`;
+ *   const { rows } = await deepsql.query(sql);
+ * }
+ * + * So extraction produced an empty set and, failing closed, refused every query on every + * existing public dashboard. Declarations are resolved first and the call's argument is looked + * up among them, which is why this does not key on particular variable names. + * + *

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 { + + /** A string literal in any of the three quoting styles the agent emits. */ + private static final String LITERAL = + "`(?:[^`\\\\]|\\\\.)*`|\"(?:[^\"\\\\]|\\\\.)*\"|'(?:[^'\\\\]|\\\\.)*'"; + + /** {@code const|let|var = } — how every real artifact holds its SQL. */ + private static final Pattern DECLARATION = Pattern.compile( + "\\b(?:const|let|var)\\s+([A-Za-z_$][\\w$]*)\\s*=\\s*(" + LITERAL + ")", + Pattern.DOTALL); + + /** The argument of a {@code deepsql.query(...)} call: a literal, or an identifier. */ + private static final Pattern QUERY_CALL = Pattern.compile( + "deepsql\\s*\\.\\s*query\\s*\\(\\s*(" + LITERAL + "|[A-Za-z_$][\\w$]*)\\s*[,)]", + Pattern.DOTALL); + + /** Any {@code deepsql.query(} call at all, used to spot arguments neither branch resolved. */ + private static final Pattern ANY_QUERY_CALL = Pattern.compile("deepsql\\s*\\.\\s*query\\s*\\("); + + /** + * One {@code + """); + + 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(); + } + + // ── real artifacts assign the SQL to a variable first ──────────────────── + + /** + * Every {@code deepsql.query} call site in this database's real dashboards passes a + * variable, never a literal — 18 of 18 when this was checked, across four + * different names ({@code query}, {@code sql}, {@code trendQuery}, {@code totalQuery}). + * The first version of this service matched only a literal argument, so it extracted + * nothing from a real artifact and, failing closed on an empty set, refused every query + * on every existing public dashboard. This fixture is copied from a stored + * {@code dashboard_config} rather than written by hand, which is what the original tests + * got wrong: they encoded the author's assumption about the agent's output instead of its + * actual output. + */ + private static final String REAL_ARTIFACT = """ + + """; + + @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 + 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..0f6b910 --- /dev/null +++ b/docs/security/2026-09-11-public-dashboard-arbitrary-sql.md @@ -0,0 +1,182 @@ +# 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 `