fix: close SQL-injection and secrets gaps found by a security audit - #4
Merged
Merged
Conversation
Why A round of security and protocol-conformance audits found real gaps: EXCLUDE constraint elements and ATTACH PARTITION bounds went straight into SQL without passing through the classifier, the same class of bypass that hit a competing Postgres MCP server; SSH private keys and TLS client keys loaded with no permission check, unlike every other secret file; a rate-limited HTTP response carried a JSON-RPC id of null, which the 2026-07-28 wire schema forbids; and the health-check endpoint shared its rate-limit bucket with real traffic instead of having its own. What changed render.rs gained operator, partition_bound, and returns_type, three validators that probe a fragment through the classifier the same way type_name and expression already do; table.rs, routine.rs, and index.rs now route every untrusted fragment through one of them. connect/ssh.rs and connect/tls.rs now call refuse_open_permissions before reading a key file, matching the existing profile and token file checks. config/resolve.rs extends the Windows permission warning to sslkey, the SSH key file, and the HTTP state key file. The HTTP rate limiter's default moved from 60 calls per minute to unlimited, since the fixed default was throttling normal agent traffic; the mechanism itself still has test coverage, pinned to a low limit for that one test. A readiness-only rate limiter was added so health checks no longer share a bucket with tool calls. classify::check, CopyShape, and a handful of other internal names were renamed for clarity; classify::check becomes classify::authorize, a public rename flagged for review since ownpg-core's API is semver-checked. README, CHANGELOG, SECURITY, and the CLAUDE.md/AGENTS.md pair were corrected against the real source and against the repository's current public visibility. Risk classify::authorize is a public rename; nothing outside this crate calls it today, but it is a breaking change to ownpg-core's API.
PR Summary by QodoHarden SQL rendering, secret loading, and MCP HTTP conformance
AI Description
Diagram
High-Level Assessment
Files changed (28)
|
Code Review by Qodo
1. Functions bypass safe privilege settings
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+192
to
+195
| let probe = format!( | ||
| "CREATE FUNCTION ownpg_probe_fn() RETURNS {trimmed} LANGUAGE sql AS $$ SELECT 1 $$" | ||
| ); | ||
| let parsed = classify::classify(&probe).map_err(|error| Error::ArgumentInvalid { |
There was a problem hiding this comment.
1. Functions bypass safe privilege settings 🐞 Bug ⛨ Security
returns_type accepts a crafted value such as `int LANGUAGE sql SECURITY DEFINER AS $$ SELECT 1 $$ --` because its probe remains one valid function statement, then returns the entire fragment unchanged. When routine creation appends the configured clauses, the comment suppresses them, allowing an injected owner-privileged function without the mandatory pinned search path.
Agent Prompt
## Issue description
`returns_type` validates only the probe's top-level statement kind, so an input can terminate the return type, inject function clauses and a body, and comment out the renderer-controlled clauses. This permits `SECURITY DEFINER` without the search-path protection normally added by the routine renderer.
## Fix Focus Areas
- crates/ownpg-core/src/render.rs[184-211]
- crates/ownpg-core/src/tools/ddl/routine.rs[255-297]
## Recommended Fix
Inspect the parsed function definition and require the untrusted text to populate only the return-type grammar node, rejecting comments or any supplied language, body, security, configuration, or other function clause. Add a regression test using `int LANGUAGE sql SECURITY DEFINER AS $$ SELECT 1 $$ --` and verify that normal scalar, `SETOF`, and `TABLE (...)` return types remain accepted.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
190
to
+193
| StatusCode::TOO_MANY_REQUESTS, | ||
| axum::Json(serde_json::json!({ | ||
| "jsonrpc": "2.0", | ||
| "id": serde_json::Value::Null, | ||
| "id": 0, |
There was a problem hiding this comment.
2. Rate-limit errors carry the wrong id 🐞 Bug ≡ Correctness
too_many_requests hard-codes JSON-RPC request ID 0 even though the guard runs before the request body is parsed and cannot know the caller's ID. Any rate-limited request using another ID receives an uncorrelatable response, while a notification incorrectly receives an ID-bearing response at all.
Agent Prompt
## Issue description
The HTTP guard emits a JSON-RPC error with a fixed ID before the MCP service parses the request body. The resulting ID does not match most requests and cannot correctly represent notifications.
## Fix Focus Areas
- crates/ownpg-core/src/server/http/mod.rs[187-205]
- crates/ownpg-core/src/server/http/mod.rs[246-322]
- crates/ownpg-core/src/server/http/mod.rs[560-570]
## Recommended Fix
Either apply request rate limiting after JSON-RPC decoding so the original request ID can be copied and notifications can remain response-free, or return a protocol-appropriate plain HTTP 429 without fabricating a JSON-RPC envelope. Extend tests with nonzero string and integer IDs and with a notification.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+569
to
+573
| tokio::task::spawn_blocking(move || { | ||
| if let Err(error) = audit.record(&entry) { | ||
| tracing::error!(%error, "the audit line could not be written"); | ||
| } | ||
| }); |
There was a problem hiding this comment.
4. Concurrent calls scramble audit chronology 🐞 Bug ◔ Observability
Each completed request now submits an independent blocking job, and whichever job first acquires the audit mutex determines its position and timestamp in the hash chain. Concurrent calls can therefore appear in a different order from their completion and submission order, making incident timelines misleading even though the resulting chain remains structurally valid.
Agent Prompt
## Issue description
Independent blocking audit jobs race to acquire the sink mutex, so the append order no longer preserves the order in which completed calls submit their records. The hash chain remains valid but can describe a misleading event chronology.
## Fix Focus Areas
- crates/ownpg-core/src/server/mod.rs[298-303]
- crates/ownpg-core/src/server/mod.rs[568-573]
- crates/ownpg-core/src/audit.rs[251-285]
## Recommended Fix
Send completed audit entries through one FIFO worker that performs blocking writes serially, or assign submission sequence numbers and enforce that order before writing. Add a concurrent-call test that delays selected writes and verifies the resulting audit order remains the submission order.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+568
to
+573
| let audit = Arc::clone(&self.audit); | ||
| tokio::task::spawn_blocking(move || { | ||
| if let Err(error) = audit.record(&entry) { | ||
| tracing::error!(%error, "the audit line could not be written"); | ||
| } | ||
| }); |
There was a problem hiding this comment.
3. Shutdown can omit tool audit entries 🐞 Bug ☼ Reliability
record_protocol_request and record_tool_call discard their spawn_blocking handles, while Server::shutdown calls audit.flush() without retaining or awaiting the submitted audit writes. When a request drains immediately before shutdown, a queued closure can acquire the audit lock after the flush has synchronized existing records, leaving the final completed protocol or tool call outside the shutdown durability boundary on process exit.
Agent Prompt
## Issue description
Audit writes are submitted as detached blocking tasks whose handles are discarded, so `Server::shutdown()` can flush and synchronize the audit sink before already-accepted protocol or tool records have been written.
## Fix Focus Areas
- crates/ownpg-core/src/server/mod.rs[298-303]
- crates/ownpg-core/src/server/mod.rs[568-573]
- crates/ownpg-core/src/server/mod.rs[588-607]
- crates/ownpg-core/src/audit.rs[251-299]
## Recommended Fix
Track audit-write completion through a dedicated owned audit worker or by retaining all pending task handles, and route both protocol and tool records through the same tracked path. During shutdown, stop accepting new entries, drain or await every accepted write, and only then call `audit.flush()` to synchronize the sink. Preserve error logging for individual failed writes, and add a shutdown test proving that records submitted immediately before shutdown are present afterward.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
557
to
+563
| if cfg!(windows) | ||
| && (profile.password.is_some() | ||
| || profile.sslkey.is_some() | ||
| || profile | ||
| .ssh | ||
| .as_ref() | ||
| .is_some_and(|ssh| ssh.key_file.is_some()) |
There was a problem hiding this comment.
5. Windows key permissions go unwarned 🐞 Bug ⛨ Security
The expanded Windows warning tests profile.sslkey, but TLS resolution accepts OWNPG_SSLKEY and PGSSLKEY into the separate resolved sslkey value. On Windows the permission check is intentionally a no-op, so operators supplying either environment-sourced client key load it without the new permissions_unchecked warning.
Agent Prompt
Issue description
The Windows permission warning only examines profile fields, so TLS keys supplied through `OWNPG_SSLKEY` or `PGSSLKEY` are not covered even though Windows cannot enforce their file modes.
Fix Focus Areas
- crates/ownpg-core/src/config/resolve.rs[557-573]
- crates/ownpg-core/src/config/resolve.rs[427-429]
Recommended Fix
Build the Windows warning condition from the resolved secret-file settings, including `sslkey`, rather than only profile fields. Apply the same approach to other environment or libpq sourced secret-file paths covered by the warning.
ⓘ 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
A security and protocol-conformance audit of ownpg-core found a SQL-injection-class bypass in
two DDL tools, two secret files loaded with no permission check, and a rate-limited HTTP response
that violated the MCP wire schema. This PR fixes all of them, plus the gaps a live conformance run
against the fixed server then surfaced.
Motivation
Statement classification is the security boundary this whole project depends on: every argument
that becomes part of a SQL statement is supposed to pass through it before running. EXCLUDE
constraint elements and ATTACH PARTITION bounds did not. SSH private keys and TLS client keys were
read with no permission check, unlike every other secret file in the config layer. Running the
real
@modelcontextprotocol/conformancesuite against a live server then found a wire-schemaviolation (a rate-limited response with
id: null) and a missing readiness-only rate limit.Changes
operator,partition_bound, andreturns_typetorender.rs, three validators thatprobe an untrusted fragment through the classifier, matching the existing
type_name/expressionpattern.table.rs) through thosevalidators instead of interpolating them raw.
refuse_open_permissions) beforeconnect/ssh.rsandconnect/tls.rsread a private key file.
config/resolve.rsto coversslkey, the SSH keyfile, and the HTTP state key file.
id: null, which the 2026-07-28 MCP wire schemaforbids.
/healthz/readyno longer shares a bucket with real toolcalls.
stay configurable by environment variable, and the rate-limit mechanism itself keeps live test
coverage pinned to a low limit.
extensionscapability in the server'sinitializeresponse, closing awire-format gap the conformance suite flagged.
classify::checktoclassify::authorize,CopyShapetoCopyDetails, and severalother internal names the codebase's own naming pass flagged;
classify::authorizeis a publicrename.
source and the repository's current public visibility.
Type of Change
fix)!to type)Testing
Test type:
Steps to verify:
cargo nextest run --workspace --all-features --lockedwithOWNPG_TEST_DSNset against alocal PostgreSQL: 296 tests pass.
cargo fmt --all -- --check,cargo clippy --workspace --all-targets --all-features --locked -- -D warnings,cargo doc,cargo machete,cargo audit,cargo deny check: all pass clean.npx @modelcontextprotocol/conformance server --requirements 2026-07-28against a live localbuild: baseline check passes, every remaining failure is a documented, verified gap in
.github/conformance/expected-failures.yaml.Breaking Changes
What breaks:
classify::authorizereplaces the public functionclassify::checkinownpg-core. Nothing outside this crate calls it today.Migration: rename the call site from
classify::checktoclassify::authorize; the signatureis unchanged.
Checklist