fix: security audit — plugin RCE, CORS, token leak, shell exec, login rate limit - #1106

sequenceDiagram
  participant Client
  participant AuthRoutes
  participant RateLimiter
  participant AuthHandler
  Client->>AuthRoutes: Send registration or login request
  AuthRoutes->>RateLimiter: Check client attempt
  RateLimiter->>AuthHandler: Invoke next handler when allowed
  RateLimiter-->>Client: Return 429 with Retry-After when locked out

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 help to get the list of available commands.}

P0 — Plugin install executes npm run build on attacker-controlled repositories

Vulnerability description

installPluginFromGit(url) and updatePluginFromGit(name) in server/modules/plugins/plugin-registry.service.ts cloned an arbitrary Git URL into a temp directory and ran npm run build whenever the cloned package.json declared a build script. --ignore-scripts blocks postinstall hooks but does not cover npm run build, so the build step ran with the full privileges of the host Node process.

// server/modules/plugins/plugin-registry.service.ts (before)
// runBuildIfNeeded checked only for the existence of a build script,
// not for who authored it or what it contained.
const buildProcess = spawn('npm', ['run', 'build'], {
  cwd: dir,
  stdio: ['ignore', 'pipe', 'pipe'],
});

Any party able to supply a plugin URL — including an authenticated user tricked into pasting a hostile URL, or a leaked auth token — gained remote code execution on the CloudCLI host.

Fix approach

The build script is now opt-in. The runBuildIfNeeded helper takes a new options.allowBuild flag and a process-wide override (setAllowPluginBuildScript); if both are unset, a present build script causes the install/update to fail with a clear error. The HTTP routes POST /api/plugins/install and POST /api/plugins/<name>/update accept an explicit allowBuild: true in the JSON body for callers that have manually vetted the plugin's build command.

// server/modules/plugins/plugin-registry.service.ts (after)
if (!ALLOW_PLUGIN_BUILD_SCRIPT && !options?.allowBuild) {
  return onError(new Error(
    'Plugin declares a "build" script but plugin builds are disabled by default. ' +
    'Plugin build scripts run arbitrary code with the server process privileges. ' +
    'To install this plugin, ship a pre-built artifact and remove the build script, ' +
    'or set `allowBuild: true` after manually inspecting the build script.',
  ));
}

// server/modules/plugins/plugins.routes.ts (after)
router.post('/install', respond((req) => {
  const body = (req.body ?? {}) as { url?: unknown; allowBuild?: unknown };
  // Build scripts run arbitrary code with server privileges. Only opt in
  // when the operator has manually vetted the plugin's build script.
  const allowBuild = body.allowBuild === true;
  return service.install(body.url, { allowBuild });
}));

Referenced code

• Commit: 55d126e — fix(plugins): disable auto-running npm run build during plugin install
• Files: server/modules/plugins/plugin-registry.service.ts, server/modules/plugins/plugins.service.ts, server/modules/plugins/plugins.routes.ts

P1 — GitHub personal access token leaks via the clone-progress SSE stream

Vulnerability description

startCloneProject in server/modules/projects/services/project-clone.service.ts embedded the user-supplied GitHub personal access token into the clone URL (https://<token>@host/...) and forwarded git's stdout/stderr verbatim to the SSE clone-progress feed via onProgress. git echoes the full clone URL in its progress output, so every progress event leaked the token to whoever was watching the feed — including the user, but also anyone whose logs subscribe to the same stream.

// 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: sanitizeGitError was already applied to lastError (line 280) — but after the same text had already been forwarded to the SSE consumer as a progress event.

Fix approach

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).

// 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

• Commit: 15dbf2b — fix(projects): sanitize GitHub tokens from clone progress stream
• File: server/modules/projects/services/project-clone.service.ts (lines 244-259)

P1 — CORS reflects any Origin header

Vulnerability description

app.use(cors({ exposedHeaders: [...] })) in server/index.ts was invoked with no origin option, so the cors 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 (and which the frontend attaches to every fetch via authenticatedFetch), any malicious site a victim visits in the same browser session could read responses from the server on the victim's behalf by issuing requests with the victim's token.

// 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 approach

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 — e.g. server-to-server, curl, the Electron desktop app) 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.

// 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'],
}));

SERVER_PORT / HOST / DISPLAY_HOST / VITE_PORT are moved above the CORS middleware so the reflector can read them at module load time.

Referenced code

• Commit: 990ede3 — fix(server): restrict CORS to same host:port as the server
• File: server/index.ts (CORS section, plus the SERVER_PORT/HOST move)

P2 — System update spawns commands through sh -c

Vulnerability description

runShellCommand in server/modules/system/system.module.ts 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, executed with the server process's privileges. A poisoned $PATH is already enough to substitute a malicious npm/git binary into the call.

// server/modules/system/system.module.ts (before)
function runShellCommand(
  command: string,
  workingDirectory: string,
  environment: NodeJS.ProcessEnv,
  onOutput: (output: string) => void,
  onErrorOutput: (errorOutput: string) => void,
): Promise<...> {
  return new Promise((resolve, reject) => {
    const childProcess = spawn('sh', ['-c', command], {
      cwd: workingDirectory,
      env: environment,
    });
    ...

Fix approach

Split the executor into (command, args) argv arrays and disable the shell. The git workflow 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.

// server/modules/system/system.module.ts (after)
function runShellCommand(
  command: string,
  args: string[],
  workingDirectory: string,
  environment: NodeJS.ProcessEnv,
  onOutput: (output: string) => void,
  onErrorOutput: (errorOutput: string) => void,
): Promise<...> {
  return new Promise((resolve, reject) => {
    const childProcess = spawn(command, args, {
      cwd: workingDirectory,
      env: environment,
      shell: false,
    });
    ...

The service now plans each branch as { command, args }:

// server/modules/system/system.service.ts (after)
const updatePlan = dependencies.isPlatform
  ? { command: 'npm', args: ['run', 'update:platform'] }
  : dependencies.installMode === 'git'
    ? { command: 'sh', args: ['-c', 'git checkout main && git pull && npm install'] }
    : { command: 'npm', args: ['install', '-g', '@cloudcli-ai/cloudcli@latest'] };

Existing service tests are updated to match the new argv signature.

Referenced code

• Commit: c5bb982 — fix(system): spawn update commands without a shell
• Files: server/modules/system/system.module.ts, server/modules/system/system.service.ts, server/modules/system/tests/system.service.test.ts

P2 — /api/auth/login and /api/auth/register have no rate limiting

Vulnerability description

The 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. 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.

// server/modules/auth/auth.routes.ts (before)
router.post('/login', async (req, res, next) => {
  try {
    const body = req.body as { username?: unknown; password?: unknown };
    res.json(await service.login(body.username, body.password));
  } catch (error) {
    next(error);
  }
});

Fix approach

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 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.

// server/modules/auth/rate-limit.middleware.ts (excerpt)
const middleware: RequestHandler = (req: Request, res: Response, next) => {
  const clientKey = readClientKey(req);
  const now = clock();
  let record = records.get(clientKey);
  if (!record) {
    record = { timestamps: [], blockedUntil: 0 };
    records.set(clientKey, record);
  }

  // An active lockout short-circuits the limiter; do not consume an attempt
  // slot so a misbehaving client cannot extend the lockout indefinitely.
  if (record.blockedUntil > now) {
    const retryAfterSeconds = Math.max(1, Math.ceil((record.blockedUntil - now) / 1000));
    res.setHeader('Retry-After', String(retryAfterSeconds));
    res.status(429).json({ success: false, error: { code: 'RATE_LIMITED', ... } });
    return;
  }

  const cutoff = now - windowMs;
  record.timestamps = record.timestamps.filter((timestamp) => timestamp > cutoff);

  if (record.timestamps.length >= maxAttempts) {
    record.blockedUntil = now + lockoutMs;
    ...res.status(429)...
    return;
  }

  record.timestamps.push(now);
  next();
};

The limiter is mounted on both /register and /login:

// server/modules/auth/auth.routes.ts (after)
router.post('/register', authRateLimit.middleware, async (req, res, next) => { ... });
router.post('/login', authRateLimit.middleware, async (req, res, next) => { ... });

A focused unit test (server/modules/auth/tests/rate-limit.middleware.test.ts) verifies the under-cap, over-cap, lockout-no-extend, rolling-window, and per-client-key behaviors.

Referenced code

• Commit: 03e6876 — fix(auth): rate-limit login and registration per client
• Files: server/modules/auth/auth.routes.ts, server/modules/auth/rate-limit.middleware.ts (new), server/modules/auth/tests/rate-limit.middleware.test.ts (new)

hey @wjc2821296948, can you check the coderabbit comments?

hey @wjc2821296948, can you check the coderabbit comments?

OK,I'm checking.

CodeRabbit follow-up — rate-limit Retry-After must reflect the next permitted request

Vulnerability description

CodeRabbit correctly pointed out that the previous lockout calculation set blockedUntil = now + lockoutMs, but if lockoutMs < windowMs, the rolling window retained maxAttempts timestamps whose earliest expiry was timestamps[0] + windowMs. The client was told to retry in lockoutMs seconds, but on its next request the limiter tripped again — starting a new lockout — because the cap was still full.

// server/modules/auth/rate-limit.middleware.ts (before)
if (record.timestamps.length >= maxAttempts) {
  record.blockedUntil = now + lockoutMs;
  const retryAfterSeconds = Math.max(1, Math.ceil(lockoutMs / 1000));
  ...
}

Fix approach

Set blockedUntil to max(now + lockoutMs, earliestRetainedTimestamp + windowMs) so Retry-After always points at the next moment a request can succeed. retryAfterSeconds is derived from the final blockedUntil value.

// server/modules/auth/rate-limit.middleware.ts (after)
if (record.timestamps.length >= maxAttempts) {
  const earliestExpiry = record.timestamps.length > 0
    ? record.timestamps[0]! + windowMs
    : now + windowMs;
  const nextAvailableAt = Math.max(now + lockoutMs, earliestExpiry);
  record.blockedUntil = nextAvailableAt;
  const retryAfterSeconds = Math.max(1, Math.ceil((nextAvailableAt - now) / 1000));
  ...
}

The previously written "lockout does not extend" test advanced the clock by only 500 ms — still inside both the original lockout and the rolling window — so the assertion could not discriminate between a preserved lockout and a newly reset one. Advance by 1.1 s instead and assert the updated Retry-After header reflects the remaining lockout time.

Add a focused regression test that configures lockoutMs < windowMs and verifies Retry-After returns the rolling-window expiry, not the lockout length.

Referenced code

• Commit: 1b48532 — fix(auth): make rate-limit Retry-After reflect the next permitted request
• Files: server/modules/auth/rate-limit.middleware.ts, server/modules/auth/tests/rate-limit.middleware.test.ts

CodeRabbit follow-up — redact GitHub tokens split across stdout/stderr chunks

Vulnerability description

sanitizeGitError replaces exact matches of the token, but git's data events are arbitrary byte slices, not full messages. A credential can be split across two consecutive events — e.g. https://ghp_abc... in one chunk and ...def@github.com/... in the next — in which case neither half matches the full token and both fragments leak through the SSE clone-progress feed.

// server/modules/projects/services/project-clone.service.ts (before)
gitProcess.stdout?.on('data', (data: Buffer | string) => {
  const message = data.toString().trim();
  if (!message) return;
  handlers.onProgress(sanitizeGitError(message, githubToken));  // ← per-chunk replace; misses split tokens
});

Fix approach

Wrap sanitizeGitError in a streaming redactor that buffers up to token.length - 1 characters across chunks. Only the safe prefix (everything older than the last possible token-prefix window) is forwarded as progress; the trailing window is retained until the next chunk confirms whether it completes a token. flush() is wired to the end event of both streams so a half-token that straddles EOF is also redacted wholesale.

// server/modules/projects/services/project-clone.service.ts (after)
function createStreamingRedactor(token: string | null) {
  if (!token) {
    return { feed: (chunk) => chunk, flush: () => '' };
  }
  const maxPrefix = token.length - 1;
  let buffer = '';
  return {
    feed(chunk: string): string {
      if (!chunk) return '';
      buffer += chunk;
      if (buffer.length <= maxPrefix) return '';
      const safeEnd = buffer.length - maxPrefix;
      const safeSlice = buffer.slice(0, safeEnd);
      buffer = buffer.slice(safeEnd);
      return sanitizeGitError(safeSlice, token);
    },
    flush(): string {
      if (!buffer) return '';
      const remainder = buffer;
      buffer = '';
      return sanitizeGitError(remainder, token);
    },
  };
}

Add a regression test that splits a token across two stdout chunks and two stderr chunks and verifies no token fragment reaches the progress callback.

Referenced code

• Commit: 8d83216 — fix(projects): redact GitHub tokens split across stdout/stderr chunks
• Files: server/modules/projects/services/project-clone.service.ts, server/modules/projects/tests/project-clone.service.test.ts

CodeRabbit follow-up — stage plugin updates so a rejected update leaves the live plugin untouched

Vulnerability description

updatePluginFromGit previously performed git pull --ff-only directly against the live plugin directory. After the build-policy change in the previous commit, runBuildIfNeeded rejects updates whose package.json declares a build script without allowBuild: true. That rejection now happens after the pull has already mutated the live directory (and after npm install --ignore-scripts has already rewritten node_modules). The caller (plugins.service.ts update()) had also already stopped the running plugin server before invoking the registry, so a rejected update left the operator with both a half-updated plugin directory and a stopped plugin server.

// server/modules/plugins/plugin-registry.service.ts (before)
export function updatePluginFromGit(name, options) {
  ...
  // Performs side effects directly on the live plugin directory.
  const gitProcess = spawn('git', ['pull', '--ff-only', '--'], {
    cwd: pluginDir, ...
  });
  ...
  npmProcess.on('close', (npmCode) => {
    ...
    runBuildIfNeeded(pluginDir, packageJsonPath, options, ...);  // ← rejection after live-dir mutation
  });
}

// server/modules/plugins/plugins.service.ts (before)
async update(pluginName, options) {
  ...
  const wasRunning = dependencies.isServerRunning(pluginName);
  if (wasRunning) await dependencies.stopServer(pluginName);  // ← stopped even if update is rejected
  const plugin = normalizePluginManifest(await dependencies.update(pluginName, options));
  if (wasRunning) await startServerIfAvailable(plugin);
  return { success: true, plugin };
}

Fix approach

Switch the registry to the same staging pattern installPluginFromGit already uses: re-clone the plugin's remote URL into a sibling temp directory, validate the manifest, run npm install, apply the build policy, and only then atomically rename the temp directory over the live one. A rejection at any step cleans up the temp directory and the live plugin directory is never touched.

// server/modules/plugins/plugin-registry.service.ts (after)
const tempDir = fs.mkdtempSync(path.join(pluginsDir, `.tmp-update-${name}-`));
...
const cloneProcess = spawn('git', ['clone', '--depth', '1', '--', remoteUrl, tempDir], ...);
cloneProcess.on('close', (code) => {
  ...
  // Validate manifest, run npm install, apply build policy — all against tempDir.
  ...
  // Only swap into place when every step has succeeded.
  runBuildIfNeeded(tempDir, packageJsonPath, options, () => finalize(manifest), (err) => { cleanupTemp(); reject(err); });
});

Update the service to restart the previously running plugin server when the update is rejected — the live directory is unchanged, so a clean restart restores the previous plugin state.

// server/modules/plugins/plugins.service.ts (after)
async update(pluginName, options) {
  ...
  const wasRunning = dependencies.isServerRunning(pluginName);
  if (wasRunning) await dependencies.stopServer(pluginName);
  try {
    const plugin = normalizePluginManifest(await dependencies.update(pluginName, options));
    if (wasRunning) await startServerIfAvailable(plugin);
    return { success: true, plugin };
  } catch (error) {
    if (wasRunning) await startServerIfAvailable(this.getManifest(pluginName));
    throw error;
  }
}

Cover the new contract with a service test that verifies a rejected update stops, attempts the update, and then restarts the previously running server.

Referenced code

• Commit: 6ae7bbd — fix(plugins): stage plugin updates so a rejected update leaves the live plugin untouched
• Files: server/modules/plugins/plugin-registry.service.ts, server/modules/plugins/plugins.service.ts, server/modules/plugins/tests/plugins.service.test.ts

CodeRabbit follow-up — corrected the simulated-time explanation in the lockout-extension test

Vulnerability description

CodeRabbit noted that the comment above currentTime += 1100 in server/modules/auth/tests/rate-limit.middleware.test.ts reversed the two timing conditions: it claimed currentTime was past the original lockout and still inside the rolling window, when in fact it is past the rolling window and still inside the lockout.

// server/modules/auth/tests/rate-limit.middleware.test.ts (before)
// Advance time past one second — past the original lockout boundary — and
// verify the block window does NOT reset/extend (would happen if we kept
// consuming slots). Advancing by 1100 ms puts us 100 ms past the original
// `now + lockoutMs` of 3000 but still inside `timestamps[0] + windowMs`
// (1000 + 1000 = 2000), so the preserved lockout is observable in the
// updated `Retry-After` header.
currentTime += 1100;

Fix approach

Rewrite the explanation so the two boundaries (lockout expiry at 3000 vs. rolling-window expiry at 2000) and their relative positions to the advanced clock (2100) are described correctly. No behaviour change to the test itself.

// server/modules/auth/tests/rate-limit.middleware.test.ts (after)
// Advance simulated time by 1100 ms. The original lockout expires at
// `now + lockoutMs` = 1000 + 2000 = 3000, so 2100 ms is still inside it.
// The rolling-window timestamp at 1000 expires at 1000 + 1000 = 2000, so
// 2100 ms is 100 ms past that expiry. A preserved lockout therefore
// returns the remaining `Retry-After` duration; a reset lockout would
// also return the same number. Advance far enough that the next request
// would only succeed if the lockout was preserved — the next call sits
// at `now = 2100`, so the preserved lockout returns 1 (`3000 - 2100`).
currentTime += 1100;

Referenced code

• Commit: ebff12b — test(auth): correct the simulated-time explanation in the lockout-extension test
• File: server/modules/auth/tests/rate-limit.middleware.test.ts

CodeRabbit follow-up — redact any token prefix that reaches the SSE stream

Vulnerability description

The previous streaming redactor buffered the last token.length - 1 characters across chunks so a credential split across two data events was redacted. That addressed the obvious cross-chunk split, but two corner cases still leaked credentials:

1. feed could emit a safeSlice whose suffix matched a prefix of the token. The redactor only held back a fixed maxPrefix window regardless of how safeSlice ended. If a token wholly inside a single chunk had its last character land in the held-back window, the rest of the token was already in safeSlice and was forwarded verbatim — sanitizeGitError only matches the full token, so the prefix fragment leaked.

2. flush returned the remaining buffer unchanged except for an exact-match pass. If a token straddled the end of the stream with one or more characters still in the buffer, the trailing portion was a credential prefix that sanitizeGitError did not recognise and therefore forwarded verbatim.

Additionally, lastError was being overwritten with each raw stderr chunk, so a token split across stderr chunks could land in lastError and be returned verbatim from resolveCloneFailureMessage when the clone failed.

// server/modules/projects/services/project-clone.service.ts (before)
return {
  feed(chunk: string): string {
    if (!chunk) return '';
    buffer += chunk;
    if (buffer.length <= maxPrefix) return '';
    const safeEnd = buffer.length - maxPrefix;
    const safeSlice = buffer.slice(0, safeEnd);
    buffer = buffer.slice(safeEnd);
    return sanitizeGitError(safeSlice, token);  // ← does NOT catch token prefix in safeSlice
  },
  flush(): string {
    if (!buffer) return '';
    const remainder = buffer;
    buffer = '';
    return sanitizeGitError(remainder, token);  // ← does NOT catch token prefix in remainder
  },
};

Fix approach

Update the redactor so feed additionally holds back the longest suffix of safeSlice that is itself a (non-empty) prefix of the token, and both feed and flush run their output through a new redactAnyTokenPrefix helper that replaces any prefix of the token of length ≥ 3 with ***. The minimum prefix length avoids false-positive redaction of single characters in legitimate output.

// server/modules/projects/services/project-clone.service.ts (after)
feed(chunk: string): string {
  if (!chunk) return '';
  buffer += chunk;
  if (buffer.length <= maxPrefix) return '';
  const safeEnd = buffer.length - maxPrefix;
  let safeSlice = buffer.slice(0, safeEnd);
  buffer = buffer.slice(safeEnd);
  // Never emit text that ends with a non-empty prefix of the token.
  const retain = trailingPrefixLength(safeSlice);
  if (retain > 0) {
    const retained = safeSlice.slice(safeSlice.length - retain);
    safeSlice = safeSlice.slice(0, safeSlice.length - retain);
    buffer = retained + buffer;
  }
  return redactAnyTokenPrefix(safeSlice, token);
},
flush(): string {
  if (!buffer) return '';
  const remainder = buffer;
  buffer = '';
  return redactAnyTokenPrefix(remainder, token);
},

Fix the lastError accumulator to consume the redacted stderr output:

// server/modules/projects/services/project-clone.service.ts (after)
gitProcess.stderr?.on('data', (data: Buffer | string) => {
  const raw = data.toString();
  lastError += stderrRedactor.feed(raw);  // ← accumulates redacted text, not raw
});

Tighten the regression test for the split-chunk redaction so it asserts the security property the redactor actually guarantees (the full token cannot be reconstructed from the SSE stream, in any single message or across concatenated messages) rather than checking for individual substrings. Add a second regression test that verifies the failure-path redaction for a split token in stderr.

Referenced code

• Commit: 47b036c — fix(projects): redact any token prefix that reaches the SSE stream
• Files: server/modules/projects/services/project-clone.service.ts, server/modules/projects/tests/project-clone.service.test.ts

CodeRabbit follow-up — preserve the live plugin directory when the update swap fails

Vulnerability description

updatePluginFromGit's finalize step performed fs.rmSync(pluginDir) followed by fs.renameSync(tempDir, pluginDir). If the rename failed (partition full, permissions race, Windows AV scanner holding a file handle, etc.) the temp directory was cleaned up by the catch block but the previous plugin directory was already gone. The previous plugin was lost and plugins.service.ts update() could not load the previous manifest during server recovery.

// server/modules/plugins/plugin-registry.service.ts (before)
const finalize = (manifest) => {
  try {
    if (fs.existsSync(pluginDir)) {
      fs.rmSync(pluginDir, { recursive: true, force: true });  // ← previous plugin lost if next line fails
    }
    fs.renameSync(tempDir, pluginDir);
  } catch (err) {
    cleanupTemp();
    return reject(new Error(`Failed to move updated plugin into place: ${err.message}`));
  }
  resolve(manifest);
};

Fix approach

Switch to a backup-restore pattern: rename the live directory to a sibling backup, rename the temp directory into place, then delete the backup. If the second rename fails, restore the backup to the live directory and clean up the temp directory. The previous plugin is always recoverable from either the live path or the backup.

// server/modules/plugins/plugin-registry.service.ts (after)
const finalize = (manifest) => {
  const backupDir = fs.existsSync(pluginDir)
    ? `${pluginDir}.previous-${process.pid}-${Date.now()}`
    : null;
  try {
    if (backupDir) {
      fs.renameSync(pluginDir, backupDir);
    }
    fs.renameSync(tempDir, pluginDir);
  } catch (err) {
    try { fs.rmSync(tempDir, { recursive: true, force: true }); } catch {}
    if (backupDir && fs.existsSync(backupDir) && !fs.existsSync(pluginDir)) {
      try { fs.renameSync(backupDir, pluginDir); } catch {}
    }
    return reject(new Error(`Failed to move updated plugin into place: ${err.message}`));
  }
  if (backupDir) {
    try { fs.rmSync(backupDir, { recursive: true, force: true }); } catch {}
  }
  resolve(manifest);
};

Add a small smoke test for the registry's URL pre-checks (full swap-failure coverage would require an fs mock framework that this codebase does not currently depend on).

Referenced code

• Commit: 327e28c — fix(plugins): preserve the live plugin directory when the update swap fails
• Files: server/modules/plugins/plugin-registry.service.ts, server/modules/plugins/tests/plugin-registry.service.test.ts (new)

CodeRabbit follow-up — bound redaction work and require URL scheme for installs

Vulnerability description

Two follow-ups to the previous streaming-redactor change:

1. redactAnyTokenPrefix previously built an O(N) regex from N token prefixes joined with |. A megabyte-long token would produce a megabyte-long regex pattern, which is a ReDoS risk (CWE-1333) and an unbounded CPU/memory cost per chunk.

2. installPluginFromGit previously validated only that the URL was a non-empty string that did not start with -. Any other shape (including file:// and http://) passed through to git clone — and the registry's repoName regex happened to accept paths like /tmp/local. The HTTP route layer already enforces https:///git@ upstream, but the registry should also enforce the scheme so any internal caller cannot bypass the check.

// server/modules/projects/services/project-clone.service.ts (before)
function redactAnyTokenPrefix(message: string, token: string): string {
  if (!message || !token) return message;
  const alternatives: string[] = [];
  for (let length = token.length; length >= MIN_TOKEN_PREFIX_LENGTH; length -= 1) {
    const escaped = token.slice(0, length).replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
    alternatives.push(escaped);
  }
  return message.replace(new RegExp(alternatives.join('|'), 'g'), '***');
  // ↑ O(N) alternations; ReDoS surface
}

// server/modules/plugins/plugin-registry.service.ts (before)
if (typeof url !== 'string' || !url.trim()) {
  return reject(new Error('Invalid URL: must be a non-empty string'));
}
if (url.startsWith('-')) {
  return reject(new Error('Invalid URL: must not start with "-"'));
}
// file://, http://, etc. all pass through to git clone

Fix approach

Replace the regex with a bounded linear scanner that tries the longest prefix first at each position. Add a MAX_TOKEN_LENGTH_FOR_REDACTION cap (2048) so the per-token memory and CPU budget is bounded regardless of input size; the createStreamingRedactor buffer uses the same cap. Add a first-character fast path so the inner loop only runs at positions that could plausibly start a token prefix.

// server/modules/projects/services/project-clone.service.ts (after)
function redactAnyTokenPrefix(message: string, token: string): string {
  if (!message || !token) return message;
  const effectiveToken = token.length > MAX_TOKEN_LENGTH_FOR_REDACTION
    ? token.slice(0, MAX_TOKEN_LENGTH_FOR_REDACTION)
    : token;
  if (effectiveToken.length < MIN_TOKEN_PREFIX_LENGTH) return message;

  const firstChar = effectiveToken[0];
  let result = '';
  let cursor = 0;
  while (cursor < message.length) {
    if (message[cursor] !== firstChar) {
      result += message[cursor];
      cursor += 1;
      continue;
    }
    const remaining = message.length - cursor;
    const maxMatch = Math.min(effectiveToken.length, remaining);
    let matchedLength = 0;
    for (let length = maxMatch; length >= MIN_TOKEN_PREFIX_LENGTH; length -= 1) {
      if (message.startsWith(effectiveToken.slice(0, length), cursor)) {
        matchedLength = length;
        break;
      }
    }
    if (matchedLength > 0) {
      result += '***';
      cursor += matchedLength;
    } else {
      result += message[cursor];
      cursor += 1;
    }
  }
  return result;
}

Reject URLs that do not start with https:// or git@ with a clear Invalid URL error before any disk work.

// server/modules/plugins/plugin-registry.service.ts (after)
if (typeof url !== 'string' || !url.trim()) {
  return reject(new Error('Invalid URL: must be a non-empty string'));
}
if (url.startsWith('-')) {
  return reject(new Error('Invalid URL: must not start with "-"'));
}
// Only allow the supported remote transports. The HTTP layer in
// plugins.service.ts already enforces this for incoming requests, but
// validating here too prevents any internal caller from bypassing the
// scheme check.
if (!url.startsWith('https://') && !url.startsWith('git@')) {
  return reject(new Error('Invalid URL: only https:// and git@ remotes are supported'));
}

Referenced code

• Commit: 05f87bd — fix(projects,plugins): bound redaction work and require URL scheme for installs
• Files: server/modules/projects/services/project-clone.service.ts, server/modules/plugins/plugin-registry.service.ts

hey, can u separate out the PRs for each different fix you had. It's very difficult to review as it stands.

After submitting a different PR for each of them, I will take a review.

OK

Done — split into 5 separate PRs as you requested:

• fix(plugins): disable auto-running npm run build and stage plugin updates (P0) #1117 (P0) plugin install RCE + update staging + URL scheme
• fix(projects): redact GitHub tokens from clone progress stream (P1) #1118 (P1) GitHub PAT redaction across SSE chunks + bounded scanner
• fix(server): restrict CORS to same host:port as the server (P1) #1119 (P1) CORS restricted to same host:port
• fix(system): spawn update commands without a shell (P2) #1120 (P2) system update spawns without sh -c
• fix(auth): rate-limit login and registration per client (P2) #1121 (P2) auth login/register rate limit

Each PR has a per-commit comment block (severity → description → fix → code snippet) and is built on its own branch off upstream/main, so they can be reviewed and merged independently.

🤖 Generated with Claude Code