fix(security): defense-in-depth hardening for plugin_audit#52
fix(security): defense-in-depth hardening for plugin_audit#52somethingwithproof wants to merge 4 commits intoCacti:developfrom
Conversation
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
There was a problem hiding this comment.
Pull request overview
Defense-in-depth hardening for the Cacti audit plugin, focused on reducing attack surface (XSS mitigation in UI output) and adding repository-level security automation (CodeQL + Dependabot).
Changes:
- Escape the
pagerequest variable before rendering it into an HTML hidden input. - Add a CodeQL workflow for JavaScript/TypeScript analysis.
- Add a Dependabot configuration for GitHub Actions (and npm).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
audit.php |
Escapes a request variable before embedding it into HTML output. |
.github/workflows/codeql.yml |
Introduces CodeQL scanning workflow (JS/TS matrix). |
.github/dependabot.yml |
Enables Dependabot updates for GitHub Actions and npm. |
| - package-ecosystem: "npm" | ||
| directory: "/" | ||
| schedule: | ||
| interval: "weekly" | ||
| open-pull-requests-limit: 10 |
There was a problem hiding this comment.
The Dependabot config enables the npm ecosystem at directory "/", but this repository does not contain a package.json/package-lock.json. This will cause Dependabot runs for npm to fail with "manifest file not found". Remove the npm entry, or add an npm manifest (and ensure it matches the actual JS dependency management) before enabling npm updates.
| - package-ecosystem: "npm" | |
| directory: "/" | |
| schedule: | |
| interval: "weekly" | |
| open-pull-requests-limit: 10 |
| </tr> | ||
| </table> | ||
| <input type='hidden' id='page' value='<?php print get_request_var('page');?>'> | ||
| <input type='hidden' id='page' value='<?php print html_escape_request_var('page'); ?>'> |
There was a problem hiding this comment.
The PR description mentions converting string-concatenated SQL to prepared statements, but audit.php still builds SQL via string concatenation with request variables (e.g., in audit_export_rows() and the main audit_log() query construction). Either include those SQL changes in this PR or adjust the PR description to match the actual scope.
- 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>
|
Converted to draft to serialize the stack in this repo. Blocked by #51; will un-draft after that merges to avoid cross-PR merge conflicts. |
Summary
Defense-in-depth hardening addressing 7 security audit findings.
html_escape_request_var()allowed_classes => falsetounserialize()All changes PHP 7.0+ compatible.
Test plan