From e13166a7b14d9e3e29e6448dfac87a70a2d64b0e Mon Sep 17 00:00:00 2001 From: Emma Malysz Date: Fri, 29 Jan 2021 22:56:54 +0000 Subject: [PATCH] Bug 1649611: Remove OS.File usage from DownloadIntegration.jsm and respect umask in IOUtils::SetPermissions r=barret Depends on D103520 Differential Revision: https://phabricator.services.mozilla.com/D99729 --- dom/chrome-webidl/IOUtils.webidl | 6 +- dom/chrome-webidl/PathUtils.webidl | 9 ++ dom/system/IOUtils.cpp | 13 ++- dom/system/IOUtils.h | 3 +- dom/system/PathUtils.cpp | 34 +++++++ dom/system/PathUtils.h | 3 + .../ioutils/test_ioutils_set_permissions.html | 32 +++++++ .../downloads/DownloadIntegration.jsm | 90 ++++++++----------- .../test/unit/common_test_Download.js | 6 +- .../test/unit/test_DownloadIntegration.js | 22 ++--- 10 files changed, 148 insertions(+), 70 deletions(-) diff --git a/dom/chrome-webidl/IOUtils.webidl b/dom/chrome-webidl/IOUtils.webidl index b34b9806b0d2..6ce51a66c900 100644 --- a/dom/chrome-webidl/IOUtils.webidl +++ b/dom/chrome-webidl/IOUtils.webidl @@ -187,11 +187,15 @@ namespace IOUtils { * * @param path An absolute file path * @param permissions The UNIX file mode representing the permissions. + * @param honorUmask If omitted or true, any UNIX file mode value is + * modified by the process umask. If false, the exact value + * of UNIX file mode will be applied. This value has no effect + * on Windows. * * @return Resolves if the permissions were set successfully, otherwise * rejects with a DOMException. */ - Promise setPermissions(DOMString path, unsigned long permissions); + Promise setPermissions(DOMString path, unsigned long permissions, optional boolean honorUmask = true); /** * Return whether or not the file exists at the given path. * diff --git a/dom/chrome-webidl/PathUtils.webidl b/dom/chrome-webidl/PathUtils.webidl index 9bc4c81fbd95..c07ae76f6ad1 100644 --- a/dom/chrome-webidl/PathUtils.webidl +++ b/dom/chrome-webidl/PathUtils.webidl @@ -58,6 +58,15 @@ namespace PathUtils { [Throws] DOMString createUniquePath(DOMString path); + /** + * Creates an adjusted path using a path whose length is already close + * to MAX_PATH. For windows only. + * + * @param path An absolute path. + */ + [Throws] + DOMString toExtendedWindowsPath(DOMString path); + /** * Normalize a path by removing multiple separators and `..` and `.` * directories. diff --git a/dom/system/IOUtils.cpp b/dom/system/IOUtils.cpp index e36344fc3082..2e2dea07537b 100644 --- a/dom/system/IOUtils.cpp +++ b/dom/system/IOUtils.cpp @@ -47,6 +47,10 @@ #include "prtime.h" #include "prtypes.h" +#ifndef ANDROID +# include "nsSystemInfo.h" +#endif + #define REJECT_IF_SHUTTING_DOWN(aJSPromise) \ do { \ if (sShutdownStarted) { \ @@ -588,7 +592,8 @@ already_AddRefed IOUtils::GetChildren(GlobalObject& aGlobal, /* static */ already_AddRefed IOUtils::SetPermissions(GlobalObject& aGlobal, const nsAString& aPath, - const uint32_t aPermissions) { + uint32_t aPermissions, + const bool aHonorUmask) { MOZ_ASSERT(XRE_IsParentProcess()); RefPtr promise = CreateJSPromise(aGlobal); if (!promise) { @@ -598,6 +603,12 @@ already_AddRefed IOUtils::SetPermissions(GlobalObject& aGlobal, nsCOMPtr file = new nsLocalFile(); REJECT_IF_INIT_PATH_FAILED(file, aPath, promise); +#if defined(XP_UNIX) && !defined(ANDROID) + if (aHonorUmask) { + aPermissions &= ~nsSystemInfo::gUserUmask; + } +#endif + RunOnBackgroundThreadAndResolve( promise, [file = std::move(file), permissions = aPermissions]() { return SetPermissionsSync(file, permissions); diff --git a/dom/system/IOUtils.h b/dom/system/IOUtils.h index 177de0a461fe..ff9ee7a2676d 100644 --- a/dom/system/IOUtils.h +++ b/dom/system/IOUtils.h @@ -113,7 +113,8 @@ class IOUtils final { static already_AddRefed SetPermissions(GlobalObject& aGlobal, const nsAString& aPath, - const uint32_t aPermissions); + uint32_t aPermissions, + const bool aHonorUmask); static already_AddRefed Exists(GlobalObject& aGlobal, const nsAString& aPath); diff --git a/dom/system/PathUtils.cpp b/dom/system/PathUtils.cpp index 98c7211b31fc..84d0a470317b 100644 --- a/dom/system/PathUtils.cpp +++ b/dom/system/PathUtils.cpp @@ -217,6 +217,40 @@ void PathUtils::CreateUniquePath(const GlobalObject&, const nsAString& aPath, MOZ_ALWAYS_SUCCEEDS(path->GetPath(aResult)); } +void PathUtils::ToExtendedWindowsPath(const GlobalObject&, + const nsAString& aPath, nsString& aResult, + ErrorResult& aErr) { +#ifndef XP_WIN + aErr.ThrowNotAllowedError("Operation is windows specific"_ns); + return; +#else + if (aPath.IsEmpty()) { + aErr.ThrowNotAllowedError(ERROR_EMPTY_PATH); + return; + } + + const nsAString& str1 = Substring(aPath, 1, 1); + const nsAString& str2 = Substring(aPath, 2, aPath.Length() - 2); + + bool isUNC = aPath.Length() >= 2 && + (aPath.First() == '\\' || aPath.First() == '/') && + (str1.EqualsLiteral("\\") || str1.EqualsLiteral("/")); + + constexpr auto pathPrefix = u"\\\\?\\"_ns; + const nsAString& uncPath = pathPrefix + u"UNC\\"_ns + str2; + const nsAString& normalPath = pathPrefix + aPath; + + nsCOMPtr path = new nsLocalFile(); + if (nsresult rv = path->InitWithPath(isUNC ? uncPath : normalPath); + NS_FAILED(rv)) { + ThrowError(aErr, rv, ERROR_INITIALIZE_PATH); + return; + } + + MOZ_ALWAYS_SUCCEEDS(path->GetPath(aResult)); +#endif +} + void PathUtils::Normalize(const GlobalObject&, const nsAString& aPath, nsString& aResult, ErrorResult& aErr) { if (aPath.IsEmpty()) { diff --git a/dom/system/PathUtils.h b/dom/system/PathUtils.h index 05059d0d4469..9e43f911eece 100644 --- a/dom/system/PathUtils.h +++ b/dom/system/PathUtils.h @@ -39,6 +39,9 @@ class PathUtils final { static void CreateUniquePath(const GlobalObject&, const nsAString& aPath, nsString& aResult, ErrorResult& aErr); + static void ToExtendedWindowsPath(const GlobalObject&, const nsAString& aPath, + nsString& aResult, ErrorResult& aErr); + static void Normalize(const GlobalObject&, const nsAString& aPath, nsString& aResult, ErrorResult& aErr); diff --git a/dom/system/tests/ioutils/test_ioutils_set_permissions.html b/dom/system/tests/ioutils/test_ioutils_set_permissions.html index 8cf4494c5dbe..15e9e34a1ca0 100644 --- a/dom/system/tests/ioutils/test_ioutils_set_permissions.html +++ b/dom/system/tests/ioutils/test_ioutils_set_permissions.html @@ -23,6 +23,38 @@ let stat = await IOUtils.stat(tempFile); + if (Services.appinfo.OS === "WINNT") { + // setPermissions ignores the x bit on Windows. + is(stat.permissions, 0o666, "Permissions munged on Windows"); + } else { + let umask = Services.sysinfo.getProperty("umask"); + is(stat.permissions, 0o421 & ~umask, "Permissions match"); + } + + await IOUtils.setPermissions(tempFile, 0o400); + stat = await IOUtils.stat(tempFile); + + if (Services.appinfo.OS === "WINNT") { + is(stat.permissions, 0o444, "Permissions munged on Windows"); + + // We need to make the file writable to delete it on Windows. + await IOUtils.setPermissions(tempFile, 0o600); + } else { + is(stat.permissions, 0o400, "Permissions match"); + } + + await cleanup(tempFile); + }); + + add_task(async function test_setPermissionsWithoutHonoringUmask() { + const tempDir = await PathUtils.getTempDir(); + const tempFile = PathUtils.join(tempDir, "setPermissions.tmp"); + + await IOUtils.writeUTF8(tempFile, ""); + await IOUtils.setPermissions(tempFile, 0o421, false); + + let stat = await IOUtils.stat(tempFile); + if (Services.appinfo.OS === "WINNT") { // setPermissions ignores the x bit on Windows. is(stat.permissions, 0o666, "Permissions munged on Windows"); diff --git a/toolkit/components/downloads/DownloadIntegration.jsm b/toolkit/components/downloads/DownloadIntegration.jsm index c3397fba5ac0..94e0198130c3 100644 --- a/toolkit/components/downloads/DownloadIntegration.jsm +++ b/toolkit/components/downloads/DownloadIntegration.jsm @@ -58,7 +58,6 @@ ChromeUtils.defineModuleGetter( "NetUtil", "resource://gre/modules/NetUtil.jsm" ); -ChromeUtils.defineModuleGetter(this, "OS", "resource://gre/modules/osfile.jsm"); ChromeUtils.defineModuleGetter( this, "PlacesUtils", @@ -264,7 +263,7 @@ var DownloadIntegration = { this._store = new DownloadStore( list, - OS.Path.join(OS.Constants.Path.profileDir, "downloads.json") + PathUtils.join(await PathUtils.getProfileDir(), "downloads.json") ); this._store.onsaveitem = this.shouldPersistDownload.bind(this); @@ -371,7 +370,9 @@ var DownloadIntegration = { Ci.nsIFile ); directoryPath = directory.path; - await OS.File.makeDir(directoryPath, { ignoreExisting: true }); + await IOUtils.makeDirectory(directoryPath, { + createAncestors: false, + }); } catch (ex) { // Either the preference isn't set or the directory cannot be created. directoryPath = await this.getSystemDownloadsDirectory(); @@ -497,7 +498,7 @@ var DownloadIntegration = { referrerInfo: aDownload.source.referrerInfo, fileSize: aDownload.currentBytes, sha256Hash: hash, - suggestedFileName: OS.Path.basename(aDownload.target.path), + suggestedFileName: PathUtils.filename(aDownload.target.path), signatureInfo: sigInfo, redirects: channelRedirects, }, @@ -604,40 +605,30 @@ var DownloadIntegration = { // whatever reason. zone = Ci.mozIDownloadPlatform.ZONE_INTERNET; } - try { - // Don't write zone IDs for Local, Intranet, or Trusted sites - // to match Windows behavior. - if (zone >= Ci.mozIDownloadPlatform.ZONE_INTERNET) { - let streamPath = aDownload.target.path + ":Zone.Identifier"; - let stream = await OS.File.open( - streamPath, - { create: true }, - { winAllowLengthBeyondMaxPathWithCaveats: true } - ); - try { - let zoneId = "[ZoneTransfer]\r\nZoneId=" + zone + "\r\n"; - let { url, isPrivate, referrerInfo } = aDownload.source; - if (!isPrivate) { - let referrer = referrerInfo - ? referrerInfo.computedReferrerSpec - : ""; - zoneId += - this._zoneIdKey("ReferrerUrl", referrer) + - this._zoneIdKey("HostUrl", url, "about:internet"); - } - await stream.write(new TextEncoder().encode(zoneId)); - } finally { - await stream.close(); + // Don't write zone IDs for Local, Intranet, or Trusted sites + // to match Windows behavior. + if (zone >= Ci.mozIDownloadPlatform.ZONE_INTERNET) { + let path = aDownload.target.path + ":Zone.Identifier"; + try { + let zoneId = "[ZoneTransfer]\r\nZoneId=" + zone + "\r\n"; + let { url, isPrivate, referrerInfo } = aDownload.source; + if (!isPrivate) { + let referrer = referrerInfo + ? referrerInfo.computedReferrerSpec + : ""; + zoneId += + this._zoneIdKey("ReferrerUrl", referrer) + + this._zoneIdKey("HostUrl", url, "about:internet"); + } + await IOUtils.writeUTF8( + PathUtils.toExtendedWindowsPath(path), + zoneId + ); + } catch (ex) { + // If writing to the file fails, we ignore the error and continue. + if (!(ex instanceof DOMException)) { + Cu.reportError(ex); } - } - } catch (ex) { - // If writing to the stream fails, we ignore the error and continue. - // The Windows API error 123 (ERROR_INVALID_NAME) is expected to - // occur when working on a file system that does not support - // Alternate Data Streams, like FAT32, thus we don't report this - // specific error. - if (!(ex instanceof OS.File.Error) || ex.winLastError != 123) { - Cu.reportError(ex); } } } @@ -660,26 +651,19 @@ var DownloadIntegration = { )); // Permanently downloaded files are made accessible by other users on // this system, while temporary downloads are marked as read-only. - let options = {}; + let unixMode; if (isTemporaryDownload) { - options.unixMode = 0o400; - options.winAttributes = { readOnly: true }; + unixMode = 0o400; } else { - options.unixMode = 0o666; + unixMode = 0o666; } // On Unix, the umask of the process is respected. - await OS.File.setPermissions(aDownload.target.path, options); + await IOUtils.setPermissions(aDownload.target.path, unixMode); } catch (ex) { // We should report errors with making the permissions less restrictive // or marking the file as read-only on Unix and Mac, but this should not // prevent the download from completing. - // The setPermissions API error EPERM is expected to occur when working - // on a file system that does not support file permissions, like FAT32, - // thus we don't report this error. - if ( - !(ex instanceof OS.File.Error) || - ex.unixErrno != OS.Constants.libc.EPERM - ) { + if (!(ex instanceof DOMException)) { Cu.reportError(ex); } } @@ -958,15 +942,15 @@ var DownloadIntegration = { // We read the name of the directory from the list of translated strings // that is kept by the UI helper module, even if this string is not strictly // displayed in the user interface. - let directoryPath = OS.Path.join( + let directoryPath = PathUtils.join( this._getDirectory(aName), DownloadUIHelper.strings.downloadsFolder ); // Create the Downloads folder and ignore if it already exists. - return OS.File.makeDir(directoryPath, { ignoreExisting: true }).then( - () => directoryPath - ); + return IOUtils.makeDirectory(directoryPath, { + createAncestors: false, + }).then(() => directoryPath); }, /** diff --git a/toolkit/components/downloads/test/unit/common_test_Download.js b/toolkit/components/downloads/test/unit/common_test_Download.js index 86c3fbb54640..f15c5fcc2256 100644 --- a/toolkit/components/downloads/test/unit/common_test_Download.js +++ b/toolkit/components/downloads/test/unit/common_test_Download.js @@ -323,18 +323,18 @@ add_task(async function test_unix_permissions() { } let isTemporary = launchWhenSucceeded && (autoDelete || isPrivate); - let stat = await OS.File.stat(download.target.path); + let stat = await IOUtils.stat(download.target.path); if (Services.appinfo.OS == "WINNT") { // On Windows // Temporary downloads should be read-only - Assert.equal(stat.winAttributes.readOnly, !!isTemporary); + Assert.equal(stat.permissions, isTemporary ? 0o444 : 0o666); } else { // On Linux, Mac // Temporary downloads should be read-only and not accessible to other // users, while permanently downloaded files should be readable and // writable as specified by the system umask. Assert.equal( - stat.unixMode, + stat.permissions, isTemporary ? 0o400 : 0o666 & ~OS.Constants.Sys.umask ); } diff --git a/toolkit/components/downloads/test/unit/test_DownloadIntegration.js b/toolkit/components/downloads/test/unit/test_DownloadIntegration.js index f2750b48e34d..6914f7fa9041 100644 --- a/toolkit/components/downloads/test/unit/test_DownloadIntegration.js +++ b/toolkit/components/downloads/test/unit/test_DownloadIntegration.js @@ -83,25 +83,25 @@ add_task(async function test_getSystemDownloadsDirectory_exists_or_creates() { ) { downloadDir = await DownloadIntegration.getSystemDownloadsDirectory(); Assert.equal(downloadDir, tempDir.path); - Assert.ok(await OS.File.exists(downloadDir)); + Assert.ok(await IOUtils.exists(downloadDir)); - let info = await OS.File.stat(downloadDir); - Assert.ok(info.isDir); + let info = await IOUtils.stat(downloadDir); + Assert.equal(info.type, "directory"); } else { - let targetPath = OS.Path.join( + let targetPath = PathUtils.join( tempDir.path, gStringBundle.GetStringFromName("downloadsFolder") ); try { - await OS.File.removeEmptyDir(targetPath); + await IOUtils.remove(targetPath); } catch (e) {} downloadDir = await DownloadIntegration.getSystemDownloadsDirectory(); Assert.equal(downloadDir, targetPath); - Assert.ok(await OS.File.exists(downloadDir)); + Assert.ok(await IOUtils.exists(downloadDir)); - let info = await OS.File.stat(downloadDir); - Assert.ok(info.isDir); - await OS.File.removeEmptyDir(targetPath); + let info = await IOUtils.stat(downloadDir); + Assert.equal(info.type, "directory"); + await IOUtils.remove(targetPath); } }); @@ -163,8 +163,8 @@ add_task(async function test_getPreferredDownloadsDirectory() { downloadDir = await DownloadIntegration.getPreferredDownloadsDirectory(); Assert.notEqual(downloadDir, ""); Assert.equal(downloadDir, tempDir.path); - Assert.ok(await OS.File.exists(downloadDir)); - await OS.File.removeEmptyDir(tempDir.path); + Assert.ok(await IOUtils.exists(downloadDir)); + await IOUtils.remove(tempDir.path); // Should return the system downloads directory beacause the path is invalid // in the dir preference.