Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Fixed #5898 -- Changed a few response processing paths to make things…

… harder to get wrong and easier to get right. Previous behaviour wasn't buggy, but it was harder to use than necessary.

We now have automatic HEAD processing always (previously required ConditionalGetMiddleware), middleware benefits from the Location header rewrite, so they can use relative URLs as well, and responses with response codes 1xx, 204 or 304 will always have their content removed, in accordance with the HTTP spec (so it's much harder to indavertently deliver invalid responses).

Based on a patch and diagnosis from regexbot@gmail.com.


git-svn-id: http://code.djangoproject.com/svn/django/trunk@6662 bcc190cf-cafb-0310-a4f2-bffc1f526a37
  • Loading branch information...
commit 3ee3d6b5f3f3b20c509ae1516354914c5e52f773 1 parent 30848df
Malcolm Tredinnick malcolmt authored
28 django/core/handlers/base.py
View
@@ -4,6 +4,10 @@
import sys
class BaseHandler(object):
+ # Changes that are always applied to a response (in this order).
+ response_fixes = [http.fix_location_header,
+ http.conditional_content_removal]
+
def __init__(self):
self._request_middleware = self._view_middleware = self._response_middleware = self._exception_middleware = None
@@ -50,10 +54,6 @@ def load_middleware(self):
def get_response(self, request):
"Returns an HttpResponse object for the given HttpRequest"
- response = self._real_get_response(request)
- return fix_location_header(request, response)
-
- def _real_get_response(self, request):
from django.core import exceptions, urlresolvers
from django.core.mail import mail_admins
from django.conf import settings
@@ -134,15 +134,13 @@ def _get_traceback(self, exc_info=None):
import traceback
return '\n'.join(traceback.format_exception(*(exc_info or sys.exc_info())))
-def fix_location_header(request, response):
- """
- Ensure that we always use an absolute URI in any location header in the
- response. This is required by RFC 2616, section 14.30.
-
- Code constructing response objects is free to insert relative paths and
- this function converts them to absolute paths.
- """
- if 'Location' in response and request.get_host():
- response['Location'] = request.build_absolute_uri(response['Location'])
- return response
+ def apply_response_fixes(self, request, response):
+ """
+ Applies each of the functions in self.response_fixes to the request and
+ response, modifying the response in the process. Returns the new
+ response.
+ """
+ for func in self.response_fixes:
+ response = func(request, response)
+ return response
1  django/core/handlers/modpython.py
View
@@ -162,6 +162,7 @@ def __call__(self, req):
# Apply response middleware
for middleware_method in self._response_middleware:
response = middleware_method(request, response)
+ response = self.apply_response_fixes(request, response)
finally:
dispatcher.send(signal=signals.request_finished)
2  django/core/handlers/wsgi.py
View
@@ -207,6 +207,7 @@ def __call__(self, environ, start_response):
# Apply response middleware
for middleware_method in self._response_middleware:
response = middleware_method(request, response)
+ response = self.apply_response_fixes(request, response)
finally:
dispatcher.send(signal=signals.request_finished)
@@ -220,3 +221,4 @@ def __call__(self, environ, start_response):
response_headers.append(('Set-Cookie', str(c.output(header=''))))
start_response(status, response_headers)
return response
+
1  django/http/__init__.py
View
@@ -5,6 +5,7 @@
from urlparse import urljoin
from django.utils.datastructures import MultiValueDict, FileDict
from django.utils.encoding import smart_str, iri_to_uri, force_unicode
+from utils import *
RESERVED_CHARS="!*'();:@&=+$,/?%#[]"
34 django/http/utils.py
View
@@ -0,0 +1,34 @@
+"""
+Functions that modify an HTTP request or response in some way.
+"""
+
+# This group of functions are run as part of the response handling, after
+# everything else, including all response middleware. Think of them as
+# "compulsory response middleware". Be careful about what goes here, because
+# it's a little fiddly to override this behaviour, so they should be truly
+# universally applicable.
+
+def fix_location_header(request, response):
+ """
+ Ensures that we always use an absolute URI in any location header in the
+ response. This is required by RFC 2616, section 14.30.
+
+ Code constructing response objects is free to insert relative paths and
+ this function converts them to absolute paths.
+ """
+ if 'Location' in response and request.get_host():
+ response['Location'] = request.build_absolute_uri(response['Location'])
+ return response
+
+def conditional_content_removal(request, response):
+ """
+ Removes the content of responses for HEAD requests, 1xx, 204 and 304
+ responses. Ensures compliance with RFC 2616, section 4.3.
+ """
+ if 100 <= response.status_code < 200 or response.status_code in (204, 304):
+ response.content = ''
+ response['Content-Length'] = 0
+ if request.method == 'HEAD':
+ response.content = ''
+ return response
+
18 django/middleware/http.py
View
@@ -6,8 +6,6 @@ class ConditionalGetMiddleware(object):
Last-Modified header, and the request has If-None-Match or
If-Modified-Since, the response is replaced by an HttpNotModified.
- Removes the content from any response to a HEAD request.
-
Also sets the Date and Content-Length response-headers.
"""
def process_response(self, request, response):
@@ -18,19 +16,17 @@ def process_response(self, request, response):
if response.has_header('ETag'):
if_none_match = request.META.get('HTTP_IF_NONE_MATCH', None)
if if_none_match == response['ETag']:
- response.status_code = 304
- response.content = ''
- response['Content-Length'] = '0'
+ # Setting the status is enough here. The response handling path
+ # automatically removes content for this status code (in
+ # http.conditional_content_removal()).
+ response.status = 304
if response.has_header('Last-Modified'):
if_modified_since = request.META.get('HTTP_IF_MODIFIED_SINCE', None)
if if_modified_since == response['Last-Modified']:
- response.status_code = 304
- response.content = ''
- response['Content-Length'] = '0'
-
- if request.method == 'HEAD':
- response.content = ''
+ # Setting the status code is enough here (same reasons as
+ # above).
+ response.status = 304
return response
2  django/test/client.py
View
@@ -42,7 +42,7 @@ def __call__(self, environ):
# Apply response middleware
for middleware_method in self._response_middleware:
response = middleware_method(request, response)
-
+ response = self.apply_response_fixes(request, response)
finally:
dispatcher.send(signal=signals.request_finished)
52 tests/regressiontests/test_client_regress/models.py
View
@@ -31,12 +31,12 @@ def test_contains(self):
self.assertContains(response, 'once', 2)
except AssertionError, e:
self.assertEquals(str(e), "Found 1 instances of 'once' in response (expected 2)")
-
+
try:
self.assertContains(response, 'twice', 1)
except AssertionError, e:
self.assertEquals(str(e), "Found 2 instances of 'twice' in response (expected 1)")
-
+
try:
self.assertContains(response, 'thrice')
except AssertionError, e:
@@ -46,37 +46,37 @@ def test_contains(self):
self.assertContains(response, 'thrice', 3)
except AssertionError, e:
self.assertEquals(str(e), "Found 0 instances of 'thrice' in response (expected 3)")
-
+
class AssertTemplateUsedTests(TestCase):
fixtures = ['testdata.json']
-
+
def test_no_context(self):
"Template usage assertions work then templates aren't in use"
response = self.client.get('/test_client_regress/no_template_view/')
# Check that the no template case doesn't mess with the template assertions
self.assertTemplateNotUsed(response, 'GET Template')
-
+
try:
self.assertTemplateUsed(response, 'GET Template')
except AssertionError, e:
self.assertEquals(str(e), "No templates used to render the response")
- def test_single_context(self):
+ def test_single_context(self):
"Template assertions work when there is a single context"
response = self.client.get('/test_client/post_view/', {})
- #
+ #
try:
self.assertTemplateNotUsed(response, 'Empty GET Template')
except AssertionError, e:
self.assertEquals(str(e), "Template 'Empty GET Template' was used unexpectedly in rendering the response")
-
+
try:
- self.assertTemplateUsed(response, 'Empty POST Template')
+ self.assertTemplateUsed(response, 'Empty POST Template')
except AssertionError, e:
self.assertEquals(str(e), "Template 'Empty POST Template' was not a template used to render the response. Actual template(s) used: Empty GET Template")
-
+
def test_multiple_context(self):
"Template assertions work when there are multiple contexts"
post_data = {
@@ -99,37 +99,37 @@ def test_multiple_context(self):
self.assertEquals(str(e), "Template 'base.html' was used unexpectedly in rendering the response")
try:
- self.assertTemplateUsed(response, "Valid POST Template")
+ self.assertTemplateUsed(response, "Valid POST Template")
except AssertionError, e:
self.assertEquals(str(e), "Template 'Valid POST Template' was not a template used to render the response. Actual template(s) used: form_view.html, base.html")
class AssertRedirectsTests(TestCase):
def test_redirect_page(self):
- "An assertion is raised if the original page couldn't be retrieved as expected"
+ "An assertion is raised if the original page couldn't be retrieved as expected"
# This page will redirect with code 301, not 302
- response = self.client.get('/test_client/permanent_redirect_view/')
+ response = self.client.get('/test_client/permanent_redirect_view/')
try:
self.assertRedirects(response, '/test_client/get_view/')
except AssertionError, e:
self.assertEquals(str(e), "Response didn't redirect as expected: Response code was 301 (expected 302)")
-
+
def test_lost_query(self):
"An assertion is raised if the redirect location doesn't preserve GET parameters"
response = self.client.get('/test_client/redirect_view/', {'var': 'value'})
try:
self.assertRedirects(response, '/test_client/get_view/')
except AssertionError, e:
- self.assertEquals(str(e), "Response redirected to 'http://testserver/test_client/get_view/?var=value', expected '/test_client/get_view/'")
+ self.assertEquals(str(e), "Response redirected to 'http://testserver/test_client/get_view/?var=value', expected 'http://testserver/test_client/get_view/'")
def test_incorrect_target(self):
"An assertion is raised if the response redirects to another target"
- response = self.client.get('/test_client/permanent_redirect_view/')
+ response = self.client.get('/test_client/permanent_redirect_view/')
try:
# Should redirect to get_view
self.assertRedirects(response, '/test_client/some_view/')
except AssertionError, e:
self.assertEquals(str(e), "Response didn't redirect as expected: Response code was 301 (expected 302)")
-
+
def test_target_page(self):
"An assertion is raised if the response redirect target cannot be retrieved as expected"
response = self.client.get('/test_client/double_redirect_view/')
@@ -138,7 +138,7 @@ def test_target_page(self):
self.assertRedirects(response, 'http://testserver/test_client/permanent_redirect_view/')
except AssertionError, e:
self.assertEquals(str(e), "Couldn't retrieve redirection page '/test_client/permanent_redirect_view/': response code was 301 (expected 200)")
-
+
class AssertFormErrorTests(TestCase):
def test_unknown_form(self):
"An assertion is raised if the form name is unknown"
@@ -157,7 +157,7 @@ def test_unknown_form(self):
self.assertFormError(response, 'wrong_form', 'some_field', 'Some error.')
except AssertionError, e:
self.assertEqual(str(e), "The form 'wrong_form' was not used to render the response")
-
+
def test_unknown_field(self):
"An assertion is raised if the field name is unknown"
post_data = {
@@ -175,7 +175,7 @@ def test_unknown_field(self):
self.assertFormError(response, 'form', 'some_field', 'Some error.')
except AssertionError, e:
self.assertEqual(str(e), "The form 'form' in context 0 does not contain the field 'some_field'")
-
+
def test_noerror_field(self):
"An assertion is raised if the field doesn't have any errors"
post_data = {
@@ -193,7 +193,7 @@ def test_noerror_field(self):
self.assertFormError(response, 'form', 'value', 'Some error.')
except AssertionError, e:
self.assertEqual(str(e), "The field 'value' on form 'form' in context 0 contains no errors")
-
+
def test_unknown_error(self):
"An assertion is raised if the field doesn't contain the provided error"
post_data = {
@@ -211,7 +211,7 @@ def test_unknown_error(self):
self.assertFormError(response, 'form', 'email', 'Some error.')
except AssertionError, e:
self.assertEqual(str(e), "The field 'email' on form 'form' in context 0 does not contain the error 'Some error.' (actual errors: [u'Enter a valid e-mail address.'])")
-
+
def test_unknown_nonfield_error(self):
"""
Checks that an assertion is raised if the form's non field errors
@@ -231,7 +231,7 @@ def test_unknown_nonfield_error(self):
try:
self.assertFormError(response, 'form', None, 'Some error.')
except AssertionError, e:
- self.assertEqual(str(e), "The form 'form' in context 0 does not contain the non-field error 'Some error.' (actual errors: )")
+ self.assertEqual(str(e), "The form 'form' in context 0 does not contain the non-field error 'Some error.' (actual errors: )")
class FileUploadTests(TestCase):
def test_simple_upload(self):
@@ -256,8 +256,8 @@ def test_login_different_client(self):
# Get a redirection page with the second client.
response = c.get("/test_client_regress/login_protected_redirect_view/")
-
- # At this points, the self.client isn't logged in.
- # Check that assertRedirects uses the original client, not the
+
+ # At this points, the self.client isn't logged in.
+ # Check that assertRedirects uses the original client, not the
# default client.
self.assertRedirects(response, "http://testserver/test_client_regress/get_view/")
Please sign in to comment.
Something went wrong with that request. Please try again.