Sinatra leaks sensitive information from any exception with an http_status attribute

[garybernhardt]

This is fundamentally the same issue as #1204. However, I think I have a clearer explanation of what's happening, including an easily-reproduced example. I also think the bug is a serious security problem and want to make that clear.

First, Stripe's errors have an http_status attribute. This attribute represents the status code returned by Stripe's servers. For example, here you can see Stripe::InvalidRequestError taking an http_status argument, which is 400 when the error is actually raised.

  class InvalidRequestError < StripeError
    attr_accessor :param

    def initialize(message, param, http_status: nil, http_body: nil, json_body: nil,
                   http_headers: nil, code: nil)

In our application, we were seeing this error raised for correct reasons (i.e., we had an actual error). But Sinatra will treat any exception with an http_status attribute as a Sinatra-level error that should be translated into that HTTP status code.

    def handle_exception!(boom)
      [...]

      if boom.respond_to? :http_status
        status(boom.http_status)

Note that the Sinatra code above does not respond to any configuration option. As far as I can tell, this problem exists regardless of how Sinatra is configured.

By these two code snippets' powers combined:

1. Stripe's servers are 400ing because we did something wrong.
2. That 400 response exception's text contains sensitive Stripe identifiers, like Stripe token IDs.
3. Sinatra sees the http_status attribute and incorrectly thinks that this exception is actually a Sinatra exception.
4. Sinatra returns that status code and the response text to the user, including sensitive information from Stripe.

You can reproduce this yourself by installing the "stripe" and "sinatra" gems and running this single-file Sinatra app. Go to localhost:4567/error_test and you'll see the "sensitive information from stripe" string leaked to the end user.

#!/usr/bin/env ruby

require "sinatra"
require "stripe"

get "/error_test" do
  raise Stripe::InvalidRequestError.new(
    "sensitive information from stripe", 2, http_status: 400)
end

[mwpastore]

Add an error handler to your app to modify the response body.

error Stripe::InvalidRequestError do
  "boom"
end

[jkowens]

Looking at this, it seems the exception message will be printed to the body if it is http status 400 or 404. Still not good. See

sinatra/lib/sinatra/base.rb

Line 1132 in 26fa5c8

body boom_message || '<h1>Not Found</h1>'

.

I'm wondering why there isn't a Sinatra::Error class with internal errors extending from that. Seems like this is a case where checking type would be better than just checking if it quacks.

[garybernhardt]

@jkowens I also reprod it with 401 when I was diagnosing it. Didn't dig any deeper into the exact status codes affected, though.

[garybernhardt]

@mwpastore We'll work around it now that we know. But the problem is that I've never heard of this in ten years of using Sinatra. And we only discovered it because we happened to see an unhandled Stripe::InvalidRequestError during development and tracked it down. We could've just as easily found out because it happened in production, leaking sensitive billing data to the client. Like @jkowens, I wonder why this conditional isn't checking for a specific error class, rather than merely checking for the existence of a method.

[jkowens]

It looks like Sinatra has tried to set the http_status of an error as the response status back to version 1. The difference in 2.0 is that if http_status returns 404 or 400, the message of the error is sent as the body of the response by default. Clearly something that should only be reserved for internal Sinatra errors.

[garybernhardt]

Edited: Oops, this comment was suggesting what @jkowens' PR #1519 already does. Looks like we're on the same page. Thanks, @jkowens!

[namusyaka]

This is an API design issue, not a bug.
Sinatra provides several features by duck-typing according to the Ruby style.
Thus, I don't intend to take a patch from @jkowens right away (but thanks for working on this issue).

By reporting this issue, I could recognize the problem on the API design.
Indeed, we didn't mention it in our document explicitly.

At least, we can take a following action:

• Towards 3.0.0, we can discuss the best API design.
• Mention the http_status method and its duck-typing feature in our document (probably README?).

[garybernhardt]

I don't think that mentioning http_status in the docs solves the problem. Every Sinatra user would have to manually check for an http_status method in every exception class that can be raised by any library in the system.

[namusyaka]

@garybernhardt I don't say that mentioning in the document will solve this problem. I said that it's important to clarify the matter until the problem is solved in the future.
Please don't misunderstand

[garybernhardt]

@namusyaka Got it, thanks for clarifying and sorry for misunderstanding!

[SwagDevOps]

Interesting issue 👍

[jkowens]

Closed by #1519. Will be released in v3.0.