Merge pull request #738 from kiwix/HTTPErrorHtmlResponse

src/server/internalServer.cpp

@@ -359,10 +359,12 @@ std::unique_ptr<Response> InternalServer::handle_request(const RequestContext& r
     return handle_content(request);
   } catch (std::exception& e) {
     fprintf(stderr, "===== Unhandled error : %s\n", e.what());
-    return Response::build_500(*this, e.what());
+    return HTTP500HtmlResponse(*this, request)
+         + e.what();
   } catch (...) {
     fprintf(stderr, "===== Unhandled unknown error\n");
-    return Response::build_500(*this, "Unknown error");
+    return HTTP500HtmlResponse(*this, request)
+         + "Unknown error";
   }
 }
 
@@ -593,7 +595,8 @@ std::unique_ptr<Response> InternalServer::handle_search(const RequestContext& re
       data.set("pattern", encodeDiples(searchInfo.pattern));
       auto response = ContentResponse::build(*this, RESOURCE::templates::no_search_result_html, data, "text/html; charset=utf-8");
       response->set_code(MHD_HTTP_NOT_FOUND);
-      return withTaskbarInfo(searchInfo.bookName, archive.get(), std::move(response));
+      response->set_taskbar(searchInfo.bookName, archive.get());
+      return std::move(response);
     }
 
 
@@ -622,14 +625,16 @@ std::unique_ptr<Response> InternalServer::handle_search(const RequestContext& re
     renderer.setSearchProtocolPrefix(m_root + "/search?");
     renderer.setPageLength(pageLength);
     auto response = ContentResponse::build(*this, renderer.getHtml(), "text/html; charset=utf-8");
-    return withTaskbarInfo(searchInfo.bookName, archive.get(), std::move(response));
+    response->set_taskbar(searchInfo.bookName, archive.get());
+    return std::move(response);
   } catch (const std::invalid_argument& e) {
     return HTTP400HtmlResponse(*this, request)
       + invalidUrlMsg
       + std::string(e.what());
   } catch (const std::exception& e) {
     std::cerr << e.what() << std::endl;
-    return Response::build_500(*this, e.what());
+    return HTTP500HtmlResponse(*this, request)
+         + e.what();
   }
 }
 
@@ -660,8 +665,9 @@ std::unique_ptr<Response> InternalServer::handle_random(const RequestContext& re
     return build_redirect(bookName, getFinalItem(*archive, entry));
   } catch(zim::EntryNotFound& e) {
     const std::string error_details = "Oops! Failed to pick a random article :(";
-    auto response = Response::build_404(*this, "", error_details);
+    return HTTP404HtmlResponse(*this, request)
-    return withTaskbarInfo(bookName, archive.get(), std::move(response));
+           + error_details
+           + TaskbarInfo(bookName, archive.get());
   }
 }
 
@@ -838,11 +844,11 @@ std::unique_ptr<Response> InternalServer::handle_content(const RequestContext& r
   } catch (const std::out_of_range& e) {}
 
   if (archive == nullptr) {
-    std::string searchURL = m_root + "/search?pattern=" + kiwix::urlEncode(pattern, true); // Make a full search on the entire library.
+    const std::string searchURL = m_root + "/search?pattern=" + kiwix::urlEncode(pattern, true);
-    const std::string details = searchSuggestionHTML(searchURL, kiwix::urlDecode(pattern));
+    return HTTP404HtmlResponse(*this, request)
-
+           + urlNotFoundMsg
-    auto response = Response::build_404(*this, request.get_full_url(), details);
+           + searchSuggestionHTML(searchURL, kiwix::urlDecode(pattern))
-    return withTaskbarInfo(bookName, nullptr, std::move(response));
+           + TaskbarInfo(bookName);
   }
 
   auto urlStr = request.get_url().substr(bookName.size()+1);
@@ -872,11 +878,11 @@ std::unique_ptr<Response> InternalServer::handle_content(const RequestContext& r
     if (m_verbose.load())
       printf("Failed to find %s\n", urlStr.c_str());
 
-    std::string searchURL = m_root + "/search?content=" + bookName + "&pattern=" + kiwix::urlEncode(pattern, true); // Make a search on this specific book only.
+    std::string searchURL = m_root + "/search?content=" + bookName + "&pattern=" + kiwix::urlEncode(pattern, true);
-    const std::string details = searchSuggestionHTML(searchURL, kiwix::urlDecode(pattern));
+    return HTTP404HtmlResponse(*this, request)
-
+           + urlNotFoundMsg
-    auto response = Response::build_404(*this, request.get_full_url(), details);
+           + searchSuggestionHTML(searchURL, kiwix::urlDecode(pattern))
-    return withTaskbarInfo(bookName, archive.get(), std::move(response));
+           + TaskbarInfo(bookName, archive.get());
   }
 }
 
@@ -899,7 +905,9 @@ std::unique_ptr<Response> InternalServer::handle_raw(const RequestContext& reque
 
   if (kind != "meta" && kind!= "content") {
     const std::string error_details = kind + " is not a valid request for raw content.";
-    return Response::build_404(*this, request.get_full_url(), error_details);
+    return HTTP404HtmlResponse(*this, request)
+           + urlNotFoundMsg
+           + error_details;
   }
 
   std::shared_ptr<zim::Archive> archive;
@@ -936,7 +944,9 @@ std::unique_ptr<Response> InternalServer::handle_raw(const RequestContext& reque
       printf("Failed to find %s\n", itemPath.c_str());
     }
     const std::string error_details = "Cannot find " + kind + " entry " + itemPath;
-    return Response::build_404(*this, request.get_full_url(), error_details);
+    return HTTP404HtmlResponse(*this, request)
+           + urlNotFoundMsg
+           + error_details;
   }
 }
 

src/server/internalServer.h

@@ -177,8 +177,6 @@ class InternalServer {
     friend std::unique_ptr<Response> Response::build(const InternalServer& server);
     friend std::unique_ptr<ContentResponse> ContentResponse::build(const InternalServer& server, const std::string& content, const std::string& mimetype, bool isHomePage, bool raw);
     friend std::unique_ptr<Response> ItemResponse::build(const InternalServer& server, const RequestContext& request, const zim::Item& item, bool raw);
-    friend std::unique_ptr<Response> Response::build_500(const InternalServer& server, const std::string& msg);
-
 };
 
 }

src/server/response.cpp

@@ -84,31 +84,6 @@ std::unique_ptr<Response> Response::build_304(const InternalServer& server, cons
   return response;
 }
 
-kainjow::mustache::data make404ResponseData(const std::string& url, const std::string& details)
-{
-  kainjow::mustache::list pList;
-  if ( !url.empty() ) {
-    kainjow::mustache::mustache msgTmpl(R"(The requested URL "{{url}}" was not found on this server.)");
-    const auto urlNotFoundMsg = msgTmpl.render({"url", url});
-    pList.push_back({"p", urlNotFoundMsg});
-  }
-  pList.push_back({"p", details});
-  return {"details", pList};
-}
-
-std::unique_ptr<ContentResponse> Response::build_404(const InternalServer& server, const std::string& url, const std::string& details)
-{
-  return build_404(server, make404ResponseData(url, details));
-}
-
-std::unique_ptr<ContentResponse> Response::build_404(const InternalServer& server, const kainjow::mustache::data& data)
-{
-  auto response = ContentResponse::build(server, RESOURCE::templates::_404_html, data, "text/html");
-  response->set_code(MHD_HTTP_NOT_FOUND);
-
-  return response;
-}
-
 const UrlNotFoundMsg urlNotFoundMsg;
 const InvalidUrlMsg invalidUrlMsg;
 
@@ -116,31 +91,49 @@ std::unique_ptr<ContentResponse> ContentResponseBlueprint::generateResponseObjec
 {
   auto r = ContentResponse::build(m_server, m_template, m_data, m_mimeType);
   r->set_code(m_httpStatusCode);
-  return m_taskbarInfo
+  if ( m_taskbarInfo ) {
-       ? withTaskbarInfo(m_taskbarInfo->bookName, m_taskbarInfo->archive, std::move(r))
+    r->set_taskbar(m_taskbarInfo->bookName, m_taskbarInfo->archive);
-       : std::move(r);
+  }
+  return r;
+}
+
+HTTPErrorHtmlResponse::HTTPErrorHtmlResponse(const InternalServer& server,
+                                             const RequestContext& request,
+                                             int httpStatusCode,
+                                             const std::string& pageTitleMsg,
+                                             const std::string& headingMsg)
+  : ContentResponseBlueprint(&server,
+                             &request,
+                             httpStatusCode,
+                             "text/html; charset=utf-8",
+                             RESOURCE::templates::error_html)
+{
+  kainjow::mustache::list emptyList;
+  this->m_data = kainjow::mustache::object{
+                    {"PAGE_TITLE",   pageTitleMsg},
+                    {"PAGE_HEADING", headingMsg},
+                    {"details", emptyList}
+  };
 }
 
 HTTP404HtmlResponse::HTTP404HtmlResponse(const InternalServer& server,
-                                         const RequestContext& request)
+                                             const RequestContext& request)
-  : ContentResponseBlueprint(&server,
+  : HTTPErrorHtmlResponse(server,
-                             &request,
+                          request,
-                             MHD_HTTP_NOT_FOUND,
+                          MHD_HTTP_NOT_FOUND,
-                             "text/html",
+                          "Content not found",
-                             RESOURCE::templates::_404_html)
+                          "Not Found")
 {
-  kainjow::mustache::list emptyList;
-  this->m_data = kainjow::mustache::object{{"details", emptyList}};
 }
 
-HTTP404HtmlResponse& HTTP404HtmlResponse::operator+(UrlNotFoundMsg /*unused*/)
+HTTPErrorHtmlResponse& HTTP404HtmlResponse::operator+(UrlNotFoundMsg /*unused*/)
 {
   const std::string requestUrl = m_request.get_full_url();
   kainjow::mustache::mustache msgTmpl(R"(The requested URL "{{url}}" was not found on this server.)");
   return *this + msgTmpl.render({"url", requestUrl});
 }
 
-HTTP404HtmlResponse& HTTP404HtmlResponse::operator+(const std::string& msg)
+HTTPErrorHtmlResponse& HTTPErrorHtmlResponse::operator+(const std::string& msg)
 {
   m_data["details"].push_back({"p", msg});
   return *this;
@@ -148,17 +141,15 @@ HTTP404HtmlResponse& HTTP404HtmlResponse::operator+(const std::string& msg)
 
 HTTP400HtmlResponse::HTTP400HtmlResponse(const InternalServer& server,
                                          const RequestContext& request)
-  : ContentResponseBlueprint(&server,
+  : HTTPErrorHtmlResponse(server,
-                             &request,
+                          request,
-                             MHD_HTTP_BAD_REQUEST,
+                          MHD_HTTP_BAD_REQUEST,
-                             "text/html",
+                          "Invalid request",
-                             RESOURCE::templates::_400_html)
+                          "Invalid request")
 {
-  kainjow::mustache::list emptyList;
-  this->m_data = kainjow::mustache::object{{"details", emptyList}};
 }
 
-HTTP400HtmlResponse& HTTP400HtmlResponse::operator+(InvalidUrlMsg /*unused*/)
+HTTPErrorHtmlResponse& HTTP400HtmlResponse::operator+(InvalidUrlMsg /*unused*/)
 {
   std::string requestUrl = m_request.get_full_url();
   const auto query = m_request.get_query();
@@ -169,12 +160,29 @@ HTTP400HtmlResponse& HTTP400HtmlResponse::operator+(InvalidUrlMsg /*unused*/)
   return *this + msgTmpl.render({"url", requestUrl});
 }
 
-HTTP400HtmlResponse& HTTP400HtmlResponse::operator+(const std::string& msg)
+HTTP500HtmlResponse::HTTP500HtmlResponse(const InternalServer& server,
+                                         const RequestContext& request)
+  : HTTPErrorHtmlResponse(server,
+                          request,
+                          MHD_HTTP_INTERNAL_SERVER_ERROR,
+                          "Internal Server Error",
+                          "Internal Server Error")
 {
-  m_data["details"].push_back({"p", msg});
+  // operator+() is a state-modifying operator (akin to operator+=)
-  return *this;
+  *this + "An internal server error occured. We are sorry about that :/";
 }
 
+std::unique_ptr<ContentResponse> HTTP500HtmlResponse::generateResponseObject() const
+{
+  // We want a 500 response to be a minimalistic one (so that the server doesn't
+  // have to provide additional resources required for its proper rendering)
+  // ";raw=true" in the MIME-type below disables response decoration
+  // (see ContentResponse::contentDecorationAllowed())
+  const std::string mimeType = "text/html;charset=utf-8;raw=true";
+  auto r = ContentResponse::build(m_server, m_template, m_data, mimeType);
+  r->set_code(m_httpStatusCode);
+  return r;
+}
 
 ContentResponseBlueprint& ContentResponseBlueprint::operator+(const TaskbarInfo& taskbarInfo)
 {
@@ -195,26 +203,6 @@ std::unique_ptr<Response> Response::build_416(const InternalServer& server, size
   return response;
 }
 
-std::unique_ptr<Response> Response::build_500(const InternalServer& server, const std::string& msg)
-{
-  MustacheData data;
-  data.set("error", msg);
-  auto content = render_template(RESOURCE::templates::_500_html, data);
-  std::unique_ptr<Response> response (
-      new ContentResponse(
-        server.m_root, //root
-        true, //verbose
-        true, //raw
-        false, //withTaskbar
-        false, //withLibraryButton
-        false, //blockExternalLinks
-        content, //content
-        "text/html" //mimetype
-  ));
-  response->set_code(MHD_HTTP_INTERNAL_SERVER_ERROR);
-  return response;
-}
-
 
 std::unique_ptr<Response> Response::build_redirect(const InternalServer& server, const std::string& redirectUrl)
 {
@@ -467,15 +455,6 @@ std::unique_ptr<ContentResponse> ContentResponse::build(
   return ContentResponse::build(server, content, mimetype, isHomePage);
 }
 
-std::unique_ptr<ContentResponse> withTaskbarInfo(
-  const std::string& bookName,
-  const zim::Archive* archive,
-  std::unique_ptr<ContentResponse> r)
-{
-  r->set_taskbar(bookName, archive);
-  return r;
-}
-
 ItemResponse::ItemResponse(bool verbose, const zim::Item& item, const std::string& mimetype, const ByteRange& byterange) :
   Response(verbose),
   m_item(item),

src/server/response.h

@@ -42,8 +42,6 @@ namespace kiwix {
 class InternalServer;
 class RequestContext;
 
-class ContentResponse;
-
 class Response {
   public:
     Response(bool verbose);
@@ -51,10 +49,7 @@ class Response {
 
     static std::unique_ptr<Response> build(const InternalServer& server);
     static std::unique_ptr<Response> build_304(const InternalServer& server, const ETag& etag);
-    static std::unique_ptr<ContentResponse> build_404(const InternalServer& server, const kainjow::mustache::data& data);
-    static std::unique_ptr<ContentResponse> build_404(const InternalServer& server, const std::string& url, const std::string& details="");
     static std::unique_ptr<Response> build_416(const InternalServer& server, size_t resourceLength);
-    static std::unique_ptr<Response> build_500(const InternalServer& server, const std::string& msg);
     static std::unique_ptr<Response> build_redirect(const InternalServer& server, const std::string& redirectUrl);
 
     MHD_Result send(const RequestContext& request, MHD_Connection* connection);
@@ -140,10 +135,6 @@ struct TaskbarInfo
   {}
 };
 
-std::unique_ptr<ContentResponse> withTaskbarInfo(const std::string& bookName,
-                                                 const zim::Archive* archive,
-                                                 std::unique_ptr<ContentResponse> r);
-
 class ContentResponseBlueprint
 {
 public: // functions
@@ -161,12 +152,6 @@ public: // functions
 
   virtual ~ContentResponseBlueprint() = default;
 
-  ContentResponseBlueprint& operator+(kainjow::mustache::data&& data)
-  {
-    this->m_data = std::move(data);
-    return *this;
-  }
-
   operator std::unique_ptr<ContentResponse>() const
   {
     return generateResponseObject();
@@ -193,32 +178,53 @@ public: //data
   std::unique_ptr<TaskbarInfo> m_taskbarInfo;
 };
 
+struct HTTPErrorHtmlResponse : ContentResponseBlueprint
+{
+  HTTPErrorHtmlResponse(const InternalServer& server,
+                      const RequestContext& request,
+                      int httpStatusCode,
+                      const std::string& pageTitleMsg,
+                      const std::string& headingMsg);
+
+  using ContentResponseBlueprint::operator+;
+  HTTPErrorHtmlResponse& operator+(const std::string& msg);
+};
+
 class UrlNotFoundMsg {};
 
 extern const UrlNotFoundMsg urlNotFoundMsg;
 
-struct HTTP404HtmlResponse : ContentResponseBlueprint
+struct HTTP404HtmlResponse : HTTPErrorHtmlResponse
 {
   HTTP404HtmlResponse(const InternalServer& server,
                       const RequestContext& request);
 
-  using ContentResponseBlueprint::operator+;
+  using HTTPErrorHtmlResponse::operator+;
-  HTTP404HtmlResponse& operator+(UrlNotFoundMsg /*unused*/);
+  HTTPErrorHtmlResponse& operator+(UrlNotFoundMsg /*unused*/);
-  HTTP404HtmlResponse& operator+(const std::string& errorDetails);
 };
 
 class InvalidUrlMsg {};
 
 extern const InvalidUrlMsg invalidUrlMsg;
 
-struct HTTP400HtmlResponse : ContentResponseBlueprint
+struct HTTP400HtmlResponse : HTTPErrorHtmlResponse
 {
   HTTP400HtmlResponse(const InternalServer& server,
                       const RequestContext& request);
 
-  using ContentResponseBlueprint::operator+;
+  using HTTPErrorHtmlResponse::operator+;
-  HTTP400HtmlResponse& operator+(InvalidUrlMsg /*unused*/);
+  HTTPErrorHtmlResponse& operator+(InvalidUrlMsg /*unused*/);
-  HTTP400HtmlResponse& operator+(const std::string& errorDetails);
+};
+
+struct HTTP500HtmlResponse : HTTPErrorHtmlResponse
+{
+  HTTP500HtmlResponse(const InternalServer& server,
+                      const RequestContext& request);
+
+private: // overrides
+  // generateResponseObject() is overriden in order to produce a minimal
+  // response without any need for additional resources from the server
+  std::unique_ptr<ContentResponse> generateResponseObject() const override;
 };
 
 class ItemResponse : public Response {

static/resources_list.txt

@@ -35,9 +35,7 @@ skin/block_external.js
 skin/search_results.css
 templates/search_result.html
 templates/no_search_result.html
-templates/400.html
+templates/error.html
-templates/404.html
-templates/500.html
 templates/index.html
 templates/suggestion.json
 templates/head_taskbar.html

static/templates/404.html

@@ -1,15 +0,0 @@
-<!DOCTYPE html>
-<html xmlns="http://www.w3.org/1999/xhtml">
-  <head>
-    <meta content="text/html;charset=UTF-8" http-equiv="content-type" />
-    <title>Content not found</title>
-  </head>
-  <body>
-    <h1>Not Found</h1>
-{{#details}}
-    <p>
-      {{{p}}}
-    </p>
-{{/details}}
-  </body>
-</html>

static/templates/500.html

@@ -1,16 +0,0 @@
-<!DOCTYPE html>
-<html>
-  <head>
-    <meta content="text/html;charset=UTF-8" http-equiv="content-type" />
-    <title>Internal Server Error</title>
-  </head>
-  <body>
-    <h1>Internal Server Error</h1>
-    <p>
-      An internal server error occured. We are sorry about that :/
-    </p>
-    <p>
-      {{ error }}
-    </p>
-  </body>
-</html>

static/templates/error.html

@@ -2,10 +2,10 @@
 <html xmlns="http://www.w3.org/1999/xhtml">
   <head>
     <meta content="text/html;charset=UTF-8" http-equiv="content-type" />
-    <title>Invalid request</title>
+    <title>{{PAGE_TITLE}}</title>
   </head>
   <body>
-    <h1>Invalid request</h1>
+    <h1>{{PAGE_HEADING}}</h1>
 {{#details}}
     <p>
       {{{p}}}

test/meson.build

@@ -29,6 +29,7 @@ if gtest_dep.found() and not meson.is_cross_build()
       'zimfile.zim',
       'zimfile&other.zim',
       'corner_cases.zim',
+      'poor.zim',
       'library.xml'
     ]
     foreach file : data_files

test/server.cpp

@@ -142,6 +142,7 @@ protected:
   const int PORT = 8001;
   const ZimFileServer::FilePathCollection ZIMFILES {
     "./test/zimfile.zim",
+    "./test/poor.zim",
     "./test/corner_cases.zim"
   };
 
@@ -761,6 +762,31 @@ TEST_F(ServerTest, 400WithBodyTesting)
   }
 }
 
+TEST_F(ServerTest, 500)
+{
+  const std::string expectedBody = R"(<!DOCTYPE html>
+<html xmlns="http://www.w3.org/1999/xhtml">
+  <head>
+    <meta content="text/html;charset=UTF-8" http-equiv="content-type" />
+    <title>Internal Server Error</title>
+  </head>
+  <body>
+    <h1>Internal Server Error</h1>
+    <p>
+      An internal server error occured. We are sorry about that :/
+    </p>
+    <p>
+      Entry redirect_loop.html is a redirect entry.
+    </p>
+  </body>
+</html>
+)";
+
+  const auto r = zfs1_->GET("/ROOT/poor/A/redirect_loop.html");
+  EXPECT_EQ(r->status, 500);
+  EXPECT_EQ(r->body, expectedBody);
+}
+
 TEST_F(ServerTest, RandomPageRedirectsToAnExistingArticle)
 {
   auto g = zfs1_->GET("/ROOT/random?content=zimfile");