CP-15418 - Fix completed Docuseal documents failing to open when the file name has spaces

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

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
 

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

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/<uuid>/<filename>"), 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)

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

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

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