Fix URI percent-decoding

CMakeLists.txt

@@ -6,7 +6,7 @@ set(PROJECT_URL "https://github.com/badaix/snapcast")
 
 option(BUILD_SHARED_LIBS "Build snapcast in a shared context" ON)
 option(BUILD_STATIC_LIBS "Build snapcast in a static context" ON)
-option(BUILD_TESTS "Build tests (run tests with make test)" ON)
+option(BUILD_TESTS "Build tests (in test/snapcast_test)" ON)
 option(WERROR "Treat warnings as errors" OFF)
 
 option(ASAN "Enable AddressSanitizer" OFF)
@@ -20,6 +20,10 @@ IF (REVISION)
     add_definitions(-DREVISION=\"${REVISION}\")
 ENDIF()
 
+if (BUILD_TESTS)
+    add_subdirectory(test)
+endif (BUILD_TESTS)
+
 IF (TIDY)
     FIND_PROGRAM(CLANG_TIDY "clang-tidy")
     IF(CLANG_TIDY)

server/streamreader/stream_uri.cpp

@@ -47,17 +47,17 @@ void StreamUri::parse(const std::string& streamUri)
     while (!uri.empty() && ((uri[uri.length() - 1] == '\'') || (uri[uri.length() - 1] == '"')))
         uri = uri.substr(0, this->uri.length() - 1);
 
-    string decodedUri = strutils::uriDecode(uri);
+    // string decodedUri = strutils::uriDecode(uri);
-    LOG(DEBUG) << "StreamUri decoded: " << decodedUri << "\n";
+    // LOG(DEBUG) << "StreamUri decoded: " << decodedUri << "\n";
 
-    string tmp(decodedUri);
+    string tmp(uri);
 
     pos = tmp.find(':');
     if (pos == string::npos)
         throw invalid_argument("missing ':'");
-    scheme = strutils::trim_copy(tmp.substr(0, pos));
+    scheme = strutils::uriDecode(strutils::trim_copy(tmp.substr(0, pos)));
     tmp = tmp.substr(pos + 1);
-    LOG(DEBUG) << "scheme: '" << scheme << "' tmp: '" << tmp << "'\n";
+    LOG(TRACE) << "scheme: '" << scheme << "', tmp: '" << tmp << "'\n";
 
     if (tmp.find("//") != 0)
         throw invalid_argument("missing host separator: '//'");
@@ -71,27 +71,29 @@ void StreamUri::parse(const std::string& streamUri)
             pos = tmp.length();
     }
 
-    host = strutils::trim_copy(tmp.substr(0, pos));
+    host = strutils::uriDecode(strutils::trim_copy(tmp.substr(0, pos)));
     tmp = tmp.substr(pos);
     path = tmp;
-    LOG(DEBUG) << "host: '" << host << "' tmp: '" << tmp << "' path: '" << path << "'\n";
+    pos = std::min(path.find('?'), path.find('#'));
+    path = strutils::uriDecode(strutils::trim_copy(path.substr(0, pos)));
+    LOG(TRACE) << "host: '" << host << "', tmp: '" << tmp << "', path: '" << path << "'\n";
 
+    string queryStr;
     pos = tmp.find('?');
-    if (pos == string::npos)
+    if (pos != string::npos)
-        return;
+    {
-
-    path = strutils::trim_copy(tmp.substr(0, pos));
         tmp = tmp.substr(pos + 1);
-    string queryStr = tmp;
+        queryStr = tmp;
-    LOG(DEBUG) << "path: '" << path << "' tmp: '" << tmp << "' query: '" << queryStr << "'\n";
+        LOG(TRACE) << "path: '" << path << "', tmp: '" << tmp << "', query: '" << queryStr << "'\n";
+    }
 
     pos = tmp.find('#');
     if (pos != string::npos)
     {
         queryStr = tmp.substr(0, pos);
         tmp = tmp.substr(pos + 1);
-        fragment = strutils::trim_copy(tmp);
+        fragment = strutils::uriDecode(strutils::trim_copy(tmp));
-        LOG(DEBUG) << "query: '" << queryStr << "' fragment: '" << fragment << "' tmp: '" << tmp << "'\n";
+        LOG(TRACE) << "query: '" << queryStr << "', fragment: '" << fragment << "', tmp: '" << tmp << "'\n";
     }
 
     vector<string> keyValueList = strutils::split(queryStr, '&');
@@ -100,8 +102,8 @@ void StreamUri::parse(const std::string& streamUri)
         pos = kv.find('=');
         if (pos != string::npos)
         {
-            string key = strutils::trim_copy(kv.substr(0, pos));
+            string key = strutils::uriDecode(strutils::trim_copy(kv.substr(0, pos)));
-            string value = strutils::trim_copy(kv.substr(pos + 1));
+            string value = strutils::uriDecode(strutils::trim_copy(kv.substr(pos + 1)));
             query[key] = value;
         }
     }

test/CMakeLists.txt

@@ -0,0 +1,10 @@
+# Prepare "Catch" library for other executables
+set(CATCH_INCLUDE_DIR ${CMAKE_CURRENT_SOURCE_DIR})
+add_library(Catch INTERFACE)
+target_include_directories(Catch INTERFACE ${CATCH_INCLUDE_DIR} ${CMAKE_SOURCE_DIR})
+
+# Make test executable
+set(TEST_SOURCES ${CMAKE_CURRENT_SOURCE_DIR}/test_main.cpp ${CMAKE_SOURCE_DIR}/server/streamreader/stream_uri.cpp)
+add_executable(snapcast_test ${TEST_SOURCES})
+target_link_libraries(snapcast_test Catch)
+

test/test_main.cpp

@@ -0,0 +1,104 @@
+/***
+    This file is part of snapcast
+    Copyright (C) 2014-2021  Johannes Pohl
+
+    This program is free software: you can redistribute it and/or modify
+    it under the terms of the GNU General Public License as published by
+    the Free Software Foundation, either version 3 of the License, or
+    (at your option) any later version.
+
+    This program is distributed in the hope that it will be useful,
+    but WITHOUT ANY WARRANTY; without even the implied warranty of
+    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+    GNU General Public License for more details.
+
+    You should have received a copy of the GNU General Public License
+    along with this program.  If not, see <http://www.gnu.org/licenses/>.
+***/
+
+#define CATCH_CONFIG_MAIN
+#include "catch.hpp"
+#include "common/aixlog.hpp"
+#include "common/utils/string_utils.hpp"
+#include "server/streamreader/stream_uri.hpp"
+
+using namespace std;
+
+
+TEST_CASE("String utils")
+{
+    using namespace utils::string;
+    REQUIRE(ltrim_copy(" test") == "test");
+}
+
+
+TEST_CASE("Uri")
+{
+    AixLog::Log::init<AixLog::SinkCout>(AixLog::Severity::debug);
+    using namespace streamreader;
+    StreamUri uri("pipe:///tmp/snapfifo?name=default&codec=flac");
+    REQUIRE(uri.scheme == "pipe"); 
+    REQUIRE(uri.path == "/tmp/snapfifo"); 
+    REQUIRE(uri.host.empty());
+
+    // uri = StreamUri("scheme:[//host[:port]][/]path[?query=none][#fragment]");
+    // Test with all fields
+    uri = StreamUri("scheme://host:port/path?query=none&key=value#fragment");
+    REQUIRE(uri.scheme == "scheme"); 
+    REQUIRE(uri.host == "host:port"); 
+    REQUIRE(uri.path == "/path");
+    REQUIRE(uri.query["query"] == "none");
+    REQUIRE(uri.query["key"] == "value");
+    REQUIRE(uri.fragment == "fragment");
+
+    // Test with all fields, url encoded
+    // "%21%23%24%25%26%27%28%29%2A%2B%2C%2F%3A%3B%3D%3F%40%5B%5D"
+    // "!#$%&'()*+,/:;=?@[]"
+    uri = StreamUri("scheme%26://%26host%3f:port/pa%2Bth?%21%23%24%25%26%27%28%29=%2A%2B%2C%2F%3A%3B%3D%3F%40%5B%5D&key%2525=value#fragment%3f%21%3F");
+    REQUIRE(uri.scheme == "scheme&"); 
+    REQUIRE(uri.host == "&host?:port"); 
+    REQUIRE(uri.path == "/pa+th");
+    REQUIRE(uri.query["!#$%&'()"] == "*+,/:;=?@[]");
+    REQUIRE(uri.query["key%25"] == "value");
+    REQUIRE(uri.fragment == "fragment?!?");
+
+    // No host
+    uri = StreamUri("scheme:///path?query=none#fragment");
+    REQUIRE(uri.scheme == "scheme"); 
+    REQUIRE(uri.path == "/path");
+    REQUIRE(uri.query["query"] == "none");
+    REQUIRE(uri.fragment == "fragment");
+
+    // No host, no query
+    uri = StreamUri("scheme:///path#fragment");
+    REQUIRE(uri.scheme == "scheme"); 
+    REQUIRE(uri.path == "/path");
+    REQUIRE(uri.query.empty());
+    REQUIRE(uri.fragment == "fragment");
+
+    // No host, no fragment
+    uri = StreamUri("scheme:///path?query=none");
+    REQUIRE(uri.scheme == "scheme"); 
+    REQUIRE(uri.path == "/path");
+    REQUIRE(uri.query["query"] == "none");
+    REQUIRE(uri.fragment.empty());
+
+    // just schema and path
+    uri = StreamUri("scheme:///path");
+    REQUIRE(uri.scheme == "scheme"); 
+    REQUIRE(uri.path == "/path");
+    REQUIRE(uri.query.empty());
+    REQUIRE(uri.fragment.empty());
+
+    // Issue #850
+    uri = StreamUri("spotify:///librespot?name=Spotify&username=EMAIL&password=string%26with%26ampersands&devicename=Snapcast&bitrate=320&killall=false");
+    REQUIRE(uri.scheme == "spotify"); 
+    REQUIRE(uri.host.empty()); 
+    REQUIRE(uri.path == "/librespot");
+    REQUIRE(uri.query["name"] == "Spotify");
+    REQUIRE(uri.query["username"] == "EMAIL");
+    REQUIRE(uri.query["password"] == "string&with&ampersands");
+    REQUIRE(uri.query["devicename"] == "Snapcast");
+    REQUIRE(uri.query["bitrate"] == "320");
+    REQUIRE(uri.query["killall"] == "false");
+}