diff --git a/app/controllers/api/signed_document_urls_controller.rb b/app/controllers/api/signed_document_urls_controller.rb index 5b600fe7be..39626ae4b5 100644 --- a/app/controllers/api/signed_document_urls_controller.rb +++ b/app/controllers/api/signed_document_urls_controller.rb @@ -4,6 +4,8 @@ module Api class SignedDocumentUrlsController < ApiBaseController load_and_authorize_resource :submission + rescue_from DocumentSecurityService::SigningError, with: :render_signing_error + def show last_submitter = @submission.last_completed_submitter @@ -21,5 +23,14 @@ def show documents: SignedDocumentUrlBuilder.new(last_submitter).call } end + + private + + # ATS maps 5xx to Docuseal::DocusealError and shows an "unable to retrieve" + # toast; a signed-URL failure must never leak a broken S3 URL to the browser. + def render_signing_error(exception) + Airbrake.notify(exception) + render json: { error: 'Unable to generate secure document URLs' }, status: :bad_gateway + end end end diff --git a/app/controllers/submissions_download_controller.rb b/app/controllers/submissions_download_controller.rb index 77c2256ac4..f9727fa3e0 100644 --- a/app/controllers/submissions_download_controller.rb +++ b/app/controllers/submissions_download_controller.rb @@ -4,6 +4,11 @@ class SubmissionsDownloadController < ApplicationController skip_before_action :authenticate_via_token! skip_authorization_check + rescue_from DocumentSecurityService::SigningError do |e| + Airbrake.notify(e) + head :bad_gateway + end + TTL = 40.minutes FILES_TTL = 5.minutes diff --git a/app/controllers/submit_form_download_controller.rb b/app/controllers/submit_form_download_controller.rb index 6cba869ab6..b71972c626 100644 --- a/app/controllers/submit_form_download_controller.rb +++ b/app/controllers/submit_form_download_controller.rb @@ -4,6 +4,11 @@ class SubmitFormDownloadController < ApplicationController skip_before_action :authenticate_via_token! skip_authorization_check + rescue_from DocumentSecurityService::SigningError do |e| + Airbrake.notify(e) + head :bad_gateway + end + FILES_TTL = 5.minutes def index diff --git a/app/services/document_security_service.rb b/app/services/document_security_service.rb index 13216588d6..832477d937 100644 --- a/app/services/document_security_service.rb +++ b/app/services/document_security_service.rb @@ -5,24 +5,28 @@ # Service for handling secure document access with CloudFront signed URLs # Reuses same infrastructure and key pairs as ATS class DocumentSecurityService + # Raised when a secure URL cannot be generated. Callers must surface this as + # an error response -- never fall back to attachment.url: a presigned S3 URL + # for the secured bucket is rejected with a raw S3 AccessDenied XML page in + # the user's browser (CP-15418). + class SigningError < StandardError; end + class << self - # Generate signed URL for a secured attachment # @param attachment [ActiveStorage::Attachment] The attachment to generate URL for # @param expires_in [ActiveSupport::Duration] How long the URL should be valid # @return [String] Signed CloudFront URL + # @raise [SigningError] when CloudFront is not configured or signing fails def signed_url_for(attachment, expires_in: 1.hour) - return attachment.url unless cloudfront_configured? - - # Get the CloudFront URL for this attachment - cloudfront_url = build_cloudfront_url(attachment) - - # Generate signed URL using same system as ATS - signer = cloudfront_signer - signer.signed_url(cloudfront_url, expires: expires_in.from_now.to_i) + unless cloudfront_configured? + raise SigningError, + 'CloudFront is not configured (CF_URL/CF_KEY_PAIR_ID/SECURE_ATTACHMENT_PRIVATE_KEY)' + end + + cloudfront_signer.signed_url(build_cloudfront_url(attachment), expires: expires_in.from_now.to_i) + rescue SigningError + raise rescue StandardError => e - Rails.logger.error("Failed to generate signed URL: #{e.message}") - # Fallback to direct URL if signing fails - attachment.url + raise SigningError, "CloudFront signing failed: #{e.class}: #{e.message}" end private @@ -42,10 +46,18 @@ def cloudfront_signer def build_cloudfront_url(attachment) key = ensure_docuseal_prefix(attachment.blob.key) - base_url = "#{cloudfront_base_url}/#{key}" - query_string = build_query_params(attachment) + "#{cloudfront_base_url}/#{encode_path_segments(key)}?#{build_query_params(attachment)}" + end - "#{base_url}?#{query_string}" + # CloudFront validates the signature against the exact URL the client + # requests. Secured blob keys embed the original filename verbatim + # ("docuseal//"), and browsers percent-encode characters + # like spaces before sending, so a signature over the raw key never + # matches the request (CP-15418). Encode each segment individually so + # "/" separators survive; strict RFC 3986 encoding keeps unreserved + # characters literal, so already-safe keys are not double-encoded. + def encode_path_segments(key) + key.split('/').map { |segment| ERB::Util.url_encode(segment) }.join('/') end def ensure_docuseal_prefix(s3_key) diff --git a/config/initializers/secure_attachment.rb b/config/initializers/secure_attachment.rb index 28d0233bc6..5bd36dbfc0 100644 --- a/config/initializers/secure_attachment.rb +++ b/config/initializers/secure_attachment.rb @@ -16,3 +16,18 @@ Rails.logger.error("Failed to load CloudFront private key: #{e.message}") end end + +# Fail loudly (but do not block boot) when secured-storage serving is not +# usable outside of local dev/test. Without these, every aws_s3_secured +# document raises DocumentSecurityService::SigningError, so completed-doc +# downloads from ATS fail. Mirrors careerplug_webhook_config.rb. +unless Rails.env.local? + missing = %w[CF_URL CF_KEY_PAIR_ID SECURE_ATTACHMENT_PRIVATE_KEY].reject { |key| ENV[key].present? } + + unless missing.empty? + message = "CloudFront secured-storage config missing in #{Rails.env}: #{missing.join(', ')}. " \ + 'Signed document URLs for secured storage will fail until this is fixed.' + Rails.logger.error("[secure_attachment] #{message}") + Airbrake.notify(message) + end +end diff --git a/spec/requests/signed_document_urls_spec.rb b/spec/requests/signed_document_urls_spec.rb index add5df09e5..1f11a8b6ce 100644 --- a/spec/requests/signed_document_urls_spec.rb +++ b/spec/requests/signed_document_urls_spec.rb @@ -151,5 +151,28 @@ end.to raise_error(StandardError, 'Generation failed') end end + + context 'when secure URL signing fails' do + let(:submission) { create(:submission, :with_submitters, template:, created_by_user: author) } + let(:builder) { instance_double(SignedDocumentUrlBuilder) } + + before do + submission.submitters.first.update!(completed_at: Time.current) + allow(Submissions::EnsureResultGenerated).to receive(:call) + allow(SignedDocumentUrlBuilder).to receive(:new).with(submission.submitters.first).and_return(builder) + allow(builder).to receive(:call) + .and_raise(DocumentSecurityService::SigningError, 'CloudFront is not configured') + allow(Airbrake).to receive(:notify) + end + + it 'returns 502 with an error message instead of a broken URL' do + get "/api/submissions/#{submission.id}/signed_document_url", + headers: { 'x-auth-token': author.access_token.token } + + expect(response).to have_http_status(:bad_gateway) + expect(response.parsed_body['error']).to eq('Unable to generate secure document URLs') + expect(Airbrake).to have_received(:notify).at_least(:once) + end + end end end diff --git a/spec/services/document_security_service_spec.rb b/spec/services/document_security_service_spec.rb index affd56ba3c..82537c3af3 100644 --- a/spec/services/document_security_service_spec.rb +++ b/spec/services/document_security_service_spec.rb @@ -31,9 +31,9 @@ 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) + it 'raises SigningError instead of returning an unusable fallback URL' do + expect { described_class.signed_url_for(attachment) } + .to raise_error(DocumentSecurityService::SigningError, /CloudFront is not configured/) end end @@ -104,6 +104,62 @@ end end + context 'when the S3 key contains spaces and special characters' do + it 'percent-encodes the path so the signature matches the requested 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') + allow(attachment.blob).to receive(:key) + .and_return('docuseal/f11918fd-3d36-4204-8376-9162f1885e2b/' \ + 'F-O-C-880 CLIENT CONFIDENTIALITY.docx (1).pdf') + + described_class.signed_url_for(attachment) + + expect(signer).to have_received(:signed_url) do |url, **_options| + path = URI.parse(url).path + expect(path).not_to include(' ') + expect(path).to eq('/docuseal/f11918fd-3d36-4204-8376-9162f1885e2b/' \ + 'F-O-C-880%20CLIENT%20CONFIDENTIALITY.docx%20%281%29.pdf') + expect(CGI.unescape(path)) + .to eq('/docuseal/f11918fd-3d36-4204-8376-9162f1885e2b/' \ + 'F-O-C-880 CLIENT CONFIDENTIALITY.docx (1).pdf') + end + end + + it 'encodes percent signs, ampersands, and non-ASCII without double-encoding' 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(attachment.blob).to receive(:key) + .and_return('docuseal/abc123/100% done & más~v2.pdf') + + described_class.signed_url_for(attachment) + + expect(signer).to have_received(:signed_url) do |url, **_options| + path = URI.parse(url).path + expect(path).not_to include(' ') + expect(path).to include('100%25%20done%20%26%20m%C3%A1s~v2.pdf') + expect(path).not_to include('más') + expect(CGI.unescape(path)).to eq('/docuseal/abc123/100% done & más~v2.pdf') + end + end + + it 'does not encode keys that are already URL-safe' 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(attachment.blob).to receive(:key) + .and_return('docuseal/f11918fd-3d36-4204-8376-9162f1885e2b/plain-file.pdf') + + described_class.signed_url_for(attachment) + + expect(signer).to have_received(:signed_url) do |url, **_options| + expect(URI.parse(url).path) + .to eq('/docuseal/f11918fd-3d36-4204-8376-9162f1885e2b/plain-file.pdf') + end + 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) @@ -181,25 +237,13 @@ 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 + it 'raises SigningError and does not fall back to a raw 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) + allow(signer).to receive(:signed_url).and_raise(StandardError, 'Signing failed') - result = described_class.signed_url_for(attachment) - expect(result).to eq(attachment.url) + expect { described_class.signed_url_for(attachment) } + .to raise_error(DocumentSecurityService::SigningError, /CloudFront signing failed.*Signing failed/) end end end