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

Introduce `render :plain` and `render :html`, make `render :body` as an alias to `render :text` #12374

Closed
sikachu opened this Issue Sep 26, 2013 · 35 comments

Comments

Projects
None yet
@sikachu
Member

sikachu commented Sep 26, 2013

This is related to #3234.

Per discussion, render :text misdirect people to think that it would render content with text/plain MIME type. However, render :text actually sets the response body directly, and inherits the default response MIME type, which is text/html.

In order to reduce confusion, we're introducing 3 more render format to render:

render html: '<strong>HTML String</strong>' # render with `text/html` MIME type

render plain: 'plain text' # render with `text/plain` MIME type

render body: 'raw body' # render raw content, does not set content type, inherits 
                        # default content type, which currently is `text/html`

We want to phrase out the usage of render :text, to reduce the confusion in the future. There's currently no plan for deprecating render :text.


Progress: here

  • adds render :body
  • adds render :plain
  • adds render :html
  • deprecates render :text

@ghost ghost assigned sikachu Sep 26, 2013

@mdesantis

This comment has been minimized.

Contributor

mdesantis commented Sep 27, 2013

As a Rails user I agree with you, for what it matters :)

@kylemacey

This comment has been minimized.

kylemacey commented Sep 27, 2013

+1

@tadast

This comment has been minimized.

Contributor

tadast commented Sep 29, 2013

The suggested change makes perfect sense to me. Although body might be confusing for some new users (html <body> v.s. response body), especially if using turbolinks. Maybe response_bodywould be a safer option?

@nfm

This comment has been minimized.

Contributor

nfm commented Nov 11, 2013

Massive +1 on this :)

@sikachu are you working on a PR for this? I'm happy to take a look at this if not.

@mbhnyc

This comment has been minimized.

Contributor

mbhnyc commented Nov 11, 2013

Agree with @tadast, body strikes me as asking for confusion, since the other names actually mirror the content type being applied, i would propose render no_content_type:

@sikachu

This comment has been minimized.

Member

sikachu commented Nov 11, 2013

I am working on this. Still need to finish it up, though.

On Sun, Nov 10, 2013 at 7:18 PM, Nicholas Firth-McCoy
notifications@github.com wrote:

Massive +1 on this :)

@sikachu are you working on a PR for this? I'm happy to take a look at this if not.

Reply to this email directly or view it on GitHub:
#12374 (comment)

@homakov

This comment has been minimized.

Contributor

homakov commented Nov 18, 2013

@sikachu everyone will just switch to using :body and vulnerable systems will stay vulnerable. What do you think about :text = text/plain and :html = text/html?

@sikachu

This comment has been minimized.

Member

sikachu commented Nov 18, 2013

@homakov I totally understand the concern. However, I don't think we can do a breaking change between 4.0 -> 4.1.

@homakov

This comment has been minimized.

Contributor

homakov commented Nov 18, 2013

@sikachu this is also true.. maybe start doing warnings like we did with $^ regexps before major release & change

@sikachu

This comment has been minimized.

Member

sikachu commented Nov 18, 2013

@homakov I can buy that. So, render :text will show deprecation warning in 4.x.

/cc @jeremy, @rafaelfranca wdyt?

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Nov 18, 2013

Seems a good plan. 👍

@sikachu

This comment has been minimized.

Member

sikachu commented Nov 20, 2013

@homakov just to be clear — all of them will still not introduce sanitization. Is that the behavior you looking for, or should render :html call #html_safe?

@homakov

This comment has been minimized.

Contributor

homakov commented Nov 20, 2013

@sikachu i don't see profit in sanitizing for text/plain, all browsers handle it properly..

i checked PR again and i think :plain/:html naming is more obvious (text/plain and text/html). Way to go

@homakov

This comment has been minimized.

Contributor

homakov commented Nov 20, 2013

on the other side, jQuery has html() and text() methods, where text() can't lead to XSS. So what will we do with original :text - that's the question..

@jeremy

This comment has been minimized.

Member

jeremy commented Nov 21, 2013

Good plan, as discussed. Why is this an Issue instead of a PR? :bowtie:

@sikachu

This comment has been minimized.

Member

sikachu commented Nov 21, 2013

Code incoming!!! Will convert this to PR afterward. 😄

@homakov the question wasn't about text/plain, it was about text/html. Per your comment it seems like it's a good idea to make sure that a string is html safe — be sanitized — before render it (just like when you render something from the view). Was that something you were thinking about?

@homakov

This comment has been minimized.

Contributor

homakov commented Nov 21, 2013

@sikachu nevermind about sanitization option, i think text/plain is much more elegant way to deal with XSS. Just do whatever you feel right - we are on the same page

@homakov

This comment has been minimized.

Contributor

homakov commented Nov 24, 2013

to not create another issue will propose here: removing :js responder. Using RJS style response (eval JS instead of using JSON) leads to some data leaking vulnerabilities and is considered obsolete. Let's deprecate this responder?

@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Nov 25, 2013

@homakov there are plenty of situations where using js.erb templates are perfectly acceptable and don't run the risk of leaking user data. If we start deprecating things that might be used incorrectly I don't think we'd have much of a framework left 😄

@homakov

This comment has been minimized.

Contributor

homakov commented Nov 27, 2013

@pixeltrix :D exactly. But this is sort of bad practise, why should it be built-in? And what use cases of it, which cannot be replaced with JSON, I wonder?

@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Nov 27, 2013

I was just using it the other day for a contact form that returned a rendered partial with errors or a success message - no user session data to leak. I suppose I could've written a Backbone/Ember/etc. view that implemented client side validation and then submitted a JSON request and handled a response but that seems like a lot of work compared to the 15 minutes it took using js.erb templates.

@mdesantis

This comment has been minimized.

Contributor

mdesantis commented Nov 27, 2013

In my development I deprecated the use of RJS, because I prefer to have all the JS in the assets and the controller serves the data to it, rather than have some JS spreaded in views.The former is much better for me, because I can organize better the JS.

The cases like @pixeltrix said (where you have to render some partials that JS must use, like f.e. when you add something to a list via AJAX) are easily managed with something like:

render json: { html: render_to_string(partial: 'some_partial') }

And the JS that pushes the HTML into the list is located inside the AJAX success callback, which is in turn located in the assets and so better manageable.

I strongly suggest to render :json instead of RJS, I did this choice one year ago and I have got only advantages.

@homakov

This comment has been minimized.

Contributor

homakov commented Nov 28, 2013

I completely agree with @mdesantis because while @pixeltrix usecase is innocent it seeds a plant in codebase, other devs can use it for other functionalities not taking into account security implications.

Security audit is my work and believe me, I find this vulnerability often enough to consider removing/deprecating this functionality at all.. Shall we ask other core members' opinions? (cc them if you don't mind)

@dmathieu

This comment has been minimized.

Contributor

dmathieu commented Nov 28, 2013

I think this discussion should happen in the rails core mailing list, not in an issue about something related to it.

@sikachu

This comment has been minimized.

Member

sikachu commented Nov 28, 2013

Yeah, let's move the js discussion into rails core mailing list, that way we can gather more opinion about this.

Patch is coming soon for this, stay tuned.

@mdesantis

This comment has been minimized.

Contributor

mdesantis commented Nov 28, 2013

Is there already a thread on RoR mailing list? If yes, can you please write the link? If no, can you please write the link when it will be created? I'm interested in following the discussion :-)

@chancancode

This comment has been minimized.

@mdesantis

This comment has been minimized.

Contributor

mdesantis commented Nov 29, 2013

@chancancode Thank you

@sikachu sikachu modified the milestones: 4.2.0, 4.1.0 Feb 14, 2014

@dhh dhh modified the milestone: 4.1.0 Feb 14, 2014

sikachu added a commit to sikachu/rails that referenced this issue Feb 18, 2014

Introduce `render :body` for render raw content
This is an option for sending a raw content back to browser. Note that
this rendering option will unset the default content type and does not
include "Content-Type" header back in the response.

You should only use this option if you are expecting the "Content-Type"
header to not be set. More information on "Content-Type" header can be
found on RFC 2616, section 7.2.1.

Please see rails#12374 for more detail.

sikachu added a commit to sikachu/rails that referenced this issue Feb 18, 2014

Introduce `render :plain` for render plain text
This is as an option to render content with a content type of
`text/plain`. This is the preferred option if you are planning to render
a plain text content.

Please see #12374 for more detail.

sikachu added a commit to sikachu/rails that referenced this issue Feb 18, 2014

Introduce `render :html` for render HTML string
This is an option for to HTML content with a content type of
`text/html`. This rendering option calls `ERB::Util.html_escape`
internally to escape unsafe HTML string, so you will have to mark your
string as html safe if you have any HTML tag in it.

Please see #12374 for more detail.
@sai43

This comment has been minimized.

sai43 commented Apr 15, 2015

Hey guys is it possible to render html like this

render html: {'some_path_to_partial'}

sikachu added a commit to sikachu/rails that referenced this issue Jul 17, 2015

Add deprecation warning for `render :text`
We've started on discouraging the usage of `render :text` in #12374.
This is a follow-up commit to make sure that we print out the
deprecation warning.

sikachu added a commit to sikachu/rails that referenced this issue Jul 17, 2015

Add deprecation warning for `render :text`
We've started on discouraging the usage of `render :text` in #12374.
This is a follow-up commit to make sure that we print out the
deprecation warning.

sikachu added a commit to sikachu/rails that referenced this issue Jul 17, 2015

Add deprecation warning for `render :text`
We've started on discouraging the usage of `render :text` in #12374.
This is a follow-up commit to make sure that we print out the
deprecation warning.

sikachu added a commit to sikachu/rails that referenced this issue Jul 18, 2015

Add deprecation warning for `render :text`
We've started on discouraging the usage of `render :text` in #12374.
This is a follow-up commit to make sure that we print out the
deprecation warning.

acoffman added a commit to griffithlab/dgi-db that referenced this issue Jun 29, 2017

JoeCohen added a commit to JoeCohen/mushroom-observer that referenced this issue Sep 1, 2017

Change deprecated `render :text` to `render :plain`
**Exception**: observation_controller/other.rb, where it's changed to `render(html`

**Question**: should others be `html` instead of `plain`?

See http://edgeguides.rubyonrails.org/upgrading_ruby_on_rails.html#rendering-content-from-string,
rails/rails#12374
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment