From a995004d7dbf9a44de285db657f97d2937a891a1 Mon Sep 17 00:00:00 2001 From: Bernardo Anderson Date: Mon, 31 Aug 2026 15:44:11 -0500 Subject: [PATCH] CP-15418 - Fix completed Docuseal documents failing to open when the file name has spaces What Completed Docuseal documents now open correctly even when the original file name contains spaces, parentheses, or other special characters. If a secure document link still cannot be generated, users now see a friendly error message instead of a raw "Access denied" page, and the failure is reported to error tracking. Why Customers could not view completed employee forms (for example "F-O-C-880 CLIENT CONFIDENTIALITY.docx (1)") from the employee Documents tab. The signed link was built from the un-encoded file name, so the browser's version of the URL no longer matched the signature and access was denied. The files were never lost; the link was just invalid. How to test Open an employee's Documents tab and click a completed Docuseal form whose file name contains spaces or parentheses. The PDF should open. Also confirm a document with a plain file name (no spaces) still opens as before. --- .../api/signed_document_urls_controller.rb | 11 +++ .../submissions_download_controller.rb | 5 ++ .../submit_form_download_controller.rb | 5 ++ app/services/document_security_service.rb | 42 ++++++---- config/initializers/secure_attachment.rb | 15 ++++ spec/requests/signed_document_urls_spec.rb | 23 ++++++ .../document_security_service_spec.rb | 82 ++++++++++++++----- 7 files changed, 149 insertions(+), 34 deletions(-) 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