Merge pull request #7445 from kenjis/fix-redirect-status-code

fix: redirect status code

Author: kenjis

system/HTTP/RedirectResponse.php

@@ -50,7 +50,7 @@ public function to(string $uri, ?int $code = null, string $method = 'auto')
      *
      * @throws HTTPException
      */
-    public function route(string $route, array $params = [], int $code = 302, string $method = 'auto')
+    public function route(string $route, array $params = [], ?int $code = null, string $method = 'auto')
     {
         $namedRoute = $route;
 

system/HTTP/ResponseTrait.php

@@ -494,29 +494,41 @@ public function sendBody()
     /**
      * Perform a redirect to a new URL, in two flavors: header or location.
      *
-     * @param string $uri  The URI to redirect to
-     * @param int    $code The type of redirection, defaults to 302
+     * @param string   $uri  The URI to redirect to
+     * @param int|null $code The type of redirection, defaults to 302
      *
      * @return $this
      *
      * @throws HTTPException For invalid status code.
      */
     public function redirect(string $uri, string $method = 'auto', ?int $code = null)
     {
-        // Assume 302 status code response; override if needed
-        if (empty($code)) {
-            $code = 302;
-        }
-
         // IIS environment likely? Use 'refresh' for better compatibility
-        if ($method === 'auto' && isset($_SERVER['SERVER_SOFTWARE']) && strpos($_SERVER['SERVER_SOFTWARE'], 'Microsoft-IIS') !== false) {
+        if (
+            $method === 'auto'
+            && isset($_SERVER['SERVER_SOFTWARE'])
+            && strpos($_SERVER['SERVER_SOFTWARE'], 'Microsoft-IIS') !== false
+        ) {
             $method = 'refresh';
+        } elseif ($method !== 'refresh' && $code === null) {
+            // override status code for HTTP/1.1 & higher
+            if (
+                isset($_SERVER['SERVER_PROTOCOL'], $_SERVER['REQUEST_METHOD'])
+                && $this->getProtocolVersion() >= 1.1
+            ) {
+                if ($_SERVER['REQUEST_METHOD'] === 'GET') {
+                    $code = 302;
+                } elseif (in_array($_SERVER['REQUEST_METHOD'], ['POST', 'PUT', 'DELETE'], true)) {
+                    // reference: https://en.wikipedia.org/wiki/Post/Redirect/Get
+                    $code = 303;
+                } else {
+                    $code = 307;
+                }
+            }
         }
 
-        // override status code for HTTP/1.1 & higher
-        // reference: http://en.wikipedia.org/wiki/Post/Redirect/Get
-        if (isset($_SERVER['SERVER_PROTOCOL'], $_SERVER['REQUEST_METHOD']) && $this->getProtocolVersion() >= 1.1 && $method !== 'refresh') {
-            $code = ($_SERVER['REQUEST_METHOD'] !== 'GET') ? 303 : ($code === 302 ? 307 : $code);
+        if ($code === null) {
+            $code = 302;
         }
 
         switch ($method) {

tests/system/CodeIgniterTest.php

@@ -446,15 +446,18 @@ public function testRunRedirectionWithURI()
     /**
      * @see https://github.com/codeigniter4/CodeIgniter4/issues/3041
      */
-    public function testRunRedirectionWithURINotSet()
+    public function testRunRedirectionWithGET()
     {
         $_SERVER['argv'] = ['index.php', 'example'];
         $_SERVER['argc'] = 2;
 
-        $_SERVER['REQUEST_URI'] = '/example';
+        $_SERVER['REQUEST_URI']     = '/example';
+        $_SERVER['SERVER_PROTOCOL'] = 'HTTP/1.1';
+        $_SERVER['REQUEST_METHOD']  = 'GET';
 
         // Inject mock router.
         $routes = Services::routes();
+        // addRedirect() sets status code 302 by default.
         $routes->addRedirect('example', 'pages/notset');
 
         $router = Services::router($routes, Services::incomingrequest());
@@ -463,11 +466,37 @@ public function testRunRedirectionWithURINotSet()
         ob_start();
         $this->codeigniter->run();
         ob_get_clean();
+
         $response = $this->getPrivateProperty($this->codeigniter, 'response');
         $this->assertSame('http://example.com/pages/notset', $response->header('Location')->getValue());
+        $this->assertSame(302, $response->getStatusCode());
+    }
+
+    public function testRunRedirectionWithGETAndHTTPCode301()
+    {
+        $_SERVER['argv'] = ['index.php', 'example'];
+        $_SERVER['argc'] = 2;
+
+        $_SERVER['REQUEST_URI']     = '/example';
+        $_SERVER['SERVER_PROTOCOL'] = 'HTTP/1.1';
+        $_SERVER['REQUEST_METHOD']  = 'GET';
+
+        // Inject mock router.
+        $routes = Services::routes();
+        $routes->addRedirect('example', 'pages/notset', 301);
+
+        $router = Services::router($routes, Services::incomingrequest());
+        Services::injectMock('router', $router);
+
+        ob_start();
+        $this->codeigniter->run();
+        ob_get_clean();
+
+        $response = $this->getPrivateProperty($this->codeigniter, 'response');
+        $this->assertSame(301, $response->getStatusCode());
     }
 
-    public function testRunRedirectionWithHTTPCode303()
+    public function testRunRedirectionWithPOSTAndHTTPCode301()
     {
         $_SERVER['argv'] = ['index.php', 'example'];
         $_SERVER['argc'] = 2;
@@ -488,7 +517,7 @@ public function testRunRedirectionWithHTTPCode303()
         ob_get_clean();
 
         $response = $this->getPrivateProperty($this->codeigniter, 'response');
-        $this->assertSame(303, $response->getStatusCode());
+        $this->assertSame(301, $response->getStatusCode());
     }
 
     public function testStoresPreviousURL()

tests/system/HTTP/ResponseTest.php

@@ -269,23 +269,84 @@ public function testRedirectSetsDefaultCodeAndLocationHeader()
         $this->assertSame(302, $response->getStatusCode());
     }
 
-    public function testRedirectSetsCode()
-    {
-        $response = new Response(new App());
+    /**
+     * @dataProvider provideForRedirect
+     */
+    public function testRedirect(
+        string $server,
+        string $protocol,
+        string $method,
+        ?int $code,
+        int $expectedCode
+    ) {
+        $_SERVER['SERVER_SOFTWARE'] = $server;
+        $_SERVER['SERVER_PROTOCOL'] = $protocol;
+        $_SERVER['REQUEST_METHOD']  = $method;
 
-        $response->redirect('example.com', 'auto', 307);
+        $response = new Response(new App());
+        $response->redirect('example.com', 'auto', $code);
 
         $this->assertTrue($response->hasHeader('location'));
         $this->assertSame('example.com', $response->getHeaderLine('Location'));
-        $this->assertSame(307, $response->getStatusCode());
+        $this->assertSame($expectedCode, $response->getStatusCode());
+    }
+
+    public function provideForRedirect()
+    {
+        yield from [
+            ['Apache/2.4.17', 'HTTP/1.1', 'GET', null, 302],
+            ['Apache/2.4.17', 'HTTP/1.1', 'GET', 307, 307],
+            ['Apache/2.4.17', 'HTTP/1.1', 'GET', 302, 302],
+            ['Apache/2.4.17', 'HTTP/1.1', 'POST', null, 303],
+            ['Apache/2.4.17', 'HTTP/1.1', 'POST', 307, 307],
+            ['Apache/2.4.17', 'HTTP/1.1', 'POST', 302, 302],
+            ['Apache/2.4.17', 'HTTP/1.1', 'HEAD', null, 307],
+            ['Apache/2.4.17', 'HTTP/1.1', 'HEAD', 307, 307],
+            ['Apache/2.4.17', 'HTTP/1.1', 'HEAD', 302, 302],
+            ['Apache/2.4.17', 'HTTP/1.1', 'OPTIONS', null, 307],
+            ['Apache/2.4.17', 'HTTP/1.1', 'OPTIONS', 307, 307],
+            ['Apache/2.4.17', 'HTTP/1.1', 'OPTIONS', 302, 302],
+            ['Apache/2.4.17', 'HTTP/1.1', 'PUT', null, 303],
+            ['Apache/2.4.17', 'HTTP/1.1', 'PUT', 307, 307],
+            ['Apache/2.4.17', 'HTTP/1.1', 'PUT', 302, 302],
+            ['Apache/2.4.17', 'HTTP/1.1', 'DELETE', null, 303],
+            ['Apache/2.4.17', 'HTTP/1.1', 'DELETE', 307, 307],
+            ['Apache/2.4.17', 'HTTP/1.1', 'DELETE', 302, 302],
+        ];
     }
 
-    public function testRedirectWithIIS()
-    {
+    /**
+     * @dataProvider provideForRedirectWithIIS
+     */
+    public function testRedirectWithIIS(
+        string $protocol,
+        string $method,
+        ?int $code,
+        int $expectedCode
+    ) {
         $_SERVER['SERVER_SOFTWARE'] = 'Microsoft-IIS';
-        $response                   = new Response(new App());
-        $response->redirect('example.com', 'auto', 307);
+        $_SERVER['SERVER_PROTOCOL'] = 'HTTP/1.1';
+        $_SERVER['REQUEST_METHOD']  = 'POST';
+
+        $response = new Response(new App());
+        $response->redirect('example.com', 'auto', $code);
+
         $this->assertSame('0;url=example.com', $response->getHeaderLine('Refresh'));
+        $this->assertSame($expectedCode, $response->getStatusCode());
+
+        unset($_SERVER['SERVER_SOFTWARE']);
+    }
+
+    public function provideForRedirectWithIIS()
+    {
+        yield from [
+            ['HTTP/1.1', 'GET', null, 302],
+            ['HTTP/1.1', 'GET', 307, 307],
+            ['HTTP/1.1', 'GET', 302, 302],
+            ['HTTP/1.1', 'POST', null, 302],
+            ['HTTP/1.1', 'POST', 307, 307],
+            ['HTTP/1.1', 'POST', 302, 302],
+        ];
     }
 
     public function testSetCookieFails()
@@ -458,7 +519,7 @@ public function testMisbehaving()
         $response->getStatusCode();
     }
 
-    public function testTemporaryRedirect11()
+    public function testTemporaryRedirectHTTP11()
     {
         $_SERVER['SERVER_PROTOCOL'] = 'HTTP/1.1';
         $_SERVER['REQUEST_METHOD']  = 'POST';
@@ -470,7 +531,7 @@ public function testTemporaryRedirect11()
         $this->assertSame(303, $response->getStatusCode());
     }
 
-    public function testTemporaryRedirectGet11()
+    public function testTemporaryRedirectGetHTTP11()
     {
         $_SERVER['SERVER_PROTOCOL'] = 'HTTP/1.1';
         $_SERVER['REQUEST_METHOD']  = 'GET';
@@ -479,7 +540,7 @@ public function testTemporaryRedirectGet11()
         $response->setProtocolVersion('HTTP/1.1');
         $response->redirect('/foo');
 
-        $this->assertSame(307, $response->getStatusCode());
+        $this->assertSame(302, $response->getStatusCode());
     }
 
     // Make sure cookies are set by RedirectResponse this way

user_guide_src/source/changelogs/v4.3.4.rst

@@ -12,6 +12,29 @@ Release Date: Unreleased
 BREAKING
 ********
 
+Behavior Changes
+================
+
+.. _v434-redirect-status-code:
+
+Redirect Status Code
+--------------------
+
+- Due to a bug, in previous versions, when using HTTP/1.1 or later the status
+  code of the actual redirect response might be changed even if a status code was
+  specified. For example, for a GET request, 302 would change to 307; for a POST
+  request, 307 and 302 would change to 303.
+- Starting with this version, if you specify a status code in
+  :ref:`redirect <response-redirect-status-code>`, that code will always be used
+  in the response.
+- The default code for GET requests has been corrected from 307 to 302 when using
+  HTTP/1.1 or later.
+- The default code for HEAD and OPTIONS requests has been corrected from 303 to
+  307 when using HTTP/1.1 or later.
+- In ``$routes->addRedirect()``, 302 is specified by default. Therefor 302 will
+  always be used when you don't specify a status code. In previous versions,
+  302 might be changed.
+
 Message Changes
 ***************
 

user_guide_src/source/general/common_functions.rst

@@ -320,48 +320,8 @@ Miscellaneous Functions
     :param  string  $route: The route name or Controller::method to redirect the user to.
     :rtype: RedirectResponse
 
-    .. important:: When you use this function, an instance of ``RedirectResponse`` must be returned
-        in the method of the :doc:`Controller <../incoming/controllers>` or
-        the :doc:`Controller Filter <../incoming/filters>`. If you forget to return it,
-        no redirection will occur.
-
     Returns a RedirectResponse instance allowing you to easily create redirects.
-
-    **Redirect to a URI path**
-
-    When you want to pass a URI path (relative to baseURL), use ``redirect()->to()``:
-
-    .. literalinclude:: common_functions/005.php
-        :lines: 2-
-
-    .. note:: If there is a fragment in your URL that you want to remove, you can use the refresh parameter in this function.
-        Like ``return redirect()->to('to', null, 'refresh');``.
-
-    **Redirect to a Defined Route**
-
-    When you want to pass a :ref:`route name <using-named-routes>` or Controller::method
-    for :ref:`reverse routing <reverse-routing>`, use ``redirect()->route()``:
-
-    .. literalinclude:: common_functions/013.php
-        :lines: 2-
-
-    When passing an argument into the function, it is treated as a route name or
-    Controller::method for reverse routing, not a relative/full URI,
-    treating it the same as using ``redirect()->route()``:
-
-    .. literalinclude:: common_functions/006.php
-        :lines: 2-
-
-    **Redirect Back**
-
-    When you want to redirect back, use ``redirect()->back()``:
-
-    .. literalinclude:: common_functions/014.php
-        :lines: 2-
-
-    .. note:: ``redirect()->back()`` is not the same as browser "back" button.
-        It takes a visitor to "the last page viewed during the Session" when the Session is available.
-        If the Session hasn’t been loaded, or is otherwise unavailable, then a sanitized version of HTTP_REFERER will be used.
+    See :ref:`response-redirect` for details.
 
 .. php:function:: remove_invisible_characters($str[, $urlEncoded = true])
 

user_guide_src/source/installation/upgrade_434.rst

@@ -18,6 +18,13 @@ Mandatory File Changes
 Breaking Changes
 ****************
 
+Redirect Status Code
+====================
+
+- Due to a bug fix, the status codes of redirects may be changed. See
+  :ref:`ChangeLog v4.3.4 <v434-redirect-status-code>` and if the code is not
+  what you want, :ref:`specify status codes <response-redirect-status-code>`.
+
 Breaking Enhancements
 *********************