Skip to content

fix(security): defense-in-depth hardening for plugin_npc - #18

Open
somethingwithproof wants to merge 10 commits into
Cacti:developfrom
somethingwithproof:fix/defense-in-depth
Open

somethingwithproof wants to merge 10 commits into
Cacti:developfrom
somethingwithproof:fix/defense-in-depth

Conversation

@somethingwithproof

Copy link
Copy Markdown
Member

Summary

Defense-in-depth: SQLi prepared statements, unserialize hardening, XSS escaping.
39 findings addressed.

  • PHP lint clean
  • Pre-push review PASS
  • Verify functionality

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR applies defense-in-depth security hardening across the plugin and its bundled Doctrine components, primarily by tightening unserialize() usage to reduce PHP object-injection risk.

Changes:

  • Harden multiple unserialize() call sites by setting allowed_classes => false.
  • Update Doctrine cache/parser/query paths to use the hardened unserialize behavior.
  • Update NPC settings handling to use hardened unserialize when loading persisted UI state.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
lib/Doctrine/Table.php Hardens Doctrine field preparation by restricting classes during unserialization.
lib/Doctrine/Query/Abstract.php Hardens cached query reconstruction by restricting classes during unserialization.
lib/Doctrine/Parser/Serialize.php Hardens parser load path by restricting classes during unserialization.
lib/Doctrine/Cache/Db.php Hardens DB cache fetch path by restricting classes during unserialization.
controllers/settings.php Hardens user settings load path by restricting classes during unserialization.
controllers/layoutDev.php Attempts to harden persisted UI state load for dev layout (but currently introduces a call-site bug).
Comments suppressed due to low confidence (1)

lib/Doctrine/Query/Abstract.php:1159

  • getCachedForm() serializes $customComponent, which can be a Doctrine_Collection (and therefore includes Doctrine object graphs). Unserializing with allowed_classes => false will prevent those Doctrine classes from being instantiated, so result/query cache hydration can return __PHP_Incomplete_Class objects and break consumers expecting Doctrine_Collection / records.

To keep the hardening without breaking caching, consider an allowlist of the Doctrine classes that are expected here, or change the cached form to avoid serializing objects (e.g., cache arrays/scalars only).

        $cached = unserialize($cached, array('allowed_classes' => false));
        $this->_tableAliasMap = $cached[2];
        $customComponent = $cached[0];

        $queryComponents = array();

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread controllers/layoutDev.php Outdated
Comment thread controllers/layoutDev.php Outdated
Comment thread controllers/settings.php Outdated
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>
- 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>
The single-quoted regex strings contained ['"] character classes
which PHP parsed as string termination. Escape the inner quotes.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
- comments.php: whitelist sort columns to prevent SQL injection via
  $this->sort/$this->dir
- layoutDev.php: use db_fetch_cell_prepared for user_id binding, pass
  allowed_classes option to unserialize() instead of db_fetch_cell()

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

@TheWitness TheWitness left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No compose.json in plugins.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants