FIX: Don't throw 500 for invalid website url input

FIX: Don’t throw 500 for invalid website url input

It’s possible to cause a 500 error by putting in weird characters in the input field for updating a users website on their profile.

Normal invalid input like not including the domain extension is already handled by the user_profile model validation. This fix ensures a server error doesn’t occur for weird input characters.

diff --git a/app/services/user_updater.rb b/app/services/user_updater.rb
index 5b3396f..cdd5203 100644
--- a/app/services/user_updater.rb
+++ b/app/services/user_updater.rb
@@ -149,25 +149,30 @@ class UserUpdater
 
     saved = nil
 
-    User.transaction do
-      if attributes.key?(:muted_usernames)
-        update_muted_users(attributes[:muted_usernames])
-      end
+    begin
+      User.transaction do
+        if attributes.key?(:muted_usernames)
+          update_muted_users(attributes[:muted_usernames])
+        end
 
-      if attributes.key?(:ignored_usernames)
-        update_ignored_users(attributes[:ignored_usernames])
-      end
+        if attributes.key?(:ignored_usernames)
+          update_ignored_users(attributes[:ignored_usernames])
+        end
 
-      name_changed = user.name_changed?
-      if (saved = (!save_options || user.user_option.save) && user_profile.save && user.save) &&
-         (name_changed && old_user_name.casecmp(attributes.fetch(:name)) != 0)
+        name_changed = user.name_changed?
+        if (saved = (!save_options || user.user_option.save) && user_profile.save && user.save) &&
+           (name_changed && old_user_name.casecmp(attributes.fetch(:name)) != 0)
 
-        StaffActionLogger.new(@actor).log_name_change(
-          user.id,
-          old_user_name,
-          attributes.fetch(:name) { '' }
-        )
+          StaffActionLogger.new(@actor).log_name_change(
+            user.id,
+            old_user_name,
+            attributes.fetch(:name) { '' }
+          )
+        end
       end
+    rescue Addressable::URI::InvalidURIError => e
+      # Prevent 500 for crazy url input
+      return saved
     end
 
     DiscourseEvent.trigger(:user_updated, user) if saved
diff --git a/spec/services/user_updater_spec.rb b/spec/services/user_updater_spec.rb
index 5ca012f..c18949a 100644
--- a/spec/services/user_updater_spec.rb
+++ b/spec/services/user_updater_spec.rb
@@ -416,6 +416,15 @@ describe UserUpdater do
       end
     end
 
+    context 'when website is invalid' do
+      it 'returns an error' do
+        user = Fabricate(:user)
+        updater = UserUpdater.new(acting_user, user)
+
+        expect(updater.update(website: 'ʔ<')).to eq nil
+      end
+    end
+
     context 'when custom_fields is empty string' do
       it "update is successful" do
         user = Fabricate(:user)

GitHub sha: 9cbbaf42

Minor tip. Since Ruby 2.5, you can do put the rescue/ensure in do..end blocks

So something like

User.transaction do
  ...
rescue Addressable::URI::InvalidURIError => e
  ... 
end
1 Like