LG-3697: Define connect-src S3 presigned URLs by full path

[aduth]

Why: So that the browser will allow connect to upload images to S3.

The work in #4409 assumes that a wildcard * can be used in the path fragment of a connect-src source list, which is not correct (see specification). Thus, connection attempts would still be rejected.

This largely reverts the prior effort, instead appending to the CSP header in the view itself. This may not be ideal, though it has a few upsides:

• Closer association to where the CSP URLs are relevant (as opposed to application-wide configuration)
• There is prior art

Alternatives explored:

• Append at point in code where URLs are assigned into view variables. This felt like an unexpected side-effect of what should be expected to be a pure function.
• Some other pre-render hook of the document capture step. Unfortunately most existing behavior of a step appears to be tailored for a step's submission, not its initial rendering.

Possible follow-on: Ideally the feature tests added in #4407 could exercise these headers. Unfortunately, it's made challenging by the fact that our fake endpoints are served by the local server, which is allowed by default in the CSP headers. This would also be contingent upon LG-3785.

[aduth]

Actually, as soon as I opened this, I took an even closer look at the spec to discover that we could just omit the * and it would work just as well to allow subpaths:

An ASCII string (path A) path-part matches another ASCII string (path B) if a CSP source expression that contained the first as a path-part could potentially match a URL containing the latter as a path. For example, we say that "/subdirectory/" path-part matches "/subdirectory/file".

It's a much smaller fix (literally just removing the * from this line of code), but I've actually come to prefer the revised approach.

Open to either though!

[zachmargolis]

I like the approach of only adding the S3 URLs to the pages that we know need it. Instead of doing it inside the view, we could try to do it inside the controller? Like the SecureHeadersConcern but for connect-src instead?

[aduth]

I like the approach of only adding the S3 URLs to the pages that we know need it. Instead of doing it inside the view, we could try to do it inside the controller? Like the SecureHeadersConcern but for connect-src instead?

The closest thing we have to controllers for these views are the DocAuthController and CaptureDocController that encompass the entire flow. There's prior art for doing this exact sort of CSP manipulation, but:

• It still seems preferable to have the code live with the step, not the flow
• We have to duplicate this code between the DocAuthController and CaptureDocController controllers
• There's not a clear way to share the URLs between the view variables and the controller

If we went this route, I think we'd want to return to an approach like what we had with the bucket-wide permission:

# app/controllers/idv/doc_auth_controller.rb
# app/controllers/idv/capture_doc_controller.rb

before_action :append_connect_src_to_capture_step

def append_connect_src_to_capture_step
  return unless params[:step] == 'document_capture'
  return unless AppConfig.env.doc_auth_enable_presigned_s3_urls == 'true'
  image_upload_bucket_url = ImageUploadPresignedUrlGenerator.new.bucket_url
  return unless image_upload_bucket_url
  SecureHeaders.append_content_security_policy_directives request, connect_src: [image_upload_bucket_url]
end

Alternatively, the extra_view_variables method where the URLs are currently assigned into the view doesn't currently see much use outside this step, so I could imagine an option where we refactor this to be more tailored as a "prepare the view", where side-effects both of injecting view variables and of appending associated headers might be appropriate. Something like a prepare_step_render to complement or replace extra_view_variables at FlowStateMachine#render_step. I've encountered at least one situation in the past where this sort of thing would be helpful (#4165), so it could see some reuse.

Any preference? I'm leaning toward the latter option, though the precise implementation isn't quite as clear to me as in the code snippet above.

[zachmargolis]

LGTM. Looking at this again, it's the simplest change, re-uses the same mixin methods for URLs we already have. The whole "flows/steps are not quite a controller" thing bugs me but that is a battle for a different day