Skip to content

Commit

Permalink
Fix Content Publisher returning 500 on document 404s
Browse files Browse the repository at this point in the history
Content Publisher currently returns an unstyled 500 error [1] when a 404 page
is encountered. This happens because an exception loop occurs during the
rendering of an error response. The problem occurs because `check_user_access`
before action runs before all actions (including error actions). So if a
document is going to 404, the `ActiveRecord::RecordNotFound` is raised once
before the requested action and then again in the ErrorsController.

To avoid this we are using skip_before_action callback in ErrorsController so
the `check_user_access` action only runs once.

We are setting `config.action_dispatch.show_exceptions` to true in the test
environment so exceptions are rescued and Rails returns a corresponding HTTP
status code instead of returning `ActiveRecord::RecordNotFound` exeption

[1] https://github.com/rails/rails/blob/98a57aa5f610bc66af31af409c72173cdeeb3c9e/actionpack/lib/action_dispatch/middleware/show_exceptions.rb#L20-L24
  • Loading branch information
Aga Dufrat committed Nov 21, 2019
1 parent 018f98d commit 184a93d
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 2 deletions.
1 change: 1 addition & 0 deletions app/controllers/errors_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

class ErrorsController < ApplicationController
skip_before_action :verify_authenticity_token
skip_before_action :check_user_access

def bad_request
render status: :bad_request, formats: :html
Expand Down
4 changes: 2 additions & 2 deletions config/environments/test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@
config.consider_all_requests_local = true
config.action_controller.perform_caching = false

# Raise exceptions instead of rendering exception templates.
config.action_dispatch.show_exceptions = false
# Handle exceptions ourselves and return HTTP status instead of raising exceptions.
config.action_dispatch.show_exceptions = true

# Disable request forgery protection in test environment.
config.action_controller.allow_forgery_protection = false
Expand Down
8 changes: 8 additions & 0 deletions spec/requests/errors_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,12 @@
expect(response.body).to include(I18n.t!("errors.internal_server_error.title"))
end
end

describe "bypassing user access checks" do
it "returns a not found response when a document doesn't exist" do
get document_path("document-that-does-not-exist")

expect(response).to have_http_status(:not_found)
end
end
end

0 comments on commit 184a93d

Please sign in to comment.