Skip to content

hardening(forms): render input attributes safely#7253

Merged
TheWitness merged 9 commits into
Cacti:developfrom
somethingwithproof:hardening/form-output-contexts
Jul 24, 2026
Merged

hardening(forms): render input attributes safely#7253
TheWitness merged 9 commits into
Cacti:developfrom
somethingwithproof:hardening/form-output-contexts

Conversation

@somethingwithproof

@somethingwithproof somethingwithproof commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Adds small HTML input rendering helpers that escape attribute names and values, then migrates package import hidden fields and the aggregate graph rfilter textbox away from raw value interpolation.

This narrows two recurring reflected-XSS patterns: serialized hidden state in single-quoted attributes and request-derived filter text in text input values. Package import state now uses safer form helpers instead of hand-built hidden inputs.

Validation: local PHP syntax checks passed for the changed files. GitHub checks pass for PHP 8.1, 8.2, 8.3, 8.4, and the Pest coverage gate.

Part of #7371

@somethingwithproof
somethingwithproof marked this pull request as ready for review July 7, 2026 17:48
Copilot AI review requested due to automatic review settings July 7, 2026 17:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 HTML form rendering in Cacti by introducing centralized helpers for safely rendering <input> attributes and then adopting them in a couple of high-risk spots (package import hidden fields and aggregate graph filtering) to reduce reflected-XSS exposure.

Changes:

  • Add html_attributes(), html_hidden_input(), and html_text_input() helpers in lib/html.php to consistently escape attribute values.
  • Replace raw hidden-field serialization in package_import.php with a session-backed import_state token and render hidden inputs via the new helper.
  • Migrate the Aggregate Graphs “rfilter” textbox to html_text_input() and add unit tests covering the new helpers.

Reviewed changes

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

File Description
lib/html.php Adds attribute/input rendering helpers that escape attribute values (and sanitize attribute names).
package_import.php Switches import execution state from request-hidden serialized blobs to a session token, and uses helper-rendered hidden inputs.
aggregate_graphs.php Uses the new helper to safely render the request-derived rfilter textbox value.
tests/Unit/HtmlEscapeTest.php Adds unit tests for the new helpers’ escaping behavior.

Comment thread lib/html.php
Comment thread lib/html.php Outdated
Comment thread package_import.php
Comment thread tests/Unit/HtmlEscapeTest.php
@somethingwithproof
somethingwithproof force-pushed the hardening/form-output-contexts branch 2 times, most recently from c45d2bb to 1f44c43 Compare July 11, 2026 23:43
@somethingwithproof somethingwithproof added the develop Bug in the development branch label Jul 12, 2026

@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.

CI is failing on this one.

@TheWitness

Copy link
Copy Markdown
Member

@somethingwithproof, it looks like your commits are not running the pre-commit hooks. Failing from a PSR issue now.

@somethingwithproof

Copy link
Copy Markdown
Contributor Author

Addressed in 8f519b2.

The failing CI check was PHP-CS-Fixer only: its required alignment for the package-import state token is now applied. No runtime behavior changed.

Validation: PHP lint and diff check passed; the PR CI rerun is the remaining confirmation.

@TheWitness
TheWitness requested a review from a team as a code owner July 13, 2026 19:54
@TheWitness
TheWitness requested review from cigamit and removed request for a team July 13, 2026 19:54
@somethingwithproof somethingwithproof self-assigned this Jul 14, 2026
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
somethingwithproof force-pushed the hardening/form-output-contexts branch from 9ec556b to 3319c47 Compare July 14, 2026 10:46
@somethingwithproof

Copy link
Copy Markdown
Contributor Author

The previously reported formatting check is now passing on the latest completed CI run; the remaining review gate is ready for re-check.

@somethingwithproof

Copy link
Copy Markdown
Contributor Author

@copilot review

@somethingwithproof

Copy link
Copy Markdown
Contributor Author

CI failure fixed. Green now.

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

Copy link
Copy Markdown
Contributor Author

Fixed, that was the php-cs-fixer PSR issue the pre-commit hook catches on the test file.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Copilot AI review requested due to automatic review settings July 22, 2026 20:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Comment thread package_import.php
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Copilot AI review requested due to automatic review settings July 24, 2026 09:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

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

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

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

Comment thread package_import.php
Comment thread package_import.php
Comment thread package_import.php
Comment thread package_import.php
Comment thread lib/html.php
Copilot AI review requested due to automatic review settings July 24, 2026 16:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot AI review requested due to automatic review settings July 24, 2026 23:30
@TheWitness
TheWitness merged commit da9d7e7 into Cacti:develop Jul 24, 2026
19 checks passed

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

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

Comments suppressed due to low confidence (1)

package_import.php:140

  • When import_state is present, the code overrides several import-affecting inputs from the stored state, but drp_action is still read from the request later (e.g. if (grv('drp_action') == 1)). That means a valid import_state token can still be replayed with a mismatched drp_action, undermining the “bind every import-affecting input” goal and potentially altering behavior.

At minimum, force drp_action to the expected action when consuming an import_state token (or ideally store/restore it from the state token as well).

	if (isrv('import_state')) {
		$import_state = package_import_take_state(grv('import_state'));

		if ($import_state === false) {
			raise_message('invalid_import_state', __('The selected package import state has expired or is invalid. Please review the package and try again.'), MESSAGE_LEVEL_ERROR);
			header('Location: package_import.php');

			exit;
		}

		$package_location = $import_state['package_location'];
		$profile_id       = $import_state['data_source_profile'];
		$remove_orphans   = $import_state['remove_orphans'];
		$replace_svalues  = $import_state['replace_svalues'];
	}

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

Labels

develop Bug in the development branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants