From 9dbae6e8a120fc18fdc007503375b53f6b061b8f Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Fri, 16 Feb 2018 07:22:20 +0100 Subject: [PATCH] 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 + .../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 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