From 192bdac209005e0edcc33f3cb31a301ccfb99f81 Mon Sep 17 00:00:00 2001 From: Michael Zanetti Date: Sat, 23 Apr 2022 00:02:20 +0200 Subject: [PATCH] Fix webserver tests canonicalFilePath() behavior seems to have changed at some point and not existing files outside the public dir will return a 404 rather than a 403. Ideally the logic would be fixed to first check for the file being outside the public directory, directly returning a 403, then checking if the file exists, return a 404 if not, and lastly check for permissions on the file and return a 403 again of denied. However, that would result in a bigger change. Also, the tests were failing all along if syslog exists but is not readable (code is ok, just bad test) but none of our autotesters seemed to have such a setup till now. --- libnymea-core/servers/webserver.cpp | 2 +- tests/auto/webserver/testwebserver.cpp | 40 +++++++++++++++----------- 2 files changed, 25 insertions(+), 17 deletions(-) diff --git a/libnymea-core/servers/webserver.cpp b/libnymea-core/servers/webserver.cpp index f647ffd1..c83d7419 100644 --- a/libnymea-core/servers/webserver.cpp +++ b/libnymea-core/servers/webserver.cpp @@ -169,7 +169,7 @@ bool WebServer::verifyFile(QSslSocket *socket, const QString &fileName) // make sure the file is in the public directory if (!file.canonicalFilePath().startsWith(QDir(m_configuration.publicFolder).canonicalPath())) { - qCDebug(dcWebServer()) << "Requested file" << file.fileName() << "is outside the public folder."; + qCDebug(dcWebServer()) << "Requested file" << file.canonicalFilePath() << "is outside the public folder."; HttpReply *reply = HttpReply::createErrorReply(HttpReply::Forbidden); reply->setClientId(m_clientList.key(socket)); sendHttpReply(reply); diff --git a/tests/auto/webserver/testwebserver.cpp b/tests/auto/webserver/testwebserver.cpp index 7b6e877d..9a43e5fa 100644 --- a/tests/auto/webserver/testwebserver.cpp +++ b/tests/auto/webserver/testwebserver.cpp @@ -66,8 +66,8 @@ private slots: void getDebugServer(); public slots: - void onSslErrors(const QList &) { - qWarning() << "SSL error"; + void onSslErrors(const QList &errors) { + qCWarning(dcTests()) << "SSL errors:" << errors; QSslSocket *socket = static_cast(sender()); socket->ignoreSslErrors(); } @@ -75,7 +75,7 @@ public slots: void TestWebserver::initTestCase() { - NymeaTestBase::initTestCase(); + NymeaTestBase::initTestCase("*.debug=false\nWebServer.debug=true\ntests.debug=true\nServerManager.debug=true"); qDebug() << "TestWebserver starting"; foreach (const WebServerConfiguration &config, NymeaCore::instance()->configuration()->webServerConfigurations()) { @@ -215,7 +215,8 @@ void TestWebserver::checkAllowedMethodCall() QFETCH(int, expectedStatusCode); QNetworkAccessManager nam; - connect(&nam, &QNetworkAccessManager::sslErrors, [](QNetworkReply* reply, const QList &) { + connect(&nam, &QNetworkAccessManager::sslErrors, [](QNetworkReply* reply, const QList &errors) { + qCWarning(dcTests) << "SSL errors:" << errors; reply->ignoreSslErrors(); }); QSignalSpy clientSpy(&nam, SIGNAL(finished(QNetworkReply*))); @@ -244,7 +245,7 @@ void TestWebserver::checkAllowedMethodCall() } else if(method == "TRACE") { reply = nam.sendCustomRequest(request, "TRACE"); } else { - // just to make shore there will be a reply to delete + // just to make sure there will be a reply to delete reply = nam.get(request); } @@ -279,7 +280,7 @@ void TestWebserver::badRequests_data() wrongHeaderFormatting.append("\r\n"); QByteArray userAgentMissing; - userAgentMissing.append("GET /abc HTTP/1.1\r\n"); + userAgentMissing.append("GET /index.html HTTP/1.1\r\n"); userAgentMissing.append("\r\n"); QTest::newRow("wrong content length") << wrongContentLength << 400; @@ -334,8 +335,8 @@ void TestWebserver::getFiles_data() QTest::newRow("get /etc/passwd") << "/etc/passwd" << 404; QTest::newRow("get /blub/blub/blabla") << "/etc/passwd" << 404; QTest::newRow("get /../../etc/passwd") << "/../../etc/passwd" << 404; - QTest::newRow("get /../../") << "/../../" << 403; - QTest::newRow("get /../") << "/../" << 403; + QTest::newRow("get /../../") << "/../../" << 404; + QTest::newRow("get /../") << "/../" << 404; QTest::newRow("get /etc/nymea/nymead.conf") << "/etc/nymea/nymead.conf" << 404; QTest::newRow("get /etc/sudoers") << "/etc/sudoers" << 404; QTest::newRow("get /root/.ssh/id_rsa.pub") << "/root/.ssh/id_rsa.pub" << 404; @@ -347,7 +348,7 @@ void TestWebserver::getFiles() QFETCH(int, expectedStatusCode); QNetworkAccessManager nam; - connect(&nam, &QNetworkAccessManager::sslErrors, [this, &nam](QNetworkReply* reply, const QList &) { + connect(&nam, &QNetworkAccessManager::sslErrors, this, [](QNetworkReply* reply, const QList &) { reply->ignoreSslErrors(); }); QSignalSpy clientSpy(&nam, SIGNAL(finished(QNetworkReply*))); @@ -489,17 +490,24 @@ void TestWebserver::getDebugServer_data() // Check if syslog is accessable QFileInfo syslogFileInfo("/var/log/syslog"); - if (syslogFileInfo.exists() && syslogFileInfo.isReadable()) { - // syslog enabled - QTest::newRow("GET /debug/syslog | server enabled | 200") << "get" << "/debug/syslog" << true << 200; - QTest::newRow("OPTIONS /debug/syslog | server enabled | 200") << "options" << "/debug/syslog" << true << 200; + if (!syslogFileInfo.exists()) { + // syslog file doesn't exist + QTest::newRow("GET /debug/syslog | server enabled | 200") << "get" << "/debug/syslog" << true << 404; + QTest::newRow("OPTIONS /debug/syslog | server enabled | 200") << "options" << "/debug/syslog" << true << 404; + QTest::newRow("PUT /debug/syslog | server enabled | 405") << "put" << "/debug/syslog" << true << 405; + QTest::newRow("POST /debug/syslog | server enabled | 405") << "post" << "/debug/syslog" << true << 405; + QTest::newRow("DELETE /debug/syslog | server enabled | 405") << "delete" << "/debug/syslog" << true << 405; + } else if (!syslogFileInfo.isReadable()) { + // syslog enabled, but not readable + QTest::newRow("GET /debug/syslog | server enabled | 200") << "get" << "/debug/syslog" << true << 403; + QTest::newRow("OPTIONS /debug/syslog | server enabled | 200") << "options" << "/debug/syslog" << true << 403; QTest::newRow("PUT /debug/syslog | server enabled | 405") << "put" << "/debug/syslog" << true << 405; QTest::newRow("POST /debug/syslog | server enabled | 405") << "post" << "/debug/syslog" << true << 405; QTest::newRow("DELETE /debug/syslog | server enabled | 405") << "delete" << "/debug/syslog" << true << 405; } else { - // syslog enabled, but not readable - QTest::newRow("GET /debug/syslog | server enabled | 200") << "get" << "/debug/syslog" << true << 403; - QTest::newRow("OPTIONS /debug/syslog | server enabled | 200") << "options" << "/debug/syslog" << true << 403; + // syslog enabled + QTest::newRow("GET /debug/syslog | server enabled | 200") << "get" << "/debug/syslog" << true << 200; + QTest::newRow("OPTIONS /debug/syslog | server enabled | 200") << "options" << "/debug/syslog" << true << 200; QTest::newRow("PUT /debug/syslog | server enabled | 405") << "put" << "/debug/syslog" << true << 405; QTest::newRow("POST /debug/syslog | server enabled | 405") << "post" << "/debug/syslog" << true << 405; QTest::newRow("DELETE /debug/syslog | server enabled | 405") << "delete" << "/debug/syslog" << true << 405;