Conversation
Closes CalloraOrg#1259 - Execute EXPLAIN queries on dedicated clients inside BEGIN READ ONLY; SET LOCAL statement_timeout = ...; ROLLBACK transactions - Add defence-in-depth static keyword allowlist to reject data-modifying statements (DELETE, UPDATE, INSERT, MERGE, DDL) in CTEs and subqueries - Cancel queries exceeding statement_timeout and return 400 Bad Request - Guarantee transaction ROLLBACK and client release on all code paths (success, query failure, and timeout cancellation) - Add getReadClient() method to ReplicaPool supporting optional dedicated client checkout from read replicas with primary fallback - Cover CTE DML rejection, read-only transaction errors, statement timeout cancellation, rollback, and client release in src/routes/admin/explain.test.ts - Restore root project files (package.json, README.md, jest.env-setup.cjs, adminAuth.ts) inadvertently deleted in recent upstream commits
|
@Ahbiz Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
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
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.
Closes #1259
Summary
Secures the
POST /api/admin/db/explaindiagnostic endpoint by executing queries inside dedicated read-only transactions with strict statement timeouts, guaranteed client rollback/release, and static defence-in-depth against data-modifying CTEs.Why this matters
EXPLAIN (ANALYZE, FORMAT JSON)actually executes SQL statements. Under the previous implementation, any query with aWITHprefix was allowed directly on the pool, enabling data-modifying CTEs (such asWITH d AS (DELETE FROM users RETURNING 1) SELECT * FROM d) to mutate production data, orpg_sleepcalls to pin connections.Acceptance Criteria Mapping
isAllowedQuery/hasDisallowedDmlKeywordsinsrc/validators/admin.ts, rejectingDELETE,UPDATE,INSERT,MERGE,DROP,ALTER,TRUNCATEbefore database execution.BEGIN READ ONLYtriggers SQLSTATE25006(cannot execute DELETE in a read-only transaction) and the subsequentROLLBACKguarantees no rows change.SET LOCAL statement_timeout = ...(default 5000 ms, configurable via router deps,ADMIN_EXPLAIN_TIMEOUT_MS, or bodystatementTimeoutMs).57014) are caught, rolled back, and returned as HTTP400 Bad Request.poolorreplicaPool.getReadClient().ROLLBACKon both success and error paths.client.release()is executed unconditionally in afinallyblock.src/routes/admin/explain.test.tscovers both casesSecurity & Failure-Mode Handling
stripSqlLiteralsAndCommentsstrips single-quoted strings, dollar-quoted blocks ($$...$$), single-line comments (--), and block comments (/* ... */) before keyword checking, preventing false positives (e.g.WHERE status = 'DELETE') while preventing keyword smuggling.ROLLBACKthrows during error cleanup,client.release()is still executed infinally.getReadClient()toReplicaPool(src/db/replicaPool.ts), allowing read-only explain diagnostics to be offloaded to read replicas with fallback to the primary pool.Restored Files Note
Restores
package.json,README.md,jest.env-setup.cjs, andsrc/middleware/adminAuth.tswhich were inadvertently deleted by recent upstream commits599ab6eand092ece9, enablingnpm ci, test execution, and CI to pass cleanly.Testing & Verification
All 195 tests across affected suites pass:
npm test -- src/routes/admin/explain.test.ts src/validators/admin.test.ts src/db/replicaPool.test.ts