From 08cfe9065b02a47d8fddb3dc9ceadb04b0cc488d Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Tue, 24 Oct 2023 10:08:30 +0100 Subject: [PATCH] Update AttachmentMaskController referer handling Addresses potential cross-site scripting issue identified by Brakeman. This change uses `MessageVerifier` to encodes the referer when passing it from the `AttachmentController` to the `AttachmentMaskController`, this is needed because we don't know which route the attachment was requested from (public, share by link, project). We were relying on `redirect_to` raising an `UnsafeRedirectError` exception when the referer param was changes to an external link. This works for the controller, but we also use the referer param on the `done` view. So once the attachment is available, if the param is manually changed the resulting page would have a download link which points to changed referer param. We use temporary signed attachment IDs so the resulting URL would only be available for a short period of time so the attack vector is minimal. --- .../attachment_masks_controller.rb | 22 +++++++++------ app/controllers/attachments_controller.rb | 6 +++- app/views/attachment_masks/done.html.erb | 2 +- .../attachment_masks_controller_spec.rb | 28 +++++++++++++++++-- .../attachments_controller_spec.rb | 14 +++++++++- 5 files changed, 58 insertions(+), 14 deletions(-) diff --git a/app/controllers/attachment_masks_controller.rb b/app/controllers/attachment_masks_controller.rb index 37cdd09497..f5e349fa56 100644 --- a/app/controllers/attachment_masks_controller.rb +++ b/app/controllers/attachment_masks_controller.rb @@ -4,14 +4,14 @@ # class AttachmentMasksController < ApplicationController before_action :set_no_crawl_headers - before_action :find_attachment - before_action :ensure_referer, :ensure_attachment + before_action :decode_referer, :ensure_referer + before_action :find_attachment, :ensure_attachment def wait if @attachment.masked? redirect_to done_attachment_mask_path( id: @attachment.to_signed_global_id, - referer: params[:referer] + referer: verifier.generate(@referer) ) else @@ -23,11 +23,9 @@ def done unless @attachment.masked? redirect_to wait_for_attachment_mask_path( id: @attachment.to_signed_global_id, - referer: params[:referer] + referer: verifier.generate(@referer) ) end - - @show_attachment_path = params[:referer] end private @@ -36,16 +34,24 @@ def set_no_crawl_headers headers['X-Robots-Tag'] = 'noindex' end + def decode_referer + @referer = verifier.verified(params[:referer]) + end + def find_attachment @attachment = GlobalID::Locator.locate_signed(params[:id]) rescue ActiveRecord::RecordNotFound end def ensure_referer - raise RouteNotFound unless params[:referer].present? + raise RouteNotFound unless @referer end def ensure_attachment - redirect_to(params[:referer]) unless @attachment + redirect_to(@referer) unless @attachment + end + + def verifier + Rails.application.message_verifier('AttachmentsController') end end diff --git a/app/controllers/attachments_controller.rb b/app/controllers/attachments_controller.rb index 554dbd1115..3048407f43 100644 --- a/app/controllers/attachments_controller.rb +++ b/app/controllers/attachments_controller.rb @@ -36,7 +36,7 @@ def show rescue Timeout::Error redirect_to wait_for_attachment_mask_path( @attachment.to_signed_global_id, - referer: request.fullpath + referer: verifier.generate(request.fullpath) ) end @@ -224,4 +224,8 @@ def prominence def with_prominence @info_request end + + def verifier + Rails.application.message_verifier('AttachmentsController') + end end diff --git a/app/views/attachment_masks/done.html.erb b/app/views/attachment_masks/done.html.erb index f7bbad69b3..f4a6fcd2b6 100644 --- a/app/views/attachment_masks/done.html.erb +++ b/app/views/attachment_masks/done.html.erb @@ -7,5 +7,5 @@

- <%= link_to _('Download attachment'), @show_attachment_path, class: 'button' %> + <%= link_to _('Download attachment'), @referer, class: 'button' %>

diff --git a/spec/controllers/attachment_masks_controller_spec.rb b/spec/controllers/attachment_masks_controller_spec.rb index 11b636e6e3..297e7997bb 100644 --- a/spec/controllers/attachment_masks_controller_spec.rb +++ b/spec/controllers/attachment_masks_controller_spec.rb @@ -2,11 +2,17 @@ RSpec.describe AttachmentMasksController, type: :controller do let(:attachment) { FactoryBot.build(:body_text, id: 1) } - let(:referer) { '/referer' } + let(:referer) { 'DEF' } before do allow(GlobalID::Locator).to receive(:locate_signed).with('ABC'). and_return(attachment) + + verifier = double('ActiveSupport::MessageVerifier') + allow(controller).to receive(:verifier).and_return(verifier) + allow(verifier).to receive(:generate).with('/referer').and_return('DEF') + allow(verifier).to receive(:verified).and_return(nil) + allow(verifier).to receive(:verified).with('DEF').and_return('/referer') end describe 'GET wait' do @@ -19,7 +25,7 @@ def wait allow(attachment).to receive(:to_signed_global_id).and_return('ABC') wait expect(response).to redirect_to( - done_attachment_mask_path('ABC', referer: '/referer') + done_attachment_mask_path('ABC', referer: 'DEF') ) end end @@ -69,6 +75,14 @@ def wait expect { wait }.to raise_error(ApplicationController::RouteNotFound) end end + + context 'with modified referer' do + let(:referer) { 'http://example.com/attack' } + + it 'raises route not found error' do + expect { wait }.to raise_error(ApplicationController::RouteNotFound) + end + end end describe 'GET done' do @@ -83,7 +97,7 @@ def done allow(attachment).to receive(:to_signed_global_id).and_return('ABC') done expect(response).to redirect_to( - wait_for_attachment_mask_path('ABC', referer: '/referer') + wait_for_attachment_mask_path('ABC', referer: 'DEF') ) end end @@ -125,5 +139,13 @@ def done expect { done }.to raise_error(ApplicationController::RouteNotFound) end end + + context 'with modified referer' do + let(:referer) { 'http://example.com/attack' } + + it 'raises route not found error' do + expect { done }.to raise_error(ApplicationController::RouteNotFound) + end + end end end diff --git a/spec/controllers/attachments_controller_spec.rb b/spec/controllers/attachments_controller_spec.rb index 1f2eb82148..cc6cad6093 100644 --- a/spec/controllers/attachments_controller_spec.rb +++ b/spec/controllers/attachments_controller_spec.rb @@ -138,9 +138,21 @@ def show(params = {}) it 'redirects to wait for attachment mask route' do allow_any_instance_of(FoiAttachment).to receive(:to_signed_global_id). and_return('ABC') + + verifier = double('ActiveSupport::MessageVerifier') + allow(controller).to receive(:verifier).and_return(verifier) + allow(verifier).to receive(:generate).with( + get_attachment_path( + incoming_message_id: attachment.incoming_message_id, + id: info_request.id, + part: attachment.url_part_number, + file_name: attachment.filename + ) + ).and_return('DEF') + show expect(response).to redirect_to( - wait_for_attachment_mask_path('ABC', referer: request.fullpath) + wait_for_attachment_mask_path('ABC', referer: 'DEF') ) end end