From b243d382fcbcde033cdde38b825bafe220df0830 Mon Sep 17 00:00:00 2001 From: Dale Glass Date: Wed, 23 Sep 2020 22:34:01 +0200 Subject: [PATCH 1/3] Fix warning due to multiline comment --- libraries/script-engine/src/ScriptEngine.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/script-engine/src/ScriptEngine.h b/libraries/script-engine/src/ScriptEngine.h index 8eeb72e658..f393177c52 100644 --- a/libraries/script-engine/src/ScriptEngine.h +++ b/libraries/script-engine/src/ScriptEngine.h @@ -240,7 +240,7 @@ public: * @param {object} enum - Enum. * @deprecated This function is deprecated and will be removed. */ - // WARNING: This function must be called after a registerGlobalObject that creates the namespace this enum is located in, or\ + // WARNING: This function must be called after a registerGlobalObject that creates the namespace this enum is located in, or // the globalObject won't function. E.g., if you have a Foo object and a Foo.FooType enum, Foo must be registered first. /// registers a global enum Q_INVOKABLE void registerEnum(const QString& enumName, QMetaEnum newEnum); From 1d58ac4d793dd1842ea04f50b994ee2b455103b7 Mon Sep 17 00:00:00 2001 From: Dale Glass Date: Wed, 23 Sep 2020 23:33:22 +0200 Subject: [PATCH 2/3] Sanitize the URL further by removing leading slashes and any duplicated ones --- libraries/networking/src/ExternalResource.cpp | 23 +++++++++++++++++-- libraries/script-engine/src/ScriptEngine.h | 3 +-- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/libraries/networking/src/ExternalResource.cpp b/libraries/networking/src/ExternalResource.cpp index e2b6a40682..a760e3554f 100644 --- a/libraries/networking/src/ExternalResource.cpp +++ b/libraries/networking/src/ExternalResource.cpp @@ -49,13 +49,32 @@ QUrl ExternalResource::getQUrl(Bucket bucket, const QUrl& path) { base = _bucketBases[bucket]; } - QUrl merged = base.resolved(path).adjusted(QUrl::NormalizePathSegments); + QUrl filteredPath = path; + QString pathQString = filteredPath.path(); + + // De-duplicate URL separators, since S3 doesn't actually have subdirectories, and treats a '/' as a part + // of the filename. This means that "/dir/file.txt" and "/dir//file.txt" are not equivalent on S3. + while(pathQString.contains("//")) { + pathQString = pathQString.replace("//", "/"); + } + + // If a path starts with a / it would have the effect of overriding any path specified in the bucket. + // We remove it, ensuring that getQUrl(Bucket.Assets, "/file.txt") and getQUrl(Bucket.Assets, "file.txt") + // are equivalent. + if (pathQString.startsWith("/")) { + pathQString.remove(0,1); + } + + filteredPath.setPath(pathQString); + + QUrl merged = base.resolved(filteredPath).adjusted(QUrl::NormalizePathSegments); if ( merged.isValid() ) { qCDebug(external_resource) << "External resource resolved to " << merged; } else { qCCritical(external_resource) << "External resource resolved to invalid URL " << merged << "; Error " - << merged.errorString() << "; base = " << base << "; path = " << path; + << merged.errorString() << "; base = " << base << "; path = " << path + << "; filtered path = " << filteredPath; } return merged; diff --git a/libraries/script-engine/src/ScriptEngine.h b/libraries/script-engine/src/ScriptEngine.h index f393177c52..fff1cab00a 100644 --- a/libraries/script-engine/src/ScriptEngine.h +++ b/libraries/script-engine/src/ScriptEngine.h @@ -693,8 +693,7 @@ public: * @param {string} path - The path within the external resource bucket where the asset is located. *

Normally, this should start with a path or filename to be appended to the bucket URL. * Alternatively, it can be a relative path starting with ./ or ../, to navigate within the - * resource bucket's URL. Or it can be an absolute path starting with /, in which case the bucket's path - * is discarded when calculating the asset's URL.

+ * resource bucket's URL.

* @Returns {string} The URL of an external asset. * @example Report the URL of a default particle. * print(Script.getExternalPath(Script.ExternalPaths.Assets, "Bazaar/Assets/Textures/Defaults/Interface/default_particle.png")); From 9b4d115a5d40d462a7b7e8b4736ec4a10d1718f7 Mon Sep 17 00:00:00 2001 From: Dale Glass Date: Thu, 24 Sep 2020 19:37:14 +0200 Subject: [PATCH 3/3] Review fixes Make it ignore a // at the beginning of the URL --- libraries/networking/src/ExternalResource.cpp | 74 ++++++++++--------- libraries/networking/src/ExternalResource.h | 13 +++- 2 files changed, 49 insertions(+), 38 deletions(-) diff --git a/libraries/networking/src/ExternalResource.cpp b/libraries/networking/src/ExternalResource.cpp index a760e3554f..1058bddff8 100644 --- a/libraries/networking/src/ExternalResource.cpp +++ b/libraries/networking/src/ExternalResource.cpp @@ -23,58 +23,64 @@ ExternalResource* ExternalResource::getInstance() { return &instance; } -QUrl ExternalResource::getQUrl(Bucket bucket, const QUrl& path) { +QUrl ExternalResource::getQUrl(Bucket bucket, QString path) { qCDebug(external_resource) << "Requested URL for bucket " << bucket << ", path " << path; - if (!_bucketBases.contains(bucket)) { - qCCritical(external_resource) << "External resource " << path << " was requested from unrecognized bucket " << bucket; + // Whitespace could interfere with the next step + path = path.trimmed(); + + // Remove all initial slashes. This is because if QUrl is fed //foo/bar, + // it will treat //foo as a domain name. This also ensures the URL is always treated as + // relative to any path specified in the bucket. + while (path.startsWith('/')) { + path = path.remove(0, 1); + } + + // De-duplicate URL separators, since S3 doesn't actually have subdirectories, and treats a '/' as a part + // of the filename. This means that "/dir/file.txt" and "/dir//file.txt" are not equivalent on S3. + // + // We feed it through a QUrl to ensure we're only working on the path component. + QUrl pathQUrl(path); + + QString tempPath = pathQUrl.path(); + while (tempPath.contains("//")) { + tempPath = tempPath.replace("//", "/"); + } + pathQUrl.setPath(tempPath); + + if (!pathQUrl.isValid()) { + qCCritical(external_resource) << "External resource " << pathQUrl << " was requested from bucket " << bucket + << " with an invalid path. Error: " << pathQUrl.errorString(); return QUrl(); } - if (!path.isValid()) { - qCCritical(external_resource) << "External resource " << path << " was requested from bucket " << bucket - << " with an invalid path. Error: " << path.errorString(); - return QUrl(); - } - - if (!path.isRelative()) { - qCWarning(external_resource) << "External resource " << path << " was requested from bucket " << bucket + if (!pathQUrl.isRelative()) { + qCWarning(external_resource) << "External resource " << pathQUrl << " was requested from bucket " << bucket << " without using a relative path, returning as-is."; - return path; + return pathQUrl; } QUrl base; { std::lock_guard guard(_bucketMutex); + + if (!_bucketBases.contains(bucket)) { + qCCritical(external_resource) << "External resource " << path << " was requested from unrecognized bucket " + << bucket; + return QUrl(); + } + base = _bucketBases[bucket]; } - QUrl filteredPath = path; - QString pathQString = filteredPath.path(); + QUrl merged = base.resolved(pathQUrl).adjusted(QUrl::NormalizePathSegments); - // De-duplicate URL separators, since S3 doesn't actually have subdirectories, and treats a '/' as a part - // of the filename. This means that "/dir/file.txt" and "/dir//file.txt" are not equivalent on S3. - while(pathQString.contains("//")) { - pathQString = pathQString.replace("//", "/"); - } - - // If a path starts with a / it would have the effect of overriding any path specified in the bucket. - // We remove it, ensuring that getQUrl(Bucket.Assets, "/file.txt") and getQUrl(Bucket.Assets, "file.txt") - // are equivalent. - if (pathQString.startsWith("/")) { - pathQString.remove(0,1); - } - - filteredPath.setPath(pathQString); - - QUrl merged = base.resolved(filteredPath).adjusted(QUrl::NormalizePathSegments); - - if ( merged.isValid() ) { + if (merged.isValid()) { qCDebug(external_resource) << "External resource resolved to " << merged; } else { - qCCritical(external_resource) << "External resource resolved to invalid URL " << merged << "; Error " + qCCritical(external_resource) << "External resource resolved to invalid URL " << merged << "; Error " << merged.errorString() << "; base = " << base << "; path = " << path - << "; filtered path = " << filteredPath; + << "; filtered path = " << pathQUrl; } return merged; diff --git a/libraries/networking/src/ExternalResource.h b/libraries/networking/src/ExternalResource.h index 2c1e83da29..fa6eb85cfe 100644 --- a/libraries/networking/src/ExternalResource.h +++ b/libraries/networking/src/ExternalResource.h @@ -85,19 +85,24 @@ public: /** * Returns the location of a resource as a QUrl * - * Returns the location of the resource \p relative_path in bucket \p bucket + * Returns the location of the resource \p path in bucket \p bucket * * @note The resulting path will be sanitized by condensing multiple instances of '/' to one. * This is done for easier usage with Amazon S3 and compatible systems. * + * @par It will also convert all paths into relative ones respect to the bucket. + * + * @warning This function should only be given paths with a domain name. If given a complete path, + * it will emit a warning into the log and return the unmodified path it was given. + * * @param bucket The bucket in which the resource is found * @param relative_path The path of the resource within the bucket * @returns The resulting URL as a QUrl */ - QUrl getQUrl(Bucket bucket, const QUrl& path); + QUrl getQUrl(Bucket bucket, QString path); - QString getUrl(Bucket bucket, const QString& path) { - return ExternalResource::getQUrl(bucket, QUrl(path)).toString(); + QString getUrl(Bucket bucket, QString path) { + return ExternalResource::getQUrl(bucket, path).toString(); }; /**