Subsonic API: better handling unhandled commands, safer version check

src/api/subsonic/SubsonicResource.cpp

@@ -75,9 +75,9 @@ namespace API::Subsonic
 {
 struct ClientVersion
 {
-	unsigned major;
+	unsigned major {};
-	unsigned minor;
+	unsigned minor {};
-	unsigned patch;
+	unsigned patch {};
 };
 }
 
@@ -87,7 +87,7 @@ readAs(const std::string& str)
 {
 	// Expects "X.Y.Z"
 	const auto numbers {splitString(str, ".")};
-	if (numbers.size() != 3)
+	if (numbers.size() < 2 || numbers.size() > 3)
 		return boost::none;
 
 	API::Subsonic::ClientVersion version;
@@ -102,11 +102,13 @@ readAs(const std::string& str)
 		return boost::none;
 	version.minor = *number;
 
-	number = {readAs<unsigned>(numbers[2])};
+	if (numbers.size() == 3)
-	if (!number)
+	{
-		return boost::none;
+		number = {readAs<unsigned>(numbers[2])};
-	version.patch = *number;
+		if (!number)
-
+			return boost::none;
+		version.patch = *number;
+	}
 
 	return version;
 }
@@ -255,10 +257,17 @@ getClientInfo(const Wt::Http::ParameterMap& parameters)
 	ClientInfo res;
 
 	// Mandatory parameters
+	res.version = getMandatoryParameterAs<ClientVersion>(parameters, "v");
+	if (res.version.major > API_VERSION_MAJOR)
+		throw Error {Error::Code::ServerMustUpgrade};
+	if (res.version.major < API_VERSION_MAJOR)
+		throw Error {Error::Code::ClientMustUpgrade};
+	if (res.version.minor > API_VERSION_MINOR)
+		throw Error {Error::Code::ServerMustUpgrade};
+
 	res.name = getMandatoryParameterAs<std::string>(parameters, "c");
 	res.user = getMandatoryParameterAs<std::string>(parameters, "u");
 	res.password = decodePasswordIfNeeded(getMandatoryParameterAs<std::string>(parameters, "p"));
-	res.version = getMandatoryParameterAs<ClientVersion>(parameters, "v");
 
 	return res;
 }
@@ -563,13 +572,6 @@ userToResponseNode(const User::pointer& user)
 	return userNode;
 }
 
-static
-Response
-handleNotImplementedRequest(RequestContext& context)
-{
-	throw Error {Error::CustomType::NotImplemented};
-}
-
 static
 Response
 handlePingRequest(RequestContext& context)
@@ -1796,58 +1798,47 @@ struct RequestEntryPointInfo
 
 static std::map<std::string, RequestEntryPointInfo> requestEntryPoints
 {
-	{"/rest/changePassword.view",		{handleChangePassword,			false}},
+	{"changePassword",	{handleChangePassword,			false}},
-	{"/rest/createPlaylist.view",		{handleCreatePlaylistRequest,		false}},
+	{"createPlaylist",	{handleCreatePlaylistRequest,		false}},
-	{"/rest/createShare.view",		{handleNotImplementedRequest,		false}},
+	{"createUser",		{handleCreateUserRequest,		true}},
-	{"/rest/createUser.view",		{handleCreateUserRequest,		true}},
+	{"deletePlaylist",	{handleDeletePlaylistRequest,		false}},
-	{"/rest/deletePlaylist.view",		{handleDeletePlaylistRequest,		false}},
+	{"deleteUser",		{handleDeleteUserRequest,		true}},
-	{"/rest/deleteShare.view",		{handleNotImplementedRequest,		false}},
+	{"getAlbumList",	{handleGetAlbumListRequest,		false}},
-	{"/rest/deleteUser.view",		{handleDeleteUserRequest,		true}},
+	{"getAlbumList2",	{handleGetAlbumList2Request,		false}},
-	{"/rest/getAvatar.view",		{handleNotImplementedRequest,		false}},
+	{"getAlbum",		{handleGetAlbumRequest,			false}},
-	{"/rest/getAlbumList.view",		{handleGetAlbumListRequest,		false}},
+	{"getArtist",		{handleGetArtistRequest,		false}},
-	{"/rest/getAlbumList2.view",		{handleGetAlbumList2Request,		false}},
+	{"getArtistInfo",	{handleGetArtistInfoRequest,		false}},
-	{"/rest/getAlbum.view",			{handleGetAlbumRequest,			false}},
+	{"getArtistInfo2",	{handleGetArtistInfo2Request,		false}},
-	{"/rest/getArtist.view",		{handleGetArtistRequest,		false}},
+	{"getArtists",		{handleGetArtistsRequest,		false}},
-	{"/rest/getArtistInfo.view",		{handleGetArtistInfoRequest,		false}},
+	{"getGenres",		{handleGetGenresRequest,		false}},
-	{"/rest/getArtistInfo2.view",		{handleGetArtistInfo2Request,		false}},
+	{"getIndexes",		{handleGetIndexesRequest,		false}},
-	{"/rest/getArtists.view",		{handleGetArtistsRequest,		false}},
+	{"getLicense",		{handleGetLicenseRequest,		false}},
-	{"/rest/getBookmarks.view",		{handleNotImplementedRequest,		false}},
+	{"getMusicDirectory",	{handleGetMusicDirectoryRequest,	false}},
-	{"/rest/getGenres.view",		{handleGetGenresRequest,		false}},
+	{"getMusicFolders",	{handleGetMusicFoldersRequest,		false}},
-	{"/rest/getIndexes.view",		{handleGetIndexesRequest,		false}},
+	{"getRandomSongs",	{handleGetRandomSongsRequest,		false}},
-	{"/rest/getLicense.view",		{handleGetLicenseRequest,		false}},
+	{"getSimilarSongs",	{handleGetSimilarSongsRequest,		false}},
-	{"/rest/getLyrics.view",		{handleNotImplementedRequest,		false}},
+	{"getSimilarSongs2",	{handleGetSimilarSongs2Request,		false}},
-	{"/rest/getMusicDirectory.view",	{handleGetMusicDirectoryRequest,	false}},
+	{"getStarred",		{handleGetStarredRequest,		false}},
-	{"/rest/getMusicFolders.view",		{handleGetMusicFoldersRequest,		false}},
+	{"getStarred2",		{handleGetStarred2Request,		false}},
-	{"/rest/getNowPlaying.view",		{handleNotImplementedRequest,		false}},
+	{"getPlaylist",		{handleGetPlaylistRequest,		false}},
-	{"/rest/getRandomSongs.view",		{handleGetRandomSongsRequest,		false}},
+	{"getPlaylists",	{handleGetPlaylistsRequest,		false}},
-	{"/rest/getShares.view",		{handleNotImplementedRequest,		false}},
+	{"getSongsByGenre",	{handleGetSongsByGenreRequest,		false}},
-	{"/rest/getSimilarSongs.view",		{handleGetSimilarSongsRequest,		false}},
+	{"getUser",		{handleGetUserRequest,			false}},
-	{"/rest/getSimilarSongs2.view",		{handleGetSimilarSongs2Request,		false}},
+	{"getUsers",		{handleGetUsersRequest,			true}},
-	{"/rest/getStarred.view",		{handleGetStarredRequest,		false}},
+	{"ping",		{handlePingRequest,			false}},
-	{"/rest/getStarred2.view",		{handleGetStarred2Request,		false}},
+	{"search2",		{handleSearch2Request,			false}},
-	{"/rest/getPlaylist.view",		{handleGetPlaylistRequest,		false}},
+	{"search3",		{handleSearch3Request,			false}},
-	{"/rest/getPlaylists.view",		{handleGetPlaylistsRequest,		false}},
+	{"star",		{handleStarRequest,			false}},
-	{"/rest/getSongsByGenre.view",		{handleGetSongsByGenreRequest,		false}},
+	{"unstar",		{handleUnstarRequest,			false}},
-	{"/rest/getUser.view",			{handleGetUserRequest,			false}},
+	{"updateUser",		{handleUpdateUserRequest,		true}},
-	{"/rest/getUsers.view",			{handleGetUsersRequest,			true}},
+	{"updatePlaylist",	{handleUpdatePlaylistRequest,		false}},
-	{"/rest/ping.view",			{handlePingRequest,			false}},
-	{"/rest/savePlayQueue.view",		{handleNotImplementedRequest,		false}},
-	{"/rest/scrobble.view",			{handleNotImplementedRequest,		false}},
-	{"/rest/search2.view",			{handleSearch2Request,			false}},
-	{"/rest/search3.view",			{handleSearch3Request,			false}},
-	{"/rest/setRating.view",		{handleNotImplementedRequest,		false}},
-	{"/rest/star.view",			{handleStarRequest,			false}},
-	{"/rest/unstar.view",			{handleUnstarRequest,			false}},
-	{"/rest/updateShare.view",		{handleNotImplementedRequest,		false}},
-	{"/rest/updateUser.view",		{handleUpdateUserRequest,		true}},
-	{"/rest/updatePlaylist.view",		{handleUpdatePlaylistRequest,		false}},
 };
 
 using MediaRetrievalHandlerFunc = std::function<MediaRetrievalResult(RequestContext&, Wt::Http::ResponseContinuation*)>;
 static std::map<std::string, MediaRetrievalHandlerFunc> mediaRetrievalHandlers
 {
-	{"/rest/stream.view",		handleStream},
+	{"/rest/stream",		handleStream},
-	{"/rest/getCoverArt.view",	handleGetCoverArt},
+	{"/rest/getCoverArt",	handleGetCoverArt},
 };
 
 void
@@ -1857,7 +1848,7 @@ SubsonicResource::handleRequest(const Wt::Http::Request &request, Wt::Http::Resp
 
 	const std::size_t requestId {curRequestId++};
 
-	LMS_LOG(API_SUBSONIC, DEBUG) << "Handling request " << requestId << " '" << request.path() << "', continuation = " << (request.continuation() ? "true" : "false") << ", params = " << parameterMapToDebugString(request.getParameterMap());
+	LMS_LOG(API_SUBSONIC, DEBUG) << "Handling request " << requestId << " '" << request.pathInfo() << "', continuation = " << (request.continuation() ? "true" : "false") << ", params = " << parameterMapToDebugString(request.getParameterMap());
 
 	const Wt::Http::ParameterMap& parameters {request.getParameterMap()};
 
@@ -1866,14 +1857,12 @@ SubsonicResource::handleRequest(const Wt::Http::Request &request, Wt::Http::Resp
 
 	try
 	{
+		std::string requestPath {request.pathInfo()};
+		if (stringEndsWith(requestPath, ".view"))
+			requestPath.resize(requestPath.length() - 5);
+
 		// Mandatory parameters
 		const ClientInfo clientInfo {getClientInfo(parameters)};
-		if (clientInfo.version.major > API_VERSION_MAJOR)
-			throw Error {Error::Code::ServerMustUpgrade};
-		if (clientInfo.version.major < API_VERSION_MAJOR)
-			throw Error {Error::Code::ClientMustUpgrade};
-		if (clientInfo.version.minor > API_VERSION_MINOR)
-			throw Error {Error::Code::ServerMustUpgrade};
 
 		Session& dbSession {getOrCreateDbSession(_db)};
 
@@ -1891,7 +1880,7 @@ SubsonicResource::handleRequest(const Wt::Http::Request &request, Wt::Http::Resp
 
 		RequestContext requestContext {.parameters = parameters, .dbSession = dbSession, .userName = clientInfo.user};
 
-		auto itEntryPoint {requestEntryPoints.find(request.path())};
+		auto itEntryPoint {requestEntryPoints.find(requestPath)};
 		if (itEntryPoint != requestEntryPoints.end())
 		{
 			if (itEntryPoint->second.mustBeAdmin)
@@ -1915,7 +1904,7 @@ SubsonicResource::handleRequest(const Wt::Http::Request &request, Wt::Http::Resp
 			return;
 		}
 
-		auto itStreamHandler {mediaRetrievalHandlers.find(request.path())};
+		auto itStreamHandler {mediaRetrievalHandlers.find(requestPath)};
 		if (itStreamHandler != mediaRetrievalHandlers.end())
 		{
 			MediaRetrievalResult res {itStreamHandler->second(requestContext, request.continuation())};
@@ -1943,7 +1932,7 @@ SubsonicResource::handleRequest(const Wt::Http::Request &request, Wt::Http::Resp
 		}
 
 		LMS_LOG(API_SUBSONIC, ERROR) << "Unhandled command '" << request.path() << "'";
-		throw Error {Error::CustomType::InternalError};
+		throw Error {Error::CustomType::NotImplemented};
 	}
 	catch (const Error& e)
 	{
@@ -1956,19 +1945,5 @@ SubsonicResource::handleRequest(const Wt::Http::Request &request, Wt::Http::Resp
 	}
 }
 
-std::vector<std::string>
-SubsonicResource::getPaths()
-{
-	std::vector<std::string> paths;
-
-	for (auto it : requestEntryPoints)
-		paths.emplace_back(it.first);
-
-	for (auto it : mediaRetrievalHandlers)
-		paths.emplace_back(it.first);
-
-	return paths;
-}
-
 } // namespace api::subsonic
 

src/api/subsonic/SubsonicResource.hpp

@@ -34,7 +34,7 @@ class SubsonicResource final : public Wt::WResource
 		SubsonicResource(Database::Database& db);
 		~SubsonicResource();
 
-		static std::vector<std::string> getPaths();
+		static std::string getPath() { return "/rest/"; }
 	private:
 
 		void handleRequest(const Wt::Http::Request &request, Wt::Http::Response &response) override;

src/api/subsonic/SubsonicResponse.cpp

@@ -78,9 +78,9 @@ CustomTypeToString(Error::CustomType type)
 		case Error::CustomType::BadIdFormat:
 			return "Bad id format";
 		case Error::CustomType::BadPasswordFormat:
-			return "Internal error";
-		case Error::CustomType::InternalError:
 			return "Bad password format";
+		case Error::CustomType::InternalError:
+			return "Internal error";
 		case Error::CustomType::LoginThrottled:
 			return "Login throttled, too many attempts";
 		case Error::CustomType::NotImplemented:

src/main/main.cpp

@@ -147,10 +147,7 @@ int main(int argc, char* argv[])
 
 		// bind API resources
 		if (getService<Config>()->getBool("api-subsonic", true))
-		{
+			server.addResource(&subsonicResource, subsonicResource.getPath());
-			for (const std::string& path : API::Subsonic::SubsonicResource::getPaths())
-				server.addResource(&subsonicResource, path);
-		}
 
 		// bind UI entry point
 		server.addEntryPoint(Wt::EntryPointType::Application,

src/utils/Utils.cpp

@@ -117,17 +117,24 @@ bufferToString(const std::vector<unsigned char>& data)
 }
 
 std::string
-replaceInString(std::string str, const std::string& from, const std::string& to)
+replaceInString(const std::string& str, const std::string& from, const std::string& to)
 {
-    size_t pos = 0;
+	std::string res {str};
+	size_t pos = 0;
 
-    while ((pos = str.find(from, pos)) != std::string::npos)
+	while ((pos = res.find(from, pos)) != std::string::npos)
-    {
+	{
-         str.replace(pos, from.length(), to);
+		res.replace(pos, from.length(), to);
-         pos += to.length();
+		pos += to.length();
-    }
+	}
 
-    return str;
+	return res;
+}
+
+bool
+stringEndsWith(const std::string& str, const std::string& ending)
+{
+	return boost::algorithm::ends_with(str, ending);
 }
 
 boost::optional<std::string>

src/utils/Utils.hpp

@@ -66,7 +66,10 @@ boost::optional<T> readAs(const std::string& str)
 }
 
 std::string
-replaceInString(std::string str, const std::string& from, const std::string& to);
+replaceInString(const std::string& str, const std::string& from, const std::string& to);
+
+bool
+stringEndsWith(const std::string& str, const std::string& ending);
 
 boost::optional<std::string>
 stringFromHex(const std::string& str);