framework icon indicating copy to clipboard operation
framework copied to clipboard

[12.x] Switch Js::encode() to return HtmlString

Open valorin opened this issue 11 months ago • 7 comments

Since it missed 11 in https://github.com/laravel/framework/pull/49641, let's try again and get it into 12. 🤞

Updating the Js::encode() helper to return an instance of HtmlString rather than a raw string.

The primary reason is to support using the normal escaping Blade tags: {{ Js::encode() }}, rather than {!! Js::encode() !}}, when using this in Blade. This will then match the behaviour already possible with Js::from() which returns an instance of Htmlable and supports {{ Js::from() }}.

Since this is a breaking change, it will need to go into ~v11~ v12 - hence the master branch.

There are no downsides to this change, it may just require minor code updates - which should be trivial to identify and perform with a search for Js::encode(.

P.s. Is this the first PR into 12?

valorin avatar Mar 14 '24 12:03 valorin

P.s. Is this the first PR into 12?

Think so yeah

driesvints avatar Mar 14 '24 13:03 driesvints

@taylorotwell gonna mark this ready for you again if that's okay.

driesvints avatar Apr 11 '24 07:04 driesvints

What would the upgrade guide need to contain?

taylorotwell avatar Apr 12 '24 13:04 taylorotwell

The upgrade guide will need to advise that Js::encode() no longer returns a raw string, but instead an instance of Htmlable.

I can see three scenarios that will be affected:

  1. All uses of {{ Js::encode() }} will no longer be escaped. There is a chance this could introduce an XSS vector if the JSON encoding doesn't break HTML structures within it's values. It may also introduce vulns or just break, if the app is unescaping the output - as it would no longer be escaped.
  2. Uses of {!! Js::encode() !!} will continue to work as expected, although devs can update to {{ Js::encode() }} and it will continue to work the same.
  3. Code that relies on Js::encode() returning specifically a string value will break. They will need to toString or typecast the output.

valorin avatar Apr 16 '24 00:04 valorin

@valorin if you need a new review, don't forget to mark the PR as ready.

driesvints avatar Apr 16 '24 07:04 driesvints

Marking this as draft for a bit.

taylorotwell avatar Apr 16 '24 17:04 taylorotwell

I wonder this change seems to be a big backward incompatibility.

Given:

$foo = ['bar' => 'baz'];
<div data-foo="{{ Js::encode($foo) }}">

Before:

<div data-foo="{&quot;bar&quot;:&quot;baz&quot;}">
<!-- div.dataset.foo returns a valid JSON string '{"bar":"baz"}' -->

After:

<div data-foo="{"bar":"baz"}">
<!-- div.dataset.foo returns '{' and bar":"baz"} will be a garbage -->

We should add a cast to escape correctly...

<div data-foo="{{ Js::encode($foo)->toString() }}">

Force to use {!! !!} is reasonable for me. Example:

<script type="application/ld+json">{!! Js::encode($ld) !!}</script>

Tietew avatar May 10 '24 03:05 Tietew