Repository navigation
Lib/http/server.py: Return HTTPStatus.NOT_FOUND if path.endswith(/) and not a directory #78892
Description
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = <Date 2018-12-28.14:07:12.834>
created_at = <Date 2018-09-17.10:23:18.283>
labels = ['3.8', 'type-bug', 'library']
title = 'Lib/http/server.py: Return HTTPStatus.NOT_FOUND if path.endswith(/) and not a directory'
updated_at = <Date 2019-09-10.13:48:50.068>
user = 'https://github.com/aixtools'bugs.python.org fields:
activity = <Date 2019-09-10.13:48:50.068>
actor = 'ned.deily'
assignee = 'none'
closed = True
closed_date = <Date 2018-12-28.14:07:12.834>
closer = 'ncoghlan'
components = ['Library (Lib)']
creation = <Date 2018-09-17.10:23:18.283>
creator = 'Michael.Felt'
dependencies = []
files = ['47840', '47841']
hgrepos = []
issue_num = 34711
keywords = ['patch']
message_count = 19.0
messages = ['325519', '325521', '325532', '325546', '325624', '326867', '326880', '326886', '326887', '326888', '326889', '326899', '326901', '326919', '326953', '332531', '337633', '337653', '337696']
nosy_count = 7.0
nosy_names = ['ncoghlan', 'vstinner', 'jkloth', 'martin.panter', 'eryksun', 'matrixise', 'Michael.Felt']
pr_nums = ['9360', '9675', '9687', '14197']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue34711'
versions = ['Python 3.8']
Activity
aixtools commented on Sep 17, 2018
Going back to bpo-17234 - there has been a test to check that a URL with a trailing slash reports 404 status.
On AIX a trailing-slash is ignored - if the rest of the path is a valid filename.
At a very low-level, in Modules/_io/fileio.c the code could be adjusted to always look for a trailing slash, and if AIX considers the name to be a file (S_ISREG(mode)) then this could be promoted to an error. just as it does for "posix" opens directories with trailing slashes (and fileio.c deals with that in a Python way).
A quick fix is to skip the test bpo-17324 introduced;
A medium fix would be to modify Lib/http/server.py to verify that a URL with trailing slash is a directory, and otherwise raise an exception;
A deeper fix would be to add "a hack" specific to AIX, and raise (emulate) an OSError when there is a trailing slash and the pathname is a file.
+AIX = platform.system() == 'AIX'
class NoLogRequestHandler: def log_message(self, *args): @@ -417,9 +419,11 @@ class SimpleHTTPServerTestCase(BaseTestCase): #constructs the path relative to the root directory of the HTTPServer response = self.request(self.base_url + '/test') self.check_status_and_reason(response, HTTPStatus.OK, data=self.data) - # check for trailing "/" which should return 404. See Issue17324 - response = self.request(self.base_url + '/test/') - self.check_status_and_reason(response, HTTPStatus.NOT_FOUND) + # AIX open() will open a filename with a trailing slash - as a file + if not AIX: + # check for trailing "/" which should return 404. See Issue17324 + response = self.request(self.base_url + '/test/') + self.check_status_and_reason(response, HTTPStatus.NOT_FOUND) response = self.request(self.base_url + '/') self.check_status_and_reason(response, HTTPStatus.OK) response = self.request(self.base_url) @@ -519,8 +523,9 @@ class SimpleHTTPServerTestCase(BaseTestCase): def test_path_without_leading_slash(self): response = self.request(self.tempdir_name + '/test') self.check_status_and_reason(response, HTTPStatus.OK, data=self.data) - response = self.request(self.tempdir_name + '/test/') - self.check_status_and_reason(response, HTTPStatus.NOT_FOUND) + if not AIX: + response = self.request(self.tempdir_name + '/test/') + self.check_status_and_reason(response, HTTPStatus.NOT_FOUND) response = self.request(self.tempdir_name + '/') self.check_status_and_reason(response, HTTPStatus.OK) response = self.request(self.tempdir_name)Comments!
jkloth commented on Sep 17, 2018
This is also an issue on Windows when the target path resides within a junction, paths outside of a junction respond (err, fail) as expected.
https://developercommunity.visualstudio.com/content/problem/272379/createfile-non-error-on-filename-with-trailing-bac.html
aixtools commented on Sep 17, 2018
On 17/09/2018 12:47, Jeremy Kloth wrote:
[ENOTDIR]
A component of the path prefix names an existing file that is
neither a directory nor a symbolic link to a directory; or O_CREAT
and O_EXCL are not specified, the /path/ argument contains at least
one non- <slash> character and ends with one or more trailing
<slash> characters, and the last pathname component names an
existing file that is neither a directory nor a symbolic link to a
directory; or O_DIRECTORY was specified and the /path/ argument
resolves to a non-directory file.
from http://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html
aixtools commented on Sep 17, 2018
OK - bpo-17324 (not 1 7 2 3 4)
And, as jkloth reported that Windows also has an issue - sometimes, found a way to do this in Modules/_io/fileio.c
And, now for the PR tests...
aixtools commented on Sep 18, 2018
On 17/09/2018 16:00, Michael Felt wrote:
Had a review - many thanks, but before I press the resolve button -
Ihope someone can help me understand why the "Travis" etc, checks are
failing, e.g.,
./python -E -S -m sysconfig --generate-posix-vars ;\
if test $? -ne 0 ; then \
echo "generate-posix-vars failed" ; \
rm -f ./pybuilddir.txt ; \
exit 1 ; \
fi
clang -pthread -L/home/travis/multissl/openssl/1.1.0h/lib
-L/home/travis/multissl/openssl/1.1.0h/lib -Xlinker -export-dynamic -o
Programs/_testembed Programs/_testembed.o libpython3.8dm.a -lpthread
-ldl -lutil -lm -lm
Segmentation fault (core dumped)
generate-posix-vars failed
I have no clue how my relatively small change can have this kind of a
effect and then nothing moves forward. Very frustrating!
Thx,
Michael
vstinner commented on Oct 2, 2018
Jeremy Kloth: "This is also an issue on Windows when the target path resides within a junction, paths outside of a junction respond (err, fail) as expected."
https://developercommunity.visualstudio.com/content/problem/272379/createfile-non-error-on-filename-with-trailing-bac.html
I don't know the behavior on Windows. I tried to create a file name "a\" (U+0061, U+005C): I get an OSError "invalid argument" (error 22).
I confirm that in a junction point, I am able to:
open an existing file with an additional trailing antislash (U+005C): the extra character is simply ignored and I am able to open the file
create a new file with an additional trailing antislash (U+005C): the filename is created without the trailing antislash
On the PR, I wrote:
Ok, let me give some examples of function which directly open a file:
Ok... But there are other functions to access files... stat()/fstat() functions:
To start to have a better ideas of how many functions accept filenames, open also Lib/shutil.py. shutil.copyfile() uses os.stat(), but then it uses os.symlink() and open()... So what about os.symlink()?
Ok, here I only listen a *few* examples of functions which are "controlled" by Python. But there are *many* wrappers to 3rd party libraries which accept a filename. Examples:
Where is the limit? How many functions must be patched in Python? How do we patch OpenSSL and SQLite libraries?
Python is designed as a thin wrapper to the operating system. IMHO Python must not validate the filename itself.
--
IMHO you must fix a single place: the SimpleHTTPServer, not all code handling the filesytem.
Same remark for AIX and Windows junctions.
I suggest to reject this issue.
vstinner commented on Oct 2, 2018
You replied with an encrypted message which isn't understood by the bug tracker.
aixtools commented on Oct 2, 2018
Final attempt to send as plain text
On 10/2/2018 1:07 AM, Benjamin Peterson wrote:
At the surface, it appears Python is using PY_LDFLAGS (with
CONFIGURE_LDFLAGS coming from LDFLAGS during the ./configure moment.
A reason for a separate variable is that this particular option is only
relevant for the python EXE, and not for shared libraries and "other
things". IMHO, a reason for LDMAXDATA is because LDLAST is actually
already too widely used:
root@x066:[/data/prj/python/git/cpython-master]grep LDFLAGS *.in
Makefile.pre.in:CONFIGURE_LDFLAGS= @ldflags@
Makefile.pre.in:# Avoid assigning CFLAGS, LDFLAGS, etc. so users can use
them on the
Makefile.pre.in:# Both CPPFLAGS and LDFLAGS need to contain the shell's
value for setup.py to
Makefile.pre.in:PY_LDFLAGS= $(CONFIGURE_LDFLAGS) $(LDFLAGS)
Makefile.pre.in:LDSHARED= @LDSHARED@ $(PY_LDFLAGS)
Makefile.pre.in:BLDSHARED= @BLDSHARED@ $(PY_LDFLAGS)
Makefile.pre.in:OPENSSL_LDFLAGS=@OPENSSL_LDFLAGS@
Makefile.pre.in: $(MAKE) @DEF_MAKE_RULE@ CFLAGS_NODIST="$(CFLAGS)
$(PGO_PROF_GEN_FLAG)" LDFLAGS="$(LDFLAGS) $(PGO_PROF_GEN_FLAG)"
LIBS="$(LIBS)"
Makefile.pre.in: $(MAKE) @DEF_MAKE_RULE@ CFLAGS_NODIST="$(CFLAGS)
$(PGO_PROF_USE_FLAG)" LDFLAGS="$(LDFLAGS)"
Makefile.pre.in: $(LINKCC) $(PY_LDFLAGS) $(LINKFORSHARED) -o $@
Programs/python.o $(BLDLIBRARY) $(LIBS) $(MODLIBS) $(SYSLIBS) $(LDLAST)
Makefile.pre.in: $(CC) -dynamiclib -Wl,-single_module
$(PY_LDFLAGS) -undefined dynamic_lookup
-Wl,-install_name,$(prefix)/lib/libpython$(LDVERSION).dylib
-Wl,-compatibility_version,$(VERSION) -Wl,-current_version,$(VERSION) -o
$@ $(LIBRARY_OBJS) $(SHLIBS) $(LIBC) $(LIBM) $(LDLAST); \
Makefile.pre.in: $(CC) -o $(LDLIBRARY) $(PY_LDFLAGS) -dynamiclib \
Makefile.pre.in: $(LINKCC) $(PY_LDFLAGS) $(LINKFORSHARED) -o $@
Programs/_testembed.o $(BLDLIBRARY) $(LIBS) $(MODLIBS) $(SYSLIBS) $(LDLAST)
Makefile.pre.in: $(LINKCC) $(PY_LDFLAGS) -o $@
Programs/_freeze_importlib.o $(LIBRARY_OBJS_OMIT_FROZEN) $(LIBS)
$(MODLIBS) $(SYSLIBS) $(LDLAST)
Makefile.pre.in: $(CC) $(OPT) $(PY_LDFLAGS) $(PGENOBJS)
$(LIBS) -o $(PGEN)
The ONLY line that needs $LDMAXDATA is:
Makefile.pre.in: $(LINKCC) $(PY_LDFLAGS) -o $@
Programs/_freeze_importlib.o $(LIBRARY_OBJS_OMIT_FROZEN) $(LIBS)
$(MODLIBS) $(SYSLIBS) $(LDLAST) $(LDMAXDATA)
or set $(LDLAST) at the end rather than append $(LDMAXDATA)
On 10/2/2018 1:02 PM, Michael Felt wrote:
aixtools commented on Oct 2, 2018
On 10/2/2018 10:36 AM, STINNER Victor wrote:
IMHO: the "statement" of the test is that the name "test/" MUST be
rejected in all cases.
# check for trailing "/" which should return 404. See bpo-17324
response = self.request(self.base_url + '/test/')
self.check_status_and_reason(response, HTTPStatus.NOT_FOUND)
Title <http://docs.python.org/devguide/triaging.html#title\>:
SimpleHTTPServer serves files even if the URL has a trailing slash
I can assume the test was adjusted to catch the 404 return code, and let
that pass on OS that will not open .../test.txt and .../text.txt/ as the
same object - while AIX (and windows does).
As to your comment about "other" things that can happen - the message in
the test implies that Python only wants "filenames" that are opened
without a slash. I guess there is not a PEP on this, so anyones guess is
likely to be wrong for someone.
In short, this PR took the road where Python says .../filename/ is wrong
So, what should be done: code in HTTPServer that checks for trailing
slash and the test can be left ASIS, or modify the test to skip this one
aspect on an OS that is known to accept ".../filename/" as a valid filename?
I am not "core" enough to make this choice. I can attempt to code any
decision made by "python-dev" aka core-devs.
aixtools commented on Oct 2, 2018
Was not my intent. Firewall issues. After 4 more attempts gave up until now.
On 10/2/2018 3:17 PM, STINNER Victor wrote:
3 remaining items
eryksun commented on Oct 2, 2018
This path-parsing bug that strips a trailing slash occurs when a traversed directory is a reparse point, such as a mount-point (junction) or directory symbolic link. It may be limited to just the NTFS and ReFS file-system drivers, which is all I tested. Or it may be a bug in the I/O manager itself, which actually implements the name grafting for Microsoft's mount-point (0xA0000003) and symlink (0xA000000C) reparse tags.
I thought the suggested patch was ok because it followed the pattern of existing code that prevents opening directories on Unix. But I can see the distinction in that the latter is implementing behavior policy rather than working around a platform bug. Also, while _Py_open and _Py_fopen do allow opening a directory on a Linux box, in apparent violation of this policy, these are private, internal functions, and there's no use that I'm aware of in the CPython implementation for directory FDs or FILE streams opened this way.
vadmium commented on Oct 2, 2018
Hi Michael, I agree with Victor that the best place to fix the problem is in the HTTP server module. In other words, the “medium fix” you mentioned in your original post.
Your recent proposal to just skip the test means that AIX will continue to suffer from the original bug.
aixtools commented on Oct 3, 2018
Closed "test" version.
made new PR that makes server.py conform to bpo-17234 demands
ncoghlan commented on Dec 26, 2018
New changeset 2062a20 by Nick Coghlan (Michael Felt) in branch 'master':
bpo-34711: Return HTTPStatus.NOT_FOUND if path.endswith('/') and not a directory (GH-9687)
2062a20
aixtools commented on Mar 10, 2019
Could this also be backported to Version 3.7 and 3.6?
matrixise commented on Mar 11, 2019
Hi Michael,
I think no, because 3.6 is in security mode.
aixtools commented on Mar 11, 2019
On 11/03/2019 09:42, Stéphane Wirtel wrote: