Fix escape modifier being a no-op under auto-escaping, breaking nofilter (#1188)#1204
Open
Amaury wants to merge 1 commit into
Open
Fix escape modifier being a no-op under auto-escaping, breaking nofilter (#1188)#1204Amaury wants to merge 1 commit into
Amaury wants to merge 1 commit into
Conversation
…ter (smarty-php#1188) When auto-escaping is enabled, the escape modifier (default 'html' mode) compiled to a no-op to avoid double-escaping. When the outer auto-escaping was suppressed (nofilter flag or |raw), nothing escaped the value at all, e.g. {$var|escape|nl2br nofilter} output raw HTML (CWE-79). The modifier now always emits htmlspecialchars() and marks its output as safe via setRawOutput(true) - the mechanism already used by the htmlall/ url/quotes/javascript modes - so auto-escaping skips it. This also keeps nl2br's <br /> tags unescaped in {$var|escape|nl2br}, and honors the charset/double_encode parameters again. The 'force' mode keeps its forced double-escaping semantics. Also harden the raw output marker so it cannot leak out of tags that do not print anything ({if $x|escape:'url'}, {assign var=y value=$x|raw}, {$x|escape assign=y}): it is now discarded at the end of every tag compilation and in the assign branch of PrintExpressionCompiler. Restore the nofilter documentation and document the 'force' escape mode. Fixes smarty-php#1188
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1188. Implements the approach discussed in the issue and approved by @wisskid.
Problem
Since 5.4.0 (#1030), when auto-escaping is enabled, the
escapemodifier (defaulthtmlmode) compiled to a no-op, on the assumption that the outer auto-escaping would escape the value anyway. When that outer escaping does not run (nofilterflag or|rawmodifier) nothing escapes the value at all:Fix
escape(htmlmode) now always emitshtmlspecialchars()and marks its output as safe viasetRawOutput(true)(the mechanism already used by thehtmlall,url,quotesandjavascriptmodes) so auto-escaping skips the already-escaped value. The explicit escape then applies regardless ofnofilter. Theforcemode keeps its forced double-escaping semantics by not setting the marker.{$var|escape|nl2br nofilter}<br />{$var|escape|nl2br}<br />escaped (broken)<br />{$var|escape}{$var|escape:'html':'UTF-8':false}{$var|escape:'force'}Hardening: the raw output marker no longer leaks between tags
The marker was only reset by the display branch of
PrintExpressionCompiler. When a marker-setting modifier was compiled inside a tag that prints nothing, the marker leaked and disabled auto-escaping of the next printed variable:This pre-existing issue would have widened with this fix (plain
|escapenow sets the marker), so the marker is now discarded at the end of everycompileTag()and in theassignbranch ofPrintExpressionCompiler. Any residual ordering edge case fails toward over-escaping, never toward raw output.Known limitation (unchanged, inherent to the single boolean marker, and already the case for
|rawand the other escape modes): the marker applies to the whole printed expression, so in{$a|escape|cat:$b}the concatenated$bis not escaped.Tests
9 new tests in
AutoEscapeTest(@group issue1188): the exact case from the report,nofilteralone,escape+nofilter,double_encodeparameter, and four marker-leak scenarios. Full suite: 2234 tests, 0 failures.Docs
nofilterdocumentation (dropped from the navigation with the Smarty 4$escape_htmlpage): now in the auto-escaping section ofconfiguring.md, plus mentions on theescapeandrawmodifier pages.forcemode and the interaction ofescapewith auto-escaping.