FIX: Do not try to recover invalid Upload#short_url in UploadRecovery.

lib/upload_recovery.rb

@@ -34,9 +34,12 @@ class UploadRecovery
   private
 
   def recover_post_upload(post, short_url)
+    sha1 = Upload.sha1_from_short_url(short_url)
+    return unless sha1.present?
+
     attributes = {
       post: post,
-      sha1: Upload.sha1_from_short_url(short_url)
+      sha1: sha1
     }
 
     if Discourse.store.external?
@@ -47,8 +50,6 @@ class UploadRecovery
   end
 
   def recover_from_local(post:, sha1:)
-    return unless sha1.present?
-
     public_path = Rails.root.join("public")
 
     @paths ||= begin

spec/lib/upload_recovery_spec.rb

@@ -18,31 +18,45 @@ RSpec.describe UploadRecovery do
     ).tap(&:link_post_uploads)
   end
 
+  let(:upload_recovery) { UploadRecovery.new }
+
   before do
     SiteSetting.queue_jobs = false
   end
 
   describe '#recover' do
-    it 'should recover the upload' do
-      begin
-        stub_request(:get, "http://test.localhost#{upload.url}")
-          .to_return(status: 200)
+    after do
+      public_path = "#{Discourse.store.public_dir}#{upload.url}"
 
-        expect do
-          upload.destroy!
-        end.to change { post.reload.uploads.count }.from(1).to(0)
+      [
+        public_path,
+        public_path.sub("uploads", "uploads/tombstone")
+      ].each { |path| File.delete(path) if File.exists?(path) }
+    end
 
-        expect do
-          UploadRecovery.new.recover
-        end.to change { post.reload.uploads.count }.from(0).to(1)
-      ensure
-        public_path = "#{Discourse.store.public_dir}#{upload.url}"
+    describe 'when given an invalid sha1' do
+      it 'should not do anything' do
+        upload_recovery.expects(:recover_from_local).never
 
-        [
-          public_path,
-          public_path.sub("uploads", "uploads/tombstone")
-        ].each { |path| File.delete(path) if File.exists?(path) }
+        post.update!(
+          raw: "![logo.png](upload://#{'a' * 28}.png)"
+        )
+
+        upload_recovery.recover
       end
     end
+
+    it 'should recover the upload' do
+      stub_request(:get, "http://test.localhost#{upload.url}")
+        .to_return(status: 200)
+
+      expect do
+        upload.destroy!
+      end.to change { post.reload.uploads.count }.from(1).to(0)
+
+      expect do
+        upload_recovery.recover
+      end.to change { post.reload.uploads.count }.from(0).to(1)
+    end
   end
 end