"The only solution I see to this is pre-commit syntax checking for committed PHP files."
How would that have helped here? The file was obviously still valid syntactically. Try it for yourself: create a PHP file that looks like
i?php
$config_data = array();
and run it through PHP's syntax check (php -l /path/to/file from the command line). It will verify that the script is valid. However, the content will still not be parsed as PHP.
One way to fix this is to store your configuration data in another file format (eg: YAML) outside of the document root. Since the configuration file doesn't try to be parsed as PHP, it can't be leaked in this way.
Yeah, my solution is to have a separate config file too but have a config file solely for password and never check that file into the repository.
Of course whenever someone does a fresh pull of the project, that is one file that they'd have to create for themselves. (So this is documented very clearly.)
It's minor inconvenience but it solves the problems like this.
It physically can't: without the opening PHP tag the parser just thinks the entire file is HTML to be outputted to the browser, which is still a valid PHP file. ;)
I suppose you could also write a quick bash script to ensure the first 5 characters of any .class.php file matches "<?php", but it wouldn't be very generally applicable and wouldn't fix a problem like this for a file named config.php
Well, arguably, the syntax checker could detect "valid but dangerous" PHP statements. So, for instance, if there is a terminating </php> but no <php>, that could be triggered as suspicious or dangerous. (I'm just saying this as a general example, but I know it might not be the best solution for different reasons).
Also, unit tests should definitely find that error.. no? I mean, a simple high level test that use something in config.php should fail.
I wanted to joke about something like that ;) (Even thought I much more prefer vim, it happens to me from time to time to see a i/j/whatever in my file and getting random errors)
I'm far from being a PHP expert, but wouldn't writing critical password/data in a file instead of directly in the source code, avoided that issue? I mean, we would have seen the code to read the file.. and not the pass?
It's a symptom of PHP's slightly horrible execution model. When you request a page, a new PHP process spins up, parses it, compiles it, and executes it. There is no shared state between requests within PHP (generally - there are ways to add it) and so if configuration was stored in YAML or XML, it would have to be parsed /on every request/.
Many people have made suggestions on how to trap these kinds of error... storing PHP files outside of document root is a good practice, otherwise the code can be revealed if the webserver stops interpreting PHP files for whatever reason.
Any amount of testing would catch accidentally dumping the configuration file, if it isn't being parsed then nothing on the site will work. Development -> testing -> production setups aren't as common in the PHP world as they should be, though.
Personally, I'm tempted to say store configuration in a .ini or YAML file or similar. If you're building a small site, it won't be a big speed issue anyway. If it becomes a speed issue, there are many ways to mitigate it, such as compiling the human-written config file to a PHP file, removing the human element that is more likely to transpose a character.
Does the person not use syntax highlighting? A missing <?php tag would cause the PHP code to be highlighted as HTML code which would look very strange to anyone who has edited a PHP file with a syntax-highlighting-enabled editor.
19 comments
[ 3.9 ms ] story [ 54.2 ms ] threadHow would that have helped here? The file was obviously still valid syntactically. Try it for yourself: create a PHP file that looks like
and run it through PHP's syntax check (php -l /path/to/file from the command line). It will verify that the script is valid. However, the content will still not be parsed as PHP.One way to fix this is to store your configuration data in another file format (eg: YAML) outside of the document root. Since the configuration file doesn't try to be parsed as PHP, it can't be leaked in this way.
Of course whenever someone does a fresh pull of the project, that is one file that they'd have to create for themselves. (So this is documented very clearly.)
It's minor inconvenience but it solves the problems like this.
Edit: Unless of course you're using YAML or some other representation rather than a pure PHP file.
SetEnv DB_USER=kermit SetEnv DB_PASS=Shhh
And then connect to the DB using code such as:
mysql_connect(getenv('DB_NAME'), getenv('DB_USER'), getenv('DB_PASS'))
Environmental config data should never be stored in source code.
Edit: Here's a way to force 500 for fatal errors in PHP http://blog.kevburnsjr.com/php-fatal-error-500
Also, unit tests should definitely find that error.. no? I mean, a simple high level test that use something in config.php should fail.
Many people have made suggestions on how to trap these kinds of error... storing PHP files outside of document root is a good practice, otherwise the code can be revealed if the webserver stops interpreting PHP files for whatever reason.
Any amount of testing would catch accidentally dumping the configuration file, if it isn't being parsed then nothing on the site will work. Development -> testing -> production setups aren't as common in the PHP world as they should be, though.
Personally, I'm tempted to say store configuration in a .ini or YAML file or similar. If you're building a small site, it won't be a big speed issue anyway. If it becomes a speed issue, there are many ways to mitigate it, such as compiling the human-written config file to a PHP file, removing the human element that is more likely to transpose a character.