Skip to content

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

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

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

Conversation

@solracsf

@solracsf solracsf commented Sep 6, 2026

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.

The first three commits are #1269, including the federated display name fix from review there. Please ignore them here; this branch gets rebased once that lands.

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.

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.

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 (87 tests) on MariaDB 11.8, PostgreSQL 16, and SQLite with S3 as primary object store; the earlier 80-test state also on Oracle; litmus basic, copymove, props and locks, 94 of 94; psalm, cs-fixer and rector clean; the two-process race repeated; and every commit green on its own against MariaDB.

Since the CoreQueryBuilder removal, FileLock::import() expects database column
names, but the controller feeds it the object itself when a lock is refused and
the jsonSerialize() shape when an unlock is refused. Both 423 answers therefore
died with a TypeError and the client saw a 500 with no lock in it. The same
mismatch broke the remote lock read for federated shares, which passes its own
array.

Keep the lock as an object until the responder serializes it, and let import()
accept the shape jsonSerialize() produces as well as the database one.

Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
Spotted by @benjaminfrueh in review.

Making import() tolerant moved this value to the wrong field. The name
a federated lock carries comes from the remote as free text, so it
belongs in displayName, which is where the old 'owner' key put it. As
'userId' it left displayName empty, and the line below appends the host
to it, so the web UI showed a lock owned by plain "@remotehost".

It is also not a local user id. getOwner() feeds nc:lock-owner, which
the frontend compares against the current user to decide whether to
offer unlock, so a remote name that happened to match a local uid would
have offered it wrongly. Nothing sets userId for a remote lock now, as
before.

Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
The command had no test at all. This drives it through CommandAdapter,
the same wrapper the console registers it with, so the attribute-based
signature is exercised as the runtime sees it.

Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
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>
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>
…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>
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>
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>
A lock that never expires has a lifetime of -1 internally, and that value
went out raw on the `nc:lock-timeout` WebDAV property. Clients don't read
a negative lifetime as "no expiry", they add it to `lock-time` and end up
with an expiry date in the past: the Android client returns no expiry
only for exactly 0, so anything negative shows a healthy lock as already
expired.

0 is the value the clients implement for "this lock does not expire",
documented here since #175. Only the property changes; the internal
lifetime and `ILock::getTimeout()` stay as they are.

Same change as #1271, kept here so the branch is correct on its own
whichever of the two lands first.

Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
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
solracsf force-pushed the fix/lock-audit-findings branch from 73d61ed to 0e8e7d8 Compare September 7, 2026 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant