Exclude the Security.BadFunctions.Asserts sniff.

[gabesullice]

Security.BadFunctions.Asserts sniff forces assert() function calls to use a string as their first parameter.

Using strings as the first argument to assert() has been deprecated since PHP 7.2.0. Additionally, Drupal 8 no longer supports PHP 5, which required the first argument to assert() to be a string. Finally, versions of PHP 7 in which strings were not yet deprecated are no longer supported.

See: https://www.php.net/manual/en/function.assert.php#refsect1-function.assert-changelog See: https://www.php.net/supported-versions.php

[danepowell]

Thanks for reporting. Your description and proposed fix make sense, although it seems like this is really just a workaround for an issue that needs to be fixed in phpcs-security-audit.

Would you mind opening an issue for this at https://github.com/FloeDesignTechnologies/phpcs-security-audit and linking it here? If it doesn't seem likely to be fixed upstream I'm happy to merge this in the meantime.

cc @TravisCarden (not sure if you subscribe to new issues)

[gabesullice]

Thanks for reviewing this @danepowell! I already talked to @TravisCarden about it this morning, but he hasn't had time to review it because he's travelling to DrupalCampNJ.

I went ahead and opened an upstream issue, per your suggestion: https://github.com/FloeDesignTechnologies/phpcs-security-audit/issues/49

[danepowell]

Thanks, I'm going to give the phpcs-security-audit maintainers at least a few days to respond before moving on this.

[wimleers]

6 days later, the only reactions on https://github.com/FloeDesignTechnologies/phpcs-security-audit/issues/49 are @danepowell and I leaving 👀 reactions.

[danepowell]

Fair enough, since there's been no traction upstream I'll merge this.

[jmarcil]

phpcs-security audit maintainer here 👋

I wish I would live in the same world as you guys and consider 6 days a long time for someone to respond to a GitHub issue. My response time for this project goes up to 6 months.. hopefully we'll get more maintainers soon.

That said, I don't want to scare you, but I confirm that the rule actually checks for all parameters, not just the first one. The second one description is actually important and still supported. Disabling this could possibly lead to false negative (missing a vulnerability) in the case of: assert(TRUE, $_GET['xssme']);

A clean fix for TRUE/FALSE values as not dynamic content would be difficult, but I think I found a way to solve the current issue with assert() quickly without having to disable the rule. ~I might merge to master soon~ edit: Merged in master, but no ETA on the release for it as we changed a few fundamental things.

[gabesullice]

@jmarcil , thanks for your reply and chiming in on our issue. That's going above and beyond :)

I wish I would live in the same world as you guys and consider 6 days a long time for someone to respond to a GitHub issue. My response time for this project goes up to 6 months.. hopefully we'll get more maintainers soon.

Please don't feel like we were complaining about your response time. We only wanted to move forward on our own projects sooner rather than later :) it's totally understandable that an open-source project has a long lag time :) Both Wim and I have spent years in the open source community and have a deep appreciation for the often too unacknowledged service that open source maintainers provide. Please know that we really value your tool and your work. Thank you!

[danepowell]

Per Gabe's logic in https://github.com/FloeDesignTechnologies/phpcs-security-audit/issues/49 that asserts should be disabled in production, I think it's fine to leave this rule disabled for now despite the risk of false negatives.

However, in the spirit of belt-and-suspenders security, we should re-enable it once the upstream bug is resolved.

Ditto to the comments re: response time as well, we just needed to unblock our projects and didn't intend any judgement on your own schedule or priorities. :smile:

[wimleers]

Echoing what @gabesullice and @danepowell wrote — I'm sorry we made it sound/feel like we were complaining about your response time, @jmarcil, that was not at all the intent! In Drupal core land, time frames are also measured in months, weeks in a good case, days if it's both simple and if you're lucky.

Thanks so much for this valuable tool!

[jmarcil]

No problem! You guys are too kind! 😂

I would like to point out that the scope of this project (Drupal 8 and PHP 7?) looks different than the scope of the phpcs-security-audit tool itself (PHP 5.4, has to support plain old PHP), so I don't consider the current behavior a bug in the tool.

That said, I've suggested a solution that would solve your current situation with configuration instead of being forced to disable the rule.

Reducing false positives is on my roadmap for the tool, right after getting better documentation, so if you have any more issue like this one where you were forced to disable the rules I'd be interested to know about it.

As a last wink before I go.. I've known Acquia since a relatively long time back when I organized the OWASP Montreal meetups.. and you guys still rocks! 🤘