hardening: prepared statements, PHP 7.4 idioms, and bug fixes#153
hardening: prepared statements, PHP 7.4 idioms, and bug fixes#153somethingwithproof wants to merge 4 commits into
Conversation
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens the RouterConfigs Cacti plugin by reducing injection/XSS risk and tightening a few correctness issues across UI pages, upgrade logic, and credential handling.
Changes:
- Convert several SQL calls to prepared statements and cast numeric parameters where applicable.
- Escape user-/device-sourced output in UI views (device debug/config, accounts, device types, compare selectors).
- Add stricter file-path handling for backup reads and safer credential decoding (
unserializewithallowed_classes=false), plus small typo/HTML tag fixes.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
Text/Exception/Pear.php |
Replace legacy isset() ternaries with ?? and fix “unkown” typo. |
setup.php |
Parameterize realm migration and plugin version update queries; use ??= for poller interval default. |
router-download.php |
Use ??= / ?? for cleaner defaults in CLI option parsing. |
router-devtypes.php |
Escape rendered fields; parameterize paging query; safer ID handling in action list rendering. |
router-devices.php |
Fix mismatched heading tag; escape debug output and parameterize device lookup in edit path. |
router-compare.php |
Escape option labels, normalize option values, add file path validation and fix wrong backup-id message. |
include/functions.php |
Harden IN-clause ID handling; safer credential decode; add file-path validation + escape config output. |
| db_execute_prepared('DELETE FROM user_auth_realm | ||
| WHERE user_id = ? | ||
| AND realm_id = ?', | ||
| [(int) $u['user_id'], (int) $user]); |
There was a problem hiding this comment.
In this upgrade migration, you load users from the old realm_id (86) but the DELETE uses $user (the new realm id). That will remove the newly inserted permission rows instead of removing the old realm mapping. Update the DELETE to target the old realm id (86) (or store it in a variable) so permissions actually migrate.
| $filepath1 = plugin_routerconfigs_dir($device1['directory']) . basename($device1['filename']); | ||
| $resolved1 = realpath($filepath1); | ||
| $basedir1 = realpath(plugin_routerconfigs_dir($device1['directory'])); | ||
|
|
||
| if (file_exists($filepath1)) { | ||
| $lines1 = @file($filepath1, FILE_IGNORE_NEW_LINES); | ||
| if ($resolved1 === false || $basedir1 === false || strpos($resolved1, $basedir1) !== 0) { |
There was a problem hiding this comment.
Using realpath($filepath1) as part of validation will return false when the file doesn't exist, causing the code to treat a normal missing backup file as a validation failure (and skipping the later “missing” error path). Consider validating the directory with realpath(plugin_routerconfigs_dir(...)) and then checking the candidate path (basedir + basename(filename)) without requiring the file to exist, so missing files are reported accurately.
| $filepath = plugin_routerconfigs_dir($device['directory']) . basename($device['filename']); | ||
| $resolved = realpath($filepath); | ||
| $basedir = realpath(plugin_routerconfigs_dir($device['directory'])); | ||
|
|
||
| if (file_exists($filepath)) { | ||
| $lines = @file($filepath); | ||
| if ($resolved === false || $basedir === false || strpos($resolved, $basedir) !== 0) { |
There was a problem hiding this comment.
realpath($filepath) returns false when the backup file is missing, so the current validation path will report “File path validation failed” for ordinary missing files and bypass the later “missing” message. Validate the base directory separately and compare the candidate path (basedir + basename(filename)) without depending on realpath() of the file itself, so missing backups are handled correctly.
Add targeted tests for prepared statement migration, output escaping, auth guard presence, CSRF token validation, redirect safety, and PHP 7.4 compatibility. Tests use source-scan patterns that verify security invariants without requiring the Cacti database. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Consolidated hardening: