test_extract_file_permissions test fails when umask is set to 000.

[Brady]

Environment: Mac OS version 10.14.6
Python Version: 3.7.3

The test test_extract_file_permissions fails with an assertion error of 468 != 466

Steps to reproduce:
1) Set umask to 000
2) Restart machine
3) start a virtualenv python3 -m venv django_virtualenv
4) Activate virtualenv source django_virtualenv/bin/activate
5) cd django_virtualenv
6) clone the django source code into here git clone <your forked version of django>
7) pip install -e .
8) cd tests && pip install -r requirements/py3.txt
9) ./runtests.py
10) Once this has run, it should have 6 failures all relating to the test_extract_file_permissions

[Mariusz Felisiak]

Thanks for the report. I have a similar issue on Linux after setting umask to 0, e.g.

FAIL: test_extract_file_permissions (utils_tests.test_archive.TestArchive) [foobar.tar.bz2]
archive.extract() preserves file permissions.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "django/tests/utils_tests/test_archive.py", line 48, in test_extract_file_permissions
    self.assertEqual(os.stat(filepath).st_mode & mask, 0o664 & ~umask)
AssertionError: 438 != 436

[Ad Timmering]

The same issue happens on Windows Linux Subsystem which has a default umask of 0 (see ​this thread - fix is apparently inbound).

If I'm reading this correctly, the test incorrectly assumes all systems have at least a umask of 2 (no write for world), and the fix would be to change the test to assume 0o666. For systems that do have a umask of 2, this will keep working as intended as it already negates the OS umask with & umask.

self.assertEqual(os.stat(filepath).st_mode & mask, 0o664 & ~umask)

[Ad Timmering]

Looking a bit deeper, I am puzzled by the test setup, that might be hiding a different bug:

• The archive has a file called no_permissions that is 0o0 (zero permissions) in the archive

• The current test assumes the file ends up with something similar to 0o664, which means rw-rw-r--, so also read/write for group and other.

• The reason this works is because django.utils.archive.BaseArchive has a method called _copy_permissions that only reflects permissions if the file in the archive at least has 0o4 (owner-read, defined in stat.S_IROTH). Because this file has no permissions (also no user read), permissions are not correctly updated to reflect permissions in the archive.

    @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.
        """
        if mode & stat.S_IROTH:
            os.chmod(filename, mode)

• Permission code introduced to fix #27628 (Fixed unarchiving a file without permission data)
• Above fixes a change introduced in #26494 (Made Archive.extract() preserve file permissions).

[Ad Timmering]

Having now read back the history of the previous tickets (#26494 and #27628) a few thoughts:

_copy_permissions was introduced in #27628 to ensure files remain readable by the other so that files extracted from an archive remain readable (and thus executable), even if they do not have those permissions set within the archive.

It seems we have (at least) two options:

1. Rather than checking it has other-read (stat.S_IROTH) we could do a bit-wise OR with owner-read to ensure owner-read on all extracted files (which is what is happening now too, but in current state group & world permissions are now set to OS default instead of what is set in the archive)

2. Just fix the current test to not assume a umask of 2, but leaving current permissions logic in place.

Any thoughts?

[Ad Timmering]

PR added - comments welcome.

[Ad Timmering]

TL;DR: I am wondering if we should undo fixes for #26494 (and therefore also #27628) for security reasons rather than fix the test in this issue.
Any thoughts/input welcome.

Short recap
Archive.extract is only being used by django-admin commands startproject and startapp to enable provisioning of custom archive templates.
Originally we extracted using OS default permissions (set by umask).

In #26494 it was raised that scripts (eg. manage.py) extracted from an archive were no longer executable.
In these cases the compressed scripts *did* have the executable flag set in the archive.
This was addressed by applying permissions stored in archive metadata to the extracted files.

In #27628 it was noted that subsequently in some cases file were being extracted with zero permissions, causing errors as Django would not be able to read for example template files.
In these cases the files in question have *no* permissions set in the archive, and all default permissions were removed as result of fix #26494.
This was addressed by trying to ensure that files at the very least are “readable”.

In #30807 as result of the above fix, we see that some archive tests are failing on some platforms because the tests added in the most recent bug did not properly account for a default umask not being 002.

Suggested solution
However, instead of addressing this issue, I would like to posit that we maybe should not have applied the fix for #26494:

Users will frequently refer to archives downloaded from external sites through a URL. In the current setup we rely on the archive to be trusted and set executable permissions based on archive metadata.

• Do we not expose users unnecessarily to potential risk with unknown code being set to executable?
• Should we not leave setting of “execute” permissions up to the user? (Users responsibility to see if it is indeed the script/code they intended)

[Mariusz Felisiak]

I don't see any security issue in the current solution. IMO we should only fix falling tests.

[Ad Timmering]

Understood and will submit PR to fix the test.

[Ad Timmering]

Apologies for late follow-up. Per latest feedback just focused on fixing the test.

[GitHub <noreply@…>]

In ec5aa216:

Fixed #30807 -- Fixed TestArchive.test_extract_file_permissions() when umask is 0o000.

Fixed test that checks permissions on files extracted from archives
with no permissions set, to not assume a default umask of 0o002.

Test regression in c95d063e776e849cf1a0bf616c654165cb89c706.

[Mariusz Felisiak <felisiak.mariusz@…>]

In c944df8:

[3.1.x] Fixed #30807 -- Fixed TestArchive.test_extract_file_permissions() when umask is 0o000.

Fixed test that checks permissions on files extracted from archives
with no permissions set, to not assume a default umask of 0o002.

Test regression in c95d063e776e849cf1a0bf616c654165cb89c706.
Backport of ec5aa2161d8015a3fe57dcbbfe14200cd18f0a16 from master

[Mariusz Felisiak <felisiak.mariusz@…>]

In d1ff7c5:

[3.0.x] Fixed #30807 -- Fixed TestArchive.test_extract_file_permissions() when umask is 0o000.

Fixed test that checks permissions on files extracted from archives
with no permissions set, to not assume a default umask of 0o002.

Test regression in c95d063e776e849cf1a0bf616c654165cb89c706.
Backport of ec5aa2161d8015a3fe57dcbbfe14200cd18f0a16 from master