diff --git a/app/javascript/elements/download_button.js b/app/javascript/elements/download_button.js index 160fc770..da8ed00b 100644 --- a/app/javascript/elements/download_button.js +++ b/app/javascript/elements/download_button.js @@ -42,7 +42,7 @@ export default targetable(class extends HTMLElement { const link = document.createElement('a') link.href = blobUrl - link.setAttribute('download', decodeURI(url.split('/').pop())) + link.setAttribute('download', decodeURI((url.split('?')[0] || url).split('/').pop())) link.click() @@ -67,7 +67,7 @@ export default targetable(class extends HTMLElement { const link = document.createElement('a') link.href = blobUrl - link.setAttribute('download', decodeURI(url.split('/').pop())) + link.setAttribute('download', decodeURI((url.split('?')[0] || url).split('/').pop())) return link }) diff --git a/app/javascript/submission_form/completed.vue b/app/javascript/submission_form/completed.vue index c33fd570..fd19650c 100644 --- a/app/javascript/submission_form/completed.vue +++ b/app/javascript/submission_form/completed.vue @@ -195,7 +195,7 @@ export default { const link = document.createElement('a') link.href = blobUrl - link.setAttribute('download', decodeURI(url.split('/').pop())) + link.setAttribute('download', decodeURI((url.split('?')[0] || url).split('/').pop())) link.click() @@ -224,7 +224,7 @@ export default { const link = document.createElement('a') link.href = blobUrl - link.setAttribute('download', decodeURI(url.split('/').pop())) + link.setAttribute('download', decodeURI((url.split('?')[0] || url).split('/').pop())) return link }) diff --git a/app/services/document_security_service.rb b/app/services/document_security_service.rb index 5c2170ba..13216588 100644 --- a/app/services/document_security_service.rb +++ b/app/services/document_security_service.rb @@ -41,11 +41,30 @@ class DocumentSecurityService end def build_cloudfront_url(attachment) - # Convert S3 URL to CloudFront URL with DocuSeal prefix - s3_key = attachment.blob.key - # Ensure DocuSeal prefix for document organization - prefixed_key = s3_key.start_with?('docuseal/') ? s3_key : "docuseal/#{s3_key}" - "#{cloudfront_base_url}/#{prefixed_key}" + key = ensure_docuseal_prefix(attachment.blob.key) + base_url = "#{cloudfront_base_url}/#{key}" + query_string = build_query_params(attachment) + + "#{base_url}?#{query_string}" + end + + def ensure_docuseal_prefix(s3_key) + s3_key.start_with?('docuseal/') ? s3_key : "docuseal/#{s3_key}" + end + + def build_query_params(attachment) + filename = attachment.blob.filename.to_s.presence || 'download.pdf' + + { + 'response-content-disposition' => content_disposition_for(filename), + 'response-content-type' => attachment.blob.content_type + }.to_query + end + + def content_disposition_for(filename) + # RFC 6266 with RFC 5987 encoding for international characters + rfc5987_encoded = CGI.escape(filename) + "inline; filename=\"#{filename}\"; filename*=UTF-8''#{rfc5987_encoded}" end def cloudfront_base_url diff --git a/app/views/submissions/show.html.erb b/app/views/submissions/show.html.erb index 66033237..c516ff2a 100644 --- a/app/views/submissions/show.html.erb +++ b/app/views/submissions/show.html.erb @@ -19,41 +19,16 @@ <% end %> <% if last_submitter %> <% if is_all_completed || !is_combined_enabled %> -
- - - <%= svg_icon('download', class: 'w-6 h-6') %> - - - - - <% if is_all_completed && !is_combined_enabled %> - - <% end %> -
+ + + <%= svg_icon('download', class: 'w-6 h-6') %> + + + + <% end %> <% end %> diff --git a/spec/services/document_security_service_spec.rb b/spec/services/document_security_service_spec.rb new file mode 100644 index 00000000..affd56ba --- /dev/null +++ b/spec/services/document_security_service_spec.rb @@ -0,0 +1,246 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe DocumentSecurityService do + let(:account) { create(:account) } + let(:blob) do + ActiveStorage::Blob.create_and_upload!( + io: Rails.root.join('spec/fixtures/sample-document.pdf').open, + filename: 'test-document.pdf', + content_type: 'application/pdf' + ) + end + let(:attachment) do + ActiveStorage::Attachment.create!( + blob: blob, + name: :documents, + record: account + ) + end + + before do + ActiveStorage::Current.url_options = { host: 'test.example.com' } + end + + describe '.signed_url_for' do + context 'when CloudFront is not configured' do + before do + allow(ENV).to receive(:fetch).with('CF_URL', nil).and_return(nil) + allow(ENV).to receive(:fetch).with('CF_KEY_PAIR_ID', nil).and_return(nil) + allow(ENV).to receive(:fetch).with('SECURE_ATTACHMENT_PRIVATE_KEY', nil).and_return(nil) + end + + it 'returns the regular attachment URL' do + result = described_class.signed_url_for(attachment) + expect(result).to eq(attachment.url) + end + end + + context 'when CloudFront is configured' do + let(:cloudfront_url) { 'https://d123456.cloudfront.net' } + let(:key_pair_id) { 'EXAMPLE_KEY' } + let(:private_key) { 'fake-private-key-for-testing' } + + before do + allow(ENV).to receive(:fetch).with('CF_URL', nil).and_return(cloudfront_url) + allow(ENV).to receive(:fetch).with('CF_KEY_PAIR_ID', nil).and_return(key_pair_id) + allow(ENV).to receive(:fetch).with('SECURE_ATTACHMENT_PRIVATE_KEY', nil).and_return(private_key) + end + + after do + # Clear memoized signer between examples + described_class.instance_variable_set(:@cloudfront_signer, nil) + end + + it 'generates a signed CloudFront URL' do + signer = instance_double(Aws::CloudFront::UrlSigner) + allow(Aws::CloudFront::UrlSigner).to receive(:new).and_return(signer) + allow(signer).to receive(:signed_url).and_return('https://signed-url.example.com') + + result = described_class.signed_url_for(attachment) + + expect(result).to eq('https://signed-url.example.com') + end + + it 'includes Content-Disposition header in the URL' do + signer = instance_double(Aws::CloudFront::UrlSigner) + allow(Aws::CloudFront::UrlSigner).to receive(:new).and_return(signer) + allow(signer).to receive(:signed_url).and_return('https://signed-url.example.com') + expected_url_pattern = %r{ + #{Regexp.escape(cloudfront_url)}/docuseal/.* + \?response-content-disposition=.*filename%3D%22test-document\.pdf%22.* + &response-content-type=application%2Fpdf + }x + + described_class.signed_url_for(attachment) + + expect(signer).to have_received(:signed_url) do |url, **_options| + expect(url).to match(expected_url_pattern) + end + end + + it 'properly escapes special characters in filename' do + signer = instance_double(Aws::CloudFront::UrlSigner) + allow(Aws::CloudFront::UrlSigner).to receive(:new).and_return(signer) + allow(signer).to receive(:signed_url).and_return('https://signed-url.example.com') + + special_blob = ActiveStorage::Blob.create_and_upload!( + io: StringIO.new('test'), + filename: 'document with spaces & special.pdf', + content_type: 'application/pdf' + ) + special_attachment = ActiveStorage::Attachment.create!( + blob: special_blob, + name: :documents, + record: account + ) + + described_class.signed_url_for(special_attachment) + + expect(signer).to have_received(:signed_url) do |url, **_options| + expect(url).to include('response-content-disposition=') + expect(url).to include(CGI.escape('document with spaces & special.pdf')) + end + end + + it 'uses default filename when blob filename is empty' do + signer = instance_double(Aws::CloudFront::UrlSigner) + allow(Aws::CloudFront::UrlSigner).to receive(:new).and_return(signer) + allow(signer).to receive(:signed_url).and_return('https://signed-url.example.com') + + empty_blob = ActiveStorage::Blob.create_and_upload!( + io: StringIO.new('test'), + filename: '', + content_type: 'application/pdf' + ) + empty_attachment = ActiveStorage::Attachment.create!( + blob: empty_blob, + name: :documents, + record: account + ) + + described_class.signed_url_for(empty_attachment) + + expect(signer).to have_received(:signed_url) do |url, **_options| + decoded_url = CGI.unescape(url) + expect(decoded_url).to include('filename="download.pdf"') + end + end + + it 'adds docuseal prefix to S3 key if not present' do + signer = instance_double(Aws::CloudFront::UrlSigner) + allow(Aws::CloudFront::UrlSigner).to receive(:new).and_return(signer) + allow(signer).to receive(:signed_url).and_return('https://signed-url.example.com') + + described_class.signed_url_for(attachment) + + expect(signer).to have_received(:signed_url) do |url, **_options| + expect(url).to include('/docuseal/') + end + end + + it 'does not duplicate docuseal prefix if already present' do + signer = instance_double(Aws::CloudFront::UrlSigner) + allow(Aws::CloudFront::UrlSigner).to receive(:new).and_return(signer) + allow(signer).to receive(:signed_url).and_return('https://signed-url.example.com') + allow(blob).to receive(:key).and_return('docuseal/existing-key') + + described_class.signed_url_for(attachment) + + expect(signer).to have_received(:signed_url) do |url, **_options| + expect(url).to match(%r{/docuseal/[^/]}) + expect(url).not_to match(%r{/docuseal/docuseal/}) + end + end + + it 'respects the expires_in parameter' do + signer = instance_double(Aws::CloudFront::UrlSigner) + allow(Aws::CloudFront::UrlSigner).to receive(:new).and_return(signer) + allow(signer).to receive(:signed_url).and_return('https://signed-url.example.com') + expires_time = 2.hours.from_now + + described_class.signed_url_for(attachment, expires_in: 2.hours) + + expect(signer).to have_received(:signed_url) do |_url, **options| + expect(options[:expires]).to be_within(1).of(expires_time.to_i) + end + end + + it 'uses inline disposition in Content-Disposition header' do + signer = instance_double(Aws::CloudFront::UrlSigner) + allow(Aws::CloudFront::UrlSigner).to receive(:new).and_return(signer) + allow(signer).to receive(:signed_url).and_return('https://signed-url.example.com') + + described_class.signed_url_for(attachment) + + expect(signer).to have_received(:signed_url) do |url, **_options| + decoded_url = CGI.unescape(url) + expect(decoded_url).to include('inline; filename="test-document.pdf"') + end + end + + context 'when signing fails' do + it 'logs the error' do + signer = instance_double(Aws::CloudFront::UrlSigner) + allow(Aws::CloudFront::UrlSigner).to receive(:new).and_return(signer) + allow(signer).to receive(:signed_url).and_raise(StandardError.new('Signing failed')) + allow(Rails.logger).to receive(:error) + + described_class.signed_url_for(attachment) + + expect(Rails.logger).to have_received(:error).with(/Failed to generate signed URL: Signing failed/) + end + + it 'falls back to the regular attachment URL' do + signer = instance_double(Aws::CloudFront::UrlSigner) + allow(Aws::CloudFront::UrlSigner).to receive(:new).and_return(signer) + allow(signer).to receive(:signed_url).and_raise(StandardError.new('Signing failed')) + allow(Rails.logger).to receive(:error) + + result = described_class.signed_url_for(attachment) + expect(result).to eq(attachment.url) + end + end + end + + context 'with different content types' do + let(:cloudfront_url) { 'https://d123456.cloudfront.net' } + let(:key_pair_id) { 'EXAMPLE_KEY' } + let(:private_key) { 'fake-private-key-for-testing' } + + before do + allow(ENV).to receive(:fetch).with('CF_URL', nil).and_return(cloudfront_url) + allow(ENV).to receive(:fetch).with('CF_KEY_PAIR_ID', nil).and_return(key_pair_id) + allow(ENV).to receive(:fetch).with('SECURE_ATTACHMENT_PRIVATE_KEY', nil).and_return(private_key) + end + + after do + # Clear memoized signer between examples + described_class.instance_variable_set(:@cloudfront_signer, nil) + end + + it 'includes the correct content type for images' do + signer = instance_double(Aws::CloudFront::UrlSigner) + allow(Aws::CloudFront::UrlSigner).to receive(:new).and_return(signer) + allow(signer).to receive(:signed_url).and_return('https://signed-url.example.com') + image_blob = ActiveStorage::Blob.create_and_upload!( + io: StringIO.new('fake image'), + filename: 'image.jpg', + content_type: 'image/jpeg' + ) + image_attachment = ActiveStorage::Attachment.create!( + blob: image_blob, + name: :documents, + record: account + ) + + described_class.signed_url_for(image_attachment) + + expect(signer).to have_received(:signed_url) do |url, **_options| + expect(url).to include('response-content-type=image%2Fjpeg') + end + end + end + end +end