phpcs-security-audit icon indicating copy to clipboard operation
phpcs-security-audit copied to clipboard

Support request: Potential XSS found with #value on $raw_form_input

Open hkirsman opened this issue 6 years ago • 1 comments

Could somebody say what to do about this error:

------------------------------------------------------------------------------------------------------------------------
 66 | WARNING | Potential XSS found with #value on $raw_form_input
    |         | (PHPCS_SecurityAudit.Drupal7.XSSFormValue.D7XSSWarFormValue)
------------------------------------------------------------------------------------------------------------------------

The error is coming from where the #value' is red:

function mymodule_file_move_form_submit_handler(array &$form, FormStateInterface $form_state) {

	// Value from form select.
	$raw_form_input = Xss::filter($form['uri_scheme']['markup']['#value']);
	$uri_scheme = Html::escape($raw_form_input);

There's no other key I could use to get the value.

hkirsman avatar Oct 21 '19 09:10 hkirsman

This tool generally gives a WARNING when it thinks that something is a potential issue.

In your case you are using some type of filtering functions that the tool doesn't recognize.

For Drupal it's actually:

'check_plain', 't', 'l', 'url', 'drupal_attributes', 'drupal_render_children', 'drupal_render'

https://github.com/FloeDesignTechnologies/phpcs-security-audit/blob/master/Security/Sniffs/Drupal7/Utils.php#L31

Currently the tool doesn't support customization of the sort to remove false positive, but it's certainly a feature that would be awesome to have.

You could technically add your function to that list I stated above, but right now the sniff code path for it doesn't check mitigation: https://github.com/FloeDesignTechnologies/phpcs-security-audit/blob/master/Security/Sniffs/Drupal7/XSSFormValueSniff.php#L40

If you wanna hack that file too then with both hacks together it will dismiss the warning:

			if ($next == $closer && $tokens[$next]['code'] == T_SEMICOLON)  {
				$funcprv = $phpcsFile->findPrevious(T_OPEN_PARENTHESIS, $next);
				if (!in_array($tokens[$funcprv - 1]['content'], $utils::getXSSMitigationFunctions())) {
					// Case of $label = $element['#value'];
					$next = $phpcsFile->findPrevious(\PHP_CodeSniffer\Util\Tokens::$assignmentTokens, $next);
					$next = $phpcsFile->findPrevious(T_VARIABLE, $next);
					$phpcsFile->addWarning('0Potential XSS found with #value on ' . $tokens[$next]['content'], $next, 'D7XSSWarFormValue');
				}

I think we should open 2 issues following your comment:

  • One for the bug in Drupal7\XSSFormValueSniff of not checking getXSSMitigationFunctions on the Case of $label = $element['#value']; (and we can only do after after we prove that a #value is considered safe after a check_plain and the others)
  • One for the feature or adding custom mitigation functions

Let me know if this would solve your current concerns.

jmarcil avatar Feb 18 '20 07:02 jmarcil