#30807 closed Bug (fixed)
test_extract_file_permissions test fails when umask is set to 000.
Reported by: | Brady | Owned by: | Ad Timmering |
---|---|---|---|
Component: | Testing framework | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | pope1ni | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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
Change History (13)
comment:1 by , 5 years ago
Cc: | added |
---|---|
Component: | Uncategorized → Testing framework |
Summary: | Test Suite fails with umask set to 000 → test_extract_file_permissions test fails when umask is set to 000. |
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 5 years ago
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)
comment:3 by , 5 years ago
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 is0o0
(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 instat.S_IROTH
). Because this file has no permissions (also no user read), permissions are not correctly updated to reflect permissions in the archive.
comment:4 by , 5 years ago
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:
- 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)
- Just fix the current test to not assume a umask of 2, but leaving current permissions logic in place.
Any thoughts?
comment:5 by , 5 years ago
Has patch: | set |
---|---|
Owner: | changed from | to
Status: | new → assigned |
PR added - comments welcome.
comment:6 by , 5 years ago
Patch needs improvement: | set |
---|
comment:7 by , 5 years ago
Has patch: | unset |
---|---|
Patch needs improvement: | unset |
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)
comment:8 by , 5 years ago
I don't see any security issue in the current solution. IMO we should only fix falling tests.
comment:10 by , 4 years ago
Has patch: | set |
---|
Apologies for late follow-up. Per latest feedback just focused on fixing the test.
Thanks for the report. I have a similar issue on Linux after setting
umask
to0
, e.g.