Opened 3 days ago
Closed 3 days ago
#37095 closed Bug (fixed)
URL redirect max length check should check %-encoded value
| Reported by: | Jacob Walls | Owned by: | Jacob Walls |
|---|---|---|---|
| Component: | HTTP handling | Version: | dev |
| Severity: | Release blocker | Keywords: | |
| Cc: | Varun Kasyap Pentamaraju, Jake Howard | Triage Stage: | Ready for checkin |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
In #36767 we made configurable the length check on redirect URLs we added for CVE-2025-64458. "Cowork" sent us a flurry of nuisance security reports last night, but among them was a reasonable suggestion that we apply the length check against the percent-encoded URI:
-
django/http/response.py
diff --git a/django/http/response.py b/django/http/response.py index 45fb0177d1..0a61fb723f 100644
a b class HttpResponseRedirectBase(HttpResponse): 641 641 **kwargs, 642 642 ): 643 643 super().__init__(*args, **kwargs) 644 self["Location"] = iri_to_uri(redirect_to)645 644 redirect_to_str = str(redirect_to) 646 if max_length is not None and len(redirect_to_str) > max_length: 645 location = iri_to_uri(redirect_to_str) 646 if max_length is not None and len(location) > max_length: 647 647 raise DisallowedRedirect( 648 648 f"Unsafe redirect exceeding {max_length} characters" 649 649 ) 650 self["Location"] = location 650 651 parsed = urlsplit(redirect_to_str) 651 652 if preserve_request: 652 653 self.status_code = self.status_code_preserve_request -
tests/httpwrappers/tests.py
diff --git a/tests/httpwrappers/tests.py b/tests/httpwrappers/tests.py index b990e9f816..3430dbbcd3 100644
a b from django.http import ( 24 24 parse_cookie, 25 25 ) 26 26 from django.test import SimpleTestCase 27 from django.utils.encoding import iri_to_uri 27 28 from django.utils.functional import lazystr 28 29 from django.utils.http import MAX_URL_REDIRECT_LENGTH 29 30 … … class HttpResponseTests(SimpleTestCase): 498 499 response = response_class(long_url) 499 500 self.assertEqual(response.url, long_url) 500 501 502 def test_redirect_url_max_length_checks_encoded_location(self): 503 long_url = "/" + "é" * (MAX_URL_REDIRECT_LENGTH - 1) 504 self.assertLessEqual(len(long_url), MAX_URL_REDIRECT_LENGTH) 505 self.assertGreater(len(iri_to_uri(long_url)), MAX_URL_REDIRECT_LENGTH) 506 for response_class in (HttpResponseRedirect, HttpResponsePermanentRedirect): 507 with ( 508 self.subTest(response_class=response_class), 509 self.assertRaisesMessage( 510 DisallowedRedirect, 511 f"Unsafe redirect exceeding {MAX_URL_REDIRECT_LENGTH} characters", 512 ), 513 ): 514 response_class(long_url) 515 516 def test_redirect_url_max_length_allows_encoded_location_at_limit(self): 517 redirect_to = "/" + "é" * ((MAX_URL_REDIRECT_LENGTH - 1) // 6) 518 location = iri_to_uri(redirect_to) 519 self.assertLessEqual(len(location), MAX_URL_REDIRECT_LENGTH) 520 for response_class in (HttpResponseRedirect, HttpResponsePermanentRedirect): 521 with self.subTest(response_class=response_class): 522 response = response_class(redirect_to) 523 self.assertEqual(response.url, location) 524 501 525 def test_redirect_url_max_length_override_via_param(self): 502 526 base_url = "https://example.com/" 503 527 for (max_length, length), response_class in itertools.product(
Although we didn't take this as a security issue (the limit we apply on the raw value is good enough for DoS), this seems like a functionality bug to fix before the release.
Change History (4)
comment:1 by , 3 days ago
| Cc: | added |
|---|---|
| Triage Stage: | Unreviewed → Accepted |
comment:2 by , 3 days ago
| Has patch: | set |
|---|
comment:3 by , 3 days ago
| Triage Stage: | Accepted → Ready for checkin |
|---|
Note:
See TracTickets
for help on using tickets.
PR