Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mistake in checking for duplicates. #6

Open
Zegnat opened this issue Apr 28, 2016 · 8 comments · May be fixed by #8
Open

Mistake in checking for duplicates. #6

Zegnat opened this issue Apr 28, 2016 · 8 comments · May be fixed by #8
Labels

Comments

@Zegnat
Copy link
Contributor

Zegnat commented Apr 28, 2016

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

    filesize() === $_POST['filesize']
@Zegnat Zegnat added the bug label Apr 28, 2016
@fliker09
Copy link

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

@Zegnat
Copy link
Contributor Author

Zegnat commented Apr 28, 2016

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
Copy link

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
Copy link
Contributor Author

Zegnat commented Apr 28, 2016

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
Copy link

fliker09 commented May 1, 2016

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

@Zegnat
Copy link
Contributor Author

Zegnat commented May 1, 2016

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
Copy link
Contributor Author

Zegnat commented May 2, 2016

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
Copy link

fliker09 commented May 4, 2016

Thank you!

jkufner added a commit to jkufner/server-php that referenced this issue Aug 10, 2016
Fixes PhotoBackup#2: index.php is now considered a configuration file (which loads
the server at the end). To avoid conflicts during update the index.php
is not committed to repository, instead a index.php.example is provided.
It is also possible to move index.php somewhere outside the document
root and include it from the real index.php (see instructions in the
index.php.example).

The whole project is now a Composer package. Composer is not neccessary
when running this as standalone server, but it allows to use this
project as a library.

Also, it fixes PhotoBackup#6 - a missing parameter in checking for duplicates.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants