Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(form-v2): update CSP and image URL computation to use the correct URL #4483

Merged
merged 6 commits into from
Aug 4, 2022

Conversation

justynoh
Copy link
Contributor

@justynoh justynoh commented Aug 3, 2022

Problem

Image and logo uploads in general are not working in both the preview and saved mode. This occurs for two different reasons:

  1. Prior to file upload, we display images and logos from local blob:s. However the browser refuses to load these because blob:s are not allowed in our CSP.
  2. After files are uploaded to S3, image and logo uploads result in 403 Forbidden. They are being uploaded to S3 but the URLs pointing to their location is not being generated correctly in uploadFile.

Closes #4360

Solution

  1. Add blob: to our imgSrc CSP.
  2. Change uploadFile to use the correct source for the uploaded file ID.

See also:
https://stackoverflow.com/questions/28467789/content-security-policy-object-src-blob
https://csplite.com/csp105/#:~:text=The%20Content%20Security%20Policy%20controls,from%20the%20blob%3A-URI.
https://content-security-policy.com/#source_list
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/script-src

Breaking Changes

  • No - this PR is backwards compatible

Screenshots

See comment below :)

@justynoh justynoh changed the title [WIP] fix(form-v2): update CSP as well as image URL computation to use the correct URL [WIP] fix(form-v2): update CSP and image URL computation to use the correct URL Aug 3, 2022
@justynoh
Copy link
Contributor Author

justynoh commented Aug 3, 2022

As is, everything works as intended. However, the browser console does complain about the blob: image being attempted to load as a script. But the obvious fix for this (add blob: to scriptSrc) seems... more dangerous than adding to imgSrc. Should we keep it as is and let the console complain if it wants?

I don't seem to be able to find any reference to src="blob: anywhere other than the <img> in the builder content itself, so why it wants to test script-src is a bit puzzling.

Tagging @timotheeg for thoughts!

Screen.Recording.2022-08-03.at.3.53.05.PM.mov

@justynoh justynoh marked this pull request as ready for review August 3, 2022 07:59
@justynoh justynoh changed the title [WIP] fix(form-v2): update CSP and image URL computation to use the correct URL fix(form-v2): update CSP and image URL computation to use the correct URL Aug 3, 2022
@justynoh justynoh linked an issue Aug 3, 2022 that may be closed by this pull request
Copy link
Contributor

@timotheeg timotheeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also unclear to me why blob shows up in a script-src warning. If we cannot find it in source, let's do some markup inspection. Call me and we can pair on it!

@@ -117,7 +117,7 @@ const uploadFile = async ({
// POST generated formData to presigned url.
const response = await postToPresignedUrl(postData.url, formData)

const encodedFileId = encodeURIComponent(fileId)
const encodedFileId = encodeURIComponent(postData.fields.key)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

encodedFileId is concatenated straight into a url below, so just to clarify, what format do we expect postData.fields.key to have? I'm asking to because we have to be careful that meaningful url characters (e.g. /) are not encoded.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fileId is the input file ID, but the controller itself returns a new file ID because we randomized the fileID in the controller here, which instituted a change to the file handler in angular and we need need to replicate this behavior in react

@justynoh justynoh requested a review from timotheeg August 4, 2022 09:10
@justynoh justynoh merged commit de4becc into form-v2/develop Aug 4, 2022
@justynoh justynoh deleted the form-v2/fix/image-loading branch August 4, 2022 10:13
@justynoh justynoh mentioned this pull request Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image field preview not working
2 participants