From 1d6597c6466d3a83be3ef971c0391b003d92f5eb Mon Sep 17 00:00:00 2001
From: Guo Xiang Tan <tgx_world@hotmail.com>
Date: Thu, 13 Sep 2018 13:59:17 +0800
Subject: [PATCH] FIX: Do not try to recover invalid `Upload#short_url` in
 `UploadRecovery`.

---
 lib/upload_recovery.rb           |  7 ++---
 spec/lib/upload_recovery_spec.rb | 46 +++++++++++++++++++++-----------
 2 files changed, 34 insertions(+), 19 deletions(-)

diff --git a/lib/upload_recovery.rb b/lib/upload_recovery.rb
index 70021d29ac4..8eb1e7725a8 100644
--- a/lib/upload_recovery.rb
+++ b/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
diff --git a/spec/lib/upload_recovery_spec.rb b/spec/lib/upload_recovery_spec.rb
index 1d3f74a9d55..01334c5e7e5 100644
--- a/spec/lib/upload_recovery_spec.rb
+++ b/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