Fixes #574 #333 #223 - s3store: Feature for saving objects with custom path (including filename and extension) (also merged #668)

[Akkarine]

feat/fix #574 fix #333 fix #223 fix #488

• Checked on S3-compatible service. Uploading & downloading with customized file ID works. Have not checked without s3store. Tests passed.
• bumped alpine runtime in dockerfile
• get rid of unmaintained routing library bmizerany/pat
• fixed CORS request test (status must be just 404, not 405, consequences of bmizerany/pat)

[Acconut]

Thank you for your work on this PR, I have a bit of feedback:

As already mentioned in #692, please avoid combined PRs like this where multiple independent changes are made in a single PR. This makes it hard to review, track changes and messes with the commit history.

get rid of unmaintained routing library bmizerany/pat

pat might be unmaintained but I don't see this as a reason to get rid of it. As mentioned in the linked thread, pat is considered feature complete and there are also not bugs which are posing a problem to tusd. So getting rid of pat and replacing it with our own routing solution only adds additional code cost and complexity. I do not think this is a good approach.

Feature for saving objects with custom path (including filename and extension)

If I understand your code correctly, s3store will just blindly accept the object key presented in the CustomFilepath metadata. This is a vulnerability where users can freely overwrite any object in a bucket they want, which can pose a huge threat for systems. I am sorry, but custom paths can not be included in tusd like this.

Custom filepaths are often discussed here, but for me the best option is to not include a feature for custom filepaths in tusd, but just let your server rename the file after the upload has been complete. This is still the simplest and most customizable approach for this.

If we decide to implement custom filepaths, there are two major requirements for its implementation: It should be able to support all types of data stores and not be tailored to one specific store. It does not have to be implemented for every store, but it should be possible to extend the system to any other store. Secondly, the custom filepath needs to be decided by the tusd server (e.g. through the pre-create hook) and not freely by the end-user to avoid vulnerabilities.

I hope you can understand this.

[Akkarine]

@Acconut Thank you for your time and detailed review!

pat might be unmaintained but I don't see this as a reason to get rid of it.

That is not the only reason. I wouldn't do that if I had an option to leave it with custom filepath feature. I've forgot exact details, but I've struggled, that library does not provide enough flexible path templates so you can't exctract custom filepath and call an appropriate handler. Furthermore, this exctracted id is not even used anywhere. And this heavy functionality used just for switching on incoming method. So I've followed author's steps 😄

This is a vulnerability where users can freely overwrite any object in a bucket they want

I understand your worries. Also that is a reason of followed PR's changes, that allow to send authorization headers in hooks. Where I've implemented such check (as you suggest).

Secondly, the custom filepath needs to be decided by the tusd server ... through the pre-create hook

I would like to implement this feature by this way, but unfortunately, hooks right now do not provide features to give usable feedback to tusd. Maybe it is in your new hooks system?

It should be able to support all types of data stores

Isn't it not ideal, but well-documented functionality is better than its absence? S3 is very popular backend storage. Just comment for example. And how many there are just leaving this project seeing this comment or using slow copying of objects in cloud?

I see you want to protect users from shooting oneself in foot. Right now maybe we can put a BIG STARTUP WARNING or even disable this feature, if there is no pre-create hook?

Please avoid combined PRs

Yes, I understand. Sorry for unconvinience (worked in rush), but, as you see, refactor on routing system is part of this feature.
What is the best approach to present my work in multiple PR's? Should I erase all commits and just copy changed files and provide separated PR's for:

• refactor routing system
• add custom filepath feature
• merge Add a flag to disable downloads of uploaded files #668

But if each following PR rely on previous work, last PR anyway will look, that it contains all changes. Is it right? Also, please consider, that I'm already using fork in my project and can't wait while one PR will be approved before creating another.

[Akkarine]

I have another idea for overright protection. Before storing file to custom filepath, check, if there any file present on this way. If so, deny with 409 status. So user will have to explicitly delete file before.

[Acconut]

I would like to implement this feature by this way, but unfortunately, hooks right now do not provide features to give usable feedback to tusd. Maybe it is in your new hooks system?

Yes, the new hook system can be used to implement such a feature a lot more easily. It would be best to first wait until the new hook system is finished before looking into a feature to allow customizing the storage path for uploads.

Isn't it not ideal, but well-documented functionality is better than its absence?

That's not what I wanted to say :) Instead, features should be designed in such a way that they can theoretically support all data stores. It is not necessary to implement it for all data stores in the beginning. However, if we design a feature that only works with one data stores, then I do not see it as a good feature design and we need to rework the design to make it more general.

I have another idea for overright protection. Before storing file to custom filepath, check, if there any file present on this way. If so, deny with 409 status. So user will have to explicitly delete file before.

I am sorry, but this is also not enough. All in all, I do not see that your approach is suitable for tusd. We can revisit this topic once the new hook system is completed and tested. It will allow implementing customizable storage path in a safe and secure manner. Thank you very much for you work on this PR and I am sorry that we cannot merge your efforts. Hope you understand that.

[Akkarine]

Ok, thank you for detail answer.

[halconel]

Hello! I also need this to be able to specify a custom filename and path. And that's basically a headache when using s3storage.
In my opinion, not all types of storage are the same and should work differently.

[Acconut]

@halconel Thanks for your feedback. We have such feature on our roadmap.