False positive The JEXEC security check was not found in this file.

[rogercreagh]

The following code will generate a JEXEC security error

  <?php
/*******
 * your standard header info
 ******/
defined('_JEXEC') or die;

class MycompControllerView extends JControllerAdmin {
    //standard expected code    
}

because there is whitespace on the first line before the <?php

Either this needs to be a separate warning <?php not at start of first line or the test needs modifying so that it is only checking for what it says it is checking for - missing JEXEC security check.

Currently it can cause hours of frustration looking for an error in the JEXEC line and missing the single space at the start of the file.

[Llewellynvdm]

Noted, we can look at that. Yes your right it's frustrating when the issue is that obvious... happens to all of us at times.

@dryabov we can add a separate warning when a file has a space before the <?php opening tag, and avoid this false positive. Seems like the issue comes from this line in the regex. I don't see how we can remove or change that, but adding the extra warning that denotes that as the cause would be helpful.

[Llewellynvdm]

@rogercreagh you are of course also welcome to make a PR to correct this issue :+1: as long as your open for guidance from other collaborators.

[rogercreagh]

@rogercreagh you are of course also welcome to make a PR to correct this issue +1 as long as your open for guidance from other collaborators.

Yes, I have had a quick look starting from the regex @dryabov pointed to, but concluded it would take a long time for me to unpick the code and work out how to fix it without breaking something else. It seems as if that regex is part of a general function that is used for many tests, but I couldn't really follow it. I don't really have time to spend on it at present.

If it helps I can report that v2.0 probably did not have this problem as the component I submitted and got rejected had been tested with 2.0 and passed. (why I was still using 2.0 is another story - essentially the upgrade site given with 2.0 wasn't working so I was happily assuming my install was up to date when it wasn't)

[dryabov]

This is a debatable question, and it seems to me that there are only two possible options:

1. The _JEXEC guard should precede any script's output, even if it's just a leading space. It's exactly how JEDChecker works right now. Maybe, in this case it's sufficient to just modify the displayed title of this rule ("PHP Files missing JEXEC security" currently), but the full description seems to be quite correct ("All the PHP files in your extension needs to have a defined('_JEXEC') or die(); statement in the beginning of each file. This ensures that the file cannot be opened outside of the joomla installation and increases the security of your site.").
2. The _JEXEC guard should precede PHP code only, so allowing any leading text. But such a behavior is a non-standard one for most extension developers.

PS. Looks like there are a few typos in the above-mentioned rule's description: needs->need, in the->at the, joomla->Joomla!.

[Llewellynvdm]

Okay typos can be fixed, I agree with point number one (1) as to how we should continue to deal with this.

Would you say that having this space in-front is a small enough typo mistake to ignore? Since I agree we can't really go after every possible developer typo error. What is true is the false positive, meaning sure lets ignore the space... but lets not blame something like JEXEC when it isn't the real issue. This we should avoid, as this is what the real issue is about. the JED Checker gave a missing JEXEC error when it was in-fact in the file. We should try and avoid that.

So as far as regex goes we are asking the script to start from the first <?php tag at the top of the file with #^ hmmm I am sure we can add the option to optionally allow for blank space #^(\s)?... I know that ignores the obvious typo, but the test is not targeted at detecting blank space at the start of the file.

We could add a test separately for that as I said... @dryabov do you think this will introduce a vulnerability?

[rogercreagh]

Hmmm, interesting discussion. There is a further inconsistency here.

"All the PHP files in your extension needs to have a defined('_JEXEC') or die(); statement in the beginning of each file. This ensures that the file cannot be opened outside of the joomla installation and increases the security of your site."

EXCEPT - it is okay to have a block comment after the <?php (every standard Joomla file has this) and also it is allowed to have any number of empty lines between <?php and defined( '_JEXEC' )... (I think some standard Joomla files do this, and certainly JEDchecker allows it currently).

AND ALSO - you can have use statements before the JEXEC (at least that is okay according to JEDchecker although definitely unusual it seems to me.)

But you can't have a newline followed by a space - ie some some whitespace (newlines) is ok but some (spaces and tabs) is not. This seems outside the php spec which allows whitespace.

Of course the rule quoted actually says "in the beginning" not "at the beginning". Perhaps it should actually say "as the first executable line of the file"

Personally I think it is better style to have a blank line between the block comment and the JEXEC line and poor style to put use statements above the JEXEC.

I would be interested to know what exactly the security risk is with having whitespace (newlines, spaces or tabs) before the opening <?php is? I presume it is a genuine risk otherwise it should not be generating and error.

If it is a genuine risk then the error is valid but the message should state "<?php not at start of file, or JEXEC not on first executable line"

If it is not a genuine risk then whitespace before the <?php should be tested separately and downgraded to a warning not an error. (style police warning) - and in my opinion it should also flag use statements appearing before JEXEC as a style police warning.

[Llewellynvdm]

@rogercreagh I think you need to look at the regex some more, since what your saying is not what is happening.

The regex simply targets the first <?php tag at the very top of the file and then searches down the file for the JEXEC statement.

We are just trying to make sure the the JEXEC is set... and this for your own extensions safety. This test is not about where we should have use statements or white space or tabs or any of that, this specific issue is about JEXEC at the top of your PHP file.

Now you reported that you had a space in front of the opening tag, which you agree is not suppose to be there, or so it seemed from your first post. This caused a false positive on the JEXEC error, and set you on a search of frustration.

We are not going to change the JEXEC validation it will stay, our only willingness is to avoid false positives, and to this end the current conversation is still ongoing.

Two objectives:

• make sure the JEXEC is in all PHP files where required (there are exceptions)
• avoid saying that its not there, when it is there (false positives)

Should you have a solution where both these objectives can be met with this new edge case in mind (space before opening tag), then please share the code.

[Llewellynvdm]

@dryabov what seems weird to me, is the fact that we have php_strip_whitespace and still this error triggered. Have you been able to replicate this issue?

[Llewellynvdm]

Just ran a few tests... and it seems like a bug:

$file="  <?php
/*******
 * your standard header info
 ******/
defined('_JEXEC') or die;

class MycompControllerView extends JControllerAdmin {
    //standard expected code    
}";

$content = preg_replace('/\s+/', '', $file); // does the same as php_strip_whitespace will for a file

var_dump(preg_match('#^<\?php\s+$#', $content));

echo $content;
echo "\n";

Returns:

While the regex is looking for space preg_match('#^<\?php\s+$#', $content) and here, but there is no space.

[Llewellynvdm]

I know the first check is for empty files... but the second <\?php\s+ does not seem optional... so I thought to change it to <\?php(\s+)? so I tested it again like this:

<?php

$file = "  <?php
/**
 * your standard header info
*/

defined('_JEXEC') or die('Restricted access');

class MycompControllerView extends JControllerAdmin {
    //standard expected code    
}";

$content = preg_replace('/\s+/', '', $file); // does the same as php_strip_whitespace will for a file

$defines="_JEXEC, JPATH_PLATFORM, JPATH_BASE, AKEEBAENGINE, WF_EDITOR";
$defines = explode(',', $defines);
foreach ($defines as $i => $define)
{
    $defines[$i] = preg_quote(trim($define), '#');
}

$regex
    = '#^' // at the beginning of the file
    . '<\?php(\s+)?' // there is an opening php tag
    . '(?:declare ?\(strict_types ?= ?1 ?\) ?; ?)?' // optionally followed by declare(strict_types=1) directive
    . '(?:namespace [0-9A-Za-z_\\\\]+ ?; ?)?' // optionally followed by namespace directive
    . '(?:use [0-9A-Za-z_\\\\]+ ?(?:as [0-9A-Za-z_]+ ?)?; ?)*' // optionally followed by use directives
    . '\\\\?defined ?\( ?' // followed by defined test
    . '([\'"])(?:' . implode('|', $defines) . ')\1' // of any of given constant
    . ' ?\) ?(?:or |\|\| ?)(?:die|exit)\b' // or exit
    . '#i';

var_dump(preg_match($regex, $content));

echo $content;
echo "\n";

But the same outcome... yet the white space in front of the opening tags are not there anymore. What am I missing here?

[Llewellynvdm]

Must say I am looking forward to this new regex class that @nibra is proposing to add to the utilities... will make writing regex much easier. Even if its not added soon... I think we should look to advance the JED checker to make use of it... this will help simplify these regex areas :wink:

[dryabov]

@Llewellynvdm php_strip_whitespace strips spaces from PHP code, not from HTML.

And there is another issue with a space (or BOM mark) at the beginning of PHP file: it may result in the "headers already sent" error if inclusion of the file is not wrapped into ob_start function. So, I'd recommend to avoid it in the code anyway. But if you like, the workaround for this specific case is to replace '#^' by '#^\s*' in the $regex value.

[rogercreagh]

"The regex simply targets the first <?php tag at the very top of the file and then searches down the file for the JEXEC statement."

Except that it throws an error if it finds a line with a space before it finds the JEXEC. Or at least it does on a fresh install of JEDchecker 2.4 on my Joomla 3.10.3 testing site. Even if that line comes after the <?php. It didn't occur to me to try anything other than the use statement before it - are you saying that the JEXEC could be anywhere in the file? That doesn't sound right, surely it needs to be the very first executable line?

[dryabov]

@rogercreagh To me the code <?php .... is an alternative to <?php echo ' '; ...., and, in this case, it's clear that _JEXEC check should precede echo (no matter direct or indirect).

Anyway, what is your reason to start PHP file with a space (or new line character, or any other output)?

PS. In the Framework ruleset there is a check for BOM (byte-order-mark) at the beginning of PHP file (to avoid the "headers already sent" issue), and most likely we can report leading spaces/newlines in a similar way. Then, it'll be safe to use ^\s* in the $regex, because leading space issue is treated separately.

[rogercreagh]

@rogercreagh To me the code <?php .... is an alternative to <?php echo ' '; ...., and, in this case, it's clear that _JEXEC check should precede echo (no matter direct or indirect).

Sorry I wasn't clear, by the ellipsis (...) I simply meant "and the rest of the php code." My understanding is that as per the php manual <?= 'text'; ?> is equivalent to <?php echo 'text'; ?> as a way of saving typing 7 more characters in a one line bit of php to echo some text. <?php ... is not valid php in itself

see https://www.php.net/manual/en/language.basic-syntax.phptags.php php manual for a third way of doing the same thing, but that's a digression.

[rogercreagh]

Anyway, what is your reason to start PHP file with a space (or new line character, or any other output)?

No reason at all within Joomla. My point is simply that it is not illegal or a security risk so shouldn't be reported as an error.

[rogercreagh]

Anyway, what is your reason to start PHP file with a space (or new line character, or any other output)?

No reason at all within Joomla. My point is simply that it is not illegal or a security risk so shouldn't be reported as an error.

(actually as an afterthought I suppose if you were righting code that might be used outside Joomla as well then you might put an html comment at the very top <!-- Warning this version contains the Joomla security check on line 7 and will not run outside joomla -->

[rogercreagh]

Thanks guys/gals - that looks like it should fix the issue.