caxlsx / caxlsx_rails

A Rails plugin to provide templates for the axlsx gem
MIT License
735 stars 84 forks source link

Bug (potential security issue): XLSX content is included in body when `render nothing: true` is called #156

Closed edtjones closed 2 years ago

edtjones commented 2 years ago

Hi folks,

I'm generating an XLSX file in a controller which checks authorization and raises an error if the user isn't authorized to view the file.

The issue If you raise an error in the controller method and rescue it in ApplicationController, the content of the excel file is still Base64-encoded in the body when you render nothing: true. This could be a security risk - it would have been in my case.

Roughly, my setup is as follows (this is a small replication of the problem, not my actual code). My route has XLSX as the default format, and the view renders the XLSX as described in the README.

Example replication

# route to /items/:id
resources :items, only: [:show], defaults: {format: :xlsx}

# controller in which the error is raised
class ItemsController < ApplicationController
   def show
      @model_data = SomeModel.find(params[:id])
      raise SomeError, "you are not allowed to view this file"
   end
end

# rescuing the error in ApplicationController

class ApplicationController

#....

   rescue_from SomeError do |error|
     # THIS IS THE DANGEROUS BIT - render nothing: true here will return 
     # the XLSX binary, base64-encoded, inside the body of the response, 
     # with an HTTP 403 response code.
      render nothing: true, layout: 'error', status: 403
   end

#....

end

Workaround My solution is to instead call head status: :forbidden in the rescue block, which does what you would expect and includes no body.

I can't replicate this bug using other renderers, so I think the problem exists in the XLSX renderer. I did have a look through the source and couldn't immediately see the problem.

Hope this is useful.

Ed

noniq commented 2 years ago

Since Rails 5.1 render nothing: true is no longer supported (it was deprecated in Rails 5.0, see https://github.com/rails/rails/pull/20336 )

As such it does really “nothing” in the sense that it does not change (override) the content that will be rendered. Using head … is the correct way in current Rails versions.