From cd7256a2f29867340c4e81a9a6a5f9ce22ca27d7 Mon Sep 17 00:00:00 2001 From: Lenart Kos Date: Fri, 3 Jul 2026 12:31:55 +0200 Subject: [PATCH 1/5] test: cover read-only file download and rename (#9885) Add sync tests validating the behavior changed by the read-only permission reorder in PropagateDownloadFile::downloadFinished(): - a read-only shared file downloads and ends up read-only locally - a remote change to an already-read-only local file re-downloads and overwrites it (the temp-file -> rename-over-read-only-destination path that regressed on Windows with Error 5) - a file regaining write permission on the server becomes writable locally again Signed-off-by: Lenart Kos --- test/testpermissions.cpp | 70 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/test/testpermissions.cpp b/test/testpermissions.cpp index dd9cf4e8ab5a0..3becf3bbe259f 100644 --- a/test/testpermissions.cpp +++ b/test/testpermissions.cpp @@ -1053,6 +1053,76 @@ private Q_SLOTS: QVERIFY(ensureReadOnlyItem("readOnlyFolder/newFolder")); } + // Regression test for https://github.com/nextcloud/desktop/issues/9885 + // + // A read-only shared file (for example a Talk attachment) must download + // successfully and end up read-only locally. The read-only permission is + // applied to the final file only after it has been renamed into place. + // Applying it to the temporary download file before the rename used to add + // a deny-DELETE ACE on Windows that blocked the rename, turning the download + // into a persistent Error 5 (Access Denied) retry loop. + void testDownloadReadOnlyFileMakesItReadOnly() + { + FakeFolder fakeFolder{FileInfo{}}; + auto &remote = fakeFolder.remoteModifier(); + + remote.insert("readOnlyFile.txt", 100, 'A'); + // "mG" excludes the W (CanWrite) permission, so the file is read-only. + remote.find("readOnlyFile.txt")->permissions = RemotePermissions::fromServerString("mG"); + + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + QVERIFY(!QFileInfo(fakeFolder.localPath() + "readOnlyFile.txt").isWritable()); + } + + // Companion to the test above: a remote change to a file that is already + // read-only locally must re-download and overwrite it. This exercises the + // temp-file -> rename-over-read-only-destination path that regressed in + // #9885, and confirms the file stays read-only afterwards. + void testUpdateExistingReadOnlyFileMakesItReadOnly() + { + FakeFolder fakeFolder{FileInfo{}}; + auto &remote = fakeFolder.remoteModifier(); + + remote.insert("readOnlyFile.txt", 100, 'A'); + remote.find("readOnlyFile.txt")->permissions = RemotePermissions::fromServerString("mG"); + + QVERIFY(fakeFolder.syncOnce()); + QVERIFY(!QFileInfo(fakeFolder.localPath() + "readOnlyFile.txt").isWritable()); + + // Remote update while the file is read-only locally: the client must + // download to a temp file and rename it over the read-only destination. + remote.setContents("readOnlyFile.txt", 'B'); + remote.appendByte("readOnlyFile.txt"); + + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + QVERIFY(!QFileInfo(fakeFolder.localPath() + "readOnlyFile.txt").isWritable()); + } + + // A file that becomes writable again on the server must lose its local + // read-only state on the next sync. Validates the read-write branch of the + // permission block that the fix moved after the rename. + void testDownloadReadWriteFileMakesItWritable() + { + FakeFolder fakeFolder{FileInfo{}}; + auto &remote = fakeFolder.remoteModifier(); + + remote.insert("file.txt", 100, 'A'); + remote.find("file.txt")->permissions = RemotePermissions::fromServerString("mG"); + + QVERIFY(fakeFolder.syncOnce()); + QVERIFY(!QFileInfo(fakeFolder.localPath() + "file.txt").isWritable()); + + // Grant write permission and change the content so the file re-downloads. + remote.find("file.txt")->permissions = RemotePermissions::fromServerString("WmG"); + remote.appendByte("file.txt"); + + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + QVERIFY(QFileInfo(fakeFolder.localPath() + "file.txt").isWritable()); + } + void testForbiddenDownload() { FakeFolder fakeFolder{FileInfo{}}; From ce038356f197e0096da105a3defb8f18ac450d0c Mon Sep 17 00:00:00 2001 From: Lenart Kos Date: Thu, 1 Oct 2026 14:59:33 +0200 Subject: [PATCH 2/5] test: reproduce read-only download rename failure on Windows (#9885) The existing read-only download tests pass even with the read-only permissions applied to the temporary file before the rename: the test folder grants "Full control", and FILE_DELETE_CHILD on the parent folder lets Windows rename the temporary file despite its deny-DELETE ACE. Restrict the sync folder to "Modify" permissions for the current user, which does not include FILE_DELETE_CHILD, so the rename depends on the DELETE access of the temporary file itself. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Lenart Kos --- test/testpermissions.cpp | 79 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/test/testpermissions.cpp b/test/testpermissions.cpp index 3becf3bbe259f..c3d4a255f6058 100644 --- a/test/testpermissions.cpp +++ b/test/testpermissions.cpp @@ -18,6 +18,10 @@ #include #include +#if defined(Q_OS_WIN) +#include +#endif + using namespace OCC; using namespace Qt::StringLiterals; @@ -56,6 +60,46 @@ static bool isReadOnlyFolder(const std::wstring &path) return FileSystem::isFolderReadOnly(std::filesystem::path{path}); } +#if defined(Q_OS_WIN) +// Replace the DACL of `path` with a protected one that only grants `accessPermissions` +// to the current user. The ACE is inherited by all files and folders below `path`. +static bool setCurrentUserOnlyDacl(const QString &path, DWORD accessPermissions) +{ + HANDLE tokenUnmanaged = nullptr; + if (!OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &tokenUnmanaged)) { + return false; + } + const auto token = Utility::UniqueHandle{tokenUnmanaged}; + + auto tokenUserSize = DWORD{0}; + GetTokenInformation(token.get(), TokenUser, nullptr, 0, &tokenUserSize); + auto tokenUserBuffer = std::vector(tokenUserSize); + if (!GetTokenInformation(token.get(), TokenUser, tokenUserBuffer.data(), tokenUserSize, &tokenUserSize)) { + return false; + } + const auto tokenUser = reinterpret_cast(tokenUserBuffer.data()); + + auto explicitAccess = EXPLICIT_ACCESS_W{}; + explicitAccess.grfAccessPermissions = accessPermissions; + explicitAccess.grfAccessMode = SET_ACCESS; + explicitAccess.grfInheritance = SUB_CONTAINERS_AND_OBJECTS_INHERIT; + explicitAccess.Trustee.TrusteeForm = TRUSTEE_IS_SID; + explicitAccess.Trustee.TrusteeType = TRUSTEE_IS_USER; + explicitAccess.Trustee.ptstrName = reinterpret_cast(tokenUser->User.Sid); + + PACL daclUnmanaged = nullptr; + if (SetEntriesInAclW(1, &explicitAccess, nullptr, &daclUnmanaged) != ERROR_SUCCESS) { + return false; + } + const auto dacl = Utility::UniqueLocalFree{daclUnmanaged}; + + auto windowsPath = FileSystem::longWinPath(QDir::cleanPath(path)); + const auto rawWindowsPath = reinterpret_cast(windowsPath.data()); + constexpr SECURITY_INFORMATION securityInfo = DACL_SECURITY_INFORMATION | PROTECTED_DACL_SECURITY_INFORMATION; + return SetNamedSecurityInfoW(rawWindowsPath, SE_FILE_OBJECT, securityInfo, nullptr, nullptr, dacl.get(), nullptr) == ERROR_SUCCESS; +} +#endif + SyncFileItemPtr findDiscoveryItem(const SyncFileItemVector &spy, const QString &path) { for (const auto &item : spy) { @@ -1100,6 +1144,41 @@ private Q_SLOTS: QVERIFY(!QFileInfo(fakeFolder.localPath() + "readOnlyFile.txt").isWritable()); } +#if defined(Q_OS_WIN) + // Same scenario as the two tests above, but in a sync folder where the user + // has "Modify" instead of "Full control" permissions. + // + // Renaming the temporary download file into place needs DELETE access on it. + // Windows also grants that through FILE_DELETE_CHILD on the parent folder, + // which is part of "Full control" but not of "Modify". Without it, a + // read-only ACL applied to the temporary file blocks the rename. + void testDownloadReadOnlyFileWithoutDeleteChildPermission() + { + FakeFolder fakeFolder{FileInfo{}}; + const auto localPath = fakeFolder.localPath(); + constexpr auto modifyPermissions = DWORD{FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE}; + QVERIFY(setCurrentUserOnlyDacl(localPath, modifyPermissions)); + const auto restoreFullControl = qScopeGuard([&localPath] { + setCurrentUserOnlyDacl(localPath, FILE_ALL_ACCESS); + }); + + auto &remote = fakeFolder.remoteModifier(); + remote.insert("readOnlyFile.txt", 100, 'A'); + remote.find("readOnlyFile.txt")->permissions = RemotePermissions::fromServerString("mG"); + + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + QVERIFY(!QFileInfo(localPath + "readOnlyFile.txt").isWritable()); + + remote.setContents("readOnlyFile.txt", 'B'); + remote.appendByte("readOnlyFile.txt"); + + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + QVERIFY(!QFileInfo(localPath + "readOnlyFile.txt").isWritable()); + } +#endif + // A file that becomes writable again on the server must lose its local // read-only state on the next sync. Validates the read-write branch of the // permission block that the fix moved after the rename. From ac76bcbb4fbbc106f459a6ff147e35e3329a1a49 Mon Sep 17 00:00:00 2001 From: Lenart Kos Date: Fri, 3 Jul 2026 12:30:45 +0200 Subject: [PATCH 3/5] fix(libsync): set read-only permissions after rename, not before setFileReadOnly() on Windows adds an ACCESS_DENIED_ACE for BUILTIN\Users to the file's DACL. Applying this to the temp file before renaming it to its final destination blocks the rename, because Windows requires Delete access on the source path to complete the operation. This caused a persistent Error 5 (Access Denied) retry loop for read-only shares and Talk attachments on Windows. Move the permission block to after uncheckedRenameReplace() succeeds, targeting the final filename instead of _tmpFile.fileName(). Fixes: #9885 Signed-off-by: Lenart Kos --- src/libsync/propagatedownload.cpp | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/libsync/propagatedownload.cpp b/src/libsync/propagatedownload.cpp index 5e850af085392..b337e02f149c0 100644 --- a/src/libsync/propagatedownload.cpp +++ b/src/libsync/propagatedownload.cpp @@ -1218,16 +1218,6 @@ void PropagateDownloadFile::downloadFinished() } } - if (_item->_locked == SyncFileItem::LockStatus::LockedItem && (_item->_lockOwnerType != SyncFileItem::LockOwnerType::UserLock || _item->_lockOwnerId != propagator()->account()->davUser())) { - qCDebug(lcPropagateDownload()) << _tmpFile.fileName() << "file is locked: making it read only"; - FileSystem::setFileReadOnly(_tmpFile.fileName(), true); - } else { - qCDebug(lcPropagateDownload()) << _tmpFile.fileName() << "file is not locked: making it" - << ((!_item->_remotePerm.isNull() && !_item->_remotePerm.hasPermission(RemotePermissions::CanWrite)) ? "read only" - : "read write"); - FileSystem::setFileReadOnlyWeak(_tmpFile.fileName(), (!_item->_remotePerm.isNull() && !_item->_remotePerm.hasPermission(RemotePermissions::CanWrite))); - } - const auto isConflict = (_item->_instruction == CSYNC_INSTRUCTION_CONFLICT && (FileSystem::isDir(filename) || !FileSystem::fileEquals(filename, _tmpFile.fileName()))) || _item->_instruction == CSYNC_INSTRUCTION_CASE_CLASH_CONFLICT; @@ -1292,6 +1282,17 @@ void PropagateDownloadFile::downloadFinished() FileSystem::setFileHidden(filename, false); + if (_item->_locked == SyncFileItem::LockStatus::LockedItem + && (_item->_lockOwnerType != SyncFileItem::LockOwnerType::UserLock || _item->_lockOwnerId != propagator()->account()->davUser())) { + qCDebug(lcPropagateDownload()) << filename << "file is locked: making it read only"; + FileSystem::setFileReadOnly(filename, true); + } else { + qCDebug(lcPropagateDownload()) << filename << "file is not locked: making it" + << ((!_item->_remotePerm.isNull() && !_item->_remotePerm.hasPermission(RemotePermissions::CanWrite)) ? "read only" + : "read write"); + FileSystem::setFileReadOnlyWeak(filename, (!_item->_remotePerm.isNull() && !_item->_remotePerm.hasPermission(RemotePermissions::CanWrite))); + } + if (_needParentFolderRestorePermissions) { FileSystem::setFolderPermissions(QString::fromStdWString(_parentPath.wstring()), FileSystem::FolderPermissions::ReadOnly); Q_EMIT propagator()->touchedFile(QString::fromStdWString(_parentPath.wstring())); From 283064270bff171e42bad2a93a1deeccef3baeb7 Mon Sep 17 00:00:00 2001 From: Lenart Kos Date: Thu, 1 Oct 2026 15:25:56 +0200 Subject: [PATCH 4/5] test: recover a read-only temporary download file on Windows (#9885) Clients that applied read-only permissions to the temporary download file left it behind fully downloaded after the rename into place failed. The download info in the journal still references it, so the next sync reuses it instead of downloading the file again. Recreate that state in a sync folder without FILE_DELETE_CHILD and check that the next sync renames the temporary file into place. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Lenart Kos --- test/testpermissions.cpp | 42 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/test/testpermissions.cpp b/test/testpermissions.cpp index c3d4a255f6058..9622b8d50facf 100644 --- a/test/testpermissions.cpp +++ b/test/testpermissions.cpp @@ -1177,6 +1177,48 @@ private Q_SLOTS: QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); QVERIFY(!QFileInfo(localPath + "readOnlyFile.txt").isWritable()); } + + // Clients affected by the bug above left a fully downloaded, read-only + // temporary file behind after failing to rename it into place. The next + // sync reuses that temporary file instead of downloading the file again, + // and must be able to rename it. + void testDownloadRecoversReadOnlyTemporaryFileWithoutDeleteChildPermission() + { + FakeFolder fakeFolder{FileInfo{}}; + const auto localPath = fakeFolder.localPath(); + constexpr auto modifyPermissions = DWORD{FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE}; + QVERIFY(setCurrentUserOnlyDacl(localPath, modifyPermissions)); + const auto restoreFullControl = qScopeGuard([&localPath] { + setCurrentUserOnlyDacl(localPath, FILE_ALL_ACCESS); + }); + + auto &remote = fakeFolder.remoteModifier(); + remote.insert("readOnlyFile.txt", 100, 'A'); + const auto remoteFile = remote.find("readOnlyFile.txt"); + remoteFile->permissions = RemotePermissions::fromServerString("mG"); + + // Recreate the state left behind by an affected client. + const auto tmpFileName = QStringLiteral(".readOnlyFile.txt.~1234abcd"); + const auto tmpFilePath = localPath + tmpFileName; + { + auto tmpFile = QFile{tmpFilePath}; + QVERIFY(tmpFile.open(QIODevice::WriteOnly)); + QCOMPARE(tmpFile.write(QByteArray(remoteFile->size, remoteFile->contentChar)), remoteFile->size); + } + FileSystem::setFileHidden(tmpFilePath, true); + FileSystem::setFileReadOnly(tmpFilePath, true); + + auto downloadInfo = SyncJournalDb::DownloadInfo{}; + downloadInfo._tmpfile = tmpFileName; + downloadInfo._etag = remoteFile->etag; + downloadInfo._valid = true; + fakeFolder.syncJournal().setDownloadInfo("readOnlyFile.txt", downloadInfo); + + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + QVERIFY(!QFileInfo::exists(tmpFilePath)); + QVERIFY(!QFileInfo(localPath + "readOnlyFile.txt").isWritable()); + } #endif // A file that becomes writable again on the server must lose its local From 85b29761423ab3a32e5c901fb1bcabbfdc22035d Mon Sep 17 00:00:00 2001 From: Lenart Kos Date: Thu, 1 Oct 2026 15:29:13 +0200 Subject: [PATCH 5/5] fix(libsync): clear read-only state of a reused temporary download file Clients that made the temporary download file read-only before renaming it could leave it behind fully downloaded when the rename failed on Windows. The next sync reuses it and skips straight to renaming it, but the read-only permissions were only cleared on the path that reopens the file to resume the download, so the rename kept failing. Clear them as soon as an existing temporary file is reused. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Lenart Kos --- src/libsync/propagatedownload.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/libsync/propagatedownload.cpp b/src/libsync/propagatedownload.cpp index b337e02f149c0..feecae685a233 100644 --- a/src/libsync/propagatedownload.cpp +++ b/src/libsync/propagatedownload.cpp @@ -673,6 +673,14 @@ void PropagateDownloadFile::startDownload() _tmpFile.setFileName(propagator()->fullLocalPath(tmpFileName)); makeParentFolderModifiable(_tmpFile.fileName()); + // Can't open(Append) read-only files, make sure to make + // file writable if it exists. + // Older clients also made the temporary file read-only before renaming it, which + // fails on Windows: clear that before reusing a complete temporary file too. + if (_tmpFile.exists()) { + FileSystem::setFileReadOnly(_tmpFile.fileName(), false); + } + _resumeStart = _tmpFile.size(); if (_resumeStart > 0 && _resumeStart == _item->_size) { qCInfo(lcPropagateDownload) << "File is already complete, no need to download"; @@ -680,12 +688,6 @@ void PropagateDownloadFile::startDownload() return; } - // Can't open(Append) read-only files, make sure to make - // file writable if it exists. - if (_tmpFile.exists()) { - FileSystem::setFileReadOnly(_tmpFile.fileName(), false); - } - if (!_tmpFile.open(QIODevice::Append | QIODevice::Unbuffered)) { qCWarning(lcPropagateDownload) << "could not open temporary file" << _tmpFile.fileName(); done(SyncFileItem::NormalError, _tmpFile.errorString(), ErrorCategory::GenericError);