kd2org / karadav

Lightweight NextCloud compatible WebDAV server
https://fossil.kd2.org/karadav/
GNU Affero General Public License v3.0
155 stars 14 forks source link

Use an actual temporary storage location for uploaded files #20

Closed theo546 closed 2 years ago

theo546 commented 2 years ago

Hello,

I made this pull request so the PHP script would use the /tmp location when uploading some files instead of putting them in the directory of the Storage.php script file.

Feel free to edit this MR if more locations should be added.

theo546 commented 2 years ago

I may also add that adding a setting to choose on which directory to upload temporary files should be a much wiser solution.

bohwaz commented 2 years ago

Thanks for your PR, but yes, this should be configurable, not a static location, especially as on mass hosting you don't get the right to access anything outside of your document root. So I can't accept it like that.

bohwaz commented 2 years ago

Also, I would not recommend this, as it can create issues when renaming files across different file systems.

The fact that the temporary file is created in the same directory is a design choice, to make sure that the temporary file and the destination are in the same file system.

theo546 commented 2 years ago

I can understand this choice, but currently, I am running the Docker image in a read-only container, the fact that I cannot use a tmpfs mount is a flaw, as currently, I cannot create or upload any file without the patch I made.

theo546 commented 2 years ago

A quick fix would be to allow using a folder inside of the same directory as the script, that way, mounting a tmpfs would be possible and would allow the use of a read-only container.

bohwaz commented 2 years ago

Oh right sorry I didn't see that there is something missing!

The temporary files should be created inside the users storage path, that line of code is missing the path as a prefix, let me fix that.

bohwaz commented 2 years ago

OK done, should be good now :)

bohwaz commented 2 years ago

The intention was to store the temporary files inside STORAGE_PATH, I just forgot to include it in the path of the file :)

My intention was never to store files inside the app root :)

theo546 commented 2 years ago

Just tested now, seems to work perfectly fine!

bohwaz commented 2 years ago

Great! Thanks for your report :)