From d70c3ab4c39e642d41138ab693af77dd6c258e8c Mon Sep 17 00:00:00 2001 From: ThibG Date: Tue, 11 Aug 2020 23:09:13 +0200 Subject: [PATCH 1/5] Add HTML form validation for the registration form (#14560) * Add HTML-level validation of username in sign-up form * Make required fields with incorrect values more visible * Enable HTML form validation for the registration form * Mark agreement checkbox as required client-side * Add minimum length to password * Add client-side password confirmation validation --- app/javascript/packs/public.js | 10 ++++++++++ app/javascript/styles/mastodon/forms.scss | 3 ++- app/views/about/_registration.html.haml | 8 ++++---- 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/app/javascript/packs/public.js b/app/javascript/packs/public.js index 08cc662e60b..777182b7f51 100644 --- a/app/javascript/packs/public.js +++ b/app/javascript/packs/public.js @@ -116,6 +116,16 @@ function main() { new Rellax('.parallax', { speed: -1 }); } + delegate(document, '#registration_user_password_confirmation,#registration_user_password', 'input', () => { + const password = document.getElementById('registration_user_password'); + const confirmation = document.getElementById('registration_user_password_confirmation'); + if (password.value && password.value !== confirmation.value) { + confirmation.setCustomValidity((new IntlMessageFormat(messages['password_confirmation.mismatching'] || 'Password confirmation does not match', locale)).format()); + } else { + confirmation.setCustomValidity(''); + } + }); + delegate(document, '.custom-emoji', 'mouseover', getEmojiAnimationHandler('data-original')); delegate(document, '.custom-emoji', 'mouseout', getEmojiAnimationHandler('data-static')); diff --git a/app/javascript/styles/mastodon/forms.scss b/app/javascript/styles/mastodon/forms.scss index 7a0b2f9a398..a6df51f9547 100644 --- a/app/javascript/styles/mastodon/forms.scss +++ b/app/javascript/styles/mastodon/forms.scss @@ -364,7 +364,8 @@ code { box-shadow: none; } - &:focus:invalid:not(:placeholder-shown) { + &:focus:invalid:not(:placeholder-shown), + &:required:invalid:not(:placeholder-shown) { border-color: lighten($error-red, 12%); } diff --git a/app/views/about/_registration.html.haml b/app/views/about/_registration.html.haml index af28e217491..f65e6742794 100644 --- a/app/views/about/_registration.html.haml +++ b/app/views/about/_registration.html.haml @@ -1,13 +1,13 @@ .simple_form__overlay-area{ class: (closed_registrations? && @instance_presenter.closed_registrations_message.present?) ? 'simple_form__overlay-area__blurred' : '' } - = simple_form_for(new_user, url: user_registration_path, namespace: 'registration') do |f| + = simple_form_for(new_user, url: user_registration_path, namespace: 'registration', html: { novalidate: false }) do |f| %p.lead= t('about.federation_hint_html', instance: content_tag(:strong, site_hostname)) .fields-group = f.simple_fields_for :account do |account_fields| - = account_fields.input :username, wrapper: :with_label, label: false, required: true, input_html: { 'aria-label' => t('simple_form.labels.defaults.username'), :autocomplete => 'off', placeholder: t('simple_form.labels.defaults.username') }, append: "@#{site_hostname}", hint: false, disabled: closed_registrations? + = account_fields.input :username, wrapper: :with_label, label: false, required: true, input_html: { 'aria-label' => t('simple_form.labels.defaults.username'), :autocomplete => 'off', placeholder: t('simple_form.labels.defaults.username'), pattern: Account::USERNAME_RE.source }, append: "@#{site_hostname}", hint: false, disabled: closed_registrations? = f.input :email, placeholder: t('simple_form.labels.defaults.email'), required: true, input_html: { 'aria-label' => t('simple_form.labels.defaults.email'), :autocomplete => 'off' }, hint: false, disabled: closed_registrations? - = f.input :password, placeholder: t('simple_form.labels.defaults.password'), required: true, input_html: { 'aria-label' => t('simple_form.labels.defaults.password'), :autocomplete => 'off' }, hint: false, disabled: closed_registrations? + = f.input :password, placeholder: t('simple_form.labels.defaults.password'), required: true, input_html: { 'aria-label' => t('simple_form.labels.defaults.password'), :autocomplete => 'off', :minlength => User.password_length.first, :maxlength => User.password_length.last }, hint: false, disabled: closed_registrations? = f.input :password_confirmation, placeholder: t('simple_form.labels.defaults.confirm_password'), required: true, input_html: { 'aria-label' => t('simple_form.labels.defaults.confirm_password'), :autocomplete => 'off' }, hint: false, disabled: closed_registrations? - if approved_registrations? @@ -16,7 +16,7 @@ = invite_request_fields.input :text, as: :text, wrapper: :with_block_label, required: false .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), disabled: closed_registrations? + = 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? .actions = f.button :button, sign_up_message, type: :submit, class: 'button button-primary', disabled: closed_registrations? From 7dc4c742650ac69ec9a4459b656e172283511e4c Mon Sep 17 00:00:00 2001 From: ThibG Date: Wed, 12 Aug 2020 12:11:15 +0200 Subject: [PATCH 2/5] Add client-side validation in password change forms (#14564) * Fix client-side username validation at registration It used the Account::USERNAME_RE regexp which is for *remote* users, local user validation is stricter. Also take into account max username length. * Add client-side form validation for password change * Add client-side form validation to dedicated registration form Previous changes only applied to the /about page, not the dedicated form on /auth --- app/javascript/packs/public.js | 12 ++++++++++++ app/views/about/_registration.html.haml | 2 +- app/views/auth/passwords/edit.html.haml | 4 ++-- app/views/auth/registrations/edit.html.haml | 4 ++-- app/views/auth/registrations/new.html.haml | 8 ++++---- 5 files changed, 21 insertions(+), 9 deletions(-) diff --git a/app/javascript/packs/public.js b/app/javascript/packs/public.js index 777182b7f51..551e281a8ee 100644 --- a/app/javascript/packs/public.js +++ b/app/javascript/packs/public.js @@ -126,6 +126,18 @@ function main() { } }); + delegate(document, '#user_password,#user_password_confirmation', 'input', () => { + const password = document.getElementById('user_password'); + const confirmation = document.getElementById('user_password_confirmation'); + if (!confirmation) return; + + if (password.value && password.value !== confirmation.value) { + confirmation.setCustomValidity((new IntlMessageFormat(messages['password_confirmation.mismatching'] || 'Password confirmation does not match', locale)).format()); + } else { + confirmation.setCustomValidity(''); + } + }); + delegate(document, '.custom-emoji', 'mouseover', getEmojiAnimationHandler('data-original')); delegate(document, '.custom-emoji', 'mouseout', getEmojiAnimationHandler('data-static')); diff --git a/app/views/about/_registration.html.haml b/app/views/about/_registration.html.haml index f65e6742794..336acad6ac6 100644 --- a/app/views/about/_registration.html.haml +++ b/app/views/about/_registration.html.haml @@ -4,7 +4,7 @@ .fields-group = f.simple_fields_for :account do |account_fields| - = account_fields.input :username, wrapper: :with_label, label: false, required: true, input_html: { 'aria-label' => t('simple_form.labels.defaults.username'), :autocomplete => 'off', placeholder: t('simple_form.labels.defaults.username'), pattern: Account::USERNAME_RE.source }, append: "@#{site_hostname}", hint: false, disabled: closed_registrations? + = account_fields.input :username, wrapper: :with_label, label: false, required: true, input_html: { 'aria-label' => t('simple_form.labels.defaults.username'), :autocomplete => 'off', placeholder: t('simple_form.labels.defaults.username'), pattern: '[a-z0-9_]+', maxlength: 30 }, append: "@#{site_hostname}", hint: false, disabled: closed_registrations? = f.input :email, placeholder: t('simple_form.labels.defaults.email'), required: true, input_html: { 'aria-label' => t('simple_form.labels.defaults.email'), :autocomplete => 'off' }, hint: false, disabled: closed_registrations? = f.input :password, placeholder: t('simple_form.labels.defaults.password'), required: true, input_html: { 'aria-label' => t('simple_form.labels.defaults.password'), :autocomplete => 'off', :minlength => User.password_length.first, :maxlength => User.password_length.last }, hint: false, disabled: closed_registrations? diff --git a/app/views/auth/passwords/edit.html.haml b/app/views/auth/passwords/edit.html.haml index 383d44f00fa..114a7445422 100644 --- a/app/views/auth/passwords/edit.html.haml +++ b/app/views/auth/passwords/edit.html.haml @@ -1,14 +1,14 @@ - content_for :page_title do = t('auth.set_new_password') -= simple_form_for(resource, as: resource_name, url: password_path(resource_name), html: { method: :put }) do |f| += simple_form_for(resource, as: resource_name, url: password_path(resource_name), html: { method: :put, novalidate: false }) do |f| = render 'shared/error_messages', object: resource - if !use_seamless_external_login? || resource.encrypted_password.present? = f.input :reset_password_token, as: :hidden .fields-group - = f.input :password, wrapper: :with_label, autofocus: true, label: t('simple_form.labels.defaults.new_password'), input_html: { 'aria-label' => t('simple_form.labels.defaults.new_password'), :autocomplete => 'off' }, required: true + = f.input :password, wrapper: :with_label, autofocus: true, label: t('simple_form.labels.defaults.new_password'), input_html: { 'aria-label' => t('simple_form.labels.defaults.new_password'), :autocomplete => 'off', :minlength => User.password_length.first, :maxlength => User.password_length.last }, required: true .fields-group = f.input :password_confirmation, wrapper: :with_label, label: t('simple_form.labels.defaults.confirm_new_password'), input_html: { 'aria-label' => t('simple_form.labels.defaults.confirm_new_password'), :autocomplete => 'off' }, required: true diff --git a/app/views/auth/registrations/edit.html.haml b/app/views/auth/registrations/edit.html.haml index a155c75c987..4a46b27a93c 100644 --- a/app/views/auth/registrations/edit.html.haml +++ b/app/views/auth/registrations/edit.html.haml @@ -5,7 +5,7 @@ %h3= t('auth.security') -= simple_form_for(resource, as: resource_name, url: registration_path(resource_name), html: { method: :put, class: 'auth_edit' }) do |f| += simple_form_for(resource, as: resource_name, url: registration_path(resource_name), html: { method: :put, class: 'auth_edit', novalidate: false }) do |f| = render 'shared/error_messages', object: resource - if !use_seamless_external_login? || resource.encrypted_password.present? @@ -17,7 +17,7 @@ .fields-row .fields-row__column.fields-group.fields-row__column-6 - = f.input :password, wrapper: :with_label, label: t('simple_form.labels.defaults.new_password'), input_html: { 'aria-label' => t('simple_form.labels.defaults.new_password'), :autocomplete => 'off' }, hint: t('simple_form.hints.defaults.password'), disabled: current_account.suspended? + = f.input :password, wrapper: :with_label, label: t('simple_form.labels.defaults.new_password'), input_html: { 'aria-label' => t('simple_form.labels.defaults.new_password'), :autocomplete => 'off', :minlength => User.password_length.first, :maxlength => User.password_length.last }, hint: t('simple_form.hints.defaults.password'), disabled: current_account.suspended? .fields-row__column.fields-group.fields-row__column-6 = f.input :password_confirmation, wrapper: :with_label, label: t('simple_form.labels.defaults.confirm_new_password'), input_html: { 'aria-label' => t('simple_form.labels.defaults.confirm_new_password'), :autocomplete => 'off' }, disabled: current_account.suspended? diff --git a/app/views/auth/registrations/new.html.haml b/app/views/auth/registrations/new.html.haml index 457bc1d2314..d5698b42626 100644 --- a/app/views/auth/registrations/new.html.haml +++ b/app/views/auth/registrations/new.html.haml @@ -4,7 +4,7 @@ - content_for :header_tags do = render partial: 'shared/og', locals: { description: description_for_sign_up } -= simple_form_for(resource, as: resource_name, url: registration_path(resource_name)) do |f| += simple_form_for(resource, as: resource_name, url: registration_path(resource_name), html: { novalidate: false }) do |f| = render 'shared/error_messages', object: resource - if @invite.present? && @invite.autofollow? @@ -14,13 +14,13 @@ = f.simple_fields_for :account do |ff| .fields-group - = ff.input :username, wrapper: :with_label, autofocus: true, label: t('simple_form.labels.defaults.username'), required: true, input_html: { 'aria-label' => t('simple_form.labels.defaults.username'), :autocomplete => 'off' }, append: "@#{site_hostname}", hint: t('simple_form.hints.defaults.username', domain: site_hostname) + = ff.input :username, wrapper: :with_label, autofocus: true, label: t('simple_form.labels.defaults.username'), required: true, input_html: { 'aria-label' => t('simple_form.labels.defaults.username'), :autocomplete => 'off', pattern: '[a-z0-9_]+', maxlength: 30 }, append: "@#{site_hostname}", hint: t('simple_form.hints.defaults.username', domain: site_hostname) .fields-group = f.input :email, wrapper: :with_label, label: t('simple_form.labels.defaults.email'), required: true, input_html: { 'aria-label' => t('simple_form.labels.defaults.email'), :autocomplete => 'off' } .fields-group - = f.input :password, wrapper: :with_label, label: t('simple_form.labels.defaults.password'), required: true, input_html: { 'aria-label' => t('simple_form.labels.defaults.password'), :autocomplete => 'off' } + = f.input :password, wrapper: :with_label, label: t('simple_form.labels.defaults.password'), required: true, input_html: { 'aria-label' => t('simple_form.labels.defaults.password'), :autocomplete => 'off', :minlength => User.password_length.first, :maxlength => User.password_length.last } .fields-group = f.input :password_confirmation, wrapper: :with_label, label: t('simple_form.labels.defaults.confirm_password'), required: true, input_html: { 'aria-label' => t('simple_form.labels.defaults.confirm_password'), :autocomplete => 'off' } @@ -33,7 +33,7 @@ = f.input :invite_code, as: :hidden .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) + = 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 .actions = f.button :button, @invite.present? ? t('auth.register') : sign_up_message, type: :submit From 8d217d7231be46af552c63aff9e53d0ed5dca0f6 Mon Sep 17 00:00:00 2001 From: ThibG Date: Wed, 12 Aug 2020 12:40:25 +0200 Subject: [PATCH 3/5] Improve email address validation (#14565) * Increase DNS timeout from 1 second to 5 seconds for MX check 1 seconds is rather short when using a recursive DNS resolver which hasn't got a cached result already available. Use 5 seconds instead, which is the timeout value we use for outgoing HTTP queries. * Add more precise error messages for invalid e-mail addresses --- .../admin/email_domain_blocks_controller.rb | 2 +- app/validators/blacklisted_email_validator.rb | 2 +- app/validators/email_mx_validator.rb | 30 ++++++++++++++----- config/locales/en.yml | 2 ++ lib/mastodon/email_domain_blocks_cli.rb | 2 +- .../blacklisted_email_validator_spec.rb | 4 +-- 6 files changed, 29 insertions(+), 13 deletions(-) diff --git a/app/controllers/admin/email_domain_blocks_controller.rb b/app/controllers/admin/email_domain_blocks_controller.rb index c2591972624..f7bdfb0c5f6 100644 --- a/app/controllers/admin/email_domain_blocks_controller.rb +++ b/app/controllers/admin/email_domain_blocks_controller.rb @@ -27,7 +27,7 @@ module Admin ips = [] Resolv::DNS.open do |dns| - dns.timeouts = 1 + dns.timeouts = 5 hostnames = dns.getresources(@email_domain_block.domain, Resolv::DNS::Resource::IN::MX).to_a.map { |e| e.exchange.to_s } diff --git a/app/validators/blacklisted_email_validator.rb b/app/validators/blacklisted_email_validator.rb index 0d01a1c47fb..16e3abf1298 100644 --- a/app/validators/blacklisted_email_validator.rb +++ b/app/validators/blacklisted_email_validator.rb @@ -6,7 +6,7 @@ class BlacklistedEmailValidator < ActiveModel::Validator @email = user.email - user.errors.add(:email, I18n.t('users.invalid_email')) if blocked_email? + user.errors.add(:email, I18n.t('users.blocked_email_provider')) if blocked_email? end private diff --git a/app/validators/email_mx_validator.rb b/app/validators/email_mx_validator.rb index 9b500996683..ef1554494cc 100644 --- a/app/validators/email_mx_validator.rb +++ b/app/validators/email_mx_validator.rb @@ -4,22 +4,38 @@ require 'resolv' class EmailMxValidator < ActiveModel::Validator def validate(user) - user.errors.add(:email, I18n.t('users.invalid_email')) if invalid_mx?(user.email) + domain = get_domain(user.email) + + if domain.nil? + user.errors.add(:email, I18n.t('users.invalid_email')) + else + ips, hostnames = resolve_mx(domain) + if ips.empty? + user.errors.add(:email, I18n.t('users.invalid_email_mx')) + elsif on_blacklist?(hostnames + ips) + user.errors.add(:email, I18n.t('users.blocked_email_provider')) + end + end end private - def invalid_mx?(value) + def get_domain(value) _, domain = value.split('@', 2) - return true if domain.nil? + return nil if domain.nil? - domain = TagManager.instance.normalize_domain(domain) + TagManager.instance.normalize_domain(domain) + rescue Addressable::URI::InvalidURIError + nil + end + + def resolve_mx(domain) hostnames = [] ips = [] Resolv::DNS.open do |dns| - dns.timeouts = 1 + dns.timeouts = 5 hostnames = dns.getresources(domain, Resolv::DNS::Resource::IN::MX).to_a.map { |e| e.exchange.to_s } @@ -29,9 +45,7 @@ class EmailMxValidator < ActiveModel::Validator end end - ips.empty? || on_blacklist?(hostnames + ips) - rescue Addressable::URI::InvalidURIError - true + [ips, hostnames] end def on_blacklist?(values) diff --git a/config/locales/en.yml b/config/locales/en.yml index 2cae0a3e3ba..40adfc21e4d 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1325,9 +1325,11 @@ en: tips: Tips title: Welcome aboard, %{name}! users: + blocked_email_provider: This e-mail provider isn't allowed follow_limit_reached: You cannot follow more than %{limit} people generic_access_help_html: Trouble accessing your account? You may get in touch with %{email} for assistance invalid_email: The e-mail address is invalid + invalid_email_mx: The e-mail address does not seem to exist invalid_otp_token: Invalid two-factor code invalid_sign_in_token: Invalid security code otp_lost_help_html: If you lost access to both, you may get in touch with %{email} diff --git a/lib/mastodon/email_domain_blocks_cli.rb b/lib/mastodon/email_domain_blocks_cli.rb index 7fe1efaaa5c..55a637d6828 100644 --- a/lib/mastodon/email_domain_blocks_cli.rb +++ b/lib/mastodon/email_domain_blocks_cli.rb @@ -63,7 +63,7 @@ module Mastodon ips = [] Resolv::DNS.open do |dns| - dns.timeouts = 1 + dns.timeouts = 5 hostnames = dns.getresources(email_domain_block.domain, Resolv::DNS::Resource::IN::MX).to_a.map { |e| e.exchange.to_s } ([email_domain_block.domain] + hostnames).uniq.each do |hostname| diff --git a/spec/validators/blacklisted_email_validator_spec.rb b/spec/validators/blacklisted_email_validator_spec.rb index ccc5dc0f485..f0708dc4623 100644 --- a/spec/validators/blacklisted_email_validator_spec.rb +++ b/spec/validators/blacklisted_email_validator_spec.rb @@ -17,7 +17,7 @@ RSpec.describe BlacklistedEmailValidator, type: :validator do let(:blocked_email) { true } it 'calls errors.add' do - expect(errors).to have_received(:add).with(:email, I18n.t('users.invalid_email')) + expect(errors).to have_received(:add).with(:email, I18n.t('users.blocked_email_provider')) end end @@ -25,7 +25,7 @@ RSpec.describe BlacklistedEmailValidator, type: :validator do let(:blocked_email) { false } it 'not calls errors.add' do - expect(errors).not_to have_received(:add).with(:email, I18n.t('users.invalid_email')) + expect(errors).not_to have_received(:add).with(:email, I18n.t('users.blocked_email_provider')) end end end From 01647b8acb0d881609e1d6729e01e373a11030fb Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Wed, 12 Aug 2020 15:36:07 +0200 Subject: [PATCH 4/5] Fix destructuring error when unsubscribing without subscribing (#14566) --- streaming/index.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/streaming/index.js b/streaming/index.js index 7c0c6a465eb..7072d0bd767 100644 --- a/streaming/index.js +++ b/streaming/index.js @@ -210,6 +210,7 @@ const startWorker = (workerId) => { if (subs[channel].length === 0) { log.verbose(`Unsubscribe ${channel}`); redisSubscribeClient.unsubscribe(channel); + delete subs[channel]; } }; @@ -866,19 +867,21 @@ const startWorker = (workerId) => { channelNameToIds(request, channelName, params).then(({ channelIds }) => { log.verbose(request.requestId, `Ending stream from ${channelIds.join(', ')} for ${request.accountId}`); - const { listener, stopHeartbeat } = subscriptions[channelIds.join(';')]; + const subscription = subscriptions[channelIds.join(';')]; - if (!listener) { + if (!subscription) { return; } + const { listener, stopHeartbeat } = subscription; + channelIds.forEach(channelId => { unsubscribe(`${redisPrefix}${channelId}`, listener); }); stopHeartbeat(); - subscriptions[channelIds.join(';')] = undefined; + delete subscriptions[channelIds.join(';')]; }).catch(err => { log.verbose(request.requestId, 'Unsubscription error:', err); socket.send(JSON.stringify({ error: err.toString() })); From 0f38f9726a0a9d8e13633424d03e79f54df047a4 Mon Sep 17 00:00:00 2001 From: ThibG Date: Thu, 13 Aug 2020 12:04:28 +0200 Subject: [PATCH 5/5] Fix hardcoded non-breaking space in public view (#14568) --- app/views/statuses/_simple_status.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/statuses/_simple_status.html.haml b/app/views/statuses/_simple_status.html.haml index 36375794522..0c0b5711241 100644 --- a/app/views/statuses/_simple_status.html.haml +++ b/app/views/statuses/_simple_status.html.haml @@ -17,7 +17,7 @@ %span.display-name %bdi %strong.display-name__html.p-name.emojify= display_name(status.account, custom_emojify: true, autoplay: autoplay) -   + = ' ' %span.display-name__account = acct(status.account) = fa_icon('lock') if status.account.locked?