Read-only Deny ACE applied on file lock is never removed when the lock is released (Windows, VFS)
- Dominant language
- C++
- Stars
- 3.9k
- Forks
- 1k
- Avg merge
- 1d 21h
- Merged PRs (30d)
- 127
Description
### ⚠️ 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 have searched for 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
When a file is locked server-side (files_lock), the desktop client applies an explicit non-inherited BUILTIN\Users Deny ACE to the local file. When the lock is later released - by explicit unlock or by server-side timeout expiry - that ACE is never removed. The file remains permanently unwritable on the client, while the web UI shows no lock and the server has already dropped the lock row.
Clearing the ReadOnly attribute does not help: the attribute is not what blocks writes. Only removing the ACE restores write access.
The client's sync journal also retains lock=1 for these files indefinitely past lockTime + lockTimeout, and does not reconcile on discovery or across a full restart.
### Steps to reproduce
1. Server with the files_lock app enabled.
2. Windows client in Virtual Files mode, file already hydrated (content present locally).
3. Open a .md file in the web UI - the Text app takes an app lock (lockType=1, owner Text).
4. Do not touch the file on the client. Wait for the server-side lock to expire (eg have server set to lock_timeout = 5 minutes), and confirm server-side release (SELECT * FROM oc_files_lock — no row).
5. Try to write the file locally. It will report as locked locally.
### Expected behavior
Once the server reports the file unlocked, the client removes the read-only enforcement and the file becomes writable.
### Which files are affected by this bug
5 Files - listed below
### Operating system
Windows
### Which version of the operating system you are running.
Windows 11
### Installation method
Official Windows MSI
### Nextcloud Server version
34.0.0, via AIO
### Nextcloud Desktop Client version
33.0.7.20260629, VFS mode.
### Did this occur after an update or on a clean installation?
Clean desktop client installation
### Are you using the Nextcloud Server Encryption module?
No
### Are you using an external user-backend?
- [x] Default internal user-backend
- [ ] LDAP or Active Directory
- [ ] SSO - SAML
- [x] Other
### Nextcloud Server logs
None — and the absence is itself relevant.
nextcloud.log (level warning) contains zero entries referencing any of the affected files, across the entire window covering all five lock events. No LockedException, no lock-related warning, nothing. This is expected: the server behaved correctly throughout — it granted the locks, expired them on schedule, and dropped the rows. There is no server-side error to report because there is no server-side fault.
### Additional info
The file is permanently unwritable. icacls shows:
Deny BUILTIN\Users (non-inherited)
CreateFiles, AppendData, WriteExtendedAttributes,
DeleteSubdirectoriesAndFiles, Delete
A write probe fails with access denied:
[System.IO.File]::Open($path,'Open','Write','None')
Access to the path '...' is denied.
Clearing the ReadOnly attribute (attrib -R) does not restore write access — the ACE is still present and still blocking. Removing the ACE does:
icacls "path-to-file" /remove:d "BUILTIN\Users"
(Note Set-Acl fails here with PrivilegeNotHeldException: SeSecurityPrivilege, since it attempts to write the SACL section; icacls touches only the DACL.)
Journal state
Five files in .sync_.db → metadata retain lock=1 long past expiry. Server has no corresponding oc_file_locks rows.
path | lockType | lockOwnerEditor | lockTime (UTC) | lockTimeout | age at capture
(file1).md | 1 | text | 2026-07-24 20:20:44 | 300 | ~2 days
(file2).docx | 0 | (user) | 2026-07-25 02:45:22 | 300 | ~1.7 days
(file3).md | 1 | text | 2026-07-26 06:30:14 | 300 | ~12 h
(file4).md | 1 | text | 2026-07-26 06:37:24 | 300 | ~12 h
(file5).md | 1 | text | 2026-07-26 18:32:10 | 1200 | —
A full client shutdown and restart with fresh discovery left all five rows byte-identical — same lockToken, same lockTime, lock=1 throughout.
Why only some files carry the ACE
Some files unwritable; two were not. Both exceptions are explained by the code paths below, which makes the model self-consistent:
File 2 - lockType=0 (user lock) owned by the current user, so the guard in LockFileJob (_lockOwnerType == AppLock || _userId != account()->davUser()) excludes it. The ACE was never applied.
File 3 - received a content download after its lock was taken, and PropagateDownloadFile::updateMetadata() cleared the read-only state as a side effect.
File 5 - looks like a counter-example but confirms the ordering: it was downloaded at 18:32:05 and locked at 18:32:10 — the lock landed after the download, so nothing cleared it.
Analysis
There appears to be no code path that clears read-only for an already-hydrated file when its lock is released:
LockFileJob::setFileRecordLocked() / its caller (lockfilejobs.cpp) — the journal record is written unconditionally, but FileSystem::setFileReadOnly() is guarded by _lockStatus == LockedItem and only ever passes true. There is no setFileReadOnly(..., false) anywhere in this file.
discovery.cpp — the branch handling "server says unlocked, DB says locked":
} else if (serverEntry.locked == SyncFileItem::LockStatus::UnlockedItem && dbEntry._lockstate._locked) {
_discoveryData->_filesUnscheduleSync.append(path._original);
}
This only cancels the scheduled re-poll. It does not clear the ACL, does not force propagation, and does not rewrite the journal record — which is why lock=1 survives indefinitely.
PropagateDownloadFile::updateMetadata() and BulkPropagatorDownloadJob::updateMetadata() — these do have the clearing branch (setFileReadOnlyWeak), but they sit in download-completion paths and only run when content actually transfers.
syncengine.cpp — has a correct lock/unlock toggle including the clearing branch, but it is gated on item->_type == CSyncEnums::ItemTypeVirtualFile.
Suspected reason (4) does not fire for these files - not verified: in CfAPI mode ItemTypeVirtualFile is determined by FILE_ATTRIBUTE_SPARSE_FILE, not by the reparse point. A hydrated placeholder keeps its reparse point but loses the sparse flag, so it is classified ItemTypeFile and skips that block entirely. If correct, the one path that correctly clears read-only on unlock never runs for hydrated VFS files.
Note also that lock release by timeout expiry is not distinguished from explicit unlock — the client learns both the same way, by comparing server lock status at the next discovery pass, i.e. path (2).
Related
#9885 — same FileSystem::setFileReadOnly() Deny ACE mechanism, but reached via the missing-W-permission download/rename path rather than the locking API; does not cover lock lifetime.
#6998 — same "client doesn't reconcile after server unlock" symptom, but scoped to the Explorer overlay indicator rather than write access and journal state.
Contributor guide
Research direction
Start with LockFileJob in lockfilejobs.cpp, then trace the unlocked branch in discovery.cpp and the virtual-file lock toggle in syncengine.cpp. Compare these with the clearing paths in PropagateDownloadFile::updateMetadata() and BulkPropagatorDownloadJob::updateMetadata(), reproducing on Windows in VFS mode while inspecting the ACL and sync journal. Done means an expired or explicitly released lock removes the Deny ACE and clears the journal lock state.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- desktop, operating-systems
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 55/100