Robustify photo upload code

VERIFIED FIXED

Status

P2
normal
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: ozten, Assigned: davedash)

Tracking

Details

(Reporter)

Description

8 years ago
We need to add a few steps to our photo upload to make it more secure:
1) Re-encode the image
Make sure it's actually an image. Strips any funny business out. Re-encode it to the same image format... jpeg -> jpeg and png -> png.

2) Limit max filesize

3) ???

Note: Images are stored in LDAP, so some exploits that we typically guard against are not a concern:
* file path name
* file path location
(Reporter)

Updated

8 years ago
Blocks: 657053
(Reporter)

Updated

7 years ago
Assignee: nobody → dd
I'd like to see a varied list of photo sizes on resize, after we start storing multiple photos. The largest photo could be sanely limited to 300x300 I think. Sizes I'd like:

300x300 (Max)
100x100 (Useful to have later)
70x70 (Profile edit page)
50x50 (Search results)
(Reporter)

Comment 2

7 years ago
If we store a photo outside of LDAP's jpegPhoto...

We cannot just setup an Apache mount point. Requests should go through Django so that the ACL's decide if they have access to the user's photo. If no access, then a 404 should be returned to maintain plausible deniability.

Currently we have /user/photo/{unique_id}. Any new urls would also use unique_id.
I talked to Aakash regarding privacy concerns.  As far as photos go, they do not need to be explicitly behind LDAP.

This means we can do the following:

Upload photo -> store the original sized photo after reprocessing
-> store the various sizes
-> have URLS like: ROOT/photo/:username/hashoforiginal/(original|300|50|70|100).jpg
-> store the hash in the DB or LDAP (ozten how hard is it to store in LDAP)

We'll need to build:
* garbage collection scripts to iterate through hashes and look them up and delete things that have been left behind
* delete hooks to delete photos
* helper methods to render the images.
(Reporter)

Updated

7 years ago
Blocks: 680497
Dave, is this a blocker for 1.0?
Priority: -- → P2
Target Milestone: --- → 1.0
Target Milestone: 1.0 → 1.1

Comment 5

7 years ago
Commit pushed to https://github.com/mozilla/mozillians

https://github.com/mozilla/mozillians/commit/2a964bc19ac35d96df4b306d0b279dfc40305a2c
Reprocess Images, bug 674363, bug 689665

This code will:

    * Resize all images that have a dimension larger than 300x300
    * Save all images as JPEG

There's no tests, because PIL tests take a while to write, and we are
potentially throwing this code away by 1.1.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
QA verified. Tested with jpgs & pngs larger than 300x300 and smaller than 300x300. 

Per comment 1:
300x300 (Max)
70x70 (Profile edit page)
50x50 (Search results)

The only nit is images for search results should be scaled to 50x50 but appear to currently be 70x70 (comment 1). I don't know if this really matters.

Tofumatt suggested I not worry about these numbers and go drink a beer. Bumping to verified.
Status: RESOLVED → VERIFIED
> The only nit is images for search results should be scaled to 50x50 but
> appear to currently be 70x70 (comment 1). I don't know if this really
> matters.

File it, Matt. It's not important, but it is a bug.
Component: mozillians.org → Phonebook
Product: Websites → Community Tools
QA Contact: mozillians-org → phonebook
Target Milestone: 1.1 → ---
Version: unspecified → other
You need to log in before you can comment on or make changes to this bug.