From 5f7a9f8946f73e658d59fed4fbcedfc3b860ca31 Mon Sep 17 00:00:00 2001 From: Moncef Belyamani Date: Wed, 7 Nov 2018 13:01:33 -0500 Subject: [PATCH] LG-746 Return 400 error for invalid String params **Why**: When a controller uses Strong Parameters such as: `params.require(:user).permit(:email)`, the `user` param is assumed to be a Hash, but it's easy for a pentester (for example) to set the `user` param to a String instead, which by default would raise a 500 error because the String class doesn't respond to `permit`. **How**: To get around that, we monkey patched the Ruby String class to raise an instance of `ActionController::ParameterMissing`, which will return a 400 error. 500 errors can potentially page people in the middle of the night, whereas 400 errors don't. Note that Devise handles this scenario gracefully in SessionsController, but because it determines whether or not to call `permit` based on whether or not the object responds to `permit`, the Devise code will end up calling `permit` because all Strings respond to it now after our monkey patch. This is why the `invalid_sign_in_params_spec` had to change. --- app/controllers/application_controller.rb | 3 ++ .../sign_up/email_resend_controller.rb | 2 -- lib/core_extensions/string/permit.rb | 18 +++++++++++ spec/requests/invalid_sign_in_params_spec.rb | 8 +++-- .../params_is_string_instead_of_hash_spec.rb | 30 +++++++++++++++++++ 5 files changed, 57 insertions(+), 4 deletions(-) create mode 100644 lib/core_extensions/string/permit.rb create mode 100644 spec/requests/params_is_string_instead_of_hash_spec.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 462c8f3e7df..a36efcf8e2f 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -1,4 +1,7 @@ +require 'core_extensions/string/permit' + class ApplicationController < ActionController::Base + String.include CoreExtensions::String::Permit include UserSessionContext include VerifyProfileConcern include LocaleHelper diff --git a/app/controllers/sign_up/email_resend_controller.rb b/app/controllers/sign_up/email_resend_controller.rb index 5a46b03636d..2ee962a5af0 100644 --- a/app/controllers/sign_up/email_resend_controller.rb +++ b/app/controllers/sign_up/email_resend_controller.rb @@ -1,7 +1,5 @@ module SignUp class EmailResendController < ApplicationController - include UnconfirmedUserConcern - def new @user = User.new @resend_email_confirmation_form = ResendEmailConfirmationForm.new diff --git a/lib/core_extensions/string/permit.rb b/lib/core_extensions/string/permit.rb new file mode 100644 index 00000000000..37fb341ebd4 --- /dev/null +++ b/lib/core_extensions/string/permit.rb @@ -0,0 +1,18 @@ +# When a controller uses Strong Parameters such as: +# params.require(:user).permit(:email), the `user` param is assumed to +# be a Hash, but it's easy for a pentester (for example) to set the +# `user` param to a String instead, which by default would raise a 500 +# error because the String class doesn't respond to `permit`. To get +# around that, we monkey patched the Ruby String class to raise an +# instance of ActionController::ParameterMissing, which will return +# a 400 error. 500 errors can potentially page people in the middle of +# the night, whereas 400 errors don't. +module CoreExtensions + module String + module Permit + def permit(*) + raise ActionController::ParameterMissing, '#permit called on String' + end + end + end +end diff --git a/spec/requests/invalid_sign_in_params_spec.rb b/spec/requests/invalid_sign_in_params_spec.rb index c4721221143..b42a0196ea2 100644 --- a/spec/requests/invalid_sign_in_params_spec.rb +++ b/spec/requests/invalid_sign_in_params_spec.rb @@ -1,8 +1,12 @@ require 'rails_helper' describe 'visiting sign in page with invalid user params' do - it 'does not raise an exception' do - get new_user_session_path, params: { user: 'test@test.com' } + it 'raises ActionController::ParameterMissing' do + params = { user: 'test@test.com' } + message_string = 'param is missing or the value is empty: #permit called on String' + + expect { get new_user_session_path, params: params }. + to raise_error(ActionController::ParameterMissing, message_string) end end diff --git a/spec/requests/params_is_string_instead_of_hash_spec.rb b/spec/requests/params_is_string_instead_of_hash_spec.rb new file mode 100644 index 00000000000..8c265907ce8 --- /dev/null +++ b/spec/requests/params_is_string_instead_of_hash_spec.rb @@ -0,0 +1,30 @@ +require 'rails_helper' + +# When a controller uses Strong Parameters such as: +# params.require(:user).permit(:email), the `user` param is assumed to +# be a Hash, but it's easy for a pentester (for example) to set the +# `user` param to a String instead, which by default would raise a 500 +# error because the String class doesn't respond to `permit`. To get +# around that, we monkey patched the Ruby String class to raise an +# instance of ActionController::ParameterMissing, which will return +# a 400 error. 500 errors can potentially page people in the middle of +# the night, whereas 400 errors don't. +describe 'submitting email resend form with required param as String' do + it 'raises ActionController::ParameterMissing' do + params = { resend_email_confirmation_form: 'abcdef' } + message_string = 'param is missing or the value is empty: #permit called on String' + + expect { post sign_up_create_email_resend_path, params: params }. + to raise_error(ActionController::ParameterMissing, message_string) + end +end + +describe 'submitting email registration form with required param as String' do + it 'raises ActionController::ParameterMissing' do + params = { user: 'abcdef' } + message_string = 'param is missing or the value is empty: #permit called on String' + + expect { post sign_up_register_path, params: params }. + to raise_error(ActionController::ParameterMissing, message_string) + end +end