Skip to content

Commit 67d840f

Browse files
committed
fix(stack): count codepoints in the free-text needle floor
Review of #607 found the needle guard it added was itself incomplete, and two claims it made were false. Three agents verified each finding against live Postgres before anything changed here. The guard counted UTF-16 code units; the ngram tokenizer counts CODEPOINTS. A needle of 1-2 astral-plane characters cleared the floor, produced no trigram, and so built an EMPTY bloom filter -- `stored_bf @> '{}'` matches every row. Measured live against three seeded rows: "👍👍" utf16=4 cp=2 -> 3/3 rows <- passed the guard "👍👍👍" utf16=6 cp=3 -> 0/3 rows "joh" utf16=3 cp=3 -> 1/3 rows Codepoints, not graphemes: NFD "e+combining-acute" twice is 4 codepoints but 2 grapheme clusters, and does build a non-empty filter (0/3 rows live). A grapheme floor would wrongly reject it; a byte floor would wrongly accept the emoji. The three measurements are consistent only with codepoint counting. Also rejects the empty needle under ANY tokenizer. `matchNeedleMinLength` returns undefined for the `standard` tokenizer, so the guard short-circuited and `contains(col, '')` tokenized to nothing and matched every row. Unreachable via the v3 `types` namespace today, which hardcodes ngram, but the schema permits it. Tests, written first and watched fail: - `__tests__/match-needle-guard.test.ts` pins the unit. The astral and empty-needle cases failed before the fix; the NFD case passed before and after, proving the fix does not over-correct into rejecting usable needles. Unicode is built from explicit escapes -- the precomposed NFC form is a different, 2-codepoint string, so a bare literal would silently test the opposite case depending on the file's on-disk normalisation. - The live `contains` rejection now asserts the guard's own message, not just `EncryptionOperatorError`. `operandFailure` throws the same class, and that is not hypothetical: `text_search` carries an `ore` index and cannot encrypt any non-ASCII operand ("Can only order strings that are pure ASCII"), so the astral case was passing there for the wrong reason. That constraint now has its own test, and the codepoint complement runs only on match-only columns. Two false claims corrected rather than papered over: - The changeset and the doc comment both said the floor was "shared with the v2 adapter". It is not -- `matchNeedleError` has exactly one caller, in v3. v2's `like`/`ilike` reduces to `eql_v2.bloom_filter(a) @> eql_v2.bloom_filter(b)` (cipherstash-encrypt.sql), native array containment, so v2 has the same fail-open and is NOT fixed here. Wiring it in needs its own measurement first: v2 needles carry SQL wildcards, so the floor may belong on the unwrapped term. - `decryptDenied` returned a bare boolean and treated any throw as denial, so a ZeroKMS outage read as "the identity boundary held". It now returns the message and the call sites assert /^Failed to retrieve key/, matching the assertion this PR already made in `lock-context.test.ts`. The `as SQL` cast in `inArrayOp` was reviewed and kept: drizzle returns undefined only for an empty filtered list, and the empty-list guard throws first. Restoring the old `?? sql`false`` fallback would be worse -- it would mask a `notInArray` that silently matches everything.
1 parent 0ebf57e commit 67d840f

5 files changed

Lines changed: 250 additions & 41 deletions

File tree

.changeset/eql-v3-drizzle-fail-open-guards.md

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,20 @@
44

55
Close two fail-open paths in the EQL v3 Drizzle adapter.
66

7-
`ops.contains()` now throws `EncryptionOperatorError` when the search term is
8-
shorter than the match index tokenizer's `token_length` (3 by default). Such a
9-
term produces no ngrams, so its bloom filter is empty — and `stored_bf @> '{}'`
10-
is true for every row. Previously a user searching `"ad"` silently received the
11-
entire table; measured live, the needles `"ad"`, `"a"` and `"x"` each returned
12-
every seeded row, including one in which `"x"` did not appear. The length floor
13-
is shared with the v2 adapter via `matchNeedleError` in `schema/match-defaults`,
14-
since both build byte-identical bloom filters.
7+
`ops.contains()` now throws `EncryptionOperatorError` for a search term that
8+
tokenizes to nothing: the empty string, or a term shorter than the match index
9+
tokenizer's `token_length` (3 by default). Such a term produces an empty bloom
10+
filter, and `stored_bf @> '{}'` is true for every row — so a user searching
11+
`"ad"` silently received the entire table. Measured live, the terms `"ad"`,
12+
`"a"` and `"x"` each returned every seeded row, including one in which `"x"`
13+
did not appear.
14+
15+
The floor counts Unicode codepoints, matching the tokenizer. A UTF-16 length
16+
check would wave through an astral-plane term — `"👍👍"` is 4 code units but
17+
only 2 codepoints, yields no trigram, and matched every row.
18+
19+
**Breaking for callers passing short terms:** `contains()` calls that previously
20+
returned every row now throw. Terms of 3+ codepoints are unaffected.
1521

1622
`v3FromDriver()` now throws the new `EqlV3CodecError` on a payload that is not
1723
an EQL envelope, instead of surfacing a raw `SyntaxError` for malformed JSON and
@@ -23,3 +29,8 @@ accepts both scalar envelopes (ciphertext at `c`) and SteVec documents
2329

2430
Also removes an unreachable branch in `inArray`/`notInArray`, whose empty-list
2531
guard already throws before it.
32+
33+
Note: the v2 Drizzle adapter's `like`/`ilike` path builds the same bloom filters
34+
and has the same short-term fail-open. It is **not** fixed here — v2 terms carry
35+
SQL wildcards, so the floor must be measured against what its tokenizer actually
36+
receives before the shared guard can be reused. Tracked separately.

packages/stack/__tests__/drizzle-v3/operators-live-pg.test.ts

Lines changed: 67 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -549,12 +549,74 @@ describeLivePg('v3 drizzle operators (live pg matrix)', () => {
549549
// A needle below the tokenizer's `token_length` builds an EMPTY bloom filter,
550550
// and `stored_bf @> '{}'` holds for every row — so before the SDK guard this
551551
// silently returned the whole table.
552+
//
553+
// `'👍👍'` is the regression case: 4 UTF-16 code units but only 2 codepoints,
554+
// so a `needle.length` floor waved it through and it matched every row. The
555+
// tokenizer counts codepoints. `''` tokenizes to nothing under any tokenizer.
552556
it.each(
553-
matchDomains,
554-
)('%s contains rejects a needle shorter than token_length', async (eqlType) => {
555-
await expect(
556-
ops.contains(matrixColumn(eqlType), 'ad'),
557-
).rejects.toBeInstanceOf(EncryptionOperatorError)
557+
matchDomains.flatMap(([eqlType]) =>
558+
['ad', '👍👍', ''].map((needle) => [eqlType, needle] as const),
559+
),
560+
)(
561+
'%s contains rejects the unanswerable needle %j before encrypting',
562+
async (eqlType, needle) => {
563+
const attempt = ops.contains(matrixColumn(eqlType), needle)
564+
await expect(attempt).rejects.toBeInstanceOf(EncryptionOperatorError)
565+
// Assert the GUARD rejected it, not the encryption layer.
566+
// `operandFailure` also throws `EncryptionOperatorError`, so matching only
567+
// the class would let an encryption error stand in for the guard and the
568+
// test would pass for the wrong reason.
569+
await expect(attempt).rejects.toThrow(/free-text search needs/)
570+
},
571+
30000,
572+
)
573+
574+
// The complement: a needle that DOES reach the floor in codepoints must be
575+
// ANSWERED, not over-rejected — otherwise the fix for the astral fail-open
576+
// would just over-correct into rejecting usable needles.
577+
//
578+
// Restricted to match-ONLY columns: a column that also carries an `ore` index
579+
// (`text_search`) cannot encrypt a non-ASCII operand at all — the ORE term
580+
// raises "Can only order strings that are pure ASCII" — so a non-ASCII needle
581+
// there fails inside encryption, long after the guard has passed it. That
582+
// constraint is pinned by its own test below rather than silently conflated
583+
// with the codepoint floor.
584+
//
585+
// Escapes, not literals: the precomposed NFC form of `ee-with-accents` is only
586+
// 2 codepoints and IS rejected, so a bare literal would test the opposite case
587+
// depending on the file's on-disk normalisation.
588+
const NFD_EE = 'e\u0301e\u0301' // 4 codepoints, 2 grapheme clusters
589+
const matchOnlyDomains = matchDomains.filter(([, spec]) => !spec.indexes.ore)
590+
591+
it.each(
592+
matchOnlyDomains.flatMap(([eqlType]) =>
593+
['\u{1F44D}\u{1F44D}\u{1F44D}', NFD_EE].map(
594+
(needle) => [eqlType, needle] as const,
595+
),
596+
),
597+
)(
598+
'%s contains answers the codepoint-sufficient needle %j with no match',
599+
async (eqlType, needle) => {
600+
const rows = await selectRowKeys(
601+
await ops.contains(matrixColumn(eqlType), needle),
602+
)
603+
expect(rows).toEqual([])
604+
},
605+
30000,
606+
)
607+
608+
// An `ore`-bearing match column rejects a non-ASCII needle inside ENCRYPTION,
609+
// not in the needle guard. Pinned so the distinction stays visible: the guard
610+
// is about tokenization, this is about the ORE term's ASCII-only ordering.
611+
it.each(
612+
matchDomains.filter(([, spec]) => spec.indexes.ore),
613+
)('%s contains rejects a non-ASCII needle in the ORE term, not the guard', async (eqlType) => {
614+
const attempt = ops.contains(
615+
matrixColumn(eqlType),
616+
'\u{1F44D}\u{1F44D}\u{1F44D}',
617+
)
618+
await expect(attempt).rejects.toThrow(/pure ASCII/)
619+
await expect(attempt).rejects.not.toThrow(/free-text search needs/)
558620
}, 30000)
559621

560622
it.each(

packages/stack/__tests__/drizzle-v3/operators-lock-context-live-pg.test.ts

Lines changed: 40 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -84,22 +84,38 @@ function unwrap<T>(result: { data?: T; failure?: { message: string } }): T {
8484
}
8585

8686
/**
87-
* True when a decrypt attempt was DENIED. `decrypt` reports denial as a
88-
* `Result.failure` rather than throwing, so a bare `await` would resolve and a
89-
* naive `expect(...).rejects` would never fire — denial has to be read off the
90-
* result. A throw also counts as denial.
87+
* The outcome of a decrypt attempt that is EXPECTED to be denied. `decrypt`
88+
* reports denial as a `Result.failure` rather than throwing, so a bare `await`
89+
* would resolve and a naive `expect(...).rejects` would never fire — denial has
90+
* to be read off the result. A throw also counts as denial.
91+
*
92+
* Returns the message, not just a boolean, so callers can assert WHY it was
93+
* denied. A bare boolean would let any infrastructure fault — a DNS failure, an
94+
* expired CTS token, a ZeroKMS outage ("SendRequest: Failed to send request") —
95+
* read as "the identity boundary held", and the security test would pass for
96+
* the wrong reason.
9197
*/
92-
async function decryptDenied(
98+
async function decryptOutcome(
9399
attempt: () => PromiseLike<{ failure?: { message: string } }>,
94-
): Promise<boolean> {
100+
): Promise<{ denied: boolean; message: string }> {
95101
try {
96102
const result = await attempt()
97-
return Boolean(result.failure)
98-
} catch {
99-
return true
103+
return {
104+
denied: Boolean(result.failure),
105+
message: result.failure?.message ?? '',
106+
}
107+
} catch (error) {
108+
return { denied: true, message: (error as Error).message }
100109
}
101110
}
102111

112+
/**
113+
* A genuine key-derivation denial. ZeroKMS cannot reproduce the key tag without
114+
* the identity claim the row was sealed under, and says so. Asserted verbatim
115+
* elsewhere in the suite (`lock-context.test.ts`, `protect-ops.test.ts`).
116+
*/
117+
const KEY_DENIAL = /^Failed to retrieve key/
118+
103119
/** Run-scoped SELECT of row keys under an already-encrypted SQL condition. */
104120
async function selectRowKeys(condition: SQL): Promise<string[]> {
105121
const rows = (await db
@@ -230,10 +246,14 @@ describeLivePg('v3 drizzle operators with lock context (live pg)', () => {
230246
)
231247

232248
// `decrypt` reports denial as a `Result.failure` and does not throw, so a
233-
// bare `await` here would silently pass. Require denial via either channel.
234-
expect(await decryptDenied(() => client.decrypt(row.value as never))).toBe(
235-
true,
249+
// bare `await` here would silently pass. Require denial via either channel,
250+
// AND require it be a key-derivation denial rather than an outage.
251+
const { denied, message } = await decryptOutcome(() =>
252+
client.decrypt(row.value as never),
236253
)
254+
255+
expect(denied).toBe(true)
256+
expect(message).toMatch(KEY_DENIAL)
237257
}, 30000)
238258

239259
it('an identity-bound row does not decrypt under a different identity claim', async () => {
@@ -244,12 +264,13 @@ describeLivePg('v3 drizzle operators with lock context (live pg)', () => {
244264
[RUN, ROW_A],
245265
)
246266

247-
expect(
248-
await decryptDenied(() =>
249-
client
250-
.decrypt(row.value as never)
251-
.withLockContext({ identityClaim: ['email'] } as never),
252-
),
253-
).toBe(true)
267+
const { denied, message } = await decryptOutcome(() =>
268+
client
269+
.decrypt(row.value as never)
270+
.withLockContext({ identityClaim: ['email'] } as never),
271+
)
272+
273+
expect(denied).toBe(true)
274+
expect(message).toMatch(KEY_DENIAL)
254275
}, 30000)
255276
})
Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
import { describe, expect, it } from 'vitest'
2+
import type { MatchIndexOpts } from '@/schema'
3+
import { matchNeedleError, matchNeedleMinLength } from '@/schema/match-defaults'
4+
5+
// The floor a free-text needle must clear before it yields any ngram. A needle
6+
// below it tokenizes to nothing, so its bloom filter is EMPTY — and
7+
// `stored_bf @> '{}'` is true for every row. Such a query is unanswerable, not
8+
// merely unmatched, so the guard must reject it rather than silently return the
9+
// whole table.
10+
const ngram: MatchIndexOpts = { tokenizer: { kind: 'ngram', token_length: 3 } }
11+
12+
describe('matchNeedleMinLength', () => {
13+
it('is the ngram tokenizer token_length', () => {
14+
expect(matchNeedleMinLength(ngram)).toBe(3)
15+
expect(
16+
matchNeedleMinLength({ tokenizer: { kind: 'ngram', token_length: 4 } }),
17+
).toBe(4)
18+
})
19+
20+
it('defaults an absent tokenizer to the schema default rather than skipping the floor', () => {
21+
expect(matchNeedleMinLength({})).toBe(3)
22+
})
23+
})
24+
25+
describe('matchNeedleError', () => {
26+
it('accepts a needle at or above the floor', () => {
27+
expect(matchNeedleError('joh', ngram)).toBeUndefined()
28+
expect(matchNeedleError('lovelace', ngram)).toBeUndefined()
29+
})
30+
31+
it('rejects a needle below the floor, naming the floor and the term', () => {
32+
expect(matchNeedleError('ad', ngram)).toMatch(/at least 3 characters/)
33+
expect(matchNeedleError('ad', ngram)).toMatch(/"ad"/)
34+
})
35+
36+
// The tokenizer counts Unicode CODEPOINTS. `'👍👍'` is 2 codepoints but 4
37+
// UTF-16 code units, so a `needle.length` check waves it through — and it
38+
// then matches every row. Measured live: 3/3 rows returned.
39+
it('rejects an astral-plane needle below the floor in CODEPOINTS, not UTF-16 units', () => {
40+
expect('👍👍'.length).toBe(4) // pins why a naive length check passes it
41+
expect([...'👍👍'].length).toBe(2)
42+
43+
expect(matchNeedleError('👍👍', ngram)).toMatch(/at least 3 characters/)
44+
})
45+
46+
it('reports the codepoint count, not the UTF-16 length, in the message', () => {
47+
expect(matchNeedleError('👍👍', ngram)).toMatch(/has 2\b/)
48+
})
49+
50+
it('accepts an astral-plane needle that reaches the floor in codepoints', () => {
51+
expect(matchNeedleError('👍👍👍', ngram)).toBeUndefined()
52+
})
53+
54+
// Combining acute accents. NFD 'e\u0301e\u0301' is 4 codepoints but only 2
55+
// grapheme clusters, and (measured live) builds a NON-empty filter — so the
56+
// unit is codepoints, not graphemes. A grapheme floor would wrongly reject it.
57+
//
58+
// Built from explicit escapes: the PRECOMPOSED NFC form is a different string
59+
// of only 2 codepoints, which the guard correctly rejects. A bare literal here
60+
// would test whichever form the file happened to be normalised to on disk.
61+
const NFD_EE = 'e\u0301e\u0301'
62+
const NFC_EE = '\u00e9\u00e9'
63+
64+
it('accepts a combining-accent needle that is 4 codepoints but only 2 graphemes', () => {
65+
expect([...NFD_EE].length).toBe(4)
66+
expect(matchNeedleError(NFD_EE, ngram)).toBeUndefined()
67+
})
68+
69+
it('rejects the precomposed NFC form, which really is 2 codepoints', () => {
70+
expect([...NFC_EE].length).toBe(2)
71+
expect(matchNeedleError(NFC_EE, ngram)).toMatch(/at least 3 characters/)
72+
})
73+
74+
it('rejects the empty needle whatever the tokenizer', () => {
75+
expect(matchNeedleError('', ngram)).toBeTypeOf('string')
76+
// A `standard` tokenizer imposes no ngram floor, but the empty string still
77+
// yields zero tokens and so still matches every row.
78+
expect(
79+
matchNeedleError('', { tokenizer: { kind: 'standard' } }),
80+
).toBeTypeOf('string')
81+
})
82+
83+
it('imposes no floor on a non-empty needle under a standard tokenizer', () => {
84+
expect(
85+
matchNeedleError('a', { tokenizer: { kind: 'standard' } }),
86+
).toBeUndefined()
87+
})
88+
89+
it('ignores non-string operands, leaving them to the encryption layer', () => {
90+
expect(matchNeedleError(42, ngram)).toBeUndefined()
91+
expect(matchNeedleError(null, ngram)).toBeUndefined()
92+
})
93+
})

packages/stack/src/schema/match-defaults.ts

Lines changed: 31 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -95,21 +95,43 @@ export function matchNeedleMinLength(opts: MatchIndexOpts): number | undefined {
9595
* Why a needle cannot be answered by this match index, or `undefined` when it
9696
* can. Callers throw their own error type with this as the reason.
9797
*
98-
* A needle shorter than the ngram tokenizer's `token_length` produces NO
99-
* ngrams, so its bloom filter is empty — and `stored_bf @> '{}'` is true for
100-
* every row ("contains nothing, contained by everything"). Such a query is
101-
* unanswerable rather than merely unmatched, so it must fail loudly instead of
102-
* silently returning the whole table.
98+
* A needle that tokenizes to NOTHING has an empty bloom filter, and
99+
* `stored_bf @> '{}'` is true for every row ("contains nothing, contained by
100+
* everything"). Such a query is unanswerable rather than merely unmatched, so
101+
* it must fail loudly instead of silently returning the whole table. Two ways
102+
* to tokenize to nothing:
103103
*
104-
* Shared by the v2 and v3 adapters: both build byte-identical bloom filters, so
105-
* the floor is a property of the index config, not of the adapter version.
104+
* - the empty needle, under ANY tokenizer;
105+
* - a needle shorter than the ngram tokenizer's `token_length`.
106+
*
107+
* The ngram floor counts Unicode CODEPOINTS, because that is what the tokenizer
108+
* counts. `needle.length` would count UTF-16 code units and wave through an
109+
* astral-plane needle: `'👍👍'` is 4 code units but only 2 codepoints, yields no
110+
* trigram, and (measured live) matched every row. Graphemes are the wrong unit
111+
* in the other direction — NFD `'éé'` is 4 codepoints but 2 grapheme clusters,
112+
* and does yield trigrams.
113+
*
114+
* Currently wired ONLY into the v3 Drizzle adapter (`eql/v3/drizzle/operators`).
115+
* It lives here, beside the shared match defaults, because v2 builds the same
116+
* bloom filters and needs the same floor — but v2's `like`/`ilike` path remains
117+
* unguarded and still fails open. Do not reuse this for v2 without first
118+
* measuring what its tokenizer actually receives: v2 needles carry SQL wildcards
119+
* (`'%ada%'`), so the floor may have to apply to the unwrapped term rather than
120+
* to the string the caller passed.
106121
*/
107122
export function matchNeedleError(
108123
needle: unknown,
109124
opts: MatchIndexOpts,
110125
): string | undefined {
111126
if (typeof needle !== 'string') return undefined
127+
128+
// Codepoints, not UTF-16 code units — see above.
129+
const length = [...needle].length
112130
const min = matchNeedleMinLength(opts)
113-
if (min === undefined || needle.length >= min) return undefined
114-
return `free-text search needs at least ${min} characters (the index tokenizer's token_length), but the search term ${JSON.stringify(needle)} has ${needle.length}. A shorter term produces no tokens and would match every row.`
131+
132+
if (length === 0) {
133+
return `free-text search needs a non-empty search term; an empty term produces no tokens and would match every row.`
134+
}
135+
if (min === undefined || length >= min) return undefined
136+
return `free-text search needs at least ${min} characters (the index tokenizer's token_length), but the search term ${JSON.stringify(needle)} has ${length}. A shorter term produces no tokens and would match every row.`
115137
}

0 commit comments

Comments
 (0)