Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Setting to strip EXIF metadata from JPEG uploads #1307

Closed
wants to merge 19 commits into from

Conversation

t3chguy
Copy link
Member

@t3chguy t3chguy commented Aug 15, 2017

This strips EXIF metadata from uploaded JPEGs by default, thus avoiding privacy sensitive metadata such as GPS and timestamps being leaked over Matrix.

Fixes element-hq/element-web#4426

@t3chguy
Copy link
Member Author

t3chguy commented Aug 15, 2017

Leads to broken thumbnails: (but working and stripped EXIFs)
image

{
  "origin_server_ts": 1502833036623,
  "sender": "@abc123abc:matrix.org",
  "event_id": "$15028330362519985ISVKc:matrix.org",
  "unsigned": {
    "age": 7299
  },
  "content": {
    "body": "12382975864_09e6e069e7_o.jpg",
    "info": {
      "mimetype": "image/jpeg",
      "thumbnail_info": {
        "mimetype": "image/jpeg",
        "h": 600,
        "w": 800,
        "size": 205399
      },
      "h": 2976,
      "thumbnail_url": "mxc://matrix.org/nNjRtwWgHjjLCvXCKuubQAfx",
      "w": 3968,
      "size": 6327505
    },
    "msgtype": "m.image",
    "url": "mxc://matrix.org/KPImtwzFwwJDXVSthmbyedRh"
  },
  "type": "m.room.message",
  "room_id": "!UInSXSErRtlgialnsk:riot.ovh"
}

@t3chguy
Copy link
Member Author

t3chguy commented Aug 21, 2017

/me is unsure what is going on :/

@ara4n ara4n added the blocked label Sep 17, 2017
@lukebarnard1
Copy link
Contributor

@ara4n any update on this?

@ara4n
Copy link
Member

ara4n commented Jun 25, 2018

not from me; it didn't work for @t3chguy and we haven't yet debugged it.

@jryans
Copy link
Collaborator

jryans commented Mar 20, 2019

Thanks for making this contribution a while back. Since the code base has changed since this was opened, it no longer applies cleanly, and I don't think there's a need to keep it open in this state, as we can always find the code here again if needed.

It looks like we should probably file an issue (if there isn't one already) to track the problem being solved here.

@ara4n
Copy link
Member

ara4n commented Mar 20, 2019

element-hq/element-web#4426 is the bug.

@ara4n ara4n reopened this May 10, 2020
@ara4n ara4n marked this pull request as draft May 10, 2020 13:02
@ara4n ara4n marked this pull request as ready for review May 10, 2020 14:35
@ara4n ara4n marked this pull request as draft May 10, 2020 14:45
@ara4n
Copy link
Member

ara4n commented May 10, 2020

So the reason why the thumbnails weren't working is that synapse silently chokes on thumbnailing the exif-stripped image, and so the server-side generated thumbnails 404. looks like it's refusing to thumbnail an application/octet-stream upload, given we've lost the mime-type while stripping.

@ara4n
Copy link
Member

ara4n commented May 10, 2020

empirically this seems to work now. tested in e2ee & non-e2ee rooms, with small & massive images.

It's worth noting that you have to try quite hard to make Riot/Web use server-generated thumbnails these days since #2439; images have to be bigger than 1MB and larger than 1600x1200 (on a retina display) before riot-web will actually ask synapse for a thumbnail.

@ara4n ara4n marked this pull request as ready for review May 10, 2020 15:21
@ara4n ara4n requested a review from a team May 10, 2020 15:21
@ara4n ara4n removed the blocked label May 10, 2020
@ara4n ara4n changed the title Setting to strip EXIF metadata from JPEGs Setting to strip EXIF metadata from JPEG uploads May 10, 2020
src/settings/Settings.js Outdated Show resolved Hide resolved
@t3chguy t3chguy requested a review from dbkr May 14, 2020 23:51
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

Also the two FIXMEs look like we should establish whether they do indeed break things in the way that they say they might.

src/ContentMessages.js Outdated Show resolved Hide resolved
src/ContentMessages.js Outdated Show resolved Hide resolved
@jryans
Copy link
Collaborator

jryans commented Mar 4, 2021

Thanks for making this contribution a while back. Since the code base has changed since this was opened, it no longer applies cleanly, and I don't think there's a need to keep it open in this state, as we can always find the code here again if needed. If you are still interested in pursuing this feature, please discuss with us in #element-dev:matrix.org to find a good approach forward.

@jryans jryans closed this Mar 4, 2021
@t3chguy t3chguy self-assigned this Jul 9, 2021
@t3chguy t3chguy reopened this Jul 9, 2021
… matthew/strip-exif

� Conflicts:
�	src/ContentMessages.tsx
�	src/components/views/settings/tabs/user/PreferencesUserSettingsTab.tsx
�	src/i18n/strings/en_EN.json
�	src/settings/Settings.js
@t3chguy
Copy link
Member Author

t3chguy commented Jul 9, 2021

One deficiency with this PR is that it strips the Orientation EXIF which will cause images to rotate

image

top is with EXIF stripping
bottom is without

image
image

@t3chguy
Copy link
Member Author

t3chguy commented Jul 9, 2021

So I don't see a sane way to retain the Orientation IFD0 - as for the colorimetry we definitely strip colourSpace and all the chromacities stuff, all residing in APP1 along with IFD0

@t3chguy
Copy link
Member Author

t3chguy commented Jul 9, 2021

So whilst this won't work ideally in all cases, I think its good enough for 90% of cases.
Alternative solutions of course are heavyweight library or canvas based approach.

@t3chguy t3chguy requested a review from dbkr July 9, 2021 21:49
@ara4n
Copy link
Member

ara4n commented Jul 9, 2021

ugh. i failed to think through that this would break orientation and colorimetry. can we exclude those from being stripped? last thing we need is to make orientation even more flakey.

@t3chguy
Copy link
Member Author

t3chguy commented Jul 10, 2021

can we exclude those from being stripped?

Not easily, would be saner to load re-export out of a canvas IMO

@Palid
Copy link
Contributor

Palid commented Sep 3, 2021

@t3chguy this PR is here for a while now, does it still apply? If yes, wouldn't it make sense to make a new issue for it so we can properly prioritise it take care of it later?
It seems that you've figured out some problems while working at it, I think it'd be a good idea to add those in the issue as the definition of done.

@t3chguy
Copy link
Member Author

t3chguy commented Sep 6, 2021

does it still apply? If yes, wouldn't it make sense to make a new issue for it so we can properly prioritise it take care of it later?

@Palid what's wrong with the existing issue element-hq/element-web#4426 ?

@Palid
Copy link
Contributor

Palid commented Sep 6, 2021

does it still apply? If yes, wouldn't it make sense to make a new issue for it so we can properly prioritise it take care of it later?

@Palid what's wrong with the existing issue element-hq/element-web#4426 ?

Like a lot of old issues it has spread thin in the details over the years and it's hard to see if it's still relevant. I think this issue is still relevant, but it made more sense to ping you. I think that details of the issue should be probably updated with some newer informations that we already have after initial implementations.

@olmari
Copy link

olmari commented Oct 1, 2021

I might be late on this, but...

While I couldn't definitively be against option to strip metadata form cerrtain files if user chooses to, I'm totally against matrix (umbrella) touching anything in uploaded media by default. That is so big can of worms that will never stop even if you could figure out to do prefect stripping for every file format you accept to touch... And not to say you even can't do it really selectively for images.. (rotation, colorspace, etc data)...

element-hq/element-web#4426 (comment) is still spot on comment on the thing.. Specificly: "private and secure is not the same as anonymous", those who don't want to exif data to "leak", they shoudn't send images with exif data in first place... It can't be matrix task to decide for user, at the least not by default...

@ShadowJonathan
Copy link

Existing social media platforms (Twitter, discord, telegram) already do this, they already strip potentially user-compromising data out of media, element would only be following the trend.

I do agree that there'd be an option to disable it, but I think that element would be making a right choice for (at least) removing GPS data from images wherever possible.

@t3chguy t3chguy marked this pull request as draft March 3, 2022 16:07
@dbkr dbkr removed their request for review November 3, 2022 16:52
@langleyd langleyd closed this Sep 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Options to strip EXIF metadata from media uploads.
9 participants