fix!: wire up the Homebrew tap and close three classifier bypasses - #5
Merged
Merged
Conversation
Why [workspace.metadata.dist] declared a Homebrew tap and listed homebrew as an installer, but never set publish-jobs, so cargo-dist never generated a job that actually pushes Formula/ownpg.rb anywhere. release.yml itself only built artifacts (plan, build-local-artifacts, build-global-artifacts); it never created a GitHub Release or ran any publish step, so devops-infinity/homebrew-tap would have stayed empty through a real release. What changed Added publish-jobs = ["homebrew"] and regenerated release.yml with `dist generate --mode ci`, which added the host, publish-homebrew- formula, and announce jobs cargo-dist needs to create a GitHub Release and push the formula to the tap. Every action in the regenerated file is pinned back to a commit SHA, and the three curl-pipe-sh installer steps (cargo-dist, rustup, cargo-cyclonedx) are back to fetch-then- verify-then-run, matching this repository's own supply-chain rules; `dist generate` does not produce either on its own. Risk The publish-homebrew-formula job needs a HOMEBREW_TAP_TOKEN repository secret (a token with write access to devops-infinity/homebrew-tap) that does not exist yet. Without it, the job will fail at the push step on the first real release. `dist plan` confirms the rest of the pipeline, including ownpg.rb, is now correctly planned.
Why Three independent audit passes (the code-quality gate, a bug-bounty pass, and a general security review) all converged on the same finding: the classifier, the mechanism every OwnPG tool depends on to refuse a dangerous statement, had gaps a caller with ordinary write or DDL scope could reach with no elevated privilege. DO and CALL bodies are opaque string literals to the SQL parser, so classify() found no relations, no denied functions, and no destructive reason inside one. write.rs accepted the resulting Procedure class next to Write with nothing further checked, so a DO block reached any schema, called any function, and ran unconfirmed. CREATE FUNCTION and CREATE PROCEDURE bodies have the same blind spot, and the language argument accepted anything syntactically valid, including plpython3u and plperlu, both of which reach the filesystem and the network directly. Separately, authorize(), the one function that checks a statement's relations against the configured schema scope, was called from exactly one place in the whole crate: the read path. Every write and DDL builder tool only checked statement kind and denied-function refusals, never relations, so a filter or WHERE fragment containing a subquery against a foreign schema passed straight through. What changed classify() now always sets a destructive reason for DoStmt, CallStmt, and CreateFunctionStmt, so all three need confirm: true or the confirmation prompt, the same as any other statement the classifier cannot fully see into. routine.rs now accepts only sql and plpgsql as a function or procedure language. write::execute now calls authorize() on the final rendered statement before running it, closing the schema-scope gap for every write and DDL tool at once. types.rs refuses CREATE EXTENSION for a short list of extensions that grant OS-level or arbitrary-network access (the untrusted procedural languages, dblink, postgres_fdw, file_fdw, adminpack). Fixing this also surfaced a real, unrelated false positive: a CREATE FUNCTION ... SECURITY DEFINER SET search_path clause was being refused as if it were a standalone SET statement, because walk() collects every node shaped like a statement anywhere in the tree, and PostgreSQL represents a function's SET option using the same VariableSetStmt node type. walk() now recognizes that specific shape (a DefElem with defname set or reset) and does not descend into it. Risk This is a breaking change for anyone already relying on DO, CALL, CREATE FUNCTION, or CREATE PROCEDURE running without confirm: true, or on a non-sql/plpgsql function language. Every fix has a live test proving the exploit path is closed and the legitimate case still works: pg_delete with a cross-schema filter subquery, a DO block reaching another schema, and the security-definer search_path case. 296 pre-existing tests plus 6 new ones pass; fmt and clippy are clean.
PR Summary by QodoPublish Homebrew releases and close SQL classifier bypasses
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
Code Review by Qodo
1. Untrusted blocks can execute host code
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+309
to
+312
| if classification.destructive_reason.is_none() | ||
| && matches!( | ||
| classification.kind.as_str(), | ||
| "DoStmt" | "CallStmt" | "CreateFunctionStmt" |
There was a problem hiding this comment.
1. Untrusted blocks can execute host code 🐞 Bug ⛨ Security
classify assigns every DoStmt only a destructive reason, while the hard safe_language allowlist is confined to the routine builder. When an untrusted language is already installed, pg_run_write accepts a DO LANGUAGE ... block as a procedure and confirm: true sends its opaque body to PostgreSQL, including code with interpreter-level filesystem or network capabilities.
Agent Prompt
## Issue description
Raw `DO` statements can select an untrusted procedural language and execute after `confirm: true`, bypassing the new language restriction in the routine builder.
## Fix Focus Areas
- crates/ownpg-core/src/classify.rs[309-317]
- crates/ownpg-core/src/tools/ddl/routine.rs[16-31]
- crates/ownpg-core/src/tools/write.rs[686-713]
## Recommended Fix
Inspect the language carried by `DoStmt` during classification and add a hard refusal unless it is an explicitly permitted language for anonymous blocks. Add tests proving that an unsafe language remains refused even with confirmation enabled.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+310
to
+313
| && matches!( | ||
| classification.kind.as_str(), | ||
| "DoStmt" | "CallStmt" | "CreateFunctionStmt" | ||
| ) |
There was a problem hiding this comment.
2. Cross-schema procedures remain callable 🐞 Bug ⛨ Security
authorize scopes only classification.relations, although the walker stores a CALL target in classification.functions and the new fallback merely marks it destructive. A write-capable caller can therefore submit CALL other_schema.proc() with confirm: true, reaching a routine outside the configured schema.
Agent Prompt
## Issue description
Qualified procedure calls are recorded as functions rather than relations, so the schema authorization added around write execution does not reject calls outside the configured schema.
## Fix Focus Areas
- crates/ownpg-core/src/classify.rs[322-360]
- crates/ownpg-core/src/classify.rs[550-565]
- crates/ownpg-core/src/classify.rs[985-997]
## Recommended Fix
Capture the top-level `CALL` target as a schema-scoped object and validate it in `authorize`, including pooled-connection qualification requirements. Add tests for qualified foreign-schema calls and unqualified calls on pooled connections.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
17
to
+18
| permissions: | ||
| contents: read | ||
| "contents": "write" |
There was a problem hiding this comment.
3. Pull requests receive release access 🐞 Bug ⛨ Security
The workflow grants contents: write globally while also running on pull_request and exposing GITHUB_TOKEN to build jobs that execute code from the checked-out revision. A same-repository pull request can consequently run modified build inputs with a token capable of changing repository contents or creating releases.
Agent Prompt
## Issue description
Pull-request validation inherits the workflow-wide repository write permission even though only tag-publishing jobs require it.
## Fix Focus Areas
- .github/workflows/release.yml[17-18]
- .github/workflows/release.yml[42-45]
- .github/workflows/release.yml[247-311]
## Recommended Fix
Set workflow-level contents permission to read and grant contents write only to the tag-gated host or release job that uploads artifacts and creates the GitHub release.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+153
to
+155
| # Actually do builds and make zips and whatnot | ||
| dist build ${{ needs.plan.outputs.tag-flag }} --print=linkage --output-format=json ${{ matrix.dist_args }} > dist-manifest.json | ||
| echo "dist ran successfully" |
There was a problem hiding this comment.
4. Release archives lose attribution 🐞 Bug ≡ Correctness
The regenerated local build invokes dist build without first generating THIRD-PARTY.txt, even though that gitignored file remains configured as an archive include. A clean release checkout therefore cannot package the required dependency attribution and may fail while resolving the missing include or publish archives without it.
Agent Prompt
## Issue description
The release workflow no longer creates the gitignored attribution file required by the cargo-dist include configuration before building archives.
## Fix Focus Areas
- .github/workflows/release.yml[146-155]
- Cargo.toml[121-132]
- tools/release.sh[393-396]
## Recommended Fix
Install the pinned cargo-about version and generate `THIRD-PARTY.txt` before every archive-producing `dist build`, or generate it once and download it into the workspace root in each local build job.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+243
to
+245
| ${{ steps.cargo-dist.outputs.paths }} | ||
| ${{ steps.cargo-cyclonedx.output.paths }} | ||
| ${{ env.BUILD_MANIFEST_NAME }} |
There was a problem hiding this comment.
5. Published releases omit the sboms 🐞 Bug ≡ Correctness
The artifact upload reads steps.cargo-cyclonedx.output.paths, but GitHub step values are exposed through outputs, which is where the preceding step writes its multiline paths value. The generated SBOM paths therefore expand to nothing and those files do not reach the host job or published release.
Agent Prompt
## Issue description
The SBOM upload path references a nonexistent singular `output` property, so generated CycloneDX files are omitted from the uploaded artifact set.
## Fix Focus Areas
- .github/workflows/release.yml[225-245]
## Recommended Fix
Change the expression to `${{ steps.cargo-cyclonedx.outputs.paths }}` and verify that the uploaded global artifact contains the generated `.cdx.xml` files.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
1 participant
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 fixes from a pre-v1 release-readiness pass. The first wires up the Homebrew tap publish job
that never existed. The second closes three ways a caller with ordinary write or DDL scope could
bypass the SQL classifier entirely, found independently by three separate audit passes run against
this branch.
Motivation
[workspace.metadata.dist]declared a Homebrew tap but never enabled its publish job, andrelease.ymlonly built artifacts, never published them, sodevops-infinity/homebrew-tapwouldhave stayed empty through a real release.
Separately,
authorize(), the function that checks a statement's relations against the configuredschema scope, was called from exactly one place in the entire crate: the read path.
DO/CALLbodies and
CREATE FUNCTION/CREATE PROCEDUREbodies are opaque string literals to the SQL parser,so the classifier found nothing destructive inside one and let it run with no confirmation, no
schema check, and no denied-function check, using only ordinary write scope.
Changes
publish-jobs = ["homebrew"]toCargo.tomland regeneratedrelease.ymlwithdist generate --mode ci, adding thehost,publish-homebrew-formula, andannouncejobs.Every action is pinned back to a commit SHA and the three installer scripts are back to
fetch-then-verify-then-run, matching this repository's own supply-chain rules.
classify()now always sets a destructive reason forDoStmt,CallStmt, andCreateFunctionStmt, so all three needconfirm: trueor the confirmation prompt.routine.rsnow accepts onlysqlandplpgsqlas a function or procedure language, refusingplpython3u,plperlu, and similar languages that reach the filesystem or the network directly.write::executenow callsauthorize()on the final rendered statement before running it,closing the schema-scope gap for every write and DDL builder tool at once (a
pg_deletefilteror a DDL
WHERE/USING/CHECKfragment containing a subquery against a foreign schema is nowrefused).
types.rsrefusesCREATE EXTENSIONfor a short list of extensions that grant OS-level orarbitrary-network access: the untrusted procedural languages,
dblink,postgres_fdw,file_fdw,adminpack.CREATE FUNCTION ... SECURITY DEFINER SET search_pathclause was being refused as a standaloneSETstatement, because PostgreSQL represents a function'sSEToption using the same node typeas a real
SETcommand, and the classifier's tree walk collected it as one. It now recognizesthat specific shape and does not descend into it.
Type of Change
fix)!to type)Testing
Test type:
Steps to verify:
cargo nextest run --workspace --all-features --lockedwithOWNPG_TEST_DSNset: 302 testspass, including three new live tests that prove the closed exploit paths: a
pg_deletefilterreaching a foreign schema is refused, a
DOblock always needs confirmation, and thesecurity-definer
search_pathcase no longer false-positives.cargo fmt --all -- --checkandcargo clippy --workspace --all-targets --all-features --locked -- -D warnings: clean.dist plan --output-format=jsonconfirmsownpg.rbis now planned as a real release artifact.Breaking Changes
What breaks:
DO,CALL,CREATE FUNCTION, andCREATE PROCEDUREnow needconfirm: trueor the confirmation prompt, the same as any other statement the classifier cannot fully see into.
CREATE FUNCTION/CREATE PROCEDUREnow refuses a language other thansqlorplpgsql.Migration: pass
confirm: true(or accept the confirmation prompt) for these statement kinds;rewrite a routine using a refused language in
sqlorplpgsql, or contact the maintainer if alegitimate use case needs a different language allowed.
Deployment Notes
HOMEBREW_TAP_TOKENis required (a token with write access todevops-infinity/homebrew-tap) for thepublish-homebrew-formulajob to succeed on a realrelease. It does not exist yet as a repository secret.
Checklist