Improve BadRequest logic. Partially reverts 8f8df53, fixes sinatra#1211

mwpastore · Dec 2, 2016 · b4fdb72 · b4fdb72
1 parent 63e81bc
commit b4fdb72
Show file tree

Hide file tree

Showing 5 changed files with 68 additions and 69 deletions.
diff --git a/lib/sinatra/base.rb b/lib/sinatra/base.rb
@@ -603,11 +603,6 @@ def server_error?
       status.between? 500, 599
     end
 
-    # whether or not the status is set to 400
-    def bad_request?
-      status == 400
-    end
-
     # whether or not the status is set to 404
     def not_found?
       status == 404
@@ -1143,9 +1138,6 @@ def handle_exception!(boom)
       elsif not_found?
         headers['X-Cascade'] = 'pass' if settings.x_cascade?
         body '<h1>Not Found</h1>'
-      elsif bad_request?
-        dump_errors! boom if settings.dump_errors?
-        halt status
       end
 
       res = error_block!(boom.class, boom) || error_block!(status, boom)

diff --git a/lib/sinatra/show_exceptions.rb b/lib/sinatra/show_exceptions.rb
@@ -49,6 +49,10 @@ def call(env)
 
     private
 
+    def bad_request?(e)
+      Sinatra::BadRequest === e
+    end
+
     def prefers_plain_text?(env)
       !(Request.new(env).preferred_type("text/plain","text/html") == "text/html") &&
       [/curl/].index { |item| item =~ env["HTTP_USER_AGENT"] }
@@ -210,8 +214,10 @@ def frame_class(frame)
       <p><a href="#" id="expando"
             onclick="toggleBacktrace(); return false">(expand)</a></p>
       <p id="nav"><strong>JUMP TO:</strong>
-         <a href="#get-info">GET</a>
-         <a href="#post-info">POST</a>
+         <% unless bad_request?(exception) %>
+            <a href="#get-info">GET</a>
+            <a href="#post-info">POST</a>
+         <% end %>
          <a href="#cookie-info">COOKIES</a>
          <a href="#env-info">ENV</a>
       </p>
@@ -264,47 +270,49 @@ def frame_class(frame)
       </ul>
     </div> <!-- /BACKTRACE -->
 
-    <div id="get">
-      <h3 id="get-info">GET</h3>
-      <% if req.GET and not req.GET.empty? %>
-        <table class="req">
-          <tr>
-            <th>Variable</th>
-            <th>Value</th>
-          </tr>
-           <% req.GET.sort_by { |k, v| k.to_s }.each { |key, val| %>
-          <tr>
-            <td><%=h key %></td>
-            <td class="code"><div><%=h val.inspect %></div></td>
-          </tr>
-          <% } %>
-        </table>
-      <% else %>
-        <p class="no-data">No GET data.</p>
-      <% end %>
-      <div class="clear"></div>
-    </div> <!-- /GET -->
-
-    <div id="post">
-      <h3 id="post-info">POST</h3>
-      <% if req.POST and not req.POST.empty? %>
-        <table class="req">
-          <tr>
-            <th>Variable</th>
-            <th>Value</th>
-          </tr>
-          <% req.POST.sort_by { |k, v| k.to_s }.each { |key, val| %>
-          <tr>
-            <td><%=h key %></td>
-            <td class="code"><div><%=h val.inspect %></div></td>
-          </tr>
-          <% } %>
-        </table>
-      <% else %>
-        <p class="no-data">No POST data.</p>
-      <% end %>
-      <div class="clear"></div>
-    </div> <!-- /POST -->
+    <% unless bad_request?(exception) %>
+      <div id="get">
+        <h3 id="get-info">GET</h3>
+        <% if req.GET and not req.GET.empty? %>
+          <table class="req">
+            <tr>
+              <th>Variable</th>
+              <th>Value</th>
+            </tr>
+             <% req.GET.sort_by { |k, v| k.to_s }.each { |key, val| %>
+            <tr>
+              <td><%=h key %></td>
+              <td class="code"><div><%=h val.inspect %></div></td>
+            </tr>
+            <% } %>
+          </table>
+        <% else %>
+          <p class="no-data">No GET data.</p>
+        <% end %>
+        <div class="clear"></div>
+      </div> <!-- /GET -->
+
+      <div id="post">
+        <h3 id="post-info">POST</h3>
+        <% if req.POST and not req.POST.empty? %>
+          <table class="req">
+            <tr>
+              <th>Variable</th>
+              <th>Value</th>
+            </tr>
+            <% req.POST.sort_by { |k, v| k.to_s }.each { |key, val| %>
+            <tr>
+              <td><%=h key %></td>
+              <td class="code"><div><%=h val.inspect %></div></td>
+            </tr>
+            <% } %>
+          </table>
+        <% else %>
+          <p class="no-data">No POST data.</p>
+        <% end %>
+        <div class="clear"></div>
+      </div> <!-- /POST -->
+    <% end %>
 
     <div id="cookies">
       <h3 id="cookie-info">COOKIES</h3>

diff --git a/test/helpers_test.rb b/test/helpers_test.rb
@@ -26,23 +26,6 @@ def status_app(code, &block)
     end
   end
 
-  describe 'bad_request?' do
-    it 'is true for status == 400' do
-      status_app(400) { bad_request? }
-      assert_body 'true'
-    end
-
-    it 'is false for status gt 400' do
-      status_app(401) { bad_request? }
-      assert_body 'false'
-    end
-
-    it 'is false for status lt 400' do
-      status_app(399) { bad_request? }
-      assert_body 'false'
-    end
-  end
-
   describe 'not_found?' do
     it 'is true for status == 404' do
       status_app(404) { not_found? }

diff --git a/test/routing_test.rb b/test/routing_test.rb
@@ -54,7 +54,7 @@ class RoutingTest < Minitest::Test
 
     request = Rack::MockRequest.new(@app)
     response = request.request('GET', '/foo?bar=&bar[]=', {})
-    assert response.bad_request?
+    assert_equal 400, response.status
   end
 
   it "404s when no route satisfies the request" do

diff --git a/test/settings_test.rb b/test/settings_test.rb
@@ -247,6 +247,22 @@ def foo=(value)
       assert body.include?("<code>show_exceptions</code> setting")
     end
 
+    it 'does not attempt to show unparseable query parameters' do
+      klass = Sinatra.new(Sinatra::Application)
+      mock_app(klass) {
+        enable :show_exceptions
+
+        get '/' do
+          raise Sinatra::BadRequest
+        end
+      }
+
+      get '/'
+      assert_equal 400, status
+      refute body.include?('<div id="get">')
+      refute body.include?('<div id="post">')
+    end
+
     it 'does not override app-specified error handling when set to :after_handler' do
       ran = false
       mock_app do