From 34bfc491b01ffa93f3c36d80ad7844ce956754f4 Mon Sep 17 00:00:00 2001 From: Arit Amana <32520970+msarit@users.noreply.github.com> Date: Thu, 10 Mar 2022 13:18:59 -0500 Subject: [PATCH] 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 Co-authored-by: Dwight Scott --- .../article-form/components/ErrorList.jsx | 4 +- app/liquid_tags/loom_tag.rb | 3 +- app/liquid_tags/unified_embed/tag.rb | 44 ++++++++++++++----- config/locales/liquid_tags/en.yml | 4 +- config/locales/liquid_tags/fr.yml | 4 +- spec/liquid_tags/loom_tag_spec.rb | 10 ++++- spec/liquid_tags/unified_embed/tag_spec.rb | 14 +++++- spec/rails_helper.rb | 1 + spec/support/embeds_helpers.rb | 11 +++++ 9 files changed, 75 insertions(+), 20 deletions(-) create mode 100644 spec/support/embeds_helpers.rb diff --git a/app/javascript/article-form/components/ErrorList.jsx b/app/javascript/article-form/components/ErrorList.jsx index 39c0da955..e330efe8a 100644 --- a/app/javascript/article-form/components/ErrorList.jsx +++ b/app/javascript/article-form/components/ErrorList.jsx @@ -12,9 +12,7 @@ export const ErrorList = ({ errors }) => { {Object.keys(errors).map((key) => { return (
  • - {key} - {`: `} - {errors[key]} + {key === 'base' ? errors[key] : `${key}: ${errors[key]}`}
    • ); })} diff --git a/app/liquid_tags/loom_tag.rb b/app/liquid_tags/loom_tag.rb index 9560149d3..6f87979a5 100644 --- a/app/liquid_tags/loom_tag.rb +++ b/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] diff --git a/app/liquid_tags/unified_embed/tag.rb b/app/liquid_tags/unified_embed/tag.rb index 3f98620be..6d2a21a64 100644 --- a/app/liquid_tags/unified_embed/tag.rb +++ b/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 diff --git a/config/locales/liquid_tags/en.yml b/config/locales/liquid_tags/en.yml index 101d7955d..84d61451b 100644 --- a/config/locales/liquid_tags/en.yml +++ b/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... diff --git a/config/locales/liquid_tags/fr.yml b/config/locales/liquid_tags/fr.yml index c434e941f..c32e5ba05 100644 --- a/config/locales/liquid_tags/fr.yml +++ b/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... diff --git a/spec/liquid_tags/loom_tag_spec.rb b/spec/liquid_tags/loom_tag_spec.rb index 96839862c..86f2c9bb2 100644 --- a/spec/liquid_tags/loom_tag_spec.rb +++ b/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("