Apply upstream security hardening (parent PR #29) + cross-platform fixes - #2
Conversation
- C2: Replace exec() with execFile() in reveal.post.ts to prevent command injection - C3: Add path restriction to files.get.ts — block arbitrary file reads outside ~/.claude - C4: Restrict directories.get.ts to home directory and ~/.claude - C5: Default SDK permission mode to 'default' instead of 'bypassPermissions' - C6: Add DOMPurify to sanitize all markdown v-html output, fix XSS amplifier in decodeHTMLEntities - C7: Validate MCP import config schema — require command/url, whitelist fields, validate URLs - H1: Add path traversal protection to agent slugs via safePath utility - H3: Remove PATH environment variable leak from debug endpoint - H9: Add SIGTERM/SIGINT handlers for PTY session cleanup Adds server/utils/path-security.ts with safePath(), safeClaudePath(), isUnderAllowedPath(). Adds vitest test suite: 37 tests (unit + E2E) verifying all security fixes.
Follow-up to the cherry-pick of upstream security PR Ngxba#29, fixing issues surfaced by running the security suite on Windows. - path-security.ts: replace hardcoded '/' separator containment checks in safePath and isUnderAllowedPath with a path.relative()-based isContained() helper. The old `startsWith(base + '/')` check always failed on Windows (resolve() returns backslash paths), wrongly rejecting valid in-bounds paths with a 403 in production. path.relative() is separator-agnostic and still rejects '..' traversal, absolute-segment escapes, and shared-prefix siblings (e.g. /a/baseX is not under /a/base). - reveal.post.ts: run the isUnderAllowedPath check before the existsSync 404 so out-of-bounds paths always return 403 regardless of existence — closes a path-enumeration side channel (attacker must not distinguish forbidden from not-found). - tests/security/path-security.test.ts: rewrite to import the REAL module functions (it previously reimplemented them locally, testing dead copies) and build paths from os.tmpdir()/os.homedir() so assertions pass on Windows and Linux. Adds tests/setup.ts stubbing the Nuxt createError global, wired via vitest.config.ts setupFiles. - api-security-e2e.test.ts: add shell:true to the npx spawn so it resolves npx.cmd on Windows instead of throwing ENOENT. Security suite: 47/47 passing. Build: clean. Refs #1 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f84d194095
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| // Security: restrict file access to allowed directories | ||
| if (!isUnderAllowedPath(fullPath, getAllowedPaths(projectDir))) { |
There was a problem hiding this comment.
When path is absolute or relative and the caller supplies a matching projectDir, this check adds that untrusted query value to the allowed roots. For example, GET /api/files?path=/etc/passwd&projectDir=/etc resolves under /etc and passes, so the endpoint still returns arbitrary files despite the new guard; only allow a server-validated/current project directory here.
Useful? React with 👍 / 👎.
| * Allows safe markdown-generated tags while stripping scripts and event handlers. | ||
| */ | ||
| function sanitizeHtml(html: string): string { | ||
| if (typeof window === 'undefined') return html // SSR: no DOM available |
There was a problem hiding this comment.
With Nuxt SSR enabled, this branch returns the raw marked HTML for any v-html render that happens on the server. If a persisted/workflow/chat message containing raw HTML such as an image with an event handler is present in the initial render, the browser parses and can execute it before client-side DOMPurify ever runs, so the XSS hardening is bypassed for SSR paths.
Useful? React with 👍 / 👎.
| cwd: process.cwd(), | ||
| stdio: 'pipe', | ||
| shell: true, | ||
| env: { ...process.env, NODE_ENV: 'development' }, |
There was a problem hiding this comment.
Because the spawned Nuxt server inherits the developer's real environment here, the MCP import tests below call /api/mcp/import against the real ~/.claude.json and add test-safe/test-valid-url entries with no cleanup whenever npm run test:e2e is run. Start the server with a temporary HOME/CLAUDE_DIR or restore the file so the security suite doesn't mutate users' Claude config.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| process.on('beforeExit', cleanupAllSessions) | ||
| process.on('SIGTERM', () => { cleanupAllSessions(); process.exit(0) }) |
There was a problem hiding this comment.
In the SIGTERM/SIGINT paths, cleanupAllSessions() only starts terminateSession() promises and then process.exit(0) is called immediately, so async watcher closure and saveSessionHistory() usually won't run for active CLI sessions. Make the signal handler async and await the terminations before exiting to avoid losing session history on normal server shutdown.
Useful? React with 👍 / 👎.
cbeaulieu-gt
left a comment
cbeaulieu-gt
left a comment
There was a problem hiding this comment.
Note: posted as a comment review — GitHub does not allow
REQUEST_CHANGESon your own PR. Intended event: REQUEST_CHANGES (one maintainability item to address before merge; no security bugs).
Review: Apply upstream security hardening (PR Ngxba#29) + cross-platform fixes
Summary
This PR cherry-picks 6 critical and 3 high security vulnerabilities from upstream PR Ngxba#29, then applies Windows cross-platform fixes. All 47 tests pass; the rewritten tests correctly import the real module and use cross-platform paths. Security fixes are comprehensive and path-enumeration side-channels are properly closed.
Findings
✅ Path Security Logic — Correct and Robust
File: server/utils/path-security.ts lines 10–14
The isContained() helper using path.relative() correctly handles all edge cases:
- Empty
rel(base itself) ✓ rel === '..'(one level up) ✓rel.startsWith('..' + sep)(multi-level traversal) ✓- Absolute
rel(cross-drive on Windows) ✓ - Shared-prefix siblings rejected (e.g.,
/a/baseXvs/a/base) ✓
The slug regex (line 61: /^[a-zA-Z0-9][-a-zA-Z0-9_]*$/) correctly requires alphanumeric start, rejecting leading hyphens by design.
Test coverage is excellent with cross-platform paths (no hardcoded separators) and comprehensive edge cases (prefix-confusion, .. escapes, absolute segments).
✅ Command Injection Fix — execFile(2) Correct
File: server/api/reveal.post.ts lines 32–44
Migration from exec() to execFile() prevents shell metacharacter injection:
- Platform detection and commands (open/explorer/xdg-open) hardcoded ✓
- Path passed as argument array, not interpolated into shell string ✓
- Path-enumeration side-channel closed: 403 check before
existsSync()on line 21 ✓
The DEP0190 deprecation warning is acceptable; execFile is the correct API.
✅ XSS Sanitization — Complete Coverage
Files: app/utils/markdown.ts, app/utils/messageFormatting.ts
- DOMPurify integration wraps all render outputs (async, sync, inline, with-math, with-highlighting) ✓
- Lang attribute escaping (line 50) prevents HTML attribute injection via
replace(/['"<>&]/g, '')✓ - innerHTML pattern removed from
decodeHTMLEntities()eliminates XSS amplifier ✓ - All six markdown rendering functions pass through
sanitizeHtml()with safe tag allowlist ✓
✅ MCP Import Validation — Comprehensive
File: server/api/mcp/import.post.ts lines 28–82
Server name validation (^[a-zA-Z0-9_-]+$) ✓
Command/URL presence enforcement ✓
URL protocol whitelist (http/https only) ✓
Args array-of-strings validation ✓
Env object validation (string→string) ✓
Allowlist-based field stripping (lines 76–81) — unknown fields deleted pre-merge ✓
Attacker cannot inject arbitrary config keys; all input is pre-validated.
⚠️ Redundant Slug Validation in decodeAgentSlug()
File: server/utils/agentUtils.ts lines 14–17
The function now rejects .., /, \ early, which is correct. However, this validation duplicates the regex check in path-security.validateSlug() (line 61 of that file).
Issue: It's unclear whether decodeAgentSlug() is a public API (responsible for its own validation) or a helper called only after upstream validateSlug() (making this check redundant).
Recommendation:
- Option A (Preferred): Add JSDoc to
decodeAgentSlug()clarifying that callers must pre-validate withvalidateSlug(), or that this function performs defensive checks for untrusted input. If the latter is intended, document it explicitly. - Option B: Move the slug regex check into
decodeAgentSlug()to eliminate external dependency, and remove the duplicate fromvalidateSlug().
This is a maintainability concern (duplicated security-critical logic in two places), not a correctness bug. Both checks are in place and correct.
💡 Suggestion: Extend Test Coverage
File: tests/security/path-security.test.ts
The suite covers safePath(), isUnderAllowedPath(), and validateSlug(), but does not test:
safeClaudePath()behavior (callssafePath()internally; integration test missing)getAllowedPaths()with optionalprojectDirparameter
Recommendation: Add 2–3 integration tests for safeClaudePath() traversal rejection and getAllowedPaths() multi-base inclusion. Current coverage is sufficient (functions are simple wrappers), but explicit tests would catch any future refactors.
✅ Verification
npm test→ 47/47 passing ✓npm run build→ clean ✓- Cross-platform paths use
os.tmpdir()andos.homedir()(no hardcoded separators) ✓ - Vitest setup stubs
createErrorglobal for production module import ✓ - DEP0190 warning in e2e spawn documented in PR description ✓
Recommendation
REQUEST_CHANGES (intent) — The slug validation duplication in agentUtils.ts needs clarification. Resolve by documenting whether decodeAgentSlug() is responsible for validating untrusted input or if callers must pre-validate with validateSlug(). This is a code-clarity issue (not a security bug), but important for future maintainers.
Once addressed, the PR is production-ready. All security fixes are correct, comprehensive, and well-tested.
🤖 Generated by Claude Code on behalf of @cbeaulieu-gt
…arden shutdown Codex review of PR #2 found four issues; all verified real and fixed. P1 — Arbitrary file read via untrusted projectDir (files.get.ts): getAllowedPaths(projectDir) trusted the attacker-controlled `projectDir` query param as an allowed root, so `?path=/etc/passwd&projectDir=/etc` bypassed the read guard — defeating the upstream fix. Dropped the projectDir parameter from getAllowedPaths (now returns [claudeDir] only) and removed the query-projectDir read in files.get.ts; relative paths now resolve under ~/.claude. reveal.post.ts already called getAllowedPaths() with no arg. P1 — SSR XSS bypass (markdown.ts): sanitizeHtml() returned raw HTML on the server branch (typeof window === 'undefined'), so under Nuxt SSR unsanitized markup shipped on first render before client DOMPurify ran. Switched to isomorphic-dompurify and removed the SSR early-return so sanitization is environment-independent. P2 — E2E mutated real ~/.claude.json (api-security-e2e.test.ts): The spawned dev server inherited the real env, so MCP-import tests wrote test entries into the developer's real config with no cleanup. Spawn now uses an isolated temp CLAUDE_DIR (mkdtemp in beforeAll, rm in afterAll). P2 — Session history lost on shutdown (cliSession.ts): SIGTERM/SIGINT called cleanupAllSessions() then process.exit(0) without awaiting, so async watcher close + saveSessionHistory() never ran. Made cleanupAllSessions await all terminations (Promise.allSettled) and the signal handlers async + awaited before exit. Adds regression tests for both P1s (projectDir-bypass contract in path-security.test.ts; environment-independent sanitization in xss-sanitization.test.ts). Suite: 58/58 passing. Build: clean. Refs #1 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed all four Codex findings in
Verification: Thanks @chatgpt-codex-connector — findings #1 and #2 were genuine bypasses of the hardening this PR adds. 🤖 Generated by Claude Code on behalf of @cbeaulieu-gt |
|
To use Codex here, create an environment for this repo. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2fbab1e51
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| // Security: restrict file access to allowed directories (claudeDir only) | ||
| if (!isUnderAllowedPath(fullPath, getAllowedPaths())) { |
There was a problem hiding this comment.
When a user opens a Read/Edit result or a git-panel file from the selected working directory, the frontend still calls /api/files with that project file path and projectDir (for example in FileEditorSidebarContent.vue). This new guard only compares against getAllowedPaths(), which now contains just the Claude config dir, so ordinary files under the current project resolve correctly but always return 403. Keep a server-validated current project root in the allowed set instead of dropping project reads entirely.
Useful? React with 👍 / 👎.
| const claudeFile = join(homedir(), '.claude', 'settings.json') | ||
| const res = await fetch(`${BASE}/api/files?path=${encodeURIComponent(claudeFile)}`) |
There was a problem hiding this comment.
The server is launched above with CLAUDE_DIR=isolatedClaudeDir, but this positive-control request still targets the test runner's real homedir()/.claude/settings.json. Since /api/files now authorizes only the server's getClaudeDir(), this assertion fails with 403 in the intended isolated E2E environment; create/read a file under isolatedClaudeDir for this check instead.
Useful? React with 👍 / 👎.
Codex re-reviewed d2fbab1 and found the projectDir fix over-corrected: authorizing /api/files against claudeDir-only made every legitimate project-file read (FileEditorSidebar*.vue → /api/files?path=<projectfile> &projectDir=<workingDir>) return 403. - path-security.ts: add exported getBrowsableRoots() = [homedir(), claudeDir] as the read/browse boundary (single source of truth). getAllowedPaths() stays claudeDir-only (reveal.post.ts depends on it). - files.get.ts: authorize against getBrowsableRoots() so files under the user's home or Claude dir are readable again; paths outside home still 403. Relative paths resolve under claudeDir only (no untrusted base). Added an isSensitiveHomeSubdir guard that blocks hidden dirs directly under home (~/.ssh, ~/.gnupg, ...) while exempting the Claude dir — defense-in-depth so widening to the home boundary does not expose credential dirs. 403 still precedes 404. - directories.get.ts: use the shared getBrowsableRoots (removed the local duplicate); behavior unchanged. - e2e: positive control now reads a file under the isolated temp CLAUDE_DIR instead of the runner's real ~/.claude (which the isolation made 403). Adds getBrowsableRoots boundary tests (home file allowed, system path blocked). Suite: 63/63 passing. Build: clean. Refs #1 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed the two follow-up findings from the re-review on
Added Good catch on both, @chatgpt-codex-connector — finding #1 was a real functional regression my change introduced. 🤖 Generated by Claude Code on behalf of @cbeaulieu-gt |
|
To use Codex here, create an environment for this repo. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f831b64562
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * Used by files.get and directories.get; reveal.post uses getAllowedPaths. | ||
| */ | ||
| export function getBrowsableRoots(): string[] { | ||
| return [homedir(), getClaudeDir()] |
There was a problem hiding this comment.
Because /api/files checks the caller-provided absolute path against this list, returning homedir() authorizes reads of any non-hidden file anywhere under the user's home. For example, GET /api/files?path=/home/alice/projects/other/.env passes the root check, and isSensitiveHomeSubdir does not block it because the first segment is projects; fresh evidence is that this revision now returns homedir(), so the bypass no longer relies on the rejected projectDir. Use the Claude dir plus a server-validated current project root rather than the entire home directory.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| // Strip any unexpected fields — only allow known MCP config keys | ||
| const allowedKeys = new Set(['command', 'args', 'env', 'url', 'type', 'headers']) |
There was a problem hiding this comment.
This whitelist deletes disabled, even though server/api/mcp/index.post.ts stores disabled: !!disabled and the MCP UI treats a missing flag as enabled. Importing a config that intentionally disabled a server therefore writes it back without the flag, causing that server to be active the next time the MCP config is loaded; include and validate disabled instead of stripping it.
Useful? React with 👍 / 👎.
mcp/import.post.ts stripped `disabled` via the allowlist, so importing a config that intentionally disabled a server wrote it back without the flag — re-enabling that server on next load (index.post.ts stores disabled:!!disabled and the UI treats a missing flag as enabled). - Add `disabled` to the import allowlist so it survives the strip. - Validate it is a boolean when present (400 otherwise), matching the file's existing per-field validation style. - Regression tests: import preserves disabled:true; non-boolean disabled → 400. Codex P1 on the same review (home-dir read breadth in files.get) is tracked as a deliberate follow-up in #3, not addressed here. Suite: 65/65 passing. Build: clean. Refs #1 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Re-review on
Suite: 65/65. Build clean. Merging this PR; the remaining read-scope hardening continues in #3. Thanks again @chatgpt-codex-connector. 🤖 Generated by Claude Code on behalf of @cbeaulieu-gt |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de27fe31d8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return 'plan' | ||
| default: | ||
| return 'bypassPermissions' // Default for chat v2 | ||
| return 'default' // Safe default — require explicit opt-in for bypassPermissions |
There was a problem hiding this comment.
When chat v2 requests omit permissionMode or use the UI default “Ask”, this now maps to default, but this same query path still auto-allows every tool in sdkOptions.allowedTools before emitting a permission_request (Write, Edit, and Bash are in that list). In that default/Ask scenario, shell and file-edit tools can still run without user approval, so this does not actually close the previous bypass for normal sessions; default mode should not pre-approve the dangerous tools.
Useful? React with 👍 / 👎.
| env: { | ||
| ...process.env, | ||
| NODE_ENV: 'development', | ||
| CLAUDE_DIR: isolatedClaudeDir, | ||
| }, |
There was a problem hiding this comment.
Setting only CLAUDE_DIR does not isolate the MCP import endpoint, because server/api/mcp/import.post.ts writes join(homedir(), '.claude.json'); the MCP tests therefore append test servers to the developer/CI account's real ~/.claude.json and can overwrite it if parsing fails. In the test server environment, point HOME/USERPROFILE at a temp directory or snapshot and restore ~/.claude.json in teardown.
Useful? React with 👍 / 👎.
1 participant
Summary
Applies the security hardening from parent repo
Ngxba/claude-code-cli-uiPR Ngxba#29 (still open/unmerged upstream) to this fork by cherry-picking its commit, then fixes cross-platform defects surfaced by running the security suite on Windows.Two commits:
b050ade— cherry-pick of upstream PR fix: harden security — patch 7 critical and 3 high vulnerabilities Ngxba/claude-code-cli-ui#29 (fix/security-hardening, all 18 files).f84d194— cross-platform + hardening follow-ups (see below).Vulnerabilities patched (from PR Ngxba#29)
exec()server/api/reveal.post.tsserver/api/files.get.tsserver/api/directories.get.tsbypassPermissionsSDK modeserver/utils/claudeSdk.ts,server/utils/providers/claudeProvider.tsv-htmlsinks without sanitizerapp/utils/markdown.ts,app/utils/messageFormatting.tsserver/api/mcp/import.post.tsserver/utils/agentUtils.tsprocess.env.PATHserver/api/debug/claude-cli.get.tsserver/utils/cliSession.tsAdds
server/utils/path-security.ts,dompurify(+ types), and avitestsecurity suite.Conflict resolution during cherry-pick
server/utils/providers/claudeProvider.ts— kept this fork's MCP transport/capability work and took PR fix: harden security — patch 7 critical and 3 high vulnerabilities Ngxba/claude-code-cli-ui#29'ssafeClaudePathimport (dropped the now-unusedgetClaudeDirimport).package-lock.json— regenerated from the mergedpackage.json.Cross-platform + hardening follow-ups (
f84d194)path-security.ts(real Windows production bug): the original containment checks usedstartsWith(base + '/').node:pathresolve()returns backslash paths on Windows, so valid in-bounds paths were wrongly rejected with 403. Replaced with apath.relative()-basedisContained()helper — separator-agnostic, still rejects..traversal, absolute-segment escapes, and shared-prefix siblings.reveal.post.ts: moved theisUnderAllowedPathcheck before theexistsSync404 so out-of-bounds paths always return 403 regardless of existence — closes a path-enumeration side channel.path-security.test.ts: the upstream test reimplemented the functions locally (testing dead copies) and used POSIX path literals. Rewrote it to import the real module and build paths fromos.tmpdir()/os.homedir()so it passes on Windows and Linux. Addedtests/setup.tsstubbing the NuxtcreateErrorglobal, wired viavitest.config.ts.api-security-e2e.test.ts: addedshell: trueto thenpxspawn (resolvesnpx.cmdon Windows instead of throwingENOENT).Verification
npm test→ 47/47 passing (3 files: path-security 20, xss-sanitization 6, api-security-e2e 21).npm run build→ clean.npm run typechecknot run —vue-tscis not a project dependency (pre-existing; not introduced here).Notes / follow-ups
bun.lockbis not updated for the new deps (dompurifyetc.) — bun is not installed in this environment and upstream PR fix: harden security — patch 7 critical and 3 high vulnerabilities Ngxba/claude-code-cli-ui#29 only maintainspackage-lock.json. If this fork builds via bun, runbun installto refreshbun.lockb.shell: truee2e spawn emits a NodeDEP0190warning; harmless here since the spawn args are hardcoded (no user input / injection surface).Closes #1
🤖 Generated by Claude Code on behalf of @cbeaulieu-gt