bpo-43972: Set content-length to 0 for http.server.SimpleHTTPRequestHandler 301s #25705

sirosen · 2021-04-29T01:10:00Z

When http.server.SimpleHTTPRequestHandler sends a 301 (Moved Permanently) due to a missing file, add a Content-Length of 0. This improves the behavior for certain clients.

https://bugs.python.org/issue43972

The text was updated successfully, but these errors were encountered:

When http.server.SimpleHTTPRequestHandler sends a 301 (Moved Permanently) due to a missing file, it does not set a Content-Length of 0. Unfortunately, certain clients can be left waiting for the connection to be closed in this circumstance, even though no body will be sent. At time of writing, both curl and Firefox demonstrate this behavior.

orsenthil · 2021-04-29T03:54:02Z

Lib/http/server.py

@@ -689,6 +689,7 @@ def send_head(self):
                             parts[3], parts[4])
                new_url = urllib.parse.urlunsplit(new_parts)
                self.send_header("Location", new_url)
+                self.send_header("Content-Length", "0")


Please add a test case for this change.

I've added a single check to one of the existing http server tests.

If something else is warranted in this case, I'm more than happy to do it, but I'd need some direction.
The added check seems sufficient to me and in-keeping with the way those existing tests are written.

When serving a redirect, the SimpleHTTPRequestHandler will now send `Content-Length: 0`. Several tests for http.server already cover various behaviors and checks including redirection. This change only adds one check for the expected Content-Length on the simplest case for a redirect.

sirosen · 2021-04-30T16:59:54Z

I just added a news file using blurb, since that was the only failing check (and I think even a minor fix like this should be documented).

orsenthil · 2021-04-30T17:32:32Z

@sirosen - Thank you. You are right, this should definitely be documented.

I was doing more research on the expected behavior in MOVED_PERMANENTLY situation. What does RFC / WHATWG standards recommend, or apache does so that I could confidently accept the patch.

The change (with tests + news) does look good to me.

sirosen · 2021-04-30T18:10:42Z

The specs for HTTP do not require a Content-Length: 0 in this case, but they also don't forbid a 301 with a body.
There are a few HTTP codes which forbid the use of Content-Length, or use its absence to indicate no payload, but 301 is not one of these.

Going off of HTTP 1.1 RFC 7230, 3.3 states

All 1xx (Informational), 204 (No Content), and 304 (Not Modified)
responses do not include a message body. All other responses do
include a message body, although the body might be of zero length.

Nothing 100% unambiguously states that you MUST set Content-Length: 0 for a response with an empty payload, but it's implied by RFC 7230, 3.3:

The presence of a message body in a request is signaled by a
Content-Length or Transfer-Encoding header field.

and by RFC7230, 3.3.2:

For messages that do not include a payload
body, the Content-Length indicates the size of the selected
representation (Section 3 of [RFC7231]).

The spec doesn't require that a client does anything with a payload, but RFC 7231, 6.4.2 suggests that one should be expected

The server's response payload usually contains a short
hypertext note with a hyperlink to the new URI(s).

I think it therefore makes sense for a client which reads the headers for a 301 without Content-Length to either

immediately process the redirect and ignore any potential body, closing the connection client-side (this is what Google Chrome does)
wait for any body because the size was not specified, and process the redirect when the connection is closed (this is what Firefox does)

In terms of what webservers do, I have an nginx box which returns an HTML snippet:

301 Moved Permanently

nginx ">

<html>
<head><title>301 Moved Permanently</title></head>
<body bgcolor="white">
<center><h1>301 Moved Permanently</h1></center>
<hr><center>nginx</center>
</body>
</html>

with a Content-Length of 178.

I don't have an Apache server handy right now, but I'm pretty sure it exhibits similar behavior, serving an HTML payload with an appropriate content-length.

Not including a payload is fine, per the spec. However, it forces a client to close the connection. A client which treats a 301 with no Content-Length as not having a body and reuses the connection would be incorrect.

orsenthil · 2021-05-01T17:05:02Z

This piece of code was written in 2006 ( 4281902) and Content-Length was not added. This has survived for so long.

The client not closing properly, or difference in client behavior seems a client issue to me. (Also, could you give a simple demonstration of curl which will not close in the current scenario).
I am also thinking about cases that if we end up adding Content-Length, 0 for this case, it will potentially be a problem.

I cannot find anything for 2, and slightly in favor of this change. The history does not suggest that there is a bug here for us to fix.

orsenthil · 2021-05-01T17:19:56Z

My analysis is coming to the same as yours. Nothing forbids addition of this change, so +1. But I didn't see a problem with the existing code with the clients I used curl 7.65

orsenthil

LGTM.

orsenthil

LGTM.

orsenthil · 2021-05-06T19:25:27Z

The Azure pipelines failure is unrelated. I see that happened across few jobs from yesterday.
(In the current one https://dev.azure.com/Python/8e426817-76c0-4b99-ba9e-a48a1e4bd5db/_build/results?buildId=80180 and this https://dev.azure.com/Python/8e426817-76c0-4b99-ba9e-a48a1e4bd5db/_build/results?buildId=80180)

The issue not present in the PR's today, I am taking a call to merge this.

The backport PRs shouldn't have a problem too.

miss-islington · 2021-05-06T19:26:05Z

Thanks @sirosen for the PR, and @orsenthil for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

miss-islington · 2021-05-06T19:26:05Z

Thanks @sirosen for the PR, and @orsenthil for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

…andler 301s (pythonGH-25705) * Set content-length for simple http server 301s When http.server.SimpleHTTPRequestHandler sends a 301 (Moved Permanently) due to a missing file, it does not set a Content-Length of 0. Unfortunately, certain clients can be left waiting for the connection to be closed in this circumstance, even though no body will be sent. At time of writing, both curl and Firefox demonstrate this behavior. * Test Content-Length on simple http server redirect When serving a redirect, the SimpleHTTPRequestHandler will now send `Content-Length: 0`. Several tests for http.server already cover various behaviors and checks including redirection. This change only adds one check for the expected Content-Length on the simplest case for a redirect. * Add news entry for SimpleHTTPRequestHandler fix * Clarify the specific kind of 301 Co-authored-by: Senthil Kumaran <skumaran@gatech.edu> (cherry picked from commit fb42725) Co-authored-by: Stephen Rosen <sirosen@globus.org>

bedevere-bot · 2021-05-06T19:26:13Z

GH-25952 is a backport of this pull request to the 3.9 branch.

…andler 301s (pythonGH-25705) * Set content-length for simple http server 301s When http.server.SimpleHTTPRequestHandler sends a 301 (Moved Permanently) due to a missing file, it does not set a Content-Length of 0. Unfortunately, certain clients can be left waiting for the connection to be closed in this circumstance, even though no body will be sent. At time of writing, both curl and Firefox demonstrate this behavior. * Test Content-Length on simple http server redirect When serving a redirect, the SimpleHTTPRequestHandler will now send `Content-Length: 0`. Several tests for http.server already cover various behaviors and checks including redirection. This change only adds one check for the expected Content-Length on the simplest case for a redirect. * Add news entry for SimpleHTTPRequestHandler fix * Clarify the specific kind of 301 Co-authored-by: Senthil Kumaran <skumaran@gatech.edu> (cherry picked from commit fb42725) Co-authored-by: Stephen Rosen <sirosen@globus.org>

bedevere-bot · 2021-05-06T19:26:18Z

GH-25953 is a backport of this pull request to the 3.10 branch.

…andler 301s (GH-25705) * Set content-length for simple http server 301s When http.server.SimpleHTTPRequestHandler sends a 301 (Moved Permanently) due to a missing file, it does not set a Content-Length of 0. Unfortunately, certain clients can be left waiting for the connection to be closed in this circumstance, even though no body will be sent. At time of writing, both curl and Firefox demonstrate this behavior. * Test Content-Length on simple http server redirect When serving a redirect, the SimpleHTTPRequestHandler will now send `Content-Length: 0`. Several tests for http.server already cover various behaviors and checks including redirection. This change only adds one check for the expected Content-Length on the simplest case for a redirect. * Add news entry for SimpleHTTPRequestHandler fix * Clarify the specific kind of 301 Co-authored-by: Senthil Kumaran <skumaran@gatech.edu> (cherry picked from commit fb42725) Co-authored-by: Stephen Rosen <sirosen@globus.org>

the-knights-who-say-ni added the CLA signed label Apr 29, 2021

bedevere-bot added the awaiting review label Apr 29, 2021

orsenthil reviewed Apr 29, 2021

View changes

orsenthil self-assigned this Apr 29, 2021

sirosen added 2 commits Apr 30, 2021

Add news entry for SimpleHTTPRequestHandler fix

50a2c0e

orsenthil added 3 commits May 5, 2021

Clarify the specific kind of 301

ba785de

Update 2021-04-30-16-58-24.bpo-43972.Y2r9lg.rst

f0793d4

Update 2021-04-30-16-58-24.bpo-43972.Y2r9lg.rst

5844cf7

orsenthil approved these changes May 5, 2021

View changes

bedevere-bot added awaiting merge and removed awaiting review labels May 5, 2021

orsenthil self-requested a review May 5, 2021

orsenthil removed the awaiting merge label May 5, 2021

orsenthil approved these changes May 6, 2021

View changes

bedevere-bot added the awaiting merge label May 6, 2021

orsenthil merged commit fb42725 into python:main May 6, 2021
11 of 12 checks passed

bedevere-bot removed the awaiting merge label May 6, 2021

orsenthil added needs backport to 3.9 needs backport to 3.10 labels May 6, 2021

bedevere-bot removed the needs backport to 3.9 label May 6, 2021

bedevere-bot removed the needs backport to 3.10 label May 6, 2021

pablogsal mentioned this pull request Jul 18, 2021

[3.10] Correct the order of regen-abidump #27228

Closed

python / cpython Public

bpo-43972: Set content-length to 0 for http.server.SimpleHTTPRequestHandler 301s #25705

bpo-43972: Set content-length to 0 for http.server.SimpleHTTPRequestHandler 301s #25705

sirosen commented Apr 29, 2021 •

edited by bedevere-bot

orsenthil Apr 29, 2021

sirosen Apr 30, 2021

sirosen commented Apr 30, 2021

orsenthil commented Apr 30, 2021

sirosen commented Apr 30, 2021 •

edited

301 Moved Permanently

orsenthil commented May 1, 2021

orsenthil commented May 1, 2021

orsenthil left a comment

orsenthil left a comment

orsenthil commented May 6, 2021

miss-islington commented May 6, 2021

miss-islington commented May 6, 2021

bedevere-bot commented May 6, 2021

bedevere-bot commented May 6, 2021

python / cpython Public

bpo-43972: Set content-length to 0 for http.server.SimpleHTTPRequestHandler 301s #25705

bpo-43972: Set content-length to 0 for http.server.SimpleHTTPRequestHandler 301s #25705

Conversation

sirosen commented Apr 29, 2021 • edited by bedevere-bot

orsenthil Apr 29, 2021

Choose a reason for hiding this comment

sirosen Apr 30, 2021

Choose a reason for hiding this comment

sirosen commented Apr 30, 2021

orsenthil commented Apr 30, 2021

sirosen commented Apr 30, 2021 • edited

301 Moved Permanently

orsenthil commented May 1, 2021

orsenthil commented May 1, 2021

orsenthil left a comment

orsenthil left a comment

orsenthil commented May 6, 2021

miss-islington commented May 6, 2021

miss-islington commented May 6, 2021

bedevere-bot commented May 6, 2021

bedevere-bot commented May 6, 2021

sirosen commented Apr 29, 2021 •

edited by bedevere-bot

sirosen commented Apr 30, 2021 •

edited