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

Đang mở
  • Có gì không ổn?
#669 3 bình luận 0 reaction 0 người được giao Xem trên GitHub

Chưa có ai nhận issue này.

Đánh giá

Độ khó
4/5
Thời gian dự kiến
3-5 ngày
Mức phù hợp với người mới
48/100
Loại issue
Lỗi
Độ rõ ràng
Khá rõ ràng
Mức độ hoạt động
Ít trao đổi
Công nghệ
next.js, typescript
Lĩnh vực
api, security

Hướng nghiên cứu

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.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Mô tả

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

Ngôn ngữ chính
TypeScript
Star
6.5k
Fork
697
Không có pull request nào được merge trong 30 ngày

Chuẩn bị môi trường

  • Không có Dockerfile hay tệp Docker Compose
  • Có mẫu pull request
  • Không có hướng dẫn đóng góp

Bắt đầu từ đâu

  1. Đọc hết issue, rồi đọc hướng dẫn đóng góp của dự án.
  2. Bình luận trên issue rằng bạn sẽ nhận — tránh hai người làm cùng một việc.
  3. Fork repository và làm thay đổi trên một nhánh.
  4. Mở pull request có tham chiếu số hiệu của issue.

Issue khác của op7418/CodePilot

Tất cả issue của op7418/CodePilot

Issue tương tự

Thêm issue về TypeScript

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.