Repository navigation
Remove the e() helper override - #267
Merged
Merged
Conversation
The override dates from 2020 and exists for one reason: Laravel 5.6 changed e()'s $doubleEncode default to true, to match htmlspecialchars(), and rather than pass false at the few call sites that were double-encoding, the default was reverted globally. Its body is otherwise Laravel 5.x's, so carrying it means e() does not resolve a DeferringDisplayableValue, does not unwrap a BackedEnum - e($someEnum) raises "Object of class X could not be converted to string" where Laravel returns the backing value - and escapes with a default that matches neither Laravel nor PHP. It also makes the test suite disagree with the application. The composer binary requires vendor/autoload.php before the configured bootstrap, so PHPUnit gets Laravel's e() while the served app gets this one, and no test can observe the behaviour it is overriding. The two call sites that relied on the default are corrected in the Winter half.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 43 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
LukeTowers
added a commit
to wintercms/winter
that referenced
this pull request
Oct 23, 2026
Several backend partials interpolated a stored value straight into an attribute or an element while a sibling line in the same file encoded the same value, so the encoding of a rendered record depended on which branch of the partial happened to run. Each of those sinks now encodes where it emits: the TagList hidden inputs used for preview, read-only and disabled fields; the dropdown field's preview hidden input; the attachment name and the two attachment URLs the FileUpload widget decorates a record with; the import/export column sample heading; the switch filter scope's stored state; and the option value of the RichEditor page-links dialog, whose label keeps its deliberate indent markup while the page title it prefixes is encoded. Editor markup classes and paragraph formats are encoded where the editor reads them rather than constrained where they are saved, because the editor builds its dropdown markup by string concatenation and reading is the only place that also covers values already stored. A class name still reaches the editor as written — md:text-lg, w-1/2, [&>p]:mt-4 — since the browser decodes the attribute when the editor reads it back. A paragraph format tag is emitted as a literal element name, so it must still be a bare one and is dropped rather than rewritten if it is not. The two views that hand a whole stored document to the browser, the Mail branding preview and the event log's mail preview, now pass it as data instead of embedding it in page markup it could terminate, render it in a sandboxed frame, and, for Mail branding, strip tags from the compiled stylesheet on the cache-hit path as well as the cache-miss one. Two call sites stop escaping a value their widget already escaped. CodeEditor::prepareVars() has escaped with double_encode since 3524b17 (2015) and Form::select() escapes option labels and attributes itself, so the e() in each caller was a second layer — a no-op only because Storm's e() override defaulted $doubleEncode to false. Those two are the reason that override existed, and removing them is what lets it go. Encoding a value that is not valid UTF-8 answers with an empty string, which would silently drop it on the next save, so the TagList and dropdown preview inputs substitute the invalid bytes rather than calling e(). Otherwise: a backend.richeditor.getTypeInfo listener returning an already-encoded title now shows those entities literally and should return plain text, which no core page-link provider does; a switch filter scope renders as unset rather than raising when its stored value is not a single value; the Mail branding and event log previews no longer run script or submit forms, with Mail branding keeping allow-popups allow-popups-to-escape-sandbox because the sample message's action button is a target="_blank" link; the beautifier leaves {x-newline} and {x-tabulation} as text alongside its own tokens; and the Mail branding page no longer emits the mailPreviewTemplate script element. Saved data and attachment links are unchanged — browsers decode the entities before submitting. Tests: TagListEscapingTest, FormFieldEscapingTest, FileUploadEscapingTest, BackendMarkupEscapingTest, EditorSettingTest, MailBrandSettingTest, MailBrandSettingsTest, and the MailPreviewSandbox and ExceptionBeautifier jest cases. Each sink has a test that fails without its change, paired with an invalidation test that ordinary values still render and round-trip, including testATagThatIsNotValidUtf8StillPostsBack and testPageLinkWithATitleThatStringifiesStillRendersIt. The suite cannot cover one of these: PHPUnit resolves Laravel's e() while the application resolves Storm's, so the rich editor attributes were verified in a browser instead, along with every other sink here. Requires wintercms/storm#267, which removes the e() override. The two are mutually dependent — the editor settings encode-on-read in this PR needs e() to double-encode to survive the attribute, and the override's removal needs the two corrected call sites above. Merge and tag Storm first.
bennothommo
pushed a commit
to wintercms/wn-system-module
that referenced
this pull request
Oct 23, 2026
Several backend partials interpolated a stored value straight into an attribute or an element while a sibling line in the same file encoded the same value, so the encoding of a rendered record depended on which branch of the partial happened to run. Each of those sinks now encodes where it emits: the TagList hidden inputs used for preview, read-only and disabled fields; the dropdown field's preview hidden input; the attachment name and the two attachment URLs the FileUpload widget decorates a record with; the import/export column sample heading; the switch filter scope's stored state; and the option value of the RichEditor page-links dialog, whose label keeps its deliberate indent markup while the page title it prefixes is encoded.
Editor markup classes and paragraph formats are encoded where the editor reads them rather than constrained where they are saved, because the editor builds its dropdown markup by string concatenation and reading is the only place that also covers values already stored. A class name still reaches the editor as written — md:text-lg, w-1/2, [&>p]:mt-4 — since the browser decodes the attribute when the editor reads it back. A paragraph format tag is emitted as a literal element name, so it must still be a bare one and is dropped rather than rewritten if it is not. The two views that hand a whole stored document to the browser, the Mail branding preview and the event log's mail preview, now pass it as data instead of embedding it in page markup it could terminate, render it in a sandboxed frame, and, for Mail branding, strip tags from the compiled stylesheet on the cache-hit path as well as the cache-miss one.
Two call sites stop escaping a value their widget already escaped. CodeEditor::prepareVars() has escaped with double_encode since 3524b17 (2015) and Form::select() escapes option labels and attributes itself, so the e() in each caller was a second layer — a no-op only because Storm's e() override defaulted $doubleEncode to false. Those two are the reason that override existed, and removing them is what lets it go.
Encoding a value that is not valid UTF-8 answers with an empty string, which would silently drop it on the next save, so the TagList and dropdown preview inputs substitute the invalid bytes rather than calling e(). Otherwise: a backend.richeditor.getTypeInfo listener returning an already-encoded title now shows those entities literally and should return plain text, which no core page-link provider does; a switch filter scope renders as unset rather than raising when its stored value is not a single value; the Mail branding and event log previews no longer run script or submit forms, with Mail branding keeping allow-popups allow-popups-to-escape-sandbox because the sample message's action button is a target="_blank" link; the beautifier leaves {x-newline} and {x-tabulation} as text alongside its own tokens; and the Mail branding page no longer emits the mailPreviewTemplate script element. Saved data and attachment links are unchanged — browsers decode the entities before submitting.
Tests: TagListEscapingTest, FormFieldEscapingTest, FileUploadEscapingTest, BackendMarkupEscapingTest, EditorSettingTest, MailBrandSettingTest, MailBrandSettingsTest, and the MailPreviewSandbox and ExceptionBeautifier jest cases. Each sink has a test that fails without its change, paired with an invalidation test that ordinary values still render and round-trip, including testATagThatIsNotValidUtf8StillPostsBack and testPageLinkWithATitleThatStringifiesStillRendersIt. The suite cannot cover one of these: PHPUnit resolves Laravel's e() while the application resolves Storm's, so the rich editor attributes were verified in a browser instead, along with every other sink here.
Requires wintercms/storm#267, which removes the e() override. The two are mutually dependent — the editor settings encode-on-read in this PR needs e() to double-encode to survive the attribute, and the override's removal needs the two corrected call sites above. Merge and tag Storm first.
bennothommo
pushed a commit
to wintercms/wn-backend-module
that referenced
this pull request
Oct 23, 2026
Several backend partials interpolated a stored value straight into an attribute or an element while a sibling line in the same file encoded the same value, so the encoding of a rendered record depended on which branch of the partial happened to run. Each of those sinks now encodes where it emits: the TagList hidden inputs used for preview, read-only and disabled fields; the dropdown field's preview hidden input; the attachment name and the two attachment URLs the FileUpload widget decorates a record with; the import/export column sample heading; the switch filter scope's stored state; and the option value of the RichEditor page-links dialog, whose label keeps its deliberate indent markup while the page title it prefixes is encoded.
Editor markup classes and paragraph formats are encoded where the editor reads them rather than constrained where they are saved, because the editor builds its dropdown markup by string concatenation and reading is the only place that also covers values already stored. A class name still reaches the editor as written — md:text-lg, w-1/2, [&>p]:mt-4 — since the browser decodes the attribute when the editor reads it back. A paragraph format tag is emitted as a literal element name, so it must still be a bare one and is dropped rather than rewritten if it is not. The two views that hand a whole stored document to the browser, the Mail branding preview and the event log's mail preview, now pass it as data instead of embedding it in page markup it could terminate, render it in a sandboxed frame, and, for Mail branding, strip tags from the compiled stylesheet on the cache-hit path as well as the cache-miss one.
Two call sites stop escaping a value their widget already escaped. CodeEditor::prepareVars() has escaped with double_encode since 3524b17 (2015) and Form::select() escapes option labels and attributes itself, so the e() in each caller was a second layer — a no-op only because Storm's e() override defaulted $doubleEncode to false. Those two are the reason that override existed, and removing them is what lets it go.
Encoding a value that is not valid UTF-8 answers with an empty string, which would silently drop it on the next save, so the TagList and dropdown preview inputs substitute the invalid bytes rather than calling e(). Otherwise: a backend.richeditor.getTypeInfo listener returning an already-encoded title now shows those entities literally and should return plain text, which no core page-link provider does; a switch filter scope renders as unset rather than raising when its stored value is not a single value; the Mail branding and event log previews no longer run script or submit forms, with Mail branding keeping allow-popups allow-popups-to-escape-sandbox because the sample message's action button is a target="_blank" link; the beautifier leaves {x-newline} and {x-tabulation} as text alongside its own tokens; and the Mail branding page no longer emits the mailPreviewTemplate script element. Saved data and attachment links are unchanged — browsers decode the entities before submitting.
Tests: TagListEscapingTest, FormFieldEscapingTest, FileUploadEscapingTest, BackendMarkupEscapingTest, EditorSettingTest, MailBrandSettingTest, MailBrandSettingsTest, and the MailPreviewSandbox and ExceptionBeautifier jest cases. Each sink has a test that fails without its change, paired with an invalidation test that ordinary values still render and round-trip, including testATagThatIsNotValidUtf8StillPostsBack and testPageLinkWithATitleThatStringifiesStillRendersIt. The suite cannot cover one of these: PHPUnit resolves Laravel's e() while the application resolves Storm's, so the rich editor attributes were verified in a browser instead, along with every other sink here.
Requires wintercms/storm#267, which removes the e() override. The two are mutually dependent — the editor settings encode-on-read in this PR needs e() to double-encode to survive the attribute, and the override's removal needs the two corrected call sites above. Merge and tag Storm first.
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.
e()has been overridden in Storm since 2020 for a single reason: Laravel 5.6 changed the helper's$doubleEncodedefault fromfalsetotrue, aligning it withhtmlspecialchars()'s own default, and rather than passfalseat the handful of call sites that were double-encoding as a result, the default was reverted globally. That shim has outlived its purpose, and the call sites it was hiding are better fixed directly.Keeping it has three costs. The body is still Laravel 5.x's, so
e()here does not resolve aDeferringDisplayableValueand does not unwrap aBackedEnum—e($someEnum)raises Object of class X could not be converted to string where Laravel returns the backing value, which makes ordinary Laravel-style plugin code fail on Winter for a non-obvious reason. The escaping default matches neither Laravel nor PHP. And it makes the test suite disagree with the application: the composer binary requiresvendor/autoload.phpbefore the configured bootstrap, so PHPUnit resolves Laravel'se()while the served application resolves this one, and no test can observe the behaviour being overridden.Deleting it restores Laravel's implementation in both contexts.
bootstrap/autoload.php's priority require stays — it is still load-bearing fortrans()andcollect().Behaviour changes
e()now double-encodes by default, matching Laravel andhtmlspecialchars().e('Tom & Jerry')returnsTom & Jerryrather thanTom & Jerry.e()on a backed enum returns its value instead of raising.e()resolves aDeferringDisplayableValue.e(null)is unchanged (empty string).Upgrading
The case to check is a widget that escapes a value in PHP and escapes it again in its template. That pattern was a no-op before and now applies a second layer. For a display-only widget the result is visibly wrong text; for an editor widget whose value posts back and is saved, the corruption accumulates on every save, and no test will catch it.
To find it:
If a hit writes into a view variable, check whether the matching template also runs
e()over it. Drop one of the two — in core's case the template's, since the widget owns the escaping. Passinge($value, false)explicitly also restores the old behaviour at a single call site.Both affected sites in Winter core are corrected in the companion PR.
Companion PR
Requires wintercms/winter#1558, which carries the two corrected call sites. Merge and tag this first; the two are mutually dependent and neither is complete alone.