PHP echo GET to change a CSS theme file I have the follow code to change between different CSS theme files, I would like some reviews on it because I am sure it can be improved to be safer.

 &lt;head&gt;
    &lt;?php
    $theme = $_GET['theme'];
    $choice = preg_replace('/[^A-Za-z0-9\-]/', '', $theme);
    if (empty($choice)){
echo '&lt;link rel"stylesheet" type="text/css" href="css/default.css" /&gt;';}
    elseif (file_exists("./css/" . $choice . ".css")) 
{echo '&lt;link rel"stylesheet" type="text/css" href="css/'.$theme.'.css" /&gt;';} 
    else {echo '&lt;link rel"stylesheet" type="text/css" href="css/default.css" /&gt;';}
    exit();
    ?&gt;
    &lt;!-- other elements in head --&gt;
    &lt;/head&gt;


The functions are:


If GET string empty set CSS default file
If GET non existing CSS file set CSS default file
If GET existing CSS file them we use the echo GET.css file
Security

It's a little odd that you clean the file for the file_exists check, but not for the echoing. 

So I can inject test"&gt;&lt;script&gt;alert(1);&lt;/script&gt;, and if the file testscriptalert1script exists the alert is executed. This isn't an attack (well, at least as long as users can't upload their own custom theme), but still seems odd.

Up to version 5.xx of PHP, it was possible to inject a null byte to bypass file_exists, but that should be caught by your filter, so it shouldn't enable the attack described above, or any other attacks (such as injecting validFile\0" href="evil" foo=" and hoping there are browsers which choose the second href attribute if two are present).

So I didn't find any vulnerabilities, but I still don't like your approach all that much. At the very least I would echo the replaced file name instead of the original. I would actually prefer the approach suggested by @76484 (hardcode and use a switch or use an array and then check if the filename exists in it), because using whitelists is always safer than using blacklists.

tl;dr: I couldn't find a vulnerability, but would still use a different approach.

Misc


your indentations, newlines and spacing seem rather random, which makes your code a bit hard to read.
if you exit in the head, your website will be quite boring.
it should be rel="stylesheet".
choice isn't a good variable name. Something like cssFileName would be much clearer.
you repeat the inclusion of the default CSS file. Either extract that to a function, or just remove the empty check (assuming that .css doesn't exist).
but then you still have the inclusion code for CSS in general three times.


So your code would be cleaner like this (but still using a blacklist instead of a whitelist for security):

&lt;?php
$cssFileName = $_GET['theme'];
$cssFileName = preg_replace('/[^A-Za-z0-9\-]/', '', $cssFileName);

if (empty($cssFileName) || !file_exists("./css/" . $cssFileName . ".css")) {
    $cssFileName = "default";
}

echo '&lt;link rel"stylesheet" type="text/css" href="css/' . $cssFileName . '.css" /&gt;';