Review fixes

Make it ignore a // at the beginning of the URL

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)) {
+    // Whitespace could interfere with the next step
-        qCCritical(external_resource) << "External resource " << path << " was requested from unrecognized bucket " << bucket;
+    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()) {
+    if (!pathQUrl.isRelative()) {
-        qCCritical(external_resource) << "External resource " << path << " was requested from bucket " << bucket
+        qCWarning(external_resource) << "External resource " << pathQUrl << " 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
                                      << " 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 filteredPath = path;
+    QUrl merged = base.resolved(pathQUrl).adjusted(QUrl::NormalizePathSegments);
-    QString pathQString = filteredPath.path();
 
-    // De-duplicate URL separators, since S3 doesn't actually have subdirectories, and treats a '/' as a part
+    if (merged.isValid()) {
-    // 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 " 
+        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;

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) {
+    QString getUrl(Bucket bucket, QString path) {
-       return ExternalResource::getQUrl(bucket, QUrl(path)).toString();
+       return ExternalResource::getQUrl(bucket, path).toString();
     };
 
     /**