search

ruby-grape / grape

An opinionated framework for creating REST-like APIs in Ruby.
http://www.ruby-grape.org
MIT License
9.88k stars 1.22k forks source link

Customized Errors No Longer Including CORS Related Headers Upgrading from 1.0 to 1.3 #2078

Open jimjeffers opened 4 years ago

jimjeffers commented 4 years ago

Steps to reproduce

What we need to do to see your problem or bug?

I've upgraded Grape from 1.0 to 1.3.3 and can no longer add Access-Control-Allow-Origin and Access-Control-Request-Method to the errors thrown by grape. I am using rescue_from :all to generate the desired status code for errors thrown by active record validations and other causes via this method as well.

    rescue_from :all do |error|
      error! error.message, FilmCal::Base.status_code_for(error, error.class.to_s), {
        'Access-Control-Allow-Origin' => '*',
        'Access-Control-Request-Method' => '*'
      }
    end

The errors throw with the correct status code in my specs BUT the customized headers are not being set.

The more detailed the issue, the more likely that we will fix it ASAP.

To get around the issue earlier on Grape 1.0 -- I returned a custom Rack::Response but AFTER upgrading to 1.3.3 this resulted in all of my specs receiving a 500 error with the message "Invalid Response":

  def self.cors_response_to_error(error)
    log_error(error)
    eclass = error.class.to_s
    message = "OAuth error: #{error}" if eclass =~ /WineBouncer::Errors/

    opts = prepare_opts(message, error, eclass)
    opts[:trace] = error.backtrace[0, 10] unless Rails.env.production?
    Rack::Response.new(opts.to_json, status_code_for(error, eclass),
                       'Content-Type' => 'application/json',
                       'Access-Control-Allow-Origin' => '*',
                       'Access-Control-Request-Method' => '*').finish
  end

So my thought was maybe this work around was no longer needed. However, I cannot get the standard error! method to honor the customized headers. Without them my client does not receive the errors returned by the server as they are no longer CORS.

Expected behavior

Tell us what should happen

The error response should have 'Access-Control-Allow-Origin' = '' and 'Access-Control-Request-Method' = ''.

Actual behavior

Tell us what happens instead

The headers were not present in the response with or without the explicit customization in the call to error!. All other responses (successfull responses) are receiving the custom headers via:

    before do
      header['Access-Control-Allow-Origin'] = '*'
      header['Access-Control-Request-Method'] = '*'
    end

It's only the errors that are not.

System configuration

You can help us to understand your problem if you will share some very useful information about your project environment (don't forget to remove any confidential data if it exists). config.ru

# This file is used by Rack-based servers to start the application.
require 'rack'
require 'rack/cors'

use Rack::Cors do
    allow do
        origins '*'
        resource '*', headers: :any, methods: [:get, :post, :put, :patch, :delete, :options, :head]
    end
end

require_relative 'config/environment'

run Rails.application

Ruby version: `` ruby '2.7.1'

Gemfile.lock:

Gemfile.lock content ``` GIT remote: https://github.com/antek-drzewiecki/wine_bouncer.git revision: c82b88f73c7adc43a8e89ca73f7e18952ec30de4 specs: wine_bouncer (1.0.4) doorkeeper (>= 1.4, < 6.0) grape (>= 0.10) GEM remote: https://rubygems.org/ specs: actioncable (6.0.3.2) actionpack (= 6.0.3.2) nio4r (~> 2.0) websocket-driver (>= 0.6.1) actionmailbox (6.0.3.2) actionpack (= 6.0.3.2) activejob (= 6.0.3.2) activerecord (= 6.0.3.2) activestorage (= 6.0.3.2) activesupport (= 6.0.3.2) mail (>= 2.7.1) actionmailer (6.0.3.2) actionpack (= 6.0.3.2) actionview (= 6.0.3.2) activejob (= 6.0.3.2) mail (~> 2.5, >= 2.5.4) rails-dom-testing (~> 2.0) actionpack (6.0.3.2) actionview (= 6.0.3.2) activesupport (= 6.0.3.2) rack (~> 2.0, >= 2.0.8) rack-test (>= 0.6.3) rails-dom-testing (~> 2.0) rails-html-sanitizer (~> 1.0, >= 1.2.0) actiontext (6.0.3.2) actionpack (= 6.0.3.2) activerecord (= 6.0.3.2) activestorage (= 6.0.3.2) activesupport (= 6.0.3.2) nokogiri (>= 1.8.5) actionview (6.0.3.2) activesupport (= 6.0.3.2) builder (~> 3.1) erubi (~> 1.4) rails-dom-testing (~> 2.0) rails-html-sanitizer (~> 1.1, >= 1.2.0) active_model_serializers (0.10.10) actionpack (>= 4.1, < 6.1) activemodel (>= 4.1, < 6.1) case_transform (>= 0.2) jsonapi-renderer (>= 0.1.1.beta1, < 0.3) activejob (6.0.3.2) activesupport (= 6.0.3.2) globalid (>= 0.3.6) activemodel (6.0.3.2) activesupport (= 6.0.3.2) activerecord (6.0.3.2) activemodel (= 6.0.3.2) activesupport (= 6.0.3.2) activestorage (6.0.3.2) actionpack (= 6.0.3.2) activejob (= 6.0.3.2) activerecord (= 6.0.3.2) marcel (~> 0.3.1) activesupport (6.0.3.2) concurrent-ruby (~> 1.0, >= 1.0.2) i18n (>= 0.7, < 2) minitest (~> 5.1) tzinfo (~> 1.1) zeitwerk (~> 2.2, >= 2.2.2) acts_as_list (1.0.1) activerecord (>= 4.2) addressable (2.7.0) public_suffix (>= 2.0.2, < 5.0) ast (2.4.0) aws-eventstream (1.1.0) aws-partitions (1.334.0) aws-sdk-core (3.102.0) aws-eventstream (~> 1, >= 1.0.2) aws-partitions (~> 1, >= 1.239.0) aws-sigv4 (~> 1.1) jmespath (~> 1.0) aws-sdk-kms (1.35.0) aws-sdk-core (~> 3, >= 3.99.0) aws-sigv4 (~> 1.1) aws-sdk-lambda (1.45.0) aws-sdk-core (~> 3, >= 3.99.0) aws-sigv4 (~> 1.1) aws-sdk-s3 (1.70.0) aws-sdk-core (~> 3, >= 3.99.0) aws-sdk-kms (~> 1) aws-sigv4 (~> 1.1) aws-sdk-sns (1.26.0) aws-sdk-core (~> 3, >= 3.99.0) aws-sigv4 (~> 1.1) aws-sigv4 (1.2.1) aws-eventstream (~> 1, >= 1.0.2) bcrypt (3.1.13) benchmark-ips (2.7.2) bootsnap (1.4.6) msgpack (~> 1.0) builder (3.2.4) byebug (10.0.2) case_transform (0.2) activesupport choice (0.2.0) coderay (1.1.3) concurrent-ruby (1.1.6) connection_pool (2.2.3) crack (0.4.3) safe_yaml (~> 1.0.0) crass (1.0.6) daemons (1.3.1) database_cleaner (1.8.5) derailed_benchmarks (1.3.4) benchmark-ips (~> 2) get_process_mem (~> 0) heapy (~> 0) memory_profiler (~> 0) rack (>= 1) rake (> 10, < 13) thor (~> 0.19) devise (4.7.2) bcrypt (~> 3.0) orm_adapter (~> 0.1) railties (>= 4.1.0) responders warden (~> 1.2.3) diff-lcs (1.4.2) docile (1.3.2) doorkeeper (5.4.0) railties (>= 5) doorkeeper-jwt (0.4.0) jwt (~> 2.1) dotenv (2.7.5) dotenv-rails (2.7.5) dotenv (= 2.7.5) railties (>= 3.2, < 6.1) down (5.1.1) addressable (~> 2.5) dry-configurable (0.11.6) concurrent-ruby (~> 1.0) dry-core (~> 0.4, >= 0.4.7) dry-equalizer (~> 0.2) dry-container (0.7.2) concurrent-ruby (~> 1.0) dry-configurable (~> 0.1, >= 0.1.3) dry-core (0.4.9) concurrent-ruby (~> 1.0) dry-equalizer (0.3.0) dry-inflector (0.2.0) dry-logic (1.0.6) concurrent-ruby (~> 1.0) dry-core (~> 0.2) dry-equalizer (~> 0.2) dry-types (1.4.0) concurrent-ruby (~> 1.0) dry-container (~> 0.3) dry-core (~> 0.4, >= 0.4.4) dry-equalizer (~> 0.3) dry-inflector (~> 0.1, >= 0.1.2) dry-logic (~> 1.0, >= 1.0.2) erubi (1.9.0) eventmachine (1.2.7) factory_bot (6.0.2) activesupport (>= 5.0.0) factory_bot_rails (6.0.0) factory_bot (~> 6.0.0) railties (>= 5.0.0) faraday (1.0.1) multipart-post (>= 1.2, < 3) ffi (1.13.1) formatador (0.2.5) get_process_mem (0.2.5) ffi (~> 1.0) globalid (0.4.2) activesupport (>= 4.2.0) grape (1.3.3) activesupport builder dry-types (>= 1.1) mustermann-grape (~> 1.0.0) rack (>= 1.3.0) rack-accept grape-active_model_serializers (1.5.2) active_model_serializers (>= 0.10.0) grape (>= 0.8.0) grape_logging (1.8.3) grape rack guard (2.16.2) formatador (>= 0.2.4) listen (>= 2.7, < 4.0) lumberjack (>= 1.0.12, < 2.0) nenv (~> 0.1) notiffany (~> 0.0) pry (>= 0.9.12) shellany (~> 0.0) thor (>= 0.18.1) guard-rubocop (1.3.0) guard (~> 2.0) rubocop (~> 0.20) haml (5.0.4) temple (>= 0.8.0) tilt hashdiff (0.3.7) hashie (3.5.7) hashie-forbidden_attributes (0.1.1) hashie (>= 3.0) heapy (0.1.4) holidays (8.3.0) i18n (1.8.3) concurrent-ruby (~> 1.0) jaro_winkler (1.5.2) jmespath (1.4.0) json (2.3.0) jsonapi-renderer (0.2.2) jwt (2.2.1) launchy (2.4.3) addressable (~> 2.3) letter_opener (1.6.0) launchy (~> 2.2) listen (3.0.8) rb-fsevent (~> 0.9, >= 0.9.4) rb-inotify (~> 0.9, >= 0.9.7) loofah (2.6.0) crass (~> 1.0.2) nokogiri (>= 1.5.9) lumberjack (1.2.6) mail (2.7.1) mini_mime (>= 0.1.1) mailcatcher (0.2.4) eventmachine haml i18n json mail sinatra skinny (>= 0.1.2) sqlite3-ruby thin marcel (0.3.3) mimemagic (~> 0.3.2) memory_profiler (0.9.11) method_source (1.0.0) mimemagic (0.3.5) mini_mime (1.0.2) mini_portile2 (2.4.0) minitest (5.14.1) msgpack (1.3.3) multipart-post (2.1.1) mustermann (1.1.1) ruby2_keywords (~> 0.0.1) mustermann-grape (1.0.1) mustermann (>= 1.0.0) nenv (0.3.0) nio4r (2.5.2) nokogiri (1.10.9) mini_portile2 (~> 2.4.0) notiffany (0.1.3) nenv (~> 0.1) shellany (~> 0.0) orm_adapter (0.5.0) parallel (1.14.0) parser (2.6.0.0) ast (~> 2.4.0) pg (1.0.0) powerpack (0.1.2) pry (0.13.1) coderay (~> 1.1) method_source (~> 1.0) psych (3.1.0) public_suffix (4.0.5) puma (4.3.5) nio4r (~> 2.0) puma_worker_killer (0.1.1) get_process_mem (~> 0.2) puma (>= 2.7, < 5) rack (2.2.3) rack-accept (0.4.5) rack (>= 0.4) rack-cors (1.0.2) rack-protection (2.0.3) rack rack-test (1.1.0) rack (>= 1.0, < 3) rails (6.0.3.2) actioncable (= 6.0.3.2) actionmailbox (= 6.0.3.2) actionmailer (= 6.0.3.2) actionpack (= 6.0.3.2) actiontext (= 6.0.3.2) actionview (= 6.0.3.2) activejob (= 6.0.3.2) activemodel (= 6.0.3.2) activerecord (= 6.0.3.2) activestorage (= 6.0.3.2) activesupport (= 6.0.3.2) bundler (>= 1.3.0) railties (= 6.0.3.2) sprockets-rails (>= 2.0.0) rails-dom-testing (2.0.3) activesupport (>= 4.2.0) nokogiri (>= 1.6) rails-erd (1.5.2) activerecord (>= 3.2) activesupport (>= 3.2) choice (~> 0.2.0) ruby-graphviz (~> 1.2) rails-html-sanitizer (1.3.0) loofah (~> 2.3) rails_12factor (0.0.3) rails_serve_static_assets rails_stdout_logging rails_serve_static_assets (0.0.5) rails_stdout_logging (0.0.5) railties (6.0.3.2) actionpack (= 6.0.3.2) activesupport (= 6.0.3.2) method_source rake (>= 0.8.7) thor (>= 0.20.3, < 2.0) rainbow (3.0.0) rake (12.3.3) rb-fsevent (0.10.4) rb-inotify (0.10.1) ffi (~> 1.0) redis (4.0.3) responders (3.0.1) actionpack (>= 5.0) railties (>= 5.0) rspec-core (3.9.2) rspec-support (~> 3.9.3) rspec-expectations (3.9.2) diff-lcs (>= 1.2.0, < 2.0) rspec-support (~> 3.9.0) rspec-mocks (3.9.1) diff-lcs (>= 1.2.0, < 2.0) rspec-support (~> 3.9.0) rspec-rails (4.0.1) actionpack (>= 4.2) activesupport (>= 4.2) railties (>= 4.2) rspec-core (~> 3.9) rspec-expectations (~> 3.9) rspec-mocks (~> 3.9) rspec-support (~> 3.9) rspec-support (3.9.3) rspec_junit_formatter (0.4.1) rspec-core (>= 2, < 4, != 2.12.0) rubocop (0.65.0) jaro_winkler (~> 1.5.1) parallel (~> 1.10) parser (>= 2.5, != 2.5.1.1) powerpack (~> 0.1) psych (>= 3.1.0) rainbow (>= 2.2.2, < 4.0) ruby-progressbar (~> 1.7) unicode-display_width (~> 1.4.0) ruby-graphviz (1.2.3) ruby-progressbar (1.10.0) ruby2_keywords (0.0.2) safe_yaml (1.0.4) sentry-raven (3.0.0) faraday (>= 1.0) shellany (0.0.1) shoulda-matchers (4.3.0) activesupport (>= 4.2.0) sidekiq (5.2.9) connection_pool (~> 2.2, >= 2.2.2) rack (~> 2.0) rack-protection (>= 1.5.0) redis (>= 3.3.5, < 4.2) simplecov (0.18.5) docile (~> 1.1) simplecov-html (~> 0.11) simplecov-html (0.12.2) sinatra (2.0.3) mustermann (~> 1.0) rack (~> 2.0) rack-protection (= 2.0.3) tilt (~> 2.0) skinny (0.2.2) eventmachine (~> 1.0) thin spring (2.0.2) activesupport (>= 4.2) spring-watcher-listen (2.0.1) listen (>= 2.7, < 4.0) spring (>= 1.2, < 3.0) sprockets (4.0.2) concurrent-ruby (~> 1.0) rack (> 1, < 3) sprockets-rails (3.2.1) actionpack (>= 4.0) activesupport (>= 4.0) sprockets (>= 3.0.0) sqlite3 (1.3.13) sqlite3-ruby (1.3.3) sqlite3 (>= 1.3.3) stackprof (0.2.12) temple (0.8.0) thin (1.7.2) daemons (~> 1.0, >= 1.0.9) eventmachine (~> 1.0, >= 1.0.4) rack (>= 1, < 3) thor (0.20.3) thread_safe (0.3.6) tilt (2.0.9) timecop (0.9.1) tzinfo (1.2.7) thread_safe (~> 0.1) unicode-display_width (1.4.1) validates_email_format_of (1.6.3) i18n vcr (6.0.0) warden (1.2.8) rack (>= 2.0.6) webmock (3.4.2) addressable (>= 2.3.6) crack (>= 0.3.2) hashdiff websocket-driver (0.7.2) websocket-extensions (>= 0.1.0) websocket-extensions (0.1.5) zeitwerk (2.3.0) PLATFORMS ruby DEPENDENCIES active_model_serializers (~> 0.10.10) acts_as_list (~> 1.0.1) aws-sdk-core (~> 3) aws-sdk-lambda (~> 1) aws-sdk-s3 (~> 1) aws-sdk-sns (~> 1) bootsnap (>= 1.4.2) byebug database_cleaner derailed_benchmarks devise (~> 4.7.2) doorkeeper (~> 5.4.0) doorkeeper-jwt (~> 0.4.0) dotenv (~> 2.7.1) dotenv-rails (~> 2.7.1) down factory_bot_rails ffi (~> 1.13.1) grape (~> 1.3.3) grape-active_model_serializers (~> 1.5.2) grape_logging (~> 1.8.3) guard-rubocop hashie-forbidden_attributes holidays letter_opener listen (~> 3.0.5) mailcatcher pg puma (~> 4.1) puma_worker_killer rack-cors rails (~> 6.0.3, >= 6.0.3.2) rails-erd rails_12factor rake (~> 12.3.3) redis (~> 4.0.1) rspec-core rspec-expectations rspec-mocks rspec-rails (~> 4.0.0) rspec-support rspec_junit_formatter rubocop (~> 0.65.0) sentry-raven shoulda-matchers sidekiq simplecov spring spring-watcher-listen (~> 2.0.0) stackprof timecop tzinfo-data validates_email_format_of (~> 1.6.3) vcr webmock wine_bouncer! RUBY VERSION ruby 2.7.1p83 BUNDLED WITH 2.1.4 ```
jimjeffers commented 4 years ago

Here is an example of what's happening in my specs. All of my specs that return a 200 response have the correct headers. For example this spec passes:

    it 'returns a 200 with the [redacted] if the user is [redacted]' do
      #...
      get_as_user path
      expect(response.status).to eq(200)
      body = JSON.parse(response.body)
      expect(body.length).to eq 2
      result_ids = [body[0]['uuid'], body[1]['uuid']]
      expect(result_ids.include?(first_event.uuid)).to eq true
      expect(result_ids.include?(second_event.uuid)).to eq true
      expect(response.headers['Content-Type']).to match('json')
      expect(response.headers['Access-Control-Allow-Origin']).to eq('*')
      expect(response.headers['Access-Control-Request-Method']).to eq('*')
    end

However, any spec expecting an error gets the right content-type, error code, etc.. but the additional CORS headers are no longer present:

    it 'returns a 404 if the user does not have permission' do
      get_as_user path
      expect(response.status).to eq(404)
      expect(response.headers['Content-Type']).to match('json')
      expect(response.headers['Access-Control-Allow-Origin']).to eq('*')
      expect(response.headers['Access-Control-Request-Method']).to eq('*')
    end
dblock commented 4 years ago

At the very least I would say that the regression is not expected. Do you think you could turn this spec into one in the Grape project that was succeeding < 1.3.3?

jimjeffers commented 4 years ago

Sure. Is there a specific set of specs I should look at when writing one for the library to test this? Let me know and I will work on a PR.

jimjeffers commented 4 years ago

@dblock you know it looks like this must just be an issue with 1.3.3. I upgraded to 1.4 by installing directly from master and my specs pass when I use the headers option on error! as mentioned earlier:

rescue_from :all do |error|
      error! error.message, FilmCal::Base.status_code_for(error, error.class.to_s), {
        'Access-Control-Allow-Origin' => '*',
        'Access-Control-Request-Method' => '*'
      }
end

My work around returning a rack response directly still returns an "Invalid Response" error but that is not a big deal as it was a work around anyway.

dblock commented 4 years ago

I am a little worried that we introduced a regression, then fixed it, but still don't have specs for this. I don't see anything in CHANGELOG that says something like this should be fixed. Do appreciate if you could write a set of specs for this behavior, add a new file in spec/grape/api and we can see if we already have something similar later. Once you have a failing spec on 1.3.3 try bisecting it down to a fix in 1.4.0.