fix(security): defense-in-depth hardening for plugin_reportit - #129
Draft
somethingwithproof wants to merge 4 commits into
Draft
fix(security): defense-in-depth hardening for plugin_reportit#129somethingwithproof wants to merge 4 commits into
somethingwithproof wants to merge 4 commits into
Conversation
Automated fixes: - XSS: escape request variables in HTML output - SQLi: convert string-concat queries to prepared statements - Deserialization: add allowed_classes=>false - Temp files: replace rand() with tempnam() Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
There was a problem hiding this comment.
Pull request overview
Defense-in-depth security hardening for the ReportIt plugin UI and shared utilities, primarily focusing on reducing XSS risk in rendered HTML and tightening request parameter handling.
Changes:
- Escapes
filterrequest values when rendering into HTML<input value=...>attributes. - Tightens
idpropagation into URLs by switching to filtered request access and casting to integer in a few UI flows. - Attempts to harden
unserialize()usage to disallow object instantiation (but currently contains a runtime-breaking bug).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
view.php |
Escapes filter in input fields; uses filtered/cast id when building JS navigation URLs. |
templates.php |
Escapes filter in template search input value. |
reportit.php |
Escapes filter in report filters and item search; uses filtered/cast id in form action + JS URLs. |
lib/funct_shared.php |
Attempts to add allowed_classes => false to unserialize() in cache/table transformation logic (implementation currently incorrect). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Change Dependabot ecosystem from npm to composer (PHP-only repo) - Remove PHP from CodeQL paths-ignore so security PRs get analysis - Remove committed .omc session artifacts, add .omc/ to .gitignore Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
somethingwithproof
marked this pull request as draft
April 11, 2026 00:10
Author
|
Converted to draft to serialize the stack in this repo. Blocked by #128; will un-draft after that merges to avoid cross-PR merge conflicts. |
stripslashes() only takes one argument, so the allowed_classes hardening was silently dropped and unserialize() ran unprotected. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
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.
Summary
Automated defense-in-depth hardening addressing 45 security audit findings.
html_escape_request_var()allowed_classes => falsetounserialize()callsrand()withtempnam()All changes are PHP 7.0+ compatible for Cacti 1.2.x.
Test plan