SEC2-046: A sync connection's lease is released unconditionally, so a run that outlived it frees the next run's lease (as fixed in #576) #988

New issue

Closed

opened 2026-10-17 10:39:25 +00:00 by tiagoagueda · 0 comments

tiagoagueda commented

2026-10-17 10:39:25 +00:00

Owner

Found on 2026-10-16 in a read-only security review of postulo at 56fc9d531, checked by a second, independent pass.

Severity: Informational · Category: Business Logic · Reference: CWE-367 (Time-of-check Time-of-use Race Condition); OWASP A04:2021 (Insecure Design)

Location:

src/postulo/plugins/syncing.py:43 (LEASE_SECONDS = 15 * 60)
src/postulo/plugins/syncing.py:97-107
src/postulo/plugins/views.py:580 (Sync now calls sync_connection directly)
src/postulo/core/scheduler.py:102-108 (the fixed pattern, for comparison)

Issue

key = lease_key(connection)
if not cache.add(key, timezone.now().isoformat(), LEASE_SECONDS):
    return SyncReport(..., already_running=True)
try:
    with preferences.as_person(connection.owner):
        return _run_connection(connection)
finally:
    cache.delete(key)

The lease is a cache key that expires after fifteen minutes, and the finally deletes the key whatever it holds. A sync that takes longer than that (a large DAV address book over a slow server, the case #221 describes) loses its lease while still running; a second run takes a fresh lease and starts; when the first finishes it deletes the second's lease, and a third run can start beside the second. core/scheduler.py:102-108 fixed this for the pass lease by storing a token and deleting only a key that still holds it (#576); the connection lease was not brought along. The scheduler's path is additionally guarded by claim_connection (a conditional UPDATE on synced_at, syncing.py:142-157), but Sync now calls sync_connection without claiming, so between a person's Sync now and the scheduler the lease is the only thing that serialises the two.

Impact

Two syncs of one connection run at once, which #586 describes as the DAV plugin duplicating contacts. It needs a sync longer than the lease, so it is a correctness and data-duplication problem rather than an exposure.

Fix

Mirror scheduler.only_one_pass:

token = uuid.uuid4().hex
if not cache.add(key, token, LEASE_SECONDS):
    return SyncReport(..., already_running=True)
try:
    ...
finally:
    if cache.get(key) == token:
        cache.delete(key)

The existing test at tests/test_syncs.py:266 keeps passing. Routing Sync now through claim_connection as well is optional and adds little, since claim_connection guards scheduler against scheduler by the synced_at stamp. Done when: a test in tests/test_syncs.py holds the lease under another token and asserts sync_connection leaves it in place after finishing.

#586 (closed) - "nothing stops one connection running twice at once": its fix added the lease and claim_connection, and the unconditional delete is the residue. #576 (closed) - the same weakness in the scheduler's pass lease, fixed with a token compare.

Found on 2026-10-16 in a read-only security review of `postulo` at `56fc9d531`, checked by a second, independent pass. **Severity:** Informational · **Category:** Business Logic · **Reference:** CWE-367 (Time-of-check Time-of-use Race Condition); OWASP A04:2021 (Insecure Design) **Location:** - `src/postulo/plugins/syncing.py:43` (`LEASE_SECONDS = 15 * 60`) - `src/postulo/plugins/syncing.py:97-107` - `src/postulo/plugins/views.py:580` (*Sync now* calls `sync_connection` directly) - `src/postulo/core/scheduler.py:102-108` (the fixed pattern, for comparison) ## Issue ```python key = lease_key(connection) if not cache.add(key, timezone.now().isoformat(), LEASE_SECONDS): return SyncReport(..., already_running=True) try: with preferences.as_person(connection.owner): return _run_connection(connection) finally: cache.delete(key) ``` The lease is a cache key that expires after fifteen minutes, and the `finally` deletes the key whatever it holds. A sync that takes longer than that (a large DAV address book over a slow server, the case #221 describes) loses its lease while still running; a second run takes a fresh lease and starts; when the first finishes it deletes the *second's* lease, and a third run can start beside the second. `core/scheduler.py:102-108` fixed this for the pass lease by storing a token and deleting only a key that still holds it (#576); the connection lease was not brought along. The scheduler's path is additionally guarded by `claim_connection` (a conditional `UPDATE` on `synced_at`, `syncing.py:142-157`), but *Sync now* calls `sync_connection` without claiming, so between a person's *Sync now* and the scheduler the lease is the only thing that serialises the two. ## Impact Two syncs of one connection run at once, which #586 describes as the DAV plugin duplicating contacts. It needs a sync longer than the lease, so it is a correctness and data-duplication problem rather than an exposure. ## Fix Mirror `scheduler.only_one_pass`: ```python token = uuid.uuid4().hex if not cache.add(key, token, LEASE_SECONDS): return SyncReport(..., already_running=True) try: ... finally: if cache.get(key) == token: cache.delete(key) ``` The existing test at `tests/test_syncs.py:266` keeps passing. Routing *Sync now* through `claim_connection` as well is optional and adds little, since `claim_connection` guards scheduler against scheduler by the `synced_at` stamp. Done when: a test in `tests/test_syncs.py` holds the lease under another token and asserts `sync_connection` leaves it in place after finishing. ## Related #586 (closed) - "nothing stops one connection running twice at once": its fix added the lease and `claim_connection`, and the unconditional delete is the residue. #576 (closed) - the same weakness in the scheduler's pass lease, fixed with a token compare.

tiagoagueda added the

enhancement

security

labels

2026-10-17 10:39:25 +00:00

tiagoagueda added this to the 1.0.0 milestone

2026-10-17 10:39:26 +00:00

tiagoagueda referenced this issue

2026-10-17 10:41:23 +00:00

SEC2 index: security review of 2026-10-16 (86 issues across the core, plugins and extensions) #992

tiagoagueda referenced this issue from a commit

2026-10-17 15:36:13 +00:00