Merge pull request #753 from daleglass/improve-url-migrations

Sanitize URLs in ExternalResources

libraries/networking/src/ExternalResource.cpp

@@ -23,39 +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<std::mutex> guard(_bucketMutex);
+
+        if (!_bucketBases.contains(bucket)) {
+            qCCritical(external_resource) << "External resource " << path << " was requested from unrecognized bucket "
+                                          << bucket;
+            return QUrl();
+        }
+
         base = _bucketBases[bucket];
     }
 
-    QUrl merged = base.resolved(path).adjusted(QUrl::NormalizePathSegments);
+    QUrl merged = base.resolved(pathQUrl).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 " 
-                                      << merged.errorString() << "; base = " << base << "; path = " << path;
+        qCCritical(external_resource) << "External resource resolved to invalid URL " << merged << "; Error "
+                                      << merged.errorString() << "; base = " << base << "; path = " << path
+                                      << "; filtered path = " << pathQUrl;
     }
 
     return merged;

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();
     };
 
     /**

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);
@@ -693,8 +693,7 @@ public:
      * @param {string} path - The path within the external resource bucket where the asset is located. 
      *     <p>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 <code>./</code> or <code>../</code>, to navigate within the 
-     *     resource bucket's URL. Or it can be an absolute path starting with <code>/</code>, in which case the bucket's path
-     *     is discarded when calculating the asset's URL.</p>
+     *     resource bucket's URL.</p>
      * @Returns {string} The URL of an external asset.
      * @example <caption>Report the URL of a default particle.</caption>
      * print(Script.getExternalPath(Script.ExternalPaths.Assets, "Bazaar/Assets/Textures/Defaults/Interface/default_particle.png"));