[Bug]: Windows `Error 5: Access is denied` on downloads in Talk.

### ⚠️ Before submitting, please verify the following: ⚠️

- [x] This is a **bug**, not a question or a configuration issue.
- [x] This issue is **not** already reported on Github (I've searched it).
- [x] Nextcloud Server and Desktop Client are **up to date**. See [Server Maintenance and Release Schedule](https://github.com/nextcloud/server/wiki/Maintenance-and-Release-Schedule) and [Desktop Releases](https://nextcloud.com/install/#install-clients) for supported versions.
- [x] I agree to follow Nextcloud's [Code of Conduct](https://nextcloud.com/contribute/code-of-conduct/)

### Bug description

# Windows `Error 5: Access is denied` on every download in a Talk / read-only share – client applies a `BUILTIN\Users Deny (Delete)` ACE to the `.~XXXX` temp file *before* renaming it

> **Related:** comments on [#6149](https://github.com/nextcloud/desktop/issues/6149). Filing a fresh issue with full evidence, repro and a verified workaround because the existing thread is stuck at "can't reproduce" / "no ACL info".

## TL;DR

The desktop client downloads a file from a remote whose permission string lacks `W` (Talk attachments, read-only shares, "Can view" shares) to `..~XXXX`. Before renaming that temp file to its final name, it calls `FileSystem::setFileReadOnly*()`, which on Windows writes an **explicit non-inherited `BUILTIN\Users Deny (CreateFiles, AppendData, WriteExtendedAttributes, DeleteSubdirectoriesAndFiles, Delete)` ACE** onto the *temp* file. The next call, `MoveFileExW(.~XXXX → final)`, requires `DELETE` on the source handle. Because the interactive user is a member of `BUILTIN\Users` and the Deny ACE is evaluated before the `Authenticated Users Allow Modify` ACE, the rename returns `ERROR_ACCESS_DENIED (5)`. The orphan temp file keeps its Deny ACL, so every subsequent sync runs into exactly the same error forever.

Fix: apply the read-only ACL to the **final** path *after* the rename, not to the temp path before it. A simple reordering of two calls in `PropagateDownloadFileQNAM::downloadFinished()` removes the bug.

---

## Environment

All host-, user- and file-identifying data in this report has been replaced by the placeholders shown in the table below.

| | |
|---|---|
| Client | **Nextcloud Desktop 33.0.2** (official Windows installer), Qt 6.10.2 |
| OS | Windows 11 Pro 24H2, build 10.0.26200, x64 |
| Install path | `C:\Program Files\Nextcloud\nextcloud.exe` |
| Hostname | `` (anonymized) |
| Interactive user | `\` – member of `BUILTIN\Users`, `BUILTIN\Administrators` (deny-only under UAC), `NT AUTHORITY\Authenticated Users` |
| Sync root | `` (a regular local NTFS directory, e.g. `C:\Users\\Nextcloud`) |
| Failing subfolder | `\\` – a folder that is a **Nextcloud Talk conversation share** (attachments sent into a group chat by other participants) |
| Server | Nextcloud server ≥ 28, standard deployment; remote items report permissions `"GDNVS"` (read/delete/rename/move/reshare, **no write**) |
| File system | NTFS, local SSD, no redirection, no DFS, no compression, no encryption, no junction/mountpoint |
| Virtual files | **Not** enabled for this folder – regular always-local sync |
| AV / Controlled Folder Access | Windows Defender; CFA off; verified not involved (see below) |
| End-to-end encryption | Not used on the failing folder |
| Placeholders used below | `` = Windows computer name, `` = Windows account name, `` = local sync directory, `` = name of the failing Talk/read-only folder, `` = actual filename of the n-th affected file, `` = Nextcloud `fileid`, `` = the 1-4 char hex id the client appends after `.~` |

Reproduced with a fresh install of 33.0.2 on two Windows 11 machines owned by the same account.

---

## Symptom

30 files in `\\` never finish syncing. For each of them the full `..~` temp is on disk at the correct size, but the final rename never happens. The client is locked in a retry loop. Counters below come from the attached diagnostic transcript, summed across 186 rotated client logs:

| Pattern | Hits |
|---|---:|
| `Rename failed: …\.~ => …` | **4 950** |
| `Detected access denied ACL: assuming read-only` | **4 950** |
| `WindowsError: 5: Access is denied.` | **24 750** |
| `inserted error item "/"` | **4 956** |

Representative line-for-line sequence for one affected file (names replaced with placeholders):

```
discovery.cpp Processing "/" | perm: "GDNVS" | fileid: ""
discovery.cpp discovered "/" CSYNC_INSTRUCTION_NEW Down
propagator Starting CSYNC_INSTRUCTION_NEW propagation of "/" by PropagateDownloadFile
filesystem.cpp:525 Detected access denied ACL: assuming read-only,
path="\\?\\\..~"
propagatedownload.cpp:1329 "Rename failed: //..~
=> //"
owncloudpropagator.cpp:280 Could not complete propagation of "/" …
status SoftError and error: "WindowsError: 5: Access is denied."
```

This repeats ~165 times per file over the life of the log archive. No HTTP/network/e2ee/hydration failure anywhere in the ~630 000 scanned log lines - the failure is local and deterministic.

---

## Why it is not my FS / AV / permissions

- **Parent folder is fine.** `\` itself has no non-inherited Deny, only the default `SYSTEM/Administrators/Users/Authenticated Users` Allow set.
- **Healthy siblings are fine.** Files in the *same* folder that were synced before the share started failing have **no** non-inherited Deny ACE; only the inherited Allow set. Example: a sibling JPEG of 1.7 MB reads `Deny-ACE here? False`, attributes `Archive`.
- **AV is not at play.** The file can be opened for read by `\`, `icacls … /deny` works, Defender does not log any quarantine or access block, Controlled Folder Access is off. Removing the Deny ACE (and nothing else) makes the rename succeed.
- **It's not the `FILE_ATTRIBUTE_READONLY` attribute.** Windows ignores `ReadOnly` on rename; the failure is purely due to the DACL.

## The DACL the client puts on the temp file

Dumped from every one of the 30 stuck temp files - identical on all of them (values anonymized):

```
--- ..~ ( bytes) attr=ReadOnly, Hidden, Archive
Owner: \
ACE BUILTIN\Users Deny CreateFiles, AppendData,
WriteExtendedAttributes,
DeleteSubdirectoriesAndFiles,
Delete isInherited=False
ACE NT AUTHORITY\Authenticated Users Allow Modify, Synchronize isInherited=False
ACE NT AUTHORITY\SYSTEM Allow FullControl isInherited=False
ACE BUILTIN\Administrators Allow FullControl isInherited=False
ACE BUILTIN\Users Allow ReadAndExecute, Synchronize isInherited=False
```

**All** ACEs are `isInherited=False`, i.e. the client is explicitly writing a full DACL onto each temp file, including the Deny. This is what `filesystem.cpp:525 "Detected access denied ACL: assuming read-only"` later keys off of.

## Why this causes `ERROR_ACCESS_DENIED (5)` on rename

Windows evaluates explicit Deny ACEs before explicit Allow. The interactive user is in `BUILTIN\Users`, so `Delete` is explicitly denied to him on the source. `MoveFileExW` renames in place by opening the source with `DELETE` and calling `NtSetInformationFile(FileRenameInformation)` - it needs `DELETE` on the source handle. The Deny is matched first, `CreateFile` with `DELETE` fails with `ERROR_ACCESS_DENIED`, and the whole operation returns 5. The client catches this as `SoftError`, records an `"inserted error item"`, and moves on; the orphan temp retains its bad DACL, so it is re-detected on every subsequent sync.

---

## Where in the code this happens

- `src/libsync/filesystem.cpp`
- `FileSystem::setFileReadOnly()` / `FileSystem::setFileReadOnlyWeak()` - these are the Windows callsites that apply the Deny ACE through `SetNamedSecurityInfoW`. `filesystem.cpp:525` is the read-back / detection side (`"Detected access denied ACL: assuming read-only"`).
- `src/libsync/propagatedownload.cpp`
- `PropagateDownloadFileQNAM::downloadFinished()` calls `FileSystem::setFileReadOnlyWeak()` **on the temp path** and then `FileSystem::rename(tmp, final)`. Swapping those two, or moving the ACL step to the final path, fixes the bug.

The underlying defect is a **TOCTOU-style ordering error**: the step that enforces "final file should be read-only" is being applied to an *intermediate* path that the same code must immediately rename. On POSIX this is harmless because a directory-writable user can unlink a file regardless of the file's mode; on NTFS it is not, because `MoveFileExW` needs `DELETE` on the source ACL.

## Suggested fix (any one of)

1. **Preferred.** In `PropagateDownloadFileQNAM::downloadFinished()`, perform `FileSystem::rename(tmp, final)` first, then `FileSystem::setFileReadOnly(final, true)`. If the rename fails, don't apply the ACL.
2. Alternative: never call `setFileReadOnly*()` on the `.~` path. Only apply the DACL to the **final** file, after every rename in the propagator has completed.
3. Band-aid: when computing the new DACL on Windows, leave `DELETE` granted to `CREATOR OWNER` / the interactive SID, so `MoveFileExW` can still succeed even if the ACL is applied too early. (Still leaves the Deny ACL on an orphan file after a crash, which is messy.)

Option 1 also fixes an existing side-effect: the Deny ACL is currently only ever removed by a successful rename, so a crash between the ACL write and the rename leaves a stuck file on disk that the user can't delete without elevation.

## Scripts

I wrote two self-contained PowerShell scripts to (a) prove the diagnosis on any machine and (b) break the sync loop. Attached: `Test-NextcloudError5.ps1`, `Fix-NextcloudError5.ps1`, and three double-click wrappers (`1-Diagnose.cmd`, `2-Fix-DryRun.cmd`, `3-Fix-Apply.cmd`). The diagnostic is read-only and safe to run on any live sync; the fix refuses to change anything unless you pass `-Apply` and are elevated.

Output of the diagnostic on my machine (anonymized):

```
==== 2. Temp files ("..~") in folder =====================
Temp files found: 30
...30 files, all with hasDenyACE=True

==== 3. Healthy reference file in the same folder ==========================
Healthy file : (1,754,351 bytes)
Deny-ACE here?: False <-- should be False

==== 5. Nextcloud client log scan ==========================================
Log files scanned : 186
Matches "Rename failed" : 4950
Matches "Detected ...ACL" : 4950
Matches "WindowsError: 5" : 24750
Matches "inserted error" : 4956

==== 6. Verdict ============================================================
Verdict: CONFIRMED
Reason : All 30 temp files carry an explicit 'BUILTIN\Users Deny' ACE;
this is exactly the pattern that causes MoveFileEx to fail with error 5.
```

Running the fix (elevated, `-Apply -StopClient`) processed all 30 files with `icacls /remove:d "BUILTIN\Users"` + rename to the final name, and the client now reports them as up-to-date:

```
Candidates seen : 30
fixed : 30
skipped (no deny ACE) : 0
failed : 0
```

The fix is idempotent and does **not** restore write access to the files - it only removes the Deny ACL that should never have been on the temp name to begin with. The resulting final files still carry the inherited read-only Allow set, which matches the `GDNVS` remote permission correctly.

---
# Attachments

All attachments are anonymized (host, user, sync paths, filenames, fileids, GUIDs stripped). GitHub's upload form refuses `.cmd` / `.ps1`, so every script file has been renamed with a trailing `.txt` — reviewers just remove the `.txt` suffix to run them.

**Evidence (the actual proof — please look here first):**

- `_anon\transcript.log` + `summary.json` + `acls.txt` — output of `Test-NextcloudError5.ps1` on my machine. Shows 30 temp files, all with the identical non-inherited `BUILTIN\Users Deny (…Delete)` ACE, plus the log-scan counters and final `Verdict: CONFIRMED`.
- `fix__anon\transcript.log` + `summary.json` — output of `Fix-NextcloudError5.ps1 -Apply -StopClient`. Shows the 30 temp files being un-stuck after the Deny ACE is removed with `icacls /remove:d "BUILTIN\Users"`, proving the ACL is the sole cause.

**Scripts (optional — only if reviewers want to reproduce on their own machine):**

- `Test-NextcloudError5.ps1.txt` — read-only diagnostic.
- `Fix-NextcloudError5.ps1.txt` — the workaround (refuses to change anything without `-Apply`; needs elevation).
- `1-Diagnose.cmd.txt`, `2-Fix-DryRun.cmd.txt`, `3-Fix-Apply.cmd.txt` — double-click wrappers. The last one self-elevates via UAC.

[README.md](https://github.com/user-attachments/files/26959051/README.md)

Happy to collect a full `nextcloud.exe` ETW trace or `procmon` capture showing the `CreateFile/DELETE → ACCESS_DENIED` on the temp handle if that helps reviewers.

[acls.txt](https://github.com/user-attachments/files/26958661/acls.txt)
[transcript.log](https://github.com/user-attachments/files/26958660/transcript.log)
[summary.json](https://github.com/user-attachments/files/26958674/summary.json)
[transcript.log](https://github.com/user-attachments/files/26958673/transcript.log)

[1-Diagnose.cmd.txt](https://github.com/user-attachments/files/26958972/1-Diagnose.cmd.txt)
[2-Fix-DryRun.cmd.txt](https://github.com/user-attachments/files/26958970/2-Fix-DryRun.cmd.txt)

[Fix-NextcloudError5.ps1.txt](https://github.com/user-attachments/files/26958974/Fix-NextcloudError5.ps1.txt)
[Test-NextcloudError5.ps1.txt](https://github.com/user-attachments/files/26958973/Test-NextcloudError5.ps1.txt)

### Steps to reproduce

## Minimal repro (no server required)

This reproduces the exact Win32 call sequence the client performs, without needing a Nextcloud instance at all. Save and run on any Windows 10/11 box; the repro user just has to be a normal (non-admin) `Users` member.

```powershell
$dir = "$env:TEMP\nc-error5-repro"
New-Item -ItemType Directory -Force -Path $dir | Out-Null
$tmp = Join-Path $dir '.sample.bin.~1234'
'hello' | Set-Content -LiteralPath $tmp

# Exactly what FileSystem::setFileReadOnly() ends up doing on Windows.
# (CA=CreateFiles, AD=AppendData, WEA=WriteExtendedAttributes,
# DC=DeleteSubdirectoriesAndFiles, DE=Delete)
icacls $tmp /deny 'BUILTIN\Users:(CA,AD,WEA,DC,DE)' | Out-Null
attrib +r +h $tmp

# What the propagator does next:
Rename-Item -LiteralPath $tmp -NewName 'sample.bin'
# => Rename-Item : Access to the path '...\.sample.bin.~1234' is denied.
```

Remove the Deny and the rename succeeds:

```powershell
icacls $tmp /remove:d 'BUILTIN\Users' | Out-Null
Rename-Item -LiteralPath $tmp -NewName 'sample.bin' # works
```

### Trigger with the real client

The trigger is any share whose server-side permission string lacks `W`. Discovery logs `perm: "GDNVS"` on every affected file:

| Letter | Meaning | Present |
|---|---|---|
| G | can Get (read) | ✔ |
| D | can Delete | ✔ |
| N | can reName | ✔ |
| V | can moVe | ✔ |
| S | can reShare | ✔ |
| **W** | can **Write** | **✗** |

Reproduction recipe:

1. On the server, create a new Talk conversation, or share a folder with another user with "Can view" (no edit).
2. As the *other* participant, upload an attachment to the conversation / drop a file into the shared folder.
3. On Windows with the desktop client syncing that account, observe the `..~` temp appear and stay, client Activity tab showing repeated `Windows error 5`.

### Expected behavior

Files are synced normally.

### Which files are affected by this bug

.~

### Operating system

Windows

### Which version of the operating system you are running.

Windows 11

### Package

Official Windows MSI

### Nextcloud Server version

Nextcloud Hub 26 Winter (33.0.2)

### Nextcloud Desktop Client version

Nextcloud Desktop 33.0.2

### Is this bug present after an update or on a fresh install?

Fresh desktop client install

### Are you using the Nextcloud Server Encryption module?

Encryption is Disabled

### Are you using an external user-backend?

- [x] Default internal user-backend
- [ ] LDAP/ Active Directory
- [ ] SSO - SAML
- [ ] Other

### Nextcloud Server logs

```shell

```

### Additional info

_No response_