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] Fix Blade @parent issue #16033

Merged
merged 2 commits into from
Oct 24, 2016
Merged

[5.4] Fix Blade @parent issue #16033

merged 2 commits into from
Oct 24, 2016

Conversation

themsaid
Copy link
Member

This is an attempt to fix #10068, currently if a variable was passed to the view that contains the string @parent, Blade tries to compile this string as a @parent directive causing appending the parent content into the view if a parent section exists, or if not the @parent string will be stripped from the response.

This PR attempts to fix this by replacing the actual @parent directive with a different placeholder, the change is breaking because any @parent that exists in the blade file will be replaced, so for example this echo {{ '@parent' }} won't work.

@GrahamCampbell GrahamCampbell changed the title [5.3] Fix Blade @parent issue [5.4] Fix Blade @parent issue Oct 21, 2016
@GrahamCampbell
Copy link
Member

But the new placeholder can still be abused though, right?

@themsaid
Copy link
Member Author

Yeah if you have a ##parent-placeholder## string it'll be a problem, but I don't think this string is common, unlike @parent.

@GrahamCampbell
Copy link
Member

It doesn't fix the potential security problem we have here were users can inject this string.

@taylorotwell
Copy link
Member

We've never actually found that this is a "security" problem, tbh. I do agree however that it would be nice if we could just totally eliminate the issue instead of changing to a different string. I've forgotten why that is somewhat difficult.

@themsaid
Copy link
Member Author

@taylorotwell well the point is that the replacement of @parent has to happen after the sections actually render, so if any of the dynamic content contains the string @parent it'll be replaced.

This PR #10122 was trying to fix using ob_get_length() but was reverted for some reason, I assume this approach won't work if you use @include inside a section thus the length of the content changes causing the replacement to break.

This PR replaces a common string @parent with a less common one ##parent-placeholder## and thus decreases the possibility of encountering the issue.

@vlakoff
Copy link
Contributor

vlakoff commented Oct 23, 2016

Maybe you could add some random suffix, e.g. '##parent-placeholder-'.Str::random(16).'##', this shouldn't be too hard to implement and we would be sure to be on the safe side.

@taylorotwell
Copy link
Member

The random token may work to further make it tougher to run into the issue and prevent people from easily guessing the token. Though I'm not sure there is really even a security issue.

@themsaid
Copy link
Member Author

I don't think it should be considered as a security issue, I consider it a limitation on using the string @parent which will be replaced with '' or the value of the parent section content if any.

*
* @return string
*/
protected function compileParent()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function doesn't seem to be used anywhere?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@taylorotwell taylorotwell merged commit b35d334 into laravel:master Oct 24, 2016
@vlakoff
Copy link
Contributor

vlakoff commented Oct 24, 2016

Refs subsequent 16f72a5 :)

@halaei
Copy link
Contributor

halaei commented Oct 25, 2016

To fit the issue, can you consider somehow putting @parent related placeholder outside the string so no one can dynamically inject it?

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.

5 participants