fix: set secure file permissions (0600) for credential storage#353
Conversation
WalkthroughAdded ensureSecurePermissions(path) to set POSIX permissions to 0o600 where supported, invoked before reading storage in loadAccounts and loadAccountsUnsafe. writeFile calls updated to pass options { encoding: "utf-8", mode: 0o600 } in ensureFileExists (initial create) and in saveAccounts (temporary file write), so the temp file is created with restrictive permissions before rename. No exported API signatures changed. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
Greptile OverviewGreptile SummaryThis PR addresses a critical security vulnerability where OAuth refresh tokens stored in Changes Made
Security ImpactThis fix prevents local privilege escalation attacks where other users on the same system could read sensitive OAuth credentials. The implementation is comprehensive:
The fix correctly handles all code paths where the credentials file is accessed. Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant App as Application
participant Storage as storage.ts
participant FS as File System
participant File as antigravity-accounts.json
Note over App,File: Initial File Creation (ensureFileExists)
App->>Storage: ensureFileExists()
Storage->>FS: access(path)
FS-->>Storage: ENOENT (file doesn't exist)
Storage->>FS: mkdir(dirname, recursive)
Storage->>FS: writeFile(path, data, {mode: 0o600})
FS->>File: Create with 0600 permissions
File-->>Storage: File created securely
Note over App,File: Loading Existing Files (loadAccounts)
App->>Storage: loadAccounts()
Storage->>Storage: ensureSecurePermissions(path)
Storage->>FS: chmod(path, 0o600)
Note right of FS: Fixes existing files<br/>with insecure permissions
FS-->>Storage: Permissions updated
Storage->>FS: readFile(path)
FS->>File: Read credentials
File-->>Storage: Account data
Storage-->>App: AccountStorageV3
Note over App,File: Saving Accounts (saveAccounts)
App->>Storage: saveAccounts(storage)
Storage->>Storage: withFileLock()
Storage->>Storage: loadAccountsUnsafe()
Storage->>Storage: ensureSecurePermissions(path)
Storage->>FS: chmod(path, 0o600)
Storage->>Storage: mergeAccountStorage()
Storage->>FS: writeFile(tempPath, data, {mode: 0o600})
FS->>File: Create temp file with 0600
Storage->>FS: rename(tempPath, path)
Note right of FS: Atomic operation<br/>preserves permissions
FS->>File: Replace with secure temp file
Storage-->>App: Save complete
|
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/plugin/storage.ts
Line: 238:242
Comment:
the `ensureFileExists` function creates the accounts file with insecure default permissions (typically 0644)
```suggestion
await fs.writeFile(
path,
JSON.stringify({ version: 3, accounts: [], activeIndex: 0 }, null, 2),
{ encoding: "utf-8", mode: 0o600 },
);
```
How can I resolve this? If you propose a fix, please make it concise. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugin/storage.ts (1)
534-540:⚠️ Potential issue | 🟠 MajorBackfill 0600 on existing credential files (not just new writes).
The change secures new temp files via
fs.writeFile(..., { mode: 0o600 }), butensureFileExistscreates initial files without a mode option, resulting in default permissions (~0o644). More critically, on Unix systemsfs.renamepreserves destination file permissions, so existingantigravity-accounts.jsonat 0o644 remains insecure through upgrades unless explicitly remediated. Applychmodduring load in bothloadAccounts()andloadAccountsUnsafe()so all read paths fix permissions on existing files.🔒 Suggested chmod in load paths
+async function ensureSecurePermissions(path: string): Promise<void> { + try { + await fs.chmod(path, 0o600); + } catch { + // Best-effort: ignore on unsupported filesystems / Windows + } +} + export async function loadAccounts(): Promise<AccountStorageV3 | null> { try { const path = getStoragePath(); + await ensureSecurePermissions(path); const content = await fs.readFile(path, "utf-8");async function loadAccountsUnsafe(): Promise<AccountStorageV3 | null> { try { const path = getStoragePath(); + await ensureSecurePermissions(path); const content = await fs.readFile(path, "utf-8");
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
src/plugin/storage.ts
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Summary
This PR fixes a security vulnerability where
antigravity-accounts.json(containing OAuth refresh tokens) was created with default file permissions (typically world-readable0644).Changes
ensureFileExiststo create new files with secure0600permissions immediately (preventing race conditions).loadAccounts(backwards compatibility).saveAccountsto use0600mode when writing temporary files.ensureSecurePermissionshelper (best-effort on Windows).Impact
Prevents local privilege escalation where other users on the system could read sensitive credentials.