PhotoBackup / server-php

The PHP PhotoBackup server implementation
MIT License
8 stars 4 forks source link

Mistake in checking for duplicates. #6

Open Zegnat opened 8 years ago

Zegnat commented 8 years ago

In index.php on line 156, no argument for filesize:

    filesize() === $_POST['filesize']
fliker09 commented 8 years ago

I have a question - how are you dealing with duplicate names (not necessarily duplicate content)?

Zegnat commented 8 years ago

how are you dealing with duplicate names (not necessarily duplicate content)?

If the name matches an existing file but the file size is different, we assume that the previously uploaded file was uploaded incompletely and overwrite it with the new one.

This is modelled after the canonical Python implementation.

fliker09 commented 8 years ago

That's actually BAD. The thing is that all photos from different folders on the client are uploaded into one folder on server - name collision may happen (and eventually data loss in case user removes file(s) from his device).

Zegnat commented 8 years ago

I agree. You can easily solve this right now with a small code tweak:

--- index.php
+++ index.php
@@ -143,7 +143,10 @@
  * Sanitize the file name to maximise server operating system compatibility and
  * minimize possible attacks against this implementation.
  */
-$filename = preg_replace('@\s+@', '-', $_FILES['upfile']['name']);
+$filename = explode('.', $_FILES['upfile']['name']);
+array_splice($filename, -1, 0, array(md5_file($_FILES['upfile']['tmp_name'])));
+$filename = implode('.', $filename);
+$filename = preg_replace('@\s+@', '-', $filename);
 $filename = preg_replace('@[^0-9a-z._-]@i', '', $filename);
 $target = $MediaRoot . '/' . $filename;

I am not sure what the best place would be to file that as an issue for PhotoBackup as a whole, possibly at PhotoBackup/api. (Ping @stephanepechard.) I would love to put some time into PhotoBackup again, but life has been getting in the way.

fliker09 commented 8 years ago

How can I apply this patch (yeah, I know, dumb question, still learning things about GitHub)?

Zegnat commented 8 years ago

Sorry, I was at work and just threw this out there real quick. For further reading, what I posted is a patch file and could be applied using different diff tools. The simplest is putting it in a text file in the same folder as the index.php and then use patch on the command line:

$ patch < change.diff

It would automatically put the changes into your index.php.

Here is the patched index.php for your convenience.

Zegnat commented 8 years ago

And for my own use, here is the patch for this issue:

--- index.php
+++ index.php
@@ -153,7 +153,7 @@
  */
 if (
     file_exists($target) &&
-    filesize() === $_POST['filesize']
+    filesize($target) === $_POST['filesize']
 ) {
     header($protocol . ' 409 Conflict');
     exit();
fliker09 commented 8 years ago

Thank you!