From 1294256cabdff83f7a147711e076a00ea5dd819b Mon Sep 17 00:00:00 2001 From: sumit Date: Sat, 22 Aug 2026 13:04:16 +0530 Subject: [PATCH] fix(editor): bound CSV export, guard concurrent runs, audit query cancels MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four defects found in a production-readiness review of the Editor tab, all verified live against the running stack (real browser, real API, real DB) rather than by reading code. **CSV export was unbounded and outran the proxy.** `handleExportResults` re-ran the query with `limit: null` and `timeoutSeconds: 600`. With no limit, `QueryExecutorService` never calls `setMaxRows` and never appends a LIMIT, so every row accumulates into an unbounded ArrayList, is serialized to JSON, then held again in the browser as an array + CSV string + Blob. Observed live: a 50,000-row table returned a 7.1MB JSON payload in one response. The blast radius is the whole instance, not the exporter — a large enough export is an OOM for every tenant on that backend. The 600s timeout also violated the rule already documented for this file: `docker/nginx/default.conf` gives up at `proxy_read_timeout 300s`, so a long export returned an opaque 504 while the query kept running, and it sent no executionId, making it uncancellable by any means. Export now uses EXPORT_ROW_LIMIT (100k), the same 240s budget as Run, and carries an executionId + abort signal. The button label no longer promises "all N rows" when it will return at most 100k. **Cmd+Enter had no in-flight guard.** The Run button correctly swaps to Stop during a run, but the keyboard shortcut bypassed that and called `handleRunQuery` unconditionally. Both runs shared one `abortControllerRef` / `executionIdRef` slot, so a second run discarded the first's cancel handle, and whichever response landed last won the results panel — a user could read rows from a query they'd already replaced, on the same screen where they decide what to UPDATE or DELETE. Adds `isRunningRef` (re-entry guard) and `runSeqRef` (per- run stamp) so a superseded response is dropped and only the current run clears the shared refs. `handleStopQuery` releases the guard so Stop-then-rerun still works. Verified: 14 rapid-fire attempts across one 5s query window produce exactly 1 request. `handleExportResults` gets the same guard. It is currently unreachable during a run — `handleRunQuery` clears `results`, which hides the Export button — but that is incidental to unrelated state handling, not a guarantee, so the check is explicit rather than load-bearing on a side effect. **containsWhereClause matched the literal string " WHERE ".** A newline before WHERE (any multi-line formatted statement) read as *no* WHERE clause, while a commented-out `-- WHERE ...` satisfied the guard. Now word-bounded and comment-stripped. Only reachable via the keyword-fallback path: JSqlParser 5.2 parses ordinary multi-line DELETE/UPDATE fine and those use `Delete.getWhere()` / `Update.getWhere()`, but it cannot parse `EXPLAIN UPDATE`/`EXPLAIN DELETE` at all, so those fall through to this check. The two new tests cover exactly that path and were confirmed to fail before the fix and pass after. **Cancelling a query wrote no deliberate audit event.** Every execute outcome is audited; cancel was not. The only trace of a successful cancel was the killed query's own thread logging `pg_terminate_backend`'s error as an ordinary EDITOR_QUERY_FAILED — indistinguishable from any other failure, and dependent on a Postgres-specific error string. A cancel that missed its target (already finished, or a guessed/replayed executionId) left nothing at all. Adds EDITOR_QUERY_CANCELLED (SUCCESS with the terminated pid / INFO for a no-op) plus an executionId in the audit metadata, and audits the authorization rejection that was previously a silent 404. Verified end-to-end on the running stack: cross-user cancel returns 404, is audited as EDITOR_QUERY_BLOCKED against the *caller's* user id, and the target query provably survives (ran its full 14019ms and returned success). Mutation confirm flow, non-admin mutation block, and DELETE ... WHERE all re-checked for regressions with before/after DB row counts. Backend suite: 58/58 green. Not addressed here: the client-side `hasMultipleStatementsWithoutSemicolons` guard rejects valid multi-line SQL — including the output of the Editor's own Format button — before any request is sent. Left for a separate change. Co-Authored-By: Claude Opus 5 (1M context) --- .../dbaagent/controller/SchemaController.java | 30 +++++++- .../com/dbaagent/model/SecurityEventType.java | 1 + .../service/QueryExecutionPolicyService.java | 13 +++- .../service/SqlExecutionAuditService.java | 32 +++++++++ .../QueryExecutionPolicyServiceTest.java | 36 ++++++++++ .../service/SqlExecutionAuditServiceTest.java | 40 +++++++++++ src/components/tabs/Core/SqlRunnerTab.js | 68 +++++++++++++++---- 7 files changed, 205 insertions(+), 15 deletions(-) diff --git a/backend/src/main/java/com/dbaagent/controller/SchemaController.java b/backend/src/main/java/com/dbaagent/controller/SchemaController.java index 6f37ab9..12abb24 100644 --- a/backend/src/main/java/com/dbaagent/controller/SchemaController.java +++ b/backend/src/main/java/com/dbaagent/controller/SchemaController.java @@ -275,8 +275,10 @@ public ResponseEntity> executeQuery( @PostMapping("/query/{executionId}/cancel") public ResponseEntity> cancelQuery( @PathVariable String connectionId, - @PathVariable String executionId) { + @PathVariable String executionId, + HttpServletRequest httpRequest) { Map response = new HashMap<>(); + ClientContext client = ClientContext.fromRequest(httpRequest); try { if (!credentialService.connectionExists(connectionId)) { response.put("success", false); @@ -287,7 +289,14 @@ public ResponseEntity> cancelQuery( var running = runningQueryRegistry.find(executionId); if (running.isEmpty()) { - // Already finished, or never started. Nothing to cancel. + // Already finished, or never started. Nothing to cancel. Still + // audited: without this, a cancel that misses its target left + // no trace at all, deliberate or not. + sqlExecutionAuditService.record(SqlExecutionAuditService.AuditRecord.cancelNoOp() + .connectionId(connectionId) + .executionId(executionId) + .httpRequest(httpRequest) + .client(client)); response.put("success", true); response.put("cancelled", false); response.put("message", "Query is no longer running"); @@ -301,6 +310,12 @@ public ResponseEntity> cancelQuery( String currentUser = accessControlService.getCurrentUsername(); if (!connectionId.equals(target.connectionId()) || (target.username() != null && currentUser != null && !target.username().equals(currentUser))) { + sqlExecutionAuditService.record(SqlExecutionAuditService.AuditRecord.blocked( + "cancel requested for an execution id not owned by this caller/connection") + .connectionId(connectionId) + .executionId(executionId) + .httpRequest(httpRequest) + .client(client)); response.put("success", false); response.put("message", "Query not found for this connection"); return ResponseEntity.status(HttpStatus.NOT_FOUND).body(response); @@ -308,6 +323,11 @@ public ResponseEntity> cancelQuery( activeQueryService.killQuery(connectionId, target.sessionPid()); runningQueryRegistry.unregister(executionId); + sqlExecutionAuditService.record(SqlExecutionAuditService.AuditRecord.cancelled(target.sessionPid()) + .connectionId(connectionId) + .executionId(executionId) + .httpRequest(httpRequest) + .client(client)); response.put("success", true); response.put("cancelled", true); response.put("message", "Query cancelled"); @@ -318,6 +338,12 @@ public ResponseEntity> cancelQuery( return ResponseEntity.status(e.getStatusCode()).body(response); } catch (Exception e) { log.warn("Failed to cancel query {} on connection {}: {}", executionId, connectionId, e.getMessage()); + sqlExecutionAuditService.record(SqlExecutionAuditService.AuditRecord.failed(e.getMessage()) + .operation("cancel") + .connectionId(connectionId) + .executionId(executionId) + .httpRequest(httpRequest) + .client(client)); response.put("success", false); response.put("message", "Failed to cancel query: " + e.getMessage()); return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(response); diff --git a/backend/src/main/java/com/dbaagent/model/SecurityEventType.java b/backend/src/main/java/com/dbaagent/model/SecurityEventType.java index 7559ee2..30384fa 100644 --- a/backend/src/main/java/com/dbaagent/model/SecurityEventType.java +++ b/backend/src/main/java/com/dbaagent/model/SecurityEventType.java @@ -41,6 +41,7 @@ public enum SecurityEventType { EDITOR_QUERY_EXECUTED, EDITOR_QUERY_BLOCKED, EDITOR_QUERY_FAILED, + EDITOR_QUERY_CANCELLED, SUSPICIOUS_AUTH_ACTIVITY, SMTP_CONFIG_UPDATED, SMTP_TEST_SUCCEEDED, diff --git a/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyService.java b/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyService.java index d330e6a..ff7c1c7 100644 --- a/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyService.java +++ b/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyService.java @@ -466,8 +466,19 @@ private String stripQuotedLiterals(String sql) { .replaceAll("\"([^\"\\\\]|\\\\.)*\"", "\"\""); } + // Word-bounded and comment-stripped: the previous literal " WHERE " match + // missed a WHERE preceded by a newline (as in any multi-line formatted + // UPDATE/DELETE) and was satisfied by a commented-out "-- WHERE ..." that + // never reaches the database. This only runs on the keyword-fallback path — + // when JSqlParser succeeds, delete.getWhere()/update.getWhere() are used + // instead, which are exact. + private static final Pattern WHERE_CLAUSE_PATTERN = Pattern.compile("\\bWHERE\\b", Pattern.CASE_INSENSITIVE); + private boolean containsWhereClause(String sql) { - return sql != null && sql.toUpperCase(Locale.ROOT).contains(" WHERE "); + if (sql == null) { + return false; + } + return WHERE_CLAUSE_PATTERN.matcher(stripComments(sql)).find(); } private boolean looksLikeMultipleStatements(String sql) { diff --git a/backend/src/main/java/com/dbaagent/service/SqlExecutionAuditService.java b/backend/src/main/java/com/dbaagent/service/SqlExecutionAuditService.java index 861ae86..4c7e94d 100644 --- a/backend/src/main/java/com/dbaagent/service/SqlExecutionAuditService.java +++ b/backend/src/main/java/com/dbaagent/service/SqlExecutionAuditService.java @@ -80,6 +80,9 @@ public void record(AuditRecord rec) { metadata.put("connectionId", rec.connectionId); metadata.put("connectionName", rec.connectionRequest == null ? null : rec.connectionRequest.getConnectionName()); metadata.put("dbType", rec.connectionRequest == null ? null : rec.connectionRequest.getDbType()); + if (rec.executionId != null) { + metadata.put("executionId", rec.executionId); + } String queryText = rec.queryRequest == null ? null : rec.queryRequest.getQuery(); metadata.put("queryHash", SecurityHashUtil.sha256Hex(queryText == null ? "" : queryText)); @@ -183,6 +186,7 @@ public static final class AuditRecord { private String failureReason; private HttpServletRequest httpRequest; private ClientContext client; + private String executionId; private AuditRecord() {} @@ -214,6 +218,33 @@ public static AuditRecord failed(String reason) { return r; } + /** + * A deliberate cancel request against a still-running query, distinct + * from {@link #cancelNoOp()}. Without a dedicated event type, the only + * trace of a cancel was the killed query's own thread logging + * {@code pg_terminate_backend}'s error as an ordinary EDITOR_QUERY_FAILED + * — indistinguishable from any other failure, and absent entirely when + * the target had already finished before the kill reached it. + */ + public static AuditRecord cancelled(String sessionPid) { + AuditRecord r = new AuditRecord(); + r.eventType = SecurityEventType.EDITOR_QUERY_CANCELLED; + r.outcome = SecurityEventOutcome.SUCCESS; + r.operation = "cancel"; + r.failureReason = sessionPid == null ? null : "terminated session pid " + sessionPid; + return r; + } + + /** Cancel requested for an execution id that was already finished or unknown. */ + public static AuditRecord cancelNoOp() { + AuditRecord r = new AuditRecord(); + r.eventType = SecurityEventType.EDITOR_QUERY_CANCELLED; + r.outcome = SecurityEventOutcome.INFO; + r.operation = "cancel"; + r.failureReason = "query was no longer running"; + return r; + } + // ── fluent setters ──────────────────────────────────────────────── public AuditRecord operation(String op) { this.operation = op; return this; } @@ -225,6 +256,7 @@ public static AuditRecord failed(String reason) { public AuditRecord useAnalyze(Boolean v) { this.useAnalyze = v; return this; } public AuditRecord httpRequest(HttpServletRequest r) { this.httpRequest = r; return this; } public AuditRecord client(ClientContext c) { this.client = c; return this; } + public AuditRecord executionId(String id) { this.executionId = id; return this; } public AuditRecord eventType(SecurityEventType t) { this.eventType = t; return this; } public AuditRecord outcome(SecurityEventOutcome o) { this.outcome = o; return this; } } diff --git a/backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyServiceTest.java b/backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyServiceTest.java index 566ab0d..a457770 100644 --- a/backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyServiceTest.java +++ b/backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyServiceTest.java @@ -112,6 +112,42 @@ void editorConfirmedDeleteWithoutWhere_isBlocked() { assertThat(exception.getMessage()).contains("without a WHERE clause"); } + // EXPLAIN UPDATE/DELETE is the one reachable path to the keyword-fallback + // containsWhereClause check for a real UPDATE/DELETE: JSqlParser only models + // EXPLAIN SELECT, so these fall through to detectExplainWrappedMutation + // rather than Update.getWhere()/Delete.getWhere(), which every ordinary + // (non-EXPLAIN) mutation test above exercises instead. + @Test + void editorConfirmedExplainUpdateWithMultilineWhere_isAllowed() { + QueryExecutionPolicyService.PolicyDecision decision = service.enforce( + new QueryRequest( + "EXPLAIN UPDATE customers\nSET property_status = 'ACTIVE'\nWHERE customer_id = 9", + null, + null + ), + QueryExecutionContext.editor("admin", true, true), + "mysql" + ); + + assertThat(decision.mutating()).isTrue(); + assertThat(decision.primaryQueryType()).isEqualTo("UPDATE"); + } + + @Test + void editorConfirmedExplainDeleteWithOnlyCommentedOutWhere_isBlocked() { + QueryExecutionPolicyException exception = assertThrows( + QueryExecutionPolicyException.class, + () -> service.enforce( + new QueryRequest("EXPLAIN DELETE FROM customers -- WHERE customer_id = 9\n", null, null), + QueryExecutionContext.editor("admin", true, true), + "mysql" + ) + ); + + assertThat(exception.getErrorCode()).isEqualTo(QueryExecutionPolicyException.UNSAFE_MUTATION_BLOCKED); + assertThat(exception.getMessage()).contains("without a WHERE clause"); + } + @Test void editorMutation_multiStatementBatchIsBlocked() { QueryExecutionPolicyException exception = assertThrows( diff --git a/backend/src/test/java/com/dbaagent/service/SqlExecutionAuditServiceTest.java b/backend/src/test/java/com/dbaagent/service/SqlExecutionAuditServiceTest.java index 1bc25e6..6a648ab 100644 --- a/backend/src/test/java/com/dbaagent/service/SqlExecutionAuditServiceTest.java +++ b/backend/src/test/java/com/dbaagent/service/SqlExecutionAuditServiceTest.java @@ -128,6 +128,46 @@ void record_failed_capturesFailureReason() { assertThat(event.metadata()).containsEntry("clientType", "unknown"); } + @Test + void record_cancelled_logsSuccessWithSessionPidAndExecutionId() { + // Before this event type existed, a successful cancel was only visible + // as an accidental EDITOR_QUERY_FAILED thrown by the killed query's own + // thread when pg_terminate_backend severed it — indistinguishable from + // an ordinary query failure, and with no executionId to tie it back to + // the run the user actually meant to stop. + givenActor("alice@example.com", 1L); + + audit.record(SqlExecutionAuditService.AuditRecord.cancelled("8642") + .connectionId("conn-1") + .executionId("exec-abc-123") + .client(ClientContext.unknown())); + + SecurityEventService.EventRequest event = captureLoggedEvent(); + assertThat(event.eventType()).isEqualTo(SecurityEventType.EDITOR_QUERY_CANCELLED); + assertThat(event.outcome()).isEqualTo(SecurityEventOutcome.SUCCESS); + assertThat(event.reason()).contains("8642"); + assertThat(event.metadata()).containsEntry("operation", "cancel"); + assertThat(event.metadata()).containsEntry("executionId", "exec-abc-123"); + } + + @Test + void record_cancelNoOp_logsInfoOutcomeNotSilence() { + // A cancel that misses its target (already finished, or a guessed/ + // replayed executionId) must still leave a trace — previously it left + // none at all, so "did anyone try to cancel this?" was unanswerable. + givenActor("alice@example.com", 1L); + + audit.record(SqlExecutionAuditService.AuditRecord.cancelNoOp() + .connectionId("conn-1") + .executionId("exec-already-done") + .client(ClientContext.unknown())); + + SecurityEventService.EventRequest event = captureLoggedEvent(); + assertThat(event.eventType()).isEqualTo(SecurityEventType.EDITOR_QUERY_CANCELLED); + assertThat(event.outcome()).isEqualTo(SecurityEventOutcome.INFO); + assertThat(event.metadata()).containsEntry("executionId", "exec-already-done"); + } + @Test void record_analyzePlan_carriesPlanSignalsInsteadOfRowCount() { givenActor("alice@example.com", 1L); diff --git a/src/components/tabs/Core/SqlRunnerTab.js b/src/components/tabs/Core/SqlRunnerTab.js index 9a36be1..2777d5b 100644 --- a/src/components/tabs/Core/SqlRunnerTab.js +++ b/src/components/tabs/Core/SqlRunnerTab.js @@ -54,6 +54,15 @@ import { // query fails with a real message rather than an opaque 504. const QUERY_TIMEOUT_SECONDS = 240; +// CSV export re-runs the query with no display cap, so it needs its own bound. +// Without one, an unbounded SELECT streams every row into a JS array, a CSV +// string, and a Blob, and the equivalent unbounded read on the backend loads +// the whole result set into memory before responding — large enough result +// sets can exhaust the backend heap for every tenant on that instance, not +// just the exporter. 100k rows is generous for a CSV download while staying +// well short of that failure mode. +const EXPORT_ROW_LIMIT = 100000; + // Constants for diagram layout const DIAGRAM_NODE_WIDTH = 240; const DIAGRAM_NODE_HEIGHT = 120; @@ -298,6 +307,12 @@ export default function SqlRunnerTab({ connectionId }) { const dbObjectsRef = useRef([]); const abortControllerRef = useRef(null); const executionIdRef = useRef(null); + // Guards against a second run starting while one is already in flight (e.g. a + // held-down Cmd+Enter): without it, whichever response lands last wins the + // results panel regardless of which run the user actually meant to see last, + // and the earlier run's abort/cancel handle gets silently discarded. + const isRunningRef = useRef(false); + const runSeqRef = useRef(0); // Note: savedQueriesPanelRef kept for potential future use, but panel UI is now in modal const savedQueriesPanelRef = useRef(null); const hasRowCount = (value) => value !== null && value !== undefined; @@ -1194,6 +1209,9 @@ export default function SqlRunnerTab({ connectionId }) { }; const handleRunQuery = async (queryToRun = null, options = {}) => { + if (isRunningRef.current) { + return; + } const mutationConfirmed = options.mutationConfirmed === true; let queryText = null; @@ -1252,12 +1270,14 @@ export default function SqlRunnerTab({ connectionId }) { return; } + isRunningRef.current = true; setIsRunning(true); setError(null); setResults(null); setExplainResults(null); // Clear explain results when running query setOptimizeResult(null); + const seq = ++runSeqRef.current; const abortController = new AbortController(); abortControllerRef.current = abortController; // Identifies this run so cancelling can terminate it on the database. @@ -1284,6 +1304,12 @@ export default function SqlRunnerTab({ connectionId }) { }, ); + if (seq !== runSeqRef.current) { + // Superseded by a later run — drop this response rather than let a + // stale result overwrite what the user is now looking at. + return; + } + if (response.success) { setPendingMutationConfirmation(null); const DISPLAY_LIMIT = 1000; @@ -1355,9 +1381,12 @@ export default function SqlRunnerTab({ connectionId }) { setError(err.message || "Failed to execute query"); } } finally { - abortControllerRef.current = null; - executionIdRef.current = null; - setIsRunning(false); + if (seq === runSeqRef.current) { + abortControllerRef.current = null; + executionIdRef.current = null; + isRunningRef.current = false; + setIsRunning(false); + } } }; @@ -1366,6 +1395,7 @@ export default function SqlRunnerTab({ connectionId }) { if (abortControllerRef.current) { abortControllerRef.current.abort(); } + isRunningRef.current = false; setIsRunning(false); setError(null); // Aborting above only drops the HTTP response; the statement keeps running @@ -1406,23 +1436,37 @@ export default function SqlRunnerTab({ connectionId }) { const handleExportResults = async () => { if (!results) return; - - // If the result was limited, always re-fetch the full result set for download. - // This ensures the CSV contains all rows, not just the displayed page. + // Export re-runs the query, so it must respect the same in-flight guard as + // Run. Today `handleRunQuery` clears `results`, which hides the Export + // button for the duration of a run and makes this unreachable — but that is + // an incidental consequence of unrelated state handling, not a guarantee. + // Without this check, any change that keeps results on screen during a run + // silently reintroduces two concurrent queries from one tab. + if (isRunningRef.current || isExporting) return; + + // If the result was limited, always re-fetch for download so the CSV isn't + // just the displayed page — but still capped at EXPORT_ROW_LIMIT, not + // unbounded, and on the same timeout budget as Run so it fails with a real + // error instead of an opaque 504 from the nginx proxy. if (results.isLimited && results.query) { setIsExporting(true); // Strip trailing semicolon so backends don't reject the re-executed query const queryForExport = results.query.trim().replace(/;+$/, ""); + const exportAbortController = new AbortController(); + const exportExecutionId = + globalThis.crypto?.randomUUID?.() ?? + `exec-${Date.now()}-${Math.random().toString(16).slice(2)}`; try { const response = await queryAPI.executeQuery( connectionId, queryForExport, - null, // no limit — fetch all rows - 600, - null, + EXPORT_ROW_LIMIT, + QUERY_TIMEOUT_SECONDS, + exportAbortController.signal, { executionOrigin: "EDITOR", mutationConfirmed: false, + executionId: exportExecutionId, }, ); if (response.success) { @@ -2590,9 +2634,9 @@ export default function SqlRunnerTab({ connectionId }) { disabled={isExporting} title={ results?.isLimited - ? results?.totalRowCount != null - ? `Download all ${results.totalRowCount.toLocaleString()} rows as CSV` - : "Download full result set as CSV" + ? results?.totalRowCount != null && results.totalRowCount > EXPORT_ROW_LIMIT + ? `Download first ${EXPORT_ROW_LIMIT.toLocaleString()} of ${results.totalRowCount.toLocaleString()} rows as CSV` + : `Download up to ${EXPORT_ROW_LIMIT.toLocaleString()} rows as CSV` : "Export CSV" } >