Skip to content

Commit

Permalink
Fix http status of the response
Browse files Browse the repository at this point in the history
The http status of the response is changed from 301 (Moved Permanently)
to 302 (Found) for GET requests and to 307 (Temporary Redirect) for
other request methods because nothing prevents the URL to be reused in
the future.

This was inspired by plone/plone.rest#76

Refs #8
  • Loading branch information
ale-rt committed Feb 14, 2020
1 parent 8466ac5 commit 22399a6
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 10 deletions.
1 change: 1 addition & 0 deletions news/8.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
The http status of the response is changed from 301 (Moved Permanently) to 302 (Found) for GET requests and to 307 (Temporary Redirect) for other request methods because nothing prevents the URL to be reused in the future. [ale-rt]
12 changes: 11 additions & 1 deletion plone/app/redirector/browser.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,17 @@ def attempt_redirect(self):
# some analytics programs might use this info to track
if query_string:
url += "?" + query_string
self.request.response.redirect(url, status=301, lock=1)

# Answer GET requests with 302 (Found). Every other method will be answered
# with 307 (Temporary Redirect), which instructs the client to NOT
# switch the method (if the original request was a POST, it should
# re-POST to the new URL from the Location header).
if self.request.method.upper() == "GET":
status = 302

This comment has been minimized.

Copy link
@erral

erral Jun 21, 2023

Member

@ale-rt why did we changed this from a 301 to a 302 redirect?

plone.rest example is used as an argument to issue a 307 for POST requests, but here we changed from "permanent" to "temporary" redirects when in plone.rest we keep having "permanents"

@garaolaza @libargutxi

This comment has been minimized.

Copy link
@garaolaza

garaolaza Jun 21, 2023

I have read about this issue at #22 (comment)

The background of the question is philosophical about the meaning of "Permanent", so we are not going further and will apply a local fix for our Plone sites to return a 301

else:
status = 307

self.request.response.redirect(url, status=status, lock=1)
return True

def find_redirect_if_view(self, old_path_elements, storage):
Expand Down
18 changes: 9 additions & 9 deletions plone/app/redirector/tests/test_view.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ def test_attempt_redirect_with_known_url(self):
self.storage.add(fp + '/foo', fp + '/bar')
view = self.view(self.portal, fu + '/foo')
self.assertEqual(True, view.attempt_redirect())
self.assertEqual(301, self.request.response.getStatus())
self.assertEqual(302, self.request.response.getStatus())
self.assertEqual(fu + '/bar',
self.request.response.getHeader('location'))

Expand All @@ -48,7 +48,7 @@ def test_attempt_redirect_with_known_url_and_template(self):
self.storage.add(fp + '/foo', fp + '/bar')
view = self.view(self.portal, fu + '/foo/view')
self.assertEqual(True, view.attempt_redirect())
self.assertEqual(301, self.request.response.getStatus())
self.assertEqual(302, self.request.response.getStatus())
self.assertEqual(fu + '/bar/view',
self.request.response.getHeader('location'))

Expand All @@ -58,29 +58,29 @@ def test_attempt_redirect_with_known_url_and_view_with_part(self):
self.storage.add(fp + '/foo', fp + '/bar')
view = self.view(self.portal, fu + '/foo/@@view/part')
self.assertEqual(True, view.attempt_redirect())
self.assertEqual(301, self.request.response.getStatus())
self.assertEqual(302, self.request.response.getStatus())
self.assertEqual(fu + '/bar/@@view/part',
self.request.response.getHeader('location'))

def test_attempt_redirect_with_unknown_url(self):
fu = self.folder.absolute_url()
view = self.view(self.portal, fu + '/foo')
self.assertEqual(False, view.attempt_redirect())
self.assertNotEqual(301, self.request.response.getStatus())
self.assertNotEqual(302, self.request.response.getStatus())

def test_attempt_redirect_with_unknown_url_with_illegal_characters(self):
fu = self.folder.absolute_url()
view = self.view(self.portal, fu + '+Länder')
self.assertEqual(False, view.attempt_redirect())
self.assertNotEqual(301, self.request.response.getStatus())
self.assertNotEqual(302, self.request.response.getStatus())

def test_attempt_redirect_with_quoted_url(self):
fp = '/'.join(self.folder.getPhysicalPath())
fu = self.folder.absolute_url()
self.storage.add(fp + '/foo', fp + '/bar')
view = self.view(self.portal, fu + '/foo/baz%20quux')
self.assertEqual(True, view.attempt_redirect())
self.assertEqual(301, self.request.response.getStatus())
self.assertEqual(302, self.request.response.getStatus())
self.assertEqual(fu + '/bar/baz%20quux',
self.request.response.getHeader('location'))

Expand All @@ -90,7 +90,7 @@ def test_attempt_redirect_with_query_string(self):
self.storage.add(fp + '/foo?blah=blah', fp + '/bar')
view = self.view(self.portal, fu + '/foo', 'blah=blah')
self.assertEqual(True, view.attempt_redirect())
self.assertEqual(301, self.request.response.getStatus())
self.assertEqual(302, self.request.response.getStatus())
self.assertEqual(fu + '/bar',
self.request.response.getHeader('location'))

Expand All @@ -100,7 +100,7 @@ def test_attempt_redirect_appending_query_string(self):
self.storage.add(fp + '/foo', fp + '/bar')
view = self.view(self.portal, fu + '/foo', 'blah=blah')
self.assertEqual(True, view.attempt_redirect())
self.assertEqual(301, self.request.response.getStatus())
self.assertEqual(302, self.request.response.getStatus())
self.assertEqual(fu + '/bar?blah=blah',
self.request.response.getHeader('location'))

Expand All @@ -111,7 +111,7 @@ def test_attempt_redirect_with_external_url(self):
'http://otherhost' + fp + '/bar%20qux corge')
view = self.view(self.portal, fu + '/foo')
self.assertEqual(True, view.attempt_redirect())
self.assertEqual(301, self.request.response.getStatus())
self.assertEqual(302, self.request.response.getStatus())
self.assertEqual('http://otherhost' + fp + '/bar%20qux%20corge',
self.request.response.getHeader('location'))

Expand Down

0 comments on commit 22399a6

Please sign in to comment.