diff --git a/CHANGELOG.md b/CHANGELOG.md index 7d0110936..6296f0016 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,13 @@ Changelog All notable changes to this project will be documented in this file. +## [v3.1.5] - 2020-07-07 +### Security + +- Fix media attachment enumeration ([ThibG](https://github.com/tootsuite/mastodon/pull/14254)) +- Change rate limits for various paths ([Gargron](https://github.com/tootsuite/mastodon/pull/14253)) +- Fix other sessions not being logged out on password change ([Gargron](https://github.com/tootsuite/mastodon/pull/14252)) + ## [v3.1.4] - 2020-05-14 ### Added diff --git a/app/controllers/auth/passwords_controller.rb b/app/controllers/auth/passwords_controller.rb index b98bcecd0..5db2668f7 100644 --- a/app/controllers/auth/passwords_controller.rb +++ b/app/controllers/auth/passwords_controller.rb @@ -8,7 +8,10 @@ class Auth::PasswordsController < Devise::PasswordsController def update super do |resource| - resource.session_activations.destroy_all if resource.errors.empty? + if resource.errors.empty? + resource.session_activations.destroy_all + resource.forget_me! + end end end diff --git a/app/controllers/auth/registrations_controller.rb b/app/controllers/auth/registrations_controller.rb index 78feb1631..d31966248 100644 --- a/app/controllers/auth/registrations_controller.rb +++ b/app/controllers/auth/registrations_controller.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class Auth::RegistrationsController < Devise::RegistrationsController + include Devise::Controllers::Rememberable + layout :determine_layout before_action :set_invite, only: [:new, :create] @@ -24,7 +26,11 @@ class Auth::RegistrationsController < Devise::RegistrationsController def update super do |resource| - resource.clear_other_sessions(current_session.session_id) if resource.saved_change_to_encrypted_password? + if resource.saved_change_to_encrypted_password? + resource.clear_other_sessions(current_session.session_id) + resource.forget_me! + remember_me(resource) + end end end diff --git a/app/controllers/home_controller.rb b/app/controllers/home_controller.rb index 7c8a18d17..702889cd0 100644 --- a/app/controllers/home_controller.rb +++ b/app/controllers/home_controller.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true class HomeController < ApplicationController + before_action :redirect_unauthenticated_to_permalinks! before_action :authenticate_user! before_action :set_referrer_policy_header @@ -10,7 +11,7 @@ class HomeController < ApplicationController private - def authenticate_user! + def redirect_unauthenticated_to_permalinks! return if user_signed_in? matches = request.path.match(/\A\/web\/(statuses|accounts)\/([\d]+)\z/) @@ -35,6 +36,7 @@ class HomeController < ApplicationController end matches = request.path.match(%r{\A/web/timelines/tag/(?.+)\z}) + redirect_to(matches ? tag_path(CGI.unescape(matches[:tag])) : default_redirect_path) end diff --git a/app/controllers/media_proxy_controller.rb b/app/controllers/media_proxy_controller.rb index 014b89de1..e36673fc4 100644 --- a/app/controllers/media_proxy_controller.rb +++ b/app/controllers/media_proxy_controller.rb @@ -2,6 +2,7 @@ class MediaProxyController < ApplicationController include RoutingHelper + include Authorization skip_before_action :store_current_location skip_before_action :require_functional! @@ -10,12 +11,14 @@ class MediaProxyController < ApplicationController rescue_from ActiveRecord::RecordInvalid, with: :not_found rescue_from Mastodon::UnexpectedResponseError, with: :not_found + rescue_from Mastodon::NotPermittedError, with: :not_found rescue_from HTTP::TimeoutError, HTTP::ConnectionError, OpenSSL::SSL::SSLError, with: :internal_server_error def show RedisLock.acquire(lock_options) do |lock| if lock.acquired? - @media_attachment = MediaAttachment.remote.find(params[:id]) + @media_attachment = MediaAttachment.remote.attached.find(params[:id]) + authorize @media_attachment.status, :show? redownload! if @media_attachment.needs_redownload? && !reject_media? else raise Mastodon::RaceConditionError diff --git a/config/initializers/rack_attack.rb b/config/initializers/rack_attack.rb index 09458c540..cd29afac5 100644 --- a/config/initializers/rack_attack.rb +++ b/config/initializers/rack_attack.rb @@ -38,15 +38,6 @@ class Rack::Attack end end - PROTECTED_PATHS = %w( - /auth/sign_in - /auth - /auth/password - /auth/confirmation - ).freeze - - PROTECTED_PATHS_REGEX = Regexp.union(PROTECTED_PATHS.map { |path| /\A#{Regexp.escape(path)}/ }) - Rack::Attack.safelist('allow from localhost') do |req| req.remote_ip == '127.0.0.1' || req.remote_ip == '::1' end @@ -86,8 +77,32 @@ class Rack::Attack req.authenticated_user_id if (req.post? && req.path =~ API_DELETE_REBLOG_REGEX) || (req.delete? && req.path =~ API_DELETE_STATUS_REGEX) end - throttle('protected_paths', limit: 25, period: 5.minutes) do |req| - req.remote_ip if req.post? && req.path =~ PROTECTED_PATHS_REGEX + throttle('throttle_sign_up_attempts/ip', limit: 25, period: 5.minutes) do |req| + req.remote_ip if req.post? && req.path == '/auth' + end + + throttle('throttle_password_resets/ip', limit: 25, period: 5.minutes) do |req| + req.remote_ip if req.post? && req.path == '/auth/password' + end + + throttle('throttle_password_resets/email', limit: 5, period: 30.minutes) do |req| + req.params.dig('user', 'email').presence if req.post? && req.path == '/auth/password' + end + + throttle('throttle_email_confirmations/ip', limit: 25, period: 5.minutes) do |req| + req.remote_ip if req.post? && req.path == '/auth/confirmation' + end + + throttle('throttle_email_confirmations/email', limit: 5, period: 30.minutes) do |req| + req.params.dig('user', 'email').presence if req.post? && req.path == '/auth/password' + end + + throttle('throttle_login_attempts/ip', limit: 25, period: 5.minutes) do |req| + req.remote_ip if req.post? && req.path == '/auth/sign_in' + end + + throttle('throttle_login_attempts/email', limit: 25, period: 1.hour) do |req| + req.session[:attempt_user_id] || req.params.dig('user', 'email').presence if req.post? && req.path == '/auth/sign_in' end self.throttled_response = lambda do |env| diff --git a/config/initializers/rack_attack_logging.rb b/config/initializers/rack_attack_logging.rb index c30bd8a64..ab4822e96 100644 --- a/config/initializers/rack_attack_logging.rb +++ b/config/initializers/rack_attack_logging.rb @@ -2,5 +2,6 @@ ActiveSupport::Notifications.subscribe(/rack_attack/) do |_name, _start, _finish req = payload[:request] next unless [:throttle, :blacklist].include? req.env['rack.attack.match_type'] + Rails.logger.info("Rate limit hit (#{req.env['rack.attack.match_type']}): #{req.ip} #{req.request_method} #{req.fullpath}") end diff --git a/lib/mastodon/version.rb b/lib/mastodon/version.rb index f73b8805a..49523520d 100644 --- a/lib/mastodon/version.rb +++ b/lib/mastodon/version.rb @@ -13,7 +13,7 @@ module Mastodon end def patch - 4 + 5 end def flags diff --git a/spec/controllers/media_controller_spec.rb b/spec/controllers/media_controller_spec.rb index ac44a76f2..2925aed59 100644 --- a/spec/controllers/media_controller_spec.rb +++ b/spec/controllers/media_controller_spec.rb @@ -28,9 +28,8 @@ describe MediaController do end it 'raises when not permitted to view' do - status = Fabricate(:status) + status = Fabricate(:status, visibility: :direct) media_attachment = Fabricate(:media_attachment, status: status) - allow_any_instance_of(MediaController).to receive(:authorize).and_raise(ActiveRecord::RecordNotFound) get :show, params: { id: media_attachment.to_param } expect(response).to have_http_status(404) diff --git a/spec/controllers/media_proxy_controller_spec.rb b/spec/controllers/media_proxy_controller_spec.rb new file mode 100644 index 000000000..32510cf43 --- /dev/null +++ b/spec/controllers/media_proxy_controller_spec.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe MediaProxyController do + render_views + + before do + stub_request(:get, 'http://example.com/attachment.png').to_return(request_fixture('avatar.txt')) + end + + describe '#show' do + it 'redirects when attached to a status' do + status = Fabricate(:status) + media_attachment = Fabricate(:media_attachment, status: status, remote_url: 'http://example.com/attachment.png') + get :show, params: { id: media_attachment.id } + + expect(response).to have_http_status(302) + end + + it 'responds with missing when there is not an attached status' do + media_attachment = Fabricate(:media_attachment, status: nil, remote_url: 'http://example.com/attachment.png') + get :show, params: { id: media_attachment.id } + + expect(response).to have_http_status(404) + end + + it 'raises when id cant be found' do + get :show, params: { id: 'missing' } + + expect(response).to have_http_status(404) + end + + it 'raises when not permitted to view' do + status = Fabricate(:status, visibility: :direct) + media_attachment = Fabricate(:media_attachment, status: status, remote_url: 'http://example.com/attachment.png') + get :show, params: { id: media_attachment.id } + + expect(response).to have_http_status(404) + end + end +end