Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Override CSP for ThreatMetrix based on feature-specific config #11678

Merged
merged 1 commit into from
Jan 6, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions app/controllers/concerns/threat_metrix_concern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,6 @@ module ThreatMetrixConcern
THREAT_METRIX_WILDCARD_DOMAIN = '*.online-metrix.net'

def override_csp_for_threat_metrix
return unless FeatureManagement.proofing_device_profiling_collecting_enabled?

threat_metrix_csp_overrides
end

def threat_metrix_csp_overrides
policy = current_content_security_policy

# ThreatMetrix requires additional Content Security Policy (CSP)
Expand Down
3 changes: 2 additions & 1 deletion app/controllers/idv/in_person/ssn_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ class SsnController < ApplicationController
before_action :confirm_not_rate_limited_after_doc_auth
before_action :confirm_in_person_address_step_complete
before_action :confirm_repeat_ssn, only: :show
before_action :override_csp_for_threat_metrix
before_action :override_csp_for_threat_metrix,
if: -> { FeatureManagement.proofing_device_profiling_collecting_enabled? }

attr_reader :ssn_presenter

Expand Down
3 changes: 2 additions & 1 deletion app/controllers/idv/ssn_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ class SsnController < ApplicationController

before_action :confirm_not_rate_limited_after_doc_auth
before_action :confirm_step_allowed
before_action :override_csp_for_threat_metrix
before_action :override_csp_for_threat_metrix,
if: -> { FeatureManagement.proofing_device_profiling_collecting_enabled? }

attr_reader :ssn_presenter

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ class TwoFactorAuthenticationSetupController < ApplicationController
before_action :authenticate_user
before_action :confirm_user_authenticated_for_2fa_setup
before_action :check_if_possible_piv_user
before_action :override_csp_for_threat_metrix
before_action :override_csp_for_threat_metrix,
if: -> { FeatureManagement.account_creation_device_profiling_collecting_enabled? }

delegate :enabled_mfa_methods_count, to: :mfa_context

Expand Down
64 changes: 23 additions & 41 deletions spec/controllers/concerns/threat_metrix_concern_spec.rb
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Easier to review this file with whitespace changes hidden: https://github.com/18F/identity-idp/pull/11678/files?w=1

Original file line number Diff line number Diff line change
Expand Up @@ -12,60 +12,42 @@ def index; end
end

describe '#override_csp_for_threat_metrix' do
let(:ff_enabled) { true }
it 'modifies CSP headers' do
get :index

before do
allow(IdentityConfig.store).to receive(:proofing_device_profiling)
.and_return(ff_enabled ? :enabled : :disabled)
end

context 'ff is set' do
it 'modifies CSP headers' do
get :index
csp = response.request.content_security_policy

csp = response.request.content_security_policy
aggregate_failures do
expect(csp.directives['script-src']).to include('h.online-metrix.net')
expect(csp.directives['script-src']).to include("'unsafe-eval'")

aggregate_failures do
expect(csp.directives['script-src']).to include('h.online-metrix.net')
expect(csp.directives['script-src']).to include("'unsafe-eval'")
expect(csp.directives['style-src']).to include("'unsafe-inline'")

expect(csp.directives['style-src']).to include("'unsafe-inline'")
expect(csp.directives['child-src']).to include('h.online-metrix.net')

expect(csp.directives['child-src']).to include('h.online-metrix.net')
expect(csp.directives['connect-src']).to include('h.online-metrix.net')

expect(csp.directives['connect-src']).to include('h.online-metrix.net')

expect(csp.directives['img-src']).to include('*.online-metrix.net')
end
expect(csp.directives['img-src']).to include('*.online-metrix.net')
end
end

context 'with content security policy directives for style-src' do
let(:csp_nonce_directives) { ['style-src'] }

before do
request.content_security_policy_nonce_directives = csp_nonce_directives
end
context 'with content security policy directives for style-src' do
let(:csp_nonce_directives) { ['style-src'] }

it 'removes style-src nonce directive to allow all unsafe inline styles' do
get :index
before do
request.content_security_policy_nonce_directives = csp_nonce_directives
end

csp = parse_content_security_policy
it 'removes style-src nonce directive to allow all unsafe inline styles' do
get :index

expect(csp['style-src']).to_not include(/'nonce-.+'/)
csp = parse_content_security_policy

# Ensure that the default configuration is not mutated as a result of the request-specific
# revisions to the content security policy.
expect(csp_nonce_directives).to eq(['style-src'])
end
end
end
expect(csp['style-src']).to_not include(/'nonce-.+'/)

context 'ff is not set' do
let(:ff_enabled) { false }
it 'does not modify CSP headers' do
get :index
secure_header_config = response.request.headers.env['secure_headers_request_config']
expect(secure_header_config).to be_nil
# Ensure that the default configuration is not mutated as a result of the request-specific
# revisions to the content security policy.
expect(csp_nonce_directives).to eq(['style-src'])
end
end
end
Expand Down
40 changes: 34 additions & 6 deletions spec/controllers/idv/in_person/ssn_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@

before do
stub_sign_in(user)
subject.user_session['idv/in_person'] = flow_session
controller.user_session['idv/in_person'] = flow_session
stub_analytics
subject.idv_session.flow_path = 'standard'
controller.idv_session.flow_path = 'standard'
end

describe '#step_info' do
Expand All @@ -36,6 +36,8 @@
end

describe '#show' do
subject(:response) { get :show }

let(:analytics_name) { 'IdV: doc auth ssn visited' }
let(:analytics_args) do
{
Expand Down Expand Up @@ -66,18 +68,18 @@
end

it 'adds a threatmetrix session id to idv_session' do
expect { get :show }.to change { subject.idv_session.threatmetrix_session_id }.from(nil)
expect { get :show }.to change { controller.idv_session.threatmetrix_session_id }.from(nil)
end

it 'does not change threatmetrix_session_id when updating ssn' do
subject.idv_session.ssn = ssn
expect { get :show }.not_to change { subject.idv_session.threatmetrix_session_id }
controller.idv_session.ssn = ssn
expect { get :show }.not_to change { controller.idv_session.threatmetrix_session_id }
end

context 'with an ssn in idv_session' do
let(:referer) { idv_in_person_address_url }
before do
subject.idv_session.ssn = ssn
controller.idv_session.ssn = ssn
request.env['HTTP_REFERER'] = referer
end

Expand All @@ -98,6 +100,32 @@
end
end
end

context 'with ThreatMetrix profiling disabled' do
before do
allow(FeatureManagement).to receive(:proofing_device_profiling_collecting_enabled?)
.and_return(false)
end

it 'does not override CSPs for ThreatMetrix' do
expect(controller).not_to receive(:override_csp_for_threat_metrix)

response
end
end

context 'with ThreatMetrix profiling enabled' do
before do
allow(FeatureManagement).to receive(:proofing_device_profiling_collecting_enabled?)
.and_return(true)
end

it 'overrides CSPs for ThreatMetrix' do
expect(controller).to receive(:override_csp_for_threat_metrix)

response
end
end
end

describe '#update' do
Expand Down
60 changes: 30 additions & 30 deletions spec/controllers/idv/ssn_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

before do
stub_sign_in(user)
stub_up_to(:document_capture, idv_session: subject.idv_session)
stub_up_to(:document_capture, idv_session: controller.idv_session)
stub_analytics
end

Expand All @@ -33,16 +33,11 @@
:check_for_mail_only_outage,
)
end

it 'overrides CSPs for ThreatMetrix' do
expect(subject).to have_actions(
:before,
:override_csp_for_threat_metrix,
)
end
end

describe '#show' do
subject(:response) { get :show }

let(:analytics_name) { 'IdV: doc auth ssn visited' }
let(:analytics_args) do
{
Expand Down Expand Up @@ -73,25 +68,25 @@
end

it 'adds a threatmetrix session id to idv_session' do
expect { get :show }.to change { subject.idv_session.threatmetrix_session_id }.from(nil)
expect { get :show }.to change { controller.idv_session.threatmetrix_session_id }.from(nil)
end

context 'when updating ssn' do
let(:threatmetrix_session_id) { 'original-session-id' }

before do
subject.idv_session.ssn = ssn
subject.idv_session.threatmetrix_session_id = threatmetrix_session_id
controller.idv_session.ssn = ssn
controller.idv_session.threatmetrix_session_id = threatmetrix_session_id
end
it 'does not change threatmetrix_session_id' do
expect { get :show }.not_to change { subject.idv_session.threatmetrix_session_id }
expect { get :show }.not_to change { controller.idv_session.threatmetrix_session_id }
end

context 'but there is no threatmetrix_session_id in the session' do
let(:threatmetrix_session_id) { nil }

it 'sets a threatmetrix_session_id' do
expect { get :show }.to change { subject.idv_session.threatmetrix_session_id }
expect { get :show }.to change { controller.idv_session.threatmetrix_session_id }
end
end
end
Expand All @@ -102,22 +97,22 @@
end

it 'still add a threatmetrix session id to idv_session' do
expect { get :show }.to change { subject.idv_session.threatmetrix_session_id }.from(nil)
expect { get :show }.to change { controller.idv_session.threatmetrix_session_id }.from(nil)
end

context 'when idv_session has a threatmetrix_session_id' do
before do
subject.idv_session.threatmetrix_session_id = 'fake-session-id'
controller.idv_session.threatmetrix_session_id = 'fake-session-id'
end
it 'changes the threatmetrix_session_id' do
expect { get :show }.to change { subject.idv_session.threatmetrix_session_id }
expect { get :show }.to change { controller.idv_session.threatmetrix_session_id }
end
end
end

context 'with an ssn in idv_session' do
before do
subject.idv_session.ssn = ssn
controller.idv_session.ssn = ssn
end

it 'does not redirect and allows the back button' do
Expand All @@ -127,24 +122,29 @@
end
end

it 'overrides Content Security Policies for ThreatMetrix' do
allow(IdentityConfig.store).to receive(:proofing_device_profiling)
.and_return(:enabled)
get :show

csp = response.request.content_security_policy
context 'with ThreatMetrix profiling disabled' do
before do
allow(FeatureManagement).to receive(:proofing_device_profiling_collecting_enabled?)
.and_return(false)
end

aggregate_failures do
expect(csp.directives['script-src']).to include('h.online-metrix.net')
expect(csp.directives['script-src']).to include("'unsafe-eval'")
it 'does not override CSPs for ThreatMetrix' do
expect(controller).not_to receive(:override_csp_for_threat_metrix)

expect(csp.directives['style-src']).to include("'unsafe-inline'")
response
end
end

expect(csp.directives['child-src']).to include('h.online-metrix.net')
context 'with ThreatMetrix profiling enabled' do
before do
allow(FeatureManagement).to receive(:proofing_device_profiling_collecting_enabled?)
.and_return(true)
end

expect(csp.directives['connect-src']).to include('h.online-metrix.net')
it 'overrides CSPs for ThreatMetrix' do
expect(controller).to receive(:override_csp_for_threat_metrix)

expect(csp.directives['img-src']).to include('*.online-metrix.net')
response
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,19 @@
expect(assigns(:presenter).desktop_ft_ab_test).to be false
end

context 'with threatmetrix disabled' do
before do
allow(FeatureManagement).to receive(:proofing_device_profiling_collecting_enabled?)
.and_return(false)
end

it 'does not override CSPs for ThreatMetrix' do
expect(controller).not_to receive(:override_csp_for_threat_metrix)

response
end
end

context 'with threatmetrix enabled' do
let(:tmx_session_id) { '1234' }

Expand All @@ -52,6 +65,12 @@

expect(response).to render_template(:index)
end

it 'overrides CSPs for ThreatMetrix' do
expect(controller).to receive(:override_csp_for_threat_metrix)

response
end
end

context 'with user having gov or mil email' do
Expand Down