add connection timeouts, tighten CORS, scrub URIs - #12
Conversation
When the version in pyproject.toml is new (skip_publish=false), the workflow now also pushes v$VERSION as an annotated git tag and creates a GitHub release with auto-generated notes plus dist/* assets. Both new steps are gated on the same skip_publish flag so reruns are safe. The job grants contents:write to allow the auto-injected GITHUB_TOKEN to push the tag and create the release. refactor(security): extract shared scrub_connection_uris helper DbConnPool._set_last_error and server._scrub_error_text shared an identical URI-scrub regex. Consolidate in sql_driver to prevent drift. fix(security): add connection timeouts, tighten CORS, scrub URIs - Pool now sets statement_timeout (30s), idle_in_transaction_session_timeout (60s), lock_timeout (5s) at handshake via libpq options; configurable via PGTUNER_STATEMENT_TIMEOUT_MS / _IDLE_TXN_TIMEOUT_MS / _LOCK_TIMEOUT_MS env. - CORS default tightened from "*" to localhost-only for both streamable-http and SSE modes; PGTUNER_CORS_ALLOW_ORIGINS env opens it; wildcard "*" forces allow_credentials=False with a startup warning. - DbConnPool.last_error and tool-error responses are URI-scrubbed via obfuscate_password().
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several new MCP tools for PostgreSQL analysis, including buffer cache and TOAST storage inspection, replication and WAL health monitoring, and static query linting using pglast. It also adds security hardening features (CORS configuration, connection timeouts, and error message credential scrubbing) and sets up a multi-version PostgreSQL integration testing suite. The review feedback highlights a few critical improvements: AnalyzeToastStorageHandler must dynamically handle a.attcompression to avoid crashing on PostgreSQL versions older than 14; the pg_buffercache query should filter by reldatabase to prevent incorrect joins with database-specific tables; and the query linter rules should accumulate all findings rather than returning early on the first match.
pg_attribute.attcompression was added in PostgreSQL 14. On PG13 and older the existing cols_sql crashes with 'column a.attcompression does not exist'. Branch the SELECT list on get_postgres_version() to use NULL::char on older versions; downstream Python's compression lookup already maps None to 'default'.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a static query anti-pattern linter using pglast AST, adds several new MCP tools (for buffer cache, TOAST storage, replication health, and query linting), adapts existing tools for PostgreSQL 17 compatibility, and implements security hardening features like CORS configuration and connection-level safeguards. The review feedback highlights several key improvement opportunities: validating and sanitizing timeout environment variables to prevent configuration injection, modifying linter rules to accumulate all findings rather than returning on the first match, ensuring OrOfEqualsRule checks both sides of equality, recursively walking nested function calls in FunctionOnIndexedColRule, iterating over all statements in UnboundedSelectRule, and robustly handling CORS wildcard origins when combined with other entries.
…btree - OrOfEqualsRule previously only inspected the left operand of each = expression, missing patterns like `1 = col OR 2 = col`. Now checks both sides via lexpr/rexpr. - FunctionOnIndexedColRule previously only looked at the immediate args of the outer FuncCall, missing nested forms like lower(coalesce(col, '')) which are equally index-defeating. Now walks the FuncCall subtree for any ColumnRef. The suggestion message no longer hard-codes a single function name (which would be misleading for nested cases) — it points at expression indexes generically.
|
/gemini review |
1 similar comment
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces static query linting, buffer cache and TOAST storage analysis, and replication/WAL health monitoring tools, alongside compatibility updates for PostgreSQL 17 and security enhancements like CORS configuration and error scrubbing. Feedback highlights two key improvements: updating the CORS wildcard check in server.py to "*" in origins to prevent startup crashes with mixed origin lists, and modifying the TOAST compression check in tools_buffercache.py to target compression != "lz4" so that columns using default (pglz) compression are correctly identified for optimization.
In PG14+, attcompression=' ' (default) means PGLZ, but the previous check 'compression == \"pglz\"' missed those columns because the catalog reports the default marker rather than the literal 'pglz'. Switch to 'compression != \"lz4\"' — any non-LZ4 EXTENDED column on a large TOAST table is a candidate for conversion. Add regression tests for both the default-compression and already-lz4 cases. Reported by gemini-code-assist on PR #12. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
/gemini review |
1 similar comment
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a static query linter (QueryLinter) using pglast AST, adds several new MCP tools (for buffer cache, TOAST storage, replication health, and query linting), implements security hardening (timeouts, CORS, and URI scrubbing), and adds PostgreSQL 17 compatibility alongside integration tests. The code review feedback focuses on improving the linter rules in query_linter.py: specifically, addressing false positives in ImplicitCastRule by ignoring LIKE/ILIKE operators, and refactoring rules like SelectStarRule and UnboundedSelectRule to collect all findings across the AST and support multi-statement queries instead of returning early on the first match.
There was a problem hiding this comment.
Code Review
This pull request introduces integration testing infrastructure, security hardening (CORS configuration and connection URI password scrubbing), compatibility updates for PostgreSQL 17, and several new MCP tools, including a static query anti-pattern linter (QueryLinter) and replication health checks. The review feedback highlights critical improvement opportunities: the replication health tool should dynamically handle standby nodes to prevent crashes when calculating WAL retention, and the query linter rules should accumulate all findings across multiple statements and occurrences rather than returning early on the first match.
There was a problem hiding this comment.
Code Review
This pull request introduces integration testing, PostgreSQL 17 compatibility, security hardening, and new MCP tools, including a static query linter, buffer cache/TOAST analysis, and replication health monitoring. The reviewer's feedback highlights critical correctness and robustness issues: query truncation in the slow-query helper causes AST parsing failures in the linter; replication health checks fail on standby nodes due to primary-only WAL LSN functions; several linter rules incorrectly return early on the first finding instead of accumulating all matches; the OrOfEqualsRule logic can incorrectly suggest rewrites for mixed OR chains; and TOAST compression mapping needs more robust handling of default null characters.
Without column-type information from the AST alone, this rule cannot distinguish a safe 'text-col = text-literal' (very common) from a suspect 'timestamp-col = text-literal'. Two mitigations: 1. Skip LIKE/ILIKE operators — they are text-only operators and were producing false positives that overlapped with NonSargableLikeRule. 2. Downgrade severity from WARNING to INFO so lint_workload's default warning threshold filters the rule out by default. Users who want the heuristic must opt in via severity_threshold='info'.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a static query anti-pattern linter, adds several new MCP tools (for buffer cache, TOAST storage, and replication health analysis), implements security hardening such as CORS configuration and error message URI scrubbing, and adds PostgreSQL 17 compatibility along with a comprehensive integration testing suite. Feedback on these changes highlights that multiple linter rules return prematurely on the first violation rather than accumulating all findings, and recommends quoting database identifiers in the generated TOAST storage recommendations to ensure robustness against special characters or reserved words.
…hars The attcompression default marker varies by psycopg driver / connection settings: ' ', '', '\x00', or None. The previous dict-with-fallback could echo back raw '\x00' or unknown values in the tool payload. Switch to explicit if/elif/else so 'p' -> pglz, 'l' -> lz4, and everything else (including all default-marker variants) -> 'default'.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements to the pgtuner_mcp toolset, including a static query anti-pattern linter using pglast, new diagnostic tools for buffer cache, TOAST storage, and replication health, along with security hardening features like CORS configuration and connection URI password scrubbing. It also adds a comprehensive integration testing suite targeting PostgreSQL versions 14 through 17. The review feedback highlights three key areas for improvement: optimizing the buffer cache analysis query by joining directly on relfilenode to avoid performance overhead, updating the query linter rules to accumulate all findings rather than returning early on the first match, and refining the replication health recommendations to specify the target database when rolling back stale prepared transactions.
pg_prepared_xacts is cluster-wide, but ROLLBACK PREPARED runs in the connection's database. Without naming the target database, the recommendation would silently fail when the prepared xact belongs to a different database than the one the monitoring tool is connected to. Re
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces integration testing across multiple PostgreSQL versions, security enhancements such as CORS configuration and URI password scrubbing, and new MCP tools for replication health, buffer cache/TOAST analysis, and static query linting. The review feedback highlights three main issues: the query linter rules prematurely return only the first finding, the unbounded select rule ignores subsequent statements in multi-statement inputs, and directly aliasing PG17's dead tuple bytes to older tuple count columns in the vacuum progress tool will lead to severe over-reporting.
1 participant
When the version in pyproject.toml is new (skip_publish=false), the workflow now also pushes v$VERSION as an annotated git tag and creates a GitHub release with auto-generated notes plus dist/* assets. Both new steps are gated on the same skip_publish flag so reruns are safe. The job grants contents:write to allow the auto-injected GITHUB_TOKEN to push the tag and create the release.
refactor(security): extract shared scrub_connection_uris helper
DbConnPool._set_last_error and server._scrub_error_text shared an identical URI-scrub regex. Consolidate in sql_driver to prevent drift.
fix(security): add connection timeouts, tighten CORS, scrub URIs