From 901054fd75f986bfaa8726cade381b365023be56 Mon Sep 17 00:00:00 2001
From: Martin Brennan <mjrbrennan@gmail.com>
Date: Thu, 28 Nov 2019 07:48:29 +1000
Subject: [PATCH] FIX: Cache failed onebox URL request server-side (#8421)

We already cache failed onebox URL requests client-side, we now want to cache this on the server-side for extra protection. failed onebox previews will be cached for 1 hour, and any more requests for that URL will fail with a 404 status. Forcing a rebake via the Rebake HTML action will delete the failed URL cache (like how the oneboxer preview cache is deleted).
---
 app/controllers/onebox_controller.rb    |  3 +++
 lib/oneboxer.rb                         | 13 ++++++++++++
 spec/components/oneboxer_spec.rb        | 11 ++++++++++
 spec/requests/onebox_controller_spec.rb | 27 +++++++++++++++++++++----
 4 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/app/controllers/onebox_controller.rb b/app/controllers/onebox_controller.rb
index 10c500317b7..f656ba34146 100644
--- a/app/controllers/onebox_controller.rb
+++ b/app/controllers/onebox_controller.rb
@@ -19,6 +19,8 @@ class OneboxController < ApplicationController
     invalidate = params[:refresh] == 'true'
     url = params[:url]
 
+    return render(body: nil, status: 404) if Oneboxer.recently_failed?(url)
+
     hijack do
       Oneboxer.preview_onebox!(user_id)
 
@@ -34,6 +36,7 @@ class OneboxController < ApplicationController
       Oneboxer.onebox_previewed!(user_id)
 
       if preview.blank?
+        Oneboxer.cache_failed!(url)
         render body: nil, status: 404
       else
         render plain: preview
diff --git a/lib/oneboxer.rb b/lib/oneboxer.rb
index 695f2f19149..a594499f266 100644
--- a/lib/oneboxer.rb
+++ b/lib/oneboxer.rb
@@ -72,6 +72,7 @@ module Oneboxer
 
   def self.invalidate(url)
     Discourse.cache.delete(onebox_cache_key(url))
+    Discourse.cache.delete(onebox_failed_cache_key(url))
   end
 
   # Parse URLs out of HTML, returning the document when finished.
@@ -136,6 +137,14 @@ module Oneboxer
     Onebox::Matcher.new(url).oneboxed
   end
 
+  def self.recently_failed?(url)
+    Discourse.cache.read(onebox_failed_cache_key(url)).present?
+  end
+
+  def self.cache_failed!(url)
+    Discourse.cache.write(onebox_failed_cache_key(url), true, expires_in: 1.hour)
+  end
+
   private
 
   def self.preview_key(user_id)
@@ -150,6 +159,10 @@ module Oneboxer
     "onebox__#{url}"
   end
 
+  def self.onebox_failed_cache_key(url)
+    "onebox_failed__#{url}"
+  end
+
   def self.onebox_raw(url, opts = {})
     url = URI(url).to_s
     local_onebox(url, opts) || external_onebox(url)
diff --git a/spec/components/oneboxer_spec.rb b/spec/components/oneboxer_spec.rb
index f90eb5ba755..c04287ecb9d 100644
--- a/spec/components/oneboxer_spec.rb
+++ b/spec/components/oneboxer_spec.rb
@@ -12,6 +12,17 @@ describe Oneboxer do
     expect(Oneboxer.onebox("http://boom.com")).to eq("")
   end
 
+  describe "#invalidate" do
+    let(:url) { "http://test.com" }
+    it "clears the cached preview for the onebox URL and the failed URL cache" do
+      Discourse.cache.write(Oneboxer.onebox_cache_key(url), "test")
+      Discourse.cache.write(Oneboxer.onebox_failed_cache_key(url), true)
+      Oneboxer.invalidate(url)
+      expect(Discourse.cache.read(Oneboxer.onebox_cache_key(url))).to eq(nil)
+      expect(Discourse.cache.read(Oneboxer.onebox_failed_cache_key(url))).to eq(nil)
+    end
+  end
+
   context "local oneboxes" do
 
     def link(url)
diff --git a/spec/requests/onebox_controller_spec.rb b/spec/requests/onebox_controller_spec.rb
index 1d14499e414..babe3104760 100644
--- a/spec/requests/onebox_controller_spec.rb
+++ b/spec/requests/onebox_controller_spec.rb
@@ -4,6 +4,10 @@ require 'rails_helper'
 
 describe OneboxController do
 
+  before do
+    Discourse.cache.delete(Oneboxer.onebox_failed_cache_key(url))
+  end
+
   let(:url) { "http://google.com" }
 
   it "requires the user to be logged in" do
@@ -112,19 +116,34 @@ describe OneboxController do
     end
 
     describe "missing onebox" do
-      it "returns 404 if the onebox is nil" do
+      def stub_request_to_onebox_url(response_body)
         stub_request(:head, url)
-        stub_request(:get, url).to_return(body: nil).then.to_raise
+        stub_request(:get, url).to_return(body: response_body).then.to_raise
+      end
+
+      it "returns 404 if the onebox is nil" do
+        stub_request_to_onebox_url(nil)
         get "/onebox.json", params: { url: url, refresh: "true" }
         expect(response.response_code).to eq(404)
       end
 
       it "returns 404 if the onebox is an empty string" do
-        stub_request(:head, url)
-        stub_request(:get, url).to_return(body: " \t ").then.to_raise
+        stub_request_to_onebox_url(" \t ")
         get "/onebox.json", params: { url: url, refresh: "true" }
         expect(response.response_code).to eq(404)
       end
+
+      it "cases missing onebox URLs so we do not attempt to preview again" do
+        stub_request_to_onebox_url(nil)
+        get "/onebox.json", params: { url: url, refresh: "true" }
+        expect(response.response_code).to eq(404)
+        Oneboxer.expects(:preview_onebox!).never
+        get "/onebox.json", params: { url: url, refresh: "true" }
+        expect(response.response_code).to eq(404)
+        expect(
+          Discourse.cache.read(Oneboxer.onebox_failed_cache_key(url))
+        ).not_to eq(nil)
+      end
     end
 
     describe "local onebox" do