Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Make the Widget Patching Magic Optional #326

ad-65 started this conversation in Ideas
Discussion options

First off, thank you for the great library. Using S3 signed URLs is an awesome addition to Django S3 backed storage.

Secondly, I just wanted to put an idea out there. Sometimes we may not want all django-storages backed file uploads to use s3 pre-signed URLs, maybe some files require server processing, there are security issues with pre-signed URLs in some parts of the application, or maybe we just want to use it for large uploads only. Currently this can be accomplished by not registering the app in INSTALLED_APPS or creating a custom wrapper app that omits the widget patching magic:

class CustomS3FileConfig(S3FileConfig):
 def ready(self):
 from s3file.checks import storage_check
 checks.register(storage_check, checks.Tags.security, deploy=True)
INSTALLED_APPS = [
 ...
 "project.apps.app_name.apps.CustomS3FileConfig",
 ...
]

then a custom widget can be specified for only the fields that should use pre-signed URLs:

class DirectToS3FileWidget(S3FileInputMixin, widgets.ClearableFileInput):
 pass
class CustomForm(forms.Form):
 regular_file_upload= forms.FileField(widget=DirectToS3FileWidget)
 presigned_file_upload = forms.FileField()

The first approach of not registering the app means it isn't in the app registry and doesn't include statics files. The second approach works fine but I was wondering if a setting that can turn off the widget patching magic might be a good idea? Something like:

INSTALLED_APPS = [
 ...
 "s3file",
 ...
]
S3FILE_PATCH_WIDGET = False

Maybe there is a better way to do this but it seems having the ability to mix and match upload types would be a good feature.

Anyway, thanks again for your work on this library.

You must be logged in to vote

Replies: 1 comment

Comment options

Hi @ad-65 👋,

Thanks for reaching out and for your kind words. They really mean a lot to me.

First, let me say, yes, the patching magic should be optional (and it is). However, it is neither documented nor painless and is due for an improvement.


However, let me quickly address some of your concerns, to save you some hassle.

there are security issues with pre-signed URLs in some parts of the application

There shouldn't be, unless you execute arbitrary code from your storage (which you probably shouldn't). If your key value blob store (like S3) is properly configured, files uploaded by users shouldn't be shareable/downloadable, executable or otherwise injectable into your application.

You are also protected from cost dangers, as the max upload boy size for pre-signed URLs is 5 GB.

maybe some files require server processing

This is important! They really don't. That the whole purpose of this package. Processing a large request through your WSGI-stack (even with a reverse proxy) is likely to make you vulnerable to simple DOS attacks. In fact, I would strongly advise you to configure your reverse proxy or ELB to disconnect large requests.

File processing should always be handled asynchronously, and probably buy a machine that is optimized to handle this workload.

Even if you want to process files in your web application, this package will make this process very painless, as the Django request.FILES object will contain the files the user uploaded to S3. However, they are lazily attached and will load from the blob store when you actually access them.

maybe we just want to use it for large uploads only

To add to what I said before: Size doesn't matter 😉 Latency might. You really don't want your server to wait for a request to finish (w/o a reverse proxy). I'm restricting users to 'only upload small files', isn't adding to your user experience. That goes double, when we are talking about upload images.

That was a rather long answer, I hope you appreciate my candor and level of detail.


Now, let's get back to the topic at hand. Since the default behavior is "pretty magical", having an option to disable it, should be included (with a warning).

My preferred solution would be to add a separate app config, people can include alternatively.

Since you raised the topic, are you interested in providing a patch yourself? I don't want to steal your thunder. If you, I am happy to create a patch myself.

Best
Joe

You must be logged in to vote
0 replies
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Category
Ideas
Labels
None yet
2 participants

AltStyle によって変換されたページ (->オリジナル) /