don't make uploaded files executable by default #369

Closed
wants to merge 5 commits into
from

Projects

None yet

3 participants

@lotheac
lotheac commented Sep 12, 2012

See the previous pull request #326

@charettes charettes and 1 other commented on an outdated diff Sep 12, 2012
tests/regressiontests/file_storage/tests.py
@@ -449,6 +449,23 @@ def test_file_upload_permissions(self):
actual_mode = os.stat(self.storage.path(name))[0] & 0o777
self.assertEqual(actual_mode, 0o666)
+class FileStorageDefaultPermissions(unittest.TestCase):
+ def setUp(self):
+ self.old_perms = settings.FILE_UPLOAD_PERMISSIONS
@charettes
charettes Sep 12, 2012 Member

You could use the django.test.utils.override_settings decorator to wrap FileStorageDefaultPermissions here.

@lotheac
lotheac Sep 12, 2012

Thanks. With that, I don't actually need a separate class, I can just use the existing FileStoragePermissions and decorate the test methods. Added commit 5f69e10.

@charettes charettes and 1 other commented on an outdated diff Sep 12, 2012
tests/regressiontests/file_storage/tests.py
+ @override_settings(FILE_UPLOAD_PERMISSIONS=0o666)
@charettes
charettes Sep 12, 2012 Member

We should change the tested permissions to be something else than 0o666 now that's the default one.

@lotheac
lotheac Sep 12, 2012

Or maybe we should test with a different umask instead? The default permission should be 0666 ~ umask, but FILE_UPLOAD_PERMISSIONS should ignore umask.

@charettes
charettes Sep 12, 2012 Member

We could test both scenarios.

@charettes
Member

Is there a trac ticket attached to this PR? If that's not the case you should open one and point it to this patch since I think it's ready for check in.

@apollo13
Member

The patch looks good so far, but it doesn't pass on Python 3:

    fd = os.open(full_path, os.O_WRONLY | os.O_CREAT | os.O_EXCL | getattr(os, 'O_BINARY', 0), 0666)
                                                                                                  ^
SyntaxError: invalid token

Can you test on Python3 too and fix those? Txh!

@lotheac
lotheac commented Sep 16, 2012

Ah, you're right. Commit 67f844e should fix that.

@apollo13 apollo13 commented on the diff Sep 16, 2012
django/core/files/storage.py
@@ -192,7 +192,7 @@ def _save(self, name, content):
else:
# This fun binary flag incantation makes os.open throw an
# OSError if the file already exists before we open it.
- fd = os.open(full_path, os.O_WRONLY | os.O_CREAT | os.O_EXCL | getattr(os, 'O_BINARY', 0))
+ fd = os.open(full_path, os.O_WRONLY | os.O_CREAT | os.O_EXCL | getattr(os, 'O_BINARY', 0), 0o666)
@apollo13
apollo13 Sep 16, 2012 Member

Something just came to my mind: How does 0o666 affect windows?

@lotheac
lotheac Sep 16, 2012

The Python docs just refer to runtime C library documentation (http://docs.python.org/library/os.html#os.open), so no idea there.

@apollo13 apollo13 commented on the diff Sep 16, 2012
tests/regressiontests/file_storage/tests.py
+ @override_settings(FILE_UPLOAD_PERMISSIONS=None)
+ def test_file_upload_default_permissions(self):
+ fname = self.storage.save("some_file", ContentFile("data"))
+ mode = os.stat(self.storage.path(fname))[0] & 0o777
@apollo13
apollo13 Sep 16, 2012 Member

Does this work on windows too?

@lotheac
lotheac Sep 16, 2012

Sorry, no idea, I don't have a Windows machine to test with. But, the only new thing here is umask; test_file_upload_permissions was a similar test previously.

@apollo13
Member

Fixed in e8c6aff.

@apollo13 apollo13 closed this Sep 17, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment