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

[5.4] Escape default value of yield blade directive #19643

Merged
merged 1 commit into from
Jun 16, 2017

Conversation

ockle
Copy link
Contributor

@ockle ockle commented Jun 16, 2017

This is in the same vein as #17453.

@yield accepts a default value as the second parameter. However, Laravel does not escape the data passed in via this parameter, opening up the possibility of an XSS attack if the developer is not aware of this.

@taylorotwell taylorotwell merged commit a05e9e2 into laravel:5.4 Jun 16, 2017
mathieutu added a commit to mathieutu/framework that referenced this pull request Jun 19, 2017
* commit '05060b183dd09cee6dce498da5afde9e66fd1a29':
  Apply fixes from StyleCI (laravel#19659)
  [5.4] Add ability to remove a global scope with another global scope (laravel#19657)
  [5.4] Add fresh method on Eloquent\Collection. (laravel#19616)
  support multiple fields to validateDifferent (laravel#19637)
  escape default value of yield blade directive (laravel#19643)
  [5.4] Add "avg" and "average" to higher order proxy. (laravel#19628)
  tagged v5.4.27 release notes
  version
  Add missing dependency. (laravel#19623)
  update timestamp on soft delete only when its used (laravel#19627)
  Add new `diffAssoc()` method to collection (laravel#19604)
  Fix tests code coverage config. (laravel#19609)

# Conflicts:
#	src/Illuminate/Database/Eloquent/Collection.php
@brogier
Copy link

brogier commented Jun 30, 2017

This broke multiple sites because we use @yield('main', view('_parts.main)) so we can overwrite the main which is located between the header and footer.

Any fix for this? Or can we disable this specific change?

@ockle
Copy link
Contributor Author

ockle commented Jun 30, 2017

@rogierverbrugge You would need to tell the renderer that the rendered view is safe:

@yield('main', new Illuminate\Support\HtmlString(view('_parts.main)))

@ghost
Copy link

ghost commented Jul 2, 2017

@rogierverbrugge, I had to deal with the same problem, but I completely understand the need for this modification. Any misuse with the previous implementation would easily make a website vulnerable to XSS attacks, although it wasn't the case for me as I believe it wouldn't be the case for you.

Anyways, we used something like this to solve it:

@section('main')
    @include('_parts.main')
@show

@browner12
Copy link
Contributor

is the simple solution just to add a 3rd parameter called $safe or something that defaults false and we can set to true to not escape the data?

@brogier
Copy link

brogier commented Jul 3, 2017

Or even better a simple check to check if the $default attribute matches the Illuminate\View\View::class instance.

A view doesn't need to be escaped in my opinion because the views inner {{ $variables }} are escaped.

Any thoughts?

@ockle
Copy link
Contributor Author

ockle commented Jul 3, 2017

I have opened a pull to change the behavior to that suggested by @rogierverbrugge #19884

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.

4 participants