streetcomplete / sc-photo-service Goto Github PK

Hmm, when uploading the picture, the script returns a 415: File type not allowed. The file type as determined by

$finfo = new finfo(FILEINFO_MIME_TYPE);
$file_type = $finfo->buffer($photo);

is application\/x-empty.

The upload has amongst others the following headers:
Content-Transfer-Encoding -> binary
Content-Type -> image/jpeg
and in the test, a file from /storage/emulated/0/Android/data/de.westnordost.streetcomplete.debug/files/Pictures/photo1001169951.jpg was uploaded with this code:

try (OutputStream outputStream = httpConnection.getOutputStream())
{
	byte[] buffer = new byte[16384];
	int bytesRead;
	try (FileInputStream inputStream = new FileInputStream(file))
	{
		while ((bytesRead = inputStream.read(buffer)) != -1)
		{
			outputStream.write(buffer, 0, bytesRead);
		}
		outputStream.flush();
	}
}

(Reads a file and writes it as-is to the output stream)

OSM is deprecating OAuth 1.0a and HTTP Basic Auth.

Your application was identified as making HTTP Basic Auth calls, which will stop working later this year. We are still working out a precise timeline.

I was reminded of a reason for having filenames not generated from a counting number: Even when directory listing is off, people could mass-download the pictures with a curl-for loop.
So, if there is a really good (security? privacy?) reason to disable directory listing, then this reason is also valid for not having filenames that count up.

What do you think?

Looks very good! Can't wait to see it in action!

A few comments. I'll just cluster this in one ticket now:

1. I am not sure why there needs to be JSON at all. Doesn't it make things more complicated?:
   upload.php - could simply return the url in plaintext or a normal HTTP error code + message on failure
   activate.php - could have a normal POST parameter for the note id and the reply is not interesting for the client (=will not be parsed), perhaps if there is an error. The error could be a normal HTTP error code + message
   Generally, it seems a little weird to get a HTTP OK 200 on an error

2. Posting raw data to upload the picture - is this normal? I thought it would be always multipart/form-data for file upload. Not sure if this changes anything.

3. file upload limit: I would very much like a configurable file upload limit. On my website, the maximum upload file size in PHP.ini is 128MB and I do not want to change the default there to something as low as would be a good setting for this service (a few MB)

4. Did you handle the case that a photo has already been moved from the tmp directory to the proper one? activate.php should in that case just return "OK", no error.

5. Did you handle the case that the to-be-activated new picture is in one of the note comments and earlier comments or the first post in the thread also had links which are now dead because the note had been redacted/closed in the meantime but reopened later? (activate.php should still return "OK" and activate just the new one, ignore the rest)

6. Photos of redacted notes should be cleaned up without considering MAX_LIFETIME_AFTER_NOTE_CLOSED_DAYS

7. How well does the service handle if I would go about deleting this and that image manually from the filesystem (without touching the DB)?

8. Why is the file extension saved to DB? (Because it is in the filesystem already). Hmm and actually, why is the id even saved to DB? - It's also in the name

9. For making the file name as short as possible, perhaps a-z could also be used to count-up the filename.

10. The service must be prepared to make all calls to the OSM Notes API with a logged in user, specifyable in the config. (because of GDPR / DSGVO the notes api will perhaps not be allowed to be used as anonymous)

11. What seems to be missing is that the cleanup script also starts deleting photos if the max directory size exceeds the configured limit (oldest first)

12. Finally, double check everything regarding not accepting parameters as-is from the user and preventing the possibility of a SQL injection?

I did not check whether it is already implemented, but possibly you can have some mechanism to clean-up old photos. (also to save space and so on…)

So:

• if a note to a photo has been closed for, say, 3 months
• and if the photo is older than 12 months or so

…then we can likely delete it.

Maybe this is an idea?

Maybe this should be added to https://github.com/westnordost/StreetComplete/blob/master/CONTRIBUTING.md#streetcomplete-related-projects?

Also consider adding the "StreetComplete" tag here. 😄

I created a github organization "StreetComplete". Could we move this repo into the organization?