Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

valorin
Copy link
Contributor

@valorin valorin commented Mar 14, 2024

Since it missed 11 in #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?

@driesvints
Copy link
Member

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

Think so yeah

@taylorotwell taylorotwell marked this pull request as draft March 14, 2024 14:18
@driesvints driesvints marked this pull request as ready for review April 11, 2024 07:26
@driesvints
Copy link
Member

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

@taylorotwell
Copy link
Member

What would the upgrade guide need to contain?

@taylorotwell taylorotwell marked this pull request as draft April 15, 2024 18:51
@valorin
Copy link
Contributor Author

valorin commented Apr 16, 2024

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.

@driesvints
Copy link
Member

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

@valorin valorin marked this pull request as ready for review April 16, 2024 11:51
@taylorotwell taylorotwell marked this pull request as draft April 16, 2024 17:45
@taylorotwell
Copy link
Member

Marking this as draft for a bit.

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.

None yet

3 participants