Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update AttachmentMaskController referer handling #7975

Merged
merged 1 commit into from
Oct 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 14 additions & 8 deletions app/controllers/attachment_masks_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
6 changes: 5 additions & 1 deletion app/controllers/attachments_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -224,4 +224,8 @@ def prominence
def with_prominence
@info_request
end

def verifier
Rails.application.message_verifier('AttachmentsController')
end
end
2 changes: 1 addition & 1 deletion app/views/attachment_masks/done.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,5 @@
</p>

<p>
<%= link_to _('Download attachment'), @show_attachment_path, class: 'button' %>
<%= link_to _('Download attachment'), @referer, class: 'button' %>
</p>
28 changes: 25 additions & 3 deletions spec/controllers/attachment_masks_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
14 changes: 13 additions & 1 deletion spec/controllers/attachments_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading