From 40639ea60279e7b9e94aeaf2944d0b2190350f9a Mon Sep 17 00:00:00 2001 From: Ryan Palo Date: Tue, 22 Feb 2022 07:22:05 -0800 Subject: [PATCH] Fix RSS feed validity issue (#16418) (#16517) --- app/controllers/articles_controller.rb | 16 +++-- app/sanitizers/feed_markdown_scrubber.rb | 13 ++++ app/views/articles/feed.rss.builder | 41 +++++++++---- spec/requests/articles/articles_spec.rb | 14 ++--- .../sanitizers/feed_markdown_scrubber_spec.rb | 60 +++++++++++++++++++ 5 files changed, 121 insertions(+), 23 deletions(-) create mode 100644 app/sanitizers/feed_markdown_scrubber.rb create mode 100644 spec/sanitizers/feed_markdown_scrubber_spec.rb diff --git a/app/controllers/articles_controller.rb b/app/controllers/articles_controller.rb index ca8229697..942412955 100644 --- a/app/controllers/articles_controller.rb +++ b/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 diff --git a/app/sanitizers/feed_markdown_scrubber.rb b/app/sanitizers/feed_markdown_scrubber.rb new file mode 100644 index 000000000..e006564e8 --- /dev/null +++ b/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 diff --git a/app/views/articles/feed.rss.builder b/app/views/articles/feed.rss.builder index 9add11e14..f545287a9 100644 --- a/app/views/articles/feed.rss.builder +++ b/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 diff --git a/spec/requests/articles/articles_spec.rb b/spec/requests/articles/articles_spec.rb index 473b349dc..49b3ab3fc 100644 --- a/spec/requests/articles/articles_spec.rb +++ b/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("") - expect(response.body).to include("#{user.profile_image_90}") - expect(response.body).to include("#{user.name} profile image") + expect(response.body).to include("#{app_url(user.profile_image_90)}") + expect(response.body).to include("#{community_name}: #{user.name}") expect(response.body).to include("#{URL.user(user)}") expect(response.body).to include("") 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("") - expect(response.body).to include("#{organization.profile_image_90}") - expect(response.body).to include("#{organization.name} profile image") + expect(response.body).to include("#{app_url(organization.profile_image_90)}") + expect(response.body).to include("#{community_name}: #{organization.name}") expect(response.body).to include("#{URL.user(organization)}") expect(response.body).to include("") 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("#{URL.url}") + expect(response.body).to include("#{tag_url(tag)}") end it "does not contain image tag" do diff --git a/spec/sanitizers/feed_markdown_scrubber_spec.rb b/spec/sanitizers/feed_markdown_scrubber_spec.rb new file mode 100644 index 000000000..7f3f34da3 --- /dev/null +++ b/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 +

This is some darn good HTML right here. + and definitely not in need of any:

+ +

Even spans make it through!

+ 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 = "

Hello world!

Boo forms!
" + good_html = "

Hello world!

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 + + An actual fish + + 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 = 'Hi, I am Jerry.' + good_html = "Hi, I am Jerry." + 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 + Link 1 + Link 2 + HTML + clean = sanitize(good_html, scrubber: described_class.new) + expect(clean).to eq(good_html) + end + + it "scrubs relative links" do + bad_html = '

I link to somewhere on this page!

' + good_html = "

I link to somewhere on this page!

" + clean = sanitize(bad_html, scrubber: described_class.new) + expect(clean).to eq(good_html) + end +end