🔒 [security] Secure Storage of Session Token and App Keys#22
🔒 [security] Secure Storage of Session Token and App Keys#22
Conversation
Specifically: - Updated `src/api.ts` to save the session token with `0o600` permissions. - Updated `src/app.ts` to save the app configuration (containing appKey) with `0o600` permissions. - Replaced hardcoded `'update.json'` in `src/app.ts` with the `updateJson` constant from `src/utils/constants.ts` for consistency. - Added `fs.chmodSync` calls to ensure existing files also have restricted permissions. - Added a security test in `tests/security.test.ts` to verify the fix. Co-authored-by: sunnylqm <615282+sunnylqm@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughFilesystem permission handling is strengthened across API and app modules by explicitly setting credential and update files to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app.ts (1)
31-42:⚠️ Potential issue | 🔴 Critical
getSelectedAppstill uses hardcoded'update.json'instead of theupdateJsonconstant.Lines 34 and 37 reference the hardcoded string
'update.json', butselectApp(lines 129, 131, 142) now uses theupdateJsonconstant. WhenIS_CRESCis true,selectAppwrites to'cresc.config.json'whilegetSelectedAppreads from'update.json', breaking the CRESC variant.🐛 Proposed fix
export function getSelectedApp(platform: Platform) { assertPlatform(platform); - if (!fs.existsSync('update.json')) { + if (!fs.existsSync(updateJson)) { throw new Error(t('appNotSelected', { platform })); } - const updateInfo = JSON.parse(fs.readFileSync('update.json', 'utf8')); + const updateInfo = JSON.parse(fs.readFileSync(updateJson, 'utf8')); if (!updateInfo[platform]) { throw new Error(t('appNotSelected', { platform })); } return updateInfo[platform]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.ts` around lines 31 - 42, getSelectedApp is still using the hardcoded 'update.json' path which breaks the CRESC variant; update getSelectedApp to use the shared updateJson constant (the same constant used by selectApp) instead of the string literal in both the fs.existsSync and fs.readFileSync calls so it reads the correct file when IS_CRESC is true; modify references inside the getSelectedApp function (keep assertPlatform and JSON.parse logic) to use updateJson.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/security.test.ts`:
- Around line 128-138: The test is asserting against the hardcoded filename
'update.json' but selectApp writes to the filename referenced by the updateJson
constant (which can be 'cresc.config.json' when IS_CRESC is true); import
updateJson into tests/security.test.ts and replace the hardcoded 'update.json'
usages (e.g., the targetFile declaration and any fs checks) with the updateJson
constant so the test checks the actual file selectApp writes to; ensure the
import name matches the exported symbol and update references to
fs.existsSync(updateJson) and fs.statSync(updateJson).
- Around line 84-96: The mock registration for the get function
(mock.module('../src/api', ...)) is placed after importing appCommands, so
appCommands has already bound the original get; move the mock.module(...) block
so it runs before any imports that pull in ../src/api (specifically before the
import of appCommands and any other imports that transitively import get).
Ensure mock.module is declared alongside the other mocks at the top of the test
file so that appCommands (and any functions that depend on get) will see the
mocked get implementation.
---
Outside diff comments:
In `@src/app.ts`:
- Around line 31-42: getSelectedApp is still using the hardcoded 'update.json'
path which breaks the CRESC variant; update getSelectedApp to use the shared
updateJson constant (the same constant used by selectApp) instead of the string
literal in both the fs.existsSync and fs.readFileSync calls so it reads the
correct file when IS_CRESC is true; modify references inside the getSelectedApp
function (keep assertPlatform and JSON.parse logic) to use updateJson.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e93e5feb-81f5-4e40-b703-44c884d54be3
📒 Files selected for processing (3)
src/api.tssrc/app.tstests/security.test.ts
| import fs from 'fs'; | ||
| import { saveSession, replaceSession } from '../src/api'; | ||
| import { appCommands } from '../src/app'; | ||
| import { credentialFile } from '../src/utils/constants'; | ||
|
|
||
| // Mock the get function in api.ts | ||
| mock.module('../src/api', () => { | ||
| const original = require('../src/api'); | ||
| return { | ||
| ...original, | ||
| get: mock(async () => ({ appKey: 'test-app-key' })), | ||
| }; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if Bun's mock.module works when called after import
# Look for documentation or patterns in the codebase
# Search for other test files that use mock.module to see the pattern used
fd -e ts -e js . tests --exec grep -l "mock.module" {} 2>/dev/null | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && cat {}'Repository: reactnativecn/react-native-update-cli
Length of output: 6757
Move the ../src/api mock registration before the imports.
The mock for the get function at lines 90-96 is registered after appCommands is imported at line 86. In Bun's module system, mock.module must be called before a module is imported to intercept it. Since appCommands is already bound to the original get function, the subsequent mock won't affect its behavior. Move the ../src/api mock to the beginning with the other mocks (before line 53).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/security.test.ts` around lines 84 - 96, The mock registration for the
get function (mock.module('../src/api', ...)) is placed after importing
appCommands, so appCommands has already bound the original get; move the
mock.module(...) block so it runs before any imports that pull in ../src/api
(specifically before the import of appCommands and any other imports that
transitively import get). Ensure mock.module is declared alongside the other
mocks at the top of the test file so that appCommands (and any functions that
depend on get) will see the mocked get implementation.
| test('selectApp should create update.json with 0o600 permissions', async () => { | ||
| await appCommands.selectApp({ | ||
| args: ['123'], | ||
| options: { platform: 'ios' }, | ||
| }); | ||
|
|
||
| const targetFile = 'update.json'; | ||
| expect(fs.existsSync(targetFile)).toBe(true); | ||
| const stats = fs.statSync(targetFile); | ||
| expect(stats.mode & 0o777).toBe(0o600); | ||
| }); |
There was a problem hiding this comment.
Test hardcodes 'update.json' instead of using the updateJson constant.
If IS_CRESC is true, selectApp writes to 'cresc.config.json' (via the updateJson constant), but the test asserts on the hardcoded 'update.json', causing a false failure. Import and use updateJson for consistency.
💡 Proposed fix
Update the import at line 87:
-import { credentialFile } from '../src/utils/constants';
+import { credentialFile, updateJson } from '../src/utils/constants';Then update the test:
test('selectApp should create update.json with 0o600 permissions', async () => {
await appCommands.selectApp({
args: ['123'],
options: { platform: 'ios' },
});
- const targetFile = 'update.json';
+ const targetFile = updateJson;
expect(fs.existsSync(targetFile)).toBe(true);
const stats = fs.statSync(targetFile);
expect(stats.mode & 0o777).toBe(0o600);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('selectApp should create update.json with 0o600 permissions', async () => { | |
| await appCommands.selectApp({ | |
| args: ['123'], | |
| options: { platform: 'ios' }, | |
| }); | |
| const targetFile = 'update.json'; | |
| expect(fs.existsSync(targetFile)).toBe(true); | |
| const stats = fs.statSync(targetFile); | |
| expect(stats.mode & 0o777).toBe(0o600); | |
| }); | |
| test('selectApp should create update.json with 0o600 permissions', async () => { | |
| await appCommands.selectApp({ | |
| args: ['123'], | |
| options: { platform: 'ios' }, | |
| }); | |
| const targetFile = updateJson; | |
| expect(fs.existsSync(targetFile)).toBe(true); | |
| const stats = fs.statSync(targetFile); | |
| expect(stats.mode & 0o777).toBe(0o600); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/security.test.ts` around lines 128 - 138, The test is asserting against
the hardcoded filename 'update.json' but selectApp writes to the filename
referenced by the updateJson constant (which can be 'cresc.config.json' when
IS_CRESC is true); import updateJson into tests/security.test.ts and replace the
hardcoded 'update.json' usages (e.g., the targetFile declaration and any fs
checks) with the updateJson constant so the test checks the actual file
selectApp writes to; ensure the import name matches the exported symbol and
update references to fs.existsSync(updateJson) and fs.statSync(updateJson).
🎯 What: The vulnerability fixed
This PR addresses a security vulnerability where sensitive credential and configuration files (session tokens and app keys) were being stored with default (insecure) file permissions.
If these files are stored with default permissions (often
0o664or0o644), other users on the same multi-user system could potentially read the session tokens or app keys, leading to unauthorized access to the user's account or applications.🛡️ Solution: How the fix addresses the vulnerability
The fix ensures that these files are created and maintained with
0o600(read/write for owner only) permissions. This is achieved by:{ mode: 0o600 }tofs.writeFileSync.fs.chmodSync(file, 0o600)immediately after writing to handle cases where the file already exists (aswriteFileSyncdoes not change permissions on existing files).src/app.tsto use the branding-awareupdateJsonconstant instead of a hardcoded string.PR created automatically by Jules for task 2150250182784982543 started by @sunnylqm
Summary by CodeRabbit
Bug Fixes
Tests