Re: backup.php — a possible root-cause fix
I understand the removal has already landed on master, and I’m not trying to relitigate it. But I spent some time reading source with the assistance of AI. I think that the recurring vulnerability class here is fixable at the root rather than patched again. Before anyone invests effort, I want to find out whether a PR along these lines would even be considered.
What I think the actual problem is
The file splits into two halves, and only one of them is generating the issues.
The backup half looks clean already. The database dump wraps every value in escapeshellarg(), and none of those values come from user input — they’re all from $sqlconf (login, password, host, port, database). The site-directory archiving uses create_tar_archive() and ZipArchive, which are PHP, not shell. There’s very little injection surface in the part that actually performs backups.
The configuration export/import half is where it all lives — roughly lines 671–1000. What that code is doing is building shell command strings to accomplish things PHP can do directly:
- A long chain of
echo "..." >> file redirections whose only purpose is writing SQL text into a file.
- A pipe through perl to strip
DEFAULT CHARSET= and collate clauses with a regex.
mysqldump --where= clauses with user-selected list and layout IDs interpolated in.- Every one of the above written twice, once for
cmd.exe and once for sh.
That last point is where the three-layer escaping comes from — SQL escaping inside shell escaping inside platform-specific quoting, maintained in two parallel code paths. I think that’s the “whack-a-mole” mechanism: fixing a quoting bug on one platform can open an injection path on the other.
The proposed fix
Remove the shell from that half of the file entirely, rather than adding another escaping layer:
- The
echo >> file chains become fwrite().
- The perl pipe becomes
preg_replace() in PHP (and perl stops being a dependency).
- The
--where dumps become parameterized sqlStatement(... WHERE list_id = ?, [$listid]) queries with the INSERT statements generated in PHP. This pattern already exists in the same file — the CSV export path at lines ~193 and ~255 does exactly this. The export path just never adopted it.
- The full database dump keeps
mysqldump, but invoked via Symfony\Component\Process\Process with an argument array rather than a concatenated command string.
If that’s done, both shell-escaping helper closures get deleted and the IS_WINDOWS branching in the export path collapses to a single code path. The escaping problem doesn’t get safer — it stops existing, because there’s no shell left for anything to be escaped for.
What this doesn’t fix
Being honest about the limits:
- It doesn’t address the concern that a one-click backup lets someone drop an unencrypted database into a downloads folder. That needs the artifact encrypted and served through an expiring link, which is separate work.
- The size limitation remains — the feature stops working above a certain database size regardless.
Who this helps, and how I’d like us to support them
I want to be clear about who I think is affected, because it shapes what a good outcome looks like.
The people who use this aren’t the ones with shell access. mysqldump and rsync are the right answer for anyone who can use them, and I’m not arguing otherwise. But the same menu entry shows up in the wild under English, Greek, Spanish, and Vietnamese labels, on current releases — small practices, often non-English-speaking, frequently without a technical administrator. For that group the realistic outcome of removal isn’t that they switch to command-line tools. It’s that they quietly stop backing up, and nobody finds out until the day it matters. I don’t think a wiki page closes that gap — the operators most affected are the ones least able to act on it. That’s why I’d rather see the underlying problem fixed than the feature documented away.
Two questions
- Would a PR along these lines be considered at all, or is the decision to remove settled regardless of whether the root cause is addressed?
- Would anyone be willing to review it and own it going forward? I’ll be straightforward: I’m not able to maintain this myself, and I know that’s the deciding question. But I think “review a complete PR” is a much smaller ask than “write and maintain a module,” and I’d like to know if it’s findable.
I should also say plainly that I’d be using AI assistance to produce the code, with the diagnosis above coming from reading the source rather than from the advisories.
Given what this file is and the current advisory backlog, that should be on the table up front rather than discovered later — and it’s exactly why I’m asking about a reviewer before writing anything.
