Engineering & roadmap
Implementation review: PB-005, PR 593
Development documentation. This guide follows a reviewed snapshot of main and may describe capabilities not yet in Stable. Check Preview labels and release notes. Some executable names and paths remain unchanged for compatibility. Legacy names explained ↗
| Field | Value |
|---|---|
| Backlog item | PB-005 Encrypt the token cache (feature FEAT-AUTH-03; hardening HD-013 / EN-10; decision D-C2) |
| Pull request | #593, merged as 06c9aa94 (head cebff79b; work in 12175a8e) |
| Package | None (no docs/product/pr-ready/PB-005.md; built as HD-013) |
| Reviewer | Engineering quality review thread. It didn't write #593. |
| Date | 2026-10-27 |
| Environment | Linux container, main 6159e507, Python venv for the engine, Playwright Chromium. No tenant, no Windows, no second PC. |
Outcome#
Changes Required (recommended).
The encryption part looks right in code, and its tests fail on the code from before #593. Criterion 2 isn't fully met. The app and run-schedule each keep the whole cache in memory and write it back with no lock and no re-read, so the last writer wins. A scheduled run that is still going when the person clicks Sign out writes the signed-out account back (shown below). That behaviour predates #593, but this item's criterion asks for it to be safe. The DPAPI calls themselves can't run here: whether another Windows user can't read the cache is checked only by reading the code, plus a Windows CI test whose run wasn't seen.
Recommended by: Engineering quality review. Decided by (product owner): pending.
Acceptance criteria#
| # | Criterion (copied from the backlog item) | Result | Evidence |
|---|---|---|---|
| 1 | The cache is unreadable to another Windows user | Met by code reading; unable to verify on Windows | dpapi.protect calls CryptProtectData with only CRYPTPROTECT_UI_FORBIDDEN (sidecar/fabric_desktop/dpapi.py:15, 44), so the user's key is used, not the machine's (no CRYPTPROTECT_LOCAL_MACHINE), with fixed entropy (dpapi.py:14). cache_bytes writes KFDPAPI1\n plus the blob on Windows (auth.py:142-145). read_cache treats a blob it can't decrypt as no sign-in and logs a warning (auth.py:124-139). Tests with DPAPI faked: test_saved_cache_is_encrypted_and_reads_back (no secret-access bytes on disk) and test_cache_another_account_cant_decrypt_means_signing_in_again. test_real_dpapi_round_trip runs the real API only on Windows; it was skipped here, and CI runs pytest on windows-latest (.github/workflows/ci.yml:18-29). The run wasn't seen (no GitHub access here). Nobody has tried a second Windows account. |
| 2 | Scheduled runs and the app share it safely | Not met | Partly there. The cache is written to msal_cache.bin.<pid>.tmp and swapped in with os.replace (auth.py:223-230), so neither process ever reads a half-written file. Schedules use LogonType InteractiveToken (local_schedules.py:147), so they run as the same signed-in Windows account and can decrypt the cache. What's missing: there is no file lock and no re-read before a write. The app builds one Authenticator for its whole session (service.py:134-137), run-schedule builds its own (credentials.py:174), and each _save writes its own whole in-memory cache (auth.py:213-230). The reviewer script below shows a Sign out undone by a scheduled run that was already going. |
Checks#
| Check | Result | Evidence or note |
|---|---|---|
| Scope matches the item and package | Met with a note | The item proposed msal-extensions. #593 calls DPAPI through ctypes (dpapi.py) instead, so no package is added, as D-C2 records (implementation-decisions.md:23). That also means it doesn't get msal-extensions' PersistedTokenCache file lock and reload, which is the part criterion 2 needs. |
| New tests fail without the change, pass with it | Met | Copy of main with #593's auth.py diff reversed (git diff 12175a8e^ cebff79b -- auth.py | patch -R) and DPAPI_HEADER defined so the test module imports: test_saved_cache_is_encrypted_and_reads_back and test_plain_cache_from_an_earlier_version_is_encrypted_on_first_start fail (the file on disk is plain JSON). The other 4 pass there, as expected: they cover behaviour the old code already had (damaged file, plain file off Windows, an unreadable file meaning sign in again) or that it can't reach (DPAPI refusing). With main: 6 passed, 1 skipped (real DPAPI, Windows only). |
| Full suites pass (pytest, UI, cargo) | Partly | pytest tests/test_token_cache_dpapi.py tests/test_local.py: 39 passed, 1 skipped. pytest tests/test_app_settings.py tests/test_policy.py tests/test_policy_fail_closed.py: 59 passed, 1 skipped. The full engine suite wasn't run here. No UI or Rust change. |
| Reachable where docs say; Preview behavior right; packaged into the engine | Met | Background only. dpapi.py is in the fabric_desktop package; it uses only ctypes. |
| OneLake data safety | n/a | Local sign-in cache only. |
| Authentication and distribution | Partly | Migration: a plain cache is read, then written again encrypted at once (_save(force=True), auth.py:189-193), replacing the plain file in place with os.replace. The plain bytes aren't wiped from the disk, and a msal_cache.bin.<pid>.tmp that an older build left behind after a crash would stay plain (nothing removes *.tmp). If DPAPI refuses, the last file is kept and nothing is written plain (auth.py:218-222; test_when_dpapi_refuses_…). Off Windows the cache is plain JSON in a file created 0600 (test_off_windows_the_cache_stays_a_private_plain_file). A downgrade to an older build can't parse the encrypted file and signs in again. backup.py:40 already leaves msal_cache.bin out of backups. No new scope. |
| No unproven claim; docs honest about limits | Met | security.md says plainly that any program or notebook running as the same user can still decrypt the cache, and that this needs a Windows check. FEAT-AUTH-03 is Partially Verified. |
| Docs match the code; linked; strict docs build passes | Partly | install.md, security.md and permissions.md match the code. security.md says the cache is "written atomically" and calls it safe to share, but doesn't say a concurrent run can overwrite a sign-out. The strict docs build wasn't run here. |
| Accessibility of new controls | n/a | No UI. |
Inventory and backlog updated; product_data.py consistent |
Partly | FEAT-AUTH-03 next action is Chris's Windows check (FEATURE_INVENTORY.md:277). Its known limitation says DPAPI "is checked by tests with the Windows call faked", but the real round trip is also a Windows CI test (test_token_cache_dpapi.py:90-96). PB-005 is "In review" (PRODUCT_BACKLOG.md:1370). product_data.py wasn't run. |
Reviewer script: $S/pb005demo/race.py (scratchpad, run against main's sidecar with DPAPI faked as in the tests). The app signs in and saves; run-schedule builds its Authenticator from the file; the app signs out (sign_out(), which removes the account and saves); the scheduled run refreshes a token and saves. Output:
sched sees refresh tokens: 1 accounts: 1
after app sign-out, file has RTs: 0 accounts: 0
after scheduled save, file has RTs: 1 accounts: 1
Findings#
| # | Severity (blocking / should fix / note) | Finding | Where | Status |
|---|---|---|---|---|
| 1 | blocking (for criterion 2) | The app and run-schedule each write their whole in-memory cache with no lock and no merge. A Sign out, or the sign-out after a disallowed tenant (_check_tenant, auth.py:263-272), is undone when a run that was already going saves. The other way round, a new sign-in in the app (for example another account) is overwritten by an older copy, and the next scheduled run uses the old account. Fix: take a lock file around read, merge and write, and re-read the file before each save (as msal-extensions' PersistedTokenCache does). At least, have a save after sign-out win, or have runs reload before saving. Present before #593. |
sidecar/fabric_desktop/auth.py:213-230, 298-301 | Open |
| 2 | should fix | On Windows, os.replace fails with PermissionError if the other process has msal_cache.bin open at that moment (Python opens files without FILE_SHARE_DELETE). _save doesn't catch it, so the token call that saved fails (auth.py:257, 278). The window is short (read_bytes at start-up). A lock (finding 1) or a retry would close it. Code reading only. |
auth.py:230 | Open |
| 3 | should fix | Nobody has checked on Windows that a second Windows account (or the same profile read from another PC) can't use the file, or that a schedule run by Task Scheduler decrypts it. That needs Chris's check (FEAT-AUTH-03 next action) plus a second local account. | Windows PC | Open: Chris |
| 4 | note | Migration replaces the plain file in place but doesn't wipe it. Old plain contents can stay in free disk space, shadow copies or a backup taken before the upgrade, and a stray plain msal_cache.bin.*.tmp from an older build is never removed. Refresh tokens from before the upgrade stay valid until they expire or are revoked; signing out and in again replaces them. |
auth.py:189-193 | Open: say it in security.md or delete *.tmp on start |
| 5 | note | FEAT-AUTH-03's known limitation says DPAPI is checked only with the Windows call faked. test_real_dpapi_round_trip runs the real call on the Windows CI runner. |
FEATURE_INVENTORY.md:524 | Open: product data owner |
Inventory and backlog changes#
- Features changed: FEAT-AUTH-03: add the shared-cache limitation (finding 1) and correct the DPAPI test note (finding 5). Stays Partially Verified until the Windows check (finding 3).
- Backlog item state: PB-005 stays In review until finding 1 is fixed or accepted as a limitation by the product owner.
Need a hand?
Find a fix in troubleshooting, or share a reproducible issue with the community.
Get support ↗Improve this guide on GitHub ↗