3 ms·
"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 sy
by nbpoole 16y ago
"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.
- highwind81 16y agoYeah, 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.
- nbpoole 16y agoUntil someone types i?php when when creating the file ;) Edit: Unless of course you're using YAML or some other representation rather than a pure PHP file.
- jcstauffer 16y agoOur solution is to store environment-specific config data (such as DB name, username, password, etc.) in the Apache config: 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.
- KevBurnsJr 16y agoWow, you're right. I sort of assumed it would pick up on an error like this.
- nbpoole 16y agoIt 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. ;)
- KevBurnsJr 16y agoI 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
- KevBurnsJr 16y agoI suppose you could add some code to Varnish to tell it to squelch any and all 500 responses.
- KevBurnsJr 16y agoLooks like PHP returns a 200 when it encounters a fatal error. Edit: Here's a way to force 500 for fatal errors in PHP http://blog.kevburnsjr.com/php-fatal-error-500 http://blog.kevburnsjr.com/php-fatal-error-500
- d0m 16y agoWell, 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.