fix: security audit — plugin RCE, CORS, token leak, shell exec, login rate limit - #1106
fix: security audit — plugin RCE, CORS, token leak, shell exec, login rate limit#1106wjc2821296948 wants to merge 5 commits into
Conversation
`installPluginFromGit` and `updatePluginFromGit` cloned a remote Git repository and ran `npm run build` whenever the package.json declared a build script. Build scripts execute arbitrary code with the server process's privileges, so any party able to supply a plugin URL (e.g. an authenticated user tricked into pasting a malicious URL, or a compromised auth token) gained remote code execution on the CloudCLI host. The build script is now opt-in: the caller must pass `allowBuild: true` to the install/update service after manually inspecting the build command. The HTTP `POST /api/plugins/install` and `POST /api/plugins/<name>/update` endpoints accept an explicit `allowBuild: true` in the JSON body for that purpose. A process-wide escape hatch (`setAllowPluginBuildScript`) is exposed for tests. Co-authored-by: cgsdn <chaogeshuodiannao@users.noreply.github.com>
`startCloneProject` embeds the user-supplied GitHub personal access token into the clone URL (https://<token>@host/...) and streams `git`'s stdout/stderr straight into the SSE `clone-progress` feed via `onProgress`. `git` echoes the full clone URL in its progress output, so every progress event leaks the token to whoever is watching the feed (which includes the user, but is also captured in any server-side logs that subscribe to the same stream). Run every stdout/stderr line through `sanitizeGitError` before relaying as progress. The function already replaces the token string with `***`; the only behavior change is that the sanitized text is what the SSE consumer sees during the clone (and not only after the clone fails). Co-authored-by: cgsdn <chaogeshuodiannao@users.noreply.github.com>
`app.use(cors({ exposedHeaders: [...] }))` invoked the `cors` package with no
`origin` option, so the package reflected the request's `Origin` header back
unchanged in `Access-Control-Allow-Origin` for every cross-origin request.
Combined with the fact that most `/api` routes are only protected by a
bearer JWT that the client keeps in localStorage, any malicious site a
victim visits in the same browser could read responses from the server on
the victim's behalf by issuing requests with the victim's token.
Replace the default reflector with a callback that only allows the origin
through when its host:port matches the server's own host:port. Same-origin
requests (no Origin header) continue to be allowed through. Wildcard binds
(0.0.0.0/::) accept any host on the configured port, which preserves the
LAN-hosted use case while still refusing unrelated public origins.
Move `SERVER_PORT` / `HOST` / `DISPLAY_HOST` / `VITE_PORT` declarations above
the CORS middleware so the reflector can read them at module load time.
Co-authored-by: cgsdn <chaogeshuodiannao@users.noreply.github.com>
`runShellCommand` invoked `spawn('sh', ['-c', commandString], ...)`, passing
the entire command as a single shell string. The current templates are all
literals, but the call shape is a footgun: any future change that splices
`appRoot`, `homeDirectory`, an environment variable, or any operator-controlled
string into the template becomes a classic shell command injection, with the
server process's privileges. A poisoned `$PATH` would already be enough to
substitute a malicious `npm`/`git` binary into the call.
Split the executor into (command, args) argv arrays and disable the shell.
The git workflow still legitimately chains three commands, so it falls back
to `sh -c` with a fully literal argument string (no string concatenation
with external values) — every other path now spawns the executable
directly with `shell: false`.
Update the service to plan each branch as `{ command, args }` and update the
existing service tests to match the new argv signature.
Co-authored-by: cgsdn <chaogeshuodiannao@users.noreply.github.com>
The `/api/auth/login` and `/api/auth/register` endpoints have no protection against credential stuffing or password spraying. An attacker who can reach the server (default bind `0.0.0.0:3001`) can run an unbounded number of guesses per second from a single IP. Bcrypt with 12 rounds makes each guess slow but does not make online brute force infeasible — over a long enough window any 8-character password falls. Add a per-client sliding-window rate limiter that defaults to 10 attempts per minute and a 60-second lockout window once the cap is hit. The limiter keys on the TCP peer address (or the first `X-Forwarded-For` entry when behind a reverse proxy), so it scales to single-user self-hosted installs without needing a shared store. Successful and failed attempts both consume a slot; the limiter does not let a misbehaving client extend a lockout by retrying. Cover the limiter with a focused unit test that verifies the under-cap, over-cap, lockout-no-extend, rolling-window, and per-client-key behaviors. Co-authored-by: cgsdn <chaogeshuodiannao@users.noreply.github.com>
📝 WalkthroughWalkthroughThe server now validates CORS origins, rate-limits authentication, controls plugin build scripts, sanitizes Git clone output, and executes system update commands with separate arguments. ChangesServer origin validation
Authentication rate limiting
Plugin build policy
Clone output sanitization
System command execution
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/modules/auth/rate-limit.middleware.ts`:
- Around line 93-95: Update the lockout calculation in the rate-limit handling
around record.timestamps and blockedUntil so blockedUntil is at least the
earliest retained timestamp plus windowMs, while preserving the existing
lockoutMs-based delay when it is later. Keep retryAfterSeconds derived from the
final blockedUntil value so Retry-After reflects the next permitted request.
In `@server/modules/auth/tests/rate-limit.middleware.test.ts`:
- Around line 115-122: Update the timing assertion in the rate-limit test around
limiter.middleware to advance time beyond one second instead of 500 ms, then
assert the resulting Retry-After header reflects the preserved lockout rather
than a reset two-second window. Keep the existing 429 status assertion and
verify the updated header value discriminates between the two behaviors.
In `@server/modules/plugins/plugins.service.ts`:
- Around line 116-121: Update update() so dependencies.update() stages and
validates the candidate before modifying the live plugin directory or stopping a
running server; only after successful validation should the current plugin be
replaced and restarted as needed. Ensure a rejected build leaves both the live
directory and running server unchanged, and add coverage for a running plugin
with a build script updated without allowBuild: true.
In `@server/modules/projects/services/project-clone.service.ts`:
- Around line 246-251: Update the clone progress handlers around
sanitizeGitError so credential redaction remains effective when stdout or stderr
data chunks split a token across events. Maintain per-stream carry-over state,
redact only complete available content while retaining a possible token prefix,
and flush any remaining buffered content when each stream closes; add a
regression test that emits a token across two data events and verifies the
concatenated SSE progress output contains no credential fragments.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 18e2a222-ac6b-4c46-b194-7483369fbafe
📒 Files selected for processing (11)
server/index.tsserver/modules/auth/auth.routes.tsserver/modules/auth/rate-limit.middleware.tsserver/modules/auth/tests/rate-limit.middleware.test.tsserver/modules/plugins/plugin-registry.service.tsserver/modules/plugins/plugins.routes.tsserver/modules/plugins/plugins.service.tsserver/modules/projects/services/project-clone.service.tsserver/modules/system/system.module.tsserver/modules/system/system.service.tsserver/modules/system/tests/system.service.test.ts
| if (record.timestamps.length >= maxAttempts) { | ||
| record.blockedUntil = now + lockoutMs; | ||
| const retryAfterSeconds = Math.max(1, Math.ceil(lockoutMs / 1000)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make Retry-After match the next permitted request.
If lockoutMs is shorter than windowMs, the request after blockedUntil still sees maxAttempts retained timestamps. The limiter starts another lockout, although the prior response told the client to retry. Set blockedUntil no earlier than the earliest retained timestamp plus windowMs.
Proposed fix
- record.blockedUntil = now + lockoutMs;
- const retryAfterSeconds = Math.max(1, Math.ceil(lockoutMs / 1000));
+ const nextAvailableAt = Math.max(
+ now + lockoutMs,
+ record.timestamps[0]! + windowMs,
+ );
+ record.blockedUntil = nextAvailableAt;
+ const retryAfterSeconds = Math.max(1, Math.ceil((nextAvailableAt - now) / 1000));📝 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.
| if (record.timestamps.length >= maxAttempts) { | |
| record.blockedUntil = now + lockoutMs; | |
| const retryAfterSeconds = Math.max(1, Math.ceil(lockoutMs / 1000)); | |
| if (record.timestamps.length >= maxAttempts) { | |
| const nextAvailableAt = Math.max( | |
| now + lockoutMs, | |
| record.timestamps[0]! + windowMs, | |
| ); | |
| record.blockedUntil = nextAvailableAt; | |
| const retryAfterSeconds = Math.max(1, Math.ceil((nextAvailableAt - now) / 1000)); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/modules/auth/rate-limit.middleware.ts` around lines 93 - 95, Update
the lockout calculation in the rate-limit handling around record.timestamps and
blockedUntil so blockedUntil is at least the earliest retained timestamp plus
windowMs, while preserving the existing lockoutMs-based delay when it is later.
Keep retryAfterSeconds derived from the final blockedUntil value so Retry-After
reflects the next permitted request.
| // Advance time by half a second — still inside the lockout — and verify the | ||
| // block window does NOT extend (would happen if we kept consuming slots). | ||
| currentTime += 500; | ||
| limiter.middleware(createMockRequest('203.0.113.3') as never, response as never, next); | ||
|
|
||
| assert.equal(firstBlockEnd, '2'); | ||
| assert.equal(response.statusCode, 429); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the lockout-extension assertion discriminate.
At 500 ms, both a preserved lockout and a reset 2-second lockout return Retry-After: 2. The test can pass when the lockout extends. Advance past one second and assert the updated header.
Proposed fix
- currentTime += 500;
+ currentTime += 1_100;
limiter.middleware(createMockRequest('203.0.113.3') as never, response as never, next);
assert.equal(firstBlockEnd, '2');
assert.equal(response.statusCode, 429);
+ assert.equal(response.headers['Retry-After'], '1');📝 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.
| // Advance time by half a second — still inside the lockout — and verify the | |
| // block window does NOT extend (would happen if we kept consuming slots). | |
| currentTime += 500; | |
| limiter.middleware(createMockRequest('203.0.113.3') as never, response as never, next); | |
| assert.equal(firstBlockEnd, '2'); | |
| assert.equal(response.statusCode, 429); | |
| }); | |
| // Advance time by half a second — still inside the lockout — and verify the | |
| // block window does NOT extend (would happen if we kept consuming slots). | |
| currentTime += 1_100; | |
| limiter.middleware(createMockRequest('203.0.113.3') as never, response as never, next); | |
| assert.equal(firstBlockEnd, '2'); | |
| assert.equal(response.statusCode, 429); | |
| assert.equal(response.headers['Retry-After'], '1'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/modules/auth/tests/rate-limit.middleware.test.ts` around lines 115 -
122, Update the timing assertion in the rate-limit test around
limiter.middleware to advance time beyond one second instead of 500 ms, then
assert the resulting Retry-After header reflects the preserved lockout rather
than a reset two-second window. Keep the existing 429 status assertion and
verify the updated header value discriminates between the two behaviors.
| async update(pluginName: string, options?: { allowBuild?: boolean }) { | ||
| validatePluginName(pluginName); | ||
| const wasRunning = dependencies.isServerRunning(pluginName); | ||
| if (wasRunning) await dependencies.stopServer(pluginName); | ||
| const plugin = normalizePluginManifest(await dependencies.update(pluginName)); | ||
| const plugin = normalizePluginManifest(await dependencies.update(pluginName, options)); | ||
| if (wasRunning) await startServerIfAvailable(plugin); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Keep the current plugin available when the build policy rejects an update.
If wasRunning is true, Line 119 stops the plugin server. dependencies.update() can now reject after git pull and npm install when runBuildIfNeeded denies a build. The rejection bypasses Line 121, so the plugin remains stopped. The live plugin directory can also contain the rejected update.
Stage and validate updates before replacing the live directory and stopping its server. Add a test for a running plugin whose update declares a build script without allowBuild: true.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/modules/plugins/plugins.service.ts` around lines 116 - 121, Update
update() so dependencies.update() stages and validates the candidate before
modifying the live plugin directory or stopping a running server; only after
successful validation should the current plugin be replaced and restarted as
needed. Ensure a rejected build leaves both the live directory and running
server unchanged, and add coverage for a running plugin with a build script
updated without allowBuild: true.
| if (!message) return; | ||
| // `git` echoes the clone URL (with the embedded auth token) in progress | ||
| // messages. Always sanitize before forwarding to the SSE stream so the | ||
| // token is not exposed to anyone watching the clone-progress feed. | ||
| handlers.onProgress(sanitizeGitError(message, githubToken)); | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## locate backend standards and target files"
if [ -f .agents/skills/backend-module-standards/SKILL.md ]; then
echo "--- backend-module-standards ---"
sed -n '1,220p' .agents/skills/backend-module-standards/SKILL.md
else
echo "backend-module-standards/SKILL.md not found"
fi
echo "--- target file outline/stat ---"
wc -l server/modules/projects/services/project-clone.service.ts
ast-grep outline server/modules/projects/services/project-clone.service.ts --match sanitizeGitError --view expanded || true
ast-grep outline server/modules/projects/services/project-clone.service.ts --match startCloneProject --view expanded || true
echo "--- relevant service sections ---"
sed -n '1,120p' server/modules/projects/services/project-clone.service.ts
sed -n '180,270p' server/modules/projects/services/project-clone.service.ts
sed -n '270,340p' server/modules/projects/services/project-clone.service.ts
echo "--- sanitize function references ---"
rg -n "function sanitizeGitError|const sanitizeGitError|export .*sanitizeGitError|sanitizeGitError" server -SRepository: siteboon/claudecodeui
Length of output: 14062
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
# Read-only behavioral probe: demonstrate how splitting a secret across Node text stream chunks
# before applying the existing exact-token replace can leave fragments unreplaced.
token = "ghp_1234567890abcdefghijklmnopqrstuvwxyz12345"
chunks = ["Some git output with ghp_1234567890abc", "defghijklmnopqrstuvwxyz12345 in the middle."]
def sanitizeGitError(message, githubToken):
return message.replace(githubToken, "<token>")
received = ""
for chunk in chunks:
received += sanitizeGitError(chunk, token)
print("concatenated_after_sanitize=", received)
print("contains_full_token=", token in received)
print("unredacted_fragment_count=", received.count("ghp_1") + received.count("abcdefghijklmnopqrstuvwxyz12345"))
PYRepository: siteboon/claudecodeui
Length of output: 325
Sensitive Data Exposure (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor
Reachability: External
Preserve credential redaction across stream chunks.
stdout and stderr data events are arbitrary chunks, not complete Git messages. sanitizeGitError replaces only exact token matches, so a credential split across two events is never matched and both fragments reach the clone-progress SSE stream.
Use a per-stream carry-over buffer or streaming redactor, and flush the final partial message when the stream closes. Add a regression test that splits one token across two data events and checks the concatenated progress output.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/modules/projects/services/project-clone.service.ts` around lines 246 -
251, Update the clone progress handlers around sanitizeGitError so credential
redaction remains effective when stdout or stderr data chunks split a token
across events. Maintain per-stream carry-over state, redact only complete
available content while retaining a possible token prefix, and flush any
remaining buffered content when each stream closes; add a regression test that
emits a token across two data events and verifies the concatenated SSE progress
output contains no credential fragments.
P0 — Plugin install executes
|
P1 — GitHub personal access token leaks via the clone-progress SSE streamVulnerability description
// server/modules/projects/services/project-clone.service.ts (before)
gitProcess.stdout?.on('data', (data: Buffer | string) => {
const message = data.toString().trim();
if (message) {
handlers.onProgress(message); // ← unfiltered, may contain https://<token>@host/...
}
});
gitProcess.stderr?.on('data', (data: Buffer | string) => {
const message = data.toString().trim();
lastError = message;
if (message) {
handlers.onProgress(message); // ← same leak on stderr
}
});Note: Fix approachRun every stdout/stderr line through // server/modules/projects/services/project-clone.service.ts (after)
gitProcess.stdout?.on('data', (data: Buffer | string) => {
const message = data.toString().trim();
if (!message) return;
// `git` echoes the clone URL (with the embedded auth token) in progress
// messages. Always sanitize before forwarding to the SSE stream so the
// token is not exposed to anyone watching the clone-progress feed.
handlers.onProgress(sanitizeGitError(message, githubToken));
});
gitProcess.stderr?.on('data', (data: Buffer | string) => {
const message = data.toString().trim();
lastError = message;
if (!message) return;
// Same token-leak risk on stderr. Sanitize before relaying as progress.
handlers.onProgress(sanitizeGitError(message, githubToken));
});Referenced code
|
P1 — CORS reflects any Origin headerVulnerability description
// server/index.ts (before)
// Reflects any Origin header back. With JSON + bearer-token auth this is
// safe against classic CSRF (the attacker cannot read the response) but
// fails open once any XSS lands in the host page, and it makes every
// authenticated endpoint reachable from any origin in the browser.
app.use(cors({ exposedHeaders: ['X-Refreshed-Token', 'X-Auth-Error'] }));Fix approachReplace the default reflector with a callback that only allows the origin through when its // server/index.ts (after)
const corsOriginReflector = (
origin: string | undefined,
callback: (err: Error | null, allow?: boolean) => void,
) => {
// No Origin header → same-origin request (e.g. server-to-server, curl);
// these are not subject to CORS and should always be allowed through.
if (!origin) {
callback(null, true);
return;
}
try {
const parsed = new URL(origin);
const requestHost = parsed.hostname;
const requestPort = parsed.port || (parsed.protocol === 'https:' ? '443' : '80');
const serverHost = HOST === '0.0.0.0' || HOST === '::' ? requestHost : HOST;
const serverPort = String(SERVER_PORT);
if (requestHost === serverHost && requestPort === serverPort) {
callback(null, true);
return;
}
} catch {
// Malformed Origin header — refuse.
}
callback(null, false);
};
app.use(cors({
origin: corsOriginReflector,
exposedHeaders: ['X-Refreshed-Token', 'X-Auth-Error'],
}));
Referenced code
|
P2 — System update spawns commands through
|
P2 —
|
1 participant
Summary
This PR fixes five security issues found by a manual security audit of the CloudCLI server codebase. Each commit is a self-contained fix and ships with its own PR comment below describing the issue, the failure scenario, and the resolution.
Findings
P0 — Plugin install executes
npm run buildon attacker-controlled repositoriesserver/modules/plugins/plugin-registry.service.tscloned an arbitrary Git URL into a temp directory and rannpm run buildwheneverpackage.jsondeclared a build script.--ignore-scriptsblockspostinstallhooks but does not covernpm run build. Any party able to supply a plugin URL — including a user tricked into pasting one, or a leaked auth token — gained remote code execution on the CloudCLI host.Fix: the build script now requires explicit opt-in (
allowBuild: true) on the install/update call, after the operator has manually inspected the script. The HTTP routes accept anallowBuildfield in the JSON body. A process-wide escape hatch (setAllowPluginBuildScript) is exposed for tests. Seefix(plugins): disable auto-running npm run build during plugin install.P1 — GitHub personal access token leaks via the clone-progress SSE stream
server/modules/projects/services/project-clone.service.tsembedded the supplied GitHub PAT into the clone URL (https://<token>@host/...) and forwardedgit's stdout/stderr verbatim to the SSEclone-progressfeed.gitechoes the full URL in progress output, so the token leaked through every progress event.Fix: run every stdout/stderr line through
sanitizeGitErrorbefore relaying as progress. The function already replaces the token string with***; the only behavior change is that the sanitized text is what the SSE consumer sees during the clone (and not only after the clone fails). Seefix(projects): sanitize GitHub tokens from clone progress stream.P1 — CORS reflects any Origin header
app.use(cors({ exposedHeaders: [...] }))was invoked with nooriginoption, so thecorspackage reflected the request'sOriginheader back unchanged inAccess-Control-Allow-Originfor every cross-origin request. Combined with the fact that/apiroutes are protected by a bearer JWT that the client keeps in localStorage, any malicious site a victim visits could read responses from the server on the victim's behalf.Fix: replace the default reflector with a callback that only allows the origin through when its host:port matches the server's own host:port. Same-origin requests (no Origin header) continue to be allowed through. Wildcard binds (
0.0.0.0/::) accept any host on the configured port, preserving the LAN-hosted use case while still refusing unrelated public origins. Seefix(server): restrict CORS to same host:port as the server.P2 — System update spawns commands through
sh -cserver/modules/system/system.module.tsinvokedspawn('sh', ['-c', commandString], ...)for the in-app update workflow. The current templates are all literals, but the call shape is a footgun: any future change that splicesappRoot,homeDirectory, an environment variable, or any operator-controlled string into the template becomes a classic shell command injection. A poisoned$PATHalready substitutes a maliciousnpm/gitbinary.Fix: split the executor into
(command, args)argv arrays withshell: false. The git workflow legitimately chains three commands, so it still usessh -cwith a fully literal argument string — every other path now spawns the executable directly with no shell at all. Tests are updated to match the new argv signature. Seefix(system): spawn update commands without a shell.P2 —
/api/auth/loginand/api/auth/registerhave no rate limitingThe auth endpoints have no protection against credential stuffing or password spraying. An attacker who can reach the server (default bind
0.0.0.0:3001) can run an unbounded number of guesses per second from a single IP.Fix: add a per-client sliding-window rate limiter (
server/modules/auth/rate-limit.middleware.ts) that defaults to 10 attempts per minute and a 60-second lockout window once the cap is hit. The limiter keys on the TCP peer address (or the firstX-Forwarded-Forentry when behind a reverse proxy), so it scales to single-user self-hosted installs without needing a shared store. Successful and failed attempts both consume a slot; the limiter does not let a misbehaving client extend a lockout by retrying. Covered by a focused unit test. Seefix(auth): rate-limit login and registration per client.Test plan
npm run typecheck(pending — repo has nonode_modulesin this checkout)npm run testnpm run build🤖 Generated with Claude Code
Summary by CodeRabbit
Security
Bug Fixes
Tests