diff --git a/src/server/internalServer.cpp b/src/server/internalServer.cpp index 0c45580ad..05c2d559c 100644 --- a/src/server/internalServer.cpp +++ b/src/server/internalServer.cpp @@ -359,10 +359,12 @@ std::unique_ptr 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 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 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 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 withTaskbarInfo(bookName, archive.get(), std::move(response)); + return HTTP404HtmlResponse(*this, request) + + error_details + + TaskbarInfo(bookName, archive.get()); } } @@ -838,11 +844,11 @@ std::unique_ptr 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 details = searchSuggestionHTML(searchURL, kiwix::urlDecode(pattern)); - - auto response = Response::build_404(*this, request.get_full_url(), details); - return withTaskbarInfo(bookName, nullptr, std::move(response)); + const std::string searchURL = m_root + "/search?pattern=" + kiwix::urlEncode(pattern, true); + return HTTP404HtmlResponse(*this, request) + + urlNotFoundMsg + + searchSuggestionHTML(searchURL, kiwix::urlDecode(pattern)) + + TaskbarInfo(bookName); } auto urlStr = request.get_url().substr(bookName.size()+1); @@ -872,11 +878,11 @@ std::unique_ptr 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. - const std::string details = searchSuggestionHTML(searchURL, kiwix::urlDecode(pattern)); - - auto response = Response::build_404(*this, request.get_full_url(), details); - return withTaskbarInfo(bookName, archive.get(), std::move(response)); + std::string searchURL = m_root + "/search?content=" + bookName + "&pattern=" + kiwix::urlEncode(pattern, true); + return HTTP404HtmlResponse(*this, request) + + urlNotFoundMsg + + searchSuggestionHTML(searchURL, kiwix::urlDecode(pattern)) + + TaskbarInfo(bookName, archive.get()); } } @@ -899,7 +905,9 @@ std::unique_ptr 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 archive; @@ -936,7 +944,9 @@ std::unique_ptr 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; } } diff --git a/src/server/internalServer.h b/src/server/internalServer.h index eb8dcc5cf..69e8166c4 100644 --- a/src/server/internalServer.h +++ b/src/server/internalServer.h @@ -177,8 +177,6 @@ class InternalServer { friend std::unique_ptr Response::build(const InternalServer& server); friend std::unique_ptr ContentResponse::build(const InternalServer& server, const std::string& content, const std::string& mimetype, bool isHomePage, bool raw); friend std::unique_ptr ItemResponse::build(const InternalServer& server, const RequestContext& request, const zim::Item& item, bool raw); - friend std::unique_ptr Response::build_500(const InternalServer& server, const std::string& msg); - }; } diff --git a/src/server/response.cpp b/src/server/response.cpp index e851015c3..aefa0dc71 100644 --- a/src/server/response.cpp +++ b/src/server/response.cpp @@ -84,31 +84,6 @@ std::unique_ptr 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 Response::build_404(const InternalServer& server, const std::string& url, const std::string& details) -{ - return build_404(server, make404ResponseData(url, details)); -} - -std::unique_ptr 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 ContentResponseBlueprint::generateResponseObjec { auto r = ContentResponse::build(m_server, m_template, m_data, m_mimeType); r->set_code(m_httpStatusCode); - return m_taskbarInfo - ? withTaskbarInfo(m_taskbarInfo->bookName, m_taskbarInfo->archive, std::move(r)) - : std::move(r); + if ( m_taskbarInfo ) { + r->set_taskbar(m_taskbarInfo->bookName, m_taskbarInfo->archive); + } + 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) - : ContentResponseBlueprint(&server, - &request, - MHD_HTTP_NOT_FOUND, - "text/html", - RESOURCE::templates::_404_html) + const RequestContext& request) + : HTTPErrorHtmlResponse(server, + request, + MHD_HTTP_NOT_FOUND, + "Content not found", + "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, - &request, - MHD_HTTP_BAD_REQUEST, - "text/html", - RESOURCE::templates::_400_html) + : HTTPErrorHtmlResponse(server, + request, + MHD_HTTP_BAD_REQUEST, + "Invalid request", + "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}); - return *this; + // operator+() is a state-modifying operator (akin to operator+=) + *this + "An internal server error occured. We are sorry about that :/"; } +std::unique_ptr 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::build_416(const InternalServer& server, size return response; } -std::unique_ptr 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 ( - 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::build_redirect(const InternalServer& server, const std::string& redirectUrl) { @@ -467,15 +455,6 @@ std::unique_ptr ContentResponse::build( return ContentResponse::build(server, content, mimetype, isHomePage); } -std::unique_ptr withTaskbarInfo( - const std::string& bookName, - const zim::Archive* archive, - std::unique_ptr 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), diff --git a/src/server/response.h b/src/server/response.h index d46898923..710e2bc0f 100644 --- a/src/server/response.h +++ b/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 build(const InternalServer& server); static std::unique_ptr build_304(const InternalServer& server, const ETag& etag); - static std::unique_ptr build_404(const InternalServer& server, const kainjow::mustache::data& data); - static std::unique_ptr build_404(const InternalServer& server, const std::string& url, const std::string& details=""); static std::unique_ptr build_416(const InternalServer& server, size_t resourceLength); - static std::unique_ptr build_500(const InternalServer& server, const std::string& msg); static std::unique_ptr 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 withTaskbarInfo(const std::string& bookName, - const zim::Archive* archive, - std::unique_ptr 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() const { return generateResponseObject(); @@ -193,32 +178,53 @@ public: //data std::unique_ptr 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+; - HTTP404HtmlResponse& operator+(UrlNotFoundMsg /*unused*/); - HTTP404HtmlResponse& operator+(const std::string& errorDetails); + using HTTPErrorHtmlResponse::operator+; + HTTPErrorHtmlResponse& operator+(UrlNotFoundMsg /*unused*/); }; class InvalidUrlMsg {}; extern const InvalidUrlMsg invalidUrlMsg; -struct HTTP400HtmlResponse : ContentResponseBlueprint +struct HTTP400HtmlResponse : HTTPErrorHtmlResponse { HTTP400HtmlResponse(const InternalServer& server, const RequestContext& request); - using ContentResponseBlueprint::operator+; - HTTP400HtmlResponse& operator+(InvalidUrlMsg /*unused*/); - HTTP400HtmlResponse& operator+(const std::string& errorDetails); + using HTTPErrorHtmlResponse::operator+; + HTTPErrorHtmlResponse& operator+(InvalidUrlMsg /*unused*/); +}; + +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 generateResponseObject() const override; }; class ItemResponse : public Response { diff --git a/static/resources_list.txt b/static/resources_list.txt index ab583d7a6..7444c48ae 100644 --- a/static/resources_list.txt +++ b/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/404.html -templates/500.html +templates/error.html templates/index.html templates/suggestion.json templates/head_taskbar.html diff --git a/static/templates/404.html b/static/templates/404.html deleted file mode 100644 index 795e8c9ef..000000000 --- a/static/templates/404.html +++ /dev/null @@ -1,15 +0,0 @@ - - - - - Content not found - - -

Not Found

-{{#details}} -

- {{{p}}} -

-{{/details}} - - diff --git a/static/templates/500.html b/static/templates/500.html deleted file mode 100644 index 1b2692d5f..000000000 --- a/static/templates/500.html +++ /dev/null @@ -1,16 +0,0 @@ - - - - - Internal Server Error - - -

Internal Server Error

-

- An internal server error occured. We are sorry about that :/ -

-

- {{ error }} -

- - diff --git a/static/templates/400.html b/static/templates/error.html similarity index 78% rename from static/templates/400.html rename to static/templates/error.html index 5f4ecf42f..5fcc0ff2c 100644 --- a/static/templates/400.html +++ b/static/templates/error.html @@ -2,10 +2,10 @@ - Invalid request + {{PAGE_TITLE}} -

Invalid request

+

{{PAGE_HEADING}}

{{#details}}

{{{p}}} diff --git a/test/data/poor.zim b/test/data/poor.zim new file mode 100644 index 000000000..b30d77a18 Binary files /dev/null and b/test/data/poor.zim differ diff --git a/test/meson.build b/test/meson.build index 62b210871..1047ff0c7 100644 --- a/test/meson.build +++ b/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 diff --git a/test/server.cpp b/test/server.cpp index 59ed29bb4..6467d44fa 100644 --- a/test/server.cpp +++ b/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"( + + + + Internal Server Error + + +

Internal Server Error

+

+ An internal server error occured. We are sorry about that :/ +

+

+ Entry redirect_loop.html is a redirect entry. +

+ + +)"; + + 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");