Comments (18)
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.
from jedchecker.
@rogercreagh you are of course also welcome to make a PR to correct this issue 👍 as long as your open for guidance from other collaborators.
from jedchecker.
@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)
from jedchecker.
This is a debatable question, and it seems to me that there are only two possible options:
- 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.").
- 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!.
from jedchecker.
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?
from jedchecker.
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.
from jedchecker.
@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.
from jedchecker.
@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?
from jedchecker.
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";
While the regex is looking for space preg_match('#^<\?php\s+$#', $content)
and here, but there is no space.
from jedchecker.
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?
from jedchecker.
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 😉
from jedchecker.
@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.
from jedchecker.
"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?
from jedchecker.
@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.
from jedchecker.
@rogercreagh To me the code
<?php ....
is an alternative to<?php echo ' '; ....
, and, in this case, it's clear that_JEXEC
check should precedeecho
(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.
from jedchecker.
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.
from jedchecker.
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 -->
from jedchecker.
Thanks guys/gals - that looks like it should fix the issue.
from jedchecker.
Related Issues (20)
- [Suggestion] - Extend the readme on Crowdin Project
- False positive PH2 error HOT 4
- Dependency Dashboard
- Error: Whitespace in the key is not allowed HOT 3
- JED Checker 2.4.1 extension downloaded from the JED differs from development repository
- Language file is not loaded, when lang prefix is missing. HOT 1
- JEXEC security check HOT 6
- [PHP 8.1] Deprecated: trim(): Passing null to parameter #1 ($string) in rules/xmlinfo.php on line 322 HOT 2
- Warning: syntax error, unexpected '{' or '!' HOT 1
- NOTICE: Node <folder> has unknown attribute 'plugin' is wrong? HOT 2
- Not recognized in XML: <name>language KEY</name> HOT 3
- JEDchecked in Joomla 3.10.12 failed: TypeError: Failed to fetch HOT 17
- [J5] JED Checker extension only works with b/c plugin enabled HOT 2
- JEF Checker 2.4.2 has no checksum HOT 2
- 0 strpos(): Argument #3 ($offset) must be contained in argument #1 ($haystack) HOT 1
- JED Checker to report linebreaks in language files HOT 4
- JED Checker never finishes in PHP 8.3.0 HOT 14
- Wrong deprecation: Joomla\CMS\Filesystem\File and Joomla\CMS\Filesystem\Folder ? HOT 4
- There is interference in the mobile version, Joomla 3
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from jedchecker.