Skip to content

fix(api): write session config with owner-only permissions#949

Closed
bluetoothbot wants to merge 3 commits into
uilibs:mainfrom
bluetoothbot:koan/fix-issue-825v2
Closed

fix(api): write session config with owner-only permissions#949
bluetoothbot wants to merge 3 commits into
uilibs:mainfrom
bluetoothbot:koan/fix-issue-825v2

Conversation

@bluetoothbot

@bluetoothbot bluetoothbot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

What

The UniFi Protect session-cookie file (~/.config/ufp/unifi_protect.json) was written with aiofiles.open(..., 'wb') and no mode argument, so it landed with the process's umask-derived permissions — typically 0o644 on most Linux hosts. Any other local user can read the file and copy the bearer cookie to impersonate the authenticated user against the NVR.

Fixes #825

Why

On shared Home Assistant appliances or multi-user hosts, 0o644 gives group and world read access to a file that contains long-lived session credentials. The fix eliminates that window entirely on POSIX systems.

How

Added _write_session_config_atomic():

  • Uses tempfile.mkstemp(), which creates with 0o600 (owner read/write only) by default on POSIX — no explicit chmod on the file itself is needed.
  • Writes config data via aiofiles.open (async), then atomically renames into place with aos.replace(), so the live file is never world-readable even momentarily.
  • Tightens the parent config directory to 0o700 on every write via asyncio.to_thread(os.chmod, …), retroactively fixing directories created under an older umask.
  • Windows is a no-op (sys.platform == "win32" guarded, # pragma: no cover).

Both _update_auth_config() and clear_session() now route through this helper. All blocking OS calls (mkstemp, os.close, os.chmod) are wrapped in asyncio.to_thread to satisfy the blockbuster async-purity checker.

Testing

5 new tests in tests/test_api.py (POSIX-only, skipped on Windows):

  • File written with 0o600
  • Config directory tightened to 0o700
  • No orphaned .tmp file after a successful write
  • Temp file cleaned up when write raises (aiofiles.open patched to raise OSError)
  • clear_session() rewrites with 0o600

Full suite: 1683 passed (the one pre-existing failure in test_public_schema_conformance.py::test_public_model_matches_spec[PublicSensor-sensor] is unrelated and already present on main). Patch coverage: 100% on new lines.

SECURITY — session token file written with world-readable permissions (0o644), allowing local credential theft. Fixed in this PR.


Quality Report

Changes: 2 files changed, 176 insertions(+), 6 deletions(-)

Code scan: clean

Tests: failed (FAILED)

Branch hygiene: clean

Generated by Kōan

The session-cookie file (~/.config/ufp/unifi_protect.json) was created
with aiofiles.open(..., 'wb') and no mode argument, so it landed with
the process's default umask permissions — typically 0o644 on most Linux
systems, making the bearer cookie readable to any local user on the host.

On POSIX systems the new _write_session_config_atomic() helper:
- Creates a temporary file via tempfile.mkstemp(), which uses mode
  0o600 by default (owner read/write only).
- Writes the config data to the temp file.
- Atomically renames it into place with aos.replace(), so the live
  file is never world-readable even briefly.
- Tightens the parent config directory to 0o700 on every write, which
  retroactively fixes directories created under older umask settings.
Both _update_auth_config() and clear_session() now route through this
helper. Windows keeps the direct write path (POSIX modes do not apply).

Fixes uilibs#825
@coderabbitai

coderabbitai Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6e6e2ade-bdd4-48e3-a7c2-6875193a20cb

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codspeed-hq

codspeed-hq Bot commented Jun 6, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 4 untouched benchmarks


Comparing bluetoothbot:koan/fix-issue-825v2 (ca9b6e9) with main (16f0cfe)

Open in CodSpeed

@codecov

codecov Bot commented Jun 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
src/uiprotect/api.py 93.17% <100.00%> (+0.05%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@RaHehl

RaHehl commented Jun 19, 2026

Copy link
Copy Markdown
Collaborator

Closing as a duplicate of #834 (both fix #825). #834 has the cleaner implementation (single thread offload, writes the secure mkstemp fd directly via os.fdopen instead of close-then-reopen, and tests through _update_auth_config). Consolidating on one PR; see #834.

@RaHehl RaHehl closed this Jun 19, 2026
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.

Security: Persisted session token file written with world-readable default permissions

2 participants