From 606629577aca6b2d72776ed29a1342cf39967d67 Mon Sep 17 00:00:00 2001 From: Georg Gadinger Date: Fri, 10 Feb 2023 20:48:15 +0100 Subject: [PATCH 1/2] make URI.parse part of the validation for the sharing URL the regexp alone and web browsers allows URLs to contain non-ASCII characters, which `URI.parse` does not like -- resulting in the inbox page to suddenly break. also changed the `redirect_to` in the controller to a `render :edit` so that validation errors are shown properly --- .../settings/sharing_controller.rb | 7 ++++--- app/models/user.rb | 2 +- app/validators/valid_url_validator.rb | 21 +++++++++++++++++++ config/locales/activerecord.en.yml | 3 +++ spec/models/user_spec.rb | 2 ++ 5 files changed, 31 insertions(+), 4 deletions(-) create mode 100644 app/validators/valid_url_validator.rb diff --git a/app/controllers/settings/sharing_controller.rb b/app/controllers/settings/sharing_controller.rb index a92bd364..76a7ca76 100644 --- a/app/controllers/settings/sharing_controller.rb +++ b/app/controllers/settings/sharing_controller.rb @@ -10,10 +10,11 @@ class Settings::SharingController < ApplicationController :sharing_autoclose, :sharing_custom_url) if current_user.update(user_attributes) - flash[:success] = t(".success") + flash.now[:success] = t(".success") else - flash[:error] = t(".error") + flash.now[:error] = t(".error") end - redirect_to settings_sharing_path + + render :edit end end diff --git a/app/models/user.rb b/app/models/user.rb index 7a57479d..6370ed01 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -61,7 +61,7 @@ class User < ApplicationRecord # rubocop:disable Metrics/ClassLength end validates :email, fake_email: true, typoed_email: true - validates :sharing_custom_url, format: URI::DEFAULT_PARSER.make_regexp(%w[http https]), allow_blank: true + validates :sharing_custom_url, allow_blank: true, valid_url: true validates :screen_name, presence: true, format: { with: SCREEN_NAME_REGEX, message: I18n.t("activerecord.validation.user.screen_name.format") }, diff --git a/app/validators/valid_url_validator.rb b/app/validators/valid_url_validator.rb new file mode 100644 index 00000000..0d29426c --- /dev/null +++ b/app/validators/valid_url_validator.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +class ValidUrlValidator < ActiveModel::EachValidator + URI_REGEXP = URI::DEFAULT_PARSER.make_regexp(%w[http https]).freeze + + def validate_each(record, attribute, value) + return if valid?(value) + + record.errors.add(attribute, :invalid_url) + end + + def valid?(value) + return false unless URI_REGEXP.match?(value) + + URI.parse(value) # raises URI::InvalidURIError + + true + rescue URI::InvalidURIError + false + end +end diff --git a/config/locales/activerecord.en.yml b/config/locales/activerecord.en.yml index c8066090..d7a879ae 100644 --- a/config/locales/activerecord.en.yml +++ b/config/locales/activerecord.en.yml @@ -104,6 +104,9 @@ en: user: screen_name: format: "contains invalid characters" + errors: + messages: + invalid_url: "does not look like a valid URL" helpers: submit: user: diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 3263fa86..01d3fc47 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -63,6 +63,8 @@ RSpec.describe User, type: :model do include_examples "valid url", "http://insecurebutvalid.business/" include_examples "invalid url", "ftp://fileprotocols.cool/" include_examples "invalid url", "notevenanurl" + include_examples "invalid url", %(https://richtig oarger shice) # passes the regexp, but trips up URI.parse + include_examples "invalid url", %(https://österreich.gv.at) # needs to be ASCII end describe "email validation" do From 71be21cccc43fcad610dc8509ae531abb522212e Mon Sep 17 00:00:00 2001 From: Georg Gadinger Date: Fri, 10 Feb 2023 21:16:21 +0100 Subject: [PATCH 2/2] add specs for Settings::SharingController --- .../settings/sharing_controller_spec.rb | 133 ++++++++++++++++++ 1 file changed, 133 insertions(+) create mode 100644 spec/controllers/settings/sharing_controller_spec.rb diff --git a/spec/controllers/settings/sharing_controller_spec.rb b/spec/controllers/settings/sharing_controller_spec.rb new file mode 100644 index 00000000..6755845a --- /dev/null +++ b/spec/controllers/settings/sharing_controller_spec.rb @@ -0,0 +1,133 @@ +# frozen_string_literal: true + +require "rails_helper" + +describe Settings::SharingController, type: :controller do + describe "#edit" do + subject { get :edit } + + it "redirects to the sign in page when not signed in" do + expect(subject).to redirect_to new_user_session_path + end + + context "user signed in" do + let(:user) { FactoryBot.create(:user) } + + before { sign_in user } + + it "renders the edit template" do + expect(subject).to render_template(:edit) + end + end + end + + describe "#update" do + subject { patch :update, params: { user: user_params } } + let(:user_params) do + { + sharing_enabled: "1", + sharing_autoclose: "1", + sharing_custom_url: "", + } + end + + it "redirects to the sign in page when not signed in" do + expect(subject).to redirect_to new_user_session_path + end + + context "user signed in" do + let(:user) { FactoryBot.create :user } + + before { sign_in user } + + it "renders the edit template" do + expect(subject).to render_template(:edit) + end + + it "updates the user's sharing preferences" do + expect { subject } + .to change { user.reload.slice(:sharing_enabled, :sharing_autoclose, :sharing_custom_url).values } + .from([false, false, nil]) + .to([true, true, ""]) + end + + it "sets the success flash" do + subject + expect(flash[:success]).to eq(I18n.t("settings.sharing.update.success")) + end + + context "when passed a valid url" do + let(:user_params) do + super().merge( + sharing_custom_url: "https://example.com/share?text=" + ) + end + + it "renders the edit template" do + expect(subject).to render_template(:edit) + end + + it "updates the user's sharing preferences" do + expect { subject } + .to change { user.reload.slice(:sharing_enabled, :sharing_autoclose, :sharing_custom_url).values } + .from([false, false, nil]) + .to([true, true, "https://example.com/share?text="]) + end + + it "sets the success flash" do + subject + expect(flash[:success]).to eq(I18n.t("settings.sharing.update.success")) + end + end + + context "when passed an invalid url" do + let(:user_params) do + super().merge( + sharing_custom_url: "nfs://example.com/data" + ) + end + + it "renders the edit template" do + expect(subject).to render_template(:edit) + end + + it "updates the user's sharing preferences" do + expect { subject } + .not_to(change { user.reload.slice(:sharing_enabled, :sharing_autoclose, :sharing_custom_url).values }) + end + + it "sets the error flash" do + subject + expect(flash[:error]).to eq(I18n.t("settings.sharing.update.error")) + end + end + + context "when unticking boolean settings" do + let(:user_params) do + super().merge( + sharing_enabled: "0", + sharing_autoclose: "0" + ) + end + + let(:user) { FactoryBot.create :user, sharing_enabled: true, sharing_autoclose: true, sharing_custom_url: "https://example.com/share?text=" } + + it "renders the edit template" do + expect(subject).to render_template(:edit) + end + + it "updates the user's sharing preferences" do + expect { subject } + .to change { user.reload.slice(:sharing_enabled, :sharing_autoclose, :sharing_custom_url).values } + .from([true, true, "https://example.com/share?text="]) + .to([false, false, ""]) + end + + it "sets the success flash" do + subject + expect(flash[:success]).to eq(I18n.t("settings.sharing.update.success")) + end + end + end + end +end