Strings as assert expressions are deprecated.

[gabesullice]

Hi there!

Thanks for providing such a useful library! I'm currently using this library on a project via the https://github.com/acquia/coding-standards-php project.

Today, I received the following warning:

Assert eval function assert() detected with dynamic parameter

which I tracked down to this rule: https://github.com/FloeDesignTechnologies/phpcs-security-audit/blob/master/Security/Sniffs/BadFunctions/AssertsSniff.php

The rule could be triggered by something as simple as an assert(TRUE) and could be fixed by changing it to assert('TRUE') (added quotes).

Since PHP 7.2.0, using strings as the first argument to assert() has been deprecated.

Can this rule be removed? Perhaps I'm misinterpreting it and there's another resolution that won't trigger deprecations on the latest versions of PHP. I'd love to know your thoughts :)

For the moment, I'm working around it by excluding this rule. See acquia/coding-standards-php#8

[jmarcil]

Hello,

thank you for your feedback! Glad this tool is still useful nowadays.

It appears to me that the rule might be bugged. This goes even bigger than the assert() rules you're mentioning.

Technically the tool shouldn't say it's a dynamic parameter when it's clearly a static value. Currently it looks at this list of tokens:
https://github.com/FloeDesignTechnologies/phpcs-security-audit/blob/master/Security/Sniffs/Utils.php#L6

The reason for it not to be on that list, is that apparently TRUE is a T_STRING, and a T_STRING can be a function for the parser. That means that if you have a function that is called TRUE(), the parser sees the same type of value for both. Oh PHP 🤷‍♂.

The bad news then, is that while fixing this is doable, it won't be easy. We need a special mechanism to identify TRUE/FALSE values (btw True, tRue, tRuE, etc. are valid values for TRUE) when they are not a function. This differs from the current ways of doing with staticTokens.

I think we should open a bug describing this and would help with reducing false positive.

Now back to your request, while you are more than welcome to disable that rule in your project, you have to take in account that not all projects scanned with this tool are using the current version, so we can't simply remove the rule. Also, this would still work on 7.2 and can be a problem.

Note that the rule actually checks for all parameters and that even when the first parameter will be safe at some point, the second named description will remain a concern. Right now you might actually miss an issue by disabling it in your own project.

So in summary, I would open two issues to help with your current problem:

• Open a bug that the tool doesn't have a way to mark as staticTokens boolean values
• Open an improvement on the assert() rule to make it aware about the various parameters and some tweaking to support the staticTokens boolean values bug fix

Thanks!

[jmarcil]

Note to self: We can quick patch this in AssertsSniff by checking if strtolower($tokens[$s]) is true or false. Might be good to do before closing this issue.

[gabesullice]

@jmarcil, thank you so much for the thoughtful reply! Much appreciated 🙏

Unfortunately, the example case I gave using TRUE is not really the case I care about, it was only the simplest example that I could come up with to illustrate the problem.

A more real-world(-ish) example would be as follows:

public function rgb_to_css_hex($red, $blue, $green) {
  assert(is_int($red) && $red >= 0 && $red <= 255);
  assert(is_int($blue) && $blue >= 0 && $blue <= 255);
  assert(is_int($green) && $green >= 0 && $green <= 255);
  // Convert color values to hex.
  return $hex_code // e.g. '#ffab22'
}

In PHP 7, assertions are never executed and are cleanly optimized away if zend.assertions is in "production mode" (prior to this, they were evaluated, unless the conditional was written as a string).

Since one shouldn't have this setting in "development mode" on production, I feel that worrying too much about non-string assertions might be overzealous. IOW, the primary security vulnerability is that one has assertions enabled in production, not that one has assertions using user-input. That seems like a secondary concern.

[jmarcil]

Thank you for the detailed examples!

I'd like to put the emphasis that the goal of this tool is to be overzealous about everything when Paranoia mode is turned on 😅.

I understand that in your specific case you have good reasons of not needing it, such as using PHP 7 and "shouldn't be in dev mode", but the rules in this tool have been created to support PHP 5.4 and bad settings. They also have to be able to make a decision on a line-by-line basis, thus limiting the ways of being certain about false positive.

I can see the case of an assert(FALSE, $_GET['a']) to be problematic if the output is rendered in an HTML page on older PHP setups.

With that in mind, I'm afraid I don't have the justifications to remove this rule, sorry.

That said, I still want to address your case, and skip the check for the first parameter with a special setting like I did for EasyXSS:

<rule ref="Security.BadFunctions.EasyXSS">
	<properties>
		<!-- Comment out to follow global ParanoiaMode -->
		<property name="forceParanoia" value="1"/>
	</properties>
</rule>

So, on your end, instead of disabling the full rule, you could just set that forceParanoia to 0 and have the same results while keeping the rule functional for other oddities that might be problematic.

Would you use be able to use that instead of being forced to disable the rule?