From 21ad21cb507d7a5f48ef8ee726b2f9308052aa9d Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Fri, 12 Oct 2018 00:15:55 +0200 Subject: Improve signature verification safeguards (#8959) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Downcase signed_headers string before building the signed string The HTTP Signatures draft does not mandate the “headers” field to be downcased, but mandates the header field names to be downcased in the signed string, which means that prior to this patch, Mastodon could fail to process signatures from some compliant clients. It also means that it would not actually check the Digest of non-compliant clients that wouldn't use a lowercased Digest field name. Thankfully, I don't know of any such client. * Revert "Remove dead code (#8919)" This reverts commit a00ce8c92c06f42109aad5cfe65d46862cf037bb. * Restore time window checking, change it to 12 hours By checking the Date header, we can prevent replaying old vulnerable signatures. The focus is to prevent replaying old vulnerable requests from software that has been fixed in the meantime, so a somewhat long window should be fine and accounts for timezone misconfiguration. * Escape users' URLs when formatting them Fixes possible HTML injection * Escape all string interpolations in Formatter class Slightly improve performance by reducing class allocations from repeated Formatter#encode calls * Fix code style issues --- .../concerns/signature_verification_spec.rb | 24 ++++++++++++++++++++++ 1 file changed, 24 insertions(+) (limited to 'spec/controllers') diff --git a/spec/controllers/concerns/signature_verification_spec.rb b/spec/controllers/concerns/signature_verification_spec.rb index 3daf1fc4e..720690097 100644 --- a/spec/controllers/concerns/signature_verification_spec.rb +++ b/spec/controllers/concerns/signature_verification_spec.rb @@ -73,6 +73,30 @@ describe ApplicationController, type: :controller do end end + context 'with request older than a day' do + before do + get :success + + fake_request = Request.new(:get, request.url) + fake_request.add_headers({ 'Date' => 2.days.ago.utc.httpdate }) + fake_request.on_behalf_of(author) + + request.headers.merge!(fake_request.headers) + end + + describe '#signed_request?' do + it 'returns true' do + expect(controller.signed_request?).to be true + end + end + + describe '#signed_request_account' do + it 'returns nil' do + expect(controller.signed_request_account).to be_nil + end + end + end + context 'with body' do before do post :success, body: 'Hello world' -- cgit From d5bfba3262cf531d767f583a79fc2fbe8bff93b4 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Sat, 20 Oct 2018 07:32:26 +0200 Subject: Do not test PAM authentication by default (#9027) * Do not test PAM authentication by default * Disable PAM tests if PAM is not enabled --- .env.test | 4 -- spec/controllers/auth/sessions_controller_spec.rb | 74 ++++++++++++----------- 2 files changed, 38 insertions(+), 40 deletions(-) (limited to 'spec/controllers') diff --git a/.env.test b/.env.test index 726351c5e..fa4e1d91f 100644 --- a/.env.test +++ b/.env.test @@ -3,7 +3,3 @@ NODE_ENV=test # Federation LOCAL_DOMAIN=cb6e6126.ngrok.io LOCAL_HTTPS=true -# test pam authentication -PAM_ENABLED=true -PAM_DEFAULT_SERVICE=pam_test -PAM_CONTROLLED_SERVICE=pam_test_controlled diff --git a/spec/controllers/auth/sessions_controller_spec.rb b/spec/controllers/auth/sessions_controller_spec.rb index b4f912717..86fed7b8b 100644 --- a/spec/controllers/auth/sessions_controller_spec.rb +++ b/spec/controllers/auth/sessions_controller_spec.rb @@ -55,53 +55,55 @@ RSpec.describe Auth::SessionsController, type: :controller do request.env['devise.mapping'] = Devise.mappings[:user] end - context 'using PAM authentication' do - context 'using a valid password' do - before do - post :create, params: { user: { email: "pam_user1", password: '123456' } } - end + if ENV['PAM_ENABLED'] == 'true' + context 'using PAM authentication' do + context 'using a valid password' do + before do + post :create, params: { user: { email: "pam_user1", password: '123456' } } + end - it 'redirects to home' do - expect(response).to redirect_to(root_path) - end + it 'redirects to home' do + expect(response).to redirect_to(root_path) + end - it 'logs the user in' do - expect(controller.current_user).to be_instance_of(User) + it 'logs the user in' do + expect(controller.current_user).to be_instance_of(User) + end end - end - context 'using an invalid password' do - before do - post :create, params: { user: { email: "pam_user1", password: 'WRONGPW' } } - end + context 'using an invalid password' do + before do + post :create, params: { user: { email: "pam_user1", password: 'WRONGPW' } } + end - it 'shows a login error' do - expect(flash[:alert]).to match I18n.t('devise.failure.invalid', authentication_keys: 'Email') - end + it 'shows a login error' do + expect(flash[:alert]).to match I18n.t('devise.failure.invalid', authentication_keys: 'Email') + end - it "doesn't log the user in" do - expect(controller.current_user).to be_nil + it "doesn't log the user in" do + expect(controller.current_user).to be_nil + end end - end - context 'using a valid email and existing user' do - let(:user) do - account = Fabricate.build(:account, username: 'pam_user1') - account.save!(validate: false) - user = Fabricate(:user, email: 'pam@example.com', password: nil, account: account) - user - end + context 'using a valid email and existing user' do + let(:user) do + account = Fabricate.build(:account, username: 'pam_user1') + account.save!(validate: false) + user = Fabricate(:user, email: 'pam@example.com', password: nil, account: account) + user + end - before do - post :create, params: { user: { email: user.email, password: '123456' } } - end + before do + post :create, params: { user: { email: user.email, password: '123456' } } + end - it 'redirects to home' do - expect(response).to redirect_to(root_path) - end + it 'redirects to home' do + expect(response).to redirect_to(root_path) + end - it 'logs the user in' do - expect(controller.current_user).to eq user + it 'logs the user in' do + expect(controller.current_user).to eq user + end end end end -- cgit From 33976c8ecc6bd5e75b918737b224c9d4388f6516 Mon Sep 17 00:00:00 2001 From: takayamaki Date: Sun, 21 Oct 2018 00:28:04 +0900 Subject: fix: Execute PAM authentication tests on CircleCI (#9029) and use 'if' option of context block --- .circleci/config.yml | 3 + spec/controllers/auth/sessions_controller_spec.rb | 74 +++++++++++------------ 2 files changed, 39 insertions(+), 38 deletions(-) (limited to 'spec/controllers') diff --git a/.circleci/config.yml b/.circleci/config.yml index 20688b8e9..674d1b02d 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -13,6 +13,9 @@ aliases: ALLOW_NOPAM: true CONTINUOUS_INTEGRATION: true DISABLE_SIMPLECOV: true + PAM_ENABLED: true + PAM_DEFAULT_SERVICE: pam_test + PAM_CONTROLLED_SERVICE: pam_test_controlled working_directory: ~/projects/mastodon/ - &attach_workspace diff --git a/spec/controllers/auth/sessions_controller_spec.rb b/spec/controllers/auth/sessions_controller_spec.rb index 86fed7b8b..71fcc1a6e 100644 --- a/spec/controllers/auth/sessions_controller_spec.rb +++ b/spec/controllers/auth/sessions_controller_spec.rb @@ -55,55 +55,53 @@ RSpec.describe Auth::SessionsController, type: :controller do request.env['devise.mapping'] = Devise.mappings[:user] end - if ENV['PAM_ENABLED'] == 'true' - context 'using PAM authentication' do - context 'using a valid password' do - before do - post :create, params: { user: { email: "pam_user1", password: '123456' } } - end + context 'using PAM authentication', if: ENV['PAM_ENABLED'] == 'true' do + context 'using a valid password' do + before do + post :create, params: { user: { email: "pam_user1", password: '123456' } } + end - it 'redirects to home' do - expect(response).to redirect_to(root_path) - end + it 'redirects to home' do + expect(response).to redirect_to(root_path) + end - it 'logs the user in' do - expect(controller.current_user).to be_instance_of(User) - end + it 'logs the user in' do + expect(controller.current_user).to be_instance_of(User) end + end - context 'using an invalid password' do - before do - post :create, params: { user: { email: "pam_user1", password: 'WRONGPW' } } - end + context 'using an invalid password' do + before do + post :create, params: { user: { email: "pam_user1", password: 'WRONGPW' } } + end - it 'shows a login error' do - expect(flash[:alert]).to match I18n.t('devise.failure.invalid', authentication_keys: 'Email') - end + it 'shows a login error' do + expect(flash[:alert]).to match I18n.t('devise.failure.invalid', authentication_keys: 'Email') + end - it "doesn't log the user in" do - expect(controller.current_user).to be_nil - end + it "doesn't log the user in" do + expect(controller.current_user).to be_nil end + end - context 'using a valid email and existing user' do - let(:user) do - account = Fabricate.build(:account, username: 'pam_user1') - account.save!(validate: false) - user = Fabricate(:user, email: 'pam@example.com', password: nil, account: account) - user - end + context 'using a valid email and existing user' do + let(:user) do + account = Fabricate.build(:account, username: 'pam_user1') + account.save!(validate: false) + user = Fabricate(:user, email: 'pam@example.com', password: nil, account: account) + user + end - before do - post :create, params: { user: { email: user.email, password: '123456' } } - end + before do + post :create, params: { user: { email: user.email, password: '123456' } } + end - it 'redirects to home' do - expect(response).to redirect_to(root_path) - end + it 'redirects to home' do + expect(response).to redirect_to(root_path) + end - it 'logs the user in' do - expect(controller.current_user).to eq user - end + it 'logs the user in' do + expect(controller.current_user).to eq user end end end -- cgit