FIX: ensure local images use local CDN when uploads are stored on S3

When the S3 store was enabled, we were only applying the S3 CDN.
So all images stored locally, like the emojis, were never put on the local CDN.

Fixed a bunch of CookedPostProcessor test by adding a call to 'optimize_urls'
in order to get final URLs.

I also removed the unnecessary PrettyText.add_s3_cdn method since this is already
handled in the CookedPostProcessor.

lib/cooked_post_processor.rb

@@ -389,7 +389,7 @@ class CookedPostProcessor
     if upload
       thumbnail = upload.thumbnail(w, h)
       if thumbnail && thumbnail.filesize.to_i < upload.filesize
-        img["src"] = UrlHelper.cook_url(thumbnail.url)
+        img["src"] = thumbnail.url
 
         srcset = +""
 
@@ -408,11 +408,11 @@ class CookedPostProcessor
           img["srcset"] = "#{UrlHelper.cook_url(img["src"])}#{srcset}" if srcset.present?
         end
       else
-        img["src"] = UrlHelper.cook_url(upload.url)
+        img["src"] = upload.url
       end
 
       if small_upload = loading_image(upload)
-        img["data-small-upload"] = UrlHelper.cook_url(small_upload.url)
+        img["data-small-upload"] = small_upload.url
       end
     end
 
@@ -588,8 +588,10 @@ class CookedPostProcessor
       end
     end
 
-    @doc.css("img[src]").each do |img|
+    %w{src data-small-upload}.each do |selector|
-      img["src"] = UrlHelper.cook_url(img["src"].to_s)
+      @doc.css("img[#{selector}]").each do |img|
+        img[selector] = UrlHelper.cook_url(img[selector].to_s)
+      end
     end
   end
 

lib/file_store/local_store.rb

@@ -46,8 +46,7 @@ module FileStore
     end
 
     def cdn_url(url)
-      return url if Discourse.asset_host.blank?
+      UrlHelper.local_cdn_url(url)
-      url.sub(Discourse.base_url_no_prefix, Discourse.asset_host)
     end
 
     def path_for(upload)

lib/pretty_text.rb

@@ -254,10 +254,6 @@ module PrettyText
       add_rel_nofollow_to_user_content(doc)
     end
 
-    if SiteSetting.Upload.enable_s3_uploads && SiteSetting.Upload.s3_cdn_url.present?
-      add_s3_cdn(doc)
-    end
-
     if SiteSetting.enable_mentions
       add_mentions(doc, user_id: opts[:user_id])
     end
@@ -265,13 +261,6 @@ module PrettyText
     doc.to_html
   end
 
-  def self.add_s3_cdn(doc)
-    doc.css("img").each do |img|
-      next unless img["src"]
-      img["src"] = Discourse.store.cdn_url(img["src"])
-    end
-  end
-
   def self.add_rel_nofollow_to_user_content(doc)
     whitelist = []
 

lib/url_helper.rb

@@ -56,11 +56,20 @@ class UrlHelper
     no_cdn = SiteSetting.login_required || SiteSetting.prevent_anons_from_downloading_files
 
     url = absolute_without_cdn(url)
-    url = Discourse.store.cdn_url(url) unless is_attachment && no_cdn
+
+    unless is_attachment && no_cdn
+      url = Discourse.store.cdn_url(url)
+      url = local_cdn_url(url) if Discourse.store.external?
+    end
 
     schemaless(url)
   rescue URI::Error
     url
   end
 
+  def self.local_cdn_url(url)
+    return url if Discourse.asset_host.blank?
+    url.sub(Discourse.base_url_no_prefix, Discourse.asset_host)
+  end
+
 end

spec/components/cooked_post_processor_spec.rb

@@ -1,5 +1,6 @@
 require "rails_helper"
 require "cooked_post_processor"
+require "file_store/s3_store"
 
 describe CookedPostProcessor do
   context "#post_process" do
@@ -304,6 +305,7 @@ describe CookedPostProcessor do
 
         cpp.add_to_size_cache(upload.url, 2000, 1500)
         cpp.post_process_images
+        cpp.optimize_urls
 
         html = cpp.html
 
@@ -318,6 +320,8 @@ describe CookedPostProcessor do
         cpp = CookedPostProcessor.new(post)
         cpp.add_to_size_cache(upload.url, 2000, 1500)
         cpp.post_process_images
+        cpp.optimize_urls
+
         html = cpp.html
 
         expect(html).to include(%Q|data-small-upload="//cdn.localhost/uploads/default/10x10.png"|)
@@ -343,6 +347,7 @@ describe CookedPostProcessor do
         cpp = CookedPostProcessor.new(post)
         cpp.add_to_size_cache(upload.url, 200, 4000)
         cpp.post_process_images
+        cpp.optimize_urls
 
         expect(cpp.html).to_not include('srcset="')
       end
@@ -360,7 +365,10 @@ describe CookedPostProcessor do
       let(:post) { Fabricate(:post_with_image_urls) }
       let(:cpp) { CookedPostProcessor.new(post, image_sizes: image_sizes) }
 
-      before { cpp.post_process_images }
+      before do
+        cpp.post_process_images
+        cpp.optimize_urls
+      end
 
       context "valid" do
         let(:image_sizes) { { "http://foo.bar/image.png" => { "width" => 111, "height" => 222 } } }
@@ -422,9 +430,14 @@ describe CookedPostProcessor do
         FileStore::BaseStore.any_instance.expects(:get_depth_for).returns(0)
 
         cpp.post_process_images
-        expect(cpp.html).to match_html "<p><div class=\"lightbox-wrapper\"><a class=\"lightbox\" href=\"/uploads/default/original/1X/1234567890123456.jpg\" data-download-href=\"/uploads/default/#{upload.sha1}\" title=\"logo.png\"><img src=\"//test.localhost/uploads/default/optimized/1X/#{upload.sha1}_#{OptimizedImage::VERSION}_690x788.png\" width=\"690\" height=\"788\"><div class=\"meta\">
+        cpp.optimize_urls
-<span class=\"filename\">logo.png</span><span class=\"informations\">1750×2000 1.21 KB</span><span class=\"expand\"></span>
+
-</div></a></div></p>"
+        expect(cpp.html).to match_html <<~HTML
+          <p><div class="lightbox-wrapper"><a class="lightbox" href="//test.localhost/uploads/default/original/1X/1234567890123456.jpg" data-download-href="//test.localhost/uploads/default/#{upload.sha1}" title="logo.png"><img src="//test.localhost/uploads/default/optimized/1X/#{upload.sha1}_#{OptimizedImage::VERSION}_690x788.png" width="690" height="788"><div class="meta">
+          <span class="filename">logo.png</span><span class="informations">1750×2000 1.21 KB</span><span class="expand"></span>
+          </div></a></div></p>
+        HTML
+
         expect(cpp).to be_dirty
       end
 
@@ -439,8 +452,11 @@ describe CookedPostProcessor do
         it 'should not add lightbox' do
           cpp.post_process_oneboxes
           cpp.post_process_images
+          cpp.optimize_urls
 
-          expect(cpp.html).to match_html("<p><img class=\"onebox\" src=\"/uploads/default/original/1X/1234567890123456.jpg\" width=\"690\"\ height=\"788\"></p>")
+          expect(cpp.html).to match_html <<~HTML
+            <p><img class="onebox" src="//test.localhost/uploads/default/original/1X/1234567890123456.jpg" width="690" height="788"></p>
+          HTML
         end
       end
 
@@ -451,8 +467,11 @@ describe CookedPostProcessor do
 
         it 'should not add lightbox' do
           cpp.post_process_images
+          cpp.optimize_urls
 
-          expect(cpp.html).to match_html("<p><img src=\"/uploads/default/original/1X/1234567890123456.svg\" width=\"690\"\ height=\"788\"></p>")
+          expect(cpp.html).to match_html <<~HTML
+            <p><img src="//test.localhost/uploads/default/original/1X/1234567890123456.svg" width="690" height="788"></p>
+          HTML
         end
 
         describe 'when image src is an URL' do
@@ -463,6 +482,7 @@ describe CookedPostProcessor do
           it 'should not add lightbox' do
             SiteSetting.crawl_images = true
             cpp.post_process_images
+            cpp.optimize_urls
 
             expect(cpp.html).to match_html("<p><img src=\"http://test.discourse/uploads/default/original/1X/1234567890123456.svg?somepamas\" width=\"690\"\ height=\"788\"></p>")
           end
@@ -515,9 +535,14 @@ describe CookedPostProcessor do
 
       it "crops the image" do
         cpp.post_process_images
-        expect(cpp.html).to match_html "<p><div class=\"lightbox-wrapper\"><a class=\"lightbox\" href=\"/uploads/default/original/1X/1234567890123456.jpg\" data-download-href=\"/uploads/default/#{upload.sha1}\" title=\"logo.png\"><img src=\"//test.localhost/uploads/default/optimized/1X/#{upload.sha1}_#{OptimizedImage::VERSION}_230x500.png\" width=\"230\" height=\"500\"><div class=\"meta\">
+        cpp.optimize_urls
-<span class=\"filename\">logo.png</span><span class=\"informations\">1125×2436 1.21 KB</span><span class=\"expand\"></span>
+
-</div></a></div></p>"
+        expect(cpp.html).to match_html <<~HTML
+          <p><div class="lightbox-wrapper"><a class="lightbox" href="//test.localhost/uploads/default/original/1X/1234567890123456.jpg" data-download-href="//test.localhost/uploads/default/#{upload.sha1}" title="logo.png"><img src="//test.localhost/uploads/default/optimized/1X/#{upload.sha1}_#{OptimizedImage::VERSION}_230x500.png" width="230" height="500"><div class="meta">
+          <span class="filename">logo.png</span><span class="informations">1125×2436 1.21 KB</span><span class="expand"></span>
+          </div></a></div></p>
+        HTML
+
         expect(cpp).to be_dirty
       end
 
@@ -546,18 +571,27 @@ describe CookedPostProcessor do
 
       it "generates overlay information" do
         cpp.post_process_images
-        expect(cpp.html).to match_html "<p><div class=\"lightbox-wrapper\"><a class=\"lightbox\" href=\"/subfolder/uploads/default/original/1X/1234567890123456.jpg\" data-download-href=\"/subfolder/uploads/default/#{upload.sha1}\" title=\"logo.png\"><img src=\"//test.localhost/subfolder/uploads/default/optimized/1X/#{upload.sha1}_#{OptimizedImage::VERSION}_690x788.png\" width=\"690\" height=\"788\"><div class=\"meta\">
+        cpp.optimize_urls
-<span class=\"filename\">logo.png</span><span class=\"informations\">1750×2000 1.21 KB</span><span class=\"expand\"></span>
+
-</div></a></div></p>"
+        expect(cpp.html). to match_html <<~HTML
+          <p><div class="lightbox-wrapper"><a class="lightbox" href="//test.localhost/subfolder/uploads/default/original/1X/1234567890123456.jpg" data-download-href="//test.localhost/subfolder/uploads/default/#{upload.sha1}" title="logo.png"><img src="//test.localhost/subfolder/uploads/default/optimized/1X/#{upload.sha1}_#{OptimizedImage::VERSION}_690x788.png" width="690" height="788"><div class="meta">
+          <span class="filename">logo.png</span><span class="informations">1750×2000 1.21 KB</span><span class="expand"></span>
+          </div></a></div></p>
+        HTML
+
         expect(cpp).to be_dirty
       end
 
       it "should escape the filename" do
         upload.update_attributes!(original_filename: "><img src=x onerror=alert('haha')>.png")
         cpp.post_process_images
-        expect(cpp.html).to match_html "<p><div class=\"lightbox-wrapper\"><a class=\"lightbox\" href=\"/subfolder/uploads/default/original/1X/1234567890123456.jpg\" data-download-href=\"/subfolder/uploads/default/#{upload.sha1}\" title=\"&amp;gt;&amp;lt;img src=x onerror=alert(&amp;#39;haha&amp;#39;)&amp;gt;.png\"><img src=\"//test.localhost/subfolder/uploads/default/optimized/1X/#{upload.sha1}_#{OptimizedImage::VERSION}_690x788.png\" width=\"690\" height=\"788\"><div class=\"meta\">
+        cpp.optimize_urls
-<span class=\"filename\">&amp;gt;&amp;lt;img src=x onerror=alert(&amp;#39;haha&amp;#39;)&amp;gt;.png</span><span class=\"informations\">1750×2000 1.21 KB</span><span class=\"expand\"></span>
+
-</div></a></div></p>"
+        expect(cpp.html).to match_html <<~HTML
+          <p><div class="lightbox-wrapper"><a class="lightbox" href="//test.localhost/subfolder/uploads/default/original/1X/1234567890123456.jpg" data-download-href="//test.localhost/subfolder/uploads/default/#{upload.sha1}" title="&amp;gt;&amp;lt;img src=x onerror=alert(&amp;#39;haha&amp;#39;)&amp;gt;.png"><img src="//test.localhost/subfolder/uploads/default/optimized/1X/#{upload.sha1}_#{OptimizedImage::VERSION}_690x788.png" width="690" height="788"><div class="meta">
+          <span class="filename">&amp;gt;&amp;lt;img src=x onerror=alert(&amp;#39;haha&amp;#39;)&amp;gt;.png</span><span class="informations">1750×2000 1.21 KB</span><span class="expand"></span>
+          </div></a></div></p>
+        HTML
       end
 
     end
@@ -581,9 +615,14 @@ describe CookedPostProcessor do
 
       it "generates overlay information" do
         cpp.post_process_images
-        expect(cpp.html).to match_html "<p><div class=\"lightbox-wrapper\"><a class=\"lightbox\" href=\"/uploads/default/original/1X/1234567890123456.jpg\" data-download-href=\"/uploads/default/#{upload.sha1}\" title=\"WAT\"><img src=\"//test.localhost/uploads/default/optimized/1X/#{upload.sha1}_#{OptimizedImage::VERSION}_690x788.png\" title=\"WAT\" width=\"690\" height=\"788\"><div class=\"meta\">
+        cpp.optimize_urls
-       <span class=\"filename\">WAT</span><span class=\"informations\">1750×2000 1.21 KB</span><span class=\"expand\"></span>
+
-       </div></a></div></p>"
+        expect(cpp.html).to match_html <<~HTML
+          <p><div class="lightbox-wrapper"><a class="lightbox" href="//test.localhost/uploads/default/original/1X/1234567890123456.jpg" data-download-href="//test.localhost/uploads/default/#{upload.sha1}" title="WAT"><img src="//test.localhost/uploads/default/optimized/1X/#{upload.sha1}_#{OptimizedImage::VERSION}_690x788.png" title="WAT" width="690" height="788"><div class="meta">
+          <span class="filename">WAT</span><span class="informations">1750×2000 1.21 KB</span><span class="expand"></span>
+          </div></a></div></p>
+        HTML
+
         expect(cpp).to be_dirty
       end
 
@@ -828,6 +867,7 @@ describe CookedPostProcessor do
       cpp = CookedPostProcessor.new(post, invalidate_oneboxes: true)
       cpp.post_process_oneboxes
       cpp.post_process_images
+      cpp.optimize_urls
 
       expect(cpp.doc.to_s).to match(/<div class="large-image-placeholder">/)
     end
@@ -960,6 +1000,38 @@ describe CookedPostProcessor do
         HTML
       end
 
+      it "uses the right CDN when uploads are on S3" do
+        Rails.configuration.action_controller.stubs(:asset_host).returns("https://local.cdn.com")
+
+        SiteSetting.s3_upload_bucket = "some-bucket-on-s3"
+        SiteSetting.s3_access_key_id = "s3-access-key-id"
+        SiteSetting.s3_secret_access_key = "s3-secret-access-key"
+        SiteSetting.s3_cdn_url = "https://s3.cdn.com"
+        SiteSetting.enable_s3_uploads = true
+
+        uploaded_file = file_from_fixtures("smallest.png")
+        upload_sha1 = Digest::SHA1.hexdigest(File.read(uploaded_file))
+        upload = Fabricate(:upload,
+          original_filename: "smallest.png",
+          width: 10,
+          height: 20,
+          sha1: upload_sha1,
+          extension: "png",
+        )
+
+        upload.update_column(:url, "#{SiteSetting.Upload.absolute_base_url}/#{Discourse.store.get_path_for_upload(upload)}")
+
+        the_post = Fabricate(:post, raw: %Q{This post has a local emoji :+1: and an external upload\n\n![smallest.png|10x20](#{upload.short_url})})
+
+        cpp = CookedPostProcessor.new(the_post)
+        cpp.optimize_urls
+
+        expect(cpp.html).to match_html <<~HTML
+          <p>This post has a local emoji <img src="https://local.cdn.com/images/emoji/twitter/+1.png?v=6" title=":+1:" class="emoji" alt=":+1:"> and an external upload</p>
+          <p><img src="https://s3.cdn.com/original/1X/#{upload_sha1}.png" alt="smallest.png" width="10" height="20"></p>
+        HTML
+      end
+
     end
 
   end

spec/components/pretty_text_spec.rb

@@ -779,81 +779,6 @@ describe PrettyText do
     expect(PrettyText.cook(raw)).to eq(html.strip)
   end
 
-  describe 's3_cdn' do
-
-    def test_s3_cdn
-      # add extra img tag to ensure it does not blow up
-      raw = <<~HTML
-        <img>
-        <img src='https:#{Discourse.store.absolute_base_url}/original/9/9/99c9384b8b6d87f8509f8395571bc7512ca3cad1.jpg'>
-        <img src='http:#{Discourse.store.absolute_base_url}/original/9/9/99c9384b8b6d87f8509f8395571bc7512ca3cad1.jpg'>
-        <img src='#{Discourse.store.absolute_base_url}/original/9/9/99c9384b8b6d87f8509f8395571bc7512ca3cad1.jpg'>
-      HTML
-
-      html = <<~HTML
-        <p><img><br>
-        <img src="https://awesome.cdn/original/9/9/99c9384b8b6d87f8509f8395571bc7512ca3cad1.jpg"><br>
-        <img src="https://awesome.cdn/original/9/9/99c9384b8b6d87f8509f8395571bc7512ca3cad1.jpg"><br>
-        <img src="https://awesome.cdn/original/9/9/99c9384b8b6d87f8509f8395571bc7512ca3cad1.jpg"></p>
-      HTML
-
-      expect(PrettyText.cook(raw)).to eq(html.strip)
-    end
-
-    before do
-      GlobalSetting.reset_s3_cache!
-    end
-
-    after do
-      GlobalSetting.reset_s3_cache!
-    end
-
-    it 'can substitute s3 cdn when added via global setting' do
-
-      global_setting :s3_access_key_id, 'XXX'
-      global_setting :s3_secret_access_key, 'XXX'
-      global_setting :s3_bucket, 'XXX'
-      global_setting :s3_region, 'XXX'
-      global_setting :s3_cdn_url, 'https://awesome.cdn'
-
-      test_s3_cdn
-    end
-
-    it 'can substitute s3 cdn correctly' do
-      SiteSetting.s3_access_key_id = "XXX"
-      SiteSetting.s3_secret_access_key = "XXX"
-      SiteSetting.s3_upload_bucket = "test"
-      SiteSetting.s3_cdn_url = "https://awesome.cdn"
-
-      SiteSetting.enable_s3_uploads = true
-
-      test_s3_cdn
-    end
-
-    def test_s3_with_subfolder_cdn
-      raw = <<~RAW
-        <img src='https:#{Discourse.store.absolute_base_url}/subfolder/original/9/9/99c9384b8b6d87f8509f8395571bc7512ca3cad1.jpg'>
-      RAW
-
-      html = <<~HTML
-        <p><img src="https://awesome.cdn/subfolder/original/9/9/99c9384b8b6d87f8509f8395571bc7512ca3cad1.jpg"></p>
-      HTML
-
-      expect(PrettyText.cook(raw)).to eq(html.strip)
-    end
-
-    it 'can substitute s3 with subfolder cdn when added via global setting' do
-
-      global_setting :s3_access_key_id, 'XXX'
-      global_setting :s3_secret_access_key, 'XXX'
-      global_setting :s3_bucket, 'XXX/subfolder'
-      global_setting :s3_region, 'XXX'
-      global_setting :s3_cdn_url, 'https://awesome.cdn/subfolder'
-
-      test_s3_with_subfolder_cdn
-    end
-  end
-
   describe "emoji" do
     it "replaces unicode emoji with our emoji sets if emoji is enabled" do
       expect(PrettyText.cook("💣")).to match(/\:bomb\:/)