Security: two caller-controlled path/binary issues in local API routes (CWE-78, CWE-22)

Aberta
  • Algo errado?
#669 3 comentários 0 reações 0 responsáveis Ver no GitHub

Ninguém assumiu esta issue ainda.

Avaliação

Dificuldade
4/5
Tempo estimado
3-5 dias
Facilidade para iniciantes
48/100
Tipo de issue
Bug
Clareza
Razoavelmente clara
Status de atividade
Pouca atividade
Stack de tecnologia
next.js, typescript
Domínio
api, security

Direção de pesquisa

Read src/app/api/files/open/route.ts and the path helpers in src/lib/files.ts first, then inspect src/app/api/cli-tools/custom/route.ts and src/app/api/settings/workspace/route.ts. Decide and apply appropriate executable and workspace-root restrictions, including checks for paths derived from the stored setting. Done means callers cannot choose arbitrary executables or use workspace paths outside the permitted base.

Escrita pelo modelo de indexação a partir do texto da issue.

Descrição

(Apologies — this issue was opened accidentally with a placeholder title while I was checking permissions, and I've replaced it with the actual report rather than leave noise behind.)

Hi — thanks for CodePilot. While running a static-analysis benchmark over public repositories I found two issues in the local API routes. Verified on main at ff9dc31.

Both are local-bound: the Next.js server listens on localhost, so the realistic attacker is another process on the machine, or a web page in the browser if any route is reachable cross-origin. I don't want to overstate the severity — flagging them because both are straightforward to tighten, and because you've already built the right helpers in this repo.

说明： 两个问题都出在本地 API 路由上：一个是调用方可以指定任意可执行文件路径，另一个是工作区路径没有做目录限制。仓库里已经有 isPathSafe / assertRealPathInBase 这两个工具函数，只是这两条路由还没用上。

Context: the fix pattern already exists here

src/app/api/files/open/route.ts was recently hardened — it now uses spawn(command, args, { shell: false }) plus isPathSafe(baseDir, requestedPath) and assertRealPathInBase(...) from src/lib/files.ts, with symlink detection. That is exactly the right shape.

The two routes below predate that work and don't yet use those helpers.

1. POST /api/cli-tools/custom — caller chooses which binary is executed (CWE-78)

src/app/api/cli-tools/custom/route.ts:

const { binPath, name } = body as { binPath?: string; name?: string };
if (!binPath || typeof binPath !== 'string') { ...400... }
if (!path.isAbsolute(binPath)) { ...400... }          // :35
await access(binPath, constants.X_OK);                 // :41
const { stdout, stderr } = await execFileAsync(binPath, ['--version'], { ... });   // :49

The checks are "is it an absolute path" and "is it executable" — not "is it a CLI tool the app should be running". Any executable on the filesystem can be named and will be run.

To be fair to the design: execFileAsync with an argv array means there is no shell, so this is not command injection — an attacker cannot inject metacharacters or chain commands. The exposure is that they choose the executable, and it runs with the server's privileges.

Suggested fix: resolve the real path and require it to sit within an expected location (a configured tools directory, or a known-good install prefix), or match the basename against an allowlist of supported tools. assertRealPathInBase already does the containment half.

2. PUT /api/settings/workspace — workspace path is unconstrained (CWE-22)

src/app/api/settings/workspace/route.ts:

const { path: workspacePath, initialize, ... } = body as { path: string; ... };
if (!workspacePath || typeof workspacePath !== 'string') { ...400... }   // :173
...
fs.mkdirSync(workspacePath, { recursive: true });                        // :221
...
setSetting(ASSISTANT_WORKSPACE_PATH_SETTING, workspacePath);             // :271

The only validation is non-empty-string. With initialize: true, mkdirSync(..., { recursive: true }) will create a directory tree at any location the process can write.

The stored value is then used as a base for later writes, e.g. :313/:319:

const soulPath = path.join(workspacePath, variant);   // variant ∈ soul.md / Soul.md / SOUL.md
...
fs.writeFileSync(soulPath, cleaned.trimEnd() + '\n', 'utf-8');

So a caller-supplied path becomes a persistent base for directory creation and file writes. Neither isPathSafe nor assertRealPathInBase is used in this file.

Suggested fix: constrain the workspace root to a permitted base (home directory, or a configured projects root), reject paths that escape it after path.resolve + realpath, and apply the same check on read/write paths derived from the stored setting. This is the same treatment files/open already received.

Note on scope

I'm aware the app is intentionally local-first and that src/app/api/files/browse/route.ts carries a comment noting there's no restriction because the user chooses where to work. That's a legitimate product decision. The reason I'd still tighten these two: mkdirSync/writeFileSync and "execute this binary" are stronger primitives than directory browsing, and any route reachable from a browser page turns a local-only assumption into a remote one. Your call entirely.

Reproduced locally from a fresh clone; nothing was tested against anyone's running instance.

Linguagem predominante
TypeScript
Estrelas
6.5k
Forks
697
Nenhum PR com merge em 30d

Preparar o ambiente

  • Sem Dockerfile nem arquivo Docker Compose
  • Tem um modelo de pull request
  • Sem guia de contribuição

Primeiros passos

  1. Leia a issue inteira e depois o guia de contribuição do projeto.
  2. Comente na issue dizendo que vai assumir — evita que duas pessoas façam o mesmo trabalho.
  3. Faça um fork do repositório e trabalhe em uma branch.
  4. Abra um pull request que referencie o número da issue.

Mais de op7418/CodePilot

Todas as issues de op7418/CodePilot

Issues semelhantes

Mais issues de TypeScript

Receba novas issues na sua caixa de entrada

Um resumo curto de issues do GitHub para quem está começando.