Skip to content

fix(usage): request owner-only modes for cache snapshots - #31027

Open
ka2umaz wants to merge 1 commit into
stablyai:mainfrom
ka2umaz:fix/private-usage-cache-snapshots
Open

ka2umaz wants to merge 1 commit into
stablyai:mainfrom
ka2umaz:fix/private-usage-cache-snapshots

Conversation

@ka2umaz

@ka2umaz ka2umaz commented Oct 20, 2026

Copy link
Copy Markdown

ELI5

Usage caches can contain session and project metadata. When a snapshot replaces a file, its new temporary file currently inherits the process's default creation permissions. Request owner-only permissions for new cache snapshots so a successful save does not turn a 0600 file into 0644 under the tested umask 022 conditions.

What Changed

  • Add an optional mode argument to writeFileDurableIfCurrent and forward it to writeTempFileDurable.
  • Pass 0o600 only from the shared provider UsageCacheSnapshotWriter.
  • Add eight filesystem regression cases. Other callers' defaults and the existing flush, generation/rename veto, and cleanup behavior are retained.

Why

The cache writer can state its intended permissions at temporary-file creation, before the file is published. This keeps the change small and avoids changing every durable writer or chmodding userData and existing unrelated files. It is a storage-permissions improvement; actual cross-user exposure has not been established.

Linked Issue

Fixes #31026

Visual Proof

N/A — file creation permissions only; no visual or interaction change.

Testing

  • I manually tested these changes locally
  • Automated tests added/updated

On macOS arm64, Node 24.17.0, using the repository's Vitest configuration:

ORCA_BACKGROUND_LAUNCH=1 node node_modules/vitest/vitest.mjs run --config config/vitest.config.ts src/main/usage-cache-snapshot-writer.test.ts src/main/durable-file-write.test.ts src/main/durable-file-write-syscall-proof.test.ts src/main/codex-accounts/fs-utils.test.ts

All 41 related tests pass, including eight added cases. Before the fix, six of the eight added cases fail. Node type checking (tc:node --compiler=native), the changed-code-quality gate, scoped oxlint, oxfmt, and git diff --check also pass. The prepared patch was rechecked on main snapshot ba6599eabbdfed02eb7954c174bea704f853ecdc; the three submitted files are byte-identical to the previously tested patch. Existing isolated dependencies were reused; the intervening lockfile change adds a node-gyp root declaration and does not alter the tools used by these checks.

Separate synthetic filesystem checks exercised umask 022/077, initial creation, replacement of existing 0600/0644 files, and 0700/0755 parents. The regression tests do not modify the runner's umask and also inspect the requested creation mode, so an existing restrictive umask cannot hide its omission.

Not run: full repository suite, aggregate pnpm lint/pnpm typecheck, full app build, Bun runtime, application/real agent, Linux, Windows, or SSH. Full cross-platform CI remains outstanding; no claim that it has passed.

AI Disclosure

OpenAI Codex (GPT-6) assisted with source review, implementation, synthetic tests, and this description. The investigation used public source and synthetic data only.

Review

AI code review: the optional argument leaves existing callers unchanged. Node path APIs remain portable; POSIX assertions are skipped on Windows and 0o600 is not represented as a Windows ACL guarantee. The change runs on the host writing the snapshot, makes no local/SSH path or transport assumptions, and is shared across provider usage caches. It adds no scan, chmod sweep, extra write, or UI work. No provider-specific commands, credentials, integration settings, or git-provider behavior change. Existing temporary-file and directory access controls are not expanded by this patch.

Agent skill upstream boundary

  • Not applicable: no upstream skill-installer material is copied or translated.

Notes

Creation mode applies to a new temporary inode and remains subject to umask/platform behavior; it does not chmod a pre-existing temporary file. Existing cache files are updated only after a successful replacement write. Parent modes and untouched files are unchanged. Actual userData/ancestor permissions, ACLs, and cross-user access were not inspected. This does not claim a confirmed vulnerability and is separate from terminal-history PR #19954.

Author

GitHub: @ka2umaz. X/Twitter handle: not provided.

Checklist

  • This PR is small and focused
  • What changed, why, and before/after behavior are explained
  • Visual proof is N/A with reason
  • AI self-review covers correctness, security, compatibility, and performance within the stated scope
  • Cross-platform, SSH/remote, and path impact considered; untested environments disclosed
  • Full pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass — outstanding for CI; only scoped checks above ran locally

@coderabbitai

coderabbitai Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

📝 Walkthrough

The durable-write helper accepts an optional file-creation mode and forwards it to temporary-file creation. The usage-cache snapshot writer passes mode 0o600. Tests cover temporary and final file permissions, preserved parent permissions, default behavior when the mode is omitted, and cleanup when a commit is vetoed.

Priority: ⬇️ Low

Merge Risk: 🔵 Low · up to 34833

Snapshot permissions can be correct while the new tests fail under a restrictive umask. This is a bounded test-portability issue, not evidence that cache files are exposed.

Pre-merge checks: Passed 5
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: requesting owner-only permissions for usage cache snapshots.
Description check Passed The description follows the repository template, explains the user impact and implementation, links issue #31026, documents testing and known limitations, and completes the required checklist. Outstan…
Linked Issues check Passed The changes satisfy issue #31026. writeFileDurableIfCurrent accepts an optional mode and forwards it to writeTempFileDurable. UsageCacheSnapshotWriter passes 0o600. The eight added cases cov…
Out of Scope Changes check Passed The changes stay within issue #31026. The source changes add only mode propagation and the shared usage-cache mode. The tests verify the requested permissions and preserved behavior. The diff contains…
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 3 files.
Full details: Description check

Explanation

The description follows the repository template, explains the user impact and implementation, links issue #31026, documents testing and known limitations, and completes the required checklist. Outstanding full-suite and cross-platform checks are clearly disclosed.

Full details: Linked Issues check

Explanation

The changes satisfy issue #31026. writeFileDurableIfCurrent accepts an optional mode and forwards it to writeTempFileDurable. UsageCacheSnapshotWriter passes 0o600. The eight added cases cover initial creation, replacement of 0600 and 0644 files, 0700 and 0755 parents, omitted-mode defaults, and veto cleanup. The diff preserves the existing rename and cleanup flow.

Full details: Out of Scope Changes check

Explanation

The changes stay within issue #31026. The source changes add only mode propagation and the shared usage-cache mode. The tests verify the requested permissions and preserved behavior. The diff contains no chmod sweep, parent-directory update, unrelated-file migration, provider-specific behavior, or UI change.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: fbba66b7-1563-4191-9a2b-5a02beb49362
📥 Commits

Reviewing files that changed from the base of the PR and between ba6599e and 348335b.

📒 Files selected for processing: 3
  • src/main/durable-file-write.ts
  • src/main/usage-cache-snapshot-writer.test.ts
  • src/main/usage-cache-snapshot-writer.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

await writer.flush()

// Why: checking the open mode also catches omissions when the runner's umask is 077.
expect(tempWrites).toEqual([{ creationMode: 0o600, actualMode: 0o600 }])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Account for umask in the private-file assertions.

If the POSIX process umask removes an owner bit, a file created with mode 0o600 will not have mode 0o600. For example, umask 0o200 produces mode 0o400. This assertion, and the later private-file mode assertions, then fail even though the writer honors the requested creation mode. Compare actualMode and the final file modes with 0o600 & ~process.umask(); keep the creationMode assertion at 0o600. Node applies the mode when it creates the file. (nodejs.org)

This branch has not been deployed

No deployments
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.

[Other]: Request owner-only creation mode for usage-cache snapshots

1 participant