Add ability to protect media folder #1476
Conversation
| @@ -35,6 +35,8 @@ RUN set -eux; \ | |||
| libmagic1 \ | |||
| libcairo2 \ | |||
| libpango1.0-0 \ | |||
| libpcre3 \ | |||
| libpcre3-dev \ | |||
There was a problem hiding this comment.
Deze packages zijn nodig om uwsgi met PCRE ondersteuning te bouwen. Dit is nodig voor de X-Sendfile emulation.
| from django.core.files.storage import FileSystemStorage | ||
| from django.utils.encoding import filepath_to_uri | ||
|
|
||
| signer = signing.TimestampSigner() |
There was a problem hiding this comment.
In order to make this more secure, would it be a good idea to pass a salt to the signer?
By default the salt is set like this: https://github.com/django/django/blob/5b4d949d7ca118e70985ffc53f8191b766591c12/django/core/signing.py#L188C1-L191C10
It doesn't seem like a huge deal considering the secret is also used, so as long as that is unique we should still receive acceptable signatures. But better safe than sorry I suppose.
| if url is not None: | ||
| url = url.lstrip('/') | ||
|
|
||
| signature = signer.sign(url).split(':') |
There was a problem hiding this comment.
Please note that if the same file is requested multiple times within the same second the signature that is produced will be exactly the same. Not sure if that is acceptable?
| from django.core.files.storage import FileSystemStorage | ||
| from django.utils.encoding import filepath_to_uri | ||
|
|
||
| signer = signing.TimestampSigner() |
There was a problem hiding this comment.
SHA256 is used by default as the hashing algo here. We might want to explicitly specify it to prevent any issues of it changing in the future. That is if we deem SHA256 sufficient for this use case.
One of it's advantages is that it's fast, however that is also a disadvantage as it would also be possible to relatively quickly produce things like rainbow tables.
Considering we might not need lightning fast speeds here, we could perhaps choose an even more secure algorithm here?
| @@ -0,0 +1,23 @@ | |||
| # Protected media | |||
There was a problem hiding this comment.
Considering this is a separate app, maybe it would be worth noting that it needs to be added to the SIGNALS_APPS in app/signals/settings.py?
|
|
||
|
|
||
| class ProtectedFileSystemStorage(FileSystemStorage): | ||
| def url(self, name): |
There was a problem hiding this comment.
Could you please use type hints for the arguments and the return type?
| signer = signing.TimestampSigner() | ||
|
|
||
|
|
||
| def download_file(request, path): |
There was a problem hiding this comment.
Please use type hints for the arguments and return type here.
| response = self.client.get('/signals/media/test.txt?t=some_time&s=some_signature') | ||
| self.assertEqual(response.status_code, 401) | ||
| self.assertEqual(response.content, b'Bad signature') | ||
|
|
There was a problem hiding this comment.
Is there no test case for expired signatures?
| return HttpResponse('Bad signature', status=401) | ||
|
|
||
| if settings.DEBUG: | ||
| response = serve(request, path, document_root=settings.MEDIA_ROOT, show_indexes=False) |
There was a problem hiding this comment.
This is a little nitpicky, but this function is already using early returns, why not just return here as well instead of having the else block?
| @@ -19,6 +19,10 @@ | |||
| path('signals/', BaseSignalsAPIRootView.as_view()), | |||
| path('signals/', include('signals.apps.api.urls')), | |||
|
|
|||
| # The media folder is routed with X-Sendfile when DEBUG=False and | |||
| # with the Django static helper when DEBUG=True | |||
| path('signals/media/', include('signals.apps.media.urls')), | |||
There was a problem hiding this comment.
What happens here if the app is not configured to be loaded in the settings?
|
|
||
| @override_settings(PROTECTED_FILE_SYSTEM_STORAGE=True) | ||
| class DownloadFileTestCase(TestCase): | ||
| def setUp(self): |
There was a problem hiding this comment.
Please use type hints for the methods here. It should be as easy as adding -> None: to each of them.
Description
This app provides the possibility to protect the media folder. To use this functionality in production, make sure to configure the PROTECTED_FILE_SYSTEM_STORAGE setting.
Checklist
mainand is up to date withmainmainHow has this been tested?