rryqszq4 / ngx-php

ngx-php - Embedded php7 or php8 scripting language for nginx module. Mainline development version of the ngx-php.
BSD 2-Clause "Simplified" License
584 stars 56 forks source link

Delete closing tag ?> #60

Closed joanhey closed 4 years ago

joanhey commented 4 years ago

to avoid any extra output

mathieu-aubin commented 4 years ago

In my opinion, removing closing tags is like opening a door to hackers.

By having a closing tag, any extra php code will either produce an error or most likely not be processed at all which, again to me personally, is better than having someone inject a line at the end of a file in hopes that there is no closing tag and that it will get processed.

I know nginx config files differ from a php file in many ways but i still think code structure should be followed as it makes everything clear and adds to justification of the code.

I also think such a pr does not add value to the project and risks confusing more than helping. (unless i clearly dont get the idea behind this and if so, please explain your motivation behind this change)

just my idea, thanks

I dont get the description of this but if its to avoid blanks or such, then maybe the closong tag should be right after the command itself, on the same line as in <?php echo 'Hello Pistachios'; ?>

joanhey commented 4 years ago

From a security point, it's exactly the same with or without the closing tags. After php include the file, automatically close it. And I think it is more secure without it.

And it is not for blank lines this PR. It is to avoid extra output if added anything after the closing tag. For example in plain php you can send the headers, after any output before.No probem with this in ngx_php. That closing tag gave many errors in frameworks before. Almost all frameworks recomment not to use the closing tag.

mathieu-aubin commented 4 years ago

I can only say that from experience (i run several hundred websites, one of which gets over 52 million hits per day) that id rather have closing tags on php files, it helped sevral times. Thanks for the input

joanhey commented 4 years ago

Hi @mathieu-aubin

https://stackoverflow.com/questions/3219383/why-do-some-scripts-omit-the-closing-php-tag https://stackoverflow.com/questions/4410704/why-would-one-omit-the-close-tag There are a lot more discussions about that.

Also the php basic sintax say:

If a file contains only PHP code, it is preferable to omit the PHP closing tag at the end of the file.

https://www.php.net/manual/en/language.basic-syntax.phptags.php

Please, if you can send an example, causing a vulnerability for not use the closing tag, will be very helpfull for all the php comunity.

Thank you.

mathieu-aubin commented 4 years ago

Im sorry i dont have an example as i tend to get rid of malicious stuff i find after i analyze it. But mostly, it was malicious/hacked plugins (i have very little control over what the customers upload..) that would insert bad lines of php (without opening/closing) tags to files. When that happens, if the closing tag is missing then the code that was 'injected' or added to the file was being processed.

Again, its only my opinion and i didnt expect any change of mind. Thanks