USH-023 — Internal Server Error While Uploading Photo

[willdoran]

Expected behaviour

• Don't show (detailed) error messages to users.
• Validate the input.

Actual behaviour

• An internal server error appears when using a NULL byte in the filename. This reveals the underlying host where the images are located.
• Server throws a 500, not 422

{"errors":[{"status":0,"title":"Client error response\n[status code] 412\n[reason
 phrase] Precondition Failed\n[url] https:\/\/storage101.ord1.clouddrive.com
\/v1\/MossoCloudFS_e606dc27-4d2f-4bbb-8ea9-ff12ba538332\/platform-cloud\/svink.api.ush
ahidi.io\/5\/8\/58a6821538ad0-test.png%00","message":"Cli
ent error response\n[status code] 412\n[reason phrase] Precondition Failed\n[url]
 https:\/\/storage101.ord1.clouddrive.com\/v1\/MossoCloudFS_e606dc2
7-4d2f-4bbb-8ea9-ff12ba538332\/platform-cloud\/svink.api.ushahidi.io\/5\/8\/58a6821538
ad0-test.png%00"}]}

Steps to reproduce the behaviour/error

• Simulate a file upload where the file name is test.png%00
• Server throws an error
• Error shouldn't contain paths.

Implementation

• Validate file name in Media Create validator
• Modify Ushahidi\Core\Tool\Uploader
  • Add try/catch blocks around fs calls where needed and ensure we are catching error
  • Throw appropriate errors ie. validation or client errors.
• Modify CreateMedia usecase:
  • Move $this->uploader->upload to try/catch block.
  • If it fails, trigger a Validator exception (or something else where appropriate)

Caveat:

• Uploader is modified/deprecated in lumen. If we can resolve this issue solely within the usecase - do that.

[rowasc]

Does this need a more detailed spec or is it good to go?
@rjmackay @willdoran ?

[rjmackay]

I think it's straight forward. Error messages shouldn't include paths, in this case I think its because we send the flysystem error back in the response. We can drop that.

[rjmackay]

@rowasc added more to the spec. Is that sufficient?

[rowasc]

Yea. APPROVED, that's enough
Side question: Do you think this is still a 1?

[rjmackay]

I'll bump it to a 2 just to account for checking the develop and master branches for the issue

[rowasc]

I just tested this in qazone and it worked. I also confirmed that I was able to reproduce the issue before the fix landed in that server.