Changes default render behavior from file to template. #16888

Merged
merged 1 commit into from Sep 22, 2014

Conversation

Projects
None yet
6 participants
@jejacks0n
Contributor

jejacks0n commented Sep 12, 2014

Currently the default behavior of render "foo/bar" is to expand to render file: "foo/bar".

That means that the file being looked up is not restricted to the view paths, or the rails root path, and can lead to potentially vulnerable leaks from the filesystem.

This pull request changes the default behavior to render template: "foo/bar" instead of render file: "foo/bar", which is restricted to the view paths, as exposing a high level of access to the file system outside of rails views should potentially be explicitly opt-in and not the default.

This may result in a more secure system without having to know the low level underpinnings of the ActionView::Rendering implementation, and I'm submitting this to open a conversation about it. When I first came across this it appeared to be a security issue, and reached out to the security team before looking into it and realizing it was the behavior as designed.

@jejacks0n

This comment has been minimized.

Show comment
Hide comment
@jejacks0n

jejacks0n Sep 12, 2014

Contributor

I'll fix the specs, but want to have the conversation first, because it has some impacts outside of ActionView.

Contributor

jejacks0n commented Sep 12, 2014

I'll fix the specs, but want to have the conversation first, because it has some impacts outside of ActionView.

@sevenseacat

This comment has been minimized.

Show comment
Hide comment
@sevenseacat

sevenseacat Sep 12, 2014

Contributor

👍 to the idea

Contributor

sevenseacat commented Sep 12, 2014

👍 to the idea

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Sep 12, 2014

Contributor

👍. We have wanted to do this change since Rails 3 but we dropped the ball somewhere along the way. With Rails 5 coming, it seems to be the perfect time to get this in.

Contributor

josevalim commented Sep 12, 2014

👍. We have wanted to do this change since Rails 3 but we dropped the ball somewhere along the way. With Rails 5 coming, it seems to be the perfect time to get this in.

@jejacks0n

This comment has been minimized.

Show comment
Hide comment
@jejacks0n

jejacks0n Sep 12, 2014

Contributor

I'll finish it up then. Thanks for the feedback.

Contributor

jejacks0n commented Sep 12, 2014

I'll finish it up then. Thanks for the feedback.

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Sep 15, 2014

Member

should this go through a deprecation cycle in 4.2?

Member

senny commented Sep 15, 2014

should this go through a deprecation cycle in 4.2?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 15, 2014

Member

If we want to deprecate we need to ask users to explicitly say :file or :template and I think this will lead a lot of changes, but I'm positive for this deprecation cycle (not that it is needed though).

Member

rafaelfranca commented Sep 15, 2014

If we want to deprecate we need to ask users to explicitly say :file or :template and I think this will lead a lot of changes, but I'm positive for this deprecation cycle (not that it is needed though).

@jejacks0n

This comment has been minimized.

Show comment
Hide comment
@jejacks0n

jejacks0n Sep 15, 2014

Contributor

@rafaelfranca, the change is only to change the default from :file to :template, not to remove the default entirely. I'd suspect (from searching github) that most people didn't realize that it defaults to :file, so the change may not effect everyone, if what they're rendering is within the view paths.

To clarify, most people who are using this are probably assuming it defaulted to template, and not to file, and so are unlikely to experience issues. It's likely only the cases where someone in fact knew that not supplying an options hash defaulted to :file, and then specifically chose not to use the hash syntax, which I think is a low number.

That being said, I'm happy to follow instructions, and just need guidance on what should happen where, and when.

Contributor

jejacks0n commented Sep 15, 2014

@rafaelfranca, the change is only to change the default from :file to :template, not to remove the default entirely. I'd suspect (from searching github) that most people didn't realize that it defaults to :file, so the change may not effect everyone, if what they're rendering is within the view paths.

To clarify, most people who are using this are probably assuming it defaulted to template, and not to file, and so are unlikely to experience issues. It's likely only the cases where someone in fact knew that not supplying an options hash defaulted to :file, and then specifically chose not to use the hash syntax, which I think is a low number.

That being said, I'm happy to follow instructions, and just need guidance on what should happen where, and when.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 15, 2014

Member

I know, I just pointing if we are going to add deprecation message the only way of people remove the message from their logs is changing the code to explicitly use :file or :template.

For example if we add deprecation message to:

render 'something'

And we show the message: render will change the behaviour from :file to :template.

What would the users will have to do to remove this deprecation warning?

Member

rafaelfranca commented Sep 15, 2014

I know, I just pointing if we are going to add deprecation message the only way of people remove the message from their logs is changing the code to explicitly use :file or :template.

For example if we add deprecation message to:

render 'something'

And we show the message: render will change the behaviour from :file to :template.

What would the users will have to do to remove this deprecation warning?

@jejacks0n

This comment has been minimized.

Show comment
Hide comment
@jejacks0n

jejacks0n Sep 15, 2014

Contributor

Ah, point taken. So, we'd need to verify that the file is within a views path, and if it is don't display the deprecation warning, else display it. Seems hard given what I understand of how that part of the system works currently.

Contributor

jejacks0n commented Sep 15, 2014

Ah, point taken. So, we'd need to verify that the file is within a views path, and if it is don't display the deprecation warning, else display it. Seems hard given what I understand of how that part of the system works currently.

@jejacks0n

This comment has been minimized.

Show comment
Hide comment
@jejacks0n

jejacks0n Sep 15, 2014

Contributor

Nevermind, I understand what you mean.

Contributor

jejacks0n commented Sep 15, 2014

Nevermind, I understand what you mean.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 15, 2014

Member

Yeah, and maybe not worth to show any deprecation message. Since we are going to apply this change in a major bump and, as you said, users will probably not notice something changed I'd go without deprecation and with a note in the upgrading guides.

Member

rafaelfranca commented Sep 15, 2014

Yeah, and maybe not worth to show any deprecation message. Since we are going to apply this change in a major bump and, as you said, users will probably not notice something changed I'd go without deprecation and with a note in the upgrading guides.

@chancancode chancancode merged commit 428722b into rails:master Sep 22, 2014

1 check failed

continuous-integration/travis-ci The Travis CI build failed
Details

chancancode added a commit that referenced this pull request Sep 22, 2014

Merge pull request #16888 from jejacks0n/render_template
Changes default render behavior from file to template.
@jejacks0n

This comment has been minimized.

Show comment
Hide comment
@jejacks0n

jejacks0n Sep 22, 2014

Contributor

Oh, wow. I was going to fix specs from the other gems first. As long as that's known, awesome. :)

Contributor

jejacks0n commented Sep 22, 2014

Oh, wow. I was going to fix specs from the other gems first. As long as that's known, awesome. :)

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Sep 22, 2014

Member

@jejacks0n Sorry, no, not known. 😅 I didn't know we have open action items on this one, what gems were you referring to?

Member

chancancode commented Sep 22, 2014

@jejacks0n Sorry, no, not known. 😅 I didn't know we have open action items on this one, what gems were you referring to?

@jejacks0n

This comment has been minimized.

Show comment
Hide comment
@jejacks0n

jejacks0n Sep 22, 2014

Contributor

ActionController and a few others had failing tests. I only adjusted the ActionView tests as a proof, but it apparently has usage outside of that in other tests.

Contributor

jejacks0n commented Sep 22, 2014

ActionController and a few others had failing tests. I only adjusted the ActionView tests as a proof, but it apparently has usage outside of that in other tests.

@jejacks0n

This comment has been minimized.

Show comment
Hide comment
@jejacks0n

jejacks0n Sep 22, 2014

Contributor

I assume any call to render "foo/bar" in the tests would be affected but haven't had time to spend on them. For me each one requires considerable effort to understand the implications and then resolve.

Contributor

jejacks0n commented Sep 22, 2014

I assume any call to render "foo/bar" in the tests would be affected but haven't had time to spend on them. For me each one requires considerable effort to understand the implications and then resolve.

chancancode added a commit that referenced this pull request Sep 22, 2014

Revert "Merge pull request #16888 from jejacks0n/render_template"
This reverts commit 07635a7, reversing
changes made to 1b5f61a.

Reason: it's not ready 💣, see #16888 (comment)
@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Sep 22, 2014

Member

Oops, I reverted. We should definitely should get those fixed 😄

Member

chancancode commented Sep 22, 2014

Oops, I reverted. We should definitely should get those fixed 😄

chancancode added a commit that referenced this pull request Sep 25, 2014

chancancode added a commit that referenced this pull request Sep 25, 2014

chancancode added a commit that referenced this pull request Sep 25, 2014

chancancode added a commit that referenced this pull request Sep 25, 2014

murtuzakz added a commit to murtuzakz/rails that referenced this pull request Oct 3, 2014

Revert "Merge pull request #16888 from jejacks0n/render_template"
This reverts commit 07635a7, reversing
changes made to 1b5f61a.

Reason: it's not ready 💣, see rails#16888 (comment)

murtuzakz added a commit to murtuzakz/rails that referenced this pull request Oct 3, 2014

murtuzakz added a commit to murtuzakz/rails that referenced this pull request Oct 3, 2014

@jejacks0n

This comment has been minimized.

Show comment
Hide comment
@jejacks0n

jejacks0n Nov 19, 2014

Contributor

@chancancode what's the status of this? Is it in, or was it reverted? In either case, thanks for fielding this, I was pulled away on work stuff.

Contributor

jejacks0n commented Nov 19, 2014

@chancancode what's the status of this? Is it in, or was it reverted? In either case, thanks for fielding this, I was pulled away on work stuff.

sachin004 added a commit to sachin004/rails that referenced this pull request Dec 13, 2014

Revert "Merge pull request #16888 from jejacks0n/render_template"
This reverts commit 07635a7, reversing
changes made to 1b5f61a.

Reason: it's not ready 💣, see rails#16888 (comment)

sachin004 added a commit to sachin004/rails that referenced this pull request Dec 13, 2014

sachin004 added a commit to sachin004/rails that referenced this pull request Dec 13, 2014

@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015

eileencodes added a commit that referenced this pull request Jan 28, 2016

Revert "Revert "Merge pull request #16888 from jejacks0n/render_templ…
…ate""

This reverts commit 585e756.

Conflicts:
	actionview/CHANGELOG.md
	guides/source/4_2_release_notes.md

eileencodes added a commit that referenced this pull request Jan 28, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment