From 1122579216a0ba6d081ff9ad14a1b501636f9601 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Fri, 16 Feb 2018 04:40:53 +0100 Subject: Do not hide NSFW media/CW'd text in OpenGraph tags (#6479) Reasoning: HTML title tag affects everyone. But OpenGraph only affects when somebody is deliberately sharing the content, usually in an environment where such content is expected. Hiding the content in OpenGraph tags results in deceitful previews which inhibit the shareability of the post. Example: Somebody writes a clever post about politics but kindly puts a "uspol" content warning on it. Mastodon users are thankful, but sharing this post on another platform results in non-Mastodon users believing the entire contents of the post is "uspol" and not clicking through/reading and re-sharing. --- app/views/stream_entries/_og_description.html.haml | 5 +---- app/views/stream_entries/_og_image.html.haml | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) (limited to 'app/views') diff --git a/app/views/stream_entries/_og_description.html.haml b/app/views/stream_entries/_og_description.html.haml index d2fa99e63..9c24e0a61 100644 --- a/app/views/stream_entries/_og_description.html.haml +++ b/app/views/stream_entries/_og_description.html.haml @@ -1,4 +1 @@ -- if activity.is_a?(Status) && activity.spoiler_text? - = opengraph 'og:description', activity.spoiler_text -- else - = opengraph 'og:description', activity.content += opengraph 'og:description', [activity.spoiler_text, activity.text].reject(&:blank?).join("\n\n") diff --git a/app/views/stream_entries/_og_image.html.haml b/app/views/stream_entries/_og_image.html.haml index 1056c1744..bd14ef701 100644 --- a/app/views/stream_entries/_og_image.html.haml +++ b/app/views/stream_entries/_og_image.html.haml @@ -1,4 +1,4 @@ -- if activity.is_a?(Status) && activity.non_sensitive_with_media? +- if activity.is_a?(Status) && activity.media_attachments.any? - activity.media_attachments.each do |media| - if media.image? = opengraph 'og:image', full_asset_url(media.file.url(:original)) -- cgit From 9dbae6e8a120fc18fdc007503375b53f6b061b8f Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Fri, 16 Feb 2018 07:22:20 +0100 Subject: Save video metadata and improve video OpenGraph tags (#6481) * Save metadata from video attachments, put correct dimensions into OG tags * Add twitter:player for videos * Fix code style and test --- Gemfile | 1 + Gemfile.lock | 3 + app/controllers/media_controller.rb | 18 +++-- app/javascript/styles/mastodon/basics.scss | 4 + app/javascript/styles/mastodon/stream_entries.scss | 8 +- app/models/media_attachment.rb | 40 +++++++--- app/views/media/player.html.haml | 2 + app/views/stream_entries/_og_image.html.haml | 27 +++++-- config/brakeman.ignore | 86 ++++++++++++++++++---- config/routes.rb | 5 +- spec/models/media_attachment_spec.rb | 1 - 11 files changed, 151 insertions(+), 44 deletions(-) create mode 100644 app/views/media/player.html.haml (limited to 'app/views') diff --git a/Gemfile b/Gemfile index e219f5159..ef744064b 100644 --- a/Gemfile +++ b/Gemfile @@ -20,6 +20,7 @@ gem 'fog-local', '~> 0.4', require: false gem 'fog-openstack', '~> 0.1', require: false gem 'paperclip', '~> 5.1' gem 'paperclip-av-transcoder', '~> 0.6' +gem 'streamio-ffmpeg', '~> 3.0' gem 'active_model_serializers', '~> 0.10' gem 'addressable', '~> 2.5' diff --git a/Gemfile.lock b/Gemfile.lock index 2131afa65..8e4edb8e1 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -547,6 +547,8 @@ GEM net-scp (>= 1.1.2) net-ssh (>= 2.8.0) statsd-ruby (1.2.1) + streamio-ffmpeg (3.0.2) + multi_json (~> 1.8) strong_migrations (0.1.9) activerecord (>= 3.2.0) temple (0.8.0) @@ -707,6 +709,7 @@ DEPENDENCIES simple_form (~> 3.4) simplecov (~> 0.14) sprockets-rails (~> 3.2) + streamio-ffmpeg (~> 3.0) strong_migrations tty-command tty-prompt diff --git a/app/controllers/media_controller.rb b/app/controllers/media_controller.rb index f652f5ace..88c7232dd 100644 --- a/app/controllers/media_controller.rb +++ b/app/controllers/media_controller.rb @@ -3,20 +3,26 @@ class MediaController < ApplicationController include Authorization - before_action :verify_permitted_status + before_action :set_media_attachment + before_action :verify_permitted_status! def show - redirect_to media_attachment.file.url(:original) + redirect_to @media_attachment.file.url(:original) + end + + def player + @body_classes = 'player' + raise ActiveRecord::RecordNotFound unless @media_attachment.video? || @media_attachment.gifv? end private - def media_attachment - MediaAttachment.attached.find_by!(shortcode: params[:id]) + def set_media_attachment + @media_attachment = MediaAttachment.attached.find_by!(shortcode: params[:id] || params[:medium_id]) end - def verify_permitted_status - authorize media_attachment.status, :show? + def verify_permitted_status! + authorize @media_attachment.status, :show? rescue Mastodon::NotPermittedError # Reraise in order to get a 404 instead of a 403 error code raise ActiveRecord::RecordNotFound diff --git a/app/javascript/styles/mastodon/basics.scss b/app/javascript/styles/mastodon/basics.scss index b5d77ff63..0e3b9ad6b 100644 --- a/app/javascript/styles/mastodon/basics.scss +++ b/app/javascript/styles/mastodon/basics.scss @@ -47,6 +47,10 @@ body { padding-bottom: 0; } + &.player { + text-align: center; + } + &.embed { background: transparent; margin: 0; diff --git a/app/javascript/styles/mastodon/stream_entries.scss b/app/javascript/styles/mastodon/stream_entries.scss index a35bbee79..e11ca29d9 100644 --- a/app/javascript/styles/mastodon/stream_entries.scss +++ b/app/javascript/styles/mastodon/stream_entries.scss @@ -65,6 +65,10 @@ } } + .media-gallery__gifv__label { + bottom: 9px; + } + .status.light { padding: 14px 14px 14px (48px + 14px * 2); position: relative; @@ -258,12 +262,12 @@ .media-spoiler { background: $ui-primary-color; color: $white; - transition: all 100ms linear; + transition: all 40ms linear; &:hover, &:active, &:focus { - background: darken($ui-primary-color, 5%); + background: darken($ui-primary-color, 2%); color: unset; } } diff --git a/app/models/media_attachment.rb b/app/models/media_attachment.rb index 4b84b95fa..b6e5916cb 100644 --- a/app/models/media_attachment.rb +++ b/app/models/media_attachment.rb @@ -160,23 +160,39 @@ class MediaAttachment < ApplicationRecord meta = {} file.queued_for_write.each do |style, file| - begin - geo = Paperclip::Geometry.from_file file - - meta[style] = { - width: geo.width.to_i, - height: geo.height.to_i, - size: "#{geo.width.to_i}x#{geo.height.to_i}", - aspect: geo.width.to_f / geo.height.to_f, - } - rescue Paperclip::Errors::NotIdentifiedByImageMagickError - meta[style] = {} - end + meta[style] = style == :small || image? ? image_geometry(file) : video_metadata(file) end meta end + def image_geometry(file) + geo = Paperclip::Geometry.from_file file + + { + width: geo.width.to_i, + height: geo.height.to_i, + size: "#{geo.width.to_i}x#{geo.height.to_i}", + aspect: geo.width.to_f / geo.height.to_f, + } + rescue Paperclip::Errors::NotIdentifiedByImageMagickError + {} + end + + def video_metadata(file) + movie = FFMPEG::Movie.new(file.path) + + return {} unless movie.valid? + + { + width: movie.width, + height: movie.height, + frame_rate: movie.frame_rate, + duration: movie.duration, + bitrate: movie.bitrate, + } + end + def appropriate_extension mime_type = MIME::Types[file.content_type] diff --git a/app/views/media/player.html.haml b/app/views/media/player.html.haml new file mode 100644 index 000000000..ea868b3f6 --- /dev/null +++ b/app/views/media/player.html.haml @@ -0,0 +1,2 @@ +%video{ poster: @media_attachment.file.url(:small), preload: 'auto', autoplay: 'autoplay', muted: 'muted', loop: 'loop', controls: 'controls', style: "width: #{@media_attachment.file.meta.dig('original', 'width')}px; height: #{@media_attachment.file.meta.dig('original', 'height')}px" } + %source{ src: @media_attachment.file.url(:original), type: @media_attachment.file_content_type } diff --git a/app/views/stream_entries/_og_image.html.haml b/app/views/stream_entries/_og_image.html.haml index bd14ef701..526034faa 100644 --- a/app/views/stream_entries/_og_image.html.haml +++ b/app/views/stream_entries/_og_image.html.haml @@ -1,23 +1,34 @@ - if activity.is_a?(Status) && activity.media_attachments.any? + - player_card = false - activity.media_attachments.each do |media| - if media.image? = opengraph 'og:image', full_asset_url(media.file.url(:original)) = opengraph 'og:image:type', media.file_content_type - unless media.file.meta.nil? - = opengraph 'og:image:width', media.file.meta['original']['width'] - = opengraph 'og:image:height', media.file.meta['original']['height'] - - elsif media.video? + = opengraph 'og:image:width', media.file.meta.dig('original', 'width') + = opengraph 'og:image:height', media.file.meta.dig('original', 'height') + - elsif media.video? || media.gifv? + - player_card = true = opengraph 'og:image', full_asset_url(media.file.url(:small)) = opengraph 'og:image:type', 'image/png' - unless media.file.meta.nil? - = opengraph 'og:image:width', media.file.meta['small']['width'] - = opengraph 'og:image:height', media.file.meta['small']['height'] + = opengraph 'og:image:width', media.file.meta.dig('small', 'width') + = opengraph 'og:image:height', media.file.meta.dig('small', 'height') = opengraph 'og:video', full_asset_url(media.file.url(:original)) + = opengraph 'og:video:secure_url', full_asset_url(media.file.url(:original)) = opengraph 'og:video:type', media.file_content_type + = opengraph 'twitter:player', medium_player_url(media) + = opengraph 'twitter:player:stream', full_asset_url(media.file.url(:original)) + = opengraph 'twitter:player:stream:content_type', media.file_content_type - unless media.file.meta.nil? - = opengraph 'og:video:width', media.file.meta['small']['width'] - = opengraph 'og:video:height', media.file.meta['small']['height'] - = opengraph 'twitter:card', 'summary_large_image' + = opengraph 'og:video:width', media.file.meta.dig('original', 'width') + = opengraph 'og:video:height', media.file.meta.dig('original', 'height') + = opengraph 'twitter:player:width', media.file.meta.dig('original', 'width') + = opengraph 'twitter:player:height', media.file.meta.dig('original', 'height') + - if player_card + = opengraph 'twitter:card', 'player' + - else + = opengraph 'twitter:card', 'summary_large_image' - else = opengraph 'og:image', full_asset_url(account.avatar.url(:original)) = opengraph 'og:image:width', '120' diff --git a/config/brakeman.ignore b/config/brakeman.ignore index db7e37bb9..e8956639c 100644 --- a/config/brakeman.ignore +++ b/config/brakeman.ignore @@ -7,7 +7,7 @@ "check_name": "LinkToHref", "message": "Potentially unsafe model attribute in link_to href", "file": "app/views/admin/accounts/show.html.haml", - "line": 143, + "line": 147, "link": "http://brakemanscanner.org/docs/warning_types/link_to_href", "code": "link_to(Account.find(params[:id]).inbox_url, Account.find(params[:id]).inbox_url)", "render_path": [{"type":"controller","class":"Admin::AccountsController","method":"show","line":18,"file":"app/controllers/admin/accounts_controller.rb"}], @@ -26,7 +26,7 @@ "check_name": "LinkToHref", "message": "Potentially unsafe model attribute in link_to href", "file": "app/views/admin/accounts/show.html.haml", - "line": 149, + "line": 153, "link": "http://brakemanscanner.org/docs/warning_types/link_to_href", "code": "link_to(Account.find(params[:id]).shared_inbox_url, Account.find(params[:id]).shared_inbox_url)", "render_path": [{"type":"controller","class":"Admin::AccountsController","method":"show","line":18,"file":"app/controllers/admin/accounts_controller.rb"}], @@ -45,7 +45,7 @@ "check_name": "LinkToHref", "message": "Potentially unsafe model attribute in link_to href", "file": "app/views/admin/accounts/show.html.haml", - "line": 54, + "line": 57, "link": "http://brakemanscanner.org/docs/warning_types/link_to_href", "code": "link_to(Account.find(params[:id]).url, Account.find(params[:id]).url)", "render_path": [{"type":"controller","class":"Admin::AccountsController","method":"show","line":18,"file":"app/controllers/admin/accounts_controller.rb"}], @@ -67,7 +67,7 @@ "line": 3, "link": "http://brakemanscanner.org/docs/warning_types/dynamic_render_path/", "code": "render(action => \"stream_entries/#{Account.find_local!(params[:account_username]).statuses.find(params[:id]).stream_entry.activity_type.downcase}\", { Account.find_local!(params[:account_username]).statuses.find(params[:id]).stream_entry.activity_type.downcase.to_sym => Account.find_local!(params[:account_username]).statuses.find(params[:id]).stream_entry.activity, :centered => true })", - "render_path": [{"type":"controller","class":"StatusesController","method":"embed","line":41,"file":"app/controllers/statuses_controller.rb"}], + "render_path": [{"type":"controller","class":"StatusesController","method":"embed","line":45,"file":"app/controllers/statuses_controller.rb"}], "location": { "type": "template", "template": "stream_entries/embed" @@ -102,7 +102,7 @@ "check_name": "LinkToHref", "message": "Potentially unsafe model attribute in link_to href", "file": "app/views/admin/accounts/show.html.haml", - "line": 152, + "line": 156, "link": "http://brakemanscanner.org/docs/warning_types/link_to_href", "code": "link_to(Account.find(params[:id]).followers_url, Account.find(params[:id]).followers_url)", "render_path": [{"type":"controller","class":"Admin::AccountsController","method":"show","line":18,"file":"app/controllers/admin/accounts_controller.rb"}], @@ -121,7 +121,7 @@ "check_name": "LinkToHref", "message": "Potentially unsafe model attribute in link_to href", "file": "app/views/admin/accounts/show.html.haml", - "line": 127, + "line": 130, "link": "http://brakemanscanner.org/docs/warning_types/link_to_href", "code": "link_to(Account.find(params[:id]).salmon_url, Account.find(params[:id]).salmon_url)", "render_path": [{"type":"controller","class":"Admin::AccountsController","method":"show","line":18,"file":"app/controllers/admin/accounts_controller.rb"}], @@ -140,10 +140,10 @@ "check_name": "Render", "message": "Render path contains parameter value", "file": "app/views/admin/custom_emojis/index.html.haml", - "line": 31, + "line": 45, "link": "http://brakemanscanner.org/docs/warning_types/dynamic_render_path/", "code": "render(action => filtered_custom_emojis.eager_load(:local_counterpart).page(params[:page]), {})", - "render_path": [{"type":"controller","class":"Admin::CustomEmojisController","method":"index","line":10,"file":"app/controllers/admin/custom_emojis_controller.rb"}], + "render_path": [{"type":"controller","class":"Admin::CustomEmojisController","method":"index","line":11,"file":"app/controllers/admin/custom_emojis_controller.rb"}], "location": { "type": "template", "template": "admin/custom_emojis/index" @@ -179,7 +179,7 @@ "check_name": "Render", "message": "Render path contains parameter value", "file": "app/views/admin/accounts/index.html.haml", - "line": 64, + "line": 67, "link": "http://brakemanscanner.org/docs/warning_types/dynamic_render_path/", "code": "render(action => filtered_accounts.page(params[:page]), {})", "render_path": [{"type":"controller","class":"Admin::AccountsController","method":"index","line":12,"file":"app/controllers/admin/accounts_controller.rb"}], @@ -191,6 +191,45 @@ "confidence": "Weak", "note": "" }, + { + "warning_type": "Cross-Site Request Forgery", + "warning_code": 7, + "fingerprint": "ab491f72606337a348482d006eb67a3b1616685fd48644d5ac909bbcd62a5000", + "check_name": "ForgerySetting", + "message": "'protect_from_forgery' should be called in WellKnown::HostMetaController", + "file": "app/controllers/well_known/host_meta_controller.rb", + "line": 4, + "link": "http://brakemanscanner.org/docs/warning_types/cross-site_request_forgery/", + "code": null, + "render_path": null, + "location": { + "type": "controller", + "controller": "WellKnown::HostMetaController" + }, + "user_input": null, + "confidence": "High", + "note": "" + }, + { + "warning_type": "Redirect", + "warning_code": 18, + "fingerprint": "ba699ddcc6552c422c4ecd50d2cd217f616a2446659e185a50b05a0f2dad8d33", + "check_name": "Redirect", + "message": "Possible unprotected redirect", + "file": "app/controllers/media_controller.rb", + "line": 10, + "link": "http://brakemanscanner.org/docs/warning_types/redirect/", + "code": "redirect_to(MediaAttachment.attached.find_by!(:shortcode => ((params[:id] or params[:medium_id]))).file.url(:original))", + "render_path": null, + "location": { + "type": "method", + "class": "MediaController", + "method": "show" + }, + "user_input": "MediaAttachment.attached.find_by!(:shortcode => ((params[:id] or params[:medium_id]))).file.url(:original)", + "confidence": "High", + "note": "" + }, { "warning_type": "Cross-Site Scripting", "warning_code": 4, @@ -198,7 +237,7 @@ "check_name": "LinkToHref", "message": "Potentially unsafe model attribute in link_to href", "file": "app/views/admin/accounts/show.html.haml", - "line": 116, + "line": 119, "link": "http://brakemanscanner.org/docs/warning_types/link_to_href", "code": "link_to(Account.find(params[:id]).remote_url, Account.find(params[:id]).remote_url)", "render_path": [{"type":"controller","class":"Admin::AccountsController","method":"show","line":18,"file":"app/controllers/admin/accounts_controller.rb"}], @@ -249,6 +288,25 @@ "confidence": "Weak", "note": "" }, + { + "warning_type": "Cross-Site Request Forgery", + "warning_code": 7, + "fingerprint": "d4278f04e807ec58a23925f8ab31fad5e84692f2fb9f2f57e7931aff05d57cf8", + "check_name": "ForgerySetting", + "message": "'protect_from_forgery' should be called in WellKnown::WebfingerController", + "file": "app/controllers/well_known/webfinger_controller.rb", + "line": 4, + "link": "http://brakemanscanner.org/docs/warning_types/cross-site_request_forgery/", + "code": null, + "render_path": null, + "location": { + "type": "controller", + "controller": "WellKnown::WebfingerController" + }, + "user_input": null, + "confidence": "High", + "note": "" + }, { "warning_type": "Cross-Site Scripting", "warning_code": 4, @@ -256,7 +314,7 @@ "check_name": "LinkToHref", "message": "Potentially unsafe model attribute in link_to href", "file": "app/views/admin/accounts/show.html.haml", - "line": 146, + "line": 150, "link": "http://brakemanscanner.org/docs/warning_types/link_to_href", "code": "link_to(Account.find(params[:id]).outbox_url, Account.find(params[:id]).outbox_url)", "render_path": [{"type":"controller","class":"Admin::AccountsController","method":"show","line":18,"file":"app/controllers/admin/accounts_controller.rb"}], @@ -275,10 +333,10 @@ "check_name": "Render", "message": "Render path contains parameter value", "file": "app/views/stream_entries/show.html.haml", - "line": 21, + "line": 24, "link": "http://brakemanscanner.org/docs/warning_types/dynamic_render_path/", "code": "render(partial => \"stream_entries/#{Account.find_local!(params[:account_username]).statuses.find(params[:id]).stream_entry.activity_type.downcase}\", { :locals => ({ Account.find_local!(params[:account_username]).statuses.find(params[:id]).stream_entry.activity_type.downcase.to_sym => Account.find_local!(params[:account_username]).statuses.find(params[:id]).stream_entry.activity, :include_threads => true }) })", - "render_path": [{"type":"controller","class":"StatusesController","method":"show","line":20,"file":"app/controllers/statuses_controller.rb"}], + "render_path": [{"type":"controller","class":"StatusesController","method":"show","line":22,"file":"app/controllers/statuses_controller.rb"}], "location": { "type": "template", "template": "stream_entries/show" @@ -288,6 +346,6 @@ "note": "" } ], - "updated": "2017-11-19 20:34:18 +0100", + "updated": "2018-02-16 06:42:53 +0100", "brakeman_version": "4.0.1" } diff --git a/config/routes.rb b/config/routes.rb index 34f33fa95..9f541200a 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -103,7 +103,10 @@ Rails.application.routes.draw do resources :sessions, only: [:destroy] end - resources :media, only: [:show] + resources :media, only: [:show] do + get :player + end + resources :tags, only: [:show] resources :emojis, only: [:show] resources :invites, only: [:index, :create, :destroy] diff --git a/spec/models/media_attachment_spec.rb b/spec/models/media_attachment_spec.rb index b40a641f7..d2230b6d0 100644 --- a/spec/models/media_attachment_spec.rb +++ b/spec/models/media_attachment_spec.rb @@ -92,7 +92,6 @@ RSpec.describe MediaAttachment, type: :model do it 'sets meta' do expect(media.file.meta["original"]["width"]).to eq 128 expect(media.file.meta["original"]["height"]).to eq 128 - expect(media.file.meta["original"]["aspect"]).to eq 1.0 end end -- cgit