diff --git a/app/controllers/attachments_controller.rb b/app/controllers/attachments_controller.rb index a414aae17d..1fb20f156a 100644 --- a/app/controllers/attachments_controller.rb +++ b/app/controllers/attachments_controller.rb @@ -57,7 +57,7 @@ def find_info_request if public_token? InfoRequest.find_by!(public_token: public_token) else - InfoRequest.find(params[:id]) + InfoRequest.find_by!(url_title: params[:request_url_title]) end end diff --git a/app/helpers/info_request_helper.rb b/app/helpers/info_request_helper.rb index 1314ad220b..a39c74a113 100644 --- a/app/helpers/info_request_helper.rb +++ b/app/helpers/info_request_helper.rb @@ -315,7 +315,8 @@ def attachment_params(attachment, options = {}) if public_token? attach_params[:public_token] = public_token else - attach_params[:id] = attachment.incoming_message.info_request_id + attach_params[:request_url_title] = attachment.incoming_message. + info_request.url_title end if options[:html] attach_params[:file_name] = "#{attachment.display_filename}.html" diff --git a/config/routes.rb b/config/routes.rb index 3b4508376e..d7ee4bd8f4 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -127,13 +127,14 @@ def matches?(request) :as => :similar_request, :via => :get - match '/request/:id/response/:incoming_message_id/attach/html' \ - '/(:part(/*file_name))' => 'attachments#show_as_html', + match '/request/:request_url_title/response/:incoming_message_id/attach' \ + '/html/(:part(/*file_name))' => 'attachments#show_as_html', :format => false, :as => :get_attachment_as_html, :via => :get, :constraints => { :part => /\d+/ } - match '/request/:id/response/:incoming_message_id/attach/:part(/*file_name)' => 'attachments#show', + match '/request/:request_url_title/response/:incoming_message_id/attach' \ + '/:part(/*file_name)' => 'attachments#show', :format => false, :as => :get_attachment, :via => :get, diff --git a/config/routes/redirects.rb b/config/routes/redirects.rb index 833843f570..0b1dda0495 100644 --- a/config/routes/redirects.rb +++ b/config/routes/redirects.rb @@ -33,3 +33,12 @@ def redirect_to_info_request_url_title_path(request:, locale:, id:, suffix: nil) redirect_to_info_request_url_title_path(request: request, **params) end ) + +get '/request/:id(/*suffix)', + format: false, + constraints: { id: /\d+/, suffix: %r(response/\d+/attach(/html)?/\d+/.*) }, + to: redirect( + -> (params, request) do + redirect_to_info_request_url_title_path(request: request, **params) + end + ) diff --git a/spec/controllers/attachments_controller_spec.rb b/spec/controllers/attachments_controller_spec.rb index a2a60a95bf..21348b0d6e 100644 --- a/spec/controllers/attachments_controller_spec.rb +++ b/spec/controllers/attachments_controller_spec.rb @@ -43,39 +43,13 @@ def show(params = {}) part: attachment.url_part_number, file_name: attachment.display_filename } - default_params[:id] = info_request.id unless params[:public_token] + unless params[:public_token] + default_params[:request_url_title] = info_request.url_title + end rebuild_raw_emails(info_request) get :show, params: default_params.merge(params) end - # This is a regression test for a bug where URLs of this form were causing - # 500 errors instead of 404s. - # - # (Note that in fact only the integer-prefix of the URL part is used, so - # there are *some* “ugly URLs containing a request id that isn\'t an - # integer” that actually return a 200 response. The point is that IDs of - # this sort were triggering an error in the error-handling path, causing - # the wrong sort of error response to be returned in the case where the - # integer prefix referred to the wrong request.) - # - # https://github.com/mysociety/alaveteli/issues/351 - it 'should return 404 for ugly URLs containing a request id that isn\'t an integer' do - ugly_id = '55195' - expect { show(id: ugly_id) } - .to raise_error(ActiveRecord::RecordNotFound) - end - - it 'should return 404 when incoming message and request ids don\'t match' do - expect { show(id: info_request.id + 1) } - .to raise_error(ActiveRecord::RecordNotFound) - end - - it 'should return 404 for ugly URLs contain a request id that isn\'t an integer, even if the integer prefix refers to an actual request' do - ugly_id = '#{FactoryBot.create(:info_request).id}95' - expect { show(id: ugly_id) } - .to raise_error(ActiveRecord::RecordNotFound) - end - it 'should redirect to the incoming message if there\'s a wrong part number and an ambiguous filename' do show( part: attachment.url_part_number + 1, @@ -143,7 +117,7 @@ def show(params = {}) allow(controller).to receive(:verifier).and_return(verifier) allow(verifier).to receive(:generate).with( get_attachment_path( - info_request.id, + info_request.url_title, incoming_message_id: attachment.incoming_message_id, part: attachment.url_part_number, file_name: attachment.filename @@ -416,7 +390,9 @@ def show_as_html(params = {}) part: attachment.url_part_number, file_name: attachment.display_filename } - default_params[:id] = info_request.id unless params[:public_token] + unless params[:public_token] + default_params[:request_url_title] = info_request.url_title + end get :show_as_html, params: default_params.merge(params) end @@ -438,18 +414,6 @@ def show_as_html(params = {}) expect(response.headers['X-Robots-Tag']).to eq 'noindex' end - it 'should return 404 for ugly URLs containing a request id that isn\'t an integer' do - ugly_id = '55195' - expect { show_as_html(id: ugly_id) } - .to raise_error(ActiveRecord::RecordNotFound) - end - - it 'should return 404 for ugly URLs contain a request id that isn\'t an integer, even if the integer prefix refers to an actual request' do - ugly_id = FactoryBot.create(:info_request).id.to_s + '95' - expect { show_as_html(id: ugly_id) } - .to raise_error(ActiveRecord::RecordNotFound) - end - context 'when attachment has not been masked' do let(:attachment) do FactoryBot.create( @@ -498,7 +462,7 @@ def show_as_html(params = {}) allow(controller).to receive(:verifier).and_return(verifier) allow(verifier).to receive(:generate).with( get_attachment_as_html_path( - info_request.id, + info_request.url_title, incoming_message_id: attachment.incoming_message_id, part: attachment.url_part_number, file_name: attachment.filename diff --git a/spec/helpers/info_request_helper_spec.rb b/spec/helpers/info_request_helper_spec.rb index 6110392db9..56284c70d4 100644 --- a/spec/helpers/info_request_helper_spec.rb +++ b/spec/helpers/info_request_helper_spec.rb @@ -595,7 +595,7 @@ context 'when given no format options' do it 'returns the path to the attachment with a cookie cookie_passthrough param' do expect(attachment_path(jpeg_attachment)).to eq( - "/request/#{incoming_message.info_request_id}" \ + "/request/#{incoming_message.info_request.url_title}" \ "/response/#{incoming_message.id}/" \ "attach/#{jpeg_attachment.url_part_number}" \ "/interesting.jpg?cookie_passthrough=1" @@ -606,7 +606,7 @@ context 'when given an html format option' do it 'returns the path to the HTML version of the attachment' do expect(attachment_path(jpeg_attachment, html: true)).to eq( - "/request/#{incoming_message.info_request_id}" \ + "/request/#{incoming_message.info_request.url_title}" \ "/response/#{incoming_message.id}" \ "/attach/html/#{jpeg_attachment.url_part_number}" \ "/interesting.jpg.html" @@ -637,7 +637,7 @@ it 'returns the URL to the attachment with a cookie cookie_passthrough param' do expect(attachment_url(jpeg_attachment)).to eq( "http://test.host" \ - "/request/#{incoming_message.info_request_id}" \ + "/request/#{incoming_message.info_request.url_title}" \ "/response/#{incoming_message.id}" \ "/attach/#{jpeg_attachment.url_part_number}" \ "/interesting.jpg?cookie_passthrough=1" @@ -649,7 +649,7 @@ it 'returns the URL to the HTML version of the attachment' do expect(attachment_url(jpeg_attachment, html: true)).to eq( "http://test.host" \ - "/request/#{incoming_message.info_request_id}" \ + "/request/#{incoming_message.info_request.url_title}" \ "/response/#{incoming_message.id}" \ "/attach/html/#{jpeg_attachment.url_part_number}" \ "/interesting.jpg.html" diff --git a/spec/integration/errors_spec.rb b/spec/integration/errors_spec.rb index 228bd9298f..7254826b68 100644 --- a/spec/integration/errors_spec.rb +++ b/spec/integration/errors_spec.rb @@ -90,6 +90,7 @@ it 'should render a 403 with text body for attempts at directory listing for attachments' do info_request = FactoryBot.create(:info_request_with_pdf_attachment) id = info_request.id + url_title = info_request.url_title prefix = id.to_s[0..2] msg_id = info_request.incoming_messages.first.id @@ -99,11 +100,11 @@ ) FileUtils.mkdir_p(cache_key_path) - get "/request/#{id}/response/#{msg_id}/attach/html/1/" + get "/request/#{url_title}/response/#{msg_id}/attach/html/1/" expect(response.body).to include('Directory listing not allowed') expect(response.code).to eq('403') - get "/request/#{id}/response/#{msg_id}/attach/html" + get "/request/#{url_title}/response/#{msg_id}/attach/html" expect(response.body).to include('Directory listing not allowed') expect(response.code).to eq('403') end diff --git a/spec/integration/incoming_mail_spec.rb b/spec/integration/incoming_mail_spec.rb index 98844c2890..40451ba816 100644 --- a/spec/integration/incoming_mail_spec.rb +++ b/spec/integration/incoming_mail_spec.rb @@ -23,14 +23,14 @@ email_to: info_request.incoming_email) attachment_1_path = get_attachment_path( - info_request.id, + info_request.url_title, incoming_message_id: info_request.incoming_messages.first.id, part: 2, file_name: 'hello world.txt', skip_cache: 1 ) attachment_2_path = get_attachment_path( - info_request.id, + info_request.url_title, incoming_message_id: info_request.incoming_messages.first.id, part: 3, file_name: 'hello world.txt', @@ -65,7 +65,7 @@ receive_incoming_mail('incoming-request-two-same-name.email', email_to: info_request.incoming_email) attachment_path = get_attachment_as_html_path( - info_request.id, + info_request.url_title, incoming_message_id: info_request.incoming_messages.first.id, part: 2, file_name: 'hello world.txt.html', @@ -83,7 +83,7 @@ receive_incoming_mail('incoming-request-pdf-attachment.email', email_to: info_request.incoming_email) attachment_path = get_attachment_as_html_path( - info_request.id, + info_request.url_title, incoming_message_id: info_request.incoming_messages.first.id, part: 2, file_name: 'fs 50379341.pdf.html', @@ -103,7 +103,7 @@ # asking for an attachment by the wrong filename should result in # redirecting back to the incoming message visit get_attachment_as_html_path( - info_request.id, + info_request.url_title, incoming_message_id: info_request.incoming_messages.first.id, part: 2, file_name: 'hello world.txt.baz.html', @@ -118,7 +118,7 @@ email_to: info_request.incoming_email) attachment_path = get_attachment_path( - info_request.id, + info_request.url_title, incoming_message_id: info_request.incoming_messages.first.id, part: 2, file_name: 'hello.qwglhm', diff --git a/spec/integration/prominence/viewing_attachment_as_html_spec.rb b/spec/integration/prominence/viewing_attachment_as_html_spec.rb index beeb7d3604..3f7e01b14e 100644 --- a/spec/integration/prominence/viewing_attachment_as_html_spec.rb +++ b/spec/integration/prominence/viewing_attachment_as_html_spec.rb @@ -8,7 +8,7 @@ let(:within_session) do -> { visit get_attachment_as_html_url( - info_request.id, + info_request.url_title, incoming_message_id: attachment.incoming_message_id, part: attachment.url_part_number, file_name: "#{attachment.display_filename}.html" diff --git a/spec/integration/prominence/viewing_raw_attachment_spec.rb b/spec/integration/prominence/viewing_raw_attachment_spec.rb index 7d49a6cd0f..2e1ec5922a 100644 --- a/spec/integration/prominence/viewing_raw_attachment_spec.rb +++ b/spec/integration/prominence/viewing_raw_attachment_spec.rb @@ -10,7 +10,7 @@ rebuild_raw_emails(info_request) visit get_attachment_url( - info_request.id, + info_request.url_title, incoming_message_id: attachment.incoming_message_id, part: attachment.url_part_number, file_name: attachment.display_filename diff --git a/spec/integration/view_request_spec.rb b/spec/integration/view_request_spec.rb index 20424be918..0d35264dfe 100644 --- a/spec/integration/view_request_spec.rb +++ b/spec/integration/view_request_spec.rb @@ -51,7 +51,7 @@ ) rebuild_raw_emails(info_request) - attachment_url = "/es/request/#{info_request.id}/response/" \ + attachment_url = "/es/request/#{info_request.url_title}/response/" \ "#{incoming_message.id}/attach/#{attachment.url_part_number}/" \ "#{attachment.filename}" using_session(non_owner) { visit(attachment_url) }