Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes #30807 -- Fix permissions when extracting archives #11854

Closed
wants to merge 1 commit into from
Closed

Fixes #30807 -- Fix permissions when extracting archives #11854

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 3 additions & 5 deletions django/utils/archive.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,12 +100,10 @@ class BaseArchive:
@staticmethod
def _copy_permissions(mode, filename):
"""
If the file in the archive has some permissions (this assumes a file
won't be writable/executable without being readable), apply those
permissions to the unarchived file.
Carry over permissions from the archive, ensuring extracted files are
always owner readable.
"""
if mode & stat.S_IROTH:
os.chmod(filename, mode)
os.chmod(filename, mode | stat.S_IROTH)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

S_IROTH is "other read", not "owner read".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Nick, thanks for the comment! Apologies for having switched gears too fast here.
Agree, that should have been mode | stat.S_IRUSR to ensure "read by owner".


def split_leading_dir(self, path):
path = str(path)
Expand Down
4 changes: 1 addition & 3 deletions tests/utils_tests/test_archive.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,6 @@ def test_extract_function(self):
def test_extract_file_permissions(self):
"""archive.extract() preserves file permissions."""
mask = stat.S_IRWXU | stat.S_IRWXG | stat.S_IRWXO
umask = os.umask(0)
os.umask(umask) # Restore the original umask.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you expected here, but this was added to fetch the umask on the system the tests are being run on so that it could be applied below to avoid failures where systems have different umask configured.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand was the intention was, however it was only being applied for the test on the no_permissions file, which probably should have a minimum of permissions so the umask does not apply.
Unless I am really misunderstanding what the intended behavior is of course, could you maybe share your thoughts on that?

Having said that, looking at it again I see the umask does need to be applied when testing directory creation, where it currently isn't - agree?
(For this to work, _copy_permissions also needs to be applied to extracted directories)

Again, appreciate the comments, sorry for having switched gears too fast being eager to start contributing :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pope1ni :
I gave this a bit more thought:
Given archive.Archive is only used for providing templates (startapp, startproject) - in those scenarios my guess is we do not really care about preserving original permissions in the archive beyond making sure the extracted files are readable/executable (to avoid issues such as ticket #26494 and ticket #27628. Do you agree?

If so, my suggestion would be to:

  1. Update _copy_permissions to use bit-wise OR to ensure read rights
  2. Update the test to check if extracted files have read rights
  3. Update the test to check if extracted dir has 0o777 & ~umask

Would appreciate your input! :)

for entry in os.scandir(self.testdir):
if entry.name.startswith('leadpath_'):
continue
Expand All @@ -44,4 +42,4 @@ def test_extract_file_permissions(self):
self.assertEqual(os.stat(filepath).st_mode & mask, 0o775)
# A file is readable even if permission data is missing.
filepath = os.path.join(tmpdir, 'no_permissions')
self.assertEqual(os.stat(filepath).st_mode & mask, 0o664 & ~umask)
self.assertEqual(os.stat(filepath).st_mode & mask, 0o4)