ruby-grape/grape

NoMethodError for nil:NilClass on headers.reverse_merge after upgrading Grape from 1.6.2 to 1.8.0

alekgosk opened this issue · 6 comments

alekgosk commented

Environment

  • Ruby version: 3.1.6
  • Grape version: 1.8.0 (upgraded from 1.6.2)
  • OS: MacOS Sonoma 14.5

Description

After upgrading Grape from version 1.6.2 to 1.8.0, we started encountering a NoMethodError for nil:NilClass when calling reverse_merge on the headers variable. This issue occurs in our custom middleware where we manipulate the headers.

Steps to Reproduce

  1. Upgrade Grape from version 1.6.2 to 1.8.0.
  2. Execute a request that goes through the custom middleware which attempts to call reverse_merge on the headers variable.

Expected Behavior

The headers variable should be able to merge with another hash without throwing an error, as it worked in Grape version 1.6.2.

Actual Behavior

The application throws a NoMethodError indicating that reverse_merge is called on nil:NilClass. This suggests that the headers variable is nil at the time of the call.

Error Log

  1) Blinkist::Mobile::V4::Oauth2Api POST /oauth2/token when HTTP Signature is present and body is malformed returns bad request
     Failure/Error: post url, params

     NoMethodError:
       undefined method `reverse_merge' for nil:NilClass

               headers = headers.reverse_merge(Grape::Http::Headers::CONTENT_TYPE => content_type)
                                ^^^^^^^^^^^^^^
     # /usr/local/bundle/gems/grape-1.8.0/lib/grape/middleware/error.rb:54:in `error!'
     # ./api/shared/basic_api_trait.rb:93:in `block (2 levels) in <module:BasicApiTrait>'
     # /usr/local/bundle/gems/grape-1.8.0/lib/grape/middleware/error.rb:129:in `instance_exec'
     # /usr/local/bundle/gems/grape-1.8.0/lib/grape/middleware/error.rb:129:in `run_rescue_handler'
     # /usr/local/bundle/gems/grape-1.8.0/lib/grape/middleware/error.rb:49:in `rescue in call!'
     # /usr/local/bundle/gems/grape-1.8.0/lib/grape/middleware/error.rb:37:in `call!'
     # /usr/local/bundle/gems/grape-1.8.0/lib/grape/middleware/base.rb:29:in `call'
     # /usr/local/bundle/gems/rack-2.2.9/lib/rack/head.rb:12:in `call'
     # /usr/local/bundle/gems/grape-1.8.0/lib/grape/endpoint.rb:224:in `call!'
     # /usr/local/bundle/gems/grape-1.8.0/lib/grape/endpoint.rb:218:in `call'
     # /usr/local/bundle/gems/grape-1.8.0/lib/grape/router/route.rb:58:in `exec'
     # /usr/local/bundle/gems/grape-1.8.0/lib/grape/router.rb:120:in `process_route'
     # /usr/local/bundle/gems/grape-1.8.0/lib/grape/router.rb:74:in `block in identity'
     # /usr/local/bundle/gems/grape-1.8.0/lib/grape/router.rb:94:in `transaction'
     # /usr/local/bundle/gems/grape-1.8.0/lib/grape/router.rb:72:in `identity'
     # /usr/local/bundle/gems/grape-1.8.0/lib/grape/router.rb:56:in `block in call'
     # /usr/local/bundle/gems/grape-1.8.0/lib/grape/router.rb:136:in `with_optimization'
     # /usr/local/bundle/gems/grape-1.8.0/lib/grape/router.rb:55:in `call'
     # /usr/local/bundle/gems/grape-1.8.0/lib/grape/api/instance.rb:165:in `call'
     # /usr/local/bundle/gems/grape-1.8.0/lib/grape/api/instance.rb:70:in `call!'
     # /usr/local/bundle/gems/grape-1.8.0/lib/grape/api/instance.rb:65:in `call'
     # /usr/local/bundle/gems/grape-1.8.0/lib/grape/api.rb:81:in `call'
     # /usr/local/bundle/gems/rack-test-2.1.0/lib/rack/test.rb:360:in `process_request'
     # /usr/local/bundle/gems/rack-test-2.1.0/lib/rack/test.rb:163:in `custom_request'
     # /usr/local/bundle/gems/rack-test-2.1.0/lib/rack/test.rb:112:in `post'
     # ./spec/api/v4/api_oauth2_spec.rb:167:in `block (5 levels) in <top (required)>'
     # /usr/local/bundle/gems/webmock-3.23.1/lib/webmock/rspec.rb:39:in `block (2 levels) in <top (required)>'
     # ------------------
     # --- Caused by: ---
     # JSON::ParserError:
     #   Empty input (after ) at line 1, column 1 [parse.c:1062]
     #   /usr/local/bundle/gems/multi_json-1.15.0/lib/multi_json/adapters/oj.rb:34:in `load'

Rspec test that is failing

describe "POST /oauth2/token" do
  let(:trusted_user) { create(:user_blinkist) }
  let(:url) { "oauth2/token" }
  let!(:client) { create(:client, :secret => "abcde", :identifier => "12345") }
  let(:client_id) { client.identifier }
  let(:grant_type) { "client_credentials" }

  context "when HTTP Signature is present" do
    let(:client_secret) { client.secret_secret }
    let(:date) { Time.now.rfc2822 }

    let(:params) do
      {
        grant_type: grant_type,
        client_id: client_id,
      }.to_json
    end

    let(:signature) do
      data = JSON.parse(params).merge!({ "date" => date })
      digest = OpenSSL::Digest.new("sha256")
      OpenSSL::HMAC.hexdigest(digest, client_secret, data.to_json)
    end

    before do
      header "Authorization", "Signature #{signature}"
      header "Date", date
    end

    context "and body is malformed" do
      let(:signature) { "some signature" }
       let(:params) { { grant_type: grant_type, } }

      it "returns bad request" do
        post url, params
        expect(r_status).to be 400
      end
    end
  end
end
dblock commented

I would start by writing a spec in Grape 1.6 to reproduce this and bisect to a change between 1.6 and 1.8.

There has also been quite a few rack-related changes since 1.8. We can see if that test passes now and commit the test to ensure it doesn't break in the future.

ericproulx commented

@alekgosk headers is {} unless you pass nil.

grape/lib/grape/middleware/error.rb

Line 53 in a6cde5d

def error!(message, status = options[:default_status], headers = {}, backtrace = [], original_exception = nil)
Any chance that you call error! where the 3rd parameter is nil ?

alekgosk commented

Good catch @ericproulx, the culrpit is currently the following code:

# this code comes from a trait

rescue_from :grape_exceptions do |e|           
   # e.headers is nil here when request body is malformed
   error! e.message, e.status, e.headers
end
ericproulx commented

#2342 introduced the block execution of a rescue_from :grape_exceptions and It's been released in 1.8.0. So in 1.6.2, your block has never been executed. I would suggest to just remove the block and let grape handle it for you. FYI, It will works from 2.1.0

dblock commented

Thanks for digging this up @ericproulx! @alekgosk close?

alekgosk commented

Yup, I think we can close it, thanks @dblock