security(photon): create auth.json temp file with 0o600 atomically

[solyanviktor-star]

Problem

_save_auth() in plugins/platforms/photon/auth.py persists the Photon dashboard bearer token (and project secret via store_project_credentials) like this:

tmp = path.with_suffix(".json.tmp")
with tmp.open("w", encoding="utf-8") as fh:   # created at umask — typically 0o644
    json.dump(data, fh, ...)
os.chmod(tmp, 0o600)                            # too late: TOCTOU window
tmp.replace(path)

Two issues:

1. World-readable window — the file is created at process umask (usually 0o644) and only tightened to 0o600 after the secret is written. Any local user can read the token during that window.
2. Predictable temp name — auth.json.tmp is fixed, so it can be pre-planted (symlink attack): open("w") happily follows an existing symlink and writes the secret wherever it points.

This exact pattern was already hardened in the core writer — hermes_cli/auth.py:_save_auth_store (see #19673, #21148, #43589) — but the Photon plugin writer was left on the old sequence.

Fix

Mirror _save_auth_store:

• create the temp file with os.open(O_WRONLY | O_CREAT | O_EXCL, 0o600) — correct permissions from the first byte, and O_EXCL rejects a pre-planted file/symlink;
• per-process random temp suffix (auth.json.tmp.<pid>.<uuid>) — no collisions between concurrent writers, no reusable predictable path;
• fsync before the atomic replace;
• best-effort temp cleanup on failure.

No behavior change on Windows (POSIX mode bits are not enforced there); the atomic-replace contract is unchanged.

Tests

• test_save_auth_never_world_readable (POSIX-only) — auth.json ends up 0o600;
• test_save_auth_leaves_no_temp_files — no stale temp files after a write.

python -m pytest tests/plugins/platforms/photon/test_auth.py — 33 passed, 1 skipped (POSIX-only test on Windows).

🤖 Generated with Claude Code

[falkoro]

Reviewed against main: the removed code did tmp.open("w") → write token → chmod(0o600), which leaves the bearer token world-readable at process umask during the write window, on a predictable temp name (.json.tmp) that could also be pre-planted as a symlink. The replacement — os.open(O_WRONLY|O_CREAT|O_EXCL, 0600) + fdopen + fsync + unlink-on-failure — is exactly the canonical pattern already used for the main credential store (verified: hermes_cli/auth.py _save_auth_store, O_EXCL + 0o600), so this brings the Photon plugin in line with house style rather than inventing a new one.

Ran tests/plugins/platforms/photon/test_auth.py on the branch: 34 passed (Python 3.12). LGTM.

[teknium1]

Thanks for finding a real Photon credential-writer gap. Current main still writes through plugins/platforms/photon/auth.py:112-119 using a fixed auth.json.tmp, writes secrets before chmod(0o600), and then replaces the target, so the PR's security premise is confirmed.

Problems

• plugins/platforms/photon/auth.py:127 opens a raw descriptor before os.fdopen(). If os.fdopen() raises before taking ownership, the new exception handler unlinks the temp file but does not close fd. The cross-referenced credential-writer PR #62837 added this exact cleanup in commit e3ac0ecb5f99.
• tests/plugins/platforms/photon/test_auth.py:74-80 only checks the final mode. The old writer would also pass that assertion after its post-write chmod, so it does not protect the atomic-create guarantee.

Suggested changes

• Close the raw descriptor if os.fdopen() fails, and add a forced-fdopen-failure cleanup test.
• Add an os.open spy asserting O_CREAT, O_EXCL, and owner-only mode, following tests/hermes_cli/test_auth_toctou_file_modes.py:163-201.

Automated hermes-sweeper review.

[teknium1]

If os.fdopen() raises before it assumes ownership of fd, this handler only unlinks tmp; the raw descriptor remains open. Wrap fdopen so that failure closes fd before re-raising, and add the corresponding regression test.

[teknium1]

This asserts only the final target mode, which the removed open→write→chmod implementation also produced. Please additionally spy on os.open and assert O_CREAT | O_EXCL plus the explicit 0o600 mode, as the core auth regression test does.

[solyanviktor-star]

Addressed both review points in the follow-up commit:

• Raw fd leak: _save_auth now closes the descriptor itself when os.fdopen() raises before taking ownership (and still removes the temp file), following the cleanup pattern from fix(docker): create re-seeded auth.json at 0o600 to close a credential-exposure window #62837 (e3ac0ecb5f99).
• Test strength: added an os.open spy asserting O_CREAT | O_EXCL and an explicit 0o600 mode for the auth.json.tmp open, modeled on tests/hermes_cli/test_auth_toctou_file_modes.py — the final-mode-only check would indeed also pass under the old open→write→chmod writer, so this protects the atomic-create guarantee itself. Also added a forced-fdopen-failure test asserting the raw fd is closed and no temp file is left behind; it fails against the previous branch state (verified by stashing the source diff).

tests/plugins/platforms/photon/test_auth.py: 35 passed, 1 skipped (POSIX mode-bit test on Windows).

[GottZ]

This was generated by AI during triage.

Summary

Three PRs touch Photon authentication, but they address distinct root causes: #42539 unwraps dashboard project responses, #60427 hardens temporary credential-file creation, and #64902 serializes shared auth.json updates.

Related pull requests

• #42539 [closed] related — (+749/-169) — do not reopen as-is: the intended create-project response unwrapping fixes the verified wrapped-data credential-persistence failure, but the full diff also contains unrelated cron behavior, an Argus specification, and dependency/lockfile churn. Despite the keep_open review on #42539, the complete diff is not a focused implementation; the Photon fix remains relevant only as a candidate for a clean extraction.
• #60427 related — (+129/-7) — merge: the diff replaces the predictable, initially umask-permissioned temp file with randomized O_EXCL creation at 0o600, fsync, atomic replacement, and failure cleanup. The COMMENTED keep_open review on #60427 identified a raw-fd leak and insufficient atomic-create coverage; the follow-up explicitly closes the fd on fdopen failure and adds direct O_CREAT/O_EXCL/0o600 plus cleanup regression tests.
• #64902 related — (+144/-29) — merge separately: the diff wraps all three Photon auth.json load-mutate-save transactions in the repository's existing cross-process auth-store lock, preventing lost updates without changing the separate .env write. This is complementary to #60427 and matches the keep_open review on #64902.

Suggested consolidation

Merge #60427 after confirming the reviewed follow-up commit is present; it is the focused fix for atomic secret-file creation and now addresses both contributor review findings. #64902 should be handled as a separate complementary concurrency fix, while #42539 should remain closed unless its small Photon response-unwrapping change is resubmitted without the unrelated cron, documentation, and dependency changes; no PRs here are duplicates.

Cross-PR triage: Reviewed 3 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 54 kB of PR diffs, 6 kB of issue/PR text, 4 kB of discussion (6 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

[solyanviktor-star]

Confirming the reviewed follow-up is present: commit 3175a2d8b ("fix(photon): close the raw fd when os.fdopen fails in _save_auth") sits on top of 2e1ab26d6, so both of @teknium1's findings are addressed in the current head:

• raw-fd leak — os.fdopen() is wrapped; on failure the descriptor is closed before the temp file is unlinked and the error re-raised (plugins/platforms/photon/auth.py), with a forced-fdopen-failure regression test.
• atomic-create coverage — test_save_auth_uses_os_open_with_0o600_mode spies on os.open and asserts O_CREAT | O_EXCL plus the explicit 0o600 mode, matching the core auth regression test, rather than only checking the final mode (which the old open→write→chmod writer would also have passed).

Agreed on the split: #64902 is complementary (cross-process lock around the load-mutate-save transactions) and touches different lines, so the two merge independently with no conflict. Branch is mergeable and CI is green.

[GottZ]

This was generated by AI during triage.

Delta since our previous triage comment

@solyanviktor-star supplied the missing follow-up evidence: the current #60427 diff includes explicit raw-descriptor cleanup when os.fdopen() fails, a forced-failure cleanup test, and direct assertions for O_CREAT, O_EXCL, and 0o600. This addresses both findings from @teknium1’s COMMENTED keep_open review; the discussion identifies commit 3175a2d as containing the follow-up, without relying on that commit being the PR head.

Changed pull requests

• #60427 related — (+129/-7) — merge: New evidence confirms that the current diff addresses the COMMENTED keep_open review on #60427 by closing the raw fd on fdopen failure and testing both cleanup and atomic restricted creation directly.

Suggested consolidation

The previous recommendation is unchanged: merge #60427; no duplicate-structure change was established.

Complex graph unchanged since our previous triage comment.

Cross-PR triage: Reviewed 3 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 54 kB of PR diffs, 6 kB of issue/PR text, 5 kB of discussion (7 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

[teknium1]

Merged via #73562 — cherry-picked (both commits, incl. the fd-leak follow-up) with authorship preserved. Thanks for mirroring the hardened core writer.