fix: pin search_path on every eql_v2 function - #177
Merged
Conversation
Two related issues, one fix: 1. Lint: every `eql_v2.*` function inherits search_path from the caller, which Supabase's `function_search_path_mutable` lint flags (rule 0011). This is a privilege-escalation surface — a caller can shadow built-ins in a schema they own and steer function resolution to their code. 2. Portability: a single hard-coded `public.encrypt(...)` call in `compare_ore_block_u64_8_256_term` breaks on any Postgres where pgcrypto isn't installed in `public`. Supabase puts it in `extensions` by default, so EQL's ORE comparators error with "function public.encrypt(...) does not exist" the first time a range query runs. Fix: add `SET search_path = pg_catalog, public, extensions` to every `CREATE FUNCTION eql_v2.*` (180 functions). With that pin in place, internal calls don't need schema qualifiers — pg_catalog catches built-ins, public catches EQL's own objects + self-hosted pgcrypto, extensions catches Supabase-style pgcrypto. Drop the lone `public.` prefix on the encrypt() call so resolution flows through the pinned search_path. The schema list is intentionally broad rather than parameterised: - `pg_catalog` first defends against shadowing of built-ins - `public` covers self-hosted Postgres (pgcrypto default) and EQL's own cross-schema objects (eql_v2_encrypted, configuration table) - `extensions` covers Supabase, harmless when absent No `SECURITY DEFINER` functions exist in EQL, but pinning still matters for `INVOKER` functions that call other extensions' code.
Contributor
📝 WalkthroughWalkthroughThis PR consistently adds ChangesSearch-path hardening across eql_v2 functions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The required-tags doc validator looks back 50 lines from each CREATE FUNCTION for `@return`. Adding the SET search_path line to every function shifted the doc comments of follow-up `>` and `>=` overloads just outside that 50-line window, so they no longer inherited the first overload's `@return` tag. Add explicit `@return Boolean ...` lines to the four overloads that were silently relying on inheritance. (The remaining @PARAM warnings are pre-existing and don't fail the build.)
PostgreSQL refuses to inline LANGUAGE SQL functions that carry a SET clause. Several EQL functions (eql_v2.jsonb_contains, eql_v2.jsonb_contained_by, the chain through eql_v2.jsonb_array, etc.) are explicitly designed to be inlined so the planner sees the underlying @> / <@ operators and can use the GIN index on jsonb_array(column). The previous commit broke that — CI's containment tests fell back to seq scans. Strip the SET clause from all 37 LANGUAGE SQL function definitions (plus version.template). PL/pgSQL functions keep their SET — they're never inlinable anyway, so the lint hardening is a free win there. Trade-off: the SQL functions remain flagged by Supabase lint 0011 (function_search_path_mutable). Their bodies are already attack-resistant because every internal call is fully schema-qualified (eql_v2.*) and the only unqualified references are pg_catalog operators on jsonb. A later pass could swap operator references to OPERATOR(pg_catalog.@>) form to get full lint coverage without losing inlining, but that's a larger change and out of scope for unblocking the original Supabase pgcrypto-portability fix.
coderdan
marked this pull request as ready for review
May 5, 2026 02:47
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/encryptindex/functions.sql (1)
221-224:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnsafe dynamic SQL interpolation for table name.
format(... FROM %s ...)injectstable_nameverbatim. This allows SQL injection and malformed identifier execution. Use identifier-safe quoting (orregclass) for relation names.Proposed fix
- EXECUTE format( - 'SELECT COUNT(%I) FROM %s t WHERE %I->>%L = (SELECT id::TEXT FROM eql_v2_configuration WHERE state = %L)', - column_name, table_name, column_name, 'v', 'active' - ) + EXECUTE format( + 'SELECT COUNT(%1$I) FROM %2$I t WHERE %1$I->>%3$L = (SELECT id::TEXT FROM eql_v2_configuration WHERE state = %4$L)', + column_name, table_name, 'v', 'active' + ) INTO result;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/encryptindex/functions.sql` around lines 221 - 224, The dynamic SQL uses format(... FROM %s ...) with table_name injected verbatim, which is vulnerable to SQL injection; change the EXECUTE to use identifier-safe quoting (e.g. format(... FROM %I t ...)) or explicitly convert the table name to a regclass/quoted identifier (e.g. to_regclass or quote_ident) before formatting so table_name and column_name are passed as identifiers, not raw strings; update the EXECUTE call that references column_name and table_name accordingly.src/ore_cllw_u64_8/compare.sql (1)
54-56:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winWrong operand checked before computing
b_termLine 54 checks
eql_v2.has_ore_cllw_u64_8(a)again, sob_termextraction is gated by the wrong value. This can return wrong ordering or raise unexpectedly when only one side has an ORE term. Usebin that condition.Suggested fix
- IF eql_v2.has_ore_cllw_u64_8(a) THEN + IF eql_v2.has_ore_cllw_u64_8(b) THEN b_term := eql_v2.ore_cllw_u64_8(b); END IF;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ore_cllw_u64_8/compare.sql` around lines 54 - 56, The condition gating extraction of b_term is using the wrong operand; change the IF that now calls eql_v2.has_ore_cllw_u64_8(a) before computing b_term to instead call eql_v2.has_ore_cllw_u64_8(b), so that b_term := eql_v2.ore_cllw_u64_8(b) is only executed when b actually has the ORE term (refer to eql_v2.has_ore_cllw_u64_8, eql_v2.ore_cllw_u64_8, b_term, a, b).src/ore_cllw_var_8/compare.sql (1)
54-55:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUse
bin the second index-presence check.This branch still checks
has_ore_cllw_var_8(a), sob_termis skipped when onlybhas the ORE index andore_cllw_var_8(b)is called when onlyahas it. That makes the comparator return the wrong ordering or raise on valid inputs.Suggested fix
- IF eql_v2.has_ore_cllw_var_8(a) THEN + IF eql_v2.has_ore_cllw_var_8(b) THEN b_term := eql_v2.ore_cllw_var_8(b); END IF;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ore_cllw_var_8/compare.sql` around lines 54 - 55, The second presence-check incorrectly calls eql_v2.has_ore_cllw_var_8(a) instead of checking b, causing b_term to be skipped when only b has the ORE index; change that branch to check eql_v2.has_ore_cllw_var_8(b) and then call eql_v2.ore_cllw_var_8(b) to assign b_term (refer to the IF condition calling eql_v2.has_ore_cllw_var_8 and the assignment to b_term using eql_v2.ore_cllw_var_8, and the variables a and b).src/ore_block_u64_8_256/compare.sql (1)
49-50:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUse
bin the second index-presence check.This guard repeats
has_ore_block_u64_8_256(a). If onlybhas the index,b_termstaysNULLand the function can return equality; if onlyahas it,ore_block_u64_8_256(b)is called and raises instead of sorting the missing term correctly.Suggested fix
- IF eql_v2.has_ore_block_u64_8_256(a) THEN + IF eql_v2.has_ore_block_u64_8_256(b) THEN b_term := eql_v2.ore_block_u64_8_256(b); END IF;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ore_block_u64_8_256/compare.sql` around lines 49 - 50, The second index-presence guard mistakenly checks eql_v2.has_ore_block_u64_8_256(a) again; change it to check eql_v2.has_ore_block_u64_8_256(b) so b_term is only assigned via eql_v2.ore_block_u64_8_256(b) when b actually has the index (prevents NULL b_term or unintended exceptions when b lacks the index while a has it).src/operators/>.sql (1)
17-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd missing function attributes and documentation parameters to all
>operator functions.Four functions lack
IMMUTABLE STRICT PARALLEL SAFEattributes present on corresponding operators insrc/operators/=.sql. This inconsistency affects query optimization and NULL handling semantics. Additionally, the two overloaded function definitions at lines 65-72 and 87-94 are missing required@paramdocumentation tags, violating the coding guideline requiring parameter documentation.Proposed fix
CREATE FUNCTION eql_v2.gt(a eql_v2_encrypted, b eql_v2_encrypted) RETURNS boolean + IMMUTABLE STRICT PARALLEL SAFE SET search_path = pg_catalog, public, extensions AS $$ @@ CREATE FUNCTION eql_v2.">"(a eql_v2_encrypted, b eql_v2_encrypted) RETURNS boolean + IMMUTABLE STRICT PARALLEL SAFE SET search_path = pg_catalog, public, extensions AS $$ @@ --! `@brief` > operator for encrypted value and JSONB +--! `@param` a eql_v2_encrypted Left operand (encrypted value) +--! `@param` b jsonb Right operand (will be cast to eql_v2_encrypted) --! `@return` Boolean True if a > b --! `@see` eql_v2.">"(eql_v2_encrypted, eql_v2_encrypted) CREATE FUNCTION eql_v2.">"(a eql_v2_encrypted, b jsonb) RETURNS boolean + IMMUTABLE STRICT PARALLEL SAFE SET search_path = pg_catalog, public, extensions AS $$ @@ --! `@brief` > operator for JSONB and encrypted value +--! `@param` a jsonb Left operand (will be cast to eql_v2_encrypted) +--! `@param` b eql_v2_encrypted Right operand (encrypted value) --! `@return` Boolean True if a > b --! `@see` eql_v2.">"(eql_v2_encrypted, eql_v2_encrypted) CREATE FUNCTION eql_v2.">"(a jsonb, b eql_v2_encrypted) RETURNS boolean + IMMUTABLE STRICT PARALLEL SAFE SET search_path = pg_catalog, public, extensions AS $$🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/operators/`>.sql around lines 17 - 24, The eql_v2.gt function(s) are missing the required function attributes and some overloads lack `@param` docs; update every eql_v2.gt(...) CREATE FUNCTION to include the attributes IMMUTABLE STRICT PARALLEL SAFE (e.g., add them after the RETURNS clause so the signature reads RETURNS boolean IMMUTABLE STRICT PARALLEL SAFE AS $$ ... $$ LANGUAGE plpgsql;) and add documentation comment blocks for the overloaded eql_v2.gt definitions in this file that include `@param` tags for both parameters (use the exact parameter names a and b) to match the coding guideline.src/operators/compare.sql (1)
47-62:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPre-existing:
STRICTmakes the NULL-handling block in the body unreachable
STRICTcauses PostgreSQL to short-circuit and returnNULLautomatically when either argument isNULL, so lines 52–62 are never reached. As a result:
eql_v2.compare(NULL, NULL)→NULL, not0eql_v2.compare(NULL, val)→NULL, not-1This pre-dates this PR, but it is worth resolving because btree operator classes that rely on
eql_v2.comparemay behave inconsistently forNULLinputs. DropSTRICTto allow the body's NULL logic to execute, or remove the NULL guards entirely ifNULLinputs should genuinely returnNULL.🐛 Proposed fix — drop STRICT to activate the existing NULL logic
CREATE FUNCTION eql_v2.compare(a eql_v2_encrypted, b eql_v2_encrypted) RETURNS integer - IMMUTABLE STRICT PARALLEL SAFE + IMMUTABLE PARALLEL SAFE SET search_path = pg_catalog, public, extensions AS $$🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/operators/compare.sql` around lines 47 - 62, The function declaration includes the STRICT attribute which causes PostgreSQL to return NULL before the body runs, making the NULL-handling block in eql_v2.compare unreachable; remove the STRICT token from the function signature in src/operators/compare.sql so the procedure's internal NULL checks (the IF a IS NULL / IF b IS NULL branches) execute and return 0/−1/1 as intended.
🧹 Nitpick comments (1)
src/blake3/functions.sql (1)
66-74: ⚡ Quick winRefactor trivial wrapper functions to use
LANGUAGE SQLBoth
eql_v2.has_blake3overloads are single-expression functions without procedural logic and should be converted to SQL language per coding guidelines. ConvertLANGUAGE plpgsqlblocks toSELECTstatements withLANGUAGE SQL.Applies to: lines 66-74 and 86-94
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/blake3/functions.sql` around lines 66 - 74, Convert both eql_v2.has_blake3 PL/pgSQL single-statement functions to SQL-language functions: replace the PL/pgSQL block with a single SELECT expression that returns the boolean (e.g. "SELECT val ->> 'b3' IS NOT NULL;"), keep the same signature, attributes (IMMUTABLE, STRICT, PARALLEL SAFE, SET search_path ...) and change LANGUAGE plpgsql to LANGUAGE SQL for both eql_v2.has_blake3 overloads so they are simple SQL functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/operators/`>.sql:
- Around line 62-65: Add missing Doxygen `@param` tags for the `>` overloads:
update the comment block above the function declaration `CREATE FUNCTION
eql_v2.">"(a eql_v2_encrypted, b jsonb)` to include `@param a` and `@param b`
descriptions (type/context and purpose), and do the same for the other overload
declared around the 84-87 block (the alternate `>` overload for eql_v2_encrypted
parameters) so both blocks contain `@brief`, `@param` for each parameter, and
`@return`.
In `@src/operators/`>=.sql:
- Around line 61-63: The docs for the two ">=" operator overloads are missing
`@param` entries; update the Doxygen comment blocks for the overloads labeled
">="(eql_v2_encrypted, eql_v2_encrypted) and the second overload referenced
around the same file section to include `@param` a and `@param` b (describe
types/roles: left encrypted value and right encrypted/JSONB value) along with
the existing `@brief` and `@return` so the comments satisfy the project's required
tags.
In `@src/operators/sort.sql`:
- Line 483: The sort_compare function currently concatenates the filter
parameter into a dynamic SQL string and passes it into order_by_compare which
executes it (two-layer dynamic SQL injection via the filter variable); change
sort_compare to stop concatenating raw filter text — either validate/whitelist
the filter expression (only allow specific column names/operators), or build the
dynamic SQL using proper formatting functions (e.g., use format('%I',
column_name) for identifiers and pass literal values as parameters) and pass
parameters into EXECUTE USING instead of string interpolation; update
order_by_compare to accept a safely constructed query or parameter list rather
than raw SQL, and consider tightening privileges by documenting SECURITY
INVOKER/DEFINER intent and REVOKE EXECUTE on function eql_v2.sort_compare(text,
text, text, text, text) FROM PUBLIC if callers must be restricted.
In `@src/ore_block_u64_8_256/functions.sql`:
- Around line 144-145: The function sets "SET search_path = pg_catalog, public,
extensions" but calls the crypto function unqualified as encrypt(...), which
allows search_path hijacking; update every unqualified call to encrypt(bytea,
bytea, text) to fully qualified pgcrypto.encrypt(...) (e.g., replace
encrypt(...) with pgcrypto.encrypt(...)) or alternatively remove "public" from
the search_path; ensure you change both occurrences of the unqualified encrypt()
calls in this SQL function so the pgcrypto implementation is invoked directly.
---
Outside diff comments:
In `@src/encryptindex/functions.sql`:
- Around line 221-224: The dynamic SQL uses format(... FROM %s ...) with
table_name injected verbatim, which is vulnerable to SQL injection; change the
EXECUTE to use identifier-safe quoting (e.g. format(... FROM %I t ...)) or
explicitly convert the table name to a regclass/quoted identifier (e.g.
to_regclass or quote_ident) before formatting so table_name and column_name are
passed as identifiers, not raw strings; update the EXECUTE call that references
column_name and table_name accordingly.
In `@src/operators/`>.sql:
- Around line 17-24: The eql_v2.gt function(s) are missing the required function
attributes and some overloads lack `@param` docs; update every eql_v2.gt(...)
CREATE FUNCTION to include the attributes IMMUTABLE STRICT PARALLEL SAFE (e.g.,
add them after the RETURNS clause so the signature reads RETURNS boolean
IMMUTABLE STRICT PARALLEL SAFE AS $$ ... $$ LANGUAGE plpgsql;) and add
documentation comment blocks for the overloaded eql_v2.gt definitions in this
file that include `@param` tags for both parameters (use the exact parameter names
a and b) to match the coding guideline.
In `@src/operators/compare.sql`:
- Around line 47-62: The function declaration includes the STRICT attribute
which causes PostgreSQL to return NULL before the body runs, making the
NULL-handling block in eql_v2.compare unreachable; remove the STRICT token from
the function signature in src/operators/compare.sql so the procedure's internal
NULL checks (the IF a IS NULL / IF b IS NULL branches) execute and return 0/−1/1
as intended.
In `@src/ore_block_u64_8_256/compare.sql`:
- Around line 49-50: The second index-presence guard mistakenly checks
eql_v2.has_ore_block_u64_8_256(a) again; change it to check
eql_v2.has_ore_block_u64_8_256(b) so b_term is only assigned via
eql_v2.ore_block_u64_8_256(b) when b actually has the index (prevents NULL
b_term or unintended exceptions when b lacks the index while a has it).
In `@src/ore_cllw_u64_8/compare.sql`:
- Around line 54-56: The condition gating extraction of b_term is using the
wrong operand; change the IF that now calls eql_v2.has_ore_cllw_u64_8(a) before
computing b_term to instead call eql_v2.has_ore_cllw_u64_8(b), so that b_term :=
eql_v2.ore_cllw_u64_8(b) is only executed when b actually has the ORE term
(refer to eql_v2.has_ore_cllw_u64_8, eql_v2.ore_cllw_u64_8, b_term, a, b).
In `@src/ore_cllw_var_8/compare.sql`:
- Around line 54-55: The second presence-check incorrectly calls
eql_v2.has_ore_cllw_var_8(a) instead of checking b, causing b_term to be skipped
when only b has the ORE index; change that branch to check
eql_v2.has_ore_cllw_var_8(b) and then call eql_v2.ore_cllw_var_8(b) to assign
b_term (refer to the IF condition calling eql_v2.has_ore_cllw_var_8 and the
assignment to b_term using eql_v2.ore_cllw_var_8, and the variables a and b).
---
Nitpick comments:
In `@src/blake3/functions.sql`:
- Around line 66-74: Convert both eql_v2.has_blake3 PL/pgSQL single-statement
functions to SQL-language functions: replace the PL/pgSQL block with a single
SELECT expression that returns the boolean (e.g. "SELECT val ->> 'b3' IS NOT
NULL;"), keep the same signature, attributes (IMMUTABLE, STRICT, PARALLEL SAFE,
SET search_path ...) and change LANGUAGE plpgsql to LANGUAGE SQL for both
eql_v2.has_blake3 overloads so they are simple SQL functions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05c1e88d-b2c0-49c0-8bdb-68c3d83310e3
📒 Files selected for processing (37)
src/blake3/compare.sqlsrc/blake3/functions.sqlsrc/bloom_filter/functions.sqlsrc/common.sqlsrc/config/constraints.sqlsrc/config/functions.sqlsrc/config/functions_private.sqlsrc/encrypted/aggregates.sqlsrc/encrypted/constraints.sqlsrc/encrypted/functions.sqlsrc/encrypted/hash.sqlsrc/encryptindex/functions.sqlsrc/hmac_256/compare.sqlsrc/hmac_256/functions.sqlsrc/jsonb/functions.sqlsrc/operators/->.sqlsrc/operators/->>.sqlsrc/operators/<.sqlsrc/operators/<=.sqlsrc/operators/<>.sqlsrc/operators/<@.sqlsrc/operators/=.sqlsrc/operators/>.sqlsrc/operators/>=.sqlsrc/operators/@>.sqlsrc/operators/compare.sqlsrc/operators/order_by.sqlsrc/operators/sort.sqlsrc/operators/~~.sqlsrc/ore_block_u64_8_256/compare.sqlsrc/ore_block_u64_8_256/functions.sqlsrc/ore_block_u64_8_256/operators.sqlsrc/ore_cllw_u64_8/compare.sqlsrc/ore_cllw_u64_8/functions.sqlsrc/ore_cllw_var_8/compare.sqlsrc/ore_cllw_var_8/functions.sqlsrc/ste_vec/functions.sql
tobyhede
approved these changes
May 5, 2026
4 tasks
Addresses the search-path-hijacking concern flagged by CodeRabbit on the
earlier change in this PR.
Before: SET search_path = pg_catalog, public, extensions
After: SET search_path = pg_catalog, extensions, public
The previous order let any role with CREATE on `public` shadow pgcrypto's
encrypt(bytea, bytea, text) by planting a same-signature function in
`public`, which would then resolve before the real `encrypt()` in
`extensions` on Supabase deployments. None of the affected functions are
SECURITY DEFINER, so the blast radius is the calling session, but it's
defence-in-depth worth fixing.
With the new order:
- pg_catalog still resolves built-ins first (and is implicit anyway).
- extensions resolves pgcrypto on Supabase before public can shadow it.
- public still resolves pgcrypto on self-hosted (where extensions is
typically absent) and EQL's own `eql_v2_encrypted` /
`eql_v2_configuration` cross-schema objects.
Also fills in @PARAM tags on the four `>` and `>=` jsonb-cross-type
overloads that CodeRabbit flagged as missing required Doxygen entries.
143 SET clauses reordered across 34 files; 8 new @PARAM lines on the
operator overloads.
Contributor
Author
|
Pushed
Full local test pass: 33 sqlx test targets, 0 failures. Splinter findings unchanged at 41 (40 Out of scope, tracked separately:
|
This was referenced May 5, 2026
Contributor
Author
coderdan
added a commit
that referenced
this pull request
May 5, 2026
- Fix @file Doxygen tag (cross_type_opfamily.sql → cross_type_operator_class.sql). Leftover from the original filename. - Convert the three opfamily support functions from LANGUAGE plpgsql + SET search_path to LANGUAGE sql + schema-qualified bodies (no SET). These functions are called by the planner on every index comparison or hash probe — inlinability matters here in a way it doesn't for the user-facing operator wrappers in src/operators/{=,<,...}.sql. PG refuses to inline a LANGUAGE SQL function that carries a SET clause (per #177's investigation), so the usual EQL convention of plpgsql + SET would force a full function call per index op. Schema-qualifying every reference inside the bodies (eql_v2.compare, public.eql_v2_encrypted) gives us inlining AND keeps name resolution deterministic regardless of caller search_path. Trade-off: pins the references to current schema locations — eql_v2_encrypted lives in `public` today; will need a touch when #180 (move types into eql_v2) lands. A short comment in the file documents this rationale so the divergence from the surrounding plpgsql convention is intentional and reviewable. EXPLAIN cost on the bare-jsonb equality is unchanged (9.18 — same Index Scan + Index Cond as before). Tests still pass.
This was referenced May 7, 2026
4 tasks
tobyhede
pushed a commit
that referenced
this pull request
Jun 20, 2026
…ypto-portability fix: pin search_path on every eql_v2 function
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two related issues, one (mostly) shared fix:
public.encrypt(...)call incompare_ore_block_u64_8_256_term(src/ore_block_u64_8_256/functions.sql) breaks on any Postgres where pgcrypto isn't installed inpublic. Supabase puts it inextensionsby default, so EQL's ORE comparators error withfunction public.encrypt(bytea, bytea, unknown) does not existthe first time a range query runs against a Supabase DB.function_search_path_mutable): everyeql_v2.*function inheritssearch_pathfrom the caller, which the lint flags. This is also a privilege-escalation surface — a caller can shadow built-ins in a schema they own and steer function resolution to their code.Before:
After:
Fix
SET search_path = pg_catalog, extensions, publicto 143 PL/pgSQLCREATE FUNCTION eql_v2.*definitions.public.prefix on theencrypt(...)call so resolution flows through the pinnedsearch_path(finds pgcrypto inextensionson Supabase, inpublicon self-hosted).The schema list is intentionally broad rather than parameterised:
pg_catalogfirst defends against shadowing of built-ins.extensionsnext so pgcrypto resolves there on Supabase before any user-writablepubliccan shadow it. Schema is harmless when absent (vanilla PG).publiclast for self-hosted Postgres (pgcrypto's default location) and EQL's own cross-schema objects (eql_v2_encrypted,eql_v2_configuration).The original commit had
publicbeforeextensions; reordered inf05c909after CodeRabbit flagged the search-path-hijacking surface that ordering created. Thepgcrypto.encrypt(...)qualifier CodeRabbit suggested isn't portable — pgcrypto's functions live in whatever schemaCREATE EXTENSIONplaced them, and there's no schema literally namedpgcrypto.Same commit also added missing
@paramDoxygen tags on the four>and>=jsonb cross-type overloads (also CodeRabbit feedback).Known gap: 37 LANGUAGE SQL functions remain unpinned
Pinning every function via
SETinitially broke containment queries —eql_v2.jsonb_containsandeql_v2.jsonb_contained_byfell back to seq scans instead of using the GIN index onjsonb_array(column). PostgreSQL refuses to inline a LANGUAGE SQL function that carries aSETclause, and EQL's containment helpers are explicitly designed to be inlined so the planner sees the underlying@>/<@operators and can match the GIN index.So commit
bc7976dreverted SET on all 37 LANGUAGE SQL functions. PL/pgSQL functions keep theirs — they're never inlinable anyway, so the lint hardening is a free win there.Practical impact:
encryptcall is PL/pgSQL and retains its SET clause.eql_v2.*schema-qualified, and the only unqualified pieces are pg_catalog operators on jsonb (which are implicitly resolved against pg_catalog first).Closing the remaining lint warnings cleanly would require swapping operator references in those bodies to
OPERATOR(pg_catalog.@>)form, which makes pinning safe to add without losing inlining. That's a larger change and out of scope for unblocking the original Supabase pgcrypto-portability fix.Residual hijacking surface on self-hosted
Even with
extensionsbeforepublic, vanilla self-hosted PG installs pgcrypto intopublic, sopublicstill has to be in the search_path to resolve it. The proper structural fix is to moveeql_v2_encryptedandeql_v2_configurationintoeql_v2sopubliccan be dropped from the search_path entirely. Tracked in #180 — out of scope here.Follow-ups
eql_v2_encrypted/eql_v2_configurationintoeql_v2schema (closes the residual hijacking surface on self-hosted).filterparameter ineql_v2.sort_compare()(preexisting; CodeRabbit flagged because this PR touched the function'sSETclause).Verification
tasks/build.shrebuilds cleanly.release/cipherstash-encrypt.sqlandrelease/cipherstash-encrypt-supabase.sqlregenerate with 143SET search_path = pg_catalog, extensions, publicclauses and zeropublic.encryptreferences.splinter@55db5b1f) findings unchanged after the reorder: 40function_search_path_mutable(the LANGUAGE SQL helpers above) + 1extension_in_public(pgcrypto). No new findings introduced.required-tags.sh) reportsErrors: 0(3 pre-existing@paramwarnings, not introduced by this PR).CREATE FUNCTION ... AS $$(e.g.bytea_eq)RETURNS x \n IMMUTABLE STRICT \n AS $$BEGIN ATOMICbody (e.g.text_to_ore_block_u64_8_256_term,check_encrypted)RETURNS x AS $$ ...collapsing onto one lineCI timing
PG17 job ran ~33 min vs main's recent ~28 min baseline (April 23) and ~22 min (April 9). Within main's own run-to-run variance, but at the upper end. Plausibly a small per-call overhead from search_path save/restore in tight test loops; could also be runner noise.
Summary by CodeRabbit