From 58cc14a33babcc3b7d5cb57a1f2e4bfc98fff2d3 Mon Sep 17 00:00:00 2001 From: Keith Lawrence Date: Wed, 18 Dec 2024 16:27:37 +0000 Subject: [PATCH 1/2] Simplify @attachement_metadata - We're only ever interested in the first found value, so use #find instead of #select, check for nil? rather than empty?, and then we can remove all the #first calls in the templates. --- app/controllers/csv_preview_controller.rb | 4 ++-- app/views/csv_preview/malformed_csv.html.erb | 8 ++++---- app/views/csv_preview/show.html.erb | 8 ++++---- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/app/controllers/csv_preview_controller.rb b/app/controllers/csv_preview_controller.rb index c059ac0282..953cf78208 100644 --- a/app/controllers/csv_preview_controller.rb +++ b/app/controllers/csv_preview_controller.rb @@ -15,11 +15,11 @@ def show parent_document_uri = @asset["parent_document_url"] parent_document_path = URI(parent_document_uri).request_uri @content_item = GdsApi.content_store.content_item(parent_document_path).to_hash - @attachment_metadata = @content_item.dig("details", "attachments").select do |attachment| + @attachment_metadata = @content_item.dig("details", "attachments").find do |attachment| attachment["filename"] == asset_filename end - if @attachment_metadata.empty? + if @attachment_metadata.nil? redirect_to(parent_document_uri, status: :see_other, allow_other_host: true) and return end diff --git a/app/views/csv_preview/malformed_csv.html.erb b/app/views/csv_preview/malformed_csv.html.erb index 39ea5d9aca..4bc1d4eeab 100644 --- a/app/views/csv_preview/malformed_csv.html.erb +++ b/app/views/csv_preview/malformed_csv.html.erb @@ -2,7 +2,7 @@ add_view_stylesheet("csv_preview") %> -<% content_for :title, "#{@attachment_metadata.first["title"]} - GOV.UK" %> +<% content_for :title, "#{@attachment_metadata["title"]} - GOV.UK" %>
@@ -35,7 +35,7 @@ <%= render 'govuk_publishing_components/components/inverse_header', {} do %> <%= render "govuk_publishing_components/components/title", { context: I18n.t("csv_preview.document_type.#{@content_item['document_type']}", count: 1), - title: @attachment_metadata.first["title"], + title: @attachment_metadata["title"], font_size: "xl", inverse: true, margin_bottom: 8, @@ -44,7 +44,7 @@

Updated <%= I18n.l(Time.zone.parse(@content_item["public_updated_at"]), format: "%-d %B %Y") %>
- <%= link_to("Download CSV #{number_to_human_size(@attachment_metadata.first['file_size'])}".html_safe, @attachment_metadata.first['url'], class: "csv-preview__download-link") %> + <%= link_to("Download CSV #{number_to_human_size(@attachment_metadata['file_size'])}".html_safe, @attachment_metadata['url'], class: "csv-preview__download-link") %>

<% end %>
@@ -57,7 +57,7 @@

This CSV cannot be viewed online.
- You can <%= link_to("download the file", @attachment_metadata.first['url'], class: "govuk-link") %> to open it with your own software. + You can <%= link_to("download the file", @attachment_metadata['url'], class: "govuk-link") %> to open it with your own software.

diff --git a/app/views/csv_preview/show.html.erb b/app/views/csv_preview/show.html.erb index 1745446a4e..aec208bde1 100644 --- a/app/views/csv_preview/show.html.erb +++ b/app/views/csv_preview/show.html.erb @@ -2,7 +2,7 @@ add_view_stylesheet("csv_preview") %> -<% content_for :title, "#{@attachment_metadata.first["title"]} - GOV.UK" %> +<% content_for :title, "#{@attachment_metadata["title"]} - GOV.UK" %>
@@ -35,7 +35,7 @@ <%= render 'govuk_publishing_components/components/inverse_header', {} do %> <%= render "govuk_publishing_components/components/title", { context: I18n.t("csv_preview.document_type.#{@content_item['document_type']}", count: 1), - title: @attachment_metadata.first["title"], + title: @attachment_metadata["title"], font_size: "xl", inverse: true, margin_bottom: 8, @@ -44,7 +44,7 @@

Updated <%= I18n.l(Time.zone.parse(@content_item["public_updated_at"]), format: "%-d %B %Y") %>
- <%= link_to("Download CSV #{number_to_human_size(@attachment_metadata.first['file_size'])}".html_safe, @attachment_metadata.first['url'], class: "csv-preview__download-link") %> + <%= link_to("Download CSV #{number_to_human_size(@attachment_metadata['file_size'])}".html_safe, @attachment_metadata['url'], class: "csv-preview__download-link") %>

<% end %>
@@ -56,7 +56,7 @@ } do %>

This preview shows the first 1,000 rows and 50 columns. - <%= link_to("Download CSV #{number_to_human_size(@attachment_metadata.first['file_size'])}", @attachment_metadata.first['url'], class: "govuk-link") %> + <%= link_to("Download CSV #{number_to_human_size(@attachment_metadata['file_size'])}", @attachment_metadata['url'], class: "govuk-link") %>

<% end %> <% end %> From 7313dad0cc3adfa6c380e9299f24431d80911770 Mon Sep 17 00:00:00 2001 From: Keith Lawrence Date: Wed, 18 Dec 2024 16:29:24 +0000 Subject: [PATCH 2/2] Check content_type before showing a preview - search engines contain some links to preview paths for assets that aren't previewable. Since we only allow previews for text/csv, we check the content_type of the attachment and return 404 if it doesn't match. --- app/controllers/csv_preview_controller.rb | 2 ++ spec/requests/csv_preview_spec.rb | 13 +++++++++++++ spec/support/asset_manager_helpers.rb | 6 +++--- 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/app/controllers/csv_preview_controller.rb b/app/controllers/csv_preview_controller.rb index 953cf78208..eca3d59680 100644 --- a/app/controllers/csv_preview_controller.rb +++ b/app/controllers/csv_preview_controller.rb @@ -23,6 +23,8 @@ def show redirect_to(parent_document_uri, status: :see_other, allow_other_host: true) and return end + return cacheable_404 if @attachment_metadata["content_type"] != "text/csv" + @csv_rows, @truncated = CsvPreviewService .new(GdsApi.asset_manager.media(params[:id], params[:filename]).body) .csv_rows diff --git a/spec/requests/csv_preview_spec.rb b/spec/requests/csv_preview_spec.rb index cfef27a57f..bd8b544724 100644 --- a/spec/requests/csv_preview_spec.rb +++ b/spec/requests/csv_preview_spec.rb @@ -22,6 +22,19 @@ end end + context "when the file type of the attachment is not text/csv" do + before do + setup_asset_manager(parent_document_url, asset_manager_id, asset_manager_filename) + setup_content_item(path_from_filename(asset_manager_filename), parent_document_base_path, content_type: "application/pdf") + end + + it "redirects to parent" do + get "/#{path_from_filename(asset_manager_filename)}/preview" + + expect(response).to have_http_status(:not_found) + end + end + def path_from_filename(base_name) "media/#{asset_manager_id}/#{base_name}.csv" end diff --git a/spec/support/asset_manager_helpers.rb b/spec/support/asset_manager_helpers.rb index 077e7598c9..54f4cd45a9 100644 --- a/spec/support/asset_manager_helpers.rb +++ b/spec/support/asset_manager_helpers.rb @@ -16,7 +16,7 @@ def setup_asset_manager(parent_document_url, asset_manager_id, filename = nil, m stub_request(:get, "#{Plek.find('asset-manager')}/media/#{asset_manager_id}/#{filename}.csv").to_return(body: csv_file, status: media_code) end - def setup_content_item(non_legacy_url_path, parent_document_base_path) + def setup_content_item(non_legacy_url_path, parent_document_base_path, content_type: "text/csv") filename = non_legacy_url_path.split("/").last content_item = { base_path: parent_document_base_path, @@ -24,8 +24,8 @@ def setup_content_item(non_legacy_url_path, parent_document_base_path) public_updated_at: "2023-05-27T08:00:07.000+00:00", details: { attachments: [ - { title: "Attachment 1", filename: "file.csv", url: "https://www.gov.uk/media/5678/file.csv", file_size: "1024" }, - { title: "Attachment 2", filename:, url: "https://www.gov.uk/#{non_legacy_url_path}", file_size: "2048" }, + { title: "Attachment 1", content_type: "text/csv", filename: "file.csv", url: "https://www.gov.uk/media/5678/file.csv", file_size: "1024" }, + { title: "Attachment 2", content_type:, filename:, url: "https://www.gov.uk/#{non_legacy_url_path}", file_size: "2048" }, ], }, links: {