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

Remove Ghostscript and PDF thumb generation. #9163

Merged
merged 1 commit into from
Jun 17, 2024
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
6 changes: 2 additions & 4 deletions .github/workflows/cucumber.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 2 additions & 6 deletions .github/workflows/minitest.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,23 +29,20 @@ 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
with:
repository: alphagov/publishing-api
ref: ${{ inputs.publishingApiRef }}
path: vendor/publishing-api
show-progress: false

- name: Setup Ruby
uses: ruby/setup-ruby@v1
Expand Down Expand Up @@ -80,4 +77,3 @@ jobs:
runs-on: ubuntu-latest
steps:
- run: echo "All MiniTest tests have passed 🚀"

2 changes: 1 addition & 1 deletion Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion app/helpers/attachments_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
39 changes: 1 addition & 38 deletions app/uploaders/attachment_uploader.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
23 changes: 0 additions & 23 deletions config/brakeman.ignore
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
5 changes: 0 additions & 5 deletions docs/assets.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
34 changes: 1 addition & 33 deletions test/unit/app/uploaders/attachment_uploader_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down