[Django] #30807: Test Suite fails with umask set to 000
Django
-----------------------------------------+------------------------
Reporter: bmoe872 | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
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`
--
Ticket URL: <https://code.djangoproject.com/ticket/30807>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-----------------------------------+------------------------------------
Reporter: Brady | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* cc: Nick Pope (added)
* stage: Unreviewed => Accepted
* component: Uncategorized => Testing framework
Comment:
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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30807#comment:1>
Django
-----------------------------------+------------------------------------
Type: Bug | Status: new
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Ad Timmering):
The same issue happens on Windows Linux Subsystem which has a default
umask of 0 (see [https://github.com/microsoft/WSL/issues/352 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)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30807#comment:2>
Django
-----------------------------------+------------------------------------
Type: Bug | Status: new
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30807#comment:3>
Django
-----------------------------------+------------------------------------
Type: Bug | Status: new
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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 owner 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 owner-read (`stat.S_IROTH`) we could do a
bit-wise OR 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/30807#comment:4>
Django
Reporter: Brady | Owner: Ad
| Timmering
Type: Bug | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Ad Timmering):
* owner: nobody => Ad Timmering
* status: new => assigned
* has_patch: 0 => 1
Comment:
PR added - comments welcome.
--
Ticket URL: <https://code.djangoproject.com/ticket/30807#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Brady | Owner: Ad
| Timmering
Type: Bug | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Ad Timmering):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/30807#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Brady | Owner: Ad
| Timmering
Type: Bug | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Ad Timmering):
* needs_better_patch: 1 => 0
* has_patch: 1 => 0
Comment:
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)
--
Ticket URL: <https://code.djangoproject.com/ticket/30807#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Brady | Owner: Ad
| Timmering
Type: Bug | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by felixxm):
I don't see any security issue in the current solution. IMO we should only
fix falling tests.
--
Ticket URL: <https://code.djangoproject.com/ticket/30807#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Brady | Owner: Ad
| Timmering
Type: Bug | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Ad Timmering):
Understood and will submit PR to fix the test.
--
Ticket URL: <https://code.djangoproject.com/ticket/30807#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Brady | Owner: Ad
| Timmering
Type: Bug | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Ad Timmering):
* has_patch: 0 => 1
Comment:
Apologies for late follow-up. Per latest feedback just focused on fixing
the test.
--
Ticket URL: <https://code.djangoproject.com/ticket/30807#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Brady | Owner: Ad
| Timmering
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by GitHub <noreply@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"ec5aa2161d8015a3fe57dcbbfe14200cd18f0a16" ec5aa216]:
{{{
#!CommitTicketReference repository=""
revision="ec5aa2161d8015a3fe57dcbbfe14200cd18f0a16"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30807#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Brady | Owner: Ad
| Timmering
Type: Bug | Status: closed
Component: Testing framework | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"c944df827fcf6fb5d99bc8399049ed312e40c670" c944df8]:
{{{
#!CommitTicketReference repository=""
revision="c944df827fcf6fb5d99bc8399049ed312e40c670"
[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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30807#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Brady | Owner: Ad
| Timmering
Type: Bug | Status: closed
Component: Testing framework | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"d1ff7c50e3a9815804818ef8f76ad3c16d3f1da4" d1ff7c5]:
{{{
#!CommitTicketReference repository=""
revision="d1ff7c50e3a9815804818ef8f76ad3c16d3f1da4"
[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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30807#comment:13>