Skip to content

fix: make the locking model consistent across every access path - #1270

Open
solracsf wants to merge 8 commits into
mainfrom
fix/lock-audit-findings
Open

fix: make the locking model consistent across every access path#1270
solracsf wants to merge 8 commits into
mainfrom
fix/lock-audit-findings

Conversation

@solracsf

@solracsf solracsf commented Sep 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Both a Fix and Feature

Feel free to review each commit.

The app decides who may lock, write and release a lock separately in each entry point, and the answers disagree: the same user gets a different result through OCS, X-User-Lock, native WebDAV and the PHP API. This builds one persistence model and one policy, then moves every path onto it. Best reviewed commit by commit.

#1269 has landed, and this branch is rebased onto main, so the two regression fixes and the federated display name fix that used to sit at the bottom of it are no longer part of its diff.

feat(db) one lock per file and one absolute expiry. There was no uniqueness on file_id and acquisition read before it wrote, so two requests could both take a lock on the same file (reproduced with two processes, ten rounds, every round). Expiry existed twice over, as a ttl counted from creation for the ETA and as a creation-age query for the cleanup, so the job deleted locks that clients had refreshed and still believed they held. Adds a unique index, with a migration that reconciles the duplicates an existing installation may already hold before adding it, insert-first acquisition, and expires_at as the single expiry.

feat(policy) one authorization decision. The file-owner override was keyed on a hard-coded list of mount provider classes, so any provider not on that list granted the override to whoever happened to be looking. A token lock could only be released by presenting the token, which the server-side API cannot do, so an app could not clear its own user's stale client lock. And possession of the token was the whole credential, although the token is published to everyone who can read the file. Now a user lock belongs to its user, an app lock to the app's lock scope, and a token lock to the token together with the principal it was issued for.

feat(storage) enforce by file identity. The wrapper rebuilt a path inside the user's home folder, so it silently enforced nothing on group and team folders, external storages, or any custom mount. It now resolves the file through the wrapped storage's own cache, refuses a folder delete or move that would take a locked file with it, and runs ahead of the trash bin so a refused delete cannot have moved the file already.

feat(dav) rebuild the native adapter. Lock creation was a create-then-update, so a rejected second write left behind a row owned by a random token. The client's <D:owner> text was stored and shown to everyone as the lock owner, a missing Timeout header produced a lock that never expires, Sabre's token check authorised writes on possession alone, the requester's own user lock was hidden from the protocol, and errors came back as 500. Also reports a lock that never expires as nc:lock-timeout 0 rather than a negative lifetime, which clients read as an expiry in the past.

fix(api) OCS and CLI contracts. Status codes for ordinary client mistakes, lockType honoured on unlock, forced unlock by file id so it no longer needs the lock owner to still have access, and the README brought in line.

test collaborative multi-client churn. The suite only ever exercised one actor at a time, so nothing covered what a real deployment does to a lock: several people editing, renaming, moving and deleting the same tree from the browser, the sync client and the mobile apps at once. Six cases added. Through a real Sabre server: a manually locked file keeps its lock when its holder renames it and is still refused to the other user at the new path; only the client holding a token lock moves the file, so the same user's own browser is refused until that client releases it; a client that never sends UNLOCK stops holding the file for the rest of the team once the configured timeout passes; and directory listings follow locks other clients take and release, which is the only coverage LockPlugin::cacheDirectory() and LockService::getLockForNodeIds() have. At the storage level: a move to another storage strands no lock row, and a second user cannot move a held file out onto a mount of their own.

refactor remove what the rewrite left behind, and fix what a review found. Rebuilding acquisition and authorization orphaned a set of members nothing reaches any more: canUnlock(), update() and generateToken() on LockService, the empty enableUserOverride() stub, the getDeprecatedLocks() alias, the no-op prefetchRemoteLocks(), unused parameters on canLock() and fromLockScope(), and an IEventDispatcher the service has been taking and dropping since before this branch. getLockForNodeId() returns ?FileLock now rather than FileLock|false, which is what forced all eight DAV property handlers to open with the same check for false.

Reading the result through then turned up a handful of defects in the new code, fixed in the same commit: the token-collision retry in acquire() did not cover the retry itself, so a competing insert in that window escaped as an unhandled exception on every path but OCS; acquire() issued a delete before the select that says whether one is needed; canLock() re-implemented canModify()'s permission check without the guard that keeps a throwing storage from becoming a fatal; getLockForNodeIds() omitted rather than reported files with no lock; rmdir() ran the locks-below join twice; and the source-lock check duplicated between copyFromStorage() and moveFromStorage() is one method now.

test two gaps from the audit round. The retry that takes a fresh token when the insert is refused on the token index, and the scope written on insert together with the coercion that reads a legacy 0 back as exclusive. Both fixes had shipped without a regression test; both new tests were run against the pre-fix code and fail there. Also resyncs TEST_FILES, which had drifted apart from the fixtures the class creates, so leftovers no longer survive into the next run.

Two things worth an opinion:

  • Releasing a token lock now also needs permission to write the file. The token stays readable through PROPFIND, which RFC 4918 6.5 allows and the desktop client relies on; what changed is that possession alone is no longer enough, as 6.4 asks. Before this, a read-only share recipient could read the token and drop a colleague's lock.
  • A collection LOCK still protects only the collection itself, not its members. That is unchanged and now documented; the litmus patch in this repo already disables the member assertions.

Verified: the suite, now 93 tests, on MariaDB 11.8, PostgreSQL 16, and SQLite with S3 as primary object store; the two-process race repeated; cs-fixer clean on PHP 8.3; every commit lints clean on its own. The 80-test state was also run on Oracle, and litmus basic, copymove, props and locks passed 94 of 94, both before the refactor commit; psalm and rector were clean at that point too.

@solracsf solracsf self-assigned this Sep 6, 2026
@solracsf
solracsf force-pushed the fix/lock-audit-findings branch from 73d61ed to 0e8e7d8 Compare September 7, 2026 20:14
@solracsf
feat(db): one lock per file and one absolute expiry
100c6c3 
The lock table had no uniqueness on file_id and acquisition read before it
wrote, so two requests could both take a lock on the same file. Expiry existed
twice over, as a ttl counted from creation for the ETA and as a creation-age
query for the cleanup, and the two disagreed the moment a lock was refreshed:
the job removed a lock a client believed it still held.

- a unique index on file_id, with a migration that reconciles the duplicates an
  existing installation may already hold before it adds the constraint, and a
  second step for the index itself because some databases refuse two indexes on
  one column list in a single change
- acquisition inserts and reads the winner back when the database rejects it, so
  the conflict comes from the constraint rather than from a prior select; a
  rejection on the token index is retried with a fresh token instead of being
  reported as a conflict on the file
- expires_at is the only expiry: it drives the ETA, the refresh, the cleanup
  query and the validity check
- the cleanup deletes only rows that are still expired when the delete runs, so
  a lock refreshed after the batch was read is no longer dropped

Authorization is untouched here and moves to the policy in the next commit.

Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
@solracsf
feat(policy): one authorization decision for every access path
afc45c0 
Who may release a lock was decided again in every caller, and differently: the
file-owner override was keyed on a hard-coded list of mount provider classes, so
any provider not on it granted the override to whoever happened to be looking;
a token lock could only be released by presenting the token, which the server-
side API cannot do, so an app could not clear its own user's stale client lock;
and possession of the token was the whole credential even though the token is
published to everyone who can read the file.

LockPolicy now answers three questions for every lock type: who holds a lock,
who may write the file, and who may release it. The service asks it instead of
deciding for itself.

- a user lock belongs to its user, an app lock to the app's lock scope, a token
  lock to the token together with the principal it was issued for
- the file-owner override applies to files on a user's own home storage, stated
  positively rather than as everything-except-these-classes
- the user a lock is recorded for can always release it, on any path, so the
  override flag the callers had to remember is gone
- releasing a token lock also needs permission to write the file: the token is
  publicly readable (RFC 4918 section 6.5), so section 6.4 asks for the normal
  permission mechanism rather than the obscurity of the token
- a forced release removes the lock by file id, without resolving the file
  through the owner, who may well have lost access to it by then

Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
@solracsf
feat(storage): enforce locks by file identity, including ancestors an…
fb96f00 
…d deletion

The storage wrapper resolved the file by rebuilding a path inside the user's
home folder, so it silently enforced nothing on any storage whose internal paths
do not start with files/: group and team folders, external storages, every
custom mount. It also asked its own question about who may write, which is the
policy's job, and it never looked at what a folder contains, so deleting or
moving a parent took a locked file with it.

- the wrapper resolves the file through the wrapped storage's own cache and asks
  the policy whether the caller may write it, which makes it work the same on
  every mount and for every lock type
- deleting or moving a directory is refused while it holds a file locked by
  someone else, found with one query against the file cache rather than by
  walking the tree
- the wrapper runs as the outermost one, ahead of the trash bin, so a refused
  delete cannot have moved the file to the trash first
- a lock is dropped when its file goes away, both from the storage that removed
  it and from the node and cache events, so a restored file comes back unlocked

Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
@solracsf
feat(dav): rebuild the native WebDAV adapter on the shared policy
c2270f0 
The lock backend created a lock and then updated it with the identity, the
display name and the timeout, so a rejected second write left a row owned by a
random token behind, the client's own <D:owner> text was stored and shown to
everyone as the lock owner, and a missing Timeout header produced a lock that
never expires. Sabre's token check was left as it is, which authorizes a write
on possession of the token alone, and the requester's own user lock was hidden
from the protocol so its holder could not use it. Errors came back as 500.

- the backend builds the whole lock and stores it once, so a refused request
  leaves nothing behind; the display name comes from the user, and the timeout
  from the request or from the configured default
- the plugin answers Sabre's token validation from the policy, so a token
  authorizes a write only for the principal the lock belongs to, and a
  collection operation is checked against the locks below it
- a user lock is visible to its own holder again, and the responses carry a
  valid timeout and 423, 409 or 403 instead of 500
- only native WebDAV may lock a collection, as RFC 4918 asks; every other path
  refuses a folder
- a listing of a DAV backed mount warms the remote properties once instead of
  asking the remote server about every file

Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
@solracsf
fix(api): align the OCS and CLI contracts with what the app now does
dd65a54 
Both front ends still answered ordinary client mistakes with 500, ignored the
lock type when releasing a lock, and reported failures by dumping an exception.
The OCS conflict payload also carried the token of the lock it was reporting,
which the caller has no use for because OCS never accepts one.

- OCS validates the lock type and the file id, and answers 400, 403, 404, 412
  or 423 instead of 500; the conflict payload no longer carries the token
- the lock type given to an unlock is honoured, and the user a lock was recorded
  for can release it whatever the type
- occ reports an already locked file, a folder, a missing file or an unknown
  user in one line and exits non-zero, and its forced unlock removes the lock by
  file id so it no longer depends on the lock owner still having access
- the README describes the behaviour that is now implemented: what a token lock
  requires, how expiry is refreshed, which storages are covered, what the
  status codes mean, and that only native WebDAV may lock a collection

Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
@solracsf
test: cover locking under collaborative multi-client churn
aec2472 
The suite tested one actor at a time, so nothing covered what a real
deployment does to a lock: several people editing, renaming, moving and
deleting the same tree from the browser, the sync client and the mobile
apps at once. A lock has to be placed and released exactly where it is
needed, and everything else has to stay out of the way.

Native WebDAV, through a real Sabre server:

- a manually locked file keeps its lock when its holder renames it, is
  reported at the new path and still refuses the other user there
- only the client holding a token lock moves the file: the same user's
  browser is refused, another user is refused even with the published
  token, and everyone is free again the moment UNLOCK arrives
- a client that never sends UNLOCK stops holding the file once the
  configured timeout passes, without waiting for the cleanup job
- directory listings follow locks other clients take and release, which
  is where a stale hit in the bulk PROPFIND cache would show up as a
  phantom lock

Storage level:

- moving a file to another storage copies it under a new id and deletes
  the source, so the lock must neither outlive the old id nor follow
  onto a file nobody locked
- a second user cannot move a held file out onto a mount of their own

Green on MariaDB 11.8, PostgreSQL 16 and S3 primary storage.

Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
@solracsf
refactor: remove what the rewrite left behind and fix what a review f…
106c90d 
…ound

Rebuilding acquisition and authorization left a set of members that
nothing reaches any more, plus a few shapes that only existed to serve
the old code.

- canUnlock(), update() and generateToken() on LockService lost their
  last callers when unlock() started asking the policy directly and
  acquire() started building the whole lock itself. enableUserOverride()
  had already been reduced to an empty stub, so anyone still calling it
  was quietly getting nothing rather than the override they asked for.

- prefetchRemoteLocks() never performed a listing. It resolved the
  storage, checked whether it was a DAV one, and returned either way, so
  nothing warmed the remote properties. The per-directory batching that
  does work is getLockForNodeIds(), which reads the local locks of a
  whole listing in one query. Warming the remote properties is still
  worth doing, but it needs a remote to measure against and belongs in
  its own change.

- getLockForNodeId() returns ?FileLock rather than FileLock|false, which
  is what forced all eight DAV property handlers to open with the same
  check for false.

- gone as well: the getDeprecatedLocks() alias, the unused $current of
  canLock(), the timeout argument of fromLockScope() that every caller
  passed as 0, the expiresAt branches of import() that its one caller
  never passes, and an IEventDispatcher that LockService has been taking
  and dropping on the floor since before this branch.

A read through the result then turned up a handful of defects in the new
code, fixed here rather than left for a follow-up:

- acquire() retried a token collision without covering the retry itself,
  so a competing insert landing in that window escaped as an unhandled
  LockConflictException; only the OCS path caught it by accident. The
  retry now reads the winner back and reports it as the conflict it is.
- acquire() deleted an expired row before the select that says whether
  there is one, a wasted round trip on the common case of a file nobody
  has locked. It selects first and deletes only what it found, keeping
  the delete's own expiry condition so a refresh in between is still not
  dropped.
- canLock() had its own copy of the permission check without the guard
  canModify() wraps it in, so a storage that throws while reporting
  permissions became a fatal on the DAV paths instead of a refusal.
- getLockForNodeIds() left a file with no lock out of its result rather
  than reporting it as false, against what the signature promises.
- rmdir() resolved the folder and ran the locks-below join twice.
  checkDescendants() returns what it read and rmdir() uses it, which also
  retires getBlockingLocksBelow().
- the source-lock check that copyFromStorage() and moveFromStorage() each
  carried verbatim is one method now, and the lock types both access
  paths accept come from one constant instead of two identical private
  ones.

Folders staying unlockable through ILockManager is deliberate, but the
README only listed the other four paths, so it says so now.

Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
@solracsf
test: pin the token collision retry and the stored lock scope
47b3821 
Two fixes from the audit round shipped without a regression test.

The token carries a unique index of its own, so an insert can be refused
over a token that another file's lock already holds. That is not a
conflict on this file: acquisition takes a fresh token and completes.
Before the fix it re-read the file's own row, found nothing, and came
back with LockNotFoundException.

The scope is written on insert, and rows that predate that hold 0, which
is not a valid scope and has to read back as exclusive. The first
assertion reads the column directly on purpose, because reading it back
through the model would apply the very coercion the second assertion
covers.

TEST_FILES had drifted apart from the tests it cleans up after as well:
seven files they create were missing from it and two entries named files
that no longer exist. One run stays green because each of those seven is
created by exactly one test, but the leftovers survive into the next run
in the same container, which is what the list is for.

Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
@solracsf
solracsf force-pushed the fix/lock-audit-findings branch from 9fa07af to 47b3821 Compare September 9, 2026 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@juliusknorr juliusknorr Awaiting requested review from juliusknorr juliusknorr is a code owner
@benjaminfrueh benjaminfrueh Awaiting requested review from benjaminfrueh benjaminfrueh is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@solracsf solracsf

Labels

3. to review enhancement New feature or request feature: locking

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@solracsf