fix: pass Supabase session tokens via env to avoid argv disclosure (CWE-200)#458
Conversation
…E-200) Tokens (access_token, refresh_token) were passed as command-line arguments to the blocking-offline-update.js subprocess, making them visible to any local user via ps aux or /proc/PID/cmdline. Move sensitive tokens to environment variables (SUPABASE_ACCESS_TOKEN, SUPABASE_REFRESH_TOKEN) which are only readable by the process owner.
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
📝 WalkthroughWalkthroughMigrates subprocess authentication from positional CLI args to environment variables. Changes
Sequence Diagram(s)sequenceDiagram
participant RemoteChannel as RemoteChannel (parent)
participant OS as OS / ChildProcess
participant OfflineScript as blocking-offline-update.js (child)
participant Supabase as Supabase API
RemoteChannel->>RemoteChannel: check Supabase session (access/refresh tokens)
alt refresh_token missing
RemoteChannel-->>RemoteChannel: log error, skip spawn
else refresh_token present
RemoteChannel->>OS: spawn child process with env {SUPABASE_ACCESS_TOKEN, SUPABASE_REFRESH_TOKEN}
OS->>OfflineScript: start (reads tokens from process.env)
OfflineScript->>Supabase: perform offline update using env tokens
Supabase-->>OfflineScript: response
OfflineScript-->>OS: exit
OS-->>RemoteChannel: child exit status
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
codeant-ai
Bot
added
the
size:L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/test-cwe200-credential-exposure.js (1)
46-54: ⚡ Quick winHarden the argv regression checks against trivial renames/formatting changes.
These assertions only catch token names on the same line as
process.argv, and the args-array check ignoresaccessToken/refreshToken. A regression that destructures argv across multiple lines or uses camelCase locals could put secrets back into argv without failing this test.🔍 Tighten the source checks
- const lines = content.split('\n'); - - // Check every line that references process.argv — none should destructure token variables - for (const line of lines) { - if (line.includes('process.argv')) { - assert.ok( - !/accessToken|refreshToken|access_token|refresh_token/i.test(line), - `Line reads tokens from process.argv: ${line.trim()}` - ); - } - } + const normalized = content.replace(/\s+/g, ' '); + assert.ok( + !/process\.argv[\s\S]{0,200}(accessToken|refreshToken|access_token|refresh_token)/i.test(normalized), + 'Script should not source tokens from process.argv' + ); @@ - !argsContent.includes('access_token') && !argsContent.includes('refresh_token'), + !/(accessToken|refreshToken|access_token|refresh_token)/i.test(argsContent), `Tokens should not be in command args: ${argsContent.trim()}` );Also applies to: 83-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test-cwe200-credential-exposure.js` around lines 46 - 54, The current test loop that scans lines for "process.argv" (the for loop using variable lines and the assert.ok with the regex /accessToken|refreshToken|access_token|refresh_token/i) is too brittle because it only looks on the same line and misses multi-line destructuring or renames; update the test to (1) detect any usage of process.argv by parsing the source into an AST (or at minimum join a sliding window of adjacent lines) and check surrounding lines for identifier names that match token variants, (2) broaden the token-name check to canonicalize and match camelCase/snake_case/kebab variants (e.g., normalize to lower-case and match access ? token|refres h? token), and (3) apply the same strengthened checks to the second block referenced (the args-array/assert block around lines 83-88) so destructuring like const { accessToken } = process.argv or multi-line destructures are caught; locate and modify the loop that references process.argv, the regex used in assert.ok, and the second args-array check to implement these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/test-cwe200-credential-exposure.js`:
- Around line 46-54: The current test loop that scans lines for "process.argv"
(the for loop using variable lines and the assert.ok with the regex
/accessToken|refreshToken|access_token|refresh_token/i) is too brittle because
it only looks on the same line and misses multi-line destructuring or renames;
update the test to (1) detect any usage of process.argv by parsing the source
into an AST (or at minimum join a sliding window of adjacent lines) and check
surrounding lines for identifier names that match token variants, (2) broaden
the token-name check to canonicalize and match camelCase/snake_case/kebab
variants (e.g., normalize to lower-case and match access ? token|refres h?
token), and (3) apply the same strengthened checks to the second block
referenced (the args-array/assert block around lines 83-88) so destructuring
like const { accessToken } = process.argv or multi-line destructures are caught;
locate and modify the loop that references process.argv, the regex used in
assert.ok, and the second args-array check to implement these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 112c39cf-286c-4ec6-a199-5ba1bb917d94
📒 Files selected for processing (3)
src/remote-device/remote-channel.tssrc/remote-device/scripts/blocking-offline-update.jstest/test-cwe200-credential-exposure.js
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| env: { | ||
| ...process.env, | ||
| SUPABASE_ACCESS_TOKEN: sessionData.session.access_token, | ||
| SUPABASE_REFRESH_TOKEN: sessionData.session.refresh_token || '' |
There was a problem hiding this comment.
Suggestion: refresh_token is nullable in this codebase, but this line coerces missing values to an empty string and still spawns the offline-update subprocess. The child script treats an empty refresh token as missing and exits with code 1, so device shutdown can silently fail to mark the device offline whenever the session lacks a refresh token. Validate sessionData.session.refresh_token before spawning and skip/fallback explicitly instead of passing ''. [logic error]
Severity Level: Major ⚠️
- ❌ Device shutdown path fails to mark DB offline.
- ⚠️ `mcp_devices` table keeps stale 'online' statuses.
- ⚠️ RemoteChannel.setOffline() reliability reduced when refresh_token null.Steps of Reproduction ✅
1. Complete the device authorization flow where the backend returns a PollResponse without
a refresh_token field, which is explicitly supported by `PollResponse.refresh_token?:
string` in `src/remote-device/device-authenticator.ts:21-24`. In this case,
`pollForAuthorization()` at `device-authenticator.ts:112-24` constructs an `AuthSession`
with `refresh_token: data.refresh_token || null`, so refresh_token is stored as `null`.
2. Initialize the remote channel session using this AuthSession by calling
`RemoteChannel.setSession()` in `src/remote-device/remote-channel.ts:46-52`. The method
logs `has refresh_token: false` (line 48) and calls `this.client.auth.setSession({
access_token: session.access_token, refresh_token: session.refresh_token || '' })`,
establishing a Supabase client session where the refresh token is effectively
missing/empty.
3. Trigger a normal device shutdown by calling `Device.shutdown()` in
`src/remote-device/device.ts:3-37` (for example, running the device process and initiating
shutdown). The shutdown sequence logs "→ Marking device offline..." at `device.ts:25-27`
and invokes `await this.remoteChannel.setOffline(this.deviceId);` at `device.ts:28`.
4. Inside `RemoteChannel.setOffline()` in `src/remote-device/remote-channel.ts:21-76`, the
code calls `this.client.auth.getSession()` (line 31) and only validates
`sessionData?.session?.access_token` (line 33), not `refresh_token`. It then spawns the
blocking update script with:
- `env.SUPABASE_ACCESS_TOKEN = sessionData.session.access_token` and
- `env.SUPABASE_REFRESH_TOKEN = sessionData.session.refresh_token || ''` at line 453,
which evaluates to an empty string when the session's refresh_token is null.
The child script `src/remote-device/scripts/blocking-offline-update.js` reads these env
vars at lines 18-20 and enforces `if (!deviceId || !supabaseUrl || !supabaseKey ||
!accessToken || !refreshToken)` at line 22. An empty string `refreshToken` is falsy, so
the script prints "❌ Missing required arguments" and exits with code 1 (line 25). Back
in `setOffline()`, the parent sees `result.status === 1` and falls into the generic
error branch at `remote-channel.ts:19-20` (offset 459-520 section), logging `❌ Update
process failed with exit code: 1` but not updating `mcp_devices.status` to 'offline'.
Thus, any shutdown executed with a session lacking a refresh token deterministically
fails to mark the device offline.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/remote-device/remote-channel.ts
**Line:** 453:453
**Comment:**
*Logic Error: `refresh_token` is nullable in this codebase, but this line coerces missing values to an empty string and still spawns the offline-update subprocess. The child script treats an empty refresh token as missing and exits with code 1, so device shutdown can silently fail to mark the device offline whenever the session lacks a refresh token. Validate `sessionData.session.refresh_token` before spawning and skip/fallback explicitly instead of passing `''`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished reviewing your PR. |
|
Thanks both for the careful review — addressed in cd33546. @CodeAnt-AI (remote-channel.ts L453, nullable refresh_token): Good catch. The previous @coderabbitai (test-cwe200-credential-exposure.js L46-54, L83-88): Agreed — the line-by-line check would miss multi-line destructures and camelCase renames. Tightened both blocks:
All three tests still pass locally ( |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me verify the changes in the updated test file. 🏁 Script executed: Length of output: 4679
Both concerns are properly addressed. The fix looks good to me! (∩˃o˂∩)♡ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/test-cwe200-credential-exposure.js (1)
41-112: ⚡ Quick winReplace the regex scan with a structural assertion.
This test only looks for specific token names in source text, so a refactor that leaks secrets through generic positional
argvaccess could still pass. The raw-text parsing is also brittle around valid JS/TS syntax.For a CWE-200 regression, verify the actual
spawnSyncargv/env shape via AST inspection or a focused stubbed execution test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test-cwe200-credential-exposure.js` around lines 41 - 112, The current tests (especially the first and third in test-cwe200-credential-exposure.js) rely on fragile regex/text scans (tokenRe, statements split, and ad-hoc spawnSync parsing) which can be bypassed by positional argv usage; replace these checks with a structural assertion: parse SCRIPT_PATH and CHANNEL_PATH with a JS/TS parser (e.g., acorn/@babel/parser) and assert AST nodes that read process.argv do not reference identifier names matching access/refresh tokens and that spawnSync calls pass tokens via the options.env object (not in the second argument array). Update the tests that reference process.argv, tokenRe, statements, spawnSyncIdx, argsArrayMatch and optionsMatch to use AST traversal (or a focused stubbed spawnSync execution) to verify argv vs env shapes instead of raw regex matching.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/test-cwe200-credential-exposure.js`:
- Around line 41-112: The current tests (especially the first and third in
test-cwe200-credential-exposure.js) rely on fragile regex/text scans (tokenRe,
statements split, and ad-hoc spawnSync parsing) which can be bypassed by
positional argv usage; replace these checks with a structural assertion: parse
SCRIPT_PATH and CHANNEL_PATH with a JS/TS parser (e.g., acorn/@babel/parser) and
assert AST nodes that read process.argv do not reference identifier names
matching access/refresh tokens and that spawnSync calls pass tokens via the
options.env object (not in the second argument array). Update the tests that
reference process.argv, tokenRe, statements, spawnSyncIdx, argsArrayMatch and
optionsMatch to use AST traversal (or a focused stubbed spawnSync execution) to
verify argv vs env shapes instead of raw regex matching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 337a3f4e-e76d-48e9-a5aa-99d9f543956f
📒 Files selected for processing (2)
src/remote-device/remote-channel.tstest/test-cwe200-credential-exposure.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/remote-device/remote-channel.ts
1 participant
User description
Summary
This PR fixes a sensitive information disclosure (CWE-200 / CWE-214) where Supabase session
access_tokenandrefresh_tokenare passed as command-line arguments to a Node.js subprocess during the device-offline shutdown flow. Process arguments are world-readable on Unix systems (viaps auxand/proc/<pid>/cmdline), so any local user on the same host can capture these tokens while the subprocess is alive.Affected code
src/remote-device/remote-channel.ts—spawnSync('node', [...args])call around line 442 placedsessionData.session.access_tokenandsessionData.session.refresh_tokeninto theargvarray.src/remote-device/scripts/blocking-offline-update.js— read those tokens back fromprocess.argv.The subprocess is launched during the device shutdown handler, so the exposure window is short but deterministic (it occurs every shutdown).
Vulnerability details
Data flow
RemoteChannel.markDeviceOffline()retrieves a Supabase session containing the user'saccess_tokenandrefresh_token.argsarray ofspawnSync('node', [scriptPath, deviceId, supabaseUrl, supabaseKey, accessToken, refreshToken], ...).ps aux/ps -efon any Unix system/proc/<pid>/cmdline, which on default Linux configurations is readable by any local user (mode0444)execvearguments/proc/<pid>/environis restricted to the same UID (mode0400), so passing the tokens via theenvoption ofspawnSyncis not subject to the same disclosure.Impact — A captured
access_tokenallows an attacker to authenticate to the Supabase backend as the affected user; therefresh_tokenextends that access window.Preconditions
/procorps. On a busy system a tight polling loop can reliably catch short-lived processes.The
deviceId,supabaseUrl, andsupabaseKey(which is the public anon key) remain in argv — these are not session secrets and are safe to expose.Fix
Move the two tokens out of the
argvarray and into theenvoption ofspawnSync. The script now reads them fromprocess.env.SUPABASE_ACCESS_TOKENandprocess.env.SUPABASE_REFRESH_TOKEN.const result = spawnSync('node', [ scriptPath, deviceId, supabaseUrl, - supabaseKey, - sessionData.session.access_token, - sessionData.session.refresh_token || '' + supabaseKey ], { timeout: 3000, stdio: 'pipe', - encoding: 'utf-8' + encoding: 'utf-8', + env: { + ...process.env, + SUPABASE_ACCESS_TOKEN: sessionData.session.access_token, + SUPABASE_REFRESH_TOKEN: sessionData.session.refresh_token || '' + } });This is the canonical remediation for CWE-214 (sensitive information in process arguments) — environment variables are scoped per-process and not visible across UID boundaries on Unix.
Tests
A regression test was added at
test/test-cwe200-credential-exposure.jsthat statically verifies:blocking-offline-update.jsdoes not destructureaccessToken/refreshTokenfromprocess.argv.process.env.remote-channel.ts'sspawnSyncargv array contains neither token, and the options object includes them underenv.Result locally:
The change is the only call site of this script (verified via
grep), so no other invocations needed updating. The script's argument-count validation still rejects missing tokens with a clear usage message.Adversarial review
Before submitting, we tried to talk ourselves out of this finding. The main counter-arguments we considered: (1) maybe
/prochardening (hidepid=2) blocks the read — but that's not the default on any major distro, andpsfrom the same user always works regardless; (2) maybe the subprocess is too short-lived to race — but the 3-secondtimeoutand synchronous network call to Supabase keep it alive long enough for trivial polling; (3) maybe the tokens aren't sensitive — they are full Supabase session credentials, not the public anon key. None of those mitigations hold, so the disclosure is real for any multi-tenant host running DesktopCommander.cc @lewiswigmore
CodeAnt-AI Description
Keep sensitive device tokens out of process listings during offline shutdown
What Changed
psor/proc/<pid>/cmdline.Impact
✅ Lower risk of token exposure on shared machines✅ Safer offline shutdown flow✅ Clearer credential handling during device updates🔄 Retrigger CodeAnt AI Review
Details
💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests