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

Images aren't displaying in TinyMCE #2348

Closed
2 tasks
pphillips-fearless opened this issue Jul 15, 2020 · 6 comments · Fixed by #2683
Closed
2 tasks

Images aren't displaying in TinyMCE #2348

pphillips-fearless opened this issue Jul 15, 2020 · 6 comments · Fixed by #2683
Assignees
Labels
bug needs triage, then squashing Development Issues for the dev team resolve large sev1 Experience cannot be completed or is significantly compromised.

Comments

@pphillips-fearless
Copy link
Contributor

pphillips-fearless commented Jul 15, 2020

There are two things happening with the images. One I haven't fully investigated yet and one I have and could be problematic to fix. Either way, it's a bug and is probably already affecting production.

The URL that TinyMCE is inserting into the image href attribute is incorrect. For preview deployments it should be /api/apds/.../, but instead it's https://api//apds/.... I assume it's an issue with the URL being relative and TinyMCE is "helping" by adding a protocol and interpreting it as a full path. This only affects preview. The image URL is built on the client side by combining the relative image URL returned from the API with the full API URL. In staging and production, the API URL is absolute, so combining the two creates a new absolute URL. In preview deployments, the API URL is also relative, so the combined URL remains relative. Not clear to me offhand if there is a fix, but if it only affects preview deployments, it seems like a relatively low priority.

The API protects images with authorization just like everything else. A user can only view an image if it is associated with an APD that they have access to. The issue is that we now require authentication to happen via the Authorization header, but there's no (immediate) way to set that header on tags, which is what the TinyMCE editor is using. This worked previously because we were using cookies for authentication, and those got passed automatically by the browser – no need to set anything. There aren't a lot of good options here, but some options do exist:

My current strong lean would be towards something like a Service Worker to intercept all calls to the API. That service worker could then inject the authorization header (see this StackOverflow answer for an example of exactly how to do it without losing anything else that may have been set on the request) on the fly. We would also remove the code that Richard added to the API wrapper since it'd be incorporated by the service worker. The downside? Service workers don't exist in IE, full stop. They work in all the modern browsers, but not IE. So if we went the service worker route, images could be uploaded from IE, but they couldn't be viewed from IE.

Alternatively, we can make images publicly accessible. My instinct is that, while the images are probably mostly safe for public exposure, some won't be. Even among those that are safe, I can imagine that states (and likely CMS security folks) would be squeamish about making everything wide open like that. That said, this is an easy, universal solution. It'd just take commenting out a couple of lines of code in the API.

Rewrite the contents of the RichText field on-the-fly to append the user's token to any image hrefs found in the content. This would also require tweaking the API to look for authentication tokens in the query string along with the headers. It's also not my favorite approach because it seems fragile. It might have cacheing implications, too, since caches sometimes ignore query strings, so a malicious actor could potentially retrieve an image from an intermediate cache even though the associated token had expired.

Hopefully there's another method that's also universal, but in my brief dive, I couldn't find anything. In any case, there should probably be a bug issue related to these images. This PR didn't introduce the problem, though. The bug came about when we migrated to the Authorization header, and we just plain didn't catch it. I did not even think about tags when Richard did all that work, and I imagine he didn't either. (It was 100% the right work to do, though.) What an unexpected edge case...

Because we're switching to OKTA for authentication, this may all change again once that is done, so this is a placeholder task that we probably want to wait to deal with until then

This task is done when...

  • criteria 1
  • criteria 2

*Add a LABEL (design, dev, compliance, BUG, etc) before submitting.

*If the issue is needed to complete prioritized work for the CURRENT SPRINT, add it to the "This Sprint" pipeline. Otherwise, all other issues will be automatically added to the unprioritized pipeline for PRODUCT to prioritize.

@pphillips-fearless pphillips-fearless added Development Issues for the dev team resolve large labels Jul 15, 2020
@nicholeweems nicholeweems added the bug needs triage, then squashing label Jul 21, 2020
@jeromeleecms jeromeleecms added the sev1 Experience cannot be completed or is significantly compromised. label Jul 21, 2020
@nicholeweems nicholeweems added this to the 11/30/2020 - 12/11/2020 milestone Nov 20, 2020
@radavis
Copy link
Contributor

radavis commented Dec 2, 2020

Summary: A request for images attached to an APD yields a 401 response. This is because the Authorization: Bearer ${JWT} header is not provided when requesting images.

@radavis
Copy link
Contributor

radavis commented Dec 2, 2020

Suggested fix: Allow requests for files/images to pass-through. If a client is requesting images attached to an APD, this means they can access the APD.

S3 does a decent job of obfuscating filenames. For example: https://staging-eapd-api.cms.gov/apds/5/files/cb304e666f4c08bcbe1586af8addbd298aa2dcf143934edadaddcbc4f5fb56c4. No one is going to guess a fileId.

It would be possible for an authorized user to share an image URL with an unauthorized user. The authorized user could just as easily download the file and email it to an unauthorized user.

An alternative solution would be to hijack requests on the front-end using a service worker, and add the Authorization: Bearer ${JWT} header to requests for files.

@thetif
Copy link
Contributor

thetif commented Dec 2, 2020

I think it would be better to try to intercept it like we do for axios. If that proves to be too hard we can lower the requirements on retrieving an image.

@jeromeleecms
Copy link
Contributor

Not sure if this would be related, but at some point states will need to be able to attach other documents (contracts/amendments/RFPs) and those need to be secured. Just something to consider as you are thinking through solutions on file storage.

@beparticular
Copy link

FWIW, I don't think we'll be uploading any documents through the RTE if that has any bearing on this.

@radavis
Copy link
Contributor

radavis commented Dec 2, 2020

I will approach this bug in two-parts: One PR allowing requests for attached files to pass-through, and another that hijacks requests on the front-end and adds the necessary header.

Note that an S3 fileId is 64 characters, composed of numbers 0-9 and letters a-f (16 choices per character). This equates to a fileId space of 16^64 possibilities, or 115 quantuorvigintillion. If someone made one request per second, they would get a hit in 2^(log2(16^64)) seconds, or 3.7x10^69 years...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug needs triage, then squashing Development Issues for the dev team resolve large sev1 Experience cannot be completed or is significantly compromised.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants