Skip to content

Commit 033abe6

Browse files
notSumit25claude
andcommitted
fix(security): extract query shapes the way real artifacts are actually written
Hands-on QA against the live stack found the previous commit would have refused every query on every existing public dashboard. Three defects, all hidden by a green unit suite whose fixtures encoded assumptions about the agent's output rather than its actual output: 1. Real artifacts assign the SQL to a variable first — 18 of 18 call sites in this database, across the names query, sql, trendQuery and totalQuery, do `const sql = `...`; await deepsql.query(sql)`. The extractor matched only a literal argument, so it returned an empty set and, failing closed, refused everything. Run against a real 33KB artifact: 8 call sites, 0 shapes. 2. Widgets are separate scopes that reuse the same name. One dashboard has nine script blocks, eight declaring their own `const sql`. Resolving into one flat map collapsed them: 8 calls yielded 1 shape, silently dropping seven widgets. Declarations are now resolved per script block. 3. dashboard_config stores the broker's JSON envelope, not raw HTML, so the document arrives with newlines as a literal backslash-n. QueryNormalizer collapses real whitespace, so a published shape kept "customer_count\n from" where the runtime statement has a space and nothing matched. An earlier probe unescaped the dump by hand, making the harness more forgiving than production. Escape sequences are now decoded during extraction. Also adds hasUnresolvableQuery: an argument that cannot be resolved to a literal is reported rather than skipped, since skipping publishes a shape set missing one of the dashboard's own queries — a widget broken for the audience only, with nothing on the authoring side to indicate why. Verified against the live stack, not inferred: 28 of 28 call sites across five real artifacts extract to 28 shapes with none unresolvable; all 10 published shapes replay through the API accepted; the public share page renders all nine widgets with real data in Chrome; clicking "Last 90 days" fires six queries with new date literals, all 200, moving the KPIs from $1,953/6 orders to $144,270/426 orders — the interactivity exact-matching would have broken; and SELECT * FROM public.customers is refused while the unpatched backend still returns email, password_hash and phone. 58 tests green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent fbe83fc commit 033abe6

3 files changed

Lines changed: 349 additions & 28 deletions

File tree

backend/src/main/java/com/dbaagent/service/DashboardQueryShapeService.java

Lines changed: 163 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,9 @@
33
import com.dbaagent.util.QueryNormalizer;
44
import org.springframework.stereotype.Service;
55

6+
import java.util.HashMap;
67
import java.util.LinkedHashSet;
8+
import java.util.Map;
79
import java.util.Set;
810
import java.util.regex.Matcher;
911
import java.util.regex.Pattern;
@@ -13,18 +15,34 @@
1315
*
1416
* <p>{@code POST /api/public/dashboards/{token}/query} takes the SQL as a request body field.
1517
* Checking only that the statement reads is not enough: it answers "is this a select" when the
16-
* question is "is this a query this dashboard was published to run". Without that second check
17-
* a link shared to show one chart grants anonymous read of the whole connection.
18+
* question is "is this a query this dashboard was published to run". Without the shape check
19+
* below, a link shared to show one chart granted anonymous read of the whole connection —
20+
* verified live against a real share token, which returned customer rows including
21+
* {@code email}, {@code password_hash} and {@code phone}.
1822
*
1923
* <p>Exact string matching cannot be the answer. Dashboards are interactive by design — a date
2024
* picker re-queries with new bounds on every change ({@code dashboard-design/SKILL.md}), so the
2125
* exact string is not knowable at publish time. Matching would then fail only on the public
2226
* link while the author's own view kept working, which is the worst shape a regression can take.
2327
*
2428
* <p>So queries are matched by <em>shape</em>: the statement with its literals replaced by
25-
* placeholders, via the same {@link QueryNormalizer} that backs
26-
* {@link QueryFingerprintService}. Two queries differing only in a date range share a shape;
27-
* two naming different tables or columns do not.
29+
* placeholders, via the same {@link QueryNormalizer} that backs {@link QueryFingerprintService}.
30+
* Two queries differing only in a date range share a shape; two naming different tables or
31+
* columns do not.
32+
*
33+
* <p><strong>Real artifacts assign the SQL to a variable first.</strong> An earlier version of
34+
* this class matched only a literal argument to {@code deepsql.query(...)}. Every call site in
35+
* the real dashboards checked — 18 of 18, across the names {@code query}, {@code sql},
36+
* {@code trendQuery} and {@code totalQuery} — instead does:
37+
*
38+
* <pre>{@code
39+
* const sql = `SELECT ... WHERE created_at >= '${esc(from)}'`;
40+
* const { rows } = await deepsql.query(sql);
41+
* }</pre>
42+
*
43+
* So extraction produced an empty set and, failing closed, refused every query on every
44+
* existing public dashboard. Declarations are resolved first and the call's argument is looked
45+
* up among them, which is why this does not key on particular variable names.
2846
*
2947
* <p>This is one layer, not the only one. {@code validateReadOnlySql},
3048
* {@code connection.setReadOnly(true)}, the row cap and the {@code is_public} re-check all still
@@ -34,23 +52,36 @@
3452
@Service
3553
public class DashboardQueryShapeService {
3654

37-
/**
38-
* The first argument of a {@code deepsql.query(...)} call, in each quoting style the agent
39-
* emits — backtick, double and single. Escaped quotes are consumed so a literal containing
40-
* the delimiter does not end the match early.
41-
*/
55+
/** A string literal in any of the three quoting styles the agent emits. */
56+
private static final String LITERAL =
57+
"`(?:[^`\\\\]|\\\\.)*`|\"(?:[^\"\\\\]|\\\\.)*\"|'(?:[^'\\\\]|\\\\.)*'";
58+
59+
/** {@code const|let|var <name> = <literal>} — how every real artifact holds its SQL. */
60+
private static final Pattern DECLARATION = Pattern.compile(
61+
"\\b(?:const|let|var)\\s+([A-Za-z_$][\\w$]*)\\s*=\\s*(" + LITERAL + ")",
62+
Pattern.DOTALL);
63+
64+
/** The argument of a {@code deepsql.query(...)} call: a literal, or an identifier. */
4265
private static final Pattern QUERY_CALL = Pattern.compile(
43-
"deepsql\\s*\\.\\s*query\\s*\\(\\s*"
44-
+ "(`(?:[^`\\\\]|\\\\.)*`"
45-
+ "|\"(?:[^\"\\\\]|\\\\.)*\""
46-
+ "|'(?:[^'\\\\]|\\\\.)*')",
66+
"deepsql\\s*\\.\\s*query\\s*\\(\\s*(" + LITERAL + "|[A-Za-z_$][\\w$]*)\\s*[,)]",
4767
Pattern.DOTALL);
4868

69+
/** Any {@code deepsql.query(} call at all, used to spot arguments neither branch resolved. */
70+
private static final Pattern ANY_QUERY_CALL = Pattern.compile("deepsql\\s*\\.\\s*query\\s*\\(");
71+
72+
/**
73+
* One {@code <script>} block. Each widget is its own block and its own scope: a real
74+
* dashboard here has nine blocks, eight declaring their own {@code const sql = ...} with
75+
* different SQL. Resolving across the whole document collapses those onto one name and
76+
* silently drops seven queries, so declarations are resolved per block.
77+
*/
78+
private static final Pattern SCRIPT_BLOCK = Pattern.compile(
79+
"<script\\b[^>]*>(.*?)</script\\s*>", Pattern.DOTALL | Pattern.CASE_INSENSITIVE);
80+
4981
/**
50-
* A JS template interpolation. Replaced with a quoted placeholder before normalizing, so the
51-
* interpolated value is treated as the literal it becomes at runtime: {@code '${from}'}
52-
* already sits inside quotes in the artifact, and a bare {@code ${n}} still has to normalize
53-
* to the same placeholder the runtime's numeric literal produces.
82+
* A JS template interpolation. Replaced with a placeholder before normalizing, so
83+
* {@code '${esc(from)}'} yields the same shape as the {@code '2026-01-01'} it becomes at
84+
* runtime.
5485
*/
5586
private static final Pattern INTERPOLATION = Pattern.compile("\\$\\{[^}]*\\}");
5687

@@ -60,16 +91,38 @@ public Set<String> extractShapes(String artifactHtml) {
6091
if (artifactHtml == null || artifactHtml.isBlank()) {
6192
return shapes;
6293
}
63-
Matcher calls = QUERY_CALL.matcher(artifactHtml);
64-
while (calls.find()) {
65-
String shape = shapeOf(unwrapJsLiteral(calls.group(1)));
66-
if (!shape.isBlank()) {
67-
shapes.add(shape);
94+
for (String scope : scopes(artifactHtml)) {
95+
Map<String, String> declared = declaredLiterals(scope);
96+
Matcher calls = QUERY_CALL.matcher(scope);
97+
while (calls.find()) {
98+
String sql = resolveArgument(calls.group(1), declared);
99+
if (sql == null) {
100+
continue;
101+
}
102+
String shape = shapeOf(sql);
103+
if (!shape.isBlank()) {
104+
shapes.add(shape);
105+
}
68106
}
69107
}
70108
return shapes;
71109
}
72110

111+
/**
112+
* Whether the artifact issues a query whose SQL this class could not recover — for example
113+
* one built by concatenation or returned from a helper.
114+
*
115+
* <p>Such a call must be reported rather than skipped. Skipping it publishes a shape set
116+
* missing one of the dashboard's own queries, which then fails closed at runtime: a widget
117+
* broken for the audience only, with nothing on the authoring side to indicate why.
118+
*/
119+
public boolean hasUnresolvableQuery(String artifactHtml) {
120+
if (artifactHtml == null || artifactHtml.isBlank()) {
121+
return false;
122+
}
123+
return totalQueryCalls(artifactHtml) > resolvedQueryCalls(artifactHtml);
124+
}
125+
73126
/** The shape of one SQL statement: its literals replaced by placeholders. */
74127
public String shapeOf(String sql) {
75128
if (sql == null || sql.isBlank()) {
@@ -90,16 +143,98 @@ public boolean matches(Set<String> publishedShapes, String sql) {
90143
return !shape.isBlank() && publishedShapes.contains(shape);
91144
}
92145

146+
/**
147+
* The artifact's scopes: each {@code <script>} block, or the whole document when it has
148+
* none, so a call outside a script tag is still seen.
149+
*/
150+
private java.util.List<String> scopes(String artifactHtml) {
151+
java.util.List<String> scopes = new java.util.ArrayList<>();
152+
Matcher blocks = SCRIPT_BLOCK.matcher(artifactHtml);
153+
while (blocks.find()) {
154+
scopes.add(blocks.group(1));
155+
}
156+
if (scopes.isEmpty()) {
157+
scopes.add(artifactHtml);
158+
}
159+
return scopes;
160+
}
161+
162+
private Map<String, String> declaredLiterals(String artifactHtml) {
163+
Map<String, String> declared = new HashMap<>();
164+
Matcher declarations = DECLARATION.matcher(artifactHtml);
165+
while (declarations.find()) {
166+
declared.put(declarations.group(1), unwrapJsLiteral(declarations.group(2)));
167+
}
168+
return declared;
169+
}
170+
171+
/** A literal argument is used directly; an identifier is looked up among the declarations. */
172+
private String resolveArgument(String argument, Map<String, String> declared) {
173+
if (isLiteral(argument)) {
174+
return unwrapJsLiteral(argument);
175+
}
176+
return declared.get(argument);
177+
}
178+
179+
private boolean isLiteral(String argument) {
180+
if (argument == null || argument.length() < 2) {
181+
return false;
182+
}
183+
char first = argument.charAt(0);
184+
return first == '`' || first == '"' || first == '\'';
185+
}
186+
187+
private int totalQueryCalls(String artifactHtml) {
188+
return (int) ANY_QUERY_CALL.matcher(artifactHtml).results().count();
189+
}
190+
191+
private int resolvedQueryCalls(String artifactHtml) {
192+
int resolved = 0;
193+
for (String scope : scopes(artifactHtml)) {
194+
Map<String, String> declared = declaredLiterals(scope);
195+
Matcher calls = QUERY_CALL.matcher(scope);
196+
while (calls.find()) {
197+
if (resolveArgument(calls.group(1), declared) != null) {
198+
resolved++;
199+
}
200+
}
201+
}
202+
return resolved;
203+
}
204+
93205
/**
94206
* Strips the surrounding quotes from a JS string literal and collapses interpolations.
95207
*
96-
* <p>An interpolation becomes {@code '?'} — a quoted placeholder — so that
97-
* {@code BETWEEN '${from}' AND '${to}'} yields the same shape as the runtime statement
98-
* {@code BETWEEN '2026-01-01' AND '2026-03-01'}. The surrounding quotes already present in
99-
* the artifact are left in place and normalized away with it.
208+
* <p>An interpolation becomes {@code ?} so that {@code >= '${esc(from)}'} yields the same
209+
* shape as the runtime statement {@code >= '2026-01-01'}: the quotes around it are already
210+
* in the artifact, and the normalizer turns the quoted placeholder into its own {@code ?}.
100211
*/
101212
private String unwrapJsLiteral(String literal) {
102213
String body = literal.substring(1, literal.length() - 1);
103-
return INTERPOLATION.matcher(body).replaceAll("?");
214+
return unescape(INTERPOLATION.matcher(body).replaceAll("?"));
215+
}
216+
217+
/**
218+
* Turns escape sequences into the characters they stand for.
219+
*
220+
* <p>{@code dashboard_config} stores the broker's JSON envelope, so the artifact arrives
221+
* with its newlines as a literal backslash-n and its quotes escaped. {@link QueryNormalizer}
222+
* collapses <em>real</em> whitespace, so without this the published shape keeps
223+
* {@code customer_count\n from} where the runtime statement has a space, and no query on
224+
* the dashboard ever matches.
225+
*
226+
* <p>Worth recording how this was found: an earlier probe unescaped the database dump by
227+
* hand before extracting, so the harness was more forgiving than the production path and
228+
* three rounds of green tests missed it. It surfaced only by calling the real endpoint
229+
* against the real stored row.
230+
*/
231+
private String unescape(String text) {
232+
return text.replace("\\n", "\n")
233+
.replace("\\r", "\r")
234+
.replace("\\t", "\t")
235+
.replace("\\\"", "\"")
236+
.replace("\\'", "'")
237+
.replace("\\`", "`")
238+
.replace("\\\\", "\\");
104239
}
105240
}

0 commit comments

Comments
 (0)