From 4415a75d9304a3930b9c28763fc092299640c685 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Wed, 26 Mar 2014 18:27:07 +1300 Subject: [PATCH] BUG Fix issue with versioned dataobjects being cached between stages --- docs/en/changelogs/3.0.10.md | 12 ++++ docs/en/reference/partial-caching.md | 13 ++++ model/Versioned.php | 10 ++- tests/view/SSViewerCacheBlockTest.php | 96 ++++++++++++++++++++++++++- tests/view/SSViewerTest.php | 3 +- view/SSTemplateParser.php | 65 ++++++++++++------ view/SSTemplateParser.php.inc | 23 +++++-- view/SSViewer.php | 31 ++++++++- 8 files changed, 222 insertions(+), 31 deletions(-) create mode 100644 docs/en/changelogs/3.0.10.md diff --git a/docs/en/changelogs/3.0.10.md b/docs/en/changelogs/3.0.10.md new file mode 100644 index 000000000..427b54498 --- /dev/null +++ b/docs/en/changelogs/3.0.10.md @@ -0,0 +1,12 @@ +# 3.0.10 + +## Overview + + * Security: Partially cached content from stage or other reading modes is no longer emitted to live + +## Upgrading + + * If relying on partial caching of content between logged in users, be aware that the cache is now automatically + segmented based on both the current member ID, and the versioned reading mode. If this is not an appropriate + method (such as if the same content is served to logged in users within partial caching) then it is necessary + to adjust the config value of `SSViewer::global_key` to something more or less sensitive. \ No newline at end of file diff --git a/docs/en/reference/partial-caching.md b/docs/en/reference/partial-caching.md index 8855cd93b..b9176e878 100644 --- a/docs/en/reference/partial-caching.md +++ b/docs/en/reference/partial-caching.md @@ -51,6 +51,19 @@ From a block that shows a summary of the page edits if administrator, nothing if <% cached 'loginblock', LastEdited, CurrentMember.isAdmin %> +An additional global key is incorporated in the cache lookup. The default value for this is +`$CurrentReadingMode, $CurrentUser.ID`, which ensures that the current `[api:Versioned]` state and user ID are +used. This may be configured by changing the config value of `SSViewer.global_key`. It is also necessary +to flush the template caching when modifying this config, as this key is cached within the template itself. + +For example, to ensure that the cache is configured to respect another variable, and if the current logged in +user does not influence your template content, you can update this key as below; + + :::yaml + SSViewer: + global_key: '$CurrentReadingMode, $Locale' + + ## Aggregates Often you want to invalidate a cache when any in a set of objects change, or when the objects in a relationship change. diff --git a/model/Versioned.php b/model/Versioned.php index 8e362be2b..615c1f355 100644 --- a/model/Versioned.php +++ b/model/Versioned.php @@ -5,7 +5,8 @@ * @package framework * @subpackage model */ -class Versioned extends DataExtension { +class Versioned extends DataExtension implements TemplateGlobalProvider { + /** * An array of possible stages. * @var array @@ -1144,6 +1145,13 @@ class Versioned extends DataExtension { public function cacheKeyComponent() { return 'versionedmode-'.self::get_reading_mode(); } + + public static function get_template_global_variables() { + return array( + 'CurrentReadingMode' => 'get_reading_mode' + ); + } + } /** diff --git a/tests/view/SSViewerCacheBlockTest.php b/tests/view/SSViewerCacheBlockTest.php index 7350da29e..2c0ed683f 100644 --- a/tests/view/SSViewerCacheBlockTest.php +++ b/tests/view/SSViewerCacheBlockTest.php @@ -20,9 +20,29 @@ class SSViewerCacheBlockTest_Model extends DataObject implements TestOnly { } } +class SSViewerCacheBlockTest_VersionedModel extends DataObject implements TestOnly { + + protected $entropy = 'default'; + + public static $extensions = array( + "Versioned('Stage', 'Live')" + ); + + public function setEntropy($entropy) { + $this->entropy = $entropy; + } + + public function Inspect() { + return $this->entropy . ' ' . Versioned::get_reading_mode(); + } +} + class SSViewerCacheBlockTest extends SapphireTest { - protected $extraDataObjects = array('SSViewerCacheBlockTest_Model'); + protected $extraDataObjects = array( + 'SSViewerCacheBlockTest_Model', + 'SSViewerCacheBlockTest_VersionedModel' + ); protected $data = null; @@ -37,8 +57,7 @@ class SSViewerCacheBlockTest extends SapphireTest { if ($data === null) $data = $this->data; if (is_array($data)) $data = $this->data->customise($data); - $viewer = SSViewer::fromString($template); - return $viewer->process($data); + return SSViewer::execute_string($template, $data); } public function testParsing() { @@ -104,6 +123,77 @@ class SSViewerCacheBlockTest extends SapphireTest { $this->assertEquals($this->_runtemplate('<% cached %>$Foo<% end_cached %>', array('Foo' => 1)), '1'); $this->assertEquals($this->_runtemplate('<% cached %>$Foo<% end_cached %>', array('Foo' => 2)), '1'); } + + public function testVersionedCache() { + + $origStage = Versioned::current_stage(); + + // Run without caching in stage to prove data is uncached + $this->_reset(false); + Versioned::reading_stage("Stage"); + $data = new SSViewerCacheBlockTest_VersionedModel(); + $data->setEntropy('default'); + $this->assertEquals( + 'default Stage.Stage', + SSViewer::execute_string('<% cached %>$Inspect<% end_cached %>', $data) + ); + $data = new SSViewerCacheBlockTest_VersionedModel(); + $data->setEntropy('first'); + $this->assertEquals( + 'first Stage.Stage', + SSViewer::execute_string('<% cached %>$Inspect<% end_cached %>', $data) + ); + + // Run without caching in live to prove data is uncached + $this->_reset(false); + Versioned::reading_stage("Live"); + $data = new SSViewerCacheBlockTest_VersionedModel(); + $data->setEntropy('default'); + $this->assertEquals( + 'default Stage.Live', + $this->_runtemplate('<% cached %>$Inspect<% end_cached %>', $data) + ); + $data = new SSViewerCacheBlockTest_VersionedModel(); + $data->setEntropy('first'); + $this->assertEquals( + 'first Stage.Live', + $this->_runtemplate('<% cached %>$Inspect<% end_cached %>', $data) + ); + + // Then with caching, initially in draft, and then in live, to prove that + // changing the versioned reading mode doesn't cache between modes, but it does + // within them + $this->_reset(true); + Versioned::reading_stage("Stage"); + $data = new SSViewerCacheBlockTest_VersionedModel(); + $data->setEntropy('default'); + $this->assertEquals( + 'default Stage.Stage', + $this->_runtemplate('<% cached %>$Inspect<% end_cached %>', $data) + ); + $data = new SSViewerCacheBlockTest_VersionedModel(); + $data->setEntropy('first'); + $this->assertEquals( + 'default Stage.Stage', // entropy should be ignored due to caching + $this->_runtemplate('<% cached %>$Inspect<% end_cached %>', $data) + ); + + Versioned::reading_stage('Live'); + $data = new SSViewerCacheBlockTest_VersionedModel(); + $data->setEntropy('first'); + $this->assertEquals( + 'first Stage.Live', // First hit in live, so display current entropy + $this->_runtemplate('<% cached %>$Inspect<% end_cached %>', $data) + ); + $data = new SSViewerCacheBlockTest_VersionedModel(); + $data->setEntropy('second'); + $this->assertEquals( + 'first Stage.Live', // entropy should be ignored due to caching + $this->_runtemplate('<% cached %>$Inspect<% end_cached %>', $data) + ); + + Versioned::reading_stage($origStage); + } /** * Test that cacheblocks conditionally cache with if diff --git a/tests/view/SSViewerTest.php b/tests/view/SSViewerTest.php index 45319cb35..046bb4b68 100644 --- a/tests/view/SSViewerTest.php +++ b/tests/view/SSViewerTest.php @@ -35,9 +35,8 @@ class SSViewerTest extends SapphireTest { * Small helper to render templates from strings */ public function render($templateString, $data = null) { - $t = SSViewer::fromString($templateString); if(!$data) $data = new SSViewerTestFixture(); - return $t->process($data); + return SSViewer::execute_string($templateString, $data); } public function testRequirements() { diff --git a/view/SSTemplateParser.php b/view/SSTemplateParser.php index 5cd895538..3e4177028 100644 --- a/view/SSTemplateParser.php +++ b/view/SSTemplateParser.php @@ -646,7 +646,8 @@ class SSTemplateParser extends Parser { } - /* Translate: "<%t" < Entity < (Default:QuotedString)? < (!("is" "=") < "is" < Context:QuotedString)? < (InjectionVariables)? > "%>" */ + /* Translate: "<%t" < Entity < (Default:QuotedString)? < (!("is" "=") < "is" < Context:QuotedString)? < + (InjectionVariables)? > "%>" */ protected $match_Translate_typestack = array('Translate'); function match_Translate ($stack = array()) { $matchrule = "Translate"; $result = $this->construct($matchrule, $matchrule, null); @@ -1671,7 +1672,7 @@ class SSTemplateParser extends Parser { function Require_Call(&$res, $sub) { $res['php'] = "Requirements::".$sub['Method']['text'].'('.$sub['CallArguments']['php'].');'; } - + /* CacheBlockArgument: !( "if " | "unless " ) @@ -2743,10 +2744,25 @@ class SSTemplateParser extends Parser { function CacheBlock_CacheBlockTemplate(&$res, $sub){ // Get the block counter $block = ++$res['subblocks']; - // Build the key for this block from the passed cache key, the block index, and the sha hash of the template - // itself - $key = "'" . sha1($sub['php']) . (isset($res['key']) && $res['key'] ? "_'.sha1(".$res['key'].")" : "'") . - ".'_$block'"; + // Build the key for this block from the global key (evaluated in a closure within the template), + // the passed cache key, the block index, and the sha hash of the template. + $res['php'] .= '$keyExpression = function() use ($scope, $cache) {' . PHP_EOL; + $res['php'] .= '$val = \'\';' . PHP_EOL; + if($globalKey = Config::inst()->get('SSViewer', 'global_key')) { + // Embed the code necessary to evaluate the globalKey directly into the template, + // so that SSTemplateParser only needs to be called during template regeneration. + // Warning: If the global key is changed, it's necessary to flush the template cache. + $parser = new SSTemplateParser($globalKey); + $result = $parser->match_Template(); + if(!$result) throw new SSTemplateParseException('Unexpected problem parsing template', $parser); + $res['php'] .= $result['php'] . PHP_EOL; + } + $res['php'] .= 'return $val;' . PHP_EOL; + $res['php'] .= '};' . PHP_EOL; + $key = 'sha1($keyExpression())' // Global key + . '.\'_' . sha1($sub['php']) // sha of template + . (isset($res['key']) && $res['key'] ? "_'.sha1(".$res['key'].")" : "'") // Passed key + . ".'_$block'"; // block index // Get any condition $condition = isset($res['condition']) ? $res['condition'] : ''; @@ -2914,7 +2930,7 @@ class SSTemplateParser extends Parser { function OldTTag_OldTPart(&$res, $sub) { $res['php'] = $sub['php']; } - + /* OldSprintfTag: "<%" < "sprintf" < "(" < OldTPart < "," < CallArguments > ")" > "%>" */ protected $match_OldSprintfTag_typestack = array('OldSprintfTag'); function match_OldSprintfTag ($stack = array()) { @@ -3642,7 +3658,8 @@ class SSTemplateParser extends Parser { $method = 'OpenBlock_Handle_'.$blockname; if (method_exists($this, $method)) $res['php'] = $this->$method($res); else { - throw new SSTemplateParseException('Unknown open block "'.$blockname.'" encountered. Perhaps you missed the closing tag or have mis-spelled it?', $this); + throw new SSTemplateParseException('Unknown open block "'.$blockname.'" encountered. Perhaps you missed ' . + ' the closing tag or have mis-spelled it?', $this); } } @@ -3711,7 +3728,8 @@ class SSTemplateParser extends Parser { function MismatchedEndBlock__finalise(&$res) { $blockname = $res['Word']['text']; - throw new SSTemplateParseException('Unexpected close tag end_'.$blockname.' encountered. Perhaps you have mis-nested blocks, or have mis-spelled a tag?', $this); + throw new SSTemplateParseException('Unexpected close tag end_' . $blockname . + ' encountered. Perhaps you have mis-nested blocks, or have mis-spelled a tag?', $this); } /* MalformedOpenTag: '<%' < !NotBlockTag Tag:Word !( ( [ :BlockArguments ] )? > '%>' ) */ @@ -3796,7 +3814,8 @@ class SSTemplateParser extends Parser { function MalformedOpenTag__finalise(&$res) { $tag = $res['Tag']['text']; - throw new SSTemplateParseException("Malformed opening block tag $tag. Perhaps you have tried to use operators?", $this); + throw new SSTemplateParseException("Malformed opening block tag $tag. Perhaps you have tried to use operators?" + , $this); } /* MalformedCloseTag: '<%' < Tag:('end_' :Word ) !( > '%>' ) */ @@ -3860,7 +3879,8 @@ class SSTemplateParser extends Parser { function MalformedCloseTag__finalise(&$res) { $tag = $res['Tag']['text']; - throw new SSTemplateParseException("Malformed closing block tag $tag. Perhaps you have tried to pass an argument to one?", $this); + throw new SSTemplateParseException("Malformed closing block tag $tag. Perhaps you have tried to pass an " . + "argument to one?", $this); } /* MalformedBlock: MalformedOpenTag | MalformedCloseTag */ @@ -4448,10 +4468,12 @@ class SSTemplateParser extends Parser { $text = stripslashes($text); $text = addcslashes($text, '\'\\'); - // TODO: This is pretty ugly & gets applied on all files not just html. I wonder if we can make this non-dynamically calculated + // TODO: This is pretty ugly & gets applied on all files not just html. I wonder if we can make this + // non-dynamically calculated $text = preg_replace( '/href\s*\=\s*\"\#/', - 'href="\' . (SSViewer::$options[\'rewriteHashlinks\'] ? strip_tags( $_SERVER[\'REQUEST_URI\'] ) : "") . \'#', + 'href="\' . (SSViewer::$options[\'rewriteHashlinks\'] ? strip_tags( $_SERVER[\'REQUEST_URI\'] ) : "") . + \'#', $text ); @@ -4467,10 +4489,10 @@ class SSTemplateParser extends Parser { * * @static * @throws SSTemplateParseException - * @param $string - The source of the template - * @param string $templateName - The name of the template, normally the filename the template source was loaded from - * @param bool $includeDebuggingComments - True is debugging comments should be included in the output - * @return mixed|string - The php that, when executed (via include or exec) will behave as per the template source + * @param $string The source of the template + * @param string $templateName The name of the template, normally the filename the template source was loaded from + * @param bool $includeDebuggingComments True is debugging comments should be included in the output + * @return mixed|string The php that, when executed (via include or exec) will behave as per the template source */ static function compileString($string, $templateName = "", $includeDebuggingComments=false) { if (!trim($string)) { @@ -4481,7 +4503,8 @@ class SSTemplateParser extends Parser { $parser = new SSTemplateParser($string); $parser->includeDebuggingComments = $includeDebuggingComments; - // Ignore UTF8 BOM at begining of string. TODO: Confirm this is needed, make sure SSViewer handles UTF (and other encodings) properly + // Ignore UTF8 BOM at begining of string. TODO: Confirm this is needed, make sure SSViewer handles UTF + // (and other encodings) properly if(substr($string, 0,3) == pack("CCC", 0xef, 0xbb, 0xbf)) $parser->pos = 3; // Match the source against the parser @@ -4494,12 +4517,14 @@ class SSTemplateParser extends Parser { // Include top level debugging comments if desired if($includeDebuggingComments && $templateName && stripos($code, "]*>)/i', "\\1", $code); $code = preg_replace('/(<\/html[^>]*>)/i', "\\1", $code); } else { - $code = str_replace('\';' . "\n", $code); + $code = str_replace('\';' . "\n", $code); $code .= "\n" . '$val .= \'\';'; } } diff --git a/view/SSTemplateParser.php.inc b/view/SSTemplateParser.php.inc index 7652ed105..e58de6031 100644 --- a/view/SSTemplateParser.php.inc +++ b/view/SSTemplateParser.php.inc @@ -559,10 +559,25 @@ class SSTemplateParser extends Parser { function CacheBlock_CacheBlockTemplate(&$res, $sub){ // Get the block counter $block = ++$res['subblocks']; - // Build the key for this block from the passed cache key, the block index, and the sha hash of the template - // itself - $key = "'" . sha1($sub['php']) . (isset($res['key']) && $res['key'] ? "_'.sha1(".$res['key'].")" : "'") . - ".'_$block'"; + // Build the key for this block from the global key (evaluated in a closure within the template), + // the passed cache key, the block index, and the sha hash of the template. + $res['php'] .= '$keyExpression = function() use ($scope, $cache) {' . PHP_EOL; + $res['php'] .= '$val = \'\';' . PHP_EOL; + if($globalKey = Config::inst()->get('SSViewer', 'global_key')) { + // Embed the code necessary to evaluate the globalKey directly into the template, + // so that SSTemplateParser only needs to be called during template regeneration. + // Warning: If the global key is changed, it's necessary to flush the template cache. + $parser = new SSTemplateParser($globalKey); + $result = $parser->match_Template(); + if(!$result) throw new SSTemplateParseException('Unexpected problem parsing template', $parser); + $res['php'] .= $result['php'] . PHP_EOL; + } + $res['php'] .= 'return $val;' . PHP_EOL; + $res['php'] .= '};' . PHP_EOL; + $key = 'sha1($keyExpression())' // Global key + . '.\'_' . sha1($sub['php']) // sha of template + . (isset($res['key']) && $res['key'] ? "_'.sha1(".$res['key'].")" : "'") // Passed key + . ".'_$block'"; // block index // Get any condition $condition = isset($res['condition']) ? $res['condition'] : ''; diff --git a/view/SSViewer.php b/view/SSViewer.php index f1bb51b52..9c02253f3 100644 --- a/view/SSViewer.php +++ b/view/SSViewer.php @@ -46,7 +46,6 @@ class SSViewer_Scope { private $localIndex; - public function __construct($item){ $this->item = $item; $this->localIndex=0; @@ -568,6 +567,14 @@ class SSViewer { */ protected $includeRequirements = true; + /** + * Default prepended cache key for partial caching + * + * @var string + * @config + */ + static $global_key = '$CurrentReadingMode, $CurrentUser.ID'; + /** * Create a template from a string instead of a .ss file * @@ -936,6 +943,11 @@ class SSViewer { /** * Execute the given template, passing it the given data. * Used by the <% include %> template tag to process templates. + * + * @param string $template Template name + * @param mixed $data Data context + * @param array $arguments Additional arguments + * @return string Evaluated result */ public static function execute_template($template, $data, $arguments = null) { $v = new SSViewer($template); @@ -943,6 +955,23 @@ class SSViewer { return $v->process($data, $arguments); } + + /** + * Execute the evaluated string, passing it the given data. + * Used by partial caching to evaluate custom cache keys expressed using + * template expressions + * + * @param string $content Input string + * @param mixed $data Data context + * @param array $arguments Additional arguments + * @return string Evaluated result + */ + public static function execute_string($content, $data, $arguments = null) { + $v = SSViewer::fromString($content); + $v->includeRequirements(false); + + return $v->process($data, $arguments); + } public static function parseTemplateContent($content, $template="") { return SSTemplateParser::compileString($content, $template, Director::isDev() && self::$source_file_comments);