RFC: Introduce Template::File#35688
Merged
tenderlove merged 1 commit intorails:masterfrom Mar 30, 2019
Merged
Conversation
The previous behaviour of render file: was essentially the same as render template:, except that templates can be specified as an absolute path on the filesystem. This makes sense for historic reasons, but now render file: is almost exclusively used to render raw files (not .erb) like public/404.html. In addition to complicating the code in template/resolver.rb, I think the current behaviour is surprising to developers. This commit deprecates the existing "lookup a template from anywhere" behaviour and replaces it with "render this file exactly as it is on disk". Handlers will no longer be used (it will render the same as if the :raw handler was used), but formats (.html, .xml, etc) will still be detected (and will default to :plain). The existing render file: behaviour was the path through which Rails apps were vulnerable in the recent CVE-2019-5418. Although the vulnerability has been patched in a fully backwards-compatible way, I think it's a strong hint that we should drop the existing previously-vulnerable behaviour if it isn't a benefit to developers.
c43b37a to
c7820d8
Compare
3 tasks
| elsif options.key?(:file) | ||
| @lookup_context.with_fallbacks.find_file(options[:file], nil, false, keys, @details) | ||
| if File.exist?(options[:file]) | ||
| Template::File.new(options[:file]) |
Member
There was a problem hiding this comment.
Would not this render any file in the filesystem? Would that not add an attack vector on render file: params[:file]?
Member
Author
There was a problem hiding this comment.
That's already an attack vector. render file: is the unsafe version of render template:
This has me thinking, though. Maybe we could just fully deprecate render file:. As far as I can tell it's basically only used to render public/404.html and similar pages. Those uses could probably be replaced with either:
send_file("#{Rails.root}/public/404.html", status: 404)orrender html: File.read("#{Rails.root}/public/404.html".html_safe, status: 404)
Member
There was a problem hiding this comment.
Yeah. I agree about deprecating it. Or we can change render file to only render things relative to the rails root. It is still an attack vector though since you can render the secrets file if you have it open.
This was referenced Mar 29, 2019
tegon
added a commit
to heartcombo/devise
that referenced
this pull request
Aug 6, 2019
Render file will need the full path in order to avoid security breaches. In this particular case, there's no need to use render file, it's ok to use render template. Ref: rails/rails#35688
tegon
added a commit
to heartcombo/devise
that referenced
this pull request
Aug 6, 2019
Render file will need the full path in order to avoid security breaches. In this particular case, there's no need to use render file, it's ok to use render template. Ref: rails/rails#35688
tegon
added a commit
to heartcombo/devise
that referenced
this pull request
Aug 7, 2019
* Fix specs on Rails 6 RC2 `ActiveRecord::MigrationContext` now has a `schema_migration` attribute. Ref: https://github.com/rails/rails/pull/36439/files#diff-8d3c44120f7b67ff79e2fbe6a40d0ad6R1018 * Use `media_type` instead of `content_type` Before Rails 6 RC2, the `ActionDispatch::Response#content_type` method would return only the media part of the `Content-Type` header, without any other parts. Now the `#content_type` method returns the entire header - as it is - and `#media_type` should be used instead to get the previous behavior. Ref: - rails/rails#36034 - rails/rails#36854 * Use render template instead of render file Render file will need the full path in order to avoid security breaches. In this particular case, there's no need to use render file, it's ok to use render template. Ref: rails/rails#35688 * Don't set `represent_boolean_as_integer` on Rails 6 * Update comments [ci skip]
eileencodes
added a commit
to eileencodes/rails-controller-testing
that referenced
this pull request
Apr 16, 2020
rails/rails#35688 deprecated calling `render file` and letting it lookup the file from anywhere. This was deprecated in 6.0 and removed in 6.1, which is why travis was failing for new PRs.
eileencodes
added a commit
to eileencodes/rails-controller-testing
that referenced
this pull request
May 4, 2020
rails/rails#35688 deprecated calling `render file` and letting it lookup the file from anywhere. This was deprecated in 6.0 and removed in 6.1, which is why travis was failing for new PRs.
eileencodes
added a commit
to eileencodes/rails-controller-testing
that referenced
this pull request
May 4, 2020
These tests are now failing because of changes made in rails/rails#35688. This causes the instrumentation for `@_files` to not get run, but also don't think this should be supported since that instrumentation is for templates and now files are just rendered as files not as templates.
eileencodes
added a commit
to eileencodes/rails-controller-testing
that referenced
this pull request
May 4, 2020
These tests are now failing because of changes made in rails/rails#35688. This causes the instrumentation for `@_files` to not get run, but also don't think this should be supported since that instrumentation is for templates and now files are just rendered as files not as templates.
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
From some discussion with @tenderlove
The previous behaviour of
render file:was essentially the same asrender template:, except that templates can be specified as an absolute path on the filesystem.This makes sense for historic reasons, but now
render file:is almost exclusively used to render raw files (not.erb) likepublic/404.html. In addition to complicating the code intemplate/resolver.rb, I think the current behaviour is surprising to developers.This commit deprecates the existing "lookup a template from anywhere" behaviour and replaces it with "render this file exactly as it is on disk". Handlers will no longer be used (it will render the same as if the
:rawhandler was used), but formats (.html,.xml, etc) will still be detected (and will default to:plain).The existing
render file:behaviour was the path through which Rails apps were vulnerable in the recent CVE-2019-5418. Although the vulnerability has been patched in a fully backwards-compatible way, I think it's a strong hint that we should drop the existing previously-vulnerable behaviour if it isn't a benefit to developers.NB: This PR doesn't yet include the test changes, in hopes of discussing the change first. For historic reasons we testThis ready to review! (and merge? 🤞)render file:in Rails way more than we should (it was the old default) when we really should be testingrender template:.