On Wed, Jun 29, 2011 at 10:13:09PM +0200, Manuel Tortosa wrote:
> >From 013006224eaf48f7aec1b67ab20e9df82b920a16 Mon Sep 17 00:00:00 2001
> From: Manuel <[email protected]>
> 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 <[email protected]>
> ---
> 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
