From 1b493c9fee954b5bd4c4b00f9f945a5d97e2d699 Mon Sep 17 00:00:00 2001 From: Claire Date: Mon, 24 Jan 2022 19:06:19 +0100 Subject: [PATCH 1/8] Add optional hCaptcha support Fixes #1649 This requires setting `HCAPTCHA_SECRET_KEY` and `HCAPTCHA_SITE_KEY`, then enabling the admin setting at `/admin/settings/edit#form_admin_settings_captcha_enabled` Subsequently, a hCaptcha widget will be displayed on `/about` and `/auth/sign_up` unless: - the user is already signed-up already - the user has used an invite link - the user has already solved the captcha (and registration failed for another reason) The Content-Security-Policy headers are altered automatically to allow the third-party hCaptcha scripts on `/about` and `/auth/sign_up` following the same rules as above. --- .env.production.sample | 4 ++ Gemfile | 2 + Gemfile.lock | 3 + app/controllers/about_controller.rb | 2 + app/controllers/api/v1/accounts_controller.rb | 4 +- .../auth/registrations_controller.rb | 17 +++++ app/controllers/concerns/captcha_concern.rb | 66 +++++++++++++++++++ app/helpers/admin/settings_helper.rb | 4 ++ .../flavours/glitch/styles/forms.scss | 4 ++ app/models/form/admin_settings.rb | 2 + app/views/about/_registration.html.haml | 3 + app/views/admin/settings/edit.html.haml | 5 +- app/views/auth/registrations/new.html.haml | 3 + config/locales-glitch/en.yml | 3 + config/settings.yml | 1 + 15 files changed, 121 insertions(+), 2 deletions(-) create mode 100644 app/controllers/concerns/captcha_concern.rb diff --git a/.env.production.sample b/.env.production.sample index 13e89b40d..7de5e00f4 100644 --- a/.env.production.sample +++ b/.env.production.sample @@ -285,3 +285,7 @@ MAX_POLL_OPTION_CHARS=100 # Units are in bytes MAX_EMOJI_SIZE=51200 MAX_REMOTE_EMOJI_SIZE=204800 + +# Optional hCaptcha support +# HCAPTCHA_SECRET_KEY= +# HCAPTCHA_SITE_KEY= diff --git a/Gemfile b/Gemfile index eae5f11b7..282ab65e4 100644 --- a/Gemfile +++ b/Gemfile @@ -156,3 +156,5 @@ gem 'concurrent-ruby', require: false gem 'connection_pool', require: false gem 'xorcist', '~> 1.1' + +gem "hcaptcha", "~> 7.1" diff --git a/Gemfile.lock b/Gemfile.lock index 8d72732eb..cc9a53e41 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -271,6 +271,8 @@ GEM railties (>= 4.0.1) hashdiff (1.0.1) hashie (4.1.0) + hcaptcha (7.1.0) + json highline (2.0.3) hiredis (0.6.3) hkdf (0.3.0) @@ -719,6 +721,7 @@ DEPENDENCIES fog-openstack (~> 0.3) fuubar (~> 2.5) hamlit-rails (~> 0.2) + hcaptcha (~> 7.1) hiredis (~> 0.6) htmlentities (~> 4.3) http (~> 5.0) diff --git a/app/controllers/about_controller.rb b/app/controllers/about_controller.rb index 620c0ff78..5a35dbbcb 100644 --- a/app/controllers/about_controller.rb +++ b/app/controllers/about_controller.rb @@ -2,6 +2,7 @@ class AboutController < ApplicationController include RegistrationSpamConcern + include CaptchaConcern before_action :set_pack @@ -12,6 +13,7 @@ class AboutController < ApplicationController before_action :set_instance_presenter before_action :set_expires_in, only: [:more, :terms] before_action :set_registration_form_time, only: :show + before_action :extend_csp_for_captcha!, only: :show skip_before_action :require_functional!, only: [:more, :terms] diff --git a/app/controllers/api/v1/accounts_controller.rb b/app/controllers/api/v1/accounts_controller.rb index 5c47158e0..8916c3f96 100644 --- a/app/controllers/api/v1/accounts_controller.rb +++ b/app/controllers/api/v1/accounts_controller.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class Api::V1::AccountsController < Api::BaseController + include CaptchaConcern + before_action -> { authorize_if_got_token! :read, :'read:accounts' }, except: [:create, :follow, :unfollow, :remove_from_followers, :block, :unblock, :mute, :unmute] before_action -> { doorkeeper_authorize! :follow, :'write:follows' }, only: [:follow, :unfollow, :remove_from_followers] before_action -> { doorkeeper_authorize! :follow, :'write:mutes' }, only: [:mute, :unmute] @@ -83,7 +85,7 @@ class Api::V1::AccountsController < Api::BaseController end def check_enabled_registrations - forbidden if single_user_mode? || omniauth_only? || !allowed_registrations? + forbidden if single_user_mode? || omniauth_only? || !allowed_registrations? || captcha_enabled? end def allowed_registrations? diff --git a/app/controllers/auth/registrations_controller.rb b/app/controllers/auth/registrations_controller.rb index 6b1f3fa82..3c9b38a4b 100644 --- a/app/controllers/auth/registrations_controller.rb +++ b/app/controllers/auth/registrations_controller.rb @@ -2,6 +2,7 @@ class Auth::RegistrationsController < Devise::RegistrationsController include RegistrationSpamConcern + include CaptchaConcern layout :determine_layout @@ -15,6 +16,8 @@ class Auth::RegistrationsController < Devise::RegistrationsController before_action :require_not_suspended!, only: [:update] before_action :set_cache_headers, only: [:edit, :update] before_action :set_registration_form_time, only: :new + before_action :extend_csp_for_captcha!, only: [:new, :create] + before_action :check_captcha!, only: :create skip_before_action :require_functional!, only: [:edit, :update] @@ -135,4 +138,18 @@ class Auth::RegistrationsController < Devise::RegistrationsController def set_cache_headers response.headers['Cache-Control'] = 'no-cache, no-store, max-age=0, must-revalidate' end + + def sign_up(resource_name, resource) + clear_captcha! + super + end + + def check_captcha! + super do |error| + build_resource(sign_up_params) + resource.validate + resource.errors.add(:base, error) + respond_with resource + end + end end diff --git a/app/controllers/concerns/captcha_concern.rb b/app/controllers/concerns/captcha_concern.rb new file mode 100644 index 000000000..5a23e59e3 --- /dev/null +++ b/app/controllers/concerns/captcha_concern.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +module CaptchaConcern + extend ActiveSupport::Concern + include Hcaptcha::Adapters::ViewMethods + + CAPTCHA_TIMEOUT = 2.hours.freeze + + included do + helper_method :render_captcha_if_needed + end + + def captcha_available? + ENV['HCAPTCHA_SECRET_KEY'].present? && ENV['HCAPTCHA_SITE_KEY'].present? + end + + def captcha_enabled? + captcha_available? && Setting.captcha_enabled + end + + def captcha_recently_passed? + session[:captcha_passed_at].present? && session[:captcha_passed_at] >= CAPTCHA_TIMEOUT.ago + end + + def captcha_required? + captcha_enabled? && !current_user && !(@invite.present? && @invite.valid_for_use? && !@invite.max_uses.nil?) && !captcha_recently_passed? + end + + def clear_captcha! + session.delete(:captcha_passed_at) + end + + def check_captcha! + return true unless captcha_required? + + if verify_hcaptcha + session[:captcha_passed_at] = Time.now.utc + return true + else + if block_given? + message = flash[:hcaptcha_error] + flash.delete(:hcaptcha_error) + yield message + end + return false + end + end + + def extend_csp_for_captcha! + policy = request.content_security_policy + return unless captcha_required? && policy.present? + + %w(script_src frame_src style_src connect_src).each do |directive| + values = policy.send(directive) + values << 'https://hcaptcha.com' unless values.include?('https://hcaptcha.com') || values.include?('https:') + values << 'https://*.hcaptcha.com' unless values.include?('https://*.hcaptcha.com') || values.include?('https:') + policy.send(directive, *values) + end + end + + def render_captcha_if_needed + return unless captcha_required? + + hcaptcha_tags + end +end diff --git a/app/helpers/admin/settings_helper.rb b/app/helpers/admin/settings_helper.rb index baf14ab25..f99a2b8c8 100644 --- a/app/helpers/admin/settings_helper.rb +++ b/app/helpers/admin/settings_helper.rb @@ -8,4 +8,8 @@ module Admin::SettingsHelper link = link_to t('admin.site_uploads.delete'), admin_site_upload_path(upload), data: { method: :delete } safe_join([hint, link], '
'.html_safe) end + + def captcha_available? + ENV['HCAPTCHA_SECRET_KEY'].present? && ENV['HCAPTCHA_SITE_KEY'].present? + end end diff --git a/app/javascript/flavours/glitch/styles/forms.scss b/app/javascript/flavours/glitch/styles/forms.scss index 3433abcdd..64d441fb2 100644 --- a/app/javascript/flavours/glitch/styles/forms.scss +++ b/app/javascript/flavours/glitch/styles/forms.scss @@ -1058,3 +1058,7 @@ code { display: none; } } + +.simple_form .h-captcha { + text-align: center; +} diff --git a/app/models/form/admin_settings.rb b/app/models/form/admin_settings.rb index 3202d1fc2..34f14e312 100644 --- a/app/models/form/admin_settings.rb +++ b/app/models/form/admin_settings.rb @@ -40,6 +40,7 @@ class Form::AdminSettings noindex outgoing_spoilers require_invite_text + captcha_enabled ).freeze BOOLEAN_KEYS = %i( @@ -58,6 +59,7 @@ class Form::AdminSettings trendable_by_default noindex require_invite_text + captcha_enabled ).freeze UPLOAD_KEYS = %i( diff --git a/app/views/about/_registration.html.haml b/app/views/about/_registration.html.haml index e4d614d71..5bb5d08a2 100644 --- a/app/views/about/_registration.html.haml +++ b/app/views/about/_registration.html.haml @@ -21,6 +21,9 @@ .fields-group = f.input :agreement, as: :boolean, wrapper: :with_label, label: t('auth.checkbox_agreement_html', rules_path: about_more_path, terms_path: terms_path), required: true, disabled: closed_registrations? + .fields-group + = render_captcha_if_needed + .actions = f.button :button, sign_up_message, type: :submit, class: 'button button-primary', disabled: closed_registrations? diff --git a/app/views/admin/settings/edit.html.haml b/app/views/admin/settings/edit.html.haml index b9daae8f0..49b03a9e3 100644 --- a/app/views/admin/settings/edit.html.haml +++ b/app/views/admin/settings/edit.html.haml @@ -42,7 +42,10 @@ .fields-group = f.input :require_invite_text, as: :boolean, wrapper: :with_label, label: t('admin.settings.registrations.require_invite_text.title'), hint: t('admin.settings.registrations.require_invite_text.desc_html'), disabled: !approved_registrations? - .fields-group + + - if captcha_available? + .fields-group + = f.input :captcha_enabled, as: :boolean, wrapper: :with_label, label: t('admin.settings.captcha_enabled.title'), hint: t('admin.settings.captcha_enabled.desc_html') %hr.spacer/ diff --git a/app/views/auth/registrations/new.html.haml b/app/views/auth/registrations/new.html.haml index 6981195ed..5cb558297 100644 --- a/app/views/auth/registrations/new.html.haml +++ b/app/views/auth/registrations/new.html.haml @@ -38,6 +38,9 @@ .fields-group = f.input :agreement, as: :boolean, wrapper: :with_label, label: whitelist_mode? ? t('auth.checkbox_agreement_without_rules_html', terms_path: terms_path) : t('auth.checkbox_agreement_html', rules_path: about_more_path, terms_path: terms_path), required: true + .field-group + = render_captcha_if_needed + .actions = f.button :button, @invite.present? ? t('auth.register') : sign_up_message, type: :submit diff --git a/config/locales-glitch/en.yml b/config/locales-glitch/en.yml index 5cc2625fc..c96f21c92 100644 --- a/config/locales-glitch/en.yml +++ b/config/locales-glitch/en.yml @@ -2,6 +2,9 @@ en: admin: settings: + captcha_enabled: + desc_html: Enable hCaptcha integration, requiring new users to solve a challenge when signing up. Note that this disables app-based registration, and requires third-party scripts from hCaptcha to be embedded in the registration pages. This may have security and privacy concerns. + title: Require new users to go through a CAPTCHA to sign up enable_keybase: desc_html: Allow your users to prove their identity via keybase title: Enable keybase integration diff --git a/config/settings.yml b/config/settings.yml index 094209822..7d192f369 100644 --- a/config/settings.yml +++ b/config/settings.yml @@ -77,6 +77,7 @@ defaults: &defaults show_domain_blocks_rationale: 'disabled' outgoing_spoilers: '' require_invite_text: false + captcha_enabled: false development: <<: *defaults From 04050fbd46d7758873b33dd6f5648cc37183b078 Mon Sep 17 00:00:00 2001 From: Claire Date: Mon, 24 Jan 2022 21:29:50 +0100 Subject: [PATCH 2/8] Please CodeClimate --- Gemfile | 2 +- app/controllers/concerns/captcha_concern.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Gemfile b/Gemfile index 282ab65e4..67c50d19f 100644 --- a/Gemfile +++ b/Gemfile @@ -157,4 +157,4 @@ gem 'connection_pool', require: false gem 'xorcist', '~> 1.1' -gem "hcaptcha", "~> 7.1" +gem 'hcaptcha', '~> 7.1' diff --git a/app/controllers/concerns/captcha_concern.rb b/app/controllers/concerns/captcha_concern.rb index 5a23e59e3..5bc4ba920 100644 --- a/app/controllers/concerns/captcha_concern.rb +++ b/app/controllers/concerns/captcha_concern.rb @@ -35,14 +35,14 @@ module CaptchaConcern if verify_hcaptcha session[:captcha_passed_at] = Time.now.utc - return true + true else if block_given? message = flash[:hcaptcha_error] flash.delete(:hcaptcha_error) yield message end - return false + false end end From 3f6a36168fd74e932a301923ea3805c1e848d66e Mon Sep 17 00:00:00 2001 From: Claire Date: Mon, 24 Jan 2022 21:36:22 +0100 Subject: [PATCH 3/8] Fix tests --- spec/views/about/show.html.haml_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/views/about/show.html.haml_spec.rb b/spec/views/about/show.html.haml_spec.rb index d608bbf5d..12c96ea49 100644 --- a/spec/views/about/show.html.haml_spec.rb +++ b/spec/views/about/show.html.haml_spec.rb @@ -10,6 +10,7 @@ describe 'about/show.html.haml', without_verify_partial_doubles: true do allow(view).to receive(:site_title).and_return('example site') allow(view).to receive(:new_user).and_return(User.new) allow(view).to receive(:use_seamless_external_login?).and_return(false) + allow(view).to receive(:render_captcha_if_needed).and_return(nil) end it 'has valid open graph tags' do From 6a2f248fe4ed59f512dd318a006209fb7b71aa7e Mon Sep 17 00:00:00 2001 From: Claire Date: Mon, 24 Jan 2022 21:52:45 +0100 Subject: [PATCH 4/8] Renew Rails session ID on successful registration --- app/controllers/auth/registrations_controller.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/controllers/auth/registrations_controller.rb b/app/controllers/auth/registrations_controller.rb index 3c9b38a4b..0db9cb84d 100644 --- a/app/controllers/auth/registrations_controller.rb +++ b/app/controllers/auth/registrations_controller.rb @@ -141,6 +141,11 @@ class Auth::RegistrationsController < Devise::RegistrationsController def sign_up(resource_name, resource) clear_captcha! + + old_session_values = session.to_hash + reset_session + session.update old_session_values.except('session_id') + super end From bf351d72af76e80a29674770d9991de955f95d34 Mon Sep 17 00:00:00 2001 From: Claire Date: Mon, 24 Jan 2022 22:12:57 +0100 Subject: [PATCH 5/8] Disable captcha if registrations are disabled for various reasons --- app/controllers/concerns/captcha_concern.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/controllers/concerns/captcha_concern.rb b/app/controllers/concerns/captcha_concern.rb index 5bc4ba920..4a942c988 100644 --- a/app/controllers/concerns/captcha_concern.rb +++ b/app/controllers/concerns/captcha_concern.rb @@ -23,6 +23,8 @@ module CaptchaConcern end def captcha_required? + return false if ENV['OMNIAUTH_ONLY'] == 'true' + return false unless Setting.registrations_mode != 'none' || @invite&.valid_for_use? captcha_enabled? && !current_user && !(@invite.present? && @invite.valid_for_use? && !@invite.max_uses.nil?) && !captcha_recently_passed? end From a9269f8786033eecf0ad307c75f5717c5ab468a2 Mon Sep 17 00:00:00 2001 From: Claire Date: Tue, 25 Jan 2022 13:54:11 +0100 Subject: [PATCH 6/8] Disable `registrations` flag in /api/v1/instance when CAPTCHA is enabled This is to avoid apps trying and failing at using the registrations API, which does not let us require a CAPTCHA and cannot be clearly signaled as unavailable. --- app/serializers/rest/instance_serializer.rb | 6 +++++- config/locales-glitch/en.yml | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/app/serializers/rest/instance_serializer.rb b/app/serializers/rest/instance_serializer.rb index 48bbb55c8..94cc3ffe3 100644 --- a/app/serializers/rest/instance_serializer.rb +++ b/app/serializers/rest/instance_serializer.rb @@ -98,7 +98,7 @@ class REST::InstanceSerializer < ActiveModel::Serializer end def registrations - Setting.registrations_mode != 'none' && !Rails.configuration.x.single_user_mode + Setting.registrations_mode != 'none' && !Rails.configuration.x.single_user_mode && !captcha_enabled? end def approval_required @@ -114,4 +114,8 @@ class REST::InstanceSerializer < ActiveModel::Serializer def instance_presenter @instance_presenter ||= InstancePresenter.new end + + def captcha_enabled? + ENV['HCAPTCHA_SECRET_KEY'].present? && ENV['HCAPTCHA_SITE_KEY'].present? && Setting.captcha_enabled + end end diff --git a/config/locales-glitch/en.yml b/config/locales-glitch/en.yml index c96f21c92..9bd66c969 100644 --- a/config/locales-glitch/en.yml +++ b/config/locales-glitch/en.yml @@ -3,7 +3,7 @@ en: admin: settings: captcha_enabled: - desc_html: Enable hCaptcha integration, requiring new users to solve a challenge when signing up. Note that this disables app-based registration, and requires third-party scripts from hCaptcha to be embedded in the registration pages. This may have security and privacy concerns. + desc_html: Enable hCaptcha integration, requiring new users to solve a challenge when signing up. Note that this disables app-based registration, may prevent your instance from being listed as having open registrations, and requires third-party scripts from hCaptcha to be embedded in the registration pages. This may have security and privacy concerns. title: Require new users to go through a CAPTCHA to sign up enable_keybase: desc_html: Allow your users to prove their identity via keybase From 0fb907441c827cadc767641b29d5d2c0e554f7a4 Mon Sep 17 00:00:00 2001 From: Claire Date: Tue, 25 Jan 2022 22:37:12 +0100 Subject: [PATCH 7/8] Add ability to set hCaptcha either on registration form or on e-mail validation Upshot of CAPTCHA on e-mail validation is it does not need to break the in-band registration API. --- .../auth/confirmations_controller.rb | 50 +++++++++++++++++++ app/controllers/concerns/captcha_concern.rb | 12 ++++- app/models/form/admin_settings.rb | 4 +- app/serializers/rest/instance_serializer.rb | 2 +- app/views/admin/settings/edit.html.haml | 2 +- .../auth/confirmations/captcha.html.haml | 11 ++++ config/locales-glitch/en.yml | 17 +++++-- config/routes.rb | 1 + config/settings.yml | 2 +- 9 files changed, 91 insertions(+), 10 deletions(-) create mode 100644 app/views/auth/confirmations/captcha.html.haml diff --git a/app/controllers/auth/confirmations_controller.rb b/app/controllers/auth/confirmations_controller.rb index 0b5a2f3c9..e9a646f91 100644 --- a/app/controllers/auth/confirmations_controller.rb +++ b/app/controllers/auth/confirmations_controller.rb @@ -1,12 +1,18 @@ # frozen_string_literal: true class Auth::ConfirmationsController < Devise::ConfirmationsController + include CaptchaConcern + layout 'auth' before_action :set_body_classes before_action :set_pack + before_action :set_confirmation_user!, only: [:show, :confirm_captcha] before_action :require_unconfirmed! + before_action :extend_csp_for_captcha!, only: [:show, :confirm_captcha] + before_action :require_captcha_if_needed!, only: [:show] + skip_before_action :require_functional! def new @@ -15,8 +21,52 @@ class Auth::ConfirmationsController < Devise::ConfirmationsController resource.email = current_user.unconfirmed_email || current_user.email if user_signed_in? end + def show + clear_captcha! + + old_session_values = session.to_hash + reset_session + session.update old_session_values.except('session_id') + + super + end + + def confirm_captcha + check_captcha! do |message| + flash.now[:alert] = message + render :captcha + return + end + + show + end + private + def require_captcha_if_needed! + render :captcha if captcha_required? + end + + def set_confirmation_user! + # We need to reimplement looking up the user because + # Devise::ConfirmationsController#show looks up and confirms in one + # step. + confirmation_token = params[:confirmation_token] + return if confirmation_token.nil? + @confirmation_user = User.find_first_by_auth_conditions(confirmation_token: confirmation_token) + end + + def captcha_user_bypass? + return true if @confirmation_user.nil? || @confirmation_user.confirmed? + + invite = Invite.find(@confirmation_user.invite_id) if @confirmation_user.invite_id.present? + invite.present? && !invite.max_uses.nil? + end + + def captcha_context + 'email-confirmation' + end + def set_pack use_pack 'auth' end diff --git a/app/controllers/concerns/captcha_concern.rb b/app/controllers/concerns/captcha_concern.rb index 4a942c988..02069d205 100644 --- a/app/controllers/concerns/captcha_concern.rb +++ b/app/controllers/concerns/captcha_concern.rb @@ -15,17 +15,21 @@ module CaptchaConcern end def captcha_enabled? - captcha_available? && Setting.captcha_enabled + captcha_available? && Setting.captcha_mode == captcha_context end def captcha_recently_passed? session[:captcha_passed_at].present? && session[:captcha_passed_at] >= CAPTCHA_TIMEOUT.ago end + def captcha_user_bypass? + current_user.present? || (@invite.present? && @invite.valid_for_use? && !@invite.max_uses.nil?) + end + def captcha_required? return false if ENV['OMNIAUTH_ONLY'] == 'true' return false unless Setting.registrations_mode != 'none' || @invite&.valid_for_use? - captcha_enabled? && !current_user && !(@invite.present? && @invite.valid_for_use? && !@invite.max_uses.nil?) && !captcha_recently_passed? + captcha_enabled? && !captcha_user_bypass? && !captcha_recently_passed? end def clear_captcha! @@ -65,4 +69,8 @@ module CaptchaConcern hcaptcha_tags end + + def captcha_context + 'registration-form' + end end diff --git a/app/models/form/admin_settings.rb b/app/models/form/admin_settings.rb index 34f14e312..7abb0d6c6 100644 --- a/app/models/form/admin_settings.rb +++ b/app/models/form/admin_settings.rb @@ -40,7 +40,7 @@ class Form::AdminSettings noindex outgoing_spoilers require_invite_text - captcha_enabled + captcha_mode ).freeze BOOLEAN_KEYS = %i( @@ -59,7 +59,6 @@ class Form::AdminSettings trendable_by_default noindex require_invite_text - captcha_enabled ).freeze UPLOAD_KEYS = %i( @@ -83,6 +82,7 @@ class Form::AdminSettings validates :bootstrap_timeline_accounts, existing_username: { multiple: true } validates :show_domain_blocks, inclusion: { in: %w(disabled users all) } validates :show_domain_blocks_rationale, inclusion: { in: %w(disabled users all) } + validates :captcha_mode, inclusion: { in: %w(disabled registration-form email-confirmation) } def initialize(_attributes = {}) super diff --git a/app/serializers/rest/instance_serializer.rb b/app/serializers/rest/instance_serializer.rb index 94cc3ffe3..d343cca20 100644 --- a/app/serializers/rest/instance_serializer.rb +++ b/app/serializers/rest/instance_serializer.rb @@ -116,6 +116,6 @@ class REST::InstanceSerializer < ActiveModel::Serializer end def captcha_enabled? - ENV['HCAPTCHA_SECRET_KEY'].present? && ENV['HCAPTCHA_SITE_KEY'].present? && Setting.captcha_enabled + ENV['HCAPTCHA_SECRET_KEY'].present? && ENV['HCAPTCHA_SITE_KEY'].present? && Setting.captcha_mode == 'registration-form' end end diff --git a/app/views/admin/settings/edit.html.haml b/app/views/admin/settings/edit.html.haml index 49b03a9e3..fc042f845 100644 --- a/app/views/admin/settings/edit.html.haml +++ b/app/views/admin/settings/edit.html.haml @@ -45,7 +45,7 @@ - if captcha_available? .fields-group - = f.input :captcha_enabled, as: :boolean, wrapper: :with_label, label: t('admin.settings.captcha_enabled.title'), hint: t('admin.settings.captcha_enabled.desc_html') + = f.input :captcha_mode, as: :radio_buttons, collection: %w(disabled registration-form email-confirmation), include_blank: false, wrapper: :with_block_label, label_method: ->(type) { safe_join([t("admin.settings.captcha.#{type}.title"), content_tag(:span, t("admin.settings.captcha.#{type}.desc_html"), class: 'hint')])}, label: t('admin.settings.captcha.title'), hint: t('admin.settings.captcha.desc_html') %hr.spacer/ diff --git a/app/views/auth/confirmations/captcha.html.haml b/app/views/auth/confirmations/captcha.html.haml new file mode 100644 index 000000000..850bc1479 --- /dev/null +++ b/app/views/auth/confirmations/captcha.html.haml @@ -0,0 +1,11 @@ +- content_for :page_title do + = t('auth.confirm_captcha') + += form_tag auth_captcha_confirmation_url, method: 'POST', class: 'simple_form' do + = hidden_field_tag :confirmation_token, params[:confirmation_token] + + .field-group + = render_captcha_if_needed + + .actions + %button.button= t('challenge.continue') diff --git a/config/locales-glitch/en.yml b/config/locales-glitch/en.yml index 9bd66c969..6ad5a5365 100644 --- a/config/locales-glitch/en.yml +++ b/config/locales-glitch/en.yml @@ -2,9 +2,18 @@ en: admin: settings: - captcha_enabled: - desc_html: Enable hCaptcha integration, requiring new users to solve a challenge when signing up. Note that this disables app-based registration, may prevent your instance from being listed as having open registrations, and requires third-party scripts from hCaptcha to be embedded in the registration pages. This may have security and privacy concerns. - title: Require new users to go through a CAPTCHA to sign up + captcha: + desc_html: Configure hCaptcha integration, relying on third-party scripts. This may have security and privacy implications. + email-confirmation: + desc_html: Require new users to go through hCaptcha at the e-mail validation step. Bots will not be deterred from creating accounts, but they may be prevented from confirming them, leaving them to be automatically cleaned up after a couple days. This does not interfere with app-based registrations. + title: CAPTCHA on email validation + disabled: + desc_html: Do not require a CAPTCHA + title: Disabled + registration-form: + desc_html: Require new users to go through hCaptcha on the registration form, so that CAPTCHA requirement is immediately apparent to them. This disables app-based registrations and may prevent your instance from being listed as having open registrations. + title: CAPTCHA on registration forms + title: CAPTCHA configuration enable_keybase: desc_html: Allow your users to prove their identity via keybase title: Enable keybase integration @@ -20,6 +29,8 @@ en: show_replies_in_public_timelines: desc_html: In addition to public self-replies (threads), show public replies in local and public timelines. title: Show replies in public timelines + auth: + confirm_captcha: User verification generic: use_this: Use this settings: diff --git a/config/routes.rb b/config/routes.rb index 65dd7ad63..d0eeda1e8 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -44,6 +44,7 @@ Rails.application.routes.draw do resource :setup, only: [:show, :update], controller: :setup resource :challenge, only: [:create], controller: :challenges get 'sessions/security_key_options', to: 'sessions#webauthn_options' + post 'captcha_confirmation', to: 'confirmations#confirm_captcha', as: :captcha_confirmation end end diff --git a/config/settings.yml b/config/settings.yml index 7d192f369..b5437caee 100644 --- a/config/settings.yml +++ b/config/settings.yml @@ -77,7 +77,7 @@ defaults: &defaults show_domain_blocks_rationale: 'disabled' outgoing_spoilers: '' require_invite_text: false - captcha_enabled: false + captcha_mode: 'disabled' development: <<: *defaults From b7cf3941b3783220e6b3bc9a6d3975ceecdc64cb Mon Sep 17 00:00:00 2001 From: Claire Date: Tue, 25 Jan 2022 23:56:57 +0100 Subject: [PATCH 8/8] Change CAPTCHA handling to be only on email verification This simplifies the implementation considerably, and while not providing ideal UX, it's the most flexible approach. --- app/controllers/about_controller.rb | 2 -- app/controllers/api/v1/accounts_controller.rb | 4 +-- .../auth/confirmations_controller.rb | 6 ----- .../auth/registrations_controller.rb | 22 --------------- app/controllers/concerns/captcha_concern.rb | 27 ++++--------------- app/models/form/admin_settings.rb | 4 +-- app/serializers/rest/instance_serializer.rb | 6 +---- app/views/about/_registration.html.haml | 3 --- app/views/admin/settings/edit.html.haml | 2 +- .../auth/confirmations/captcha.html.haml | 2 +- app/views/auth/registrations/new.html.haml | 3 --- config/locales-glitch/en.yml | 15 +++-------- config/settings.yml | 2 +- spec/views/about/show.html.haml_spec.rb | 1 - 14 files changed, 15 insertions(+), 84 deletions(-) diff --git a/app/controllers/about_controller.rb b/app/controllers/about_controller.rb index 5a35dbbcb..620c0ff78 100644 --- a/app/controllers/about_controller.rb +++ b/app/controllers/about_controller.rb @@ -2,7 +2,6 @@ class AboutController < ApplicationController include RegistrationSpamConcern - include CaptchaConcern before_action :set_pack @@ -13,7 +12,6 @@ class AboutController < ApplicationController before_action :set_instance_presenter before_action :set_expires_in, only: [:more, :terms] before_action :set_registration_form_time, only: :show - before_action :extend_csp_for_captcha!, only: :show skip_before_action :require_functional!, only: [:more, :terms] diff --git a/app/controllers/api/v1/accounts_controller.rb b/app/controllers/api/v1/accounts_controller.rb index 8916c3f96..5c47158e0 100644 --- a/app/controllers/api/v1/accounts_controller.rb +++ b/app/controllers/api/v1/accounts_controller.rb @@ -1,8 +1,6 @@ # frozen_string_literal: true class Api::V1::AccountsController < Api::BaseController - include CaptchaConcern - before_action -> { authorize_if_got_token! :read, :'read:accounts' }, except: [:create, :follow, :unfollow, :remove_from_followers, :block, :unblock, :mute, :unmute] before_action -> { doorkeeper_authorize! :follow, :'write:follows' }, only: [:follow, :unfollow, :remove_from_followers] before_action -> { doorkeeper_authorize! :follow, :'write:mutes' }, only: [:mute, :unmute] @@ -85,7 +83,7 @@ class Api::V1::AccountsController < Api::BaseController end def check_enabled_registrations - forbidden if single_user_mode? || omniauth_only? || !allowed_registrations? || captcha_enabled? + forbidden if single_user_mode? || omniauth_only? || !allowed_registrations? end def allowed_registrations? diff --git a/app/controllers/auth/confirmations_controller.rb b/app/controllers/auth/confirmations_controller.rb index e9a646f91..17ad56fa8 100644 --- a/app/controllers/auth/confirmations_controller.rb +++ b/app/controllers/auth/confirmations_controller.rb @@ -22,8 +22,6 @@ class Auth::ConfirmationsController < Devise::ConfirmationsController end def show - clear_captcha! - old_session_values = session.to_hash reset_session session.update old_session_values.except('session_id') @@ -63,10 +61,6 @@ class Auth::ConfirmationsController < Devise::ConfirmationsController invite.present? && !invite.max_uses.nil? end - def captcha_context - 'email-confirmation' - end - def set_pack use_pack 'auth' end diff --git a/app/controllers/auth/registrations_controller.rb b/app/controllers/auth/registrations_controller.rb index 0db9cb84d..6b1f3fa82 100644 --- a/app/controllers/auth/registrations_controller.rb +++ b/app/controllers/auth/registrations_controller.rb @@ -2,7 +2,6 @@ class Auth::RegistrationsController < Devise::RegistrationsController include RegistrationSpamConcern - include CaptchaConcern layout :determine_layout @@ -16,8 +15,6 @@ class Auth::RegistrationsController < Devise::RegistrationsController before_action :require_not_suspended!, only: [:update] before_action :set_cache_headers, only: [:edit, :update] before_action :set_registration_form_time, only: :new - before_action :extend_csp_for_captcha!, only: [:new, :create] - before_action :check_captcha!, only: :create skip_before_action :require_functional!, only: [:edit, :update] @@ -138,23 +135,4 @@ class Auth::RegistrationsController < Devise::RegistrationsController def set_cache_headers response.headers['Cache-Control'] = 'no-cache, no-store, max-age=0, must-revalidate' end - - def sign_up(resource_name, resource) - clear_captcha! - - old_session_values = session.to_hash - reset_session - session.update old_session_values.except('session_id') - - super - end - - def check_captcha! - super do |error| - build_resource(sign_up_params) - resource.validate - resource.errors.add(:base, error) - respond_with resource - end - end end diff --git a/app/controllers/concerns/captcha_concern.rb b/app/controllers/concerns/captcha_concern.rb index 02069d205..538c1ffb1 100644 --- a/app/controllers/concerns/captcha_concern.rb +++ b/app/controllers/concerns/captcha_concern.rb @@ -4,10 +4,8 @@ module CaptchaConcern extend ActiveSupport::Concern include Hcaptcha::Adapters::ViewMethods - CAPTCHA_TIMEOUT = 2.hours.freeze - included do - helper_method :render_captcha_if_needed + helper_method :render_captcha end def captcha_available? @@ -15,32 +13,21 @@ module CaptchaConcern end def captcha_enabled? - captcha_available? && Setting.captcha_mode == captcha_context - end - - def captcha_recently_passed? - session[:captcha_passed_at].present? && session[:captcha_passed_at] >= CAPTCHA_TIMEOUT.ago + captcha_available? && Setting.captcha_enabled end def captcha_user_bypass? - current_user.present? || (@invite.present? && @invite.valid_for_use? && !@invite.max_uses.nil?) + false end def captcha_required? - return false if ENV['OMNIAUTH_ONLY'] == 'true' - return false unless Setting.registrations_mode != 'none' || @invite&.valid_for_use? - captcha_enabled? && !captcha_user_bypass? && !captcha_recently_passed? - end - - def clear_captcha! - session.delete(:captcha_passed_at) + captcha_enabled? && !captcha_user_bypass? end def check_captcha! return true unless captcha_required? if verify_hcaptcha - session[:captcha_passed_at] = Time.now.utc true else if block_given? @@ -64,13 +51,9 @@ module CaptchaConcern end end - def render_captcha_if_needed + def render_captcha return unless captcha_required? hcaptcha_tags end - - def captcha_context - 'registration-form' - end end diff --git a/app/models/form/admin_settings.rb b/app/models/form/admin_settings.rb index 7abb0d6c6..34f14e312 100644 --- a/app/models/form/admin_settings.rb +++ b/app/models/form/admin_settings.rb @@ -40,7 +40,7 @@ class Form::AdminSettings noindex outgoing_spoilers require_invite_text - captcha_mode + captcha_enabled ).freeze BOOLEAN_KEYS = %i( @@ -59,6 +59,7 @@ class Form::AdminSettings trendable_by_default noindex require_invite_text + captcha_enabled ).freeze UPLOAD_KEYS = %i( @@ -82,7 +83,6 @@ class Form::AdminSettings validates :bootstrap_timeline_accounts, existing_username: { multiple: true } validates :show_domain_blocks, inclusion: { in: %w(disabled users all) } validates :show_domain_blocks_rationale, inclusion: { in: %w(disabled users all) } - validates :captcha_mode, inclusion: { in: %w(disabled registration-form email-confirmation) } def initialize(_attributes = {}) super diff --git a/app/serializers/rest/instance_serializer.rb b/app/serializers/rest/instance_serializer.rb index d343cca20..48bbb55c8 100644 --- a/app/serializers/rest/instance_serializer.rb +++ b/app/serializers/rest/instance_serializer.rb @@ -98,7 +98,7 @@ class REST::InstanceSerializer < ActiveModel::Serializer end def registrations - Setting.registrations_mode != 'none' && !Rails.configuration.x.single_user_mode && !captcha_enabled? + Setting.registrations_mode != 'none' && !Rails.configuration.x.single_user_mode end def approval_required @@ -114,8 +114,4 @@ class REST::InstanceSerializer < ActiveModel::Serializer def instance_presenter @instance_presenter ||= InstancePresenter.new end - - def captcha_enabled? - ENV['HCAPTCHA_SECRET_KEY'].present? && ENV['HCAPTCHA_SITE_KEY'].present? && Setting.captcha_mode == 'registration-form' - end end diff --git a/app/views/about/_registration.html.haml b/app/views/about/_registration.html.haml index 5bb5d08a2..e4d614d71 100644 --- a/app/views/about/_registration.html.haml +++ b/app/views/about/_registration.html.haml @@ -21,9 +21,6 @@ .fields-group = f.input :agreement, as: :boolean, wrapper: :with_label, label: t('auth.checkbox_agreement_html', rules_path: about_more_path, terms_path: terms_path), required: true, disabled: closed_registrations? - .fields-group - = render_captcha_if_needed - .actions = f.button :button, sign_up_message, type: :submit, class: 'button button-primary', disabled: closed_registrations? diff --git a/app/views/admin/settings/edit.html.haml b/app/views/admin/settings/edit.html.haml index fc042f845..49b03a9e3 100644 --- a/app/views/admin/settings/edit.html.haml +++ b/app/views/admin/settings/edit.html.haml @@ -45,7 +45,7 @@ - if captcha_available? .fields-group - = f.input :captcha_mode, as: :radio_buttons, collection: %w(disabled registration-form email-confirmation), include_blank: false, wrapper: :with_block_label, label_method: ->(type) { safe_join([t("admin.settings.captcha.#{type}.title"), content_tag(:span, t("admin.settings.captcha.#{type}.desc_html"), class: 'hint')])}, label: t('admin.settings.captcha.title'), hint: t('admin.settings.captcha.desc_html') + = f.input :captcha_enabled, as: :boolean, wrapper: :with_label, label: t('admin.settings.captcha_enabled.title'), hint: t('admin.settings.captcha_enabled.desc_html') %hr.spacer/ diff --git a/app/views/auth/confirmations/captcha.html.haml b/app/views/auth/confirmations/captcha.html.haml index 850bc1479..0f7cf9c59 100644 --- a/app/views/auth/confirmations/captcha.html.haml +++ b/app/views/auth/confirmations/captcha.html.haml @@ -5,7 +5,7 @@ = hidden_field_tag :confirmation_token, params[:confirmation_token] .field-group - = render_captcha_if_needed + = render_captcha .actions %button.button= t('challenge.continue') diff --git a/app/views/auth/registrations/new.html.haml b/app/views/auth/registrations/new.html.haml index 5cb558297..6981195ed 100644 --- a/app/views/auth/registrations/new.html.haml +++ b/app/views/auth/registrations/new.html.haml @@ -38,9 +38,6 @@ .fields-group = f.input :agreement, as: :boolean, wrapper: :with_label, label: whitelist_mode? ? t('auth.checkbox_agreement_without_rules_html', terms_path: terms_path) : t('auth.checkbox_agreement_html', rules_path: about_more_path, terms_path: terms_path), required: true - .field-group - = render_captcha_if_needed - .actions = f.button :button, @invite.present? ? t('auth.register') : sign_up_message, type: :submit diff --git a/config/locales-glitch/en.yml b/config/locales-glitch/en.yml index 6ad5a5365..ab7f1b976 100644 --- a/config/locales-glitch/en.yml +++ b/config/locales-glitch/en.yml @@ -2,18 +2,9 @@ en: admin: settings: - captcha: - desc_html: Configure hCaptcha integration, relying on third-party scripts. This may have security and privacy implications. - email-confirmation: - desc_html: Require new users to go through hCaptcha at the e-mail validation step. Bots will not be deterred from creating accounts, but they may be prevented from confirming them, leaving them to be automatically cleaned up after a couple days. This does not interfere with app-based registrations. - title: CAPTCHA on email validation - disabled: - desc_html: Do not require a CAPTCHA - title: Disabled - registration-form: - desc_html: Require new users to go through hCaptcha on the registration form, so that CAPTCHA requirement is immediately apparent to them. This disables app-based registrations and may prevent your instance from being listed as having open registrations. - title: CAPTCHA on registration forms - title: CAPTCHA configuration + captcha_enabled: + desc_html: Enable hCaptcha integration, requiring new users to solve a challenge when confirming their email address. This requires third-party scripts from hCaptcha to be embedded in the email verification page, which may have security and privacy concerns. Users that have been invited through a limited-use invite will not need to solve a CAPTCHA challenge. + title: Require new users to go through a CAPTCHA to confirm their account enable_keybase: desc_html: Allow your users to prove their identity via keybase title: Enable keybase integration diff --git a/config/settings.yml b/config/settings.yml index b5437caee..7d192f369 100644 --- a/config/settings.yml +++ b/config/settings.yml @@ -77,7 +77,7 @@ defaults: &defaults show_domain_blocks_rationale: 'disabled' outgoing_spoilers: '' require_invite_text: false - captcha_mode: 'disabled' + captcha_enabled: false development: <<: *defaults diff --git a/spec/views/about/show.html.haml_spec.rb b/spec/views/about/show.html.haml_spec.rb index 12c96ea49..d608bbf5d 100644 --- a/spec/views/about/show.html.haml_spec.rb +++ b/spec/views/about/show.html.haml_spec.rb @@ -10,7 +10,6 @@ describe 'about/show.html.haml', without_verify_partial_doubles: true do allow(view).to receive(:site_title).and_return('example site') allow(view).to receive(:new_user).and_return(User.new) allow(view).to receive(:use_seamless_external_login?).and_return(false) - allow(view).to receive(:render_captcha_if_needed).and_return(nil) end it 'has valid open graph tags' do