diff --git a/.github/workflows/cucumber.yml b/.github/workflows/cucumber.yml index 71c80361109..b0a5de937ea 100644 --- a/.github/workflows/cucumber.yml +++ b/.github/workflows/cucumber.yml @@ -17,12 +17,10 @@ jobs: - name: Setup Redis uses: alphagov/govuk-infrastructure/.github/actions/setup-redis@main - - name: Install additional system dependencies - run: | - sudo apt-get update - sudo apt-get install -y --no-install-recommends ghostscript - name: Checkout repository uses: actions/checkout@v4 + with: + show-progress: false - name: Setup Ruby uses: ruby/setup-ruby@v1 diff --git a/.github/workflows/minitest.yml b/.github/workflows/minitest.yml index 0c90e0aba16..27bb6b85435 100644 --- a/.github/workflows/minitest.yml +++ b/.github/workflows/minitest.yml @@ -29,16 +29,12 @@ jobs: - name: Setup Redis uses: alphagov/govuk-infrastructure/.github/actions/setup-redis@main - - name: Install additional system dependencies - run: | - sudo apt-get update - sudo apt-get install -y --no-install-recommends ghostscript - - name: Checkout repository uses: actions/checkout@v4 with: repository: alphagov/whitehall ref: ${{ inputs.ref || github.ref }} + show-progress: false - name: Checkout Publishing API (for Content Schemas) uses: actions/checkout@v4 @@ -46,6 +42,7 @@ jobs: repository: alphagov/publishing-api ref: ${{ inputs.publishingApiRef }} path: vendor/publishing-api + show-progress: false - name: Setup Ruby uses: ruby/setup-ruby@v1 @@ -80,4 +77,3 @@ jobs: runs-on: ubuntu-latest steps: - run: echo "All MiniTest tests have passed 🚀" - diff --git a/Dockerfile b/Dockerfile index 61a43eb8aaa..3ffc97291d1 100644 --- a/Dockerfile +++ b/Dockerfile @@ -18,7 +18,7 @@ RUN rails assets:precompile && rm -fr log FROM --platform=$TARGETPLATFORM $base_image -RUN install_packages ghostscript imagemagick unzip zip +RUN install_packages imagemagick unzip zip ENV GOVUK_APP_NAME=whitehall diff --git a/app/helpers/attachments_helper.rb b/app/helpers/attachments_helper.rb index e6ba70f54c1..1cdf39d06c5 100644 --- a/app/helpers/attachments_helper.rb +++ b/app/helpers/attachments_helper.rb @@ -42,7 +42,7 @@ def attachment_component_params(attachment, alternative_format_contact_email: ni params[:file_size] = attachment.file_size end - # PDF attachments get a thumbnail + # PDF attachments used to have a thumbnail, now just a generic icon. if attachment.pdf? params[:thumbnail_url] = attachment.file.thumbnail.url params[:number_of_pages] = attachment.number_of_pages diff --git a/app/uploaders/attachment_uploader.rb b/app/uploaders/attachment_uploader.rb index faf0122a870..42542881f77 100644 --- a/app/uploaders/attachment_uploader.rb +++ b/app/uploaders/attachment_uploader.rb @@ -39,44 +39,13 @@ def set_correct_content_type(_ignore_argument) end def generate_thumbnail - get_first_page_as_png(105, 140) + FileUtils.cp(FALLBACK_PDF_THUMBNAIL, path) end def pdf?(file) file.content_type == PDF_CONTENT_TYPE end - def get_first_page_as_png(width, height) - output, exit_status = Timeout.timeout(THUMBNAIL_GENERATION_TIMEOUT) do - [ - `#{pdf_thumbnail_command(width, height)}`, - $CHILD_STATUS.exitstatus, - ] - end - - unless exit_status.zero? - Rails.logger.warn "Error thumbnailing PDF. Exit status: #{exit_status}; Output: #{output}" - use_fallback_pdf_thumbnail - end - rescue Timeout::Error => e - message = "PDF thumbnail generation took longer than #{THUMBNAIL_GENERATION_TIMEOUT} seconds. Using fallback pdf thumbnail: #{FALLBACK_PDF_THUMBNAIL}." - Rails.logger.warn message - - GovukError.notify( - e, - extra: { - error_message: message, - path: relative_path, - }, - ) - - use_fallback_pdf_thumbnail - end - - def pdf_thumbnail_command(width, height) - %(gs -o #{path} -sDEVICE=pngalpha -dLastPage=1 -r72 -dDEVICEWIDTHPOINTS=#{width} -dDEVICEHEIGHTPOINTS=#{height} -dPDFFitPage -dUseCropBox #{path} 2>&1) - end - def extension_allowlist EXTENSION_ALLOW_LIST end @@ -240,10 +209,4 @@ def validate_zipfile_contents!(new_file) problem = examiners.detect { |examiner| !examiner.valid? } raise CarrierWave::IntegrityError, problem.failure_message if problem end - -private - - def use_fallback_pdf_thumbnail - FileUtils.cp(FALLBACK_PDF_THUMBNAIL, path) - end end diff --git a/config/brakeman.ignore b/config/brakeman.ignore index 31fda8b17a9..cdf9f9f1bc2 100644 --- a/config/brakeman.ignore +++ b/config/brakeman.ignore @@ -168,29 +168,6 @@ ], "note": "" }, - { - "warning_type": "Command Injection", - "warning_code": 14, - "fingerprint": "82dea23cb472eca9cd5c4217767e59cab37e956a57572095a7f225d47aff2db8", - "check_name": "Execute", - "message": "Possible command injection", - "file": "app/uploaders/attachment_uploader.rb", - "line": 54, - "link": "https://brakemanscanner.org/docs/warning_types/command_injection/", - "code": "`#{pdf_thumbnail_command(width, height)}`", - "render_path": null, - "location": { - "type": "method", - "class": "AttachmentUploader", - "method": "get_first_page_as_png" - }, - "user_input": "pdf_thumbnail_command(width, height)", - "confidence": "Medium", - "cwe_id": [ - 77 - ], - "note": "" - }, { "warning_type": "Command Injection", "warning_code": 14, diff --git a/docs/assets.md b/docs/assets.md index 10a8222e7bd..efd35f74493 100644 --- a/docs/assets.md +++ b/docs/assets.md @@ -134,11 +134,6 @@ that manage the successes and failures of the file uploads. > Whitehall creates up to 7 image sizes of each uploaded image. Urls to these images are published as part of the content > so that rendering applications can then show the best image size for the use case. -#### *Members of the public* want to see a thumbnail of pdfs where an embedded link to the pdf is provided - -> Whitehall generates thumbnails for pdf file attachments. Urls to both the pdf and its thumbnail are -> published as part of the content and then used by the rendering applications. - #### *Members of the public* should see document content with working image and attachment urls > For *Editionable Content* types, such as **News Articles** or **Publication** - Publishing is prevented for documents diff --git a/test/unit/app/uploaders/attachment_uploader_test.rb b/test/unit/app/uploaders/attachment_uploader_test.rb index b37509f76f7..56a258e68c4 100644 --- a/test/unit/app/uploaders/attachment_uploader_test.rb +++ b/test/unit/app/uploaders/attachment_uploader_test.rb @@ -227,25 +227,7 @@ class AttachmentUploaderPDFTest < ActiveSupport::TestCase AssetManagerCreateAssetWorker.drain end - test "should scale the thumbnail down proportionally to A4" do - AttachmentData.create!(file: file_fixture("two-pages-with-content.pdf"), attachable: @edition) - - expect_thumbnail_sent_to_asset_manager_to_be_scaled_proportionally - - AssetManagerCreateAssetWorker.drain - end - - test "should use a generic thumbnail if conversion fails" do - AttachmentUploader.any_instance.stubs(:pdf_thumbnail_command).returns("false") - AttachmentData.create!(file: file_fixture("two-pages-with-content.pdf"), attachable: @edition) - - expect_fallback_thumbnail_to_be_uploaded_to_asset_manager - - AssetManagerCreateAssetWorker.drain - end - - test "should use a generic thumbnail if conversion takes longer than 10 seconds to complete" do - AttachmentUploader.any_instance.stubs(:pdf_thumbnail_command).raises(Timeout::Error) + test "should always use the generic thumbnail for PDFs" do AttachmentData.create!(file: file_fixture("two-pages-with-content.pdf"), attachable: @edition) expect_fallback_thumbnail_to_be_uploaded_to_asset_manager @@ -274,20 +256,6 @@ def expect_thumbnail_sent_to_asset_manager_to_be_an_actual_png end }.returns("id" => "http://asset-manager/assets/some-id", "name" => "pub-cover.png") end - - def expect_thumbnail_sent_to_asset_manager_to_be_scaled_proportionally - Services.asset_manager.stubs(:create_asset).returns("id" => "http://asset-manager/assets/some-id", "name" => "pub-cover.png") - Services.asset_manager.expects(:create_asset).with { |value| - if value[:file].path.ends_with?(".png") - identify_details = `identify "#{Rails.root.join("public", value[:file].path)}"` - - _path, _type, geometry, _rest = identify_details.split - width, height = geometry.split("x") - - assert (width == "105" || height == "140"), "geometry should be proportional scaled, but was #{geometry}" - end - }.returns("id" => "http://asset-manager/assets/some-id", "name" => "pub-cover.png") - end end class AttachmentUploaderZipFileTest < ActiveSupport::TestCase