From f94682e2c4c3db03e0d69eff765077d919178491 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Fri, 3 Jun 2022 09:02:57 +1000 Subject: [PATCH] FIX: Do not use SVGs for twitter:image metadata (#16973) Twitter does not allow SVGs to be used for twitter:image metadata (see https://developer.twitter.com/en/docs/twitter-for-websites/cards/overview/markup) so we should fall back to the site logo if the image option provided to `crawlable_meta_data` or SiteSetting.site_twitter_summary_large_image_url is an SVG, and do not add the meta tag for twitter:image at all if the site logo is an SVG. --- .../components/site-settings/upload.hbs | 1 + .../site-settings/uploaded-image-list.hbs | 1 + app/helpers/application_helper.rb | 33 +++++++++++----- config/locales/server.en.yml | 3 +- lib/site_settings/validations.rb | 6 +++ spec/helpers/application_helper_spec.rb | 39 +++++++++++++++++++ spec/lib/site_settings/validations_spec.rb | 12 ++++++ 7 files changed, 85 insertions(+), 10 deletions(-) diff --git a/app/assets/javascripts/admin/addon/templates/components/site-settings/upload.hbs b/app/assets/javascripts/admin/addon/templates/components/site-settings/upload.hbs index 78a64c25dd2..7d2c4b4e03c 100644 --- a/app/assets/javascripts/admin/addon/templates/components/site-settings/upload.hbs +++ b/app/assets/javascripts/admin/addon/templates/components/site-settings/upload.hbs @@ -6,3 +6,4 @@ id=(concat "site-setting-image-uploader-" setting.setting) }}
{{html-safe setting.description}}
+{{setting-validation-message message=validationMessage}} diff --git a/app/assets/javascripts/admin/addon/templates/components/site-settings/uploaded-image-list.hbs b/app/assets/javascripts/admin/addon/templates/components/site-settings/uploaded-image-list.hbs index bf8148800c3..df019bae62f 100644 --- a/app/assets/javascripts/admin/addon/templates/components/site-settings/uploaded-image-list.hbs +++ b/app/assets/javascripts/admin/addon/templates/components/site-settings/uploaded-image-list.hbs @@ -1,2 +1,3 @@ {{d-button label="admin.site_settings.uploaded_image_list.label" action=(action "showUploadModal") actionParam=(hash value=value setting=setting)}}
{{html-safe setting.description}}
+{{setting-validation-message message=validationMessage}} diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 338b7069055..5773efd851f 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -289,15 +289,7 @@ module ApplicationHelper result << tag(:meta, property: 'og:site_name', content: SiteSetting.title) result << tag(:meta, property: 'og:type', content: 'website') - if opts[:twitter_summary_large_image].present? - result << tag(:meta, name: 'twitter:card', content: "summary_large_image") - result << tag(:meta, name: "twitter:image", content: opts[:twitter_summary_large_image]) - elsif opts[:image].present? - result << tag(:meta, name: 'twitter:card', content: "summary") - result << tag(:meta, name: "twitter:image", content: opts[:image]) - else - result << tag(:meta, name: 'twitter:card', content: "summary") - end + result = generate_twitter_card_metadata(opts, result) result << tag(:meta, property: "og:image", content: opts[:image]) if opts[:image].present? [:url, :title, :description].each do |property| @@ -326,6 +318,29 @@ module ApplicationHelper result.join("\n") end + def generate_twitter_card_metadata(opts, result) + img_url = opts[:twitter_summary_large_image].present? ? \ + opts[:twitter_summary_large_image] : + opts[:image] + + # Twitter does not allow SVGs, see https://developer.twitter.com/en/docs/twitter-for-websites/cards/overview/markup + if img_url.ends_with?(".svg") + img_url = SiteSetting.site_logo_url.ends_with?(".svg") ? nil : SiteSetting.site_logo_url + end + + if opts[:twitter_summary_large_image].present? && img_url.present? + result << tag(:meta, name: 'twitter:card', content: "summary_large_image") + result << tag(:meta, name: "twitter:image", content: img_url) + elsif opts[:image].present? && img_url.present? + result << tag(:meta, name: 'twitter:card', content: "summary") + result << tag(:meta, name: "twitter:image", content: img_url) + else + result << tag(:meta, name: 'twitter:card', content: "summary") + end + + result + end + def render_sitelinks_search_tag json = { '@context' => 'http://schema.org', diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 83698860dd2..24ea1a07c38 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -220,6 +220,7 @@ en: slow_down_crawler_user_agent_must_be_at_least_3_characters: "User agents must be at least 3 characters long to avoid incorrectly rate limiting human users." slow_down_crawler_user_agent_cannot_be_popular_browsers: "You cannot add any of the following values to the setting: %{values}." strip_image_metadata_cannot_be_disabled_if_composer_media_optimization_image_enabled: "You cannot disable strip image metadata if 'composer media optimization image enabled' is enabled. Disable 'composer media optimization image enabled' before disabling strip image metadata." + twitter_summary_large_image_no_svg: "Twitter summary images used for twitter:image metadata cannot be an .svg image." conflicting_google_user_id: 'The Google Account ID for this account has changed; staff intervention is required for security reasons. Please contact staff and point them to
https://meta.discourse.org/t/76575' onebox: invalid_address: "Sorry, we were unable to generate a preview for this web page, because the server '%{hostname}' could not be found. Instead of a preview, only a link will appear in your post. :cry:" @@ -1559,7 +1560,7 @@ en: favicon: "A favicon for your site, see https://en.wikipedia.org/wiki/Favicon. To work correctly over a CDN it must be a png. Will be resized to 32x32. If left blank, large_icon will be used." apple_touch_icon: "Icon used for Apple touch devices. Will be automatically resized to 180x180. If left blank, large_icon will be used." opengraph_image: "Default opengraph image, used when the page has no other suitable image. If left blank, large_icon will be used" - twitter_summary_large_image: "Twitter card 'summary large image' (should be at least 280 in width, and at least 150 in height). If left blank, regular card metadata is generated using the opengraph_image." + twitter_summary_large_image: "Twitter card 'summary large image' (should be at least 280 in width, and at least 150 in height, cannot be .svg). If left blank, regular card metadata is generated using the opengraph_image, as long as that is not also a .svg" notification_email: "The from: email address used when sending all essential system emails. The domain specified here must have SPF, DKIM and reverse PTR records set correctly for email to arrive." email_custom_headers: "A pipe-delimited list of custom email headers" diff --git a/lib/site_settings/validations.rb b/lib/site_settings/validations.rb index cd2490628b3..bc5a3b80c5d 100644 --- a/lib/site_settings/validations.rb +++ b/lib/site_settings/validations.rb @@ -245,6 +245,12 @@ module SiteSettings::Validations validate_error :strip_image_metadata_cannot_be_disabled_if_composer_media_optimization_image_enabled end + def validate_twitter_summary_large_image(new_val) + return if new_val.blank? + return if !Upload.exists?(id: new_val, extension: "svg") + validate_error :twitter_summary_large_image_no_svg + end + private def validate_bucket_setting(setting_name, upload_bucket, backup_bucket) diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb index fa31d038822..e7af0c68118 100644 --- a/spec/helpers/application_helper_spec.rb +++ b/spec/helpers/application_helper_spec.rb @@ -467,6 +467,45 @@ describe ApplicationHelper do expect(helper.crawlable_meta_data).to include(Upload.find(SiteIconManager::SKETCH_LOGO_ID).url) end + + it "does not allow SVG images for twitter:image, falls back to site logo or nothing if site logo is SVG too" do + SiteSetting.logo = Fabricate(:upload, url: '/images/d-logo-sketch.png') + SiteSetting.opengraph_image = Fabricate(:upload, + url: '/images/og-image.png' + ) + + expect(helper.crawlable_meta_data).to include(<<~HTML) + + HTML + + SiteSetting.opengraph_image = Fabricate(:upload, + url: '/images/og-image.svg' + ) + + expect(helper.crawlable_meta_data).to include(<<~HTML) + + HTML + + SiteSetting.twitter_summary_large_image = Fabricate(:upload, + url: '/images/twitter.png' + ) + + expect(helper.crawlable_meta_data).to include(<<~HTML) + + HTML + + SiteSetting.twitter_summary_large_image = Fabricate(:upload, + url: '/images/twitter.svg' + ) + + expect(helper.crawlable_meta_data).to include(<<~HTML) + + HTML + + SiteSetting.logo = Fabricate(:upload, url: '/images/d-logo-sketch.svg') + + expect(helper.crawlable_meta_data).not_to include("twitter:image") + end end end diff --git a/spec/lib/site_settings/validations_spec.rb b/spec/lib/site_settings/validations_spec.rb index a0a86913a88..27550967fb6 100644 --- a/spec/lib/site_settings/validations_spec.rb +++ b/spec/lib/site_settings/validations_spec.rb @@ -402,4 +402,16 @@ describe SiteSettings::Validations do end end end + + describe "#twitter_summary_large_image" do + it "does not allow SVG image files" do + upload = Fabricate(:upload, url: '/images/logo-dark.svg', extension: "svg") + expect { subject.validate_twitter_summary_large_image(upload.id) }.to raise_error( + Discourse::InvalidParameters, I18n.t("errors.site_settings.twitter_summary_large_image_no_svg") + ) + upload.update!(url: '/images/logo-dark.png', extension: 'png') + expect { subject.validate_twitter_summary_large_image(upload.id) }.not_to raise_error + expect { subject.validate_twitter_summary_large_image(nil) }.not_to raise_error + end + end end