hardening: prepared statements, PHP 7.4 idioms, and security fixes#90
hardening: prepared statements, PHP 7.4 idioms, and security fixes#90somethingwithproof wants to merge 2 commits into
Conversation
Signed-off-by: Thomas Vincent <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR aims to harden MikroTik/Cacti plugin code by modernizing PHP 7.4+ idioms and reducing injection risk in request handling and SQL operations.
Changes:
- Replaced
isset(...) ? ... : ...patterns with??in multiple parsing/collection paths. - Hardened request deserialization by restricting
unserialize()to disallow classes and defaulting invalid input to an empty array. - Switched a
DELETE ... IN (...)query to use a prepared statement with dynamic placeholders.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
RouterOS/routeros_api.class.php |
Uses null coalescing for parsed response values to simplify defaults. |
poller_mikrotik.php |
Replaces isset ternaries with ?? for multiple RouterOS response fields. |
mikrotik_users.php |
Hardens unserialize() usage and parameterizes a bulk delete query using placeholders. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $dhcp['status'] = $row['status'] ?? 'N/A'; | ||
| $dhcp['expires_after'] = isset($row['expires-after']) ? uptimeToSeconds($row['expires-after']):0; | ||
| $dhcp['last_seen'] = isset($row['last-seen']) ? uptimeToSeconds($row['last-seen']):0; | ||
| $dhcp['active_address'] = isset($row['active_address']) ? $row['active-address']:''; |
There was a problem hiding this comment.
$dhcp['active_address'] checks isset($row['active_address']) but then reads $row['active-address']. Since this data comes from parseResponse() (hyphenated keys like mac-address), the isset() check will never pass, so active_address will be recorded as an empty string even when active-address is present. Use the same key on both sides (e.g., read via $row['active-address'] ?? '', or switch to the underscore-normalized response consistently).
| $dhcp['active_address'] = isset($row['active_address']) ? $row['active-address']:''; | |
| $dhcp['active_address'] = $row['active-address'] ?? ''; |
| $selected_items = unserialize(stripslashes(get_request_var('selected_items')), ['allowed_classes' => false]); | ||
| if (!is_array($selected_items)) { | ||
| $selected_items = []; | ||
| } |
There was a problem hiding this comment.
unserialize() on request data can emit warnings/notices on malformed input (e.g., "Error at offset...") even though you later fall back to []. Since selected_items is user-controlled, consider validating that the value is a plausible serialized array before deserializing, or otherwise handling deserialization errors in a way that avoids noisy logs (while keeping allowed_classes => false).
| // Issue QUEUE-6: use prepared statement for IN() — prevents name-column SQL injection | ||
| if (!empty($devices_to_act_on)) { | ||
| $placeholders = implode(',', array_fill(0, count($devices_to_act_on), '?')); | ||
| db_execute_prepared("DELETE FROM plugin_mikrotik_users WHERE name IN ($placeholders)", $devices_to_act_on); | ||
| } |
There was a problem hiding this comment.
The new if (!empty($devices_to_act_on)) { ... } guard changes behavior when the selection is empty: previously the code would still execute a DELETE with an empty IN ('') list, now it performs no DELETE. This is probably safer, but it contradicts the PR description’s claim of “zero behavioral impact”; either adjust the description or preserve the previous semantics explicitly.
Signed-off-by: Thomas Vincent <[email protected]>
somethingwithproof
force-pushed
the
hardening/comprehensive
branch
from
d064f17 to
a1d3d9b
Compare
Consolidated hardening PR:
3 files changed, 32 insertions(+), 25 deletions(-)
All changes are mechanical transforms with zero behavioral impact. PHP 7.4+ compatible.