Skip to content
This repository has been archived by the owner on Sep 16, 2019. It is now read-only.

Fatal error: #308

Closed
ginblue opened this issue Apr 20, 2015 · 6 comments · Fixed by #309
Closed

Fatal error: #308

ginblue opened this issue Apr 20, 2015 · 6 comments · Fixed by #309

Comments

@ginblue
Copy link

ginblue commented Apr 20, 2015

Have just started developing a theme with Foundationpress. Nothing has been done other than set up a static 'front-page.php' and change text domain. The following error is now being thrown on front page and post pages. Seems to have been caused by the front page somehow.

Fatal error: Can't use function return value in write context in C:\Program Files\Ampps\www\retro-resources-v4\wp-content\themes\retroresouces\comments.php on line 39

If anyone can shed some light on this issue it would be much appreciated.

Anthony

@olefredrik
Copy link
Owner

@josh-rathke : any ideas?

Referring to issue #297 is seems like this bug was introduced in this commit 7acc16c

@ginblue
Copy link
Author

ginblue commented Apr 20, 2015

Have adopted @LukePettway solution, which seems to work for me.

@Aetles
Copy link
Contributor

Aetles commented Apr 20, 2015

I wonder if sanitize_file_name() is necessary at all in that code?

We are only checking if the file name is exactly comments.php so I'm not sure what kind of added security a sanitizing of $_SERVER['SCRIPT_FILENAME'] would bring in this case, but maybe @josh-rathke can explain it?

@joshrathke
Copy link
Contributor

The reason I added the sanitization functions is because without them, the build does not pass WordPress Coding Standards. This is the error that it throws up:

FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 39 | ERROR | Detected usage of a non-sanitized input variable:
    |       | $_SERVER (WordPress.VIP.ValidatedSanitizedInput.)
----------------------------------------------------------------------

Further research on that particular error code will eventually bring you to this site which explains the how and why, but it would seem that it's geared far more towards user input.

All that to say, we could just omit that check from the test and leave it un-sanitized.

@Aetles
Copy link
Contributor

Aetles commented Apr 20, 2015

Ah, yeah it's definitely useful for files uploaded from users, "Never trust user input" is a good mantra. :) In this case it seemed unnecessary, but if it does not pass the check then maybe the solution by LukePettway is the way to go?

Or maybe there's a better way than checking $_SERVER['SCRIPT_FILENAME']? The WP Codex for writing a plugin suggests another way to prevent direct access:

Security Note: Consider blocking direct access to your plugin PHP files by adding the following line at the top of each of them, or be sure to refrain from executing sensitive standalone PHP code before calling any WordPress functions.

defined( 'ABSPATH' ) or die( 'No script kiddies please!' );

I don't know if there's any downside to it, though, but it seems to be suggested for templates as well.

And btw, line 40:

        die (__( 'Please do not load this page directly. Thanks!', 'FoundationPress' )); }

should be

            die ( __( 'Please do not load this page directly. Thanks!', 'FoundationPress' ) ); 
        }

;)

@joshrathke
Copy link
Contributor

I think using the WP Codex's method is cleaner. It makes more sense on an
initial read through as well.

On Mon, Apr 20, 2015 at 7:40 AM, Adrian Bengtson notifications@github.com
wrote:

Ah, yeah it's definitely useful for files uploaded from users, "Never
trust user input" is a good mantra. :) In this case it seemed unnecessary,
but if it does not pass the check then maybe the solution
#297 (comment)
by LukePettway is the way to go?

Or maybe there's a better way than checking $_SERVER['SCRIPT_FILENAME']?
The WP Codex for writing a plugin suggests another way
https://codex.wordpress.org/Writing_a_Plugin#Plugin_Files to prevent
direct access:

Security Note: Consider blocking direct access to your plugin PHP files
by adding the following line at the top of each of them, or be sure to
refrain from executing sensitive standalone PHP code before calling any
WordPress functions.

defined( 'ABSPATH' ) or die( 'No script kiddies please!' );

I don't know if there's any downside to it, though, but it seems to be
suggested
http://stackoverflow.com/questions/27250343/security-wordpress-template
for templates as well.

And btw, line 40:

    die (__( 'Please do not load this page directly. Thanks!', 'FoundationPress' )); }

should be

        die ( __( 'Please do not load this page directly. Thanks!', 'FoundationPress' ) );         }

;)


Reply to this email directly or view it on GitHub
#308 (comment)
.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants