Skip to content

harden(weathermap): fix strict false comparison for strpos in target quoting#227

Merged
TheWitness merged 9 commits into
Cacti:developfrom
somethingwithproof:hardening/weathermap
May 19, 2026
Merged

harden(weathermap): fix strict false comparison for strpos in target quoting#227
TheWitness merged 9 commits into
Cacti:developfrom
somethingwithproof:hardening/weathermap

Conversation

@somethingwithproof
Copy link
Copy Markdown
Contributor

@somethingwithproof somethingwithproof commented May 17, 2026

Summary

  • Fix strpos($target[4], ' ') == false=== false at three sites in WeatherMapLink::WriteConfig(), WeatherMapLink::asJS(), and WeatherMapNode::WriteConfig()
  • Apply cacti_escapeshellarg() to the rrdtool binary path in both ReadData call sites of WeatherMapDataSource_rrd.php
  • Apply cacti_escapeshellarg() to the fping binary path in WeatherMapDataSource_fping.php
  • Guard rrd_options against quoted/backslash arguments that would break shell word splitting; emit cacti_log() at POLLER_VERBOSITY_LOW alongside wm_warn() so operators see the rejection without enabling debug verbosity
  • html_escape($map->title) in map title output; prepared statement in weathermap-cacti-plugin.php cacti-mapper query
  • Use db_fetch_cell_prepared() for the version check in setup.php
  • Add empty-curvepoints guard in DrawComments and the post-draw label path to prevent fatal TypeError on PHP 8.x when degenerate geometry produces an empty curvepoints array

Why

strpos returns 0 (not false) when the match is at position 0; loose == comparison incorrectly skips quoting for strings that begin with a space. The shell-escape and rrd_options guard address a class of argument injection that would allow rrdtool to receive attacker-controlled arguments. The PHP 8 curvepoints crash is a regression path on any map with degenerate link geometry.

Test plan

  • Verify a target string with no spaces writes without quotes
  • Verify a target string with an embedded space is quoted
  • Verify a target string beginning with a space (strpos returns 0) is treated as containing a space and quoted
  • Set rrd_options to a value containing a " quote; confirm the map logs a WEATHERMAP warning visible in the standard Cacti poller log and no data is corrupted
  • Render a map with a link that has zero waypoints (degenerate geometry); confirm no PHP fatal error on PHP 8

Copilot AI review requested due to automatic review settings May 17, 2026 09:58
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 hardens target serialization in the WeatherMap link and node classes by fixing loose strpos() comparisons so leading spaces are detected correctly.

Changes:

  • Uses === false when checking whether target strings contain spaces.
  • Applies the fix consistently in link config output, link JavaScript output, and node config output.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
lib/WeatherMapLink.class.php Fixes strict space detection for link target quoting in config and JavaScript serialization.
lib/WeatherMapNode.class.php Fixes strict space detection for node target quoting in config serialization.

…quoting

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…aptitle, prepared cacti-mapper query

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

Reject extra_options containing quotes or backslashes with a wm_warn log
entry rather than silently tokenizing flag=value pairs that contain spaces.
Flags like --title "My Map" would otherwise become three malformed tokens.
Documents that this field accepts only space-separated single-token flags.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
somethingwithproof and others added 3 commits May 17, 2026 04:39
…options log visibility

setup.php: db_fetch_cell -> db_fetch_cell_prepared per project rule.

WeatherMapLink: DrawComments and the post-draw label path both crash with
TypeError on PHP 8 when curvepoints is empty. Add early return guard.

WeatherMapDataSource_rrd: rrd_options rejection was silent at default
verbosity. Supplement wm_warn with cacti_log at POLLER_VERBOSITY_LOW so
operators see the skip without enabling debug logging.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: TheWitness <thewitness@cacti.net>
Signed-off-by: TheWitness <thewitness@cacti.net>
@TheWitness TheWitness requested review from bmfmancini and xmacan May 18, 2026 17:16
@TheWitness TheWitness merged commit 5fd6ab7 into Cacti:develop May 19, 2026
4 checks passed
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.

5 participants