[somethingwithproof]

Security hardening for the Weathermap plugin. Each change is covered by a test and was verified against a live Cacti 1.2.31 instance on PHP 8.4 (Docker).

Changes

• Gate check.php behind the Manage Weathermap realm. The pre-install report disclosed the PHP version, host name, kernel, ini paths and loaded extensions to any anonymous requester. It now includes auth.php and is registered in the Manage realm (install and upgrade) so auth.php authorizes it; the CLI run stays open for the install check.
• Escape user-controlled output. Map titles, info URLs and config filenames rendered on the management pages, in session messages, in the generated image map and in the RSS feed now pass through html_escape() / __esc(), and a % in a title no longer breaks the feed.
• Confine the output-file directives. OUTPUTFILE, HTMLOUTPUTFILE, IMAGEOUTPUTFILE and DATAOUTPUTFILE are resolved and accepted only as a bare filename inside the plugin output/ directory, so a map config can no longer make the poller (or the mgmt "rebuild now" path) write or overwrite an arbitrary file.
• Honour disabled groups in the viewer list. get_allowed_weathermaps() now filters on uag.enabled = 'on', matching is_weathermap_allowed(), so members of a disabled group no longer see that group's maps in the list and cycle views.
• Guard the format-string path. An invalid format in a map label, title or note falls back to literal text instead of throwing an ArgumentCountError/ValueError that aborted the whole poller cycle on PHP 8.

Verification

• Plugin unit suite (Pest) green; new tests under tests/Security/ and tests/Unit/ cover each change.
• Live checks on Cacti 1.2.31 / PHP 8.4: check.php serves the report only to Manage users (anonymous → login, view-only → permission denied); an absolute/.. output-file path is rejected while a bare filename still writes to output/; a disabled-group member no longer sees the map; a malformed label no longer aborts the poll.

Corresponding details are tracked privately through the project's security advisory process.

[alcatron]

Checked 15f1469 locally. There are a few things I'd like addressed before this merges:

• weathermap_confine_output_path() checks the parent directory but accepts an existing destination symlink pointing outside output/. I confirmed this using disposable local files. This requires a pre-existing symlink; I haven't shown that a map editor can create one. The write path still needs to handle that case before it can guarantee confinement.
• The standalone PHP suite has three failures because MapDuplicatePayload.php and ConfigWriteAtomicRegression.php extract map_duplicate() without loading the new map_clean_title() helper. Loading the actual helper in those fixtures fixed the failures locally. This appears to be a fixture problem, not evidence that normal duplication is broken.
• CI stops at the translation-catalogue check. The catalogue needs updating for this PR.
• The new filename rule also rejects existing paths and names such as network.prod.png, names with spaces and uppercase extensions. Could the intended restrictions and migration be documented? Some of these are ordinary filenames even though the new check rejects them.

After the fixture corrections and the separate #341/#347 browser-test adjustment, the combined batch passed 660 PHP tests and 33 browser test files. Those results don't resolve the confinement issue above.

[somethingwithproof]

Thanks for the careful check. Addressed in d1a589e and c1e681a:

• An existing destination that is a symbolic link (including a dangling one), has a second hard link, or is a directory is now refused, so a write can't change a file outside output/.
• File names may contain dots and spaces and use any case in the extension (network.prod.png, Core Links.html, MAP.PNG). A path whose directory is output/ (relative to the plugin directory, or absolute) is accepted, so existing configs that already pointed there keep working. A script extension in the middle of a name, such as map.php.png, is refused.
• The accepted names, and what happens to anything else (ignored with a WMPOLL08 warning; the map is still drawn into output/), are documented under HTMLOUTPUTFILE in the configuration reference, with a CHANGELOG note.
• MapDuplicatePayload.php and ConfigWriteAtomicRegression.php now load the real map_clean_title().
• The translation catalogue was regenerated in 77a1010.

All 405 PHP tests pass on PHP 8.3, including new cases for each of the above.

[alcatron]

Rechecked 6624e72. The existing symlink, dangling symlink, hard-link and directory cases are now rejected. I also checked dotted filenames, spaces, uppercase extensions and paths inside output/; those are accepted, while outside paths and script-extension names are refused. The map-title fixtures now load the helper correctly.

The eight active PRs together pass 648 PHP tests and all 33 browser test files, without our previous test fixes. The specific issues I raised are addressed.

Merge housekeeping: this conflicts with the batch changelog from #333 and the catalogue creation-date header from #336. I retained both sets of changelog entries and the newer catalogue date in the test checkout; no code or test edits were needed. Minor nit: please preserve the catalogue version and Cacti translator/team metadata when resolving the header, rather than the regenerated blank version/placeholders.

[TheWitness]

Addressed the "previously missed" title-length finding

The latest review overview flagged a HIGH issue in otherwise-unchanged code: a duplicated-map title long enough to cross ReadConfig()'s fgets($fd, 4096) chunk boundary could inject a config directive (weathermap-cacti-plugin-mgmt.php:2005). This is the same class the PR's map_clean_title() targets, so I've closed it in 863cd0f.

• map_duplicate() now rejects a cleaned title longer than 4088 bytes before writing the config — matching newMap()'s existing guard and reusing its message (Map title is too long. Use at most 4088 bytes after escaping.), so there's no new translatable string.
• Rationale: TITLE (6 bytes) + 4088 + newline = 4095 bytes, the most that fits in one fgets($fd, 4096) read, so the overflow can never become a separate directive line.
• Regression (MapDuplicateTitleBoundaryPayload.php / MapDuplicateTitleBoundaryTest.php) drives the real map_duplicate() in a subprocess and asserts: a 4088-byte title reaches the write (TITLE line = 4094 bytes), while a 4089-byte title and a …INCLUDE …-bearing oversized title are both rejected before any config is written. I confirmed the test is meaningful by temporarily lifting the cap — the 4089-byte case then reaches the write.

Verification

• Full plugin Pest suite: 672 passing (3 new). The only non-pass remains the pre-existing BackgroundSizingEngineTest separate-process fixture, which is environmental in my local harness (fails identically without my changes) and passes under phpunit directly.
• php -l clean on all modified files.

[Copilot]

🔵 Needs a closer look

Human assessment is needed because preserved TITLE indentation can bypass the length guard, allowing duplicated-title content to be parsed as a separate configuration directive.

No open issues 🎉

_{Review effort: Balanced}

[TheWitness]

Addressed the preserved-indentation follow-up

The latest review overview flagged for human assessment that preserved TITLE indentation could bypass the length guard. That's a valid edge, and I've closed it in 8402e62.

Why it was reachable: the length pre-check (strlen($save['titlecache']) > 4088) assumes the global TITLE sits at column 0, as WriteConfig() emits it. But map_duplicate() injects the new title with str_replace("TITLE {old}", "TITLE {new}", $contents), which preserves any leading whitespace on the source config's TITLE line. I confirmed ReadConfig() reads with fgets($fd, 4096), pushes each chunk to $lines[], and trim()s each before matching directives — so a title that clears the pre-check (≤4088 bytes) but sits on an indented line could still push the written line past 4095 bytes and have its trimmed tail parsed as a separate directive.

The fix adds an authoritative post-substitution check: after the str_replace, any written ^\s*TITLE\s line longer than 4094 bytes (the most that still fits—with its newline—in one fgets($fd, 4096) read) is rejected before the config is written, reusing the existing message (no new translatable string).

Verification

• Extended MapDuplicateTitleBoundaryTest with an indent case: a 4086-byte title on a source TITLE line indented by 20 bytes clears the pre-check but is rejected by the post-check. I confirmed it's meaningful by temporarily lifting the post-check threshold — the indented case then reaches the write.
• Full plugin Pest suite: 673 passing (the only non-pass remains the pre-existing environmental BackgroundSizingEngineTest separate-process fixture; passes under phpunit directly). php -l clean.

I also replied to and resolved the SonarCloud eval note on the new test fixture — it's the established test-support pattern (same as MapDuplicatePayload.php), evaluating the plugin's own first-party source with no user-controlled input.

[Copilot]

🔵 Needs a closer look

Human assessment is needed because title substitution can overflow comment lines past the read limit, allowing injected directives to bypass both guards.

No open issues 🎉

_{Review effort: Balanced}

[TheWitness]

Addressed the comment-line overflow follow-up

The review overview flagged that title substitution can overflow non-TITLE lines (e.g. comments) past the read limit. Valid — closed in c027e5a.

The substitution is a whole-file str_replace("TITLE {old}", "TITLE {new}", $contents), so it rewrites the string on every line that contains it, not only the global TITLE line. A crafted source config with # TITLE {old} (or a label carrying that text) would, after substituting a long title, overflow that line past ReadConfig()'s fgets($fd, 4096) boundary; the first chunk keeps the leading # (treated as a comment) while the split tail becomes a separate $lines[] entry that is trim()-ed and parsed as a directive. My previous guard only scanned ^\s*TITLE\s lines, so it missed this.

The guard now validates every line the substitution lengthens: for each source line containing the TITLE {old} marker, it computes the rewritten line and rejects if it would exceed 4094 bytes (the most that still fits with its newline in one fgets($fd, 4096) read), before the config is written. This is precisely scoped to changed lines, so it doesn't reject unrelated pre-existing content, and it reuses the existing message (no new translatable string).

Verification

• Added a comment regression: a # TITLE Original Map source line with a 4088-byte title keeps the real TITLE line at 4094 but pushes the comment line to 4096; it's now rejected. Confirmed meaningful by lifting the threshold — it then reaches the write.
• Full plugin Pest suite: 674 passing (only the pre-existing environmental BackgroundSizingEngineTest separate-process fixture is unaccounted; passes under phpunit). php -l clean.

[Copilot]

🟢 Approval recommended

No active review issues remain for the access-control, output-safety, and file-write hardening changes.

No open issues 🎉

_{Review effort: Balanced}

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[TheWitness]

CI green + follow-up review items addressed

All checks now pass (PHP 8.2–8.6 integration, CodeQL, jsdom, both SonarCloud gates). Summary of the follow-ups since the last review round:

Review findings (all resolved threads)

• Executable-scheme info URLs → wm_href_is_safe() (http(s)/relative only, nohref otherwise).
• HTML-entity-encoded schemes (javascript:, javascript:) → decode once before the scheme check; test html_escape() stub made faithful to core (double_encode=false).
• Per-directive output extensions (DATAOUTPUTFILE can't become an includable .html).
• Duplicate-title config-injection across ReadConfig()'s fgets(4096) boundary — title length, preserved indentation, and non-TITLE lines (comments) — now guarded by map_duplicate_title_overflows_line() in lib/map-title.php.
• check.php realm migration now runs (INFO bumped to 2.1).

CI fixes

• Patch coverage 100%: covered the wm_href_is_safe() guard branches (non-string / whitespace-only inputs) and collapsed the default_charset fallback to a single expression.
• SonarCloud: removed the eval()/subprocess test fixture by extracting the line-overflow check into the testable map_duplicate_title_overflows_line() helper (tested directly), and de-duplicated the image-map and output-confinement tests to clear the new-code duplication gate.

Validation

• Plugin Pest suite: 684 passing locally (the only non-pass is the pre-existing BackgroundSizingEngineTest separate-process fixture, an environmental php:// wrapper issue locally that passes in CI).
• php -l clean on all modified files; version bumped to 2.1 (INFO + CHANGELOG).

[Copilot]

🟢 Approval recommended

No active review issues are recorded for the access-control, output-safety, and poller-hardening changes.

No open issues 🎉

_{Review effort: Balanced}