fix(security): restrict get_batch_cursor(extra_where) to pgque_admin (#108) by NikolayS · Pull Request #169 · NikolayS/pgque

NikolayS · 2026-05-02T06:20:18Z

Summary

Closes the surface area of issue #108 by restricting pgque.get_batch_cursor (both 3-arg and 4-arg overloads) to pgque_admin only.

Changes:

add tests/test_security_get_batch_cursor.sql covering PUBLIC blocked, reader/writer blocked, and admin allowed
explicitly revoke both get_batch_cursor overloads from PUBLIC, pgque_reader, and pgque_writer in sql/pgque-additions/roles.sql and generated sql/pgque.sql
keep documentation minimal: docs/reference.md only says both overloads are admin-only

Rebase notes:

rebased on current main
resolved grant-block conflict with the newer insert_event_bulk revokes by keeping both protections

Verification:

local install of sql/pgque.sql into fresh database
tests/test_security_get_batch_cursor.sql passed
tests/test_pgque_roles.sql passed
checks re-running after PUBLIC coverage update

Add tests/test_security_get_batch_cursor.sql to lock in the grant posture for pgque.get_batch_cursor (both 3-arg and 4-arg overloads): - pgque_reader / pgque_writer must get insufficient_privilege (42501) on either overload. - pgque_admin (or members) can still invoke both overloads. - The 4-arg overload's extra_where remains a raw SQL fragment; the test runs a UNION ALL forgery probe under admin to document the threat model: the chosen fix is *boundary lockdown*, not predicate parsing (SPECx Key Design Rule #2 — "the PgQ engine is sacred"). Note: PR #163 already revoked PUBLIC EXECUTE and never re-granted get_batch_cursor to reader/writer, so the assertions are green at HEAD. Subsequent commits add an explicit revoke + warning comment + docs note for defense-in-depth and to prevent regression. Wires into tests/run_all.sql alongside the other test_security_*.sql files. Refs #108

get_batch_cursor's 4-arg overload concatenates i_extra_where verbatim into the dynamic cursor body. A caller that controls extra_where can inject arbitrary predicate SQL or use "false UNION ALL SELECT ..." to forge event rows returned to application code. The PgQ engine body is sacred (SPECx Key Design Rule #2), so this is fixed at the boundary, not by parsing predicates: - Add explicit "revoke execute on function pgque.get_batch_cursor(...) from public, pgque_reader, pgque_writer" for both overloads. PR #163 already produced this posture by deny-by-default; the explicit revoke makes intent visible and prevents regression if a future grant block accidentally re-exposes the function. - Add a SECURITY comment block above the function definition warning that i_extra_where is raw SQL and must never receive user input. Access remains via "grant execute on all functions in schema pgque to pgque_admin" earlier in the same grants block. Refs #108

Add a security note to docs/reference.md for both get_batch_cursor overloads explaining that extra_where is a raw SQL fragment (not a parameter), the UNION-ALL row-forgery risk, and that access is admin-only — never pass user-controlled text. Steer application code toward pgque.receive instead. Refs #108

NikolayS

REV review — PR #169 — verdict: ready to merge (posted as COMMENT because GitHub blocks self-approval)

Note on tooling: REV (gitlab.com/postgres-ai/rev) is a GitLab-only Claude Code plugin (uses glab, validates gitlab.com URLs in its /review-mr command). It cannot be pointed at a GitHub PR directly. I cloned REV, loaded its agent prompts (security-reviewer, bug-hunter, test-analyzer, guidelines-checker, docs-reviewer), and applied the same checklist manually against this PR using GitHub MCP for diff/file access. SOC2-tagged items were skipped per maintainer instruction.

Verdict

The intent of #108 is correctly enforced. pgque.get_batch_cursor is now unreachable by PUBLIC, pgque_reader, and pgque_writer for both the 3-arg and 4-arg overloads, while pgque_admin retains execute via the prior grant execute on all functions in schema pgque to pgque_admin (admin is intentionally not listed in the new revoke statements). The extra_where SQL-interpolation vector (CWE-89) remains by design and is now only reachable from a trusted role; this is documented and consistent with PgQ's posture. Approving in spirit; downgraded to COMMENT only because GitHub disallows authors from self-approving.

Security-specific sanity check (the 5 items requested)

Both overloads revoked from PUBLIC, reader, writer — confirmed in sql/pgque-additions/roles.sql (new lines at end) and mirrored in sql/pgque.sql:4356-4357. The trailing revoke execute on all functions in schema pgque from public; at sql/pgque.sql:5084 provides defense-in-depth for PUBLIC.
pgque_admin retains execute — via the broad grant at sql/pgque.sql:4337 (grant execute on all functions in schema pgque to pgque_admin;), which executes before the new revokes (line 4356) and is never followed by a revoke that targets pgque_admin for get_batch_cursor. Path: implicit retention (broad grant + no admin-targeted revoke), not an explicit per-overload grant. Verified by grepping the entire grants block for get_batch_cursor. Test C confirms behaviorally.
Test asserts all 4 paths with errcode = '42501' — A0.1/A0.2 (PUBLIC, both overloads), A1/A2 (reader, both overloads), B1/B2 (writer, both overloads), C1/C2 (admin success, both overloads). All negative paths trap insufficient_privilege and assert sqlstate = '42501' explicitly.
No new SECURITY DEFINER functions — the diff is grant/revoke + comments + tests + docs only. CLAUDE.md's SET search_path = pgque, pg_catalog hard rule is N/A for this PR.
Generated sql/pgque.sql matches sql/pgque-additions/roles.sql — yes, the appended block in sql/pgque.sql:4354-4358 is byte-equivalent to the new addition in sql/pgque-additions/roles.sql.

Findings

Must-fix (blocking): 0

Nice-to-have (non-blocking): 2

docs/reference.md: the 4-arg overload entry could explicitly state that extra_where is interpolated as raw SQL inside the cursor and must never receive untrusted input. The PR keeps the doc minimal ("admin-only") which is acceptable, but a one-line "trusted SQL only — admin-only by grants" caveat would help future maintainers.
tests/test_security_get_batch_cursor.sql: Test C creates security_cursor_q and a consumer using superuser-level calls. If Test C fails before reaching the cleanup select pgque.unregister_consumer(...) / drop_queue(...), the queue leaks into subsequent tests in run_all.sql. Consistent with other tests in the suite, so not a regression — just a general hardening idea (e.g., wrapping setup/cleanup in a savepoint or running in its own transaction).

Per-agent summary (REV equivalents)

Agent	Result
Security Reviewer (Opus)	NO_FINDINGS at HIGH/CRITICAL. The `extra_where` injection vector is left intentionally and is no longer reachable by reader/writer; documented.
Bug Hunter (Opus)	No runtime/logic bugs. Cursor lifecycle in Test C is correct (both `security_cursor_admin_c3` and `security_cursor_admin_c4` are explicitly `close`d).
Test Analyzer (Sonnet)	Strong coverage: 8/8 cells (3 negative roles × 2 overloads + admin × 2 overloads). Explicit `42501` assertion.
Guidelines Checker (Sonnet)	Compliant: lowercase SQL, snake_case, schema-qualified, Apache-2.0 header in new test, Conventional Commit title.
Docs Reviewer (Sonnet)	Minimal but accurate. See nice-to-have above.

SOC2 items: explicitly skipped per maintainer instruction.

— review generated by Claude (Opus 4.7) using the REV checklist applied manually (REV is GitLab-only and could not be invoked directly against this GitHub PR).

Generated by Claude Code

* docs: warn extra_where is trusted SQL, not a parameter (#108) Documents the trusted-SQL-fragment contract on pgque.get_batch_cursor(..., extra_where text). Complements PR #169 which restricted the function to pgque_admin: PR #169 closed the role-grant gap, this change closes the documentation gap that #108 calls out as the minimum bar. The 4-arg overload concatenates extra_where verbatim into the cursor's select, allowing arbitrary predicates (and UNION ALL row forgery) for any caller that controls the fragment. Behavior is inherited from upstream PgQ; admin-only grants make it acceptable in PgQue, but admin callers must still treat the parameter as trusted SQL and never pass user-controlled text. Adds: - docs/reference.md: blockquote security note under the 4-arg entry, pointing to get_batch_events() for app-driven filtering. - sql/pgque.sql: header SECURITY block on the 4-arg function and a cross-reference on the i_extra_where parameter doc line. No behavior change. * docs: get_batch_cursor extra_where is trusted SQL Inject the SECURITY contract into pgque.get_batch_cursor(4-arg) at transform.sh time so the warning travels with the function body and survives `bash build/transform.sh` regeneration. Mirror the warning in docs/reference.md as a user-facing blockquote. The 4-arg overload concatenates extra_where verbatim into dynamic SQL. Both overloads are admin-only via the existing grants block; this commit closes the documentation gap so admin callers know the parameter is a trusted-SQL fragment, not user input. Also drops the previously inlined "See issue #108" cross-references and fixes "parameterised" -> "parameterized" to match the rest of the docs corpus. Red/green TDD evidence: running `bash build/transform.sh` against the prior PR head (commit fbebbf2) wiped the hand-edited SECURITY block, demonstrating the regen-rot trap. With the new awk step in place, the same run preserves the SECURITY header and parameter-doc warning verbatim; the awk END block fails loudly if either insertion point goes missing on a future PgQ resync. Re-run is idempotent. Whitespace deltas in sql/pgque.sql at lines 4362 and EOF are pre-existing assembly artifacts from echo "" calls in transform.sh's per-file concatenation; they appear any time anyone runs the regen and were last hand-trimmed by prior commits. Out of scope for this PR. * docs: tighten get_batch_cursor SECURITY contract Two REV-driven follow-ups on the prior commit: 1. Assembly verification: assert the SECURITY contract reaches the final install file. The per-function awk patch already fails the build if the upstream PgQ signature/parameter line drifts, but did not pin that the resulting comment block survives the assembly step. A future change that strips comments or reorders sections would silently ship without the warning. Mirrors the existing pgque-api / pg_notify presence checks. 2. Admin-only context in the parameter doc: the SECURITY block above the function spells out the admin-only mitigation, but the parameter doc line is what `\df+` and IDE hover tools surface in isolation. Append "Function is admin-only." so the parameter contract self-stands. --------- Co-authored-by: Claude <noreply@anthropic.com>

NikolayS force-pushed the fix/get-batch-cursor-grants-108 branch from 777287b to d5f0f43 Compare May 2, 2026 06:55

claude added 2 commits May 2, 2026 17:10

NikolayS force-pushed the fix/get-batch-cursor-grants-108 branch 6 times, most recently from f424394 to 4590724 Compare May 2, 2026 17:18

NikolayS force-pushed the fix/get-batch-cursor-grants-108 branch from 4590724 to 3604879 Compare May 2, 2026 17:18

NikolayS commented May 2, 2026

View reviewed changes

NikolayS force-pushed the fix/get-batch-cursor-grants-108 branch from 744568d to 3604879 Compare May 2, 2026 17:46

NikolayS merged commit 4544403 into main May 2, 2026
15 checks passed

NikolayS deleted the fix/get-batch-cursor-grants-108 branch May 2, 2026 18:47

NikolayS mentioned this pull request May 3, 2026

docs: warn extra_where is trusted SQL fragment #176

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(security): restrict get_batch_cursor(extra_where) to pgque_admin (#108)#169

fix(security): restrict get_batch_cursor(extra_where) to pgque_admin (#108)#169
NikolayS merged 3 commits intomainfrom
fix/get-batch-cursor-grants-108

NikolayS commented May 2, 2026 •

edited

Loading

Uh oh!

NikolayS left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

NikolayS commented May 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

NikolayS left a comment

Choose a reason for hiding this comment

REV review — PR #169 — verdict: ready to merge (posted as COMMENT because GitHub blocks self-approval)

Verdict

Security-specific sanity check (the 5 items requested)

Findings

Per-agent summary (REV equivalents)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

NikolayS commented May 2, 2026 •

edited

Loading