Skip to content
This repository has been archived by the owner. It is now read-only.

POST /avatar/upload should validate image payload #96

Closed
zaach opened this issue Feb 20, 2015 · 2 comments
Closed

POST /avatar/upload should validate image payload #96

zaach opened this issue Feb 20, 2015 · 2 comments
Assignees

Comments

@zaach
Copy link
Contributor

@zaach zaach commented Feb 20, 2015

If the body of the request doesn't look like image data (e.g. {}) the request should fail.

@jrgm
Copy link
Contributor

@jrgm jrgm commented Feb 22, 2015

Yeah, so, the profile server's worker was correctly detecting the error. However, the server did not see the error and would return 200 OK and a URL to where the image had supposedly been stored. The problem can be duplicated with:

curl -s 'https://jrgm4.dev.lcip.org/profile/v1/avatar/upload' \
  -H 'Accept: application/json' \
  -H 'Content-Type: image/png; charset=UTF-8' \
  -H 'Accept-Encoding: deflate, gzip' \
  -H 'Authorization: Bearer 347e17e015dae....' \
  -H 'Content-Length: 2' \
  -X POST \
  -d"{}" | zcat; echo

You need to obviously provide a valid Bearer token above, but if you do, that will return '201 Created' with a payload that has a url.

The problem is that the response from the worker is gzipped, but the server expect uncompressed. Possibly just need to not asking for gzip is a fix:

diff --git a/lib/routes/avatar/upload.js b/lib/routes/avatar/upload.js
index 1c0ab54..b319be2 100644
--- a/lib/routes/avatar/upload.js
+++ b/lib/routes/avatar/upload.js
@@ -30,6 +30,7 @@ function fxaUrl(id) {
 function pipeToWorker(id, payload, headers) {
   return new P(function(resolve, reject) {
     var url = WORKER_URL + '/a/' + id;
+    headers['accept-encoding'] = 'identity';
     var opts = { headers: headers, json: true };
     logger.verbose('pipeToWorker', url);
     payload.pipe(request.post(url, opts, function(err, res, body) {

With the change above, that would now return HTTP 500 with:

{"code":500,"errno":103,"error":"Internal Server Error","message":"Image processing error","_res":{"id":"deadbeefdeadbeef...","error":"Stream yields empty buffer"}}

Probably a little more detail than needed in that response, and a 400 level error would be better for bad input.

@seanmonstar
Copy link
Member

@seanmonstar seanmonstar commented Mar 3, 2015

I'm thinking of adding a gm(payload).identify() call before trying to resize, so as to detect if the buffer is indeed an image. Otherwise, it's difficult to tell if the error from gm().resize() is because of a non-image or some other reason.

@rfk rfk added this to the FxA-0: quality milestone Oct 6, 2015
@vladikoff vladikoff self-assigned this Nov 2, 2015
vladikoff added a commit that referenced this issue Nov 13, 2015
Fixes #96
vladikoff added a commit that referenced this issue Nov 13, 2015
Fixes #96
vladikoff added a commit that referenced this issue Nov 13, 2015
Fixes #96
vladikoff added a commit that referenced this issue Nov 16, 2015
Fixes #96
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants