[aur-dev] [PATCH] Convert the language selection bar in a select form

[Lukas Fleischer]

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 at 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 at 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