Better handling of 30X HTTP status codes

Reference: http://greenbytes.de/tech/tc/httpredirects/

src/HttpResponse.cc

@@ -75,27 +75,9 @@ HttpResponse::HttpResponse()
 void HttpResponse::validateResponse() const
 {
   int statusCode = getStatusCode();
-  if (statusCode >= 400) {
-    return;
-  }
-
-  if (statusCode == 304) {
-    if (!httpRequest_->conditionalRequest()) {
-      throw DL_ABORT_EX2("Got 304 without If-Modified-Since or If-None-Match",
-                         error_code::HTTP_PROTOCOL_ERROR);
-    }
-  }
-  else if (statusCode == 301 ||
-           statusCode == 302 ||
-           statusCode == 303 ||
-           statusCode == 307) {
-    if (!httpHeader_->defined(HttpHeader::LOCATION)) {
-      throw DL_ABORT_EX2(fmt(EX_LOCATION_HEADER_REQUIRED, statusCode),
-                         error_code::HTTP_PROTOCOL_ERROR);
-    }
-    return;
-  }
-  else if (statusCode == 200 || statusCode == 206) {
+  switch(statusCode) {
+  case 200: // OK
+  case 206: // Partial Content
     if (!httpHeader_->defined(HttpHeader::TRANSFER_ENCODING)) {
       // compare the received range against the requested range
       auto responseRange = httpHeader_->getRange();
@@ -110,11 +92,26 @@ void HttpResponse::validateResponse() const
                            error_code::CANNOT_RESUME);
       }
     }
+    return;
+  case 304: // Not Modified
+    if (!httpRequest_->conditionalRequest()) {
+      throw DL_ABORT_EX2("Got 304 without If-Modified-Since or If-None-Match",
+                         error_code::HTTP_PROTOCOL_ERROR);
+    }
+    return;
+  case 300: // Multiple Choices
+  case 301: // Moved Permanently
+  case 302: // Found
+  case 303: // See Other
+  case 307: // Temporary Redirect
+  case 308: // Permanent Redirect
+    return;
   }
-  else {
-    throw DL_ABORT_EX2(fmt("Unexpected status %d", statusCode),
-                       error_code::HTTP_PROTOCOL_ERROR);
+  if (statusCode >= 400) {
+    return;
   }
+  throw DL_ABORT_EX2(fmt("Unexpected status %d", statusCode),
+                     error_code::HTTP_PROTOCOL_ERROR);
 }
 
 std::string HttpResponse::determineFilename() const
@@ -151,9 +148,16 @@ void HttpResponse::retrieveCookie()
 
 bool HttpResponse::isRedirect() const
 {
-  auto code = getStatusCode();
-  return (301 == code || 302 == code || 303 == code || 307 == code) &&
-         httpHeader_->defined(HttpHeader::LOCATION);
+  switch(getStatusCode()) {
+  case 300: // Multiple Choices
+  case 301: // Moved Permanently
+  case 302: // Found
+  case 303: // See Other
+  case 307: // Temporary Redirect
+  case 308: // Permanent Redirect
+    return httpHeader_->defined(HttpHeader::LOCATION);
+  }
+  return false;
 }
 
 void HttpResponse::processRedirect()

src/HttpResponseCommand.cc

@@ -231,10 +231,11 @@ bool HttpResponseCommand::executeInternal()
 #endif // ENABLE_MESSAGE_DIGEST
   }
 
-  if (statusCode >= 300) {
-    if (statusCode == 404) {
-      grp->increaseAndValidateFileNotFoundCount();
-    }
+  if (statusCode == 404) {
+    grp->increaseAndValidateFileNotFoundCount();
+    return skipResponseBody(std::move(httpResponse));
+  }
+  if (statusCode >= 400 || statusCode == 304 || httpResponse->isRedirect()) {
     return skipResponseBody(std::move(httpResponse));
   }
 

test/HttpResponseTest.cc

@@ -220,16 +220,49 @@ void HttpResponseTest::testGetRedirectURI_with_Location()
 void HttpResponseTest::testIsRedirect()
 {
   HttpResponse httpResponse;
-  auto httpHeader = make_unique<HttpHeader>();
-  httpHeader->setStatusCode(200);
-  httpHeader->put(HttpHeader::LOCATION,
-                  "http://localhost/download/aria2-1.0.0.tar.bz2");
+  httpResponse.setHttpHeader(make_unique<HttpHeader>());
+
+  httpResponse.getHttpHeader()->setStatusCode(301);
+  CPPUNIT_ASSERT(!httpResponse.isRedirect());
+
+  httpResponse.getHttpHeader()->setStatusCode(200);
+  CPPUNIT_ASSERT(!httpResponse.isRedirect());
+
+  httpResponse.getHttpHeader()->put
+    (HttpHeader::LOCATION,
+     "http://localhost/download/aria2-1.0.0.tar.bz2");
 
-  httpResponse.setHttpHeader(std::move(httpHeader));
   CPPUNIT_ASSERT(!httpResponse.isRedirect());
 
+  httpResponse.getHttpHeader()->setStatusCode(300);
+  CPPUNIT_ASSERT(httpResponse.isRedirect());
+
   httpResponse.getHttpHeader()->setStatusCode(301);
   CPPUNIT_ASSERT(httpResponse.isRedirect());
+
+  httpResponse.getHttpHeader()->setStatusCode(302);
+  CPPUNIT_ASSERT(httpResponse.isRedirect());
+
+  httpResponse.getHttpHeader()->setStatusCode(303);
+  CPPUNIT_ASSERT(httpResponse.isRedirect());
+
+  httpResponse.getHttpHeader()->setStatusCode(304);
+  CPPUNIT_ASSERT(!httpResponse.isRedirect());
+
+  httpResponse.getHttpHeader()->setStatusCode(305);
+  CPPUNIT_ASSERT(!httpResponse.isRedirect());
+
+  httpResponse.getHttpHeader()->setStatusCode(306);
+  CPPUNIT_ASSERT(!httpResponse.isRedirect());
+
+  httpResponse.getHttpHeader()->setStatusCode(307);
+  CPPUNIT_ASSERT(httpResponse.isRedirect());
+
+  httpResponse.getHttpHeader()->setStatusCode(308);
+  CPPUNIT_ASSERT(httpResponse.isRedirect());
+
+  httpResponse.getHttpHeader()->setStatusCode(309);
+  CPPUNIT_ASSERT(!httpResponse.isRedirect());
 }
 
 void HttpResponseTest::testIsTransferEncodingSpecified()
@@ -329,19 +362,15 @@ void HttpResponseTest::testValidateResponse()
   httpResponse.setHttpHeader(make_unique<HttpHeader>());
   httpResponse.getHttpHeader()->setStatusCode(301);
 
-  try {
-    httpResponse.validateResponse();
-    CPPUNIT_FAIL("exception must be thrown.");
-  } catch(Exception& e) {
-  }
+  // It is fine without Location header
+  httpResponse.validateResponse();
 
-  httpResponse.getHttpHeader()->put
-    (HttpHeader::LOCATION,
-     "http://localhost/archives/aria2-1.0.0.tar.bz2");
-  try {
+  httpResponse.getHttpHeader()->setStatusCode(201);
+  try{
     httpResponse.validateResponse();
+    CPPUNIT_FAIL("exception must be thrown.");
   } catch(Exception& e) {
-    CPPUNIT_FAIL("exception must not be thrown.");
+    // success
   }
 }