Merge pull request #1063 from Retrospring/no-invalid-urls

make URI.parse part of the validation for the sharing URL

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

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") },

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

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:

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

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 <strong>oarger</strong> 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