Skip to content

[FIX] add image format validation - #25912

Merged
kodiakhq[bot] merged 11 commits into
developfrom
heic
Aug 18, 2022
Merged

kodiakhq[bot] merged 11 commits into
developfrom
heic

Conversation

@filipemarins

Copy link
Copy Markdown
Contributor

Proposed changes (including videos or screenshots)

Issue(s)

Steps to test or reproduce

Further comments

Add validation to prevent the 'heic' format to be uploaded on the room and user's avatar.
This format is not supported on the browsers and meteor.

Before:
Screen Shot 2022-06-17 at 14 13 49
Screen Shot 2022-06-17 at 14 14 13

After:
Screen Shot 2022-06-17 at 14 15 06
Screen Shot 2022-06-17 at 14 16 01

@guijun13 guijun13 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As window.Buffer is not a web API, it`s not recommended to create a polyfill. If you are intend to use the window.buffer for the filetype, take a look at https://www.npmjs.com/package/file-type#browser , as using fileTypeFromStream, FileTypeFromBuffer or FileTypeFromBlob.

tassoevan
tassoevan previously approved these changes Jun 22, 2022
ggazzo
ggazzo previously requested changes Jun 23, 2022

@ggazzo ggazzo left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I still thinks this is something that the sever should handle @tassoevan wdyt?

@filipemarins

filipemarins commented Jun 23, 2022 •

Copy link
Copy Markdown
Contributor Author

I still thinks this is something that the sever should handle @tassoevan wdyt?

I believe this is work for both sides because the browser will try to render this image and we will get an error, for now, we don't handle this kind of error and the user doesn't have proper feedback on what is happening. And make a request to server just to validate the format and return feedback to the client will be more expensive than handling it on the front.

guijun13
guijun13 previously approved these changes Jun 29, 2022
@ggazzo ggazzo added stat: ready to merge PR tested and approved waiting for merge and removed stat: needs QA labels Jun 29, 2022
@filipemarins
filipemarins dismissed ggazzo’s stale review June 30, 2022 16:48

A task was created to add validation on backend

@alvaropmello alvaropmello added this to the 5.0.1 milestone Jul 21, 2022
guijun13
guijun13 previously approved these changes Jul 26, 2022
@casalsgh casalsgh modified the milestones: 5.0.1, 5.0.2 Aug 1, 2022
@kodiakhq

kodiakhq Bot commented Aug 12, 2022

Copy link
Copy Markdown
Contributor

This PR currently has a merge conflict. Please resolve this and then re-add the ['stat: ready to merge', 'automerge'] label.

@kodiakhq kodiakhq Bot removed the stat: ready to merge PR tested and approved waiting for merge label Aug 12, 2022
@ggazzo ggazzo added the stat: ready to merge PR tested and approved waiting for merge label Aug 16, 2022
@codecov

codecov Bot commented Aug 16, 2022 •

Copy link
Copy Markdown

Codecov Report

Merging #25912 (b99e8aa) into develop (fe26bce) will increase coverage by 0.12%.
The diff coverage is 94.44%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #25912      +/-   ##
===========================================
+ Coverage    38.54%   38.66%   +0.12%     
===========================================
  Files          758      759       +1     
  Lines        18827    18809      -18     
  Branches      1453     1453              
===========================================
+ Hits          7256     7273      +17     
+ Misses       11355    11318      -37     
- Partials       216      218       +2     
Flag Coverage Δ
e2e 38.66% <94.44%> (+0.12%) ⬆️

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

@kodiakhq
kodiakhq Bot merged commit d2f3e1f into develop Aug 18, 2022
@kodiakhq
kodiakhq Bot deleted the heic branch August 18, 2022 16:11
@alvaropmello alvaropmello added this to the 5.1.0 milestone Aug 18, 2022
csuadev pushed a commit that referenced this pull request Aug 26, 2022
Co-authored-by: gabriellsh <40830821+gabriellsh@users.noreply.github.com>
@murtaza98 murtaza98 mentioned this pull request Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stat: ready to merge PR tested and approved waiting for merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants