Better follow redirection for HTTPClient (#7157)

* Add way to force follow redirections in `HTTPClient`

* Follow other client implementations about `HTTP_CODE_FOUND`; Small rewrite of `sendRequest` function of `HTTPClient`

* Better names for follow redirection modes in `HTTPClient`

Also changed a bit order of the enums (0 element to be DISABLED)

* Rewrite `sendRequest` to remove recursion

Also got rid of unnecessary `redirectCount` field. Now redirect counting and limiting is handled in `sendRequest` directly.

* Use new `setFollowRedirects` of `HTTPClient` instead deprecated one.

* More explanatory comment for `followRedirects_t` in HTTPClient

libraries/ESP8266HTTPClient/src/ESP8266HTTPClient.cpp

@@ -423,7 +423,6 @@ void HTTPClient::end(void)
 {
     disconnect(false);
     clear();
-    _redirectCount = 0;
 }
 
 /**
@@ -558,8 +557,17 @@ bool HTTPClient::setURL(const String& url)
 /**
  * set true to follow redirects.
  * @param follow
+ * @deprecated
  */
 void HTTPClient::setFollowRedirects(bool follow)
+{
+    _followRedirects = follow ? HTTPC_STRICT_FOLLOW_REDIRECTS : HTTPC_DISABLE_FOLLOW_REDIRECTS;
+}
+/**
+ * set redirect follow mode. See `followRedirects_t` enum for avaliable modes.
+ * @param follow
+ */
+void HTTPClient::setFollowRedirects(followRedirects_t follow)
 {
     _followRedirects = follow;
 }
@@ -652,8 +660,9 @@ int HTTPClient::sendRequest(const char * type, const String& payload)
  */
 int HTTPClient::sendRequest(const char * type, const uint8_t * payload, size_t size)
 {
+    int code;
     bool redirect = false;
-    int code = 0;
+    uint16_t redirectCount = 0;
     do {
         // wipe out any existing headers from previous request
         for(size_t i = 0; i < _headerKeysCount; i++) {
@@ -662,8 +671,7 @@ int HTTPClient::sendRequest(const char * type, const uint8_t * payload, size_t s
             }
         }
 
-        redirect = false;
+        DEBUG_HTTPCLIENT("[HTTP-Client][sendRequest] type: '%s' redirCount: %d\n", type, redirectCount);
-        DEBUG_HTTPCLIENT("[HTTP-Client][sendRequest] type: '%s' redirCount: %d\n", type, _redirectCount);
 
         // connect to server
         if(!connect()) {
@@ -687,9 +695,9 @@ int HTTPClient::sendRequest(const char * type, const uint8_t * payload, size_t s
                 int towrite = std::min((int)size, (int)HTTP_TCP_BUFFER_SIZE);
                 written = _client->write(p + bytesWritten, towrite);
                 if (written < 0) {
-                     return returnError(HTTPC_ERROR_SEND_PAYLOAD_FAILED);
+                        return returnError(HTTPC_ERROR_SEND_PAYLOAD_FAILED);
                 } else if (written == 0) {
-                     return returnError(HTTPC_ERROR_CONNECTION_LOST);
+                        return returnError(HTTPC_ERROR_CONNECTION_LOST);
                 }
                 bytesWritten += written;
                 size -= written;
@@ -700,42 +708,67 @@ int HTTPClient::sendRequest(const char * type, const uint8_t * payload, size_t s
         code = handleHeaderResponse();
 
         //
-        // We can follow redirects for 301/302/307 for GET and HEAD requests and
+        // Handle redirections as stated in RFC document:
-        // and we have not exceeded the redirect limit preventing an infinite
-        // redirect loop.
-        //
         // https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html
         //
-        if (_followRedirects &&
+        // Implementing HTTP_CODE_FOUND as redirection with GET method,
-                (_redirectCount < _redirectLimit) &&
+        // to follow most of existing user agent implementations.
-                (_location.length() > 0) &&
+        //
-                (code == 301 || code == 302 || code == 307) &&
+        redirect = false;
-                (!strcmp(type, "GET") || !strcmp(type, "HEAD"))
+        if (
-                ) {
+            _followRedirects != HTTPC_DISABLE_FOLLOW_REDIRECTS && 
-            _redirectCount += 1; // increment the count for redirect.
+            redirectCount < _redirectLimit &&
-            redirect = true;
+            _location.length() > 0
-            DEBUG_HTTPCLIENT("[HTTP-Client][sendRequest] following redirect:: '%s' redirCount: %d\n", _location.c_str(), _redirectCount);
+        ) {
-            if (!setURL(_location)) {
+            switch (code) {
-                // return the redirect instead of handling on failure of setURL()
+                // redirecting using the same method
-                redirect = false;
+                case HTTP_CODE_MOVED_PERMANENTLY:
+                case HTTP_CODE_TEMPORARY_REDIRECT: {
+                    if (
+                        // allow to force redirections on other methods
+                        // (the RFC require user to accept the redirection)
+                        _followRedirects == HTTPC_FORCE_FOLLOW_REDIRECTS ||
+                        // allow GET and HEAD methods without force
+                        !strcmp(type, "GET") || 
+                        !strcmp(type, "HEAD")
+                    ) {
+                        redirectCount += 1;
+                        DEBUG_HTTPCLIENT("[HTTP-Client][sendRequest] following redirect (the same method): '%s' redirCount: %d\n", _location.c_str(), redirectCount);
+                        if (!setURL(_location)) {
+                            DEBUG_HTTPCLIENT("[HTTP-Client][sendRequest] failed setting URL for redirection\n");
+                            // no redirection
+                            break;
+                        }
+                        // redirect using the same request method and payload, diffrent URL
+                        redirect = true;
+                    }
+                    break;
+                }
+                // redirecting with method dropped to GET or HEAD
+                // note: it does not need `HTTPC_FORCE_FOLLOW_REDIRECTS` for any method
+                case HTTP_CODE_FOUND:
+                case HTTP_CODE_SEE_OTHER: {
+                    redirectCount += 1;
+                    DEBUG_HTTPCLIENT("[HTTP-Client][sendRequest] following redirect (dropped to GET/HEAD): '%s' redirCount: %d\n", _location.c_str(), redirectCount);
+                    if (!setURL(_location)) {
+                        DEBUG_HTTPCLIENT("[HTTP-Client][sendRequest] failed setting URL for redirection\n");
+                        // no redirection
+                        break;
+                    }
+                    // redirect after changing method to GET/HEAD and dropping payload
+                    type = "GET";
+                    payload = nullptr;
+                    size = 0;
+                    redirect = true;
+                    break;
+                }
+
+                default:
+                    break;
             }
         }
-
     } while (redirect);
 
-    // handle 303 redirect for non GET/HEAD by changing to GET and requesting new url
-    if (_followRedirects &&
-            (_redirectCount < _redirectLimit) &&
-            (_location.length() > 0) &&
-            (code == 303) &&
-            strcmp(type, "GET") && strcmp(type, "HEAD")
-            ) {
-        _redirectCount += 1;
-        if (setURL(_location)) {
-            code = sendRequest("GET");
-        }
-    }
-
     // handle Server Response (Header)
     return returnError(code);
 }

libraries/ESP8266HTTPClient/src/ESP8266HTTPClient.h

@@ -130,6 +130,23 @@ typedef enum {
     HTTPC_TE_CHUNKED
 } transferEncoding_t;
 
+/**
+ * redirection follow mode.
+ * + `HTTPC_DISABLE_FOLLOW_REDIRECTS` - no redirection will be followed.
+ * + `HTTPC_STRICT_FOLLOW_REDIRECTS` - strict RFC2616, only requests using
+ *      GET or HEAD methods will be redirected (using the same method),
+ *      since the RFC requires end-user confirmation in other cases.
+ * + `HTTPC_FORCE_FOLLOW_REDIRECTS` - all redirections will be followed,
+ *      regardless of a used method. New request will use the same method,
+ *      and they will include the same body data and the same headers.
+ *      In the sense of the RFC, it's just like every redirection is confirmed.
+ */
+typedef enum {
+    HTTPC_DISABLE_FOLLOW_REDIRECTS,
+    HTTPC_STRICT_FOLLOW_REDIRECTS,
+    HTTPC_FORCE_FOLLOW_REDIRECTS
+} followRedirects_t;
+
 #if HTTPCLIENT_1_1_COMPATIBLE
 class TransportTraits;
 typedef std::unique_ptr<TransportTraits> TransportTraitsPtr;
@@ -173,8 +190,12 @@ public:
     void setAuthorization(const char * user, const char * password);
     void setAuthorization(const char * auth);
     void setTimeout(uint16_t timeout);
-    void setFollowRedirects(bool follow);
+
+    // Redirections
+    void setFollowRedirects(bool follow) __attribute__ ((deprecated));
+    void setFollowRedirects(followRedirects_t follow);
     void setRedirectLimit(uint16_t limit); // max redirects to follow for a single request
+
     bool setURL(const String& url); // handy for handling redirects
     void useHTTP10(bool usehttp10 = true);
 
@@ -252,8 +273,7 @@ protected:
     int _returnCode = 0;
     int _size = -1;
     bool _canReuse = false;
-    bool _followRedirects = false;
+    followRedirects_t _followRedirects = HTTPC_DISABLE_FOLLOW_REDIRECTS;
-    uint16_t _redirectCount = 0;
     uint16_t _redirectLimit = 10;
     String _location;
     transferEncoding_t _transferEncoding = HTTPC_TE_IDENTITY;

libraries/ESP8266httpUpdate/src/ESP8266httpUpdate.cpp

@@ -30,12 +30,12 @@ extern "C" uint32_t _FS_start;
 extern "C" uint32_t _FS_end;
 
 ESP8266HTTPUpdate::ESP8266HTTPUpdate(void)
-        : _httpClientTimeout(8000), _followRedirects(false), _ledPin(-1)
+        : _httpClientTimeout(8000), _followRedirects(HTTPC_DISABLE_FOLLOW_REDIRECTS), _ledPin(-1)
 {
 }
 
 ESP8266HTTPUpdate::ESP8266HTTPUpdate(int httpClientTimeout)
-        : _httpClientTimeout(httpClientTimeout), _followRedirects(false), _ledPin(-1)
+        : _httpClientTimeout(httpClientTimeout), _followRedirects(HTTPC_DISABLE_FOLLOW_REDIRECTS), _ledPin(-1)
 {
 }
 

libraries/ESP8266httpUpdate/src/ESP8266httpUpdate.h

@@ -82,7 +82,20 @@ public:
         _rebootOnUpdate = reboot;
     }
 
-    void followRedirects(bool follow)
+    /**
+     * set true to follow redirects.
+     * @param follow
+     * @deprecated Please use `setFollowRedirects(followRedirects_t follow)`
+      */
+    void followRedirects(bool follow) __attribute__ ((deprecated))
+    {
+        _followRedirects = follow ? HTTPC_STRICT_FOLLOW_REDIRECTS : HTTPC_DISABLE_FOLLOW_REDIRECTS;
+    }
+    /**
+      * set redirect follow mode. See `followRedirects_t` enum for avaliable modes.
+      * @param follow
+      */
+    void setFollowRedirects(followRedirects_t follow)
     {
         _followRedirects = follow;
     }
@@ -160,7 +173,7 @@ protected:
     bool _closeConnectionsOnUpdate = true;
 private:
     int _httpClientTimeout;
-    bool _followRedirects;
+    followRedirects_t _followRedirects;
 
     // Callbacks
     HTTPUpdateStartCB    _cbStart;

tests/device/test_sw_http_client/test_sw_http_client.ino

@@ -71,10 +71,10 @@ TEST_CASE("HTTP GET & POST requests", "[HTTPClient]")
         http.end();
     }
     {
-        // 301 redirect with follow enabled
+        // GET 301 redirect with strict RFC follow enabled 
         WiFiClient client;
         HTTPClient http;
-        http.setFollowRedirects(true);
+        http.setFollowRedirects(HTTPC_STRICT_FOLLOW_REDIRECTS);
         String uri = String("/redirect301?host=")+getenv("SERVER_IP");
         http.begin(client, getenv("SERVER_IP"), 8088, uri.c_str());
         auto httpCode = http.GET();
@@ -83,7 +83,7 @@ TEST_CASE("HTTP GET & POST requests", "[HTTPClient]")
         REQUIRE(payload == "redirect success");
     }
     {
-        // 301 redirect with follow disabled
+        // GET 301 redirect with follow disabled
         WiFiClient client;
         HTTPClient http;
         String uri = String("/redirect301?host=")+getenv("SERVER_IP");
@@ -92,10 +92,10 @@ TEST_CASE("HTTP GET & POST requests", "[HTTPClient]")
         REQUIRE(httpCode == 301);
     }
     {
-        // 302 redirect with follow enabled
+        // GET 302 redirect with strict RFC follow enabled 
         WiFiClient client;
         HTTPClient http;
-        http.setFollowRedirects(true);
+        http.setFollowRedirects(HTTPC_STRICT_FOLLOW_REDIRECTS);
         String uri = String("/redirect302?host=")+getenv("SERVER_IP");
         http.begin(client, getenv("SERVER_IP"), 8088, uri.c_str());
         auto httpCode = http.GET();
@@ -104,7 +104,7 @@ TEST_CASE("HTTP GET & POST requests", "[HTTPClient]")
         REQUIRE(payload == "redirect success");
     }
     {
-        // 302 redirect with follow disabled
+        // GET 302 redirect with follow disabled
         WiFiClient client;
         HTTPClient http;
         String uri = String("/redirect302?host=")+getenv("SERVER_IP");
@@ -113,10 +113,10 @@ TEST_CASE("HTTP GET & POST requests", "[HTTPClient]")
         REQUIRE(httpCode == 302);
     }
     {
-        // 307 redirect with follow enabled
+        // GET 307 redirect with strict RFC follow enabled
         WiFiClient client;
         HTTPClient http;
-        http.setFollowRedirects(true);
+        http.setFollowRedirects(HTTPC_STRICT_FOLLOW_REDIRECTS);
         String uri = String("/redirect307?host=")+getenv("SERVER_IP");
         http.begin(client, getenv("SERVER_IP"), 8088, uri.c_str());
         auto httpCode = http.GET();
@@ -125,7 +125,7 @@ TEST_CASE("HTTP GET & POST requests", "[HTTPClient]")
         REQUIRE(payload == "redirect success");
     }
     {
-        // 307 redirect with follow disabled
+        // GET 307 redirect with follow disabled
         WiFiClient client;
         HTTPClient http;
         String uri = String("/redirect307?host=")+getenv("SERVER_IP");
@@ -134,10 +134,10 @@ TEST_CASE("HTTP GET & POST requests", "[HTTPClient]")
         REQUIRE(httpCode == 307);
     }
     {
-        // 301 exceeding redirect limit
+        // GET 301 exceeding redirect limit
         WiFiClient client;
         HTTPClient http;
-        http.setFollowRedirects(true);
+        http.setFollowRedirects(HTTPC_STRICT_FOLLOW_REDIRECTS);
         http.setRedirectLimit(0);
         String uri = String("/redirect301?host=")+getenv("SERVER_IP");
         http.begin(client, getenv("SERVER_IP"), 8088, uri.c_str());
@@ -145,20 +145,22 @@ TEST_CASE("HTTP GET & POST requests", "[HTTPClient]")
         REQUIRE(httpCode == 301);
     }
     {
-        // POST 303 redirect with follow enabled
+        // POST 303 redirect with strict RFC follow enabled
         WiFiClient client;
         HTTPClient http;
-        http.setFollowRedirects(true);
+        http.setFollowRedirects(HTTPC_STRICT_FOLLOW_REDIRECTS);
         http.begin(client, getenv("SERVER_IP"), 8088, "/redirect303");
         auto httpCode = http.POST(getenv("SERVER_IP"));
         REQUIRE(httpCode == HTTP_CODE_OK);
         String payload = http.getString();
         REQUIRE(payload == "redirect success");
+        // TODO: need check for dropping: redirection should use GET method
     }
     {
         // POST 303 redirect with follow disabled
         WiFiClient client;
         HTTPClient http;
+        http.setFollowRedirects(HTTPC_DISABLE_FOLLOW_REDIRECTS);
         http.begin(client, getenv("SERVER_IP"), 8088, "/redirect303");
         auto httpCode = http.POST(getenv("SERVER_IP"));
         REQUIRE(httpCode == 303);
@@ -167,6 +169,7 @@ TEST_CASE("HTTP GET & POST requests", "[HTTPClient]")
         // 302 redirect with follow disabled
         WiFiClient client;
         HTTPClient http;
+        http.setFollowRedirects(HTTPC_DISABLE_FOLLOW_REDIRECTS);
         String uri = String("/redirect302?host=")+getenv("SERVER_IP");
         http.begin(client, getenv("SERVER_IP"), 8088, uri.c_str());
         auto httpCode = http.GET();