Make the EXTRACT deparse injection test detect an injected table in any schema - #8821
Open
ibrahim halatci (ihalatci) wants to merge 1 commit into
Open
Make the EXTRACT deparse injection test detect an injected table in any schema#8821ibrahim halatci (ihalatci) wants to merge 1 commit into
ibrahim halatci (ihalatci) wants to merge 1 commit into
Conversation
The injection check looked for the table the payload creates with
to_regclass('<schema>.injected'), but the deparser fully qualifies task
SQL on purpose (PushEmptySearchPath), so Citus never sets search_path
on a worker for a SELECT task. A successful injection therefore creates
the table in the worker's default search_path, not in the test schema,
and the assertion would still report success.
Look the relation up by name in pg_class instead, which is how the
other run_command_on_workers checks in the suite do it, so the test
fails wherever the injected table lands.
Also add a positive control. The payload is expected to raise
invalid_parameter_value and the DO block swallows it, so the only
assertion was a negative one: if a future change stopped the expression
from being pushed down, nothing would run on a worker and the test
would keep passing without covering the deparse path. Asserting that
EXTRACT with a valid field still returns the right value pins that
path. This also gives the file real coverage on PG19, where the
injection block is skipped in favour of pg19.sql.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7c6370b2-06fd-4491-bf92-ecb811d34518
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8821 +/- ##
==========================================
- Coverage 88.73% 88.72% -0.01%
==========================================
Files 289 289
Lines 65013 65013
Branches 8203 8204 +1
==========================================
- Hits 57691 57686 -5
- Misses 4954 4955 +1
- Partials 2368 2372 +4 🚀 New features to boost your workflow:
|
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
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.
DESCRIPTION: Make the EXTRACT deparse injection test detect an injected table in any schema
Follow-up to the non-blocking review comments on #8804. Test-only, no product code changes.
1. The injection check could not detect the injection it guards against
The assertion looked for the table the payload creates via
to_regclass('extract_deparse.injected') IS NULL. That only finds an injectedtable if it lands in the test schema, and it never does.
The deparser fully qualifies task SQL on purpose -
PushEmptySearchPath(),"Set search_path to NIL so that all objects outside of pg_catalog will be
schema-prefixed" (
ruleutils_17.c:655,ruleutils_18.c:683,ruleutils_19.c:689). Because names arrive fully qualified, Citus does not senda
SET search_pathfor SELECT task execution. So a successfulCREATE TABLE injected()on a worker is created in the worker session's defaultsearch_path (
public), not inextract_deparse.The old assertion therefore returned
trueeven if the injection hadsucceeded - a silent false negative in a security regression test.
Now the relation is looked up by name instead, so the test fails wherever the
injected table lands:
This matches how the rest of the suite does worker-side existence checks, e.g.
citus_internal_distribute_object.sql(L181, L188, L190, L203, L210).The same problem and fix apply to
pg19.sql, which usedpg19_repack.injected.2. Added a positive control
The payload is expected to raise
invalid_parameter_value, and theDOblockswallows it, so the only assertion was a negative one. If a planner change ever
stopped the expression being pushed down, nothing would run on a worker,
injectedwould be absent, and the test would keep passing without covering thedeparse path at all.
This also gives
extract_deparse.sqlreal coverage on PG19, where the injectionblock is a no-op by design (guarded on
server_version_num < 190000) and thePG19 case is covered by
pg19.sql.3. Not changed: the
190000literalThe review asked whether a literal PG version is the convention. It is - there
is no macro or helper for version gating in regress tests. The only other use of
server_version_numundersrc/test/regress/sql/ismulti_orderby_limit_pushdown.sql:186, which uses the identical idiom:Left as-is.