New API to concatenate an array of HTML attributes

[Deltik]

Motivation and Context

Throughout e107, HTML attributes are concatenated in a cumbersome and error-prone way: simple string concatenation.

Example from download_shortcodes::sc_download_list_link():

   function sc_download_list_link($parm='')
   {
		$tp = e107::getParser();
		$img = '';

      	$agreetext = !empty($this->pref['agree_text']) ? $tp->toAttribute($tp->toJSON($tp->toText($this->pref['agree_text'],FALSE,'DESCRIPTION'), 'str')) : '';

		if(defined('IMAGE_DOWNLOAD'))
		{
			$img = "<img src='".IMAGE_DOWNLOAD."' alt='".LAN_DOWNLOAD."' title='".LAN_DOWNLOAD."' />";
		}

		if(deftrue('BOOTSTRAP'))
		{
			$img = e107::getParser()->toGlyph('fa-download',false);
		//	$img = '<i class="icon-download"></i>'; 
		}
	  	
     	if ($this->var['download_mirror_type'])
     	{
     		return "<a class='e-tip' title='".LAN_DOWNLOAD."' href='".e_PLUGIN_ABS."download/download.php?mirror.".$this->var['download_id']."'>{$img}</a>";
     	}
     	else
     	{
     		$url = $tp->parseTemplate("{DOWNLOAD_REQUEST_URL}",true, $this); // $this->sc_download_request_url();
     	  	return (!empty($this->pref['agree_flag']) ? "<a class='e-tip' title='".LAN_DOWNLOAD."' href='".$url."' onclick= \"return confirm({$agreetext});\">{$img}</a>" : "<a class='e-tip' title='".LAN_DOWNLOAD."' href='".$url."' >{$img}</a>");
   
		//	return ($this->pref['agree_flag'] ? "<a class='e-tip' title='".LAN_DOWNLOAD."' href='".e_PLUGIN_ABS."download/request.php?".$this->var['download_id']."' onclick= \"return confirm('{$agreetext}');\">{$img}</a>" : "<a class='e-tip' title='".LAN_DOWNLOAD."' href='".e_PLUGIN_ABS."download/request.php?".$this->var['download_id']."' >{$img}</a>");
     	}
   }

Notice the cumbersome string concatenation:

"<a class='e-tip' title='".LAN_DOWNLOAD."' href='".e_PLUGIN_ABS."download/request.php?".$this->var['download_id']."' onclick= \"return confirm('{$agreetext}');\">{$img}</a>"

The client code must consider whether the value of each attribute can be vulnerable to cross-site scripting (XSS) attacks. In the example above, $this->var['download_id'] and $agreetext must be trusted not to contain a string like '></a><script type="text/javascript">alert("This would have been malicious.");</script><a href='.

This insecure-by-default behavior should be discouraged, and an easy-to-use option to eliminate XSS attacks should be introduced.

Description

Enter e_parse::toAttributes(). Let's rewrite this:

"<a class='e-tip' title='".LAN_DOWNLOAD."' href='".e_PLUGIN_ABS."download/request.php?".$this->var['download_id']."' onclick= \"return confirm('{$agreetext}');\">{$img}</a>"

As this:

"<a" . $tp->toAttributes([
	"class"   => "e-tip",
	"title"   => LAN_DOWNLOAD,
	"href"    => e_PLUGIN_ABS . "download/download.php?mirror." . $this->var['download_id'],
	"onclick" => "return confirm('$agreetext')",
]) . ">$img</a>";

This is a readable and secure alternative to the original because e_parse::toAttributes() understands each component of the HTML attributes. It knows that the keys of the array are the HTML attribute name and that the values are all to be passed through htmlspecialchars(), so there is no way to perform XSS in the HTML attributes. ($img here still needs to be trusted not to have malicious input.)

To showcase this new feature, all usages of e107::getPref()['agree_text'] in download_shortcodes have been replaced with the e_parse::toAttributes() equivalent. This fixes #4686 as a side effect.

Additionally, this feature was previewed previously in e_form::attributes(), a private method. The body of this method has been replaced with a proxy to e_parse::toAttributes(). For legacy compatibility, a second argument, $pure, was added to skip the call to e_parse::replaceConstants() done inside e_parse::toAttribute(), because the usages in e_form did not expect constants to be replaced.

How Has This Been Tested?

e_parseTest now has new methods demoing the new e_parse::toAttributes() feature.

Types of Changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation (a change to man pages or other documentation)

Checklist

• My code adheres to the e107 code style standard.
• I have read the contributing document.
• I have added tests to cover my changes.
• All new and existing tests pass.

[codeclimate]

Code Climate has analyzed commit cf86267 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	1

The test coverage on the diff in this pull request is 78.0% (80% is the threshold).

This pull request will bring the total coverage in the repository to 34.4% (0.0% change).

View more on Code Climate.

[CaMer0n]

Thank you! 👍