From 7bed81812d9c9d46ffcaf79b40080e56d913fdc3 Mon Sep 17 00:00:00 2001 From: evazion Date: Sun, 6 Feb 2022 17:02:07 -0600 Subject: [PATCH] Don't show error messages that could contain private information. Fix a potential exploit where private information could be leaked if it was contained in the error message of an unexpected exception. For example, NoMethodError contains a raw dump of the object in the error message, which could leak private user data if you could force a User object to raise a NoMethodError. Fix the error page to only show known-safe error messages from expected exceptions, not unknown error messages from unexpected exceptions. API changes: * JSON errors now have a `message` param. The message will be blank for unknown exceptions. * XML errors have a new format. This is a breaking change. They now look like this: false PaginationExtension::PaginationError You cannot go beyond page 5000. app/logical/pagination_extension.rb:54:in `paginate' app/models/application_record.rb:17:in `paginate' app/logical/post_query_builder.rb:529:in `paginated_posts' app/logical/post_sets/post.rb:95:in `posts' app/controllers/posts_controller.rb:22:in `index' instead of like this: You cannot go beyond page 5000. --- app/controllers/application_controller.rb | 18 +++++---- app/controllers/sessions_controller.rb | 2 +- app/logical/session_loader.rb | 4 +- app/views/static/_backtrace.html.erb | 11 ------ app/views/static/error.atom.erb | 1 + app/views/static/error.html.erb | 18 +++++++-- app/views/static/error.js.erb | 11 ++---- app/views/static/error.json.erb | 6 +-- app/views/static/error.xml.erb | 3 +- .../functional/application_controller_test.rb | 39 +++++++++++++++++++ test/functional/static_controller_test.rb | 2 +- 11 files changed, 74 insertions(+), 41 deletions(-) delete mode 100644 app/views/static/_backtrace.html.erb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index bfd494a02..daee00722 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -112,11 +112,11 @@ class ApplicationController < ActionController::Base when ActiveRecord::QueryCanceled render_error_page(500, exception, template: "static/search_timeout", message: "The database timed out running your query.") when ActionController::BadRequest - render_error_page(400, exception) + render_error_page(400, exception, message: exception.message) when SessionLoader::AuthenticationFailure - render_error_page(401, exception, template: "sessions/new") + render_error_page(401, exception, message: exception.message, template: "sessions/new") when ActionController::InvalidAuthenticityToken, ActionController::UnpermittedParameters, ActionController::InvalidCrossOriginRequest, ActionController::Redirecting::UnsafeRedirectError - render_error_page(403, exception) + render_error_page(403, exception, message: exception.message) when ActiveSupport::MessageVerifier::InvalidSignature, # raised by `find_signed!` User::PrivilegeError, Pundit::NotAuthorizedError @@ -124,7 +124,7 @@ class ApplicationController < ActionController::Base when ActiveRecord::RecordNotFound render_error_page(404, exception, message: "That record was not found.") when ActionController::RoutingError - render_error_page(405, exception) + render_error_page(405, exception, message: exception.message) when ActionController::UnknownFormat, ActionView::MissingTemplate render_error_page(406, exception, message: "#{request.format} is not a supported format for this page") when PaginationExtension::PaginationError @@ -132,7 +132,7 @@ class ApplicationController < ActionController::Base when PostQueryBuilder::TagLimitError render_error_page(422, exception, template: "static/tag_limit_error", message: "You cannot search for more than #{CurrentUser.tag_query_limit} tags at a time.") when RateLimiter::RateLimitError - render_error_page(429, exception) + render_error_page(429, exception, message: "Rate limit exceeded. You're doing that too fast") when Rack::Timeout::RequestTimeoutException render_error_page(500, exception, message: "Your request took too long to complete and was canceled.") when NotImplementedError @@ -140,18 +140,20 @@ class ApplicationController < ActionController::Base when PG::ConnectionBad render_error_page(503, exception, message: "The database is unavailable. Try again later.") else - raise exception if !Rails.env.production? || Danbooru.config.debug_mode + raise exception if Rails.env.development? || Danbooru.config.debug_mode render_error_page(500, exception) end end - def render_error_page(status, exception = nil, message: exception.message, template: "static/error", format: request.format.symbol) + def render_error_page(status, exception = nil, message: "", template: "static/error", format: request.format.symbol) @exception = exception @expected = status < 500 - @message = message.encode("utf-8", invalid: :replace, undef: :replace) + @message = message.to_s.encode("utf-8", invalid: :replace, undef: :replace) @backtrace = Rails.backtrace_cleaner.clean(@exception.backtrace) if @exception format = :html unless format.in?(%i[html json xml js atom]) + @api_response = { success: false, error: @exception.class.to_s, message: @message, backtrace: @backtrace } + # if InvalidAuthenticityToken was raised, CurrentUser isn't set so we have to use the blank layout. layout = CurrentUser.user.present? ? "default" : "blank" diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index be32d8303..7a5b118bf 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -22,7 +22,7 @@ class SessionsController < ApplicationController respond_with(user, location: url) else flash.now[:notice] = "Password was incorrect" - raise SessionLoader::AuthenticationFailure + raise SessionLoader::AuthenticationFailure, "Username or password incorrect" end end diff --git a/app/logical/session_loader.rb b/app/logical/session_loader.rb index 74db0c2c7..3cdbcb724 100644 --- a/app/logical/session_loader.rb +++ b/app/logical/session_loader.rb @@ -109,7 +109,7 @@ class SessionLoader elsif params[:login].present? && params[:api_key].present? authenticate_api_key(params[:login], params[:api_key]) else - raise AuthenticationFailure + raise AuthenticationFailure, "Missing `login` or `api_key`" end end @@ -129,7 +129,7 @@ class SessionLoader # permissions for this endpoint def authenticate_api_key(name, key) user, api_key = User.find_by_name(name)&.authenticate_api_key(key) - raise AuthenticationFailure if user.blank? + raise AuthenticationFailure, "Invalid API key" if user.blank? update_api_key(api_key) raise User::PrivilegeError if !api_key.has_permission?(request.remote_ip, request.params[:controller], request.params[:action]) CurrentUser.user = user diff --git a/app/views/static/_backtrace.html.erb b/app/views/static/_backtrace.html.erb deleted file mode 100644 index 149fcc852..000000000 --- a/app/views/static/_backtrace.html.erb +++ /dev/null @@ -1,11 +0,0 @@ -<%# backtrace %> - - diff --git a/app/views/static/error.atom.erb b/app/views/static/error.atom.erb index e69de29bb..70da56c18 100644 --- a/app/views/static/error.atom.erb +++ b/app/views/static/error.atom.erb @@ -0,0 +1 @@ +<%= raw @api_response.to_xml(root: "result") %> diff --git a/app/views/static/error.html.erb b/app/views/static/error.html.erb index 8aa90dc1f..9a2fd3835 100644 --- a/app/views/static/error.html.erb +++ b/app/views/static/error.html.erb @@ -1,9 +1,19 @@ -<% page_title "Error: #{@message}" %> +<% page_title "Error: #{@exception.class}" %>

Error

-

<%= @message %>

-<% unless @expected %> +<% if @message.present? %> +

<%= @message %>

+<% else %> +

Unexpected error: <%= @exception.class %>.

+

Details

- <%= render "static/backtrace", exception: @exception, backtrace: @backtrace %> + + <% end %> diff --git a/app/views/static/error.js.erb b/app/views/static/error.js.erb index a9acdfc58..22fa986fa 100644 --- a/app/views/static/error.js.erb +++ b/app/views/static/error.js.erb @@ -1,10 +1,7 @@ -var klass = <%= raw @exception.class.to_s.to_json %>; -var message = <%= raw @message.to_json %>; -var backtrace = <%= raw @backtrace.to_json %>; +console.error(<%= raw @api_response.to_json %>); -<% if @expected %> - Danbooru.Utility.error(message); +<% if @message.present? %> + Danbooru.Utility.error("<%= j @message %>"); <% else %> - console.error(klass, message, backtrace); - Danbooru.Utility.error(message); + Danbooru.Utility.error("<%= j "Unexpected error: #{@exception.class}" %>"); <% end %> diff --git a/app/views/static/error.json.erb b/app/views/static/error.json.erb index 3d730d214..9dbf8ab9f 100644 --- a/app/views/static/error.json.erb +++ b/app/views/static/error.json.erb @@ -1,5 +1 @@ -{ - "success": false, - "message": <%= raw @message.to_json %>, - "backtrace": <%= raw @backtrace.to_json %> -} +<%= raw @api_response.to_json %> diff --git a/app/views/static/error.xml.erb b/app/views/static/error.xml.erb index 4bb805938..70da56c18 100644 --- a/app/views/static/error.xml.erb +++ b/app/views/static/error.xml.erb @@ -1,2 +1 @@ - -<%= @message %> +<%= raw @api_response.to_xml(root: "result") %> diff --git a/test/functional/application_controller_test.rb b/test/functional/application_controller_test.rb index e5179a36c..53ff6a86e 100644 --- a/test/functional/application_controller_test.rb +++ b/test/functional/application_controller_test.rb @@ -36,6 +36,45 @@ class ApplicationControllerTest < ActionDispatch::IntegrationTest end end + context "on an unexpected error" do + setup do + User.stubs(:find).raises(NoMethodError.new("pwned")) + @user = create(:user) + end + + should "not return the error message in the HTML response" do + get user_path(@user) + + assert_response 500 + assert_match(/NoMethodError/, response.body.to_s) + assert_no_match(/pwned/, response.body.to_s) + end + + should "not return the error message in the JSON response" do + get user_path(@user, format: :json) + + assert_response 500 + assert_match(/NoMethodError/, response.body.to_s) + assert_no_match(/pwned/, response.body.to_s) + end + + should "not return the error message in the XML response" do + get user_path(@user, format: :xml) + + assert_response 500 + assert_match(/NoMethodError/, response.body.to_s) + assert_no_match(/pwned/, response.body.to_s) + end + + should "not return the error message in the JS response" do + get user_path(@user, format: :js) + + assert_response 500 + assert_match(/NoMethodError/, response.body.to_s) + assert_no_match(/pwned/, response.body.to_s) + end + end + context "on api authentication" do setup do @user = create(:user, password: "password") diff --git a/test/functional/static_controller_test.rb b/test/functional/static_controller_test.rb index 13b99b53c..77f1332a4 100644 --- a/test/functional/static_controller_test.rb +++ b/test/functional/static_controller_test.rb @@ -68,7 +68,7 @@ class StaticControllerTest < ActionDispatch::IntegrationTest get "/qwoiqogieqg", as: :xml assert_response 404 - assert_equal("Page not found", response.parsed_body.at("result").text) + assert_equal("Page not found", response.parsed_body.xpath("result/message").text) end should "render the 404 page when page_not_found_pool_id is configured" do