PHP echo GET to change a CSS theme file

// @author Darik

$theme = isset($_GET['theme']) ? preg_replace('/[^A-Za-z0-9\-]/', '', $_GET['theme']) : 'default';  
$choice = ['theme1' => 'lightbluecss', 'theme2' => 'darkbluecss', 'default' => 'default']; // list availeble [theme name => file name] here (make sure they exist)  


if (isset($choice[$theme])){        
    echo '<link rel="stylesheet" type="text/css" href="css/'.$choice[$theme].'.css" />';             
}
else{
    echo '<link rel="stylesheet" type="text/css" href="css/default.css" />';    
}

?>
<!-- other elements in head -->

I think this is more robust for what you are trying to do, because this will eliminate the need of file exist check, security issues, and also limits the request to the available themes listed in the $choice array; if not it will fall back to default.

As you can see the theme name doesn't has to be quite literally the file name so you can make the theme names prettier and it improves security.

I just hope this code is helpful and readable to you.

My personal advice is unless these theme files have a massive difference between them you should do it at the CSS selectors level rather than switching the whole CSS file. It will save you a lot.

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"><script>alert(1);</script>, 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):

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

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

echo '<link rel"stylesheet" type="text/css" href="css/' . $cssFileName . '.css" />';