diff --git a/config/locales/en.yml b/config/locales/en.yml index fb6d4eac..df70e0f7 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1,5 +1,6 @@ en: graphql_devise: + redirect_url_not_allowed: "Redirect to '%{redirect_url}' not allowed." registration_failed: "User couldn't be registered" resource_build_failed: "Resource couldn't be built, execution stopped." not_authenticated: "User is not logged in." @@ -7,7 +8,6 @@ en: invalid_resource: "Errors present in the resource." registrations: missing_confirm_redirect_url: "Missing 'confirm_success_url' parameter. Required when confirmable module is enabled." - redirect_url_not_allowed: "Redirect to '%{redirect_url}' not allowed." passwords: update_password_error: "Unable to update user password" missing_passwords: "You must fill out the fields labeled 'Password' and 'Password confirmation'." diff --git a/lib/graphql_devise/concerns/controller_methods.rb b/lib/graphql_devise/concerns/controller_methods.rb index 4c6bac70..382fe850 100644 --- a/lib/graphql_devise/concerns/controller_methods.rb +++ b/lib/graphql_devise/concerns/controller_methods.rb @@ -7,6 +7,12 @@ module ControllerMethods private + def check_redirect_url_whitelist!(redirect_url) + if blacklisted_redirect_url?(redirect_url) + raise_user_error(I18n.t('graphql_devise.redirect_url_not_allowed', redirect_url: redirect_url)) + end + end + def raise_user_error(message) raise GraphqlDevise::UserError, message end diff --git a/lib/graphql_devise/mutations/resend_confirmation.rb b/lib/graphql_devise/mutations/resend_confirmation.rb index 01763ea1..a6c58f1a 100644 --- a/lib/graphql_devise/mutations/resend_confirmation.rb +++ b/lib/graphql_devise/mutations/resend_confirmation.rb @@ -9,6 +9,8 @@ class ResendConfirmation < Base field :message, String, null: false def resolve(email:, redirect_url:) + check_redirect_url_whitelist!(redirect_url) + resource = find_confirmable_resource(email) if resource diff --git a/lib/graphql_devise/mutations/send_password_reset.rb b/lib/graphql_devise/mutations/send_password_reset.rb index 78a569bd..e2dda246 100644 --- a/lib/graphql_devise/mutations/send_password_reset.rb +++ b/lib/graphql_devise/mutations/send_password_reset.rb @@ -9,6 +9,8 @@ class SendPasswordReset < Base field :message, String, null: false def resolve(email:, redirect_url:) + check_redirect_url_whitelist!(redirect_url) + resource = find_resource(:email, get_case_insensitive_field(:email, email)) if resource diff --git a/lib/graphql_devise/mutations/sign_up.rb b/lib/graphql_devise/mutations/sign_up.rb index a0cdd8d6..219a1f72 100644 --- a/lib/graphql_devise/mutations/sign_up.rb +++ b/lib/graphql_devise/mutations/sign_up.rb @@ -22,9 +22,7 @@ def resolve(confirm_success_url: nil, **attrs) raise_user_error(I18n.t('graphql_devise.registrations.missing_confirm_redirect_url')) end - if blacklisted_redirect_url?(redirect_url) - raise_user_error(I18n.t('graphql_devise.registrations.redirect_url_not_allowed', redirect_url: redirect_url)) - end + check_redirect_url_whitelist!(redirect_url) resource.skip_confirmation_notification! if resource.respond_to?(:skip_confirmation_notification!) diff --git a/lib/graphql_devise/resolvers/check_password_token.rb b/lib/graphql_devise/resolvers/check_password_token.rb index 56f45565..4d91bbad 100644 --- a/lib/graphql_devise/resolvers/check_password_token.rb +++ b/lib/graphql_devise/resolvers/check_password_token.rb @@ -27,6 +27,7 @@ def resolve(reset_password_token:, redirect_url: nil) ) if redirect_url.present? + check_redirect_url_whitelist!(redirect_url) controller.redirect_to(resource.build_auth_url(redirect_url, built_redirect_headers)) else set_auth_headers(resource) diff --git a/lib/graphql_devise/resolvers/confirm_account.rb b/lib/graphql_devise/resolvers/confirm_account.rb index 55cbcd72..9eca02f6 100644 --- a/lib/graphql_devise/resolvers/confirm_account.rb +++ b/lib/graphql_devise/resolvers/confirm_account.rb @@ -7,6 +7,8 @@ class ConfirmAccount < Base argument :redirect_url, String, required: true def resolve(confirmation_token:, redirect_url:) + check_redirect_url_whitelist!(redirect_url) + resource = resource_class.confirm_by_token(confirmation_token) if resource.errors.empty? diff --git a/spec/dummy/config/initializers/devise_token_auth.rb b/spec/dummy/config/initializers/devise_token_auth.rb index 30d9c694..cec12a3f 100644 --- a/spec/dummy/config/initializers/devise_token_auth.rb +++ b/spec/dummy/config/initializers/devise_token_auth.rb @@ -39,6 +39,8 @@ config.default_confirm_success_url = 'https://google.com' + config.redirect_whitelist = ['https://google.com'] + # By default we will use callbacks for single omniauth. # It depends on fields like email, provider and uid. # config.default_callbacks = true diff --git a/spec/requests/mutations/additional_mutations_spec.rb b/spec/requests/mutations/additional_mutations_spec.rb index 0805c90c..c5d0412a 100644 --- a/spec/requests/mutations/additional_mutations_spec.rb +++ b/spec/requests/mutations/additional_mutations_spec.rb @@ -9,7 +9,6 @@ let(:password) { Faker::Internet.password } let(:password_confirmation) { password } let(:email) { Faker::Internet.email } - let(:redirect) { Faker::Internet.url } context 'when using the user model' do let(:query) do diff --git a/spec/requests/mutations/resend_confirmation_spec.rb b/spec/requests/mutations/resend_confirmation_spec.rb index ccf03632..a2ce888d 100644 --- a/spec/requests/mutations/resend_confirmation_spec.rb +++ b/spec/requests/mutations/resend_confirmation_spec.rb @@ -9,7 +9,7 @@ let!(:user) { create(:user, confirmed_at: nil, email: 'mwallace@wallaceinc.com') } let(:email) { user.email } let(:id) { user.id } - let(:redirect) { Faker::Internet.url } + let(:redirect) { 'https://google.com' } let(:query) do <<-GRAPHQL mutation { @@ -23,6 +23,21 @@ GRAPHQL end + context 'when redirect_url is not whitelisted' do + let(:redirect) { 'https://not-safe.com' } + + it 'returns a not whitelisted redirect url error' do + expect { post_request }.to not_change(ActionMailer::Base.deliveries, :count) + + expect(json_response[:errors]).to containing_exactly( + hash_including( + message: "Redirect to '#{redirect}' not allowed.", + extensions: { code: 'USER_ERROR' } + ) + ) + end + end + context 'when params are correct' do context 'when using the gem schema' do it 'sends an email to the user with confirmation url and returns a success message' do diff --git a/spec/requests/mutations/send_password_reset_spec.rb b/spec/requests/mutations/send_password_reset_spec.rb index 49dfdd94..4a42d40f 100644 --- a/spec/requests/mutations/send_password_reset_spec.rb +++ b/spec/requests/mutations/send_password_reset_spec.rb @@ -7,7 +7,7 @@ let!(:user) { create(:user, :confirmed, email: 'jwinnfield@wallaceinc.com') } let(:email) { user.email } - let(:redirect_url) { Faker::Internet.url } + let(:redirect_url) { 'https://google.com' } let(:query) do <<-GRAPHQL mutation { @@ -21,6 +21,21 @@ GRAPHQL end + context 'when redirect_url is not whitelisted' do + let(:redirect_url) { 'https://not-safe.com' } + + it 'returns a not whitelisted redirect url error' do + expect { post_request }.to not_change(ActionMailer::Base.deliveries, :count) + + expect(json_response[:errors]).to containing_exactly( + hash_including( + message: "Redirect to '#{redirect_url}' not allowed.", + extensions: { code: 'USER_ERROR' } + ) + ) + end + end + context 'when params are correct' do context 'when using the gem schema' do it 'sends password reset email' do diff --git a/spec/requests/mutations/sign_up_spec.rb b/spec/requests/mutations/sign_up_spec.rb index 1fed3795..7292d4b1 100644 --- a/spec/requests/mutations/sign_up_spec.rb +++ b/spec/requests/mutations/sign_up_spec.rb @@ -8,7 +8,7 @@ let(:name) { Faker::Name.name } let(:password) { Faker::Internet.password } let(:email) { Faker::Internet.email } - let(:redirect) { Faker::Internet.url } + let(:redirect) { 'https://google.com' } context 'when using the user model' do let(:query) do @@ -31,6 +31,24 @@ GRAPHQL end + context 'when redirect_url is not whitelisted' do + let(:redirect) { 'https://not-safe.com' } + + it 'returns a not whitelisted redirect url error' do + expect { post_request }.to( + not_change(User, :count) + .and(not_change(ActionMailer::Base.deliveries, :count)) + ) + + expect(json_response[:errors]).to containing_exactly( + hash_including( + message: "Redirect to '#{redirect}' not allowed.", + extensions: { code: 'USER_ERROR' } + ) + ) + end + end + context 'when params are correct' do it 'creates a new resource that requires confirmation' do expect { post_request }.to( diff --git a/spec/requests/queries/check_password_token_spec.rb b/spec/requests/queries/check_password_token_spec.rb index 61e48024..fc31f098 100644 --- a/spec/requests/queries/check_password_token_spec.rb +++ b/spec/requests/queries/check_password_token_spec.rb @@ -54,6 +54,21 @@ expect(response.body).to include('uid=') expect(response.body).to include('expiry=') end + + context 'when redirect_url is not whitelisted' do + let(:redirect_url) { 'https://not-safe.com' } + + before { post_request } + + it 'returns a not whitelisted redirect url error' do + expect(json_response[:errors]).to containing_exactly( + hash_including( + message: "Redirect to '#{redirect_url}' not allowed.", + extensions: { code: 'USER_ERROR' } + ) + ) + end + end end context 'when token has expired' do diff --git a/spec/requests/queries/confirm_account_spec.rb b/spec/requests/queries/confirm_account_spec.rb index 6a149abd..82b23358 100644 --- a/spec/requests/queries/confirm_account_spec.rb +++ b/spec/requests/queries/confirm_account_spec.rb @@ -7,7 +7,7 @@ context 'when using the user model' do let(:user) { create(:user, confirmed_at: nil) } - let(:redirect) { Faker::Internet.url } + let(:redirect) { 'https://google.com' } let(:query) do <<-GRAPHQL { @@ -43,6 +43,21 @@ expect(user).to be_active_for_authentication end + context 'when redirect_url is not whitelisted' do + let(:redirect) { 'https://not-safe.com' } + + it 'returns a not whitelisted redirect url error' do + expect { post_request }.to not_change(ActionMailer::Base.deliveries, :count) + + expect(json_response[:errors]).to containing_exactly( + hash_including( + message: "Redirect to '#{redirect}' not allowed.", + extensions: { code: 'USER_ERROR' } + ) + ) + end + end + context 'when unconfirmed_email is present' do let(:user) { create(:user, :confirmed, unconfirmed_email: 'vvega@wallaceinc.com') } @@ -81,7 +96,7 @@ context 'when using the admin model' do let(:admin) { create(:admin, confirmed_at: nil) } - let(:redirect) { Faker::Internet.url } + let(:redirect) { 'https://google.com' } let(:query) do <<-GRAPHQL {