Replace escape_string() concatenation with parameterized queries - #1004
Merged
DavidGoodwin merged 6 commits intoApr 6, 2026
Merged
Conversation
…tfixadmin#1003) Convert db_in_clause() and db_where_clause() to use PDO named parameters instead of escape_string() string concatenation. Update all callers to pass params through to db_query_all().
knofte
force-pushed
the
replace-escape-string-with-params
branch
from
March 31, 2026 06:42
8ca895f to
33cd1f2
Compare
There was a problem hiding this comment.
Pull request overview
This PR migrates legacy SQL construction paths away from escape_string() concatenation and toward PDO parameter binding, primarily by updating shared query-building helpers and threading parameter arrays through callers.
Changes:
- Updated
db_in_clause()anddb_where_clause()to generate SQL with placeholders and accumulate bound values via a&$paramsarray. - Updated several pages/handlers to pass parameter arrays through to
db_query_all()/ related helpers. - Extended
db_delete()to support additional bound parameters for appended WHERE fragments.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
functions.inc.php |
Refactors db_in_clause() / db_where_clause() to emit placeholders and collect params; extends db_delete() to accept extra bound params. |
model/PFAHandler.php |
Threads accumulated params from query-building into db_query_all() and pagebrowser creation. |
model/DomainHandler.php |
Converts a legacy string condition to array-based conditions; updates cleanup deletes to use bound params for LIKE. |
public/main.php |
Updates search queries to reuse a shared params array and pass it through db_query_all(). |
public/list-virtual.php |
Removes escape_string() usage for domain/search and relies on bound parameters (including db_in_clause() params). |
public/broadcast-message.php |
Switches domain filtering to db_in_clause() with params and passes them to db_query_all(). |
public/viewlog.php |
Removes escape_string() on fDomain and relies on parameterized filtering later. |
public/users/password-recover.php |
Removes redundant escaping before calling a parameterized token generator; URL-encodes username on redirect. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Author
knofte
added a commit
to knofte/postfixadmin
that referenced
this pull request
Apr 15, 2026
… real query Covers the parameterized query refactor from PR postfixadmin#1004.
DavidGoodwin
pushed a commit
that referenced
this pull request
Apr 15, 2026
* Add regression tests for global admin privilege check in TOTP exceptions (#1012) Verifies that global admins with both 'admin' and 'global-admin' roles can add TOTP exceptions for any domain, while regular admins are restricted to their managed domains. * Add tests for db_in_clause(): placeholders, empty array, unique keys, real query Covers the parameterized query refactor from PR #1004. * Move test cleanup to tearDown: add admin table delete and session unset Prevents state leaking between tests, especially when expectException() aborts the method before inline cleanup runs. * Use assertEquals with exact expected SQL per review
This was referenced Apr 19, 2026
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 #1003
Converts
db_in_clause()anddb_where_clause()to use PDO named parameters via a&$paramsreference array, instead ofescape_string()string concatenation. All callers updated to pass params through todb_query_all().Remaining
escape_string()usages are intentionally kept where parameterization doesn't apply (backup.php SQL dump output, date format strings, infinity symbol constant).Both functions are backwards compatible — the
&$paramsparameter defaults to[].Tested: lint, php-cs-fixer, psalm, phpunit all pass.