On Wed, Jun 29, 2011 at 10:13:09PM +0200, Manuel Tortosa wrote:
From 013006224eaf48f7aec1b67ab20e9df82b920a16 Mon Sep 17 00:00:00 2001 From: Manuel <manutortosa@chakra-project.org> Date: Wed, 29 Jun 2011 22:11:37 +0200 Subject: [PATCH 2/2] Convert the language selection menu in a select form
Signed-off-by: Manuel <manutortosa@chakra-project.org> --- web/template/header.php | 25 ++++++++++++++++++------- 1 files changed, 18 insertions(+), 7 deletions(-)
I'm not 100% sure if I want to push that one. Opinions? Either way, this should be cleaned up. It currently introduces at least two security vulnerabilities. See comments below.
diff --git a/web/template/header.php b/web/template/header.php index 8313bb3..47ec909 100644 --- a/web/template/header.php +++ b/web/template/header.php @@ -48,13 +48,24 @@
<div id="lang_sub"> <?php -reset($SUPPORTED_LANGS); -foreach ($SUPPORTED_LANGS as $lang => $lang_name) { - print '<a href="' - . htmlspecialchars($_SERVER["PHP_SELF"], ENT_QUOTES) - ."?setlang=$lang\" title=\"$lang_name\">" - . strtolower($lang) . "</a>\n"; -} + reset($SUPPORTED_LANGS); + $select_lang = "<form method='get' action='".$_SERVER["PHP_SELF"]."'>\n";
Using '$_SERVER["PHP_SELF"]' without escaping quotes introduces a potential XSS vulnerability [1].
+ if (isset($_REQUEST['ID'])) {
Please use "$_GET" or "$_POST" instead of "$_REQUEST".
+ $select_lang.= "<input type='hidden' name='ID' value='".$_REQUEST['ID']."'>\n";
Again, printing "$_REQUEST['ID']" without escaping introduces a XSS vulnerability. Do we actually need this field at all?
+ } + $select_lang.= "<select name='setlang'>\n"; + foreach ($SUPPORTED_LANGS as $lang => $lang_name) { + if ($lang == $LANG) { + $select_lang.= "<option selected='selected' value='$lang'>".$SUPPORTED_LANGS[$lang]."</option>";
You should use htmlspecialchars() for "$SUPPORTED_LANGS[$lang]" as well. Skipping that won't introduce a vulnerability here but it's better to have them.
+ } + else { + $select_lang.= "<option value='$lang'>".$SUPPORTED_LANGS[$lang]."</option>";
Same here.
+ } + } + $select_lang.= "</select> <input class='button' type='submit' value='".__("Go")."' />"; + $select_lang.= "</form>"; + + echo $select_lang;
Building HTML in a variable and finally echo'ing it is bad practice. Please use plain HTML with embedded PHP tags if necessary.
?> </div> <!-- Start of main content --> -- 1.7.5.3
[1] http://www.mc2design.com/blog/php_self-safe-alternatives