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: Improve FileProxy Handling, set Content-Type #30427

Merged

Conversation

nmagedman
Copy link
Contributor

@nmagedman nmagedman commented Sep 18, 2023

When proxying an asset file from Amazon S3 or Google Storage, we previously failed to copy forward important headers such as

  • Content-Type
  • Content-Length
  • Cache-Control

We also ignored the storage service's HTTP status response, effectively assuming 200, and just blindly passed on the content body. In the case of any errors or redirects, we would interpret that (empty or meaningless) body as the asset itself.

Instead, we now proxy those HTTP headers and treat any non-200 as an error.

Proposed changes (including videos or screenshots)

  • When proxying an asset file from Amazon S3 or Google Storage, forward on several HTTP headers in our response, most notably Content-Type.
  • If the asset file status code is not 200, return an error, since simply piping (forwarding) the body will not properly proxy the file.

Issue(s)

Fixes #18312 by proxying the Content-Type header

Steps to test or reproduce

  • Configure your RC installation to use Amazon S3 as the File Upload Storage Type.

  • Post a message in a chat room, along with an image attachment.

  • View the post with the image attachment.

  • Right-Click on the image and choose "Open Image in New Tab". (Wording is from Chrome. Other browsers will have the same functionality, albeit with different wording.)

  • Instead of an image, the browser will show text similar to:

    ‰PNG
    �
    ���
    IHDR���4���Ö�����¹²ÃÉ��
    

Further comments

  • This problem was first identified in Attached files are rendered as text #18312. The repro instructions in that bug report said to "Click on the image title", which in those days opened the image in its own tab. When the UI changed to make image titles no longer clickable, the issue was closed without addressing the underlying problem. However there are other ways to open an image in its own tab, namely using the browser's native context menu. As it turns out, many of our users frequently use that functionality.
  • The underlying problem is the combination of:
    • failing to provide the browser with a Content-Type header when proxying from S3
    • adding an X-Content-Type-Options: nosniff header (in apps/meteor/app/cors/server/cors.ts)
  • Together the above forces the browser to render the image using the default content type (text/plain)
  • I have not attempted to test this using other File Upload Storage Types, such as Google Storage.

When proxying an asset file from Amazon S3 or Google Storage,
we previously ignored important headers such as

   - Content-Type
   - Content-Length
   - Cache-Control

We also ignored the storage service's HTTP response,
effectively assuming 200, and just blindly passed on
the content body.  In the case of any errors or redirects,
we would interpret that (empty or meaningless) body as
the asset itself.

Instead, we now proxy those HTTP headers and treat
any non-200 as an error.
@changeset-bot
Copy link

changeset-bot bot commented Sep 18, 2023

🦋 Changeset detected

Latest commit: 2f933d7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 30 packages
Name Type
@rocket.chat/meteor Patch
@rocket.chat/core-typings Patch
@rocket.chat/rest-typings Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/gazzodown Patch
@rocket.chat/livechat Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-contexts Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/api-client Patch
@rocket.chat/license Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/ddp-client Patch
@rocket.chat/fuselage-ui-kit Patch
@rocket.chat/models Patch
@rocket.chat/ui-client Patch
@rocket.chat/ui-video-conf Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/web-ui-registration Patch
@rocket.chat/instance-status Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@nmagedman nmagedman changed the title chore: Improve FileProxy Handling fix: Improve FileProxy Handling, set Content-Type Sep 18, 2023
@casalsgh casalsgh requested a review from a team October 5, 2023 16:50
@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

Merging #30427 (2f933d7) into develop (ae71e31) will decrease coverage by 0.16%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #30427      +/-   ##
===========================================
- Coverage    51.29%   51.14%   -0.16%     
===========================================
  Files          811      805       -6     
  Lines        15059    15074      +15     
  Branches      2751     2785      +34     
===========================================
- Hits          7725     7709      -16     
- Misses        6926     6931       +5     
- Partials       408      434      +26     
Flag Coverage Δ
e2e 48.45% <ø> (-0.12%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@dionisio-bot dionisio-bot bot added stat: ready to merge PR tested and approved waiting for merge and removed stat: needs QA labels Oct 13, 2023
@dionisio-bot dionisio-bot bot added stat: ready to merge PR tested and approved waiting for merge and removed stat: ready to merge PR tested and approved waiting for merge labels Oct 13, 2023
@dionisio-bot dionisio-bot bot added stat: ready to merge PR tested and approved waiting for merge and removed stat: ready to merge PR tested and approved waiting for merge labels Oct 13, 2023
@kodiakhq kodiakhq bot merged commit 54d8ad4 into RocketChat:develop Oct 13, 2023
41 checks passed
@scuciatto scuciatto added this to the 6.4.2 milestone Oct 16, 2023
gabriellsh added a commit that referenced this pull request Oct 16, 2023
…/mentionBot

* 'develop' of github.com:RocketChat/Rocket.Chat:
  feat: add tooltip to badge mentions (#30590)
  chore: improve `Tag` a11y link (#30636)
  refactor: Replace `useForm` in favor of RHF on `AppInstallPage` (#30634)
  fix: Improve FileProxy Handling, set Content-Type (#30427)
  refactor: `EditRoomInfo` to typescript (#28318)
  fix: mobile ringing notification missing call id (#30614)
  fix: Some HTTP requests sent by apps don't have their data parsed into JSON (#30560)
  test: More tests for groups kick (#30536)
  fix: Threads breaking after sending messages too fast (#30622)
  chore: Remove text decoration from room tag (#30606)
  i18n: Language update from LingoHub 🤖 on 2023-10-10Z (#30613)
  fix: File attachments have no content when exporting room messages as file (#30596)
  fix: use setImmediate to handle username update (#30500)
  chore: `AnalyticsReports` visual adequacy (#30617)
sampaiodiego added a commit that referenced this pull request Oct 16, 2023
Co-authored-by: Diego Sampaio <8591547+sampaiodiego@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat: QA skipped stat: ready to merge PR tested and approved waiting for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Attached files are rendered as text
4 participants