When returning an HTML error, make sure it's safe (#1763)

* When calling into an API specifying a crafted format that is HTML,
the returned error renders the HTML back to the user, causing a potential XSS
issue.  For example:

http://example.com/api/endpoint?format=%3Cscript%3Ealert(document.cookie)%3C/script%3E

Renders as html:

The requested format '<script>alert(document.cookie)</script>' is not supported.

When an error generates html back to the user, make sure it's properly escaped.

Fixes issue #1762

* Add changelog entry

* Use a method that also works in rails3

* Add spec formatting for older rails/activesupport version

Author: ctennis

CHANGELOG.md

@@ -6,6 +6,8 @@
 
 #### Fixes
 
+
+* [#1762](https://github.com/ruby-grape/grape/pull/1763): Fix unsafe HTML rendering on errors - [@ctennis](https://github.com/ctennis).
 * [#1759](https://github.com/ruby-grape/grape/pull/1759): Update appraisal for rails_edge - [@zvkemp](https://github.com/zvkemp).
 * [#1758](https://github.com/ruby-grape/grape/pull/1758): Fix expanding load_path in gemspec - [@2maz](https://github.com/2maz).
 * Your contribution here.

lib/grape/middleware/error.rb

@@ -1,4 +1,5 @@
 require 'grape/middleware/base'
+require 'active_support/core_ext/string/output_safety'
 
 module Grape
   module Middleware
@@ -69,6 +70,9 @@ def error_response(error = {})
       end
 
       def rack_response(message, status = options[:default_status], headers = { Grape::Http::Headers::CONTENT_TYPE => content_type })
+        if headers[Grape::Http::Headers::CONTENT_TYPE] == TEXT_HTML
+          message = ERB::Util.html_escape(message)
+        end
         Rack::Response.new([message], status, headers).finish
       end
 

spec/grape/api_spec.rb

@@ -2142,7 +2142,11 @@ def self.call(message, _backtrace, _option, _env, _original_exception)
       end
       get '/excel.json'
       expect(last_response.status).to eq(406)
-      expect(last_response.body).to eq("The requested format 'txt' is not supported.")
+      if ActiveSupport::VERSION::MAJOR == 3
+        expect(last_response.body).to eq('The requested format 'txt' is not supported.')
+      else
+        expect(last_response.body).to eq('The requested format 'txt' is not supported.')
+      end
     end
   end
 
@@ -3524,7 +3528,27 @@ def before
       end
       get '/something'
       expect(last_response.status).to eq(406)
-      expect(last_response.body).to eq("{\"error\":\"The requested format 'txt' is not supported.\"}")
+      if ActiveSupport::VERSION::MAJOR == 3
+        expect(last_response.body).to eq('{"error":"The requested format 'txt' is not supported."}')
+      else
+        expect(last_response.body).to eq('{"error":"The requested format 'txt' is not supported."}')
+      end
+    end
+  end
+
+  context 'with unsafe HTML format specified' do
+    it 'escapes the HTML' do
+      subject.content_type :json, 'application/json'
+      subject.get '/something' do
+        'foo'
+      end
+      get '/something?format=<script>blah</script>'
+      expect(last_response.status).to eq(406)
+      if ActiveSupport::VERSION::MAJOR == 3
+        expect(last_response.body).to eq('The requested format &#x27;&lt;script&gt;blah&lt;/script&gt;&#x27; is not supported.')
+      else
+        expect(last_response.body).to eq('The requested format &#39;&lt;script&gt;blah&lt;/script&gt;&#39; is not supported.')
+      end
     end
   end
 

spec/grape/middleware/exception_spec.rb

@@ -192,7 +192,7 @@ def app
     end
     it 'is possible to return errors in jsonapi format' do
       get '/'
-      expect(last_response.body).to eq('{"error":"rain!"}')
+      expect(last_response.body).to eq('{"error":"rain!"}')
     end
   end
 
@@ -207,8 +207,8 @@ def app
 
     it 'is possible to return hash errors in jsonapi format' do
       get '/'
-      expect(['{"error":"rain!","detail":"missing widget"}',
-              '{"detail":"missing widget","error":"rain!"}']).to include(last_response.body)
+      expect(['{"error":"rain!","detail":"missing widget"}',
+              '{"detail":"missing widget","error":"rain!"}']).to include(last_response.body)
     end
   end
 
@@ -258,7 +258,7 @@ def app
     end
     it 'is possible to specify a custom formatter' do
       get '/'
-      expect(last_response.body).to eq('{:custom_formatter=>"rain!"}')
+      expect(last_response.body).to eq('{:custom_formatter=>"rain!"}')
     end
   end