Harden PdoStorageService: input validation, security, and reliability#19
Open
Harden PdoStorageService: input validation, security, and reliability#19
Conversation
Adopts the evolved version from xibosignage/xibo-cms (lib/Storage/PdoStorageService.php), relicensed AGPL-3.0+ to MIT by the rights holder, and layers on the security hardening identified in the recent review. Improvements ported from the CMS: - TLS/SSL connection support via PDO::MYSQL_ATTR_SSL_CA / SSL_VERIFY_SERVER_CERT, configurable via new optional 'ssl' and 'sslVerify' config keys. - ErrorException reconnect path catches "Error while sending QUERY packet." - Per-call \$close and \$transaction flags on every operation. - SQL syntax errors (1064) are now logged at error level. - utf8mb4 charset. Security hardening on top: - Charset is declared in the DSN (charset=utf8mb4) instead of a post-connect SET NAMES, and PDO::ATTR_EMULATE_PREPARES is disabled. Closes the emulated- prepares charset-desync class of SQL injection. - setTimeZone validates the value against a strict allow-list (numeric offsets or IANA-style names) before interpolation. MySQL does not allow placeholders in SET time_zone, so the guard is the only injection control available. - DSN host/port/name are validated before concatenation, closing DSN parameter injection via misconfigured config. - logSql now redacts bound parameter values: the SQL is logged with placeholders intact and a separate context array contains only parameter keys, never values. Deliberate divergences from CMS: - Connection state stays instance-level (\$this->conn), not static. Preserves per-instance test isolation and supports callers building multiple services. - isolated() is retained (CMS dropped it); covered by tests, part of the contract. - Generic PSR-3 LoggerInterface, not the CMS LogService::sql() helper. - Constructor-injected config, not the CMS ConfigService global. Interface change (BREAKING): - StorageServiceInterface methods gain \$close (and \$transaction where applicable). Downstream packages that implement StorageServiceInterface must add the new optional parameters to their implementations. This warrants a major version bump. Tests: - 21 new tests cover setTimeZone validation (10 malicious payloads), ErrorException reconnect on every operation, \$transaction=false skip, \$close=true release, 1064 error-level logging, and parameter-value redaction. - Existing 38 database tests continue to pass.
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
2 participants
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
This PR significantly hardens the
PdoStorageServicedatabase wrapper with input validation, improved security, enhanced error handling, and better logging practices. The changes address SQL injection vectors, improve connection resilience, and provide better observability without leaking secrets.Key Changes
Security & Input Validation
createDsn()to prevent malicious DSN parameter injectionsetTimeZone()to accept only numeric offsets (e.g.,-08:00) or IANA-style names (e.g.,Europe/London), rejecting arbitrary inputPDO::ATTR_EMULATE_PREPARES = false) to ensure escaping is performed by the server using the connection charset, closing a class of SQL injection vulnerabilitiesutf8mb4charset in the DSN to avoid SET NAMES/emulated-prepares charset desync issuesLogging & Observability
logSql()to never log raw parameter values; logs the SQL with placeholders intact and parameter keys in context onlyError Handling & Resilience
\ErrorException(e.g., "Error while sending QUERY packet") in all query methods with reconnect supportConnection Management
sslandsslVerifyparameters toconnect()for certificate-based authenticationsetConnection()to use newopenConnection()helper method$closeparameter to all query methods (select,insert,update,exists,isolated,commitIfNecessary) to allow closing connections after operations$transactionparameter toinsert(),update(), andupdateWithDeadlockLoop()to allow skipping automatic transaction managementCode Quality
\PDO[],string|null)??operator throughoutnullto'default'in method signatures for clarityopenConnection()methodTesting
Notable Implementation Details
connect()method signature now includes optional$ssland$sslVerifyparameters for TLS support$closeflag to discard the connection after use, useful for connection cleanup in certain scenarios$transactionflag allows callers to opt out of automatic transaction management when neededhttps://claude.ai/code/session_01QUqTHWNiX5nK6w1A5e1KYq