From a2a3dd622af4850e4207073b9a6befad4459774f Mon Sep 17 00:00:00 2001 From: Rex Morgan Date: Thu, 6 Aug 2026 20:33:45 -0400 Subject: [PATCH] fix: stop leaking internal SafeString type through subexpression results (issue #660) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ReturnInvoke (added for issue #543, 2026-06-20) wrapped every writer-based helper's captured output in an internal sealed SafeString when used as a subexpression, so the outer expression wouldn't double-encode already-safe content. But only a few internal call sites were taught to unwrap it (EncodedTextWriter.Write, WriteSafeString(object), HandlebarsUtils.IsFalsy, PartialBinder) — any other consumer, including reflection-based/typed helper binders in third-party packages this library has no visibility into, received an opaque internal type it could neither cast to string nor unwrap. Replaced the wrapper type with SafeStrings, a ConditionalWeakTable-backed marker keyed by object reference. ReturnInvoke now returns the captured string itself, invisibly marked as already-encoded, instead of boxing it in a new type. The value stays a genuine System.String all the way through — castable, reflectable, and indistinguishable from any other string to every consumer except the encoding writer, which is the only place the signal actually needs to matter. This also let PartialBinder revert to its original Cast(pex.PartialName) and dropped the now-redundant SafeString cases in HandlebarsUtils.IsFalsy and HandlebarsExtensions.WriteSafeString(object). Added regression coverage: a subexpression's captured result is asserted to be typeof(string) and to survive a direct (string) cast, mirroring the reporter's Append(string, string) helper receiving a subexpression argument. Co-Authored-By: Claude Sonnet 5 --- source/Handlebars.Test/IssueTests.cs | 43 +++++++++++++++++++ .../Translation/Expression/PartialBinder.cs | 12 +----- source/Handlebars/HandlebarsExtensions.cs | 6 --- source/Handlebars/HandlebarsUtils.cs | 2 - source/Handlebars/Helpers/HelperExtensions.cs | 9 ++-- source/Handlebars/IO/EncodedTextWriter.cs | 3 +- source/Handlebars/IO/SafeStrings.cs | 30 +++++++++++++ source/Handlebars/SafeString.cs | 17 -------- 8 files changed, 81 insertions(+), 41 deletions(-) create mode 100644 source/Handlebars/IO/SafeStrings.cs delete mode 100644 source/Handlebars/SafeString.cs diff --git a/source/Handlebars.Test/IssueTests.cs b/source/Handlebars.Test/IssueTests.cs index d94aaa2c..5396af96 100644 --- a/source/Handlebars.Test/IssueTests.cs +++ b/source/Handlebars.Test/IssueTests.cs @@ -1270,6 +1270,49 @@ public void Issue601_DefaultInterfaceMemberPropertyIsEnumerated() Assert.Contains($"OtherStr={data.OtherStr};", result); } + // Issue: https://github.com/Handlebars-Net/Handlebars.Net/issues/660 + // A writer-based helper used as a subexpression must hand its captured output to the + // outer helper as a plain System.String, not an opaque internal wrapper type — otherwise + // reflection-based/typed argument binders (including third-party ones this library can't + // patch) can't consume it at all. + [Fact] + public void Issue660_SubexpressionResultIsPlainString() + { + var handlebars = Handlebars.Create(); + handlebars.RegisterHelper("inner", (writer, context, arguments) => writer.WriteSafeString("ab")); + + object? captured = null; + handlebars.RegisterHelper("outer", (writer, context, arguments) => + { + captured = arguments[0]; + writer.Write(captured); + }); + + handlebars.Compile("{{outer (inner)}}")(new { }); + + Assert.IsType(captured); + Assert.Equal("ab", captured); + } + + // Mirrors the reporter's Append(string value, string append) helper: a naive binder that + // direct-casts an argument to string must not throw just because that argument came from + // a subexpression instead of template data. + [Fact] + public void Issue660_SubexpressionResultIsCastableToTypedStringParameter() + { + var handlebars = Handlebars.Create(); + handlebars.RegisterHelper("inner", (writer, context, arguments) => writer.WriteSafeString("a")); + handlebars.RegisterHelper("outer", (writer, context, arguments) => + { + string value = (string) arguments[0]!; + writer.Write(value + "b"); + }); + + var result = handlebars.Compile("{{outer (inner)}}")(new { }); + + Assert.Equal("ab", result); + } + private static void RegisterStringEqualityBlockHelper(IHandlebars handlebars) { handlebars.RegisterHelper("StringEqualityBlockHelper", (output, options, context, arguments) => diff --git a/source/Handlebars/Compiler/Translation/Expression/PartialBinder.cs b/source/Handlebars/Compiler/Translation/Expression/PartialBinder.cs index a94152fe..dd870dc3 100644 --- a/source/Handlebars/Compiler/Translation/Expression/PartialBinder.cs +++ b/source/Handlebars/Compiler/Translation/Expression/PartialBinder.cs @@ -15,12 +15,6 @@ internal class PartialBinder : HandlebarsExpressionVisitor { private static string SpecialPartialBlockName = "@partial-block"; - private static string ToPartialName(object value) - { - if (value is SafeString safe) return safe.Value; - return (string) value; - } - private CompilationContext CompilationContext { get; } public PartialBinder(CompilationContext compilationContext) @@ -55,8 +49,7 @@ protected override Expression VisitPartialExpression(PartialExpression pex) bindingContext = bindingContext.Call(o => o.CreateChildContext(value, partialTemplate)); } - var partialNameObj = Arg(pex.PartialName); - var partialName = Call(() => ToPartialName(partialNameObj)); + var partialName = Cast(pex.PartialName); var configuration = Arg(CompilationContext.Configuration); var isBlock = Arg(pex.IsBlock); var indent = Arg(pex.Indent); @@ -90,8 +83,7 @@ out _ bindingContext = bindingContext.Call(o => o.CreateChildContext(value, partialTemplate)); } - var partialNameObj = Arg(pex.PartialName); - var partialName = Call(() => ToPartialName(partialNameObj)); + var partialName = Cast(pex.PartialName); var configuration = Arg(CompilationContext.Configuration); var isBlock = Arg(pex.IsBlock); var indent = Arg(pex.Indent); diff --git a/source/Handlebars/HandlebarsExtensions.cs b/source/Handlebars/HandlebarsExtensions.cs index 3bc22177..86bd3053 100644 --- a/source/Handlebars/HandlebarsExtensions.cs +++ b/source/Handlebars/HandlebarsExtensions.cs @@ -28,12 +28,6 @@ public static void WriteSafeString(this in EncodedTextWriter writer, object? val return; } - if (value is SafeString safe) - { - writer.WriteSafeString(safe.Value); - return; - } - var current = writer.SuppressEncoding; try { diff --git a/source/Handlebars/HandlebarsUtils.cs b/source/Handlebars/HandlebarsUtils.cs index 7a2f01b9..a5dedc18 100644 --- a/source/Handlebars/HandlebarsUtils.cs +++ b/source/Handlebars/HandlebarsUtils.cs @@ -36,8 +36,6 @@ public static bool IsFalsy([NotNullWhen(false)] object? value, bool includeZero) return !b; case string s: return s == string.Empty; - case SafeString safe: - return safe.Value == string.Empty; case JsonElement element: return IsFalsyJsonElement(element, includeZero); } diff --git a/source/Handlebars/Helpers/HelperExtensions.cs b/source/Handlebars/Helpers/HelperExtensions.cs index 630805f0..dc28f135 100644 --- a/source/Handlebars/Helpers/HelperExtensions.cs +++ b/source/Handlebars/Helpers/HelperExtensions.cs @@ -20,10 +20,11 @@ in Arguments arguments descriptor.Invoke(output, options, context, arguments); - // Return a SafeString so the captured output — which already has the correct - // encoding applied by the EncodedTextWriter — is not encoded a second time - // when it is passed as an argument to an outer helper. - return new SafeString(writer.ToString()); + // Mark the captured output — which already has the correct encoding applied by the + // EncodedTextWriter — so it is not encoded a second time when written elsewhere, without + // wrapping it in a type that would leak into helper argument binding as something other + // than a plain string. + return SafeStrings.Mark(writer.ToString()); } } } \ No newline at end of file diff --git a/source/Handlebars/IO/EncodedTextWriter.cs b/source/Handlebars/IO/EncodedTextWriter.cs index b152115a..e74f3a74 100644 --- a/source/Handlebars/IO/EncodedTextWriter.cs +++ b/source/Handlebars/IO/EncodedTextWriter.cs @@ -125,10 +125,9 @@ public void Write(T? value) case Substring substring when substring.Length == 0: return; - case string v: Write(v, true); return; + case string v: Write(v, !SafeStrings.IsSafe(v)); return; case StringBuilder v: Write(v, true); return; case Substring v: Write(v, true); return; - case SafeString safe: Write(safe.Value, false); return; default: WriteFormatted(value); diff --git a/source/Handlebars/IO/SafeStrings.cs b/source/Handlebars/IO/SafeStrings.cs new file mode 100644 index 00000000..09c444e7 --- /dev/null +++ b/source/Handlebars/IO/SafeStrings.cs @@ -0,0 +1,30 @@ +using System.Runtime.CompilerServices; + +namespace HandlebarsDotNet.IO +{ + /// + /// Tracks which instances already went through the encoding pipeline + /// (e.g. captured output of a subexpression helper) so they are not encoded a second time + /// when written elsewhere. Marking is by object reference, not value, so it never affects + /// any string a caller didn't obtain from this exact pipeline — and critically, the marked + /// value stays a plain the whole way through, so it round-trips safely + /// through helper argument binding, reflection-based helpers, and any other consumer that + /// only knows how to handle . + /// + internal static class SafeStrings + { + private static readonly ConditionalWeakTable Marked = new(); + private static readonly object Sentinel = new(); + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static string Mark(string value) + { + if (value.Length == 0) return value; + Marked.GetValue(value, _ => Sentinel); + return value; + } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static bool IsSafe(string value) => value.Length == 0 || Marked.TryGetValue(value, out _); + } +} diff --git a/source/Handlebars/SafeString.cs b/source/Handlebars/SafeString.cs deleted file mode 100644 index cd5fa4c5..00000000 --- a/source/Handlebars/SafeString.cs +++ /dev/null @@ -1,17 +0,0 @@ -namespace HandlebarsDotNet -{ - /// - /// Wraps a string value that has already been HTML-encoded (or is intentionally unencoded HTML). - /// When written to an , the content is passed through without - /// additional encoding. This is the return-value counterpart of - /// . - /// - internal sealed class SafeString - { - public readonly string Value; - - public SafeString(string value) => Value = value; - - public override string ToString() => Value; - } -}