Fix 5364

[guerricv]

Description

Fix the recurring permissions warning for storage/logs/teampass_background_tasks.lock reported in #5364 by correcting runtime-file writers, without weakening the integrity or permissions audit.

• Add shared, DB-free helpers for the background-task lock/trigger and the integrity scan/enqueue locks.
• Attempt POSIX permission repair before writing, using ($currentMode & 0640) | 0600: preserve 0600, ensure owner read/write, and do not grant group/other access or change the process-wide umask.
• If opening succeeds but only chmod fails, log an actionable warning and continue with the existing access. Invalid/replaced targets and actual open/write failures still fail. The permissions audit continues reporting unresolved unsafe modes.
• Reject symbolic links and non-regular targets. Compare descriptor/path type and dev/ino before and after permission repair. These checks detect observed replacements; they do not make path-based chmod atomic. Runtime directories must remain protected against untrusted writers.
• Open locks without truncation and update the handler PID only after acquiring its exclusive lock.
• Make signal writes non-blocking with LOCK_EX | LOCK_NB, distinguishing contention from I/O failure so a web request does not wait or emit a misleading directory-permission error.
• Probe scan status using an existing read-only handle, without creating files or changing permissions.
• Complete the GPL header and update regression tests and runtime-permission documentation.

Runtime data remains excluded from release checksum comparisons but included in the permissions audit. No exclusion rule or app/files_reference.txt change is included.

The task-log writer and the existing unlink-after-unlock lifecycle race are intentionally left for separate follow-ups, as requested in review.

Related issue

Fixes #5364

Type of change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (existing behaviour changes)
• Documentation
• Translation
• Refactor / maintenance

How has this been tested?

Review follow-up commit: de5a9e5e28cfd74f4231b1935bbcb3b5b9423e3c. Validation checked on 2026-09-10.

Linux CI:

• PHP 8.2.33: OK — 1,732 tests, 59,651 assertions.
• PHP 8.3.33: OK — 1,732 tests, 59,651 assertions.
• No skipped tests, failures, errors or warnings in either PHPUnit result.
• PHPStan level 4 and quality guards: passed.
• CodeQL and Scrutinizer: passed.

The regression coverage includes creation/recreation under five umasks, preservation of 0600 and active contents/inodes, continued writing plus an audit warning after a simulated chmod denial, observed target replacement, a bounded subprocess test for contending signal writers, read-only status probing, invalid targets/symlinks, and continued detection of unsafe runtime permissions. Chmod denial is simulated in an isolated PHP subprocess; the tests require neither sudo nor account changes.

Local Windows validation:

• Syntax checks on PHP 8.2.33 and 8.3.33: passed.
• Targeted tests on both versions: 45 tests, 463 assertions, no failures/errors/warnings, 19 platform-dependent skips.
• Full configured PHPStan level 4: passed. The existing PHPStan PHAR and project configuration were used with a temporary copy of settings.sample.php, removed afterwards.
• Full suite on both versions: 1,732 tests, 59,424 assertions, nine failures, one warning and 19 skips. The Windows platform-sensitive suite limitations remain; the fresh Linux CI runs above pass completely.
• Table-prefix guard and git diff --check: passed.
• Release manifest, Composer production metadata and dependencies unchanged.

No live-instance UI/database/user-role tests were run locally. Roles and personal-folder behaviour are unchanged. Suggested lab validation: trigger background processing and an integrity scan under the normal web/cron account; verify lock/signal modes, task progress, and continued reporting of an unrelated unsafe runtime file.

Checklist

• PHPStan level 4 passes (php app/vendor/bin/phpstan analyse --memory-limit=2G)
• The test suite passes (php _tools/phpunit.phar) — Linux CI on PHP 8.2 and 8.3; Windows limitations documented above
• Every new public function has a docblock
• Variable names and comments are in English
• No var_dump() or console.log() left in the code
• New app/sources/*.queries.php files have a matching public/sources/ proxy shim — not applicable
• Changes to teampassclasses were applied to both copies — not applicable
• app/vendor/composer/ is in its production form — left untouched

Impact on install / upgrade

• No schema change
• Schema change — an install/upgrade_run_X.X.X.php script is included and the fresh install path was tested

No install/upgrade or configuration migration is required. Existing accessible locks/signals are repaired when possible. Incorrect ownership or inaccessible directories still require administrator remediation; the helper does not change ownership.

Screenshots

Not applicable: no UI change.

[nilsteampassnet]

Thanks @guerricv, the diagnosis is spot on and the fix attacks the right side of the problem: the writers, not the scanner. No exclusion rule, the audit keeps watching those files, and testPermissionScanStillReportsUnsafeRuntimeFiles proves it. The extra concurrency fix that comes with it (opening without truncation and moving ftruncate() after the flock(), so a competing handler can no longer erase the running owner's PID) is a genuine bonus.

The test coverage is a real strength, five umasks, repair of a permissive mode without changing the inode or truncating, 0600 preserved, lock contention, symlinks, invalid targets, and a real end-to-end scan.

Two things I'd like changed before merge, then a few low-cost hardenings.

1. Missing GPL file header

app/sources/runtime_files.functions.php is the only file in app/sources/ without the project header block. I checked every file in the directory, all the others carry it, including the recently added file_scope.functions.php and item_restriction_logic.php. Please add the standard Teampass - a collaborative passwords manager. --- … @license GPL-3.0 block.

2. A failed chmod stops background processing entirely

tpOpenRuntimeFile() returns false when chmod() fails. In acquireProcessLock() that propagates to processBackgroundTasks() (background_tasks___handler.php:87), which returns immediately, no task is processed at all, with only an error_log() line as evidence.

chmod() requires ownership (or root), so this triggers whenever the lock file already exists with a different mode and belongs to another account.
The realistic case: an administrator runs php app/scripts/background_tasks___handler.php manually as root while troubleshooting (a documented step), the process is killed before releaseProcessLock(), and a 0644 root:www-data lock is left behind.
From then on www-data can never open it and background tasks are silently dead. Before this PR, fopen($lockFile, 'w') on a 0664 lock still succeeded and everything kept working with a cosmetic warning.

Suggestion: when fopen() succeeded but only chmod() failed, log and keep the handle instead of returning false. The permission warning reappears (the pre-existing, harmless state) rather than turning a cosmetic finding into an outage. I note tpFileIntegrityWriteJsonFile() (file_integrity.functions.php:759) already throws on the same failure, so the PR is consistent with existing code; it's the consequence that differs (one report not written vs. the whole task pipeline stopped).

Low-cost hardenings

3. $currentMode & 0640 can drop the owner write bit. 0444 & 0640 = 0440. Practically unreachable (the c+b open would fail first) except as root, but ($currentMode & 0640) | 0600 is strictly safer at no cost.
4. TOCTOU between is_link() and chmod(). chmod() acts on the path, not the descriptor, and follows symlinks, a link swapped in between line 20 and line 44 would get an arbitrary target chmod'ed. Low risk since storage/logs must not be writable by a third party (the audit reports world_writable as an error), but you already call fstat($handle) on line36: comparing its dev/ino against an lstat($path) closes the window for free.
5. Blocking flock(LOCK_EX) in a web thread. tpWriteRuntimeFile() line 68 runs inside triggerBackgroundHandler(), on item write paths. The consumer (checkAndConsumeTrigger()) unlink()s without any lock, so the exclusive lock buys no atomicity against it, LOCK_EX | LOCK_NB would remove any chance of stalling a request without losing anything.
6. tpFileIntegrityIsRunning() uses r+b. flock() works fine on a read-only handle; rb would suffice and would let the probe work even when the lock belongs to another account. Not a regression (the previous c+ already required write), just a nit.

Adjacent, out of scope but worth a follow-up

7. The same defect survives on teampass_tasks.log: taskLogger.php:55 creates it with file_put_contents() → 0644 under umask 0022, under storage/logs, which is classified runtime and is not in tpFilePermissionsShallowRuntimeRoots(), so it is scanned in depth. Invisible for the reporter (enable_tasks_log = '0'), but the identical warning will come back for anyone who enables task logging.
8. releaseProcessLock() unlink()s a flocked file. Handler B can hold the lock on the orphaned inode while handler C creates a fresh one and locks that, two handlers running concurrently. Pre-existing design issue, not introduced here; your ftruncate-after-flock change improves the PID handling but doesn't close it. Separate ticket.

One process note

The branch is 3 commits behind develop (62c8a0d, b18e667, e85a252 — the API item-level restrictions). The three-way merge applies cleanly and everything is green, but a rebase on develop before merge would make the PR's own CI reflect the real state. As it stands, taking main.functions.php wholesale from the branch reverts those commits and breaks SecurityPostureAuthorizationTest.

Thanks for the thorough test suite.

[guerricv]

Thanks Nils — points 1–6 are addressed in de5a9e5e2.

1. Completed the standard GPL header and separated it from the function documentation.
2. A chmod-only failure now logs a warning and keeps an otherwise valid, writable handle. Actual open/write failures and invalid or replaced targets still fail; the permission audit is unchanged and still reports unsafe modes.
3. The target mode is now ($currentMode & 0640) | 0600, preserving 0600 while ensuring owner read/write.
4. Added regular-file and descriptor/path dev/ino checks before and after permission repair. These detect observed replacements, but path-based chmod is not atomic: the requirement for protected runtime directories remains explicit.
5. Signal writes now use LOCK_EX | LOCK_NB. Contention is distinguished from I/O failure, without truncation or a misleading permission-error message; the web request does not wait.
6. The scan-status probe now uses rb, validates the target, and neither creates the lock nor changes its permissions.

Added regression tests for chmod denial after successful opening, replacement during permission repair, path identity, non-blocking contention (with a bounded subprocess timeout), and probing a read-only lock. The chmod-denial fixture is isolated in a subprocess and requires no sudo/account changes. Updated the documentation and PR description to reflect the best-effort permission policy.

The fresh Linux CI passes on PHP 8.2.33 and 8.3.33: 1,732 tests and 59,651 assertions on each, without skipped tests. PHPStan level 4, CodeQL, the quality guards and Scrutinizer are also green. Local Windows validation and its platform limitations are recorded in the PR description.

The task-log writer/path handling and the persistent-lock lifecycle remain separate follow-ups. No manifest, exclusion-rule, Composer metadata or dependency changes were made.

[guerricv]

Separate follow-up on branch alignment: I fetched upstream develop again and checked both Git and GitHub's comparison.

At this check, upstream develop is da1f0524216a17d88fd403ef5516d198ad4cfb07 and this PR is de5a9e5e28cfd74f4231b1935bbcb3b5b9423e3c. The comparison reports 0 commits behind / 4 ahead, with a clean merge, and the CI for the new head is green. No rebase or history rewrite was needed against that published develop tip.

I could not resolve 62c8a0dc1, b18e66729 or e85a25245 through the upstream repository API. Could you confirm which ref contains those item-restriction commits? I will recheck alignment if the intended develop tip differs.

The new main.functions.php change is limited to handling runtime-trigger contention; the other application changes were preserved.