Implement Unified Embed for Vimeo URLs (#15699)

* complete implementation; add/fix specs; fix some straggling YoutubeTag bugs

* fix small regex typo

* make YT tag more robust; DRY strip_tags method

* Refactor level 1

* Refactor level 2

* Refactor level 3

* array to constant

* fix failing vimeo specs - tweak vimeo regexp

* PR feedback refactor

app/liquid_tags/liquid_tag_base.rb

@@ -15,6 +15,10 @@ class LiquidTagBase < Liquid::Tag
     )
   end
 
+  def strip_tags(string)
+    ActionController::Base.helpers.strip_tags(string).strip
+  end
+
   private
 
   def validate_contexts

app/liquid_tags/unified_embed_tag.rb

@@ -34,7 +34,7 @@ class UnifiedEmbedTag < LiquidTagBase
   end
 
   def render(_context)
-    link, _options = ActionController::Base.helpers.strip_tags(@markup).split
+    link, _options = strip_tags(@markup)
     %(<a href="#{link}">#{link}</a>)
   end
 end

app/liquid_tags/vimeo_tag.rb

@@ -1,9 +1,13 @@
 class VimeoTag < LiquidTagBase
   PARTIAL = "liquids/vimeo".freeze
+  # rubocop:disable Layout/LineLength
+  REGISTRY_REGEXP = %r{(?:https?://)?(?:player\.|www\.)?vimeo\.com/(?:video/|embed/|watch)?(?:ondemand/\w+/)?(?<video_id>\d*)}
+  # rubocop:enable Layout/LineLength
 
   def initialize(_tag_name, token, _parse_context)
     super
-    @id     = id_for(token)
+    input   = strip_tags(token)
+    @id     = get_id(input)
     @width  = 710
     @height = 399
   end
@@ -21,14 +25,14 @@ class VimeoTag < LiquidTagBase
 
   private
 
-  def id_for(input)
-    # This was the original plan:
-    #   require "uri"
-    #   File.basename URI(input).path
-    # But the markdown turns the link into html. This is simple enough,
-    # works for all the use cases and isn't exploitable.
-    input.to_s.scan(/\d+/).max_by(&:length)
+  def get_id(input)
+    match = input.match(REGISTRY_REGEXP)
+    # binding.pry
+    match ? match[:video_id] : input
   end
 end
 
 Liquid::Template.register_tag("vimeo", VimeoTag)
+
+# NOTE: this does not process Vimeo Showcase IDs; add to documentation
+UnifiedEmbed.register(VimeoTag, regexp: VimeoTag::REGISTRY_REGEXP)

app/liquid_tags/youtube_tag.rb

@@ -1,6 +1,11 @@
 class YoutubeTag < LiquidTagBase
   PARTIAL = "liquids/youtube".freeze
-  REGISTRY_REGEXP = %r{https?://(www\.)?youtube\.(com|be)/(embed|watch)?(\?v=)?(/)?[a-zA-Z0-9_-]{11}((\?t=)?(\d{1,})?)?}
+  # rubocop:disable Layout/LineLength
+  REGISTRY_REGEXP = %r{https?://(?:www\.)?(?:youtube\.com|youtu\.be)/(?:embed/|watch\?v=)?(?<video_id>[a-zA-Z0-9_-]{11})(?:\?|&)?(?:t=|start=)?(?<time_parameter>(?:\d{1,}h?)?(?:\d{1,2}m)?(?:\d{1,2}s)?{5,11})?}
+  VALID_ID_REGEXP = /\A(?<video_id>[a-zA-Z0-9_-]{11})(?:\?|&)?(?:t=|start=)?(?<time_parameter>(?:\d{1,}h?)?(?:\d{1,2}m)?(?:\d{1,2}s)?{5,11})?\Z/
+  # rubocop:enable Layout/LineLength
+  REGEXP_OPTIONS = [REGISTRY_REGEXP, VALID_ID_REGEXP].freeze
+
   MARKER_TO_SECONDS_MAP = {
     "h" => 60 * 60,
     "m" => 60,
@@ -10,11 +15,9 @@ class YoutubeTag < LiquidTagBase
   def initialize(_tag_name, id, _parse_context)
     super
 
-    # for if id is an unstripped URL; doesn't appear to affect bare youtube ids
-    input = ActionController::Base.helpers.strip_tags(id).strip
-
-    @id = parse_id_or_url(input)
-    @width = 710
+    input   = CGI.unescape_html(strip_tags(id))
+    @id     = parse_id_or_url(input)
+    @width  = 710
     @height = 399
   end
 
@@ -32,42 +35,35 @@ class YoutubeTag < LiquidTagBase
   private
 
   def parse_id_or_url(input)
-    if (input =~ REGISTRY_REGEXP)&.zero?
-      extract_youtube_id(input)
-    else
-      input_no_space = input.delete(" ")
-      raise StandardError, "Invalid YouTube ID" unless valid_id?(input_no_space)
-      return translate_start_time(input_no_space) if input_no_space.include?("?t=")
+    match = pattern_match_for(input)
+    raise StandardError, "Invalid YouTube ID" unless match
 
-      input_no_space
-    end
+    video_id       = match[:video_id]
+    time_parameter = match[:time_parameter]
+
+    return video_id if time_parameter.blank?
+
+    translate_start_time(video_id, time_parameter)
   end
 
-  def extract_youtube_id(url)
-    url = url.gsub(/(>|<)/, "").split(%r{(vi/|v=|/v/|youtu\.be/|/embed/)})
-    raise StandardError, "Invalid YouTube URL" if url[2].nil?
-
-    id = url[2].split(/[^a-zA-Z0-9_-]/i) # tweak this to allow for time, fix youtube_tag_spec
-    id[0]
+  def pattern_match_for(input)
+    REGEXP_OPTIONS
+      .filter_map { |regex| input.match(regex) }
+      .first
   end
 
-  def valid_id?(id)
-    id.match?(/\A[a-zA-Z0-9_-]{11}((\?t=)?(\d{1,}h?)?(\d{1,2}m)?(\d{1,2}s)?){5,11}?\Z/)
-  end
+  def translate_start_time(video_id, time_parameter)
+    return "#{video_id}?start=#{time_parameter}" if time_parameter.match?(/\A\d+\Z/)
 
-  def translate_start_time(id)
-    time = id.split("?t=")[-1]
-    return "#{id.split('?t=')[0]}?start=#{time}" if time.match?(/\A\d+\Z/)
-
-    time_elements = time.split(/[a-z]/)
-    time_markers = time.split(/\d+/)[1..]
+    time_elements = time_parameter.split(/[a-z]/)
+    time_markers = time_parameter.split(/\d+/)[1..]
 
     seconds = 0
     time_markers.each_with_index do |m, i|
       seconds += MARKER_TO_SECONDS_MAP.fetch(m, 0) * time_elements[i].to_i
     end
 
-    "#{id.split('?t=')[0]}?start=#{seconds}"
+    "#{video_id}?start=#{seconds}"
   end
 end
 

spec/liquid_tags/unified_embed_spec.rb

@@ -4,6 +4,18 @@ RSpec.describe UnifiedEmbed do
   subject(:unified_embed) { described_class }
 
   describe ".find_liquid_tag_for" do
+    valid_youtube_url_formats = [
+      "https://www.youtube.com/embed/dQw4w9WgXcQ",
+      "https://www.youtube.com/watch?v=rc5AyncB_Xw&t=18s",
+      "https://youtu.be/rc5AyncB_Xw",
+    ]
+
+    valid_vimeo_url_formats = [
+      "https://player.vimeo.com/video/652446985?h=a68f6ed1f5",
+      "https://vimeo.com/ondemand/withchude/647355334",
+      "https://vimeo.com/636725488",
+    ]
+
     it "returns GistTag for a gist url" do
       expect(described_class.find_liquid_tag_for(link: "https://gist.github.com/jeremyf/662585f5c4d22184a6ae133a71bf891a"))
         .to eq(GistTag)
@@ -49,9 +61,18 @@ RSpec.describe UnifiedEmbed do
         .to eq(WikipediaTag)
     end
 
-    it "returns YoutubeTag for a youtube url" do
-      expect(described_class.find_liquid_tag_for(link: "https://www.youtube.com/embed/dQw4w9WgXcQ"))
-        .to eq(YoutubeTag)
+    valid_youtube_url_formats.each do |url|
+      it "returns YoutubeTag for a valid youtube url format" do
+        expect(described_class.find_liquid_tag_for(link: url))
+          .to eq(YoutubeTag)
+      end
+    end
+
+    valid_vimeo_url_formats.each do |url|
+      it "returns VimeoTag for a valid vimeo url format" do
+        expect(described_class.find_liquid_tag_for(link: url))
+          .to eq(VimeoTag)
+      end
     end
   end
 end

spec/liquid_tags/vimeo_tag_spec.rb

@@ -31,14 +31,7 @@ RSpec.describe VimeoTag, type: :liquid_tag do
     assert_parses id, "ps://player.vimeo.com/video/#{id}"
   end
 
-  # NOTE: This is kinda dumb. It seems like the right answer is that
-  # either it should run liquid before markdown, or markdown shouldn't
-  # mess with the liquid tags (there is a fn to escape them, but it doesn't
-  # seem to escape the url here)
-  # https://github.com/thepracticaldev/dev.to/blob/master/app/labor/markdown_parser.rb#L73-L92
-  # My test suite isn't entirely passing, and I've spent longer on this than I
-  # wanted to, io Instead of looking into those, I'm going to just make this work  ¯\_(ツ)_/¯
-  it "accepts urls that were over-eagerly turned into links by markdown" do
-    assert_parses id, "<a href=\"https://vimeo.com/#{id}\">https://vimeo.com/192819855</a> "
+  it "accepts urls that were turned into links by markdown" do
+    assert_parses id, "<a href=\"https://vimeo.com/#{id}\">https://vimeo.com/#{id}</a> "
   end
 end

spec/liquid_tags/youtube_tag_spec.rb

@@ -2,8 +2,8 @@ require "rails_helper"
 
 RSpec.describe YoutubeTag, type: :liquid_tag do
   describe "#id" do
-    let(:valid_id_no_time) { "dQw4w9WgXcQ" }
-    let(:valid_id_with_time) { "QASbw8_0meM?t=8h12m26s" }
+    let(:valid_id_no_time) { "fhH5xX_yW6U" }
+    let(:valid_id_with_time) { "fhH5xX_yW6U?t=0h5m0s" }
     let(:invalid_id) { Faker::Lorem.characters(number: rand(12..100)) }
 
     def generate_new_liquid(id)
@@ -16,28 +16,28 @@ RSpec.describe YoutubeTag, type: :liquid_tag do
       liquid = generate_new_liquid(valid_id_no_time).render
 
       expect(liquid).to include('<iframe')
-      expect(liquid).to include('src="https://www.youtube.com/embed/dQw4w9WgXcQ"')
+      expect(liquid).to include('src="https://www.youtube.com/embed/fhH5xX_yW6U"')
     end
 
     it "accepts valid YouTube ID with starting times" do
       liquid = generate_new_liquid(valid_id_with_time).render
 
       expect(liquid).to include('<iframe')
-      expect(liquid).to include('src="https://www.youtube.com/embed/QASbw8_0meM?start=29546"')
+      expect(liquid).to include('src="https://www.youtube.com/embed/fhH5xX_yW6U?start=300"')
     end
 
     it "accepts YouTube ID with no start time and an empty space" do
       liquid = generate_new_liquid("#{valid_id_no_time} ").render
 
       expect(liquid).to include('<iframe')
-      expect(liquid).to include('src="https://www.youtube.com/embed/dQw4w9WgXcQ"')
+      expect(liquid).to include('src="https://www.youtube.com/embed/fhH5xX_yW6U"')
     end
 
     it "accepts YouTube ID with start times and one empty space" do
       liquid = generate_new_liquid("#{valid_id_with_time} ").render
 
       expect(liquid).to include('<iframe')
-      expect(liquid).to include('src="https://www.youtube.com/embed/QASbw8_0meM?start=29546"')
+      expect(liquid).to include('src="https://www.youtube.com/embed/fhH5xX_yW6U?start=300"')
     end
     # rubocop:enable Style/StringLiterals