674363 - Robustify photo upload code

Status: VERIFIED FIXED

(Participation Infrastructure :: Phonebook, defect, P2)

Description

[Austin King [:ozten]]

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

Comment 1

[tofumatt [:tofumatt]]

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)

Comment 2

[Austin King [:ozten]]

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.

Comment 3

[Dave Dash [:davedash, :dd] (assign all bugs to mbrandt)]

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.

Comment 4

[Aakash Desai [:aakashd]]

Dave, is this a blocker for 1.0?

Comment 5

[[github robot]]

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.

Comment 6

[Matt Brandt [:mbrandt]]

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.

Comment 7

[Aakash Desai [:aakashd]]

> 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.