sinatra / rack-protection

NOTE: This project has been merged upstream to sinatra/sinatra
https://github.com/sinatra/sinatra/tree/master/rack-protection
818 stars 58 forks source link

undefined method `[]' for nil:NilClass #103

Closed beanieboi closed 8 years ago

beanieboi commented 8 years ago

hey!

i currently debugging a weird problem. our setup: we have a Rails application that uses fake_braintree (http://github.com/highfidelity/fake_braintree) for testing. during our tests, we hit the fake braintree server and expect some JSON responses. the fake braintree server runs Sinatra with rack-protection. We had 1 very flaky test, where we couldn't parse the JSON coming from fake braintree. after some debugging, it turned out that the fake braintree server was returning a 500 error message and not JSON.

thats why i'm here. i try to debug the exception but i have no glue so far, why this is happening.

the exception

        NoMethodError at /merchants/xxx/transparent_redirect_requests
        undefined method `[]' for nil:NilClass
          file:
            base.rb
          location: html?
          line:
             121

RACK_ENV

      Rack ENV
          Variable
          Value
           CONTENT_LENGTH
           1341
           CONTENT_TYPE
           application/x-www-form-urlencoded
           GATEWAY_INTERFACE
           CGI/1.1
           HTTP_ACCEPT
           text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
           HTTP_ACCEPT_ENCODING
           gzip
           HTTP_ACCEPT_LANGUAGE
           en-US,*
           HTTP_CONNECTION
           Keep-Alive
           HTTP_HOST
           localhost:39947
           HTTP_ORIGIN
           http://127.0.0.1:46124
           HTTP_REFERER
           http://127.0.0.1:46124/subscription
           HTTP_USER_AGENT
           Mozilla/5.0 (Unknown; Linux x86_64) AppleWebKit/534.34 (KHTML, like Gecko) PhantomJS/1.9.7 Safari/534.34
           HTTP_VERSION
           HTTP/1.1
           PATH_INFO
           /merchants/xxx/transparent_redirect_requests
           QUERY_STRING
           REMOTE_ADDR
           127.0.0.1
           REMOTE_HOST
           localhost
           REQUEST_METHOD
           POST
           REQUEST_PATH
           /merchants/xxx/transparent_redirect_requests
           REQUEST_URI
           http://localhost:39947/merchants/xxx/transparent_redirect_requests
           SCRIPT_NAME
           SERVER_NAME
           localhost
           SERVER_PORT
           39947
           SERVER_PROTOCOL
           HTTP/1.1
           SERVER_SOFTWARE
           WEBrick/1.3.1 (Ruby/2.2.3/2015-08-18)
           rack.errors
           #<Object:0x007f30c68f8e48>
           rack.hijack
           #<Proc:0x007f30a25ff670@/home/ubuntu/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/rack-1.6.4/lib/rack/handler/webrick.rb:76 (lambda)>
           rack.hijack?
           true
           rack.hijack_io
           nil
           rack.input
           #<StringIO:0x007f30a25ff788>
           rack.logger
           #<Logger:0x007f30a2585578 @progname=nil, @level=1, @default_formatter=#<Logger::Formatter:0x007f30a2585550 @datetime_format=nil>, @formatter=nil, @logdev=#<Logger::LogDevice:0x007f30a2585500 @shift_size=nil, @shift_age=nil, @filename=nil, @dev=#<IO:<STDERR>>, @mutex=#<Logger::LogDevice::LogDeviceMutex:0x007f30a25854d8 @mon_owner=nil, @mon_count=0, @mon_mutex=#<Mutex:0x007f30a2585488>>>>
           rack.multiprocess
           false
           rack.multithread
           true
           rack.request.cookie_hash
           {}
           rack.request.form_hash
           {"utf8"=>"✓", "customer"=>{"credit_card"=>{"number"=>"4111111111111111", "expiration_month"=>"12", "expiration_year"=>"2020", "cvv"=>"123"}}, "tr_data"=>"5bade8f19f4d2d6b7da81b74a60c1f902c60fe6e|api_version=4&customer%5Bcredit_card%5D%5Bbilling_address%5D%5Bcompany%5D=&customer%5Bcredit_card%5D%5Bbilling_address%5D%5Bcountry_code_alpha2%5D=AT&customer%5Bcredit_card%5D%5Bbilling_address%5D%5Bextended_address%5D=&customer%5Bcredit_card%5D%5Bbilling_address%5D%5Bfirst_name%5D=B%C3%A4r&customer%5Bcredit_card%5D%5Bbilling_address%5D%5Blast_name%5D=Jogi&customer%5Bcredit_card%5D%5Bbilling_address%5D%5Blocality%5D=&customer%5Bcredit_card%5D%5Bbilling_address%5D%5Bpostal_code%5D=&customer%5Bcredit_card%5D%5Bbilling_address%5D%5Bregion%5D=&customer%5Bcredit_card%5D%5Bbilling_address%5D%5Bstreet_address%5D=&customer%5Bcustom_fields%5D%5Bvat_number%5D=&customer%5Bfirst_name%5D=B%C3%A4r&customer%5Blast_name%5D=Jogi&kind=create_customer&public_key=xxx&redirect_url=http%3A%2F%2F127.0.0.1%3A46124%2Fsubscription%2Fconfirm%3Faccount_id%3D40&time=20151203161041"}
           rack.request.form_input
           #<StringIO:0x007f30a25ff788>
           rack.request.form_vars
           utf8=%E2%9C%93&customer%5Bcredit_card%5D%5Bnumber%5D=4111111111111111&customer%5Bcredit_card%5D%5Bexpiration_month%5D=12&customer%5Bcredit_card%5D%5Bexpiration_year%5D=2020&customer%5Bcredit_card%5D%5Bcvv%5D=123&tr_data=5bade8f19f4d2d6b7da81b74a60c1f902c60fe6e%7Capi_version%3D4%26customer%255Bcredit_card%255D%255Bbilling_address%255D%255Bcompany%255D%3D%26customer%255Bcredit_card%255D%255Bbilling_address%255D%255Bcountry_code_alpha2%255D%3DAT%26customer%255Bcredit_card%255D%255Bbilling_address%255D%255Bextended_address%255D%3D%26customer%255Bcredit_card%255D%255Bbilling_address%255D%255Bfirst_name%255D%3DB%25C3%25A4r%26customer%255Bcredit_card%255D%255Bbilling_address%255D%255Blast_name%255D%3DJogi%26customer%255Bcredit_card%255D%255Bbilling_address%255D%255Blocality%255D%3D%26customer%255Bcredit_card%255D%255Bbilling_address%255D%255Bpostal_code%255D%3D%26customer%255Bcredit_card%255D%255Bbilling_address%255D%255Bregion%255D%3D%26customer%255Bcredit_card%255D%255Bbilling_address%255D%255Bstreet_address%255D%3D%26customer%255Bcustom_fields%255D%255Bvat_number%255D%3D%26customer%255Bfirst_name%255D%3DB%25C3%25A4r%26customer%255Blast_name%255D%3DJogi%26kind%3Dcreate_customer%26public_key%3Dxxx%26redirect_url%3Dhttp%253A%252F%252F127.0.0.1%253A46124%252Fsubscription%252Fconfirm%253Faccount_id%253D40%26time%3D20151203161041
           rack.request.query_hash
           {}
           rack.request.query_string
           rack.run_once
           false
           rack.url_scheme
           http
           rack.version
           [1, 3]
           sinatra.accept
           [#<Sinatra::Request::AcceptEntry:0x007f30a2574f48 @entry="text/html", @type="text/html", @params={}, @q=1.0>, #<Sinatra::Request::AcceptEntry:0x007f30a2574c28 @entry="application/xhtml+xml", @type="application/xhtml+xml", @params={}, @q=1.0>, #<Sinatra::Request::AcceptEntry:0x007f30a2574a98 @entry="application/xml;q=0.9", @type="application/xml", @params={}, @q=0.9>, #<Sinatra::Request::AcceptEntry:0x007f30a25746d8 @entry="*/*;q=0.8", @type="*/*", @params={}, @q=0.8>]
           sinatra.commonlogger
           true
           sinatra.route
           POST /merchants/:merchant_id/transparent_redirect_requests

parsed headers used by the html? method

{"Content-Type"=>nil, "Location"=>"http://127.0.0.1:39984/subscription/confirm?account_id=151&http_status=200&id=74fca03a9874322aab985a218c184dfd&kind=create_customer&hash=387735b38bf516a7f1d46018ab6dde61fde6d2f5”}

it turns out that Content-Type is nil, although the RACK_ENV says that it is application/x-www-form-urlencoded

so something between handing the headers to rack-protection goes wrong and rack protection blows up when it's trying to figure out the content type.

do you have any hints where i could start debugging this?

would it be ok for to just add a nil check to html? so it doesn't blow up anymore and we can safely return false?

i'm happy to debug and prepare a PR, but would love to get a heads start from you girls and guys.

a gist with the complete dump: https://gist.github.com/beanieboi/9fe9892842820b8ec915

for reference: https://github.com/highfidelity/fake_braintree/issues/111

beanieboi commented 8 years ago

hey all!

a quick update on this. it looks like the fake_braintree server or some middleware that runs before rack-protection is not correctly setting the Content-Type header. it's not a bug on rack-protection since the rack spec says that header fields must be strings.

i'll keep digging, because thats a really weird bug :)

kytrinyx commented 8 years ago

Nice find! I'm going to close this since it seems like the bug isn't here, but if you change your mind just reopen it and we can talk :)

beanieboi commented 8 years ago

@kytrinyx oh yeah, thanks! i forgot to close :) i actually chased it down to the Bitbucket gem extending NilClass to respond to content_type

https://github.com/vongrippen/bitbucket/blob/master/lib/bitbucket_rest_api/result.rb#L21-L23 https://github.com/vongrippen/bitbucket/blob/master/lib/bitbucket_rest_api/response/helpers.rb#L8-L18

because of this, Sinatra enters the wrong branch of the if over here: https://github.com/sinatra/sinatra/blob/master/lib/sinatra/base.rb#L910-L914

body[0] is a NilClass and therefor does not set the content-type to HTML for a redirect.

we (read I :D) have to fix the Bitbucket gem to not patch NilClass.

cc https://github.com/highfidelity/fake_braintree/issues/111

kytrinyx commented 8 years ago

I love a good mystery, thanks for sharing :)