diff --git a/src/Neuron/Core/Template.php b/src/Neuron/Core/Template.php index 8ea5aee..e428661 100755 --- a/src/Neuron/Core/Template.php +++ b/src/Neuron/Core/Template.php @@ -223,6 +223,10 @@ private static function getFilenames ($template, $all = false) { $out = array (); + if (!self::isSafeName ($template)) { + return false; + } + foreach (self::getPaths () as $v) { // Split prefix and folder @@ -265,6 +269,88 @@ private static function getFilenames ($template, $all = false) return false; } + /** + * A template name is a path relative to the template folders. Names that + * are absolute, that contain a parent directory segment or that contain a + * NUL byte are never looked up. + * @param $template + * @return bool + */ + private static function isSafeName ($template) + { + $template = (string) $template; + + if (strpos ($template, "\0") !== false) { + return false; + } + + // Absolute: leading (back)slash or a drive letter. + if (preg_match ('#^(?:[/\\\\]|[A-Za-z]:[/\\\\])#', $template)) { + return false; + } + + return !in_array ('..', preg_split ('#[/\\\\]#', $template), true); + } + + /** + * Escape a string for output in HTML, text or attribute value. + * @param string $value + * @return string + */ + private static function escape ($value) + { + return htmlentities ($value, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'); + } + + /** + * The variables a template gets, for a page and for a partial alike: + * the shared variables, then the variables of this template. A string + * is escaped unless it was set with setRaw(); its original is available + * as $_name_. + * Partial parameters come last and are passed on as given: they are + * values of the calling template, which already got its own variables + * escaped. + * @param array $parameters + * @return array + */ + private function getTemplateVariables ($parameters = array ()) + { + $out = array (); + + foreach (array (self::$shares, $this->values) as $variables) { + foreach ($variables as $k => $v) { + if (is_string ($v) && !$this->isRaw ($k)) { + $out[$k] = self::escape ($v); + $out['_' . $k . '_'] = $v; + } else { + $out[$k] = $v; + } + } + } + + foreach ($parameters as $k => $v) { + $out[$k] = $v; + } + + // $this is the template itself, never a variable. + unset ($out['this']); + + return $out; + } + + /** + * Include a template file in a scope of its own. + * Takes the filename and the variables. It declares no parameters and no + * local variables on purpose: this scope becomes the scope of the template, + * so whatever the variables are called, there is nothing in here they + * could replace. + */ + private function includeTemplate () + { + extract (func_get_arg (1), EXTR_SKIP); + include func_get_arg (0); + } + public static function hasTemplate ($template) { return self::getFilenames ($template) ? true : false; @@ -286,32 +372,13 @@ public function parse ($template = null, $text = null) if (! $ctlbtmpltfiles = $this->getFilenames ($template)) { $out = '
The system could not find template "'.$template.'"
'; + $out .= 'The system could not find template "' . self::escape ((string) $template) . '"
'; return $out; } ob_start (); - foreach (self::$shares as $k => $v) - { - if (is_string($v) && !$this->isRaw($k)) { - ${$k} = htmlentities($v); - ${'_' . $k . '_'} = $v; - } else { - ${$k} = $v; - } - } - - foreach ($this->values as $k => $v) { - if (is_string($v) && !$this->isRaw($k)) { - ${$k} = htmlentities($v); - ${'_' . $k . '_'} = $v; - } else { - ${$k} = $v; - } - } - - include $ctlbtmpltfiles[0]; + $this->includeTemplate ($ctlbtmpltfiles[0], $this->getTemplateVariables ()); $val = ob_get_contents(); @@ -372,21 +439,11 @@ private function combine ($template, $parameters = []) { ob_start(); - foreach (self::$shares as $k => $v) { - ${$k} = $v; - } - - foreach ($this->values as $k => $v) { - ${$k} = $v; - } - - foreach ($parameters as $k => $v) { - ${$k} = $v; - } + $ctlbtmpltvars = $this->getTemplateVariables ($parameters); if ($ctlbtmpltfiles = $this->getFilenames($template, true)) { foreach ($ctlbtmpltfiles as $ctlbtmpltfile) { - include $ctlbtmpltfile; + $this->includeTemplate ($ctlbtmpltfile, $ctlbtmpltvars); } } @@ -406,21 +463,11 @@ private function template ($template, $parameters = []) { ob_start(); - foreach (self::$shares as $k => $v) { - ${$k} = $v; - } - - foreach ($this->values as $k => $v) { - ${$k} = $v; - } - - foreach ($parameters as $k => $v) { - ${$k} = $v; - } + $ctlbtmpltvars = $this->getTemplateVariables ($parameters); if ($ctlbtmpltfiles = $this->getFilenames($template)) { foreach ($ctlbtmpltfiles as $ctlbtmpltfile) { - include $ctlbtmpltfile; + $this->includeTemplate ($ctlbtmpltfile, $ctlbtmpltvars); } } diff --git a/tests/TemplateHardeningTest.php b/tests/TemplateHardeningTest.php new file mode 100644 index 0000000..cbf8cae --- /dev/null +++ b/tests/TemplateHardeningTest.php @@ -0,0 +1,310 @@ +x it's"; + const ESCAPED = '<b>x</b> it's'; + + protected function setUp (): void + { + Template::clearShares (); + Template::setPath (__DIR__ . '/templates/'); + } + + protected function tearDown (): void + { + Template::clearShares (); + } + + /** + * Every way a template gets rendered: as the page itself, or as a + * partial through each of the two helpers. + */ + public static function renderModes () + { + return [ + 'page' => [ null ], + 'template() partial' => [ 'hardening/viaTemplate.phpt' ], + 'combine() partial' => [ 'hardening/viaCombine.phpt' ], + ]; + } + + public static function partialHelpers () + { + return [ + 'template()' => [ 'Template' ], + 'combine()' => [ 'Combine' ], + ]; + } + + /** + * Render $name directly, or through the given wrapper template. + */ + private function template ($name, $wrapper) + { + if ($wrapper === null) { + return new Template ($name); + } + + $template = new Template ($wrapper); + $template->set ('partial', $name); + return $template; + } + + /** + * @dataProvider renderModes + */ + public function testStringVariableIsEscaped ($wrapper) + { + $template = $this->template ('hardening/value.phpt', $wrapper); + $template->set ('value', self::TEXT); + + $this->assertSame ('[' . self::ESCAPED . ']', $template->parse ()); + } + + /** + * @dataProvider renderModes + */ + public function testSharedStringVariableIsEscaped ($wrapper) + { + Template::share ('value', self::TEXT); + + $template = $this->template ('hardening/value.phpt', $wrapper); + + $this->assertSame ('[' . self::ESCAPED . ']', $template->parse ()); + } + + /** + * @dataProvider renderModes + */ + public function testInvalidUtf8IsSubstitutedInsteadOfDropped ($wrapper) + { + $template = $this->template ('hardening/value.phpt', $wrapper); + $template->set ('value', "a\xC3(b"); + + $this->assertSame ("[a\u{FFFD}(b]", $template->parse ()); + } + + /** + * @dataProvider renderModes + */ + public function testRawVariableStaysRaw ($wrapper) + { + $template = $this->template ('hardening/value.phpt', $wrapper); + $template->setRaw ('value', self::TEXT); + + $this->assertSame ('[' . self::TEXT . ']', $template->parse ()); + } + + /** + * @dataProvider renderModes + */ + public function testUnderscoreVariantHoldsTheUnescapedString ($wrapper) + { + $template = $this->template ('hardening/rawVariant.phpt', $wrapper); + $template->set ('value', self::TEXT); + + $this->assertSame ('[' . self::TEXT . ']', $template->parse ()); + } + + /** + * @dataProvider renderModes + */ + public function testNonStringVariablesAreLeftAlone ($wrapper) + { + $template = $this->template ('hardening/readNamed.phpt', $wrapper); + $template->set ('name', 'list'); + $template->set ('list', array ('x', 'y')); + + $this->assertSame ('[x,y]', $template->parse ()); + } + + /** + * A partial parameter is passed on as it is given: a template that hands + * one of its own (already escaped) variables to a partial must not get + * it escaped a second time. + * + * @dataProvider partialHelpers + */ + public function testParameterIsNotEscapedTwice ($helper) + { + $template = new Template ('hardening/passOn' . $helper . '.phpt'); + $template->set ('value', self::TEXT); + + $this->assertSame ('[' . self::ESCAPED . ']', $template->parse ()); + } + + public static function internalNames () + { + $other = __DIR__ . '/templates/hardening/other.phpt'; + + return [ + 'template' => [ 'template', 'hardening/other.phpt', 'hardening/other.phpt' ], + 'ctlbtmpltfiles' => [ 'ctlbtmpltfiles', array ($other), $other ], + 'ctlbtmpltfile' => [ 'ctlbtmpltfile', $other, $other ], + 'parameters' => [ 'parameters', array ('template' => 'hardening/other.phpt'), 'hardening/other.phpt' ], + 'val' => [ 'val', 'other', 'other' ], + ]; + } + + /** + * A variable that happens to be named like something the engine uses + * is just a variable: the requested template is rendered, and it can + * read the variable. + * + * @dataProvider internalNames + */ + public function testVariableNamedLikeAnInternalInAPage ($name, $value, $printed) + { + $template = new Template ('hardening/readNamed.phpt'); + $template->set ('name', $name); + $template->set ($name, $value); + + $this->assertSame ('[' . $printed . ']', $template->parse ()); + } + + /** + * @dataProvider internalNames + */ + public function testVariableNamedLikeAnInternalInATemplatePartial ($name, $value, $printed) + { + $template = new Template ('hardening/viaTemplate.phpt'); + $template->set ('partial', 'hardening/readNamed.phpt'); + $template->set ('name', $name); + $template->set ($name, $value); + + $this->assertSame ('[' . $printed . ']', $template->parse ()); + } + + /** + * @dataProvider internalNames + */ + public function testVariableNamedLikeAnInternalInACombinePartial ($name, $value, $printed) + { + $template = new Template ('hardening/viaCombine.phpt'); + $template->set ('partial', 'hardening/readNamed.phpt'); + $template->set ('name', $name); + $template->set ($name, $value); + + $this->assertSame ('[' . $printed . ']', $template->parse ()); + } + + /** + * @dataProvider partialHelpers + */ + public function testParameterNamedLikeAnInternal ($helper) + { + foreach (self::internalNames () as $case) { + list ($name, $value) = $case; + + $template = new Template ('hardening/parameter' . $helper . '.phpt'); + $template->set ('value', 'kept'); + $template->set ('name', $name); + $template->set ('injected', $value); + + $this->assertSame ('[kept]', $template->parse (), $name); + } + } + + public function testVariableNamedThisDoesNotReplaceTheTemplate () + { + $template = new Template ('hardening/viaTemplate.phpt'); + $template->set ('partial', 'hardening/value.phpt'); + $template->set ('value', 'kept'); + $template->set ('this', 'not a template'); + + $this->assertSame ('[kept]', $template->parse ()); + } + + public static function rejectedNames () + { + return [ + 'parent segment' => [ 'hardening/../hardening/value.phpt' ], + 'leading parent segment' => [ '../templates/hardening/value.phpt' ], + 'backslash parent segment' => [ 'hardening\\..\\hardening/value.phpt' ], + 'trailing parent segment' => [ 'hardening/..' ], + 'NUL byte' => [ "hardening/value.phpt\0" ], + 'NUL byte in the middle' => [ "hardening/value.phpt\0.txt" ], + 'absolute path' => [ __DIR__ . '/templates/hardening/value.phpt' ], + 'leading slash' => [ '/hardening/value.phpt' ], + 'leading backslash' => [ '\\hardening/value.phpt' ], + 'drive letter' => [ 'C:\\hardening\\value.phpt' ], + ]; + } + + /** + * @dataProvider rejectedNames + */ + public function testUnsafeNameIsNotATemplate ($name) + { + $this->assertFalse (Template::hasTemplate ($name)); + } + + /** + * @dataProvider rejectedNames + */ + public function testUnsafeNameIsNotRenderedAsAPage ($name) + { + $template = new Template ($name); + $template->set ('value', 'rendered'); + + $out = $template->parse (); + + $this->assertStringContainsString ('Template not found', $out); + $this->assertStringNotContainsString ('[rendered]', $out); + } + + /** + * @dataProvider rejectedNames + */ + public function testUnsafeNameIsNotRenderedAsAPartial ($name) + { + foreach (array ('hardening/viaTemplate.phpt', 'hardening/viaCombine.phpt') as $wrapper) { + $template = new Template ($wrapper); + $template->setRaw ('partial', $name); + $template->set ('value', 'rendered'); + + $this->assertSame ('', $template->parse (), $wrapper); + } + } + + public function testNamesWithDotsThatAreNotParentSegmentsStillWork () + { + $this->assertTrue (Template::hasTemplate ('hardening/value.phpt')); + $this->assertTrue (Template::hasTemplate ('hardening/./value.phpt')); + $this->assertFalse (Template::hasTemplate ('hardening/..value.phpt')); + $this->assertFalse (Template::hasTemplate ('hardening/missing.phpt')); + } + + public function testNotFoundMessageEscapesTheName () + { + $out = (new Template ("x it's.phpt"))->parse (); + + $this->assertStringContainsString ('Template not found', $out); + $this->assertStringContainsString (self::ESCAPED . '.phpt', $out); + $this->assertStringNotContainsString ('', $out); + } + + public function testRenderingLeavesTheOutputBufferLevelAlone () + { + $level = ob_get_level (); + + foreach (self::renderModes () as $mode) { + $template = $this->template ('hardening/value.phpt', $mode[0]); + $template->set ('value', 'x'); + $template->parse (); + } + + $this->assertSame ($level, ob_get_level ()); + } +} diff --git a/tests/templates/hardening/other.phpt b/tests/templates/hardening/other.phpt new file mode 100644 index 0000000..27fa349 --- /dev/null +++ b/tests/templates/hardening/other.phpt @@ -0,0 +1 @@ +other \ No newline at end of file diff --git a/tests/templates/hardening/parameterCombine.phpt b/tests/templates/hardening/parameterCombine.phpt new file mode 100644 index 0000000..a09ef7b --- /dev/null +++ b/tests/templates/hardening/parameterCombine.phpt @@ -0,0 +1 @@ +combine ("hardening/value.phpt", array ($name => $injected)); ?> \ No newline at end of file diff --git a/tests/templates/hardening/parameterTemplate.phpt b/tests/templates/hardening/parameterTemplate.phpt new file mode 100644 index 0000000..0734c75 --- /dev/null +++ b/tests/templates/hardening/parameterTemplate.phpt @@ -0,0 +1 @@ +template ("hardening/value.phpt", array ($name => $injected)); ?> \ No newline at end of file diff --git a/tests/templates/hardening/passOnCombine.phpt b/tests/templates/hardening/passOnCombine.phpt new file mode 100644 index 0000000..416ea0c --- /dev/null +++ b/tests/templates/hardening/passOnCombine.phpt @@ -0,0 +1 @@ +combine ("hardening/value.phpt", array ("value" => $value)); ?> \ No newline at end of file diff --git a/tests/templates/hardening/passOnTemplate.phpt b/tests/templates/hardening/passOnTemplate.phpt new file mode 100644 index 0000000..3e3e931 --- /dev/null +++ b/tests/templates/hardening/passOnTemplate.phpt @@ -0,0 +1 @@ +template ("hardening/value.phpt", array ("value" => $value)); ?> \ No newline at end of file diff --git a/tests/templates/hardening/rawVariant.phpt b/tests/templates/hardening/rawVariant.phpt new file mode 100644 index 0000000..5fe291c --- /dev/null +++ b/tests/templates/hardening/rawVariant.phpt @@ -0,0 +1 @@ +[] \ No newline at end of file diff --git a/tests/templates/hardening/readNamed.phpt b/tests/templates/hardening/readNamed.phpt new file mode 100644 index 0000000..f0bb723 --- /dev/null +++ b/tests/templates/hardening/readNamed.phpt @@ -0,0 +1 @@ +[] \ No newline at end of file diff --git a/tests/templates/hardening/value.phpt b/tests/templates/hardening/value.phpt new file mode 100644 index 0000000..9df5dad --- /dev/null +++ b/tests/templates/hardening/value.phpt @@ -0,0 +1 @@ +[] \ No newline at end of file diff --git a/tests/templates/hardening/viaCombine.phpt b/tests/templates/hardening/viaCombine.phpt new file mode 100644 index 0000000..a319a13 --- /dev/null +++ b/tests/templates/hardening/viaCombine.phpt @@ -0,0 +1 @@ +combine ($partial); ?> \ No newline at end of file diff --git a/tests/templates/hardening/viaTemplate.phpt b/tests/templates/hardening/viaTemplate.phpt new file mode 100644 index 0000000..16a59d0 --- /dev/null +++ b/tests/templates/hardening/viaTemplate.phpt @@ -0,0 +1 @@ +template ($partial); ?> \ No newline at end of file