From 034afdea2a0438e97ffdb04ef98d8ffd5240c9cb Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Wed, 29 Jul 2026 08:23:23 +0000 Subject: [PATCH] fix(#496): reject oversized variable option probes Validate the raw 1001-row sentinel after option-column shape checks so overflow cannot enter History or detached-result source capture. Co-Authored-By: OpenAI Codex Claude-Session: unavailable (OpenAI Codex) --- CHANGELOG.md | 6 ++ docs/ADR-0003-dashboard-viewing.md | 23 ++++++++ src/core/variable-options.ts | 19 +++++- src/ui/workbench/workbench-session.ts | 13 ++-- tests/unit/variable-options.test.ts | 16 +++++ tests/unit/workbench-session.test.ts | 85 ++++++++++++++++++++++++++- 6 files changed, 153 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 47f86e59..c3f99980 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,12 @@ auto-generated per-PR notes; this file is the curated, human-readable history. ## [Unreleased] ### Fixed +- **Dashboard variable option SQL can no longer pass Run with more than 1000 + rows** (#496). The existing bounded probe still fetches one sentinel row, but + Run now rejects that 1001-row response after column-shape validation and + before History or detached-result source capture. Zero through 1000 raw rows + remain valid, authored `LIMIT` text is not parsed, and transport errors or + cancellation keep their original outcome. - **In-place authentication recovery no longer compresses the mounted workspace** (#512). The preserved editor or Dashboard now stays behind a blocking viewport overlay, rather than being pushed below a second, diff --git a/docs/ADR-0003-dashboard-viewing.md b/docs/ADR-0003-dashboard-viewing.md index b1b429fd..7b91739c 100644 --- a/docs/ADR-0003-dashboard-viewing.md +++ b/docs/ADR-0003-dashboard-viewing.md @@ -897,6 +897,29 @@ batch-level failure. Explain button itself stays visible, same as the multi-statement case) and is checked first, since option SQL is always one statement. +## Addendum (#496, 2026-07-29): Run consumes the probe's overflow sentinel + +The #465 probe already wrapped authored option SQL with +`LIMIT VARIABLE_OPTION_CAP + 1` and requested the same 1001-row client bound, +but Run validated only response columns. A query returning the sentinel row +could therefore be recorded as a successful run even though the Dashboard +option batch supports at most 1000 options. + +- **Raw row count follows successful column-shape validation.** + `validateOptionRowCount` is a pure companion to `validateOptionColumns`; the + Workbench applies it to `result.rows.length` only when the response completed + without a server error or cancellation. Column shape wins when both contracts + are invalid, and counting happens before any future filtering or + de-duplication. +- **The authored SQL text is irrelevant to the verdict.** Zero through 1000 + rows pass whether or not the query contains its own `LIMIT`; the bounded + probe's 1001st row fails with the specific maximum-option diagnostic. The + batch compiler, its independent per-variable truncation handling, transport + bounds, and read-only byte cap are unchanged. +- **Overflow uses the existing failure gate.** The diagnostic is assigned + before the success bookkeeping in `runVariableSql`, so an over-cap response + creates neither History nor a detached-result source. + ## Alternatives considered - **Durable detached snapshots:** rejected because they silently diverge from diff --git a/src/core/variable-options.ts b/src/core/variable-options.ts index 06cd0890..72ea89eb 100644 --- a/src/core/variable-options.ts +++ b/src/core/variable-options.ts @@ -25,9 +25,11 @@ // metadata describes that query alone — which is what a variable's own // main-editor tab and the ordinary Run action are for (#457/#465): // `compileOptionProbe` embeds the SQL exactly as a batch branch would (so Run -// cannot pass what the batch would reject) but drops the branch tag, and +// cannot pass what the batch would reject) but drops the branch tag; // `validateOptionColumns` reads that probe's own, unmerged response metadata — -// the one place the "exactly two String columns" rule is checkable at all. +// the one place the "exactly two String columns" rule is checkable at all — and +// `validateOptionRowCount` rejects the +1 sentinel row before Run can record the +// response as a success. // // Deliberately NOT here: cascading/dependent option queries. Option SQL may not // reference `{name:Type}` parameters at all in this issue, which is what keeps @@ -376,6 +378,19 @@ export function validateOptionColumns( return null; } +/** + * Validate the raw row count of a SINGLE variable's successful option-query + * probe (#496). The probe is bounded at `VARIABLE_OPTION_CAP + 1`, so the extra + * row is a sentinel proving that the authored query exceeded the supported + * option count. Call this only after column validation and before any future + * filtering or de-duplication. + */ +export function validateOptionRowCount(rowCount: number): VariableOptionDiagnostic | null { + if (rowCount <= VARIABLE_OPTION_CAP) return null; + return diagnostic('variable-option-row-count', + `Dashboard variable option SQL may return at most ${VARIABLE_OPTION_CAP} rows.`); +} + /** * Turn one option-batch response into per-variable option lists. * diff --git a/src/ui/workbench/workbench-session.ts b/src/ui/workbench/workbench-session.ts index 1d262831..0705ceda 100644 --- a/src/ui/workbench/workbench-session.ts +++ b/src/ui/workbench/workbench-session.ts @@ -34,7 +34,7 @@ import { newResult } from '../../core/stream.js'; import { buildResultSource } from '../../core/query-source.js'; import { VARIABLE_OPTION_BYTE_CAP, VARIABLE_OPTION_CAP, - compileOptionProbe, optionSqlDiagnostics, validateOptionColumns, + compileOptionProbe, optionSqlDiagnostics, validateOptionColumns, validateOptionRowCount, } from '../../core/variable-options.js'; import type { QueryResult, ScriptResult, ScriptEntry } from '../results.js'; import type { QueryExecutionService } from '../../application/query-execution-service.js'; @@ -564,11 +564,14 @@ export function createWorkbenchSession(deps: WorkbenchSessionDeps): WorkbenchSes }); // Only the probe's own transport succeeding makes its response metadata // meaningful — a transport error or a cancellation already has its own - // story and must not be overwritten by a shape verdict about a response - // that never fully arrived. + // story and must not be overwritten by a verdict about a response that + // never fully arrived. Shape is checked before raw row count so a malformed + // response retains the more fundamental column diagnostic; the count is + // read before any future filtering or de-duplication can hide the sentinel. if (isCurrent(registration) && !result.error && !result.cancelled) { - const shape = validateOptionColumns(result.columns); - if (shape) result.error = shape.message; + const invalid = validateOptionColumns(result.columns) + ?? validateOptionRowCount(result.rows.length); + if (invalid) result.error = invalid.message; } } finally { if (!isCurrent(registration)) { diff --git a/tests/unit/variable-options.test.ts b/tests/unit/variable-options.test.ts index b164c259..aa0b1c1d 100644 --- a/tests/unit/variable-options.test.ts +++ b/tests/unit/variable-options.test.ts @@ -11,6 +11,7 @@ import { VARIABLE_OPTION_BYTE_CAP, VARIABLE_OPTION_CAP, compileOptionProbe, compileVariableOptionBatch, isOptionColumnType, normalizeOptionSql, optionBatchVariables, optionSqlDiagnostics, readVariableOptionBatch, validateOptionColumns, + validateOptionRowCount, } from '../../src/core/variable-options.js'; import type { DashboardVariable } from '../../src/core/dashboard-variables.js'; @@ -355,6 +356,21 @@ describe('validateOptionColumns', () => { }); }); +describe('validateOptionRowCount', () => { + it('accepts every supported boundary count', () => { + expect(validateOptionRowCount(0)).toBeNull(); + expect(validateOptionRowCount(VARIABLE_OPTION_CAP)).toBeNull(); + }); + + it('rejects the probe sentinel and any defensive overage with a specific diagnostic', () => { + for (const count of [VARIABLE_OPTION_CAP + 1, VARIABLE_OPTION_CAP + 50]) { + const found = validateOptionRowCount(count)!; + expect(found.code).toBe('variable-option-row-count'); + expect(found.message).toBe('Dashboard variable option SQL may return at most 1000 rows.'); + } + }); +}); + describe('readVariableOptionBatch', () => { const cols = [ { name: '__variable_name', type: 'String' }, diff --git a/tests/unit/workbench-session.test.ts b/tests/unit/workbench-session.test.ts index 33607029..8c290096 100644 --- a/tests/unit/workbench-session.test.ts +++ b/tests/unit/workbench-session.test.ts @@ -906,6 +906,81 @@ describe('createWorkbenchSession: dashboard-variable Run (#465)', () => { expect(result?.rows).toEqual([]); }); + it('exactly 1000 raw rows pass without requiring an authored LIMIT', async () => { + const h = makeHarness({ tab: variableTab({ sqlDraft: 'SELECT a, b FROM t' }) }); + const rows = Array.from({ length: VARIABLE_OPTION_CAP }, (_, i) => [`v${i}`, `l${i}`]); + h.execFakes.executeRead.mockImplementation(async (result: StreamResult) => { + Object.assign(result, { + columns: [{ name: 'a', type: 'String' }, { name: 'b', type: 'String' }], + rows, + }); + return result; + }); + const session = createWorkbenchSession(h.deps); + await session.run(); + const result = h.tab.result as { error: string | null; source?: unknown } | null; + expect(result?.error).toBeNull(); + expect(result?.source).toBeDefined(); + expect(h.hooks.recordHistory).toHaveBeenCalledWith(h.tab, 'SELECT a, b FROM t'); + }); + + it('an authored LIMIT passes when the actual returned row count is within the cap', async () => { + const sql = 'SELECT a, b FROM t LIMIT 5'; + const h = makeHarness({ tab: variableTab({ sqlDraft: sql }) }); + h.execFakes.executeRead.mockImplementation(async (result: StreamResult) => { + Object.assign(result, { + columns: [{ name: 'a', type: 'String' }, { name: 'b', type: 'String' }], + rows: [['v', 'l']], + }); + return result; + }); + const session = createWorkbenchSession(h.deps); + await session.run(); + const result = h.tab.result as { error: string | null; source?: unknown } | null; + expect(result?.error).toBeNull(); + expect(result?.source).toBeDefined(); + expect(h.hooks.recordHistory).toHaveBeenCalledWith(h.tab, sql); + }); + + it('rejects the 1001-row sentinel by actual count even when the authored SQL has a LIMIT', async () => { + const sql = 'SELECT a, b FROM t LIMIT 5000'; + const h = makeHarness({ tab: variableTab({ sqlDraft: sql }) }); + const rows = Array.from({ length: VARIABLE_OPTION_CAP + 1 }, (_, i) => [`v${i}`, `l${i}`]); + h.execFakes.executeRead.mockImplementation(async (result: StreamResult) => { + Object.assign(result, { + columns: [{ name: 'a', type: 'String' }, { name: 'b', type: 'String' }], + rows, + }); + return result; + }); + const session = createWorkbenchSession(h.deps); + await session.run(); + const req = h.execFakes.executeRead.mock.calls[0][1] as ExecuteReadRequest; + expect(req.sql).toContain(sql); + const result = h.tab.result as { error: string | null; source?: unknown } | null; + expect(result?.error).toBe('Dashboard variable option SQL may return at most 1000 rows.'); + expect(result?.source).toBeUndefined(); + expect(h.hooks.recordHistory).not.toHaveBeenCalled(); + expect(h.hooks.recordBoundParams).not.toHaveBeenCalled(); + expect(h.tab.lastSuccessfulResultColumns).toEqual([]); + }); + + it('checks column shape before the raw row count', async () => { + const h = makeHarness({ tab: variableTab({ sqlDraft: 'SELECT a FROM t' }) }); + h.execFakes.executeRead.mockImplementation(async (result: StreamResult) => { + Object.assign(result, { + columns: [{ name: 'a', type: 'String' }], + rows: Array.from({ length: VARIABLE_OPTION_CAP + 1 }, () => ['v']), + }); + return result; + }); + const session = createWorkbenchSession(h.deps); + await session.run(); + const result = h.tab.result as { error: string | null } | null; + expect(result?.error).toMatch(/exactly two columns/); + expect(result?.error).not.toMatch(/at most 1000 rows/); + }); + it('a one-column result reports the actual column count', async () => { const h = makeHarness({ tab: variableTab({ sqlDraft: 'SELECT a FROM t' }) }); h.execFakes.executeRead.mockImplementation(async (result: StreamResult) => { @@ -951,7 +1026,10 @@ describe('createWorkbenchSession: dashboard-variable Run (#465)', () => { it('preserves a transport error, never overwritten by shape validation', async () => { const h = makeHarness({ tab: variableTab({ sqlDraft: 'SELECT a, b FROM t' }) }); h.execFakes.executeRead.mockImplementation(async (result: StreamResult) => { - result.error = 'Some server error'; + Object.assign(result, { + error: 'Some server error', + rows: Array.from({ length: VARIABLE_OPTION_CAP + 1 }, () => ['v', 'l']), + }); return result; }); const session = createWorkbenchSession(h.deps); @@ -963,7 +1041,10 @@ describe('createWorkbenchSession: dashboard-variable Run (#465)', () => { it('preserves cancellation, never overwritten by shape validation', async () => { const h = makeHarness({ tab: variableTab({ sqlDraft: 'SELECT a, b FROM t' }) }); h.execFakes.executeRead.mockImplementation(async (result: StreamResult) => { - result.cancelled = true; + Object.assign(result, { + cancelled: true, + rows: Array.from({ length: VARIABLE_OPTION_CAP + 1 }, () => ['v', 'l']), + }); return result; }); const session = createWorkbenchSession(h.deps);