fix: avoid privileged refresh of h3 distance dependents#204
Conversation
📝 WalkthroughWalkthroughThe upgrade from h3 4.2.3 to 4.5.0 shifts maintenance responsibility for Changesh3index_distance() Operator Upgrade: Manual Maintenance Workflow
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
h3/sql/updates/h3--4.2.3--4.5.0.sql (1)
50-65:⚠️ Potential issue | 🟠 MajorSecurity: the remaining
REINDEXloop can execute user-controlled index expressions/predicates (lines 50–65)PostgreSQL
REINDEXrebuilds indexes by evaluating expression-index expressions and partial-index predicates, and it runs those evaluations with the privileges of the role executingREINDEX. This means the loop thatREINDEX INDEXes every matching btree index using opclassh3index_opscan re-run user-controlled logic if any such index is expression and/or partial. Filter the candidate indexes to exclude expression/partial ones (e.g., requirepg_index.indexprs IS NULLandpg_index.indpred IS NULL) or otherwise ensure the loop can’t target them.🤖 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 `@h3/sql/updates/h3--4.2.3--4.5.0.sql` around lines 50 - 65, The REINDEX loop selects btree indexes using opclass h3index_ops but doesn't exclude expression or partial indexes, so update the FOR ... SELECT query (the one joining pg_index i, pg_class ci, pg_am am, pg_opclass opc and returning r.idx) to filter out expression and partial indexes by requiring i.indexprs IS NULL and i.indpred IS NULL before looping and executing REINDEX INDEX on r.idx; this ensures the EXECUTE pg_catalog.format('REINDEX INDEX %s', r.idx) call cannot evaluate user-controlled index expressions or predicates.
🤖 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.
Outside diff comments:
In `@h3/sql/updates/h3--4.2.3--4.5.0.sql`:
- Around line 50-65: The REINDEX loop selects btree indexes using opclass
h3index_ops but doesn't exclude expression or partial indexes, so update the FOR
... SELECT query (the one joining pg_index i, pg_class ci, pg_am am, pg_opclass
opc and returning r.idx) to filter out expression and partial indexes by
requiring i.indexprs IS NULL and i.indpred IS NULL before looping and executing
REINDEX INDEX on r.idx; this ensures the EXECUTE pg_catalog.format('REINDEX
INDEX %s', r.idx) call cannot evaluate user-controlled index expressions or
predicates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 63863f0b-8879-4c59-8da7-e50e0c8d4d43
⛔ Files ignored due to path filters (1)
h3/test/expected/extension.outis excluded by!**/*.out
📒 Files selected for processing (2)
h3/sql/updates/h3--4.2.3--4.5.0.sqlh3/test/sql/extension.sql
1 participant
Motivation
UPDATE,REINDEX, andREFRESH MATERIALIZED VIEWon catalog-discovered objects that depend onh3index_distance/<->, which can evaluate user-controlled expressions as the upgrade-running user and enable privilege escalation.Description
h3/sql/updates/h3--4.2.3--4.5.0.sqlthat walkedpg_dependand performed automaticUPDATE/REINDEX/REFRESH, and replace it with a documented comment instructing administrators to perform such maintenance explicitly after upgrade.CREATE OR REPLACE FUNCTION h3index_distance(...)and the index-repair logic that do not evaluate user expressions during upgrade.h3/test/sql/extension.sqlto perform the requiredREINDEX,UPDATE(withALTER TABLE ... DISABLE TRIGGER USER/ENABLEwhere applicable), andREFRESH MATERIALIZED VIEWcommands explicitly afterALTER EXTENSIONinstead of relying on the upgrade script.h3/test/expected/extension.outto include the explicit post-upgrade maintenance commands executed by the test.Testing
rgto confirm removal of the catalog-driven maintenance (searches forsession_replication_role,distance_dependents,REFRESH MATERIALIZED VIEW, and theUPDATE ... SETpattern) and presence of the explicit post-upgrade commands, which matched expectations.git diff --checkand local sanity checks which reported no whitespace or diff errors.cmake -B build -DCMAKE_BUILD_TYPE=Releasebut the build andctest/pg_regresscould not complete becauseFetchContentfailed to download upstream H3 from GitHub due to a network/proxyHTTP 403, so end-to-end regression execution was blocked by the environment.Codex Task