Mattermost-server: New plugin: Strip EXIF Data from Images when being uploaded

[esethna]

If you're interested please comment here and come join our "Contributors" community channel on our daily build server, where you can discuss questions with community members and the Mattermost core team. For technical advice or questions, please join our "Developers" community channel.

New contributors please see our Developer's Guide.

---

Notes: Jira ticket

This ticket should be implemented as a plugin to Mattermost, rather than implementing it in the core server repository.

When images are being uploaded to MM, this plugin should strip EXIF data from them to avoid sharing e.g. geolocation data.

This will allow installations of Mattermost where the EXIF data should be automatically stripped from all images to use the plugin to achieve this.

[grundleborg]

Note that it may be necessary to add additional hooks to the plugin API to make this plugin possible. The best place for advice and discussion on plugin development is the Developer Toolkit channel on community.mattermost.com

[nimrodshn]

I would like to take this one! :1st_place_medal:

[hanzei]

Hey @nimrodshn,

Just checking how your work is going. Do you have any questions?

[hanzei]

Making this one available for the public again due to inactivity.

[nimrodshn]

@hanzei Sorry for the inactivity. Please move this to [Help Wanted] I might go back to my project to finish it later on when I find time but at this point in time this issue is -"up for grabs".

[scottleedavis]

Hello! I'd like to do this one. :)

Started today and was able to extract EXIF from uploaded images.
https://github.com/scottleedavis/mattermost-plugin-noexif

However, stripping out the EXIF tags I haven't been able to do yet, and have asked the question for the library creators.
https://github.com/dsoprea/go-exif/issues/16

This is a work in progress.

[hanzei]

Hi Scott, great, that you are working on this! :rocket:

[scottleedavis]

@hanzei after working sometime trying to go go-exif to strip exif information, I came across a simpler solution using the image lib to decode => encode images which by default strips the exif information out.
https://github.com/scottleedavis/mattermost-plugin-noexif

Currently however, only jpg/jpeg and png images are supported. Tiff appears to not strip the information.

Should this plugin support tiff? Are there any other desired formats supported?

[hanzei]

Thanks for the update @scottleedavis :+1:

I'm deferring these questions to @aaronrothschild, as he is the PM for this area.

[aaronrothschild]

Thanks @hanzei , Scott- I love this imgur-ification of images concept. (original purpose of Imgur was to make a privacy conscious image sharing repository for reddit)

@DSchalla What was the original intent of this JIRA ticket? Was it to ensure there are absolutely no points of PII stored within Mattermost inadvertently? Or Was it to help ensure the end-user's confidence of privacy? OR was it to prevent accidental leakage of PII within a channel? I'm wondering because the reason will determine how much "coverage" we might need from such a plugin.

If it's to help ensure people in a public channel do not share PII inadvertently, I would say JPG/PNG coverage is good enough. I'm not aware of any common photo-sharing apps that share TIFFs frankly.

@scottleedavis - Something to consider is putting some text that indicates to the user that the uploaded images have been processed and cleansed - ie: "Your uploaded photos have had their location and EXIF data removed before being uploaded to the server" . I say this not just because the file has been changed, but it is possible that when the file is re-processed - different compression settings maybe used than the original. For example - if someone posts a Marketing Image as a "proof" in a channel, they expect that image to be 'identical' when someone downloads it. It would be useful for troubleshooting purposes to give the user awareness that the photo may have changed subtly (even if just some EXIF data was removed).

[scottleedavis]

Sounds good @aaronrothschild , added the text you recommended. Happy to make any additional changes as requested.

[DSchalla]

@aaronrothschild The ticket was created as a response of a HackerOne report, where exactly the concern of privacy and leakage of PII was mentioned. It's mostly a security enhancement when open communities want to avoid that people share by mistake their location or other PII. I think supporting TIFF not initially is absolutely acceptable, as you've said it's not the most common format anyway.

Since it's easy to implement, I'd make the error message optional. For a lot of services it's normal that they do strip EXIF data, but for some as you've brought up in your example it should be explicitly mentioned.

[scottleedavis]

Sounds good @DSchalla . I added a configuration option to enable/disable the message. (disabled by default)

[scottleedavis]

Code review issues created by @lieut-data have been resolved.
https://github.com/scottleedavis/mattermost-plugin-noexif/issues/3
https://github.com/scottleedavis/mattermost-plugin-noexif/issues/2

[scottleedavis]

Hello! Building this plugin was fun. 😄
I am not sure what next steps for this plugin are.
I had requested a review for ci-extensions acceptance, though I feel that completing acceptance criteria for this issue is likely of higher importance.
What are the next steps to complete this issue?

[lieut-data]

@scottleedavis, I've created an epic for @levb to track getting this through the rest of the gauntlet: https://mattermost.atlassian.net/browse/MM-18844.

I've also uploaded it to https://ci-extensions.azure.k8s.mattermost.com for immediate use and testing there.

[scottleedavis]

Thank you @lieut-data .

[levb]

@scottleedavis how would you like me to provide the code review feedback? Maybe make a PR from master into an empty branch, so I can comment on lines there?

[scottleedavis]

@levb I tried this approach in creating an empty branch, and when looking at it via a compare/PR, no changes show up https://github.com/scottleedavis/mattermost-plugin-noexif/compare/empty-branch?expand=1

I am happy to do this approach (though haven't done it before) if you'd prefer. Do you have a process for it you can share?
else
We could say all code is open for review with LOC refs ../plugin.go#L6-L26 with issues created?

Open to other ideas as well.

[jasonblais]

Hey @scottleedavis, touching base to hear how things are going and if you have any questions about this issue?

[scottleedavis]

hey @jasonblais this has been dev complete for a while. Currently waiting on review by @levb when he is available.

[jasonblais]

Thanks Scott! Looks like it's currently in QA review (https://mattermost.atlassian.net/browse/MM-18848). I'll close this issue off and we can track via that Jira ticket

[levb]

@jasonblais it's been on us to review and deploy and we have not been able to prioritize it, it's not a trivial piece of code.