Fix RSS feed validity issue (#16418) (#16517)

app/controllers/articles_controller.rb

@@ -34,11 +34,12 @@ class ArticlesController < ApplicationController
     skip_authorization
 
     @articles = Article.feed.order(published_at: :desc).page(params[:page].to_i).per(12)
+    @latest = request.path == latest_feed_path
     @articles = if params[:username]
                   handle_user_or_organization_feed
                 elsif params[:tag]
                   handle_tag_feed
-                elsif request.path == latest_feed_path
+                elsif @latest
                   @articles
                     .where("score > ?", Articles::Feeds::Latest::MINIMUM_SCORE)
                     .includes(:user)
@@ -53,12 +54,14 @@ class ArticlesController < ApplicationController
     set_surrogate_key_header "feed"
     set_cache_control_headers(10.minutes.to_i, stale_while_revalidate: 30, stale_if_error: 1.day.to_i)
 
-    render layout: false, locals: {
+    render layout: false, content_type: "application/xml", locals: {
       articles: @articles,
       user: @user,
       tag: @tag,
+      latest: @latest,
       allowed_tags: MarkdownProcessor::AllowedTags::FEED,
-      allowed_attributes: MarkdownProcessor::AllowedAttributes::FEED
+      allowed_attributes: MarkdownProcessor::AllowedAttributes::FEED,
+      scrubber: FeedMarkdownScrubber.new
     }
   end
 
@@ -263,10 +266,11 @@ class ArticlesController < ApplicationController
   end
 
   def handle_tag_feed
-    @tag = Tag.aliased_name(params[:tag])
-    return unless @tag
+    tag_name = Tag.aliased_name(params[:tag])
+    return unless tag_name
 
-    @articles = @articles.cached_tagged_with(@tag)
+    @tag = Tag.find_by(name: tag_name)
+    @articles = @articles.cached_tagged_with(tag_name)
   end
 
   def set_article

app/sanitizers/feed_markdown_scrubber.rb

@@ -0,0 +1,13 @@
+class FeedMarkdownScrubber < Rails::Html::PermitScrubber
+  def initialize
+    super
+    self.tags = MarkdownProcessor::AllowedTags::FEED
+    self.attributes = MarkdownProcessor::AllowedAttributes::FEED
+  end
+
+  def allowed_node?(node)
+    return true if tags.include?(node.name) && node.name != "a"
+
+    node.name == "a" && !node["href"].start_with?("#")
+  end
+end

app/views/articles/feed.rss.builder

@@ -3,28 +3,49 @@
 # rubocop:disable Metrics/BlockLength
 
 xml.instruct! :xml, version: "1.0"
-xml.rss version: "2.0" do
+xml.rss(:version => "2.0",
+        "xmlns:atom" => "http://www.w3.org/2005/Atom",
+        "xmlns:dc" => "http://purl.org/dc/elements/1.1/") do
   xml.channel do
-    xml.title user ? user.name : community_name
-    xml.author user ? user.name : community_name
-    xml.description user ? user.tag_line : Settings::Community.community_description
-    xml.link user ? app_url(user.path) : app_url
-    xml.language "en" # TODO: [yheuhtozr] support localized feeds (see #15136)
     if user
+      xml.title "#{community_name}: #{user.name}"
+      xml.description "The latest articles on #{community_name} by #{user.name} (@#{user.username})."
+      xml.link app_url(user.path)
       xml.image do
-        xml.url user.profile_image_90
-        xml.title t("xml.user.image", user: user.name)
+        xml.url app_url(user.profile_image_90)
+        xml.title "#{community_name}: #{user.name}"
         xml.link app_url(user.path)
       end
+    elsif tag
+      xml.title "#{community_name}: #{tag.name}"
+      xml.description "The latest articles tagged '#{tag.name}' on #{community_name}."
+      xml.link tag_url(tag)
+      # NOTE: there exists a `tag.profile_image`, but unsure if it's in use.
+      # xml.image do
+      #   xml.url app_url(tag.profile_image)
+      #   xml.title "#{community_name}: #{tag.name}"
+      #   xml.link tag_url(tag)
+      # end
+    elsif latest
+      xml.title "#{community_name}: Latest"
+      xml.description "The most recent articles on #{community_name}."
+      xml.link "#{app_url}/latest"
+    else
+      xml.title community_name
+      xml.description "The most recent home feed on #{community_name}."
+      xml.link app_url
     end
+    xml.tag! "atom:link", rel: "self", type: "application/rss+xml", href: request.original_url
+    xml.language "en" # TODO: [yheuhtozr] support localized feeds (see #15136)
     articles.each do |article|
       xml.item do
         xml.title article.title
-        xml.author(user.instance_of?(User) ? user.name : article.user.name)
+        xml.tag!("dc:creator") { user.instance_of?(User) ? user.name : article.user.name }
         xml.pubDate article.published_at.to_s(:rfc822) if article.published_at
         xml.link app_url(article.path)
         xml.guid app_url(article.path)
-        xml.description sanitize(article.plain_html, tags: allowed_tags, attributes: allowed_attributes)
+        xml.description sanitize(article.plain_html,
+                                 tags: allowed_tags, attributes: allowed_attributes, scrubber: scrubber)
         article.tag_list.each do |tag_name|
           xml.category tag_name
         end

spec/requests/articles/articles_spec.rb

@@ -11,7 +11,7 @@ RSpec.describe "Articles", type: :request do
       get feed_path
 
       expect(response).to have_http_status(:ok)
-      expect(response.media_type).to eq("application/rss+xml")
+      expect(response.media_type).to eq("application/xml")
     end
 
     it "contains the full app URL" do
@@ -101,8 +101,8 @@ RSpec.describe "Articles", type: :request do
 
       it "contains a user composite profile image tag", :aggregate_failures do
         expect(response.body).to include("<image>")
-        expect(response.body).to include("<url>#{user.profile_image_90}</url>")
-        expect(response.body).to include("<title>#{user.name} profile image</title>")
+        expect(response.body).to include("<url>#{app_url(user.profile_image_90)}</url>")
+        expect(response.body).to include("<title>#{community_name}: #{user.name}</title>")
         expect(response.body).to include("<link>#{URL.user(user)}</link>")
         expect(response.body).to include("</image>")
       end
@@ -127,8 +127,8 @@ RSpec.describe "Articles", type: :request do
 
       it "contains an organization composite profile image tag", :aggregate_failures do
         expect(response.body).to include("<image>")
-        expect(response.body).to include("<url>#{organization.profile_image_90}</url>")
-        expect(response.body).to include("<title>#{organization.name} profile image</title>")
+        expect(response.body).to include("<url>#{app_url(organization.profile_image_90)}</url>")
+        expect(response.body).to include("<title>#{community_name}: #{organization.name}</title>")
         expect(response.body).to include("<link>#{URL.user(organization)}</link>")
         expect(response.body).to include("</image>")
       end
@@ -230,10 +230,10 @@ RSpec.describe "Articles", type: :request do
         expect(rss_feed.entries.first.categories).to include(tag.name)
       end
 
-      it "contains the full app URL" do
+      it "contains the full tag URL" do
         get tag_feed_path(tag.name)
 
-        expect(response.body).to include("<link>#{URL.url}</link>")
+        expect(response.body).to include("<link>#{tag_url(tag)}</link>")
       end
 
       it "does not contain image tag" do

spec/sanitizers/feed_markdown_scrubber_spec.rb

@@ -0,0 +1,60 @@
+require "rails_helper"
+
+RSpec.describe FeedMarkdownScrubber, type: :permit_scrubber do
+  include ActionView::Helpers::SanitizeHelper
+
+  it "allows the tags specified by MarkdownProcessor" do
+    good_html = <<~HTML
+      <p>This is some <em>darn good</em> HTML right here.
+      and <b>definitely</b> not in need of any:</p>
+      <ul>
+        <li>scrubbing</li>
+        <li>cleaning</li>
+        <li><a href="www.google.com">Tricky business.</a></li>
+      </ul>
+      <p>Even <span class="awesome">spans</span> make it through!</p>
+    HTML
+    clean = sanitize(good_html, scrubber: described_class.new)
+    expect(clean).to eq(good_html)
+  end
+
+  it "scrubs out tags not allowed by MarkdownProcessor" do
+    bad_html = "<p>Hello world!</p><form>Boo forms!</form>"
+    good_html = "<p>Hello world!</p>Boo forms!"
+    clean = sanitize(bad_html, scrubber: described_class.new)
+    expect(clean).to eq(good_html)
+  end
+
+  it "allows attributes allowed by MarkdownProcessor" do
+    good_html = <<~HTML
+      <a id="linky-image" class="profile" href="www.google.com">
+        <img src="www.example.com/fish.jpg" alt="An actual fish">
+      </a>
+    HTML
+    clean = sanitize(good_html, scrubber: described_class.new)
+    expect(clean).to eq(good_html)
+  end
+
+  it "scrubs out attributes not allowed by MarkdownProcessor" do
+    bad_html = '<span name="jerry">Hi, I am Jerry.</span>'
+    good_html = "<span>Hi, I am Jerry.</span>"
+    clean = sanitize(bad_html, scrubber: described_class.new)
+    expect(clean).to eq(good_html)
+  end
+
+  it "allows links in 'a' tags as long as they're not strictly relative links" do
+    good_html = <<~HTML
+      <a href="https://www.google.com">Link 1</a>
+      <a href="https://www.google.com#title">Link 2</a>
+    HTML
+    clean = sanitize(good_html, scrubber: described_class.new)
+    expect(clean).to eq(good_html)
+  end
+
+  it "scrubs relative links" do
+    bad_html = '<a href="#relative"><h1>I link to somewhere on this page!</h1></a>'
+    good_html = "<h1>I link to somewhere on this page!</h1>"
+    clean = sanitize(bad_html, scrubber: described_class.new)
+    expect(clean).to eq(good_html)
+  end
+end