FloeDesignTechnologies / phpcs-security-audit

phpcs-security-audit is a set of PHP_CodeSniffer rules that finds vulnerabilities and weaknesses related to security in PHP code
GNU General Public License v3.0
710 stars 86 forks source link

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

Open hkirsman opened 4 years ago

hkirsman commented 4 years ago

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.

jmarcil commented 4 years ago

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:

Let me know if this would solve your current concerns.