fix(libsync): set read-only permissions after rename, not before by Lenart12 · Pull Request #10213 · nextcloud/desktop

Lenart12 · 2026-06-18T20:35:11Z

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

Copilot

Pull request overview

This PR addresses a Windows-specific download failure where applying “read-only” ACLs to the temporary download file prevents the subsequent rename/replace operation, causing persistent Error 5 (Access Denied) retry loops for read-only shares (including Talk attachments). It does so by moving the read-only permission application to after the final rename succeeds.

Changes:

Move setFileReadOnly*() calls from the temporary file (_tmpFile.fileName()) to the final destination path (filename) after FileSystem::uncheckedRenameReplace() succeeds.
Ensure the final file is un-hidden (setFileHidden(filename, false)) before applying the read-only/read-write decision.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Lenart12 · 2026-06-18T20:47:34Z

+    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)));
+    }


This condition was moved as-is, so the divergence from updateMetadata() is pre-existing. And updateMetadata() runs right after on the same path with the full UserLock/TokenLock check, so it overwrites this block anyway. Behavior is unchanged. Aligning the checks is a fine cleanup but out of scope here.

Lenart12 · 2026-06-18T20:48:07Z

+    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)));
+    }


Real behavioral change, good catch. That said, a case-clash conflict file is a local artifact the user has to resolve manually, so dropping read-only may actually be preferable. @ maintainer is read-only intended on these files? If so, the fix belongs in createCaseClashConflict() after its rename, not in downloadFinished().

Lenart12 · 2026-06-18T20:48:48Z

I checked Copilot suggestions, I think it might need some maintainer input though.

Lenart12 · 2026-06-18T21:27:38Z

Also I have checked the sync code paths and in theory all cases should handle an existing read-only temp file, so this bug should be "self healing" without any additional changes.

Lenart12 · 2026-07-02T09:40:58Z

It has been two weeks with no comment from anyone. This is ready for review, can anyone take a look at this please @mgallien

github-actions · 2026-07-03T02:20:41Z

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

mgallien · 2026-07-03T07:02:41Z

@Lenart12 I am sorry for the delay
we are a bit busy and I have it on my todo list

mgallien

@Lenart12 sorry again for the delay
since you are modifying syncing behavior, please add some automated tests changes to validate that
let us know if you have questions on that
automated tests changes are usually required for this part of the code base

mgallien · 2026-07-16T07:27:09Z

@Lenart12 gentle reminder
do you plan to keep working on this PR ?

Lenart12 · 2026-07-16T10:27:50Z

Thanks for reminding, I plan to, but not for a few weeks as im away on vacation.

AndyFx12 · 2026-08-13T15:55:54Z

Is there anyone, who can finish this PR? Current Nextcloud Desktop Client is simply unusable for me with read-only-shares.

mrtomatwo · 2026-09-01T12:41:01Z

@AndyFx12 I found a workaround that works for me: #9885 (comment)

But I'd also appreciate the proper fix being merged. Happy to help out if there's a lack of dev capacity :)

AchcarLucas · 2026-09-23T13:43:41Z

I think this PR is extremely important and critical, and it should be reviewed as soon as possible.

AchcarLucas · 2026-09-23T14:33:16Z

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

Temporary solution for those facing the problem

Since I'm dealing with this problem, I found a solution.

Perform the standard folder synchronization; you will still encounter file access errors. Once all the files in the folder have been synchronized (even with the errors), run the following commands in Windows PowerShell:

OBS: Run the commands below with synchronization paused.

# Checks the denied files
icacls . /T /C | Select-String -Pattern "DENY|NEGAR"

# Remove the negation
Get-ChildItem -LiteralPath . -Recurse -Force |
ForEach-Object {
    Write-Host "Removendo DENY: $($_.FullName)"
    icacls $_.FullName /remove:d "BUILTIN\Usuários"
}

# Check the denied files (again)
icacls . /T /C | Select-String -Pattern "DENY|NEGAR"

# forces the files to be read-only: false
Get-ChildItem -File -Recurse -Force |
Where-Object { $_.Name -match '\.~[0-9a-fA-F]+$' } |
ForEach-Object {
    $_.IsReadOnly = $false
}

# renames the temporary files
Get-ChildItem -File -Recurse -Force |
Where-Object { $_.Name -match '^\.([^\r\n]+)\.~[0-9a-fA-F]+$' } |
ForEach-Object {
    $novoNome = $_.Name -replace '^\.','' -replace '\.~[0-9a-fA-F]+$',''
    $destino = Join-Path $_.DirectoryName $novoNome

    if (-not (Test-Path -LiteralPath $destino)) {
        Rename-Item -LiteralPath $_.FullName -NewName $novoNome
        Write-Host "OK: $($_.Name) -> $novoNome"
    }
    else {
        Write-Host "IGNORADO - já existe: $destino"
    }
}

# check if any temporary files remain
Get-ChildItem -File -Recurse -Force |
Where-Object { $_.Name -match '\.~[0-9a-fA-F]+$' } |
Select-Object FullName

# removes the hidden attribute from the files
Get-ChildItem -File -Recurse -Force |
ForEach-Object {
    if ($_.Attributes -band [System.IO.FileAttributes]::Hidden) {
        $_.Attributes = $_.Attributes -bxor [System.IO.FileAttributes]::Hidden
        Write-Host "Desocultado: $($_.FullName)"
    }
}

# removes the hidden attribute from the folders
Get-ChildItem -Recurse -Force |
ForEach-Object {
    if ($_.Attributes -band [System.IO.FileAttributes]::Hidden) {
        $_.Attributes = $_.Attributes -bxor [System.IO.FileAttributes]::Hidden
        Write-Host "Desocultado: $($_.FullName)"
    }
}

After execution, you can re-enable synchronization, and NextCloud will recognize that everything is correct.

AchcarLucas · 2026-09-23T17:09:20Z

@AndyFx12 I found a workaround that works for me: #9885 (comment)

But I'd also appreciate the proper fix being merged. Happy to help out if there's a lack of dev capacity :)

Regarding issue #9885 and the permissions, I tried running Nextcloud as an administrator, and the problems went away.

morange · 2026-10-01T09:42:22Z

Independent confirmation that this fix addresses the root cause. Reproduced deterministically on Windows 11 25H2 (Nextcloud Desktop 34.0.4, build 20260916 — the current release, non-VFS) syncing a view-only shared folder, and the same failure applies to regular read-only shares, not only Talk downloads (#9885).

Symptom: when a new or changed file appears in the read-only share, the client downloads it to .<name>.~<suffix> but the final rename fails with WindowsError 5. The temp file is left behind and every retry repeats identically.

Root cause (matches this PR): setFileReadOnly() → setAclPermission(..., ReadOnly) applies a non-inherited deny ACE for S-1-5-32-545 (BUILTIN\Users) that denies DELETE, and it is applied to the temp file before the rename. MoveFileExW needs DELETE on the source, so it fails. icacls on a stuck temp file confirms this (anonymized):

BUILTIN\Users:(DENY)(D,DC,WD,AD,WEA)

Removing that single deny ACE lets the rename succeed immediately. Applying the read-only ACL to the final path after the rename, as this PR does in downloadFinished(), is the correct fix.

Client debug log (--logdebug, 34.0.4, paths/names anonymized) — the read-only ACL is applied to the temp file and the rename fails on the very next step:

filesystem.cpp:769          Detected access denied ACL: assuming read-only, path="…\<share>\.document.pdf.~XXXX"
propagatedownload.cpp:1220  downloadFinished: "…/<share>/.document.pdf.~XXXX" file is not locked: making it read only
filesystem.cpp:875          Renaming temp file to final failed: "WindowsError: 5: Zugriff verweigert"   (de locale = access denied)
propagatedownload.cpp:1275  Rename failed: …/.document.pdf.~XXXX => …/document.pdf
owncloudpropagator.cpp:935  Could not complete propagation of "<share>/document.pdf" … SoftError … "WindowsError: 5"

making it read only on line 1220 (operating on the temp file) is exactly the call this PR relocates to after the rename on line 875.

Reproduction:

Share a folder as view-only with another user.
Create or modify a file in it on the server.
Sync on the recipient's Windows client.
Without this PR: .<name>.~<suffix> remains, WindowsError 5, endless retry. With it: the file lands under its real name.

This looks like the same root cause behind #9488 (files never populating a read-only share on Windows), so it may be worth linking there as well.

As a side note on the "run as administrator" workaround mentioned above: it only works because an elevated token is not subject to the BUILTIN\Users deny ACE, so the rename succeeds despite the ACL. It doesn't address the root cause and isn't viable for standard (non-admin) workstations — this PR's reorder is the actual fix.

AndyFx12 · 2026-10-01T12:39:14Z

@Lenart12
Maybe you can help meanwhile? Seems mgalien is still waiting on some automatic-test code.

Lenart12 · 2026-10-01T13:21:43Z

Added automated tests, which reproduced the issue before the fix and then passed after the fix. More info in commit messages.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

AndyFx12 · 2026-10-02T09:28:11Z

@mgallien
Please take a look at this PR - many users are waiting for these changes ;-)
Thanks!

Lenart12 · 2026-10-08T10:17:08Z

Can someone take a look at this please :) It has been a week.

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 <koslenart@gmail.com>

…ud#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 <koslenart@gmail.com>

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: nextcloud#9885 Signed-off-by: Lenart Kos <koslenart@gmail.com>

…ud#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 <koslenart@gmail.com>

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 <koslenart@gmail.com>

Lenart12 · 2026-10-18T16:13:30Z

Rebased for latest master, still ready for review :)

Copilot AI review requested due to automatic review settings June 18, 2026 20:35

Lenart12 requested review from camilasan, i2h3, mgallien and nilsding as code owners June 18, 2026 20:35

Copilot started reviewing on behalf of Lenart12 June 18, 2026 20:35

View session

Lenart12 force-pushed the master branch from 8c20488 to b9243b0 Compare June 18, 2026 20:36

Copilot AI reviewed Jun 18, 2026

View reviewed changes

github-actions Bot added the feedback-requested label Jul 3, 2026

mgallien requested changes Jul 3, 2026

View reviewed changes

Lenart12 force-pushed the master branch from b9243b0 to db7e7e1 Compare October 1, 2026 13:19

Lenart12 requested a review from claucambra as a code owner October 1, 2026 13:19

Lenart12 requested a review from mgallien October 1, 2026 13:20

Lenart12 force-pushed the master branch from db7e7e1 to c4b045c Compare October 1, 2026 13:33

Lenart12 requested a balanced review from Copilot October 1, 2026 13:34

Copilot AI reviewed Oct 1, 2026

Lenart12 added 5 commits October 18, 2026 18:10

Lenart12 force-pushed the master branch from c4b045c to 85b2976 Compare October 18, 2026 16:12

Conversation

Lenart12 commented Jun 18, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Lenart12 Jun 18, 2026

Choose a reason for hiding this comment

Uh oh!

Lenart12 Jun 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lenart12 commented Jun 18, 2026

Uh oh!

Lenart12 commented Jun 18, 2026

Uh oh!

Lenart12 commented Jul 2, 2026

Uh oh!

github-actions Bot commented Jul 3, 2026

Uh oh!

mgallien commented Jul 3, 2026

Uh oh!

mgallien left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgallien commented Jul 16, 2026

Uh oh!

Lenart12 commented Jul 16, 2026

Uh oh!

AndyFx12 commented Aug 13, 2026

Uh oh!

mrtomatwo commented Sep 1, 2026

Uh oh!

AchcarLucas commented Sep 23, 2026

Uh oh!

AchcarLucas commented Sep 23, 2026

Temporary solution for those facing the problem

OBS: Run the commands below with synchronization paused.

Uh oh!

AchcarLucas commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morange commented Oct 1, 2026

Uh oh!

AndyFx12 commented Oct 1, 2026

Uh oh!

Lenart12 commented Oct 1, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

AndyFx12 commented Oct 2, 2026

Uh oh!

Lenart12 commented Oct 8, 2026

Uh oh!

Lenart12 commented Oct 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Lenart12 Jun 18, 2026 •

edited

Loading

mgallien left a comment •

edited

Loading

AchcarLucas commented Sep 23, 2026 •

edited

Loading