Skip to content

fix: stop leaking internal SafeString type through subexpression results (issue #660) - #664

Merged
rexm merged 1 commit into
masterfrom
fix/issue-660-safestring-leak
Aug 7, 2026
Merged

fix: stop leaking internal SafeString type through subexpression results (issue #660)#664
rexm merged 1 commit into
masterfrom
fix/issue-660-safestring-leak

Conversation

@rexm

@rexm rexm commented Aug 7, 2026

Copy link
Copy Markdown
Member

Summary

  • ReturnInvoke (added for issue Subexpression encoding/escaping issue #543 on 2026-06-20) wraps every writer-based helper's captured output in an internal sealed class SafeString whenever it's used as a subexpression, so the outer expression doesn't double-encode already-safe content (e.g. content written via WriteSafeString).
  • Only a handful of internal call sites were taught to unwrap that type: EncodedTextWriter.Write<T>, HandlebarsExtensions.WriteSafeString(object), HandlebarsUtils.IsFalsy, and PartialBinder. Any other consumer — including reflection-based/typed helper binders in third-party packages this library has no visibility into (e.g. Handlebars.Net.Helpers) — receives an opaque internal type it can neither cast to string nor unwrap.
  • Reproduced the reporter's exact failure: a naive (string) arguments[0] cast on a subexpression's result throws InvalidCastException: Unable to cast object of type 'HandlebarsDotNet.SafeString' to type 'System.String'.

Fix

Replaced the wrapper type with SafeStrings (source/Handlebars/IO/SafeStrings.cs), a ConditionalWeakTable<string, object>-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 the entire way through — castable, reflectable, and indistinguishable from any other string to every consumer except the encoding writer, which is the only place the "already encoded" signal actually needs to matter. Reference-keyed lookup means marking never affects a string a caller didn't obtain from this exact capture (no risk from string interning, since StringWriter.ToString() always produces a fresh instance).

This also let PartialBinder revert to its original Cast<string>(pex.PartialName) (dynamic partial names no longer need SafeString-aware unwrapping — they're just strings again) and dropped the now-redundant SafeString cases in HandlebarsUtils.IsFalsy and HandlebarsExtensions.WriteSafeString(object).

Fixes #660.

Test plan

  • dotnet test — full suite passes (1908/1908)
  • Existing Subexpression encoding/escaping issue #543 regression tests (HelperTests.SubexpressionWriteSafeStringNotDoubleEncoded, StandaloneWriteSafeStringNotEncoded) still pass — double-encoding fix is preserved
  • Added Issue660_SubexpressionResultIsPlainString — asserts a subexpression's captured result is typeof(string), not the wrapper type
  • Added Issue660_SubexpressionResultIsCastableToTypedStringParameter — mirrors the reporter's Append(string, string) helper, direct-casting a subexpression argument to string
  • Verified both new tests fail with InvalidCastException : Unable to cast object of type 'HandlebarsDotNet.SafeString' to type 'System.String' when the source fix is reverted, confirming they're a faithful regression guard

🤖 Generated with Claude Code

…lts (issue #660)

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<T>, 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<string>(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 <noreply@anthropic.com>
@sonarqubecloud

sonarqubecloud Bot commented Aug 7, 2026

Copy link
Copy Markdown

@rexm
rexm enabled auto-merge August 7, 2026 00:38
@rexm
rexm disabled auto-merge August 7, 2026 00:38
@rexm
rexm enabled auto-merge August 7, 2026 00:38
@rexm
rexm merged commit 08efad6 into master Aug 7, 2026
7 checks passed
@rexm
rexm deleted the fix/issue-660-safestring-leak branch August 7, 2026 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SafeString ?

1 participant