From 7aac2e69167718f44bd1b1f64c28fd9acc3a28f1 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Wed, 19 Oct 2016 10:57:23 -0700 Subject: [PATCH 1/5] restrict asset file mapping operations to file path --- assignment-client/src/assets/AssetServer.cpp | 6 +++--- libraries/networking/src/AssetUtils.cpp | 5 +++++ libraries/networking/src/AssetUtils.h | 4 +++- libraries/networking/src/MappingRequest.cpp | 6 +++--- 4 files changed, 14 insertions(+), 7 deletions(-) diff --git a/assignment-client/src/assets/AssetServer.cpp b/assignment-client/src/assets/AssetServer.cpp index 905cc6fd30..2e93de7030 100644 --- a/assignment-client/src/assets/AssetServer.cpp +++ b/assignment-client/src/assets/AssetServer.cpp @@ -453,7 +453,7 @@ bool AssetServer::loadMappingsFromFile() { while (it != _fileMappings.end()) { bool shouldDrop = false; - if (!isValidPath(it.key())) { + if (!isValidFilePath(it.key())) { qWarning() << "Will not keep mapping for" << it.key() << "since it is not a valid path."; shouldDrop = true; } @@ -508,7 +508,7 @@ bool AssetServer::writeMappingsToFile() { bool AssetServer::setMapping(AssetPath path, AssetHash hash) { path = path.trimmed(); - if (!isValidPath(path)) { + if (!isValidFilePath(path)) { qWarning() << "Cannot set a mapping for invalid path:" << path << "=>" << hash; return false; } @@ -637,7 +637,7 @@ bool AssetServer::renameMapping(AssetPath oldPath, AssetPath newPath) { oldPath = oldPath.trimmed(); newPath = newPath.trimmed(); - if (!isValidPath(oldPath) || !isValidPath(newPath)) { + if (!isValidFilePath(oldPath) || !isValidFilePath(newPath)) { qWarning() << "Cannot perform rename with invalid paths - both should have leading forward slashes:" << oldPath << "=>" << newPath; diff --git a/libraries/networking/src/AssetUtils.cpp b/libraries/networking/src/AssetUtils.cpp index c505d108e5..7818c8e5ce 100644 --- a/libraries/networking/src/AssetUtils.cpp +++ b/libraries/networking/src/AssetUtils.cpp @@ -70,6 +70,11 @@ bool saveToCache(const QUrl& url, const QByteArray& file) { return false; } +bool isValidFilePath(const AssetPath& filePath) { + QRegExp filePathRegex { ASSET_FILE_PATH_REGEX_STRING }; + return filePathRegex.exactMatch(filePath); +} + bool isValidPath(const AssetPath& path) { QRegExp pathRegex { ASSET_PATH_REGEX_STRING }; return pathRegex.exactMatch(path); diff --git a/libraries/networking/src/AssetUtils.h b/libraries/networking/src/AssetUtils.h index 3a663af420..60153dc518 100644 --- a/libraries/networking/src/AssetUtils.h +++ b/libraries/networking/src/AssetUtils.h @@ -31,7 +31,8 @@ const size_t SHA256_HASH_LENGTH = 32; const size_t SHA256_HASH_HEX_LENGTH = 64; const uint64_t MAX_UPLOAD_SIZE = 1000 * 1000 * 1000; // 1GB -const QString ASSET_PATH_REGEX_STRING = "^\\/(?!\\/)(?:[^\\/]|\\/(?!\\/))*$"; +const QString ASSET_FILE_PATH_REGEX_STRING = "^\\/([^\\/\\0]+(\\/)?)*([^\\/\\0]+)$"; +const QString ASSET_PATH_REGEX_STRING = "^\\/([^\\/\\0]+(\\/)?)+$"; const QString ASSET_HASH_REGEX_STRING = QString("^[a-fA-F0-9]{%1}$").arg(SHA256_HASH_HEX_LENGTH); enum AssetServerError : uint8_t { @@ -59,6 +60,7 @@ QByteArray hashData(const QByteArray& data); QByteArray loadFromCache(const QUrl& url); bool saveToCache(const QUrl& url, const QByteArray& file); +bool isValidFilePath(const AssetPath& path); bool isValidPath(const AssetPath& path); bool isValidHash(const QString& hashString); diff --git a/libraries/networking/src/MappingRequest.cpp b/libraries/networking/src/MappingRequest.cpp index d3950f9f30..62a60c521b 100644 --- a/libraries/networking/src/MappingRequest.cpp +++ b/libraries/networking/src/MappingRequest.cpp @@ -57,7 +57,7 @@ GetMappingRequest::GetMappingRequest(const AssetPath& path) : _path(path.trimmed void GetMappingRequest::doStart() { // short circuit the request if the path is invalid - if (!isValidPath(_path)) { + if (!isValidFilePath(_path)) { _error = MappingRequest::InvalidPath; emit finished(this); return; @@ -139,7 +139,7 @@ SetMappingRequest::SetMappingRequest(const AssetPath& path, const AssetHash& has void SetMappingRequest::doStart() { // short circuit the request if the hash or path are invalid - auto validPath = isValidPath(_path); + auto validPath = isValidFilePath(_path); auto validHash = isValidHash(_hash); if (!validPath || !validHash) { _error = !validPath ? MappingRequest::InvalidPath : MappingRequest::InvalidHash; @@ -226,7 +226,7 @@ RenameMappingRequest::RenameMappingRequest(const AssetPath& oldPath, const Asset void RenameMappingRequest::doStart() { // short circuit the request if either of the paths are invalid - if (!isValidPath(_oldPath) || !isValidPath(_newPath)) { + if (!isValidFilePath(_oldPath) || !isValidFilePath(_newPath)) { _error = InvalidPath; emit finished(this); return; From b80f3fbd7bbd3fbc3818c98e5452f891cea03576 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Wed, 19 Oct 2016 11:33:35 -0700 Subject: [PATCH 2/5] fix error handling for failed asset upload --- interface/resources/qml/AssetServer.qml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/interface/resources/qml/AssetServer.qml b/interface/resources/qml/AssetServer.qml index ec866a6f0e..167fe60183 100644 --- a/interface/resources/qml/AssetServer.qml +++ b/interface/resources/qml/AssetServer.qml @@ -349,7 +349,7 @@ ScrollingWindow { }, function(err, path) { print(err, path); - if (!err) { + if (err === "") { uploadProgressLabel.text = "Upload Complete"; timer.interval = 1000; timer.repeat = false; @@ -362,11 +362,12 @@ ScrollingWindow { console.log("Asset Browser - finished uploading: ", fileUrl); reload(); } else { - if (err > 0) { + if (err !== -1) { console.log("Asset Browser - error uploading: ", fileUrl, " - error ", err); var box = errorMessageBox("There was an error uploading:\n" + fileUrl + "\n" + Assets.getErrorString(err)); box.selected.connect(reload); } + uploadSpinner.visible = false; uploadButton.enabled = true; uploadOpen = false; From eb8f67b612b470bb145d9b7999a70cf79849b9b6 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Wed, 19 Oct 2016 11:47:57 -0700 Subject: [PATCH 3/5] cleanup asset file regex, comment in renameMapping --- assignment-client/src/assets/AssetServer.cpp | 2 +- libraries/networking/src/AssetUtils.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/assignment-client/src/assets/AssetServer.cpp b/assignment-client/src/assets/AssetServer.cpp index 2e93de7030..2fbe2f6dfe 100644 --- a/assignment-client/src/assets/AssetServer.cpp +++ b/assignment-client/src/assets/AssetServer.cpp @@ -638,7 +638,7 @@ bool AssetServer::renameMapping(AssetPath oldPath, AssetPath newPath) { newPath = newPath.trimmed(); if (!isValidFilePath(oldPath) || !isValidFilePath(newPath)) { - qWarning() << "Cannot perform rename with invalid paths - both should have leading forward slashes:" + qWarning() << "Cannot perform rename with invalid paths - both should have leading forward and no ending slashes:" << oldPath << "=>" << newPath; return false; diff --git a/libraries/networking/src/AssetUtils.h b/libraries/networking/src/AssetUtils.h index 60153dc518..9e508418ab 100644 --- a/libraries/networking/src/AssetUtils.h +++ b/libraries/networking/src/AssetUtils.h @@ -31,7 +31,7 @@ const size_t SHA256_HASH_LENGTH = 32; const size_t SHA256_HASH_HEX_LENGTH = 64; const uint64_t MAX_UPLOAD_SIZE = 1000 * 1000 * 1000; // 1GB -const QString ASSET_FILE_PATH_REGEX_STRING = "^\\/([^\\/\\0]+(\\/)?)*([^\\/\\0]+)$"; +const QString ASSET_FILE_PATH_REGEX_STRING = "^(\\/[^\\/\\0]+)+$"; const QString ASSET_PATH_REGEX_STRING = "^\\/([^\\/\\0]+(\\/)?)+$"; const QString ASSET_HASH_REGEX_STRING = QString("^[a-fA-F0-9]{%1}$").arg(SHA256_HASH_HEX_LENGTH); From 179cb67cdb5e5895b735a85b3c34afdf1c506655 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Wed, 19 Oct 2016 11:55:48 -0700 Subject: [PATCH 4/5] change order of asset upload failure changes --- interface/resources/qml/AssetServer.qml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/interface/resources/qml/AssetServer.qml b/interface/resources/qml/AssetServer.qml index 167fe60183..8d9ff1df05 100644 --- a/interface/resources/qml/AssetServer.qml +++ b/interface/resources/qml/AssetServer.qml @@ -362,15 +362,15 @@ ScrollingWindow { console.log("Asset Browser - finished uploading: ", fileUrl); reload(); } else { + uploadSpinner.visible = false; + uploadButton.enabled = true; + uploadOpen = false; + if (err !== -1) { console.log("Asset Browser - error uploading: ", fileUrl, " - error ", err); var box = errorMessageBox("There was an error uploading:\n" + fileUrl + "\n" + Assets.getErrorString(err)); box.selected.connect(reload); } - - uploadSpinner.visible = false; - uploadButton.enabled = true; - uploadOpen = false; } }, dropping); } From e3f194b4dd1e76c7525d18567c9b72c2dbb08dfb Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Wed, 19 Oct 2016 12:41:01 -0700 Subject: [PATCH 5/5] use the err directly on upload error --- interface/resources/qml/AssetServer.qml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/interface/resources/qml/AssetServer.qml b/interface/resources/qml/AssetServer.qml index 8d9ff1df05..cf61a2ae4a 100644 --- a/interface/resources/qml/AssetServer.qml +++ b/interface/resources/qml/AssetServer.qml @@ -368,7 +368,7 @@ ScrollingWindow { if (err !== -1) { console.log("Asset Browser - error uploading: ", fileUrl, " - error ", err); - var box = errorMessageBox("There was an error uploading:\n" + fileUrl + "\n" + Assets.getErrorString(err)); + var box = errorMessageBox("There was an error uploading:\n" + fileUrl + "\n" + err); box.selected.connect(reload); } }