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.

Sign up for GitHub

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

Add resource hints for reCAPTCHA #10616

Merged
merged 2 commits into from
May 14, 2024
Merged

Add resource hints for reCAPTCHA #10616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4b9b0b6
Add resource hints for reCAPTCHA
aduth May 13, 2024
aa0cb5a
Remove redundant chomp
aduth May 14, 2024
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
8 changes: 8 additions & 0 deletions app/controllers/concerns/recaptcha_concern.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,14 @@ module RecaptchaConcern
'https://recaptcha.google.com/recaptcha/',
].freeze

def add_recaptcha_resource_hints
response.headers['Link'] = [
response.headers['Link'],
'<https://www.google.com>;rel=preconnect',
'<https://www.gstatic.com>;rel=preconnect;crossorigin',
Comment on lines +17 to +18
Copy link
Contributor Author

@aduth aduth May 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

One thing I didn't check previously is whether it's valid to omit the quotes on the values here, since both Rails and related MDN resources include them.

I confirmed this in the relevant spec: https://httpwg.org/specs/rfc8288.html#header

The ABNF for the field value is:

 Link       = #link-value
 link-value = "<" URI-Reference ">" *( OWS ";" OWS link-param )
 link-param = token BWS [ "=" BWS ( token / quoted-string ) ]

Note that any link-param can be generated with values using either the token or the quoted-string syntax; therefore, recipients MUST be able to parse both forms. In other words, the following parameters are equivalent:

 x=y
 x="y"

Adding on to my comment at #10612 (comment), this could be another micro-optimization if we wanted to fully control the resource hint header and optimize its size. Edit: Turns out, Rails omits them too!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also validated them as working in Safari (related blog post):

image

].compact.join(',')
end

def allow_csp_recaptcha_src
policy = current_content_security_policy
policy.script_src(*policy.script_src, *RECAPTCHA_SCRIPT_SRC)
Expand Down
2 changes: 2 additions & 0 deletions app/controllers/users/phone_setup_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ class PhoneSetupController < ApplicationController
before_action :confirm_recently_authenticated_2fa
before_action :check_max_phone_numbers_per_account, only: %i[index create]

after_action :add_recaptcha_resource_hints, if: :recaptcha_enabled?

helper_method :in_multi_mfa_selection_flow?

def index
Expand Down
62 changes: 55 additions & 7 deletions spec/controllers/concerns/recaptcha_concern_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
require 'rails_helper'

RSpec.describe RecaptchaConcern, type: :controller do
controller ApplicationController do
include RecaptchaConcern
describe '#allow_csp_recaptcha_src' do
controller ApplicationController do
include RecaptchaConcern

before_action :allow_csp_recaptcha_src
before_action :allow_csp_recaptcha_src

def index
render plain: ''
def index
render plain: ''
end
end
end

describe '#allow_csp_recaptcha_src' do
it 'overrides csp to add directives for recaptcha' do
get :index

Expand All @@ -20,4 +20,52 @@ def index
expect(csp.frame_src).to include(*RecaptchaConcern::RECAPTCHA_FRAME_SRC)
end
end

describe '#add_recaptcha_resource_hints' do
controller ApplicationController do
include RecaptchaConcern

after_action :add_recaptcha_resource_hints

def index
if params[:add_link]
response.headers['Link'] = '<https://example.com>;rel=preconnect'
end

render plain: ''
end
end

subject(:response) { get :index }
let(:processed_links) do
response.headers['Link'].split(',').map { |link| link.split(';').map(&:chomp) }
end

it 'adds resource hints for recaptcha to response headers' do
response

expect(processed_links).to match_array(
[
['<https://www.google.com>', 'rel=preconnect'],
['<https://www.gstatic.com>', 'rel=preconnect', 'crossorigin'],
],
)
end

context 'with existing link header' do
subject(:response) { get :index, params: { add_link: '' } }

it 'appends new resource hints' do
response

expect(processed_links).to match_array(
[
['<https://example.com>', 'rel=preconnect'],
['<https://www.google.com>', 'rel=preconnect'],
['<https://www.gstatic.com>', 'rel=preconnect', 'crossorigin'],
],
)
end
end
end
end
22 changes: 22 additions & 0 deletions spec/controllers/users/phone_setup_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,4 +292,26 @@
end
end
end

describe 'after actions' do
before { stub_sign_in }

it 'does not add recaptcha resource hints' do
expect(subject).not_to receive(:add_recaptcha_resource_hints)

get :index
end

context 'recaptcha enabled' do
before do
allow(FeatureManagement).to receive(:phone_recaptcha_enabled?).and_return(true)
end

it 'adds recaptcha resource hints' do
expect(subject).to receive(:add_recaptcha_resource_hints)

get :index
end
end
end
end