Files
0pt/tests/analytics/tenant-predicate-regression.test.ts
T
Claude Opus 5 d9c8f7190b
CI (SIAX Cloud) / quality (push) Successful in 50s
CI (SIAX Cloud) / integration (push) Successful in 35s
CI (SIAX Cloud) / security (push) Successful in 21s
Security Scan (SIAX Cloud) / security (push) Successful in 26s
CI (SIAX Cloud) / quality (pull_request) Successful in 46s
CI (SIAX Cloud) / integration (pull_request) Successful in 35s
CI (SIAX Cloud) / security (pull_request) Successful in 19s
Security Scan (SIAX Cloud) / security (pull_request) Successful in 16s
feat(analytics): BI-P017 — analyst SQL contract, proven against live ClickHouse with the guard removed
AnalyticsSQLSession is a SEPARATE class, not a method on SiaxAnalyticsEngine.
executeMetric has no raw-SQL surface at all; on the same object the closed DSL
would be one refactor away from growing one. ClickHouseAnalyticsEngine
.executeQuery stays a documented refusal.

FOUR LAYERS, and the guard is deliberately called the weakest:
  1 role   — the CL0UD analyst role is checked when the SESSION IS OPENED, so
             a caller without it never obtains an object that can run SQL.
  2 guard  — statement admission. Comments and literals removed with
             ClickHouse's own lexical rules, ASCII-only outside literals, one
             statement, SELECT/WITH only, and every table reference POSITIVELY
             resolved against the tenant's catalog. Deny-lists are a second
             opinion, never the mechanism.
  3 grants — a per-tenant ClickHouse user with SELECT on that tenant's catalog
             alone, plus a restrictive row policy on the base tables.
  4 views  — the tenant catalog holds only VIEWS, each baking
             `WHERE tenant_id = '<tenant>'`, and tenant_id is not projected.

61 bypass attempts, in one shared corpus, run TWICE: against the guard, and
against a live ClickHouse 24.8.14.39 with LAYER 2 REMOVED. The live assertion
is not "the engine errored" — it is "no canary came back", with BETA_CANARY
seeded into the other tenant's rows and SECRET_CANARY into an ungranted table.
The counterfactual runs first: as admin the same base table returns 100 rows
and BETA_CANARY IS readable, so the isolation numbers mean something.
Result: 0 of 61 leaked. Evidence: evidence/bi-p017/analyst-sql-live-bypass.json.

FOUR DEFECTS FOUND BY WRITING TESTS THAT CAN FAIL — three of them mine:

  * `CREATE ROW POLICY IF NOT EXISTS` is a SILENT NO-OP on re-provisioning.
    Dropping the user empties the policy's apply-to list; the next run skips
    re-creation, so the policy survives with `apply_to_list = []`, applying to
    nobody, while `system.row_policies` still shows the correct select_filter.
    The restricted user read 100 of 100 rows — BOTH TENANTS — from the base
    table. Fixed with OR REPLACE + `rowPolicyVerificationSql`, which turns
    "the policy is applied" from an assumption into a query, and a test that
    asserts applied_to_user = 1 per object.

  * `CREATE SETTINGS PROFILE IF NOT EXISTS` has the identical shape: a changed
    budget never lands. A stale 60-row ceiling survived re-provisioning and
    starved every legitimate statement, with the DDL reporting success
    throughout. Also OR REPLACE now.

  * `describe.skipIf(!reachable)` where `reachable` is set in `beforeAll`
    skips ALWAYS — vitest evaluates skipIf at collection time. Ten tests
    reported green while skipping against a live, reachable server. Replaced
    with the repo's existing env-var gate, which FAILS when CLICKHOUSE_URL is
    set and the server is down rather than degrading to a skip.

  * MEASURED, and it changes the design: a ClickHouse user whose profile
    carries `readonly = 1` cannot have ANY setting changed from the request
    (Code: 164). The BI-P012 ceilings therefore cannot travel with an analyst
    statement; they are baked into the user's SETTINGS PROFILE at provisioning.
    Proven both ways: over-budget scan fails with code 158 from the profile
    with no setting sent, and an attempt to raise it from the request fails
    with code 164.

Honest about what the guard does NOT catch: `SELECT tenant_id FROM
t_alpha_sql.sales` is structurally legitimate and IS admitted. It fails at the
engine because the view does not project the column. Recorded in its own
GUARD_ADMITS_ENGINE_REFUSES list and asserted live, rather than deleted from
the corpus.

evidence/bi-p017 is pinned by tests/analytics/analyst-sql-evidence.test.ts,
which never skips: adding an attack without re-running the live suite breaks
the build, and the pin also checks the counterfactual numbers so the evidence
cannot be a recording of an empty database.

246 tests pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-09-07 02:17:07 +02:00

152 lines
6.6 KiB
TypeScript

/**
* BI-P009's non-negotiable line: "Do not regress the existing forced tenant
* predicate."
*
* This suite is written to FAIL IF THE FORCING IS REMOVED, which is a
* different thing from testing that it currently works. Concretely, each test
* below would go red under a specific, plausible regression:
*
* * someone makes `tenant_id` optional in the metric DSL -> "refuses a
* blank tenant" fails;
* * someone "simplifies" the WHERE builder and drops the tenant condition
* -> "every compiled statement contains the predicate" fails on a missing
* substring;
* * someone starts splicing the tenant value into SQL text instead of
* binding it -> "the tenant VALUE never appears in the SQL text" fails;
* * someone lets a caller pass a tenant through the request body rather
* than the verified identity -> "identity wins over any request field"
* fails.
*
* It covers BOTH compilers: the PRESERVED `ClickHouseAnalytics.queryMetric`
* (src/analytics/clickhouse-analytics.ts, untouched by BI-P009) and the new
* `compileMetricQuery` — because "preserved" means the old guarantee has to
* keep holding, not merely that the file still exists.
*
* The ClickHouseAnalytics half asserts against the URL the adapter actually
* builds, captured with a stub `fetch`. That is deliberate: a mock ENGINE
* could be made to return anything, but the SQL text on the wire is the thing
* the real server would receive, so asserting on it cannot be faked by the
* test double.
*/
import { describe, it, expect, afterEach, vi } from 'vitest';
import { ClickHouseAnalytics } from '@/src/analytics/clickhouse-analytics';
import {
compileMetricQuery,
compileProjectionQuery,
dimensionCatalogFrom,
FORCED_TENANT_PREDICATE,
} from '@/src/analytics/engine/query-compiler';
const catalog = dimensionCatalogFrom({ severity: 'severity', make: 'vehicle_make' });
/** Captures every URL the adapter fetches, returning an empty JSON result. */
function captureFetch(): { urls: string[]; restore: () => void } {
const urls: string[] = [];
const original = globalThis.fetch;
globalThis.fetch = (async (input: RequestInfo | URL) => {
urls.push(String(input));
return new Response(JSON.stringify({ data: [] }), { status: 200 });
}) as typeof fetch;
return { urls, restore: () => (globalThis.fetch = original) };
}
afterEach(() => {
vi.restoreAllMocks();
});
describe('BI-P009 regression guard: the forced tenant predicate', () => {
it('PRESERVED ClickHouseAnalytics still emits tenant_id = {tenant:String} on the wire', async () => {
const cap = captureFetch();
try {
const analytics = new ClickHouseAnalytics({ baseUrl: 'http://ch.invalid:8123', database: 'opt_test' });
await analytics.queryMetric({ tenant_id: 't_alpha', metric: 'rows', aggregate: 'sum' });
expect(cap.urls).toHaveLength(1);
// URLSearchParams decodes `+` as a space, which raw decodeURIComponent
// does not — read the statement the way the server will.
const query = new URL(cap.urls[0]).searchParams.get('query') ?? '';
expect(query).toContain('tenant_id = {tenant:String}');
expect(query).toMatch(/WHERE tenant_id = \{tenant:String\}/);
// The VALUE travels as a bound parameter, never inside the statement.
expect(query).not.toContain('t_alpha');
expect(new URL(cap.urls[0]).searchParams.get('param_tenant')).toBe('t_alpha');
} finally {
cap.restore();
}
});
it('PRESERVED ClickHouseAnalytics still refuses a blank tenant_id', async () => {
const analytics = new ClickHouseAnalytics({ baseUrl: 'http://ch.invalid:8123' });
await expect(
analytics.queryMetric({ tenant_id: ' ', metric: 'x', aggregate: 'sum' }),
).rejects.toMatchObject({
code: 'INVALID_REQUEST',
});
});
it('PRESERVED ClickHouseAnalytics still refuses a hostile group_by identifier', async () => {
const analytics = new ClickHouseAnalytics({ baseUrl: 'http://ch.invalid:8123' });
await expect(
analytics.queryMetric({
tenant_id: 't_alpha',
metric: 'x',
aggregate: 'sum',
group_by: "sev'; DROP TABLE opt_metric_events; --",
}),
).rejects.toMatchObject({ code: 'INVALID_REQUEST' });
});
it('every statement the NEW compiler produces contains the forced predicate', () => {
const shapes = [
{ metric: 'm', aggregate: 'sum' as const },
{ metric: 'm', aggregate: 'count' as const, group_by: ['severity'] },
{ metric: 'm', aggregate: 'avg' as const, filters: { make: 'volvo' }, dataset_id: 'ds_1' },
{ metric: 'm', aggregate: 'max' as const, from_time: '2026-01-01', to_time: '2026-02-01' },
{ metric: 'm', aggregate: 'count_distinct' as const, group_by: ['severity', 'make'], limit: 5 },
];
for (const shape of shapes) {
const compiled = compileMetricQuery({ tenant_id: 't_alpha', ...shape }, 'opt_analytics', catalog, 1000);
expect(compiled.sql).toContain(FORCED_TENANT_PREDICATE);
// The predicate must be in the WHERE, not in a comment or an alias.
expect(compiled.sql).toMatch(/WHERE tenant_id = \{tenant:String\}/);
expect(compiled.params.param_tenant).toBe('t_alpha');
expect(compiled.sql).not.toContain('t_alpha');
}
const projection = compileProjectionQuery(
{ tenant_id: 't_alpha', dataset_id: 'ds_1', entity_keys: ['a', 'b'] },
'opt_analytics',
1000,
);
expect(projection.sql).toMatch(/WHERE tenant_id = \{tenant:String\}/);
expect(projection.sql).not.toContain('t_alpha');
});
it('the NEW compiler fails closed on a blank/absent tenant instead of emitting an unfiltered query', () => {
for (const tenant of ['', ' ']) {
expect(() =>
compileMetricQuery(
{ tenant_id: tenant, metric: 'm', aggregate: 'sum' },
'opt_analytics',
catalog,
1000,
),
).toThrowError(/tenant_id is required/);
}
// The undefined case, which TypeScript would normally forbid, is what a
// JS caller or a loosely-typed HTTP boundary can actually produce.
expect(() =>
compileMetricQuery(
{ tenant_id: undefined as unknown as string, metric: 'm', aggregate: 'sum' },
'opt_analytics',
catalog,
1000,
),
).toThrowError(/tenant_id is required/);
});
it('the constant itself is the predicate, so deleting the forcing cannot pass silently', () => {
// If someone redefines FORCED_TENANT_PREDICATE to something harmless
// (e.g. "1 = 1") to make the substring assertions above pass, this fails.
expect(FORCED_TENANT_PREDICATE).toBe('tenant_id = {tenant:String}');
});
});