From b0ba2015d9684ee7b124dafcf6b59b046e20f8ed Mon Sep 17 00:00:00 2001
From: Damian Mooyman <damian@silverstripe.com>
Date: Wed, 3 Aug 2016 11:23:17 +1200
Subject: [PATCH] [ss-2016-015] Fix value / title escaping in CheckboxSetField
 and OptionsetField

---
 forms/CheckboxSetField.php           |  7 +++++--
 forms/OptionsetField.php             |  5 +++++
 templates/forms/CheckboxSetField.ss  |  2 +-
 templates/forms/OptionsetField.ss    |  2 +-
 tests/forms/CheckboxSetFieldTest.php | 21 +++++++++++++++++++++
 tests/forms/OptionsetFieldTest.php   | 14 ++++++++++++++
 6 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/forms/CheckboxSetField.php b/forms/CheckboxSetField.php
index 55eb8cf2b..f1ef4bbaa 100644
--- a/forms/CheckboxSetField.php
+++ b/forms/CheckboxSetField.php
@@ -132,11 +132,14 @@ class CheckboxSetField extends OptionsetField {
 		}
 
 		foreach($source as $value => $item) {
+			// Ensure $title is cast for template
 			if($item instanceof DataObject) {
 				$value = $item->ID;
-				$title = $item->Title;
-			} else {
+				$title = $item->obj('Title');
+			} elseif ($item instanceof DBField) {
 				$title = $item;
+			} else {
+				$title = DBField::create_field('Text', $item);
 			}
 
 			$itemID = $this->ID() . '_' . preg_replace('/[^a-zA-Z0-9]/', '', $value);
diff --git a/forms/OptionsetField.php b/forms/OptionsetField.php
index 98ade1de5..759080522 100644
--- a/forms/OptionsetField.php
+++ b/forms/OptionsetField.php
@@ -62,6 +62,11 @@ class OptionsetField extends DropdownField {
 
 		if($source) {
 			foreach($source as $value => $title) {
+				// Ensure $title is safely cast
+				if ( !($title instanceof DBField) ) {
+					$title = DBField::create_field('Text', $title);
+				}
+
 				$itemID = $this->ID() . '_' . preg_replace('/[^a-zA-Z0-9]/', '', $value);
 				$odd = ($odd + 1) % 2;
 				$extraClass = $odd ? 'odd' : 'even';
diff --git a/templates/forms/CheckboxSetField.ss b/templates/forms/CheckboxSetField.ss
index 68634042a..1cf6c1195 100644
--- a/templates/forms/CheckboxSetField.ss
+++ b/templates/forms/CheckboxSetField.ss
@@ -2,7 +2,7 @@
 	<% if $Options.Count %>
 		<% loop $Options %>
 			<li class="$Class">
-				<input id="$ID" class="checkbox" name="$Name" type="checkbox" value="$Value"<% if $isChecked %> checked="checked"<% end_if %><% if $isDisabled %> disabled="disabled"<% end_if %> />
+				<input id="$ID" class="checkbox" name="$Name" type="checkbox" value="$Value.ATT"<% if $isChecked %> checked="checked"<% end_if %><% if $isDisabled %> disabled="disabled"<% end_if %> />
 				<label for="$ID">$Title</label>
 			</li>
 		<% end_loop %>
diff --git a/templates/forms/OptionsetField.ss b/templates/forms/OptionsetField.ss
index 70012bb6f..2dba8343a 100644
--- a/templates/forms/OptionsetField.ss
+++ b/templates/forms/OptionsetField.ss
@@ -1,7 +1,7 @@
 <ul $AttributesHTML>
 	<% loop $Options %>
 		<li class="$Class">
-			<input id="$ID" class="radio" name="$Name" type="radio" value="$Value"<% if $isChecked %> checked<% end_if %><% if $isDisabled %> disabled<% end_if %> <% if $Up.Required %>required<% end_if %> />
+			<input id="$ID" class="radio" name="$Name" type="radio" value="$Value.ATT"<% if $isChecked %> checked<% end_if %><% if $isDisabled %> disabled<% end_if %> <% if $Up.Required %>required<% end_if %> />
 			<label for="$ID">$Title</label>
 		</li>
 	<% end_loop %>
diff --git a/tests/forms/CheckboxSetFieldTest.php b/tests/forms/CheckboxSetFieldTest.php
index f22f2a866..94cb4e171 100644
--- a/tests/forms/CheckboxSetFieldTest.php
+++ b/tests/forms/CheckboxSetFieldTest.php
@@ -206,6 +206,27 @@ class CheckboxSetFieldTest extends SapphireTest {
 		);
 	}
 
+	public function testSafelyCast() {
+		$member = new Member();
+		$member->FirstName = '<firstname>';
+		$member->Surname = '<surname>';
+		$member->write();
+		$field1 = new CheckboxSetField('Options', 'Options', array(
+			'one' => 'One',
+			'two' => 'Two & Three',
+			'three' => DBField::create_field('HTMLText', 'Four &amp; Five &amp; Six'),
+			$member
+		));
+		$fieldHTML = (string)$field1->Field();
+		$this->assertContains('One', $fieldHTML);
+		$this->assertContains('Two &amp; Three', $fieldHTML);
+		$this->assertNotContains('Two & Three', $fieldHTML);
+		$this->assertContains('Four &amp; Five &amp; Six', $fieldHTML);
+		$this->assertNotContains('Four & Five & Six', $fieldHTML);
+		$this->assertContains('&lt;firstname&gt;', $fieldHTML);
+		$this->assertNotContains('<firstname>', $fieldHTML);
+	}
+
 }
 
 /**
diff --git a/tests/forms/OptionsetFieldTest.php b/tests/forms/OptionsetFieldTest.php
index d07f698af..da7237086 100644
--- a/tests/forms/OptionsetFieldTest.php
+++ b/tests/forms/OptionsetFieldTest.php
@@ -63,4 +63,18 @@ class OptionsetFieldTest extends SapphireTest {
 		preg_match('/Yes/', $field->Field(), $matches);
 		$this->assertEquals($matches[0], 'Yes');
 	}
+
+	public function testSafelyCast() {
+		$field1 = new OptionsetField('Options', 'Options', array(
+			1 => 'One',
+			2 => 'Two & Three',
+			3 => DBField::create_field('HTMLText', 'Four &amp; Five &amp; Six')
+		));
+		$fieldHTML = (string)$field1->Field();
+		$this->assertContains('One', $fieldHTML);
+		$this->assertContains('Two &amp; Three', $fieldHTML);
+		$this->assertNotContains('Two & Three', $fieldHTML);
+		$this->assertContains('Four &amp; Five &amp; Six', $fieldHTML);
+		$this->assertNotContains('Four & Five & Six', $fieldHTML);
+	}
 }