Add a hook_requirements() that warns if assertions are turned on and discourage/remove double quotes in assert()

assert('Drupal\\Component\\Assertion\\Inspector::assertAllObjects($caches, \'\\Twig_CacheInterface\''); this is unreadable but even worse, it's unwritable.

Well, the reason for this madness is that we want to be able to not execute any code in the case the assertion system is not initialized. We could rewrite that into the following statement:

assert('Drupal\\Component\\Assertion\\Inspector::assertAllObjects($caches, \\Twig_CacheInterface::class');

For assertions that are unwieldy to read or very length there is the option of using an assert function in the class that needs it.

use Drupal\Component\Assertion\Inspector;

class Sample {
  public function __construct( $caches ) {
    assert('static::assertAllCachesAreObjects( $caches )', 'Invalid Argument');
  }

  protected static function assertAllCachesAreObjects( $caches ) {
    return Inspector::assertAllObjects($caches, 'Twig_CacheInterface');
  }

}

Very complicated assertions about argument state should throw exceptions rather than return false in order to relay exactly what part of the assertion was incorrect.

+++ b/core/lib/Drupal/Component/Assertion/CallableAssert.php
@@ -0,0 +1,46 @@
+   * Use callables instead of eval()'d code in assert().
+   *
+   * If calling a method is too much overhead a similar effect can be achieved
+   * by
+   * @code
+   * assert(!assert_options(ASSERT_ACTIVE) || assert($callable, $description));
+   * @endcode
+   *
+   * This method merely makes code more readable.
+   *
+   * @param callable $callable
+   *   If asserts are active and this returns FALSE then a failed assert will

Can we get an example of how to use CallableAssert here - other than than makes sense to me.

+++ b/core/lib/Drupal/Component/Assertion/CallableAssert.php
@@ -0,0 +1,50 @@
+        assert($callable(), $description);
...
+        assert($callable());

if $description is optional, doesn't passing NULL achieve the same thing? I.e can we just use assert($callable(), $description) regardless?
Not sure, have no knowledge of asserts

Part of the point of assertions is that they are test code that is omitted *entirely* in production. This patch creates the situation where the closure in the assert is created but never fires, resulting in an efficiency loss.

Two things to note - some amount of loss in this manner is tolerable. For example:

assert("preg_match('" . ExtensionDiscovery::PHP_FUNCTION_PATTERN . "', '$token') === 1", 'Tokens need to be valid Twig variables.');

In the above the compiler will always concatenate the string to be evaled and replace $token with its current value, but as that is a relatively inexpensive operation it's worth doing for the legibility improvement.

In a similar vein:

assert( $this->object instanceof RequiredInterface );

is acceptable because the instanceof check doesn't cause a noticeable performance loss. Further, it will just end up making PHP 7 look that much better when it becomes the default since PHP 7 can step over even these steps entirely.

My initial thought was that it might be best to consistently use the eval() string method with assert but now, given the legibility concerns, I'm not so sure.

Well, we could not use the eval string method at all and allow PHP 5 to take a massive performance hit once enough assertions are in the system. Or maybe the PHP team will finally own up to what a massive bug their handling of assert() really is and fix the problem in 5.6.x, though that's unlikely.

As far as security concerns - First, assert() should never be working with unsanitized user controlled data at any time. Second, public production systems should have assert() turned off. Between the two I don't see the security argument at all.

Eval() is not a magic door for hackers who's use guarantees the site will go belly up. It does what it says - eval's a string as PHP code. It is no more dangerous (or safe) than using file_put_contents() to write a string to the file system and then include()-ing it.

I find it somewhat irritating to fight the "eval() bad" parrot. I'm being honest with how assert works in PHP 5, I always have been. Be honest with me and learn how eval actually works, what it's vulnerabilities really are and understand that they can be worked around when needed - in other words think, don't parrot.

PHP's buggy implementation of assert() forces arguments sent to the function to be transformed into strings. This is unfortunate, but necessary to realize the performance, stability and even security gains enabled by examining function preconditions explicitly rather than writing them in PHPdocs and then assuming everyone will follow those docs at all time (hint - they won't). Is it hard to read? Sometimes - but it can be done - and to be honest the illegibility is entirely the fault of whichever moron on the PHP team thought having the \ character for a namespace operator was a good idea. I called that idea stupid at the time and I stand by that.

Writing a patch that has partial execution of an assertion - particularly with a closure creation call - generates more points of failures than it removes and as much time as I have invested in introducing assert() to the project I would rather not see it used at all than used in such a brittle and prone to break manner.

Keep in mind most assertions are going to be something like this:

assert('is_string($arg)')

Or an instance of comparison.

assert('$this->object instanceof \\Drupal\\Core\\Template\\TwigExtension')

Sometimes the inspector

assert('\\Drupal\\Component\\Inspector::assertAllStrings($arg)');

Difficulty to read starts coming up with multiple name paths show up, but as I pointed about above such difficult assertions can be placed out into their own static assert function and then that static assertion is invoked with assert(). It's tricky, but not insurmountable - and it isn't going to come up often enough to justify this patch.

One final word on security - there's a world of difference between this:

assert('is_string($arg)');

and this

assert("is_string($arg)");

The second is a security vulnerability - if it runs at all, because double quotes parses variables before the string is sent - never forget that. So in the second case if $arg is ever unsanitized then the statement is a break point. The first case is no more or less vulnerable than calling the expression without the single quotes.

Any reason we couldn't switch to https://github.com/beberlei/assert

check that list https://github.com/beberlei/assert#list-of-assertions

Should we create new issue for #21?

I mean if we execute this code on runtime anyway, is there a reason to not just use ordinary exceptions?

I bet there are other places in core where a change or ' to " would make a security issue. There certainly are places in javascript where changing @ to ! will be a security hole. And there argument about a developer changing the implementation in core/modules/views/src/Plugin/views/PluginBase.php seems silly me - if a developer changes code they may introduce security concerns. Well yes, that much is obvious. What we have here is a technique that needs careful review - but it is a lot easier than reviewing what ends up inside for example #markup for example.

Also by default on apache assertions are disabled because we have
php_value assert.active 0
in our .htaccess file.

For the reasons above, I'm not convinced by the criticality this issue - there is no security hole in the code that is changing. At best according to our issue priorities this is major - but I'm not convinced this is an issue at all.

Perhaps this issue could add something to system_requirements that checks the value of assert_active and informs the user if it is on that is recommended to be off in production environments?

That is certainly a great idea but extending it - perhaps it should be an item in the status report as well?

@Aki Tendo that is how stuff gets to the status report :)

Any reason we couldn't switch to https://github.com/beberlei/assert

check that list https://github.com/beberlei/assert#list-of-assertions

That class is a validator, despite the name, and in no way makes use of PHP's assert() mechanism, so it would need to be wrapped with assert() anyway. As such it doesn't solve any of the legibility problems brought up. It could displace Inspector, but instead of returning false it throws a designated exception on failure, which would break any tests looking to catch AssertionError. It provides no advantage over PHP internals, and plenty of code bloat.

I mean if we execute this code on runtime anyway, is there a reason to not just use ordinary exceptions?

There is not and we're back where we started - which is why I'm opposed to this direction. It sheers off a key benefit - being able to silent unnecessary checks in production which would otherwise incur a performance hit.

In regards to the title change of this issue - we can certainly do that but as the assertions add up the performance under PHP 5 will swiftly become very slow. PHP 7 by contrast won't be affected since it doesn't share this bug with PHP 5, but even it requires a php.ini config directive to stop the compiling of the assert statements.

EDIT:

assert as we began to use it works essential as eval. That's very, very risky as we know -- user input gets in there,

If your asserting anything about user input you're doing it wrong - assertions are not a validation technique chx.

(It also makes for unreadable code but let's not dwell on that)

I've personally had no trouble reading it - then again I use a color context editor.

Also this is micro-optimization vs security: security needs to win. Especially because this micro-optimization backfires often: asserts are by default on so unless some switches them off deliberately

We do switch them off deliberately - read the .htaccess file of the current version of drupal.

PHP will need to eval code in asserts and last I checked eval'd code can't be opcode cached.

Why would you ever need to opcode cache an assertion? Assertions check to see if arguments and returns are in the correct format and they aren't even done on production.

After a closer reading of your complaints chx I feel you really need to go back and study what assertions are, what they are used for, why they are used the way they are used, and only then try to form an opinion on them. At this point you're posting what amounts to a knee jerk reaction to a bad word - "eval"

Trust me, if PHP 5's buggy implementation didn't need assertions to be in eval string format then I wouldn't want to do it that way. When PHP 7 becomes the baseline (probably for Drupal 9) we'll be able to replace all scalar assertions with the equivalent type hints, replace most if not all of the return type assertions with PHP 7 return type hinting, heck, more than half the assertions in the system go away in PHP 7, and the ones that remain won't need to be evaled since PHP 7 can be instructed to not compile the statement as is normal for the assert() statement in all other programming languages.

Latest debate on eval at https://twitter.com/ocramius/status/621621248376852480 -- notable consensus: don't put user data in there!

And again with the red herring. Assertions aren't for validation or working with unsanitized data in any way. You need to get a firm understanding of that principle before commenting further. Once again, in short - assertions check the impossible - conditions that can only occur if the programmer makes a mistake and are therefore impossible in error free code. Anything involving a user is firmly something checked for with normal control structures and exceptions

After discussing this with chx in IRC I've been persuaded to first apologize - got a bit testy in the above exchanges. Submitted is a patch that has the Inspector hotwired to return TRUE whenever an assert.active is false - this allows it to be placed in assert statements without using an eval string with minimal performance loss (and none in PHP 7).

A regex inspection has also been added since calls to preg_match can be very expensive to make and guarding them with a hotwire will also be desirable.

This will cover 95% of assertions without having to use an eval string. The last 5% will be those linking to custom assertion functions - those functions will also need to return true immediately when assert.active is FALSE to perserve performance.

So reviewing...

assert(is_string($arg));

Minimal loss since this is an inbuilt PHP datatype check (maybe 4 processor cycles at most?)

assert(Inspector::assertAllString($arg))

Minimal loss thanks to the hotwire.

assert( $arg instanceof \Interface )

Minimal loss.

Previous patch had unrelated code leak in - I needed to rebase before building the patch.

Added documentation changes that will be necessary with this. No code changes.

It wouldn't be a bad idea to add detection for this to the Coder module so it can be reported by PHP CodeSniffer. We have similar rules in there that detect other common sources of bugs and security issues.

Here is an example of a similar detection that detects potential security issues in preg_match(), preg_filter() etc:

            // Check if there is the evil e flag.
            if (preg_match('/'.$delimiter.'[\w]{0,}e[\w]{0,}$/', substr($pattern, 0, -1))) {
                $warn = 'Using the e flag in %s is a possible security risk. For details see http://drupal.org/node/750148';
                $phpcsFile->addError(
                    $warn,
                    $argument['start'],
                    'PregEFlag',
                    array($tokens[$stackPtr]['content'])
                );
                return;
            }

I've been trying to figure out some exploit code but I'm not able to make it work:

function badFunction($user_submitted_data) {
  assert("is_string($user_submitted_data)");
}

$user_submitted_data = '$passwords = db_query("SELECT password FROM {table}")->execute()->fetchAll(); $passwords = print_r($passwords, TRUE); file_put_contents("sites/default/files/passwords.txt", $passwords);'
$this->badFunction($user_submitted_data);

Catchable fatal error: assert(): Failure evaluating code: 
is_string($passwords = db_query("SELECT password FROM {table}")->execute()->fetchAll(); $passwords = print_r($passwords, TRUE); file_put_contents("sites/default/files/passwords.txt", $passwords);) in php shell code on line 1

This doesn't throw a recoverable fatal error, and is similar to SQL escaping vulnerabilities:

$user_submitted_data = '"string");$passwords = db_query("SELECT password FROM {table}")->execute()->fetchAll(); $passwords = print_r($passwords, TRUE); file_put_contents("sites/default/files/passwords.txt", $passwords); $test = is_true(TRUE';

Still doesn't work though, neither $passwords nor $test are set.

Discussed this with @chx on IRC.

It came up how unfortunate it is that #2408013: Adding Assertions to Drupal - Test Tools. had 300 comments without the word "security" ever being mentioned. Furthermore, according to @xjm they shouldn't ever have been in scope for 8.0 anyway, but now we have them, and yet we still don't know how to use them properly and have discussions about assertions hold up issues unnecessarily.

They are however a very useful tool, and help unblock certain pain points in issues. They also definitely add robustness and prevent introduction of potential bugs in core/contrib.

But if we're not able to use single quotes, assert() statements become completely useless and we might as well use exceptions.

In trying to exploit assert() with @pfrenssen we found that assert() is only a problem (with double quotes) if assertions are turned on, whereas our htaccess file turns them off by default. So if we check in our manual reviews and eventually in our testbot that double quotes are never used, the concern ought to go away.

In the mean time, we should add an hook_requirements() that checks if they're turned on and warns if they are. Which should allow us to use single quote asserts again.

Lastly, the introduction of support for assertions was a bit rushed and we still don't have a clear write-up on how to use them properly, i.e. exceptions are for things that can happen, and assertions are for things that can't, which would allow the code to still continue sanely. So we should add a write-up about this as well.

Adding a warning in hook_requirements() to turn off asserts in production is a very good idea. +1

I am getting incredibly frustrated here. I've explained this concept for 10 months now. I must be the world's worst communicator because after explaining what I find to be a very simple concept there is still misunderstanding.

stefan.r

But if we're not able to use single quotes, assert() statements become completely useless and we might as well use exceptions.

There's a small matter of context. Exceptions have no control structures attached to them - they're a type of event at best. A conditional must lead to an exception, and once coded in there is no possible way to quickly discern between an exception that could happen and an exception that can't happen. That context is the most important part of assert(). I suppose you could write comment text before the conditional, but in a codebase with piles and piles of comments meant for PHPdocs and not human beings (Not to mention annotations which is a form of code - in comments no less) it becomes very easy to form the habit of skimming over or ignoring comments.

Also, even if single quotes are not used, assert() can be disabled entirely in PHP 7. The fact that they are not is a bug of PHP 5 and needs to be treated accordingly. Do not confuse the concept of using assert() with the workarounds forced upon the codebase to mitigate a bug.

Also, you do understand that if you use an expression or function to construct the message argument of assert that's going to be executed at all times as well right?

assert(foo(), bar());

Both foo() and bar() will get called at all times.

assert('foo()', bar());

And here bar() gets called at all times because PHP 5 has a broken assert() implementation - it is a function, not a statement as it is supposed to be, meaning any and all arguments passed to it must execute, and then the assert() check itself can be stepped around using assert.active -- the line is however still compiled. This bug is corrected in PHP 7.

But again, I'll repeat myself yet again - you don't just "use exceptions". Look at the code blocks:

function foo( $arg ) {
  assert(is_string($arg), 'Not a string');

  if (!is_string($arg)) {
    throw new AssertionError('Not a string')
  }
}

The two methods have the same result, AssertionError -- an exception -- gets thrown (That's the point of the PHP 5/7 compat shim in the Assert Tools). The only difference is that in PHP 7 we can skip assert. In PHP 5 we have to use an eval string if we want to skip the performance overhead of the assertion. That's only safe if we can insure $arg never contains unsanitized data.

I'm ok with using an eval() string. I know the perils and know they can be avoided by being careful not to pass an eval'ed string to assert. That's what a string surrounded by "" is, an eval'ed string where variables are converted to strings. Granted functions can't be called in this scope directly - but magic __get() calls can be called. Still, I understand the concern and I'm willing to give up a small amount of performance IN PHP FIVE ONLY

Again, PHP 7 doesn't have this problem.

So, to reduce the performance loss, I introduced the patch above which has the Inspector directly check the assert.active flag before allowing any of its methods to be run - if it's set false then the Inspector returns TRUE.

chx

Asserts, on the other hand, I feel, are completely misunderstood and doesn't add nearly as much value as people think.

Except that they part of an entire philosophy of coding - the design by contract approach.

chx

I can't find a single assert in core/vendors/symfony and core/vendors/doctrine -- this of course doesn't mean we should not use asserts at all it just means it is very hard to find good examples of it.

At the risk of being insulting - looking to the world of PHP programs as your only reference point of good programming examples isn't going to give you a good insight on how good programs work, especially since the vast majority of PHP code out there is utter crap. Your examples aren't in the crap pool by a long shot, but even they have their shortcomings, and a lack of expectation introspection in a large API package is a huge one. Packages like them written in Java and C++ have assert() all over the place.

Further, the most critical task of assert(), argument type validation, is covered by PHP Type hinting for arrays and objects at least. From a certain point of view any time those packages use a type hint they are using a form of assert().

chx

Here's the theory: The internal (protected and private) method arguments should be asserted because the only way they can fail is if the internal logic is wrong. Public entrypoints should use exceptions. Exceptions are for things that can possibly happen but we don't want them to, asserts are for things that can't happen.

Drupal almost never uses private, and protected can be bypassed by extending the class, so your theory is blown out of the water right there. You seem to not graps that contrib authors are programmers and should be respected as such.

If you are going to quote me, quote me in full and parse my whole statement: Assertions document the impossible given error free code and configuration. That second part of my statement is extremely important. If something cannot happen, at all, why check it at all? You shouldn't. It would be like doing this.

assert('1 > 0');

Congratulations - there's an assert() that meets your misquote of what I said and will never fail.

You don't seem to understand the difference between developers and users. Users don't write PHP code. If they can cause something to happen then an exception must be used. Developers do write PHP code. Assert() is an aid for them to get it right.

chx

For example, SafeMarkup::format needs a string as an input -- if you pass in an array then you are corrupting data. Can it happen? Easily, just needs someone passing in a render array instead of a string. So, that's an exception.

Ask yourself this question: Can a user, specifically the superuser of a Drupal install which is the account with the most power, pass an argument to SafeMarkup::format in any way? If yes, you are right - it is possible and an exception must be used. If no, you are flat wrong and it is impossible given error free code and configuration provided by the developers.

I'm not familiar enough with the code to say, but I'd be surprised if the answer was yes.

My apologies if anything above is overly terse - it's just I find myself making the same arguments over and over again and I'm getting tired of it.

I'm less interested in such arguments - I don't care much if a sanity check is implemented as a trigger_error, as an assertion or as an exception, or which is the most correct, as long it fails in some way on a development environment and makes patches go red.

Whenever a sanity check is expensive an any kind of way and there's no reason for us to stop execution, assert() with quotes is a fantastic tool as we can then turn it off on production environments and not incur the performance loss. So as it would be great to be able to use asserts with quotes again (which also display the actual code on failure rather than just FALSE), let's add that hook_requirements() check?

As to PHP 5, sadly we'll have to cater to that for a while still.

I'll agree that if we are careful using eval strings is doable. chx is of a different opinion. The patch above is a reasonable compromise on the issue - before doing any particularly lengthy assertion directly check the assert.active flag and return TRUE if it is set false to abort running the assertion. That gets us mostly there in PHP 5.

I'm still relatively new here so I'll leave the final decision to others to decide which approach is most secure.

@Aki Tendo yes I think we can just use single quote asserts and be careful.

The double quotes are only a problem if assert.active is turned on and per #45 the hook_requirements() solution had been discussed with @chx, he won't block it. It's not a perfect solution so it'd still need sign off from others but I think we can get started on a patch that implements the hook_requireemnts() check

@stefan.r the hook_requirements patch was already started at #2570951: Add Assert Active to Status Report

Hi there -

George DeMet from the Community Working Group here. We’ve been asked to review this thread, and after doing so, I’d like to remind all of the participants in this issue to be mindful of the following from our community Code of Conduct:

The Drupal community and its members treat one another with respect. Everyone can make a valuable contribution to Drupal. We may not always agree, but disagreement is no excuse for poor behavior and poor manners. We might all experience some frustration now and then, but we cannot allow that frustration to turn into a personal attack. It's important to remember that a community where people feel uncomfortable or threatened is not a productive one. We expect members of the Drupal community to be respectful when dealing with other contributors as well as with people outside the Drupal project and with users of Drupal.

If anyone does feel that there are issues that need to be addressed by the CWG, they are welcome to bring them to our attention by posting an issue in our queue (https://www.drupal.org/project/issues/drupal_cwg) or filing a private incident report (https://www.drupal.org/governance/community-working-group/incident-report).

Thanks!

chx

You are asking whether someone using the browser can pass in some argument to SafeMarkup::format and my answer is -- no but they might provide some input which triggers a bug in a condition which makes the code take a wrong, untested code path

And an EMP might switch a value of the data in the processor's L4 cache at the precise moment required to do the same. At this point I feel you're grasping for straws in an attempt to be as disruptive as possible. I've addressed the concerns you've put forward that are realistic. The example quoted above is entirely unrealistic in the extreme.

I'm done with this issue thread. I didn't volunteer to help out just to be badgered at every turn.

Going to mark this as a duplicate of #2853503: Remove all assert('string') calls from Drupal core because deprecated in PHP 7.2 since PHP 7.2 makes a lot of the decisions here for us.