Check validity of embed URLs (#16704)

* start link validation check

* keep PRs small

* rearrange check flow

* need a change to switch accounts

* Rescue from SocketError to surface a better error message

* some updates

* handle 301s

* address Travis failures

* implement request stub

* add needed unified-embed tag spec

* add needed unified-embed tag spec

* nudge Travis

* Address PR review comments

* special-case displaying "base"

* undo change to errors.add

* nudge Travis

* more succinct code

Co-authored-by: aritmock <aritmock@example.com>
Co-authored-by: Dwight Scott <dwight@forem.com>

app/javascript/article-form/components/ErrorList.jsx

@@ -12,9 +12,7 @@ export const ErrorList = ({ errors }) => {
         {Object.keys(errors).map((key) => {
           return (
             <li key={key}>
-              {key}
-              {`: `}
-              {errors[key]}
+              {key === 'base' ? errors[key] : `${key}: ${errors[key]}`}
             </li>
           );
         })}

app/liquid_tags/loom_tag.rb

@@ -20,7 +20,8 @@ class LoomTag < LiquidTagBase
   end
 
   def extract_video_id(input)
-    match = pattern_match_for(input, [REGISTRY_REGEXP])
+    input_params_removed = input.split("?")[0] # Loom URLs with params fail valid-URL check
+    match = pattern_match_for(input_params_removed, [REGISTRY_REGEXP])
     raise StandardError, I18n.t("liquid_tags.loom_tag.invalid_loom_url") unless match
 
     match[:video_id]

app/liquid_tags/unified_embed/tag.rb

@@ -1,3 +1,5 @@
+require "net/http"
+
 module UnifiedEmbed
   # This liquid tag is present to facilitate a unified user experience
   # for declaring that they want a URL to have "embedded" behavior.
@@ -21,21 +23,41 @@ module UnifiedEmbed
     # @return [LiquidTagBase]
     def self.new(tag_name, link, parse_context)
       stripped_link = ActionController::Base.helpers.strip_tags(link).strip
-      klass = UnifiedEmbed::Registry.find_liquid_tag_for(link: stripped_link)
-      # If we can't find a registered "embed" tag, let's raise an exception.
-      # This exception will give the user an opportunity to adjust their approach.
-      #
-      # In a prior implementation, we chose to render an A-tag using the given URL.
-      # With that prior implementation, a user expecting a "rich embed" might not
-      # notice that they didn't have a rich embed and instead published a basic
-      # A-tag. In addition, said A-tag would goes nowhere; which may confuse
-      # users and/or Forem readers.
-      raise StandardError, I18n.t("liquid_tags.unified_embed.tag.invalid_url") unless klass
+
+      # Before matching against the embed registry, we check if the link
+      # is valid (e.g. no typos).
+      # If the link is invalid, we raise an error encouraging the user to
+      # check their link and try again.
+      validated_link = validate_link(stripped_link)
+      klass = UnifiedEmbed::Registry.find_liquid_tag_for(link: validated_link)
+
+      # If the link is valid but doesn't match the registry, we return
+      # an "unsupported URL" error. Eventually we shall render a fallback
+      # embed using OpenGraph/TwitterCard metadata (if available).
+      # If there are no OG metatags, then we render an A-tag. Since the link
+      # has been validated, at least this A-tag will not 404.
+      raise StandardError, I18n.t("liquid_tags.unified_embed.tag.unsupported_url") unless klass
 
       # Why the __send__?  Because a LiquidTagBase class "privatizes"
       # the `.new` method.  And we want to instantiate the specific
       # liquid tag for the given link.
-      klass.__send__(:new, tag_name, stripped_link, parse_context)
+      klass.__send__(:new, tag_name, validated_link, parse_context)
+    end
+
+    def self.validate_link(link)
+      uri = URI.parse(link)
+      http = Net::HTTP.new(uri.host, uri.port)
+      http.use_ssl = true if http.port == 443
+      path = uri.path.presence || "/"
+      response = http.request_head(path)
+
+      unless response.is_a?(Net::HTTPSuccess) || response.is_a?(Net::HTTPMovedPermanently)
+        raise StandardError, I18n.t("liquid_tags.unified_embed.tag.not_found")
+      end
+
+      link
+    rescue SocketError
+      raise StandardError, I18n.t("liquid_tags.unified_embed.tag.invalid_url")
     end
   end
 end

config/locales/liquid_tags/en.yml

@@ -99,7 +99,9 @@ en:
       invalid_url: Invalid Twitter Timeline URL
     unified_embed:
       tag:
-        invalid_url: Embed URL not valid
+        invalid_url: URL provided may have a typo or error; please check and try again
+        not_found: URL provided was not found; please check and try again
+        unsupported_url: Embeds for this URL are not supported
     user_subscription_tag:
       submit: Submit
       submitting: Submitting...

config/locales/liquid_tags/fr.yml

@@ -99,7 +99,9 @@ fr:
       invalid_url: Invalid Twitter Timeline URL
     unified_embed:
       tag:
-        invalid_url: Embed URL not valid
+        invalid_url: URL provided may have a typo or error; please check and try again
+        not_found: URL provided was not found; please check and try again
+        unsupported_url: Embeds for this URL are not supported
     user_subscription_tag:
       submit: Submit
       submitting: Submitting...

spec/liquid_tags/loom_tag_spec.rb

@@ -1,6 +1,6 @@
 require "rails_helper"
 
-RSpec.describe LoomTag do
+RSpec.describe LoomTag, type: :liquid_tag do
   subject(:loom_tag) { described_class }
 
   let(:article) { create(:article) }
@@ -33,13 +33,15 @@ RSpec.describe LoomTag do
   describe "rendering" do
     it "returns StandardError for invalid Loom URL", :aggregate_failures do
       invalid_loom_urls.each do |invalid_url|
+        stub_request_head(invalid_url, 404)
         expect do
           generate_embed(invalid_url)
-        end.to raise_error(StandardError, "Embed URL not valid")
+        end.to raise_error(StandardError, "URL provided was not found; please check and try again")
       end
     end
 
     it "returns Loom embed for valid Loom share URL" do
+      stub_request_head(loom_share_url)
       embed = generate_embed(loom_share_url).render
 
       expect(embed).to include("<iframe")
@@ -47,6 +49,7 @@ RSpec.describe LoomTag do
     end
 
     it "returns Loom embed for valid Loom embed URL" do
+      stub_request_head(loom_embed_url)
       embed = generate_embed(loom_embed_url).render
 
       expect(embed).to include("<iframe")
@@ -54,6 +57,7 @@ RSpec.describe LoomTag do
     end
 
     it "returns Loom embed for valid Loom www URL" do
+      stub_request_head(www_loom_url)
       embed = generate_embed(www_loom_url).render
 
       expect(embed).to include("<iframe")
@@ -61,6 +65,8 @@ RSpec.describe LoomTag do
     end
 
     it "returns Loom embed for valid Loom URL with query paramaters" do
+      # Loom URLs with params fail valid-URL check
+      stub_request_head(loom_url_with_query.split("?")[0])
       embed = generate_embed(loom_url_with_query).render
 
       expect(embed).to include("<iframe")

spec/liquid_tags/unified_embed/tag_spec.rb

@@ -5,16 +5,28 @@ RSpec.describe UnifiedEmbed::Tag, type: :liquid_tag do
     link = "https://gist.github.com/jeremyf/662585f5c4d22184a6ae133a71bf891a"
 
     allow(GistTag).to receive(:new).and_call_original
+    stub_request_head(link)
     parsed_tag = Liquid::Template.parse("{% embed #{link} %}")
+
     expect { parsed_tag.render }.not_to raise_error
     expect(GistTag).to have_received(:new)
   end
 
+  it "raises an error when link 404s" do
+    link = "https://takeonrules.com/goes-nowhere"
+
+    expect do
+      stub_request_head(link, 404)
+      Liquid::Template.parse("{% embed #{link} %}")
+    end.to raise_error(StandardError, "URL provided was not found; please check and try again")
+  end
+
   it "raises an error when no link-matching class is found" do
     link = "https://takeonrules.com/about"
 
     expect do
+      stub_request_head(link)
       Liquid::Template.parse("{% embed #{link} %}")
-    end.to raise_error(StandardError, "Embed URL not valid")
+    end.to raise_error(StandardError, "Embeds for this URL are not supported")
   end
 end

spec/rails_helper.rb

@@ -73,6 +73,7 @@ RSpec.configure do |config|
   config.include Devise::Test::ControllerHelpers, type: :view
   config.include Devise::Test::IntegrationHelpers, type: :request
   config.include Devise::Test::IntegrationHelpers, type: :system
+  config.include EmbedsHelpers, type: :liquid_tag
   config.include FactoryBot::Syntax::Methods
   config.include OmniauthHelpers
   config.include RpushHelpers

spec/support/embeds_helpers.rb

@@ -0,0 +1,11 @@
+module EmbedsHelpers
+  def stub_request_head(url, status_code = 200)
+    stub_request(:head, url)
+      .with(
+        headers: {
+          Accept: "*/*",
+          "User-Agent": "Ruby"
+        },
+      ).to_return(status: status_code, body: "", headers: {})
+  end
+end