From 2c0384c4a2b033a644a6ddeb03b0e7666395abde Mon Sep 17 00:00:00 2001 From: Brian Smith Date: Tue, 4 Jul 2023 13:58:36 -0500 Subject: [PATCH 1/4] Issue #2255 & #1240 - Simplify and enhance Maybe and Some(). https://bugzilla.mozilla.org/show_bug.cgi?id=1325351 This is a prerequisite for our BigInt V8 fast forward and potential #2255 fix. --- js/public/Proxy.h | 9 +++++++ mfbt/Maybe.h | 67 +++++++++++++++++++++++++++++++---------------- 2 files changed, 53 insertions(+), 23 deletions(-) diff --git a/js/public/Proxy.h b/js/public/Proxy.h index e493f522c6..981adc853c 100644 --- a/js/public/Proxy.h +++ b/js/public/Proxy.h @@ -588,6 +588,15 @@ class JS_FRIEND_API(AutoEnterPolicy) inline void recordLeave() {} #endif + private: + // This operator needs to be deleted explicitly, otherwise Visual C++ will + // create it automatically when it is part of the export JS API. In that + // case, compile would fail because HandleId is not allowed to be assigned + // and consequently instantiation of assign operator of mozilla::Maybe + // would fail. See bug 1325351 comment 16. Copy constructor is removed at + // the same time for consistency. + AutoEnterPolicy(const AutoEnterPolicy&) = delete; + AutoEnterPolicy& operator=(const AutoEnterPolicy&) = delete; }; #ifdef JS_DEBUG diff --git a/mfbt/Maybe.h b/mfbt/Maybe.h index 79df8d2518..bc123b047f 100644 --- a/mfbt/Maybe.h +++ b/mfbt/Maybe.h @@ -102,16 +102,11 @@ public: } /** - * Maybe can be copy-constructed from a Maybe if U* and T* are - * compatible, or from Maybe. + * Maybe can be copy-constructed from a Maybe if U is convertible to T. */ template::value && - (std::is_same::value || - (std::is_pointer::value && - std::is_base_of::type, - typename std::remove_pointer::type>::value))>::type> + typename std::enable_if::value>::type> MOZ_IMPLICIT Maybe(const Maybe& aOther) : mIsSome(false) @@ -131,16 +126,11 @@ public: } /** - * Maybe can be move-constructed from a Maybe if U* and T* are - * compatible, or from Maybe. + * Maybe can be move-constructed from a Maybe if U is convertible to T. */ template::value && - (std::is_same::value || - (std::is_pointer::value && - std::is_base_of::type, - typename std::remove_pointer::type>::value))>::type> + typename std::enable_if::value>::type> MOZ_IMPLICIT Maybe(Maybe&& aOther) : mIsSome(false) @@ -156,13 +146,7 @@ public: if (&aOther != this) { if (aOther.mIsSome) { if (mIsSome) { - // XXX(seth): The correct code for this branch, below, can't be used - // due to a bug in Visual Studio 2010. See bug 1052940. - /* ref() = aOther.ref(); - */ - reset(); - emplace(*aOther); } else { emplace(*aOther); } @@ -173,6 +157,23 @@ public: return *this; } + template::value>::type> + Maybe& operator=(const Maybe& aOther) + { + if (aOther.isSome()) { + if (mIsSome) { + ref() = aOther.ref(); + } else { + emplace(*aOther); + } + } else { + reset(); + } + return *this; + } + Maybe& operator=(Maybe&& aOther) { MOZ_ASSERT(this != &aOther, "Self-moves are prohibited"); @@ -191,6 +192,25 @@ public: return *this; } + template::value>::type> + Maybe& operator=(Maybe&& aOther) + { + if (aOther.isSome()) { + if (mIsSome) { + ref() = Move(aOther.ref()); + } else { + emplace(Move(*aOther)); + } + aOther.reset(); + } else { + reset(); + } + + return *this; + } + /* Methods that check whether this Maybe contains a value */ explicit operator bool() const { return isSome(); } bool isSome() const { return mIsSome; } @@ -443,11 +463,12 @@ public: * if you need to construct a Maybe value that holds a const, volatile, or * reference value, you need to use emplace() instead. */ -template -Maybe::Type>::Type> +template::type>::type> +Maybe Some(T&& aValue) { - typedef typename RemoveCV::Type>::Type U; Maybe value; value.emplace(Forward(aValue)); return value; From 96d1e2766f852cc538900676a3ff47796db9f423 Mon Sep 17 00:00:00 2001 From: Moonchild Date: Wed, 5 Jul 2023 19:27:03 +0200 Subject: [PATCH 2/4] [network] Prepare for requiring Authorization in CORS ACAH preflight The Authorization header with a JSON Web Token (JWT) can be sent via XMLHttpRequest without explicit authorization via Access-Control headers. According to the spec, this must always explicitly be mentioned in ACAH request headers and isn't allowed to be wildcarded. However, nobody currently obeys this rule and many websites are misconfigured because Chromium and Firefox always allowed it. This patch adds the more stricter code but keeps it behind an #ifdef 0 to be released later on if and when there is enough consensus on the web to obey this spec. This patch explicitly avoids the added complexity Mozilla added to educate web devs since our role in that respect is not significant. it's not preffed and it won't throw an explicit deprecation warning. See Mozilla bugs 1687364 and 1841019. --- netwerk/protocol/http/nsCORSListenerProxy.cpp | 32 +++++++++++++------ 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/netwerk/protocol/http/nsCORSListenerProxy.cpp b/netwerk/protocol/http/nsCORSListenerProxy.cpp index 499c03094b..d2c37415e5 100644 --- a/netwerk/protocol/http/nsCORSListenerProxy.cpp +++ b/netwerk/protocol/http/nsCORSListenerProxy.cpp @@ -1344,7 +1344,7 @@ nsCORSPreflightListener::CheckPreflightRequestApproved(nsIRequest* aRequest) headerVal); nsTArray headers; nsCCharSeparatedTokenizer headerTokens(headerVal, ','); - bool allowAllHeaders = false; + bool wildcard = false; while(headerTokens.hasMoreTokens()) { const nsDependentCSubstring& header = headerTokens.nextToken(); if (header.IsEmpty()) { @@ -1356,19 +1356,31 @@ nsCORSPreflightListener::CheckPreflightRequestApproved(nsIRequest* aRequest) return NS_ERROR_DOM_BAD_URI; } if (header.EqualsLiteral("*") && !mWithCredentials) { - allowAllHeaders = true; + wildcard = true; } else { headers.AppendElement(header); } } - if (!allowAllHeaders) { - for (uint32_t i = 0; i < mPreflightHeaders.Length(); ++i) { - if (!headers.Contains(mPreflightHeaders[i], - nsCaseInsensitiveCStringArrayComparator())) { - LogBlockedRequest(aRequest, "CORSMissingAllowHeaderFromPreflight", - NS_ConvertUTF8toUTF16(mPreflightHeaders[i]).get()); - return NS_ERROR_DOM_BAD_URI; - } + for (uint32_t i = 0; i < mPreflightHeaders.Length(); ++i) { + if (wildcard + // Access-Control-Allow-Headers is '*', so we should skip these checks. +#if 0 + && !mPreflightHeaders[i].LowerCaseEqualsASCII("authorization") + // However, according to the spec, 'Authorization' isn't allowed to be + // wildcarded here and must always be explicitly mentioned. + // Fixme: Mainstream keeps this disabled because nobody obeys this rule. + // This should be flipped on when either mainstream does or when there's enough + // effort to make websites adhere to the spec, to keep our implementation + // in line with the consensus on the web. +#endif + ) { + continue; + } + if (!headers.Contains(mPreflightHeaders[i], + nsCaseInsensitiveCStringArrayComparator())) { + LogBlockedRequest(aRequest, "CORSMissingAllowHeaderFromPreflight", + NS_ConvertUTF8toUTF16(mPreflightHeaders[i]).get()); + return NS_ERROR_DOM_BAD_URI; } } From a299eaa965f59c68806d8399141568ae376dbec4 Mon Sep 17 00:00:00 2001 From: Moonchild Date: Wed, 5 Jul 2023 21:55:00 +0200 Subject: [PATCH 3/4] [DOM] Filter out symlinks for webkitdirectory. This is effectively a back-out of the following patches from Bug 1274959 except we add comments and test coverage: - Part 1 which made the DirectoryListingTask include symlinks in the results as exposed by Directory.getFilesAndDirectories. - Part 3 which made GetFilesHelper include symlinks in the results. Test coverage for getFilesAndDirectories is provided by dom/filesystem/tests/test_basic.html by way of changes to its included file dom/filesystem/tests/filesystem_commons.js and changes to the createTreeFile helper in dom/filesystem/tests/script_fileList.js. Test coverage for GetFilesHelper is provided by dom/filesystem/tests/test_webkitdirectory.html and changes to the createTestFile helper in dom/filesystem/tests/script_fileList.js. Commenting out either of the `isLink` test in the relevant C++ code will cause the given tests to fail on non-windows platforms. --- dom/filesystem/GetDirectoryListingTask.cpp | 11 ++- dom/filesystem/GetFilesHelper.cpp | 82 ++---------------- dom/filesystem/GetFilesHelper.h | 9 -- dom/filesystem/tests/filesystem_commons.js | 4 + dom/filesystem/tests/script_fileList.js | 29 +++++++ .../tests/test_webkitdirectory.html | 86 +++++++++++++++++++ 6 files changed, 134 insertions(+), 87 deletions(-) diff --git a/dom/filesystem/GetDirectoryListingTask.cpp b/dom/filesystem/GetDirectoryListingTask.cpp index 1d4f77b3b8..8fdfb4a5bb 100644 --- a/dom/filesystem/GetDirectoryListingTask.cpp +++ b/dom/filesystem/GetDirectoryListingTask.cpp @@ -340,9 +340,14 @@ GetDirectoryListingTaskParent::IOWork() nsCOMPtr currFile = do_QueryInterface(supp); MOZ_ASSERT(currFile); - bool isSpecial, isFile; - if (NS_WARN_IF(NS_FAILED(currFile->IsSpecial(&isSpecial))) || - isSpecial) { + bool isLink, isSpecial, isFile; + if (NS_WARN_IF(NS_FAILED(currFile->IsSymlink(&isLink)) || + NS_FAILED(currFile->IsSpecial(&isSpecial))) || + // Although we allow explicit individual selection of symlinks via the + // file picker, we do not process symlinks in directory traversal. Our + // specific policy decision is documented at + // https://bugzilla.mozilla.org/show_bug.cgi?id=1813299#c20 + isLink || isSpecial) { continue; } if (NS_WARN_IF(NS_FAILED(currFile->IsFile(&isFile)) || diff --git a/dom/filesystem/GetFilesHelper.cpp b/dom/filesystem/GetFilesHelper.cpp index 563ef60b4a..fb3cf7a24d 100644 --- a/dom/filesystem/GetFilesHelper.cpp +++ b/dom/filesystem/GetFilesHelper.cpp @@ -333,13 +333,8 @@ GetFilesHelperBase::ExploreDirectory(const nsAString& aDOMPath, nsIFile* aFile) return NS_OK; } - nsresult rv = AddExploredDirectory(aFile); - if (NS_WARN_IF(NS_FAILED(rv))) { - return rv; - } - nsCOMPtr entries; - rv = aFile->GetDirectoryEntries(getter_AddRefs(entries)); + nsresult rv = aFile->GetDirectoryEntries(getter_AddRefs(entries)); if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } @@ -361,7 +356,12 @@ GetFilesHelperBase::ExploreDirectory(const nsAString& aDOMPath, nsIFile* aFile) bool isLink, isSpecial, isFile, isDir; if (NS_WARN_IF(NS_FAILED(currFile->IsSymlink(&isLink)) || NS_FAILED(currFile->IsSpecial(&isSpecial))) || - isSpecial) { + isSpecial || + // Although we allow explicit individual selection of symlinks via the + // file picker, we do not process symlinks in directory traversal. Our + // specific policy decision is documented at + // https://bugzilla.mozilla.org/show_bug.cgi?id=1813299#c20 + isLink) { continue; } @@ -371,11 +371,6 @@ GetFilesHelperBase::ExploreDirectory(const nsAString& aDOMPath, nsIFile* aFile) continue; } - // We don't want to explore loops of links. - if (isDir && isLink && !ShouldFollowSymLink(currFile)) { - continue; - } - // The new domPath nsAutoString domPath; domPath.Assign(aDOMPath); @@ -415,69 +410,6 @@ GetFilesHelperBase::ExploreDirectory(const nsAString& aDOMPath, nsIFile* aFile) return NS_OK; } -nsresult -GetFilesHelperBase::AddExploredDirectory(nsIFile* aDir) -{ - nsresult rv; - -#ifdef DEBUG - bool isDir; - rv = aDir->IsDirectory(&isDir); - if (NS_WARN_IF(NS_FAILED(rv))) { - return rv; - } - - MOZ_ASSERT(isDir, "Why are we here?"); -#endif - - bool isLink; - rv = aDir->IsSymlink(&isLink); - if (NS_WARN_IF(NS_FAILED(rv))) { - return rv; - } - - nsAutoCString path; - - if (!isLink) { - nsAutoString path16; - rv = aDir->GetPath(path16); - if (NS_WARN_IF(NS_FAILED(rv))) { - return rv; - } - - path = NS_ConvertUTF16toUTF8(path16); - } else { - rv = aDir->GetNativeTarget(path); - if (NS_WARN_IF(NS_FAILED(rv))) { - return rv; - } - } - - mExploredDirectories.PutEntry(path); - return NS_OK; -} - -bool -GetFilesHelperBase::ShouldFollowSymLink(nsIFile* aDir) -{ -#ifdef DEBUG - bool isLink, isDir; - if (NS_WARN_IF(NS_FAILED(aDir->IsSymlink(&isLink)) || - NS_FAILED(aDir->IsDirectory(&isDir)))) { - return false; - } - - MOZ_ASSERT(isLink && isDir, "Why are we here?"); -#endif - - nsAutoCString targetPath; - if (NS_WARN_IF(NS_FAILED(aDir->GetNativeTarget(targetPath)))) { - return false; - } - - return !mExploredDirectories.Contains(targetPath); -} - void GetFilesHelper::ResolveOrRejectPromise(Promise* aPromise) { diff --git a/dom/filesystem/GetFilesHelper.h b/dom/filesystem/GetFilesHelper.h index 4afd41d7e0..b6c58ef396 100644 --- a/dom/filesystem/GetFilesHelper.h +++ b/dom/filesystem/GetFilesHelper.h @@ -9,10 +9,8 @@ #include "mozilla/Mutex.h" #include "mozilla/RefPtr.h" #include "mozilla/dom/File.h" -#include "nsClassHashtable.h" #include "nsCycleCollectionTraversalCallback.h" #include "nsTArray.h" -#include "nsTHashtable.h" #include "nsThreadUtils.h" class nsIGlobalObject; @@ -57,17 +55,10 @@ protected: nsresult ExploreDirectory(const nsAString& aDOMPath, nsIFile* aFile); - nsresult - AddExploredDirectory(nsIFile* aDirectory); - - bool - ShouldFollowSymLink(nsIFile* aDirectory); - bool mRecursiveFlag; // We populate this array in the I/O thread with the BlobImpl. FallibleTArray> mTargetBlobImplArray; - nsTHashtable mExploredDirectories; }; // Retrieving the list of files can be very time/IO consuming. We use this diff --git a/dom/filesystem/tests/filesystem_commons.js b/dom/filesystem/tests/filesystem_commons.js index 4f7234121e..c8bd9ac9fc 100644 --- a/dom/filesystem/tests/filesystem_commons.js +++ b/dom/filesystem/tests/filesystem_commons.js @@ -38,6 +38,10 @@ function test_getFilesAndDirectories(aDirectory, aRecursive, aNext) { if (data[i] instanceof File) { is(data[i].webkitRelativePath, createRelativePath(dir, data[i]), "File.webkitRelativePath should be called: parentdir.path + '/' + file.name: " + data[i].webkitRelativePath); } + ok( + !data[i].webkitRelativePath.endsWith("symlink.txt"), + "We should never see a path ending with symlink.txt, our symlink sentinel." + ); } } ); diff --git a/dom/filesystem/tests/script_fileList.js b/dom/filesystem/tests/script_fileList.js index 89fd04cabe..dedc61a448 100644 --- a/dom/filesystem/tests/script_fileList.js +++ b/dom/filesystem/tests/script_fileList.js @@ -36,6 +36,15 @@ function createTreeFile(depth, parent) { if (depth == 0) { nextFile.append('file.txt'); nextFile.create(Components.interfaces.nsIFile.NORMAL_FILE_TYPE, 0o600); + +#ifdef XP_UNIX + // It's not possible to create symlinks on windows by default or on our + // Android platforms, so we can't create the symlink file there. Our + // callers that care are aware of this. + var linkFile = parent.clone(); + linkFile.append("symlink.txt"); + createSymLink(nextFile.path, linkFile.path); +#endif } else { nextFile.append('subdir' + depth); nextFile.createUnique(Components.interfaces.nsIFile.DIRECTORY_TYPE, 0o700); @@ -84,6 +93,15 @@ function createTestFile() { file2.append('bar.txt'); file2.create(Components.interfaces.nsIFile.NORMAL_FILE_TYPE, 0o600); +#ifdef XP_UNIX + // It's not possible to create symlinks on windows by default or on our + // Android platforms, so we can't create the symlink file there. Our + // callers that care are aware of this. + var linkFile = dir.clone(); + linkFile.append("symlink.txt"); + createSymLink(file1.path, linkFile.path); +#endif + return tmpFile; } @@ -127,3 +145,14 @@ addMessageListener("file.open", function (e) { file: File.createFromNsIFile(testFile) }); }); + +addMessageListener("symlink.open", function (e) { + let testDir = createTestFile(); + let testFile = testDir.clone(); + testFile.append("subdir"); + testFile.append("symlink.txt"); + + File.createFromNsIFile(testFile).then(function (file) { + sendAsyncMessage("symlink.opened", { dir: testDir.path, file }); + }); +}); diff --git a/dom/filesystem/tests/test_webkitdirectory.html b/dom/filesystem/tests/test_webkitdirectory.html index 591619e45b..46c728718f 100644 --- a/dom/filesystem/tests/test_webkitdirectory.html +++ b/dom/filesystem/tests/test_webkitdirectory.html @@ -9,11 +9,26 @@ +