anthropics / anthropics/claude-code-action

getFileMode uses stat(), making its isSymbolicLink() branch unreachable dead code

Đang mở
#1,768 0 bình luận 0 reaction 0 người được giao Xem trên GitHub
bug p3
Ngôn ngữ chính
TypeScript
Star
8.9k
Fork
2.1k
Chỉ số merge pull request
Chỉ số pull request đang chờ

Mô tả

**Type:** bug / dead code
**Severity:** low
**Area:** `src/mcp/github-file-ops-server.ts`
**Effort:** small (the safe fix is trivial; full symlink support is not)

## Summary

`getFileMode` branches on `fileStat.isSymbolicLink()` to return git's symlink
mode `120000`. It calls `stat()`, which **follows** symlinks, so the returned
`Stats` never describes a link - `isSymbolicLink()` is always `false` and the
branch can never be taken.

## Affected code

`src/mcp/github-file-ops-server.ts:172-190`

```ts
const fileStat = await stat(filePath); // follows symlinks
if (fileStat.isFile()) {
...
} else if (fileStat.isDirectory()) {
return "040000";
} else if (fileStat.isSymbolicLink()) { // unreachable
return "120000";
} else {
return "100644";
}
```

## Actual behaviour today

A symlink passed to `commit_files` is committed as a **regular file** whose
content is the link target's content:

- `getFileMode` stats through the link and returns `100644` (or `100755`).
- `readFile(fullPath)` (line 261) also follows the link and reads the target's
bytes.
- The tree entry is a normal blob.

So the link is silently dereferenced. That is a defensible behaviour, but it is
not the behaviour the code claims to implement, and there is no test covering it.

## Why the obvious fix is wrong

Swapping `stat` for `lstat` in isolation makes things worse, not better. Git
stores a symlink as a blob whose *content is the target path*. With `lstat` alone
the entry would get mode `120000` while `readFile` still supplies the **target's
file content** as the blob - producing a committed "symlink" pointing at a path
made of the target file's bytes. That is a corrupt tree.

Correct symlink support requires changing both halves together:

```ts
const fileStat = await lstat(filePath);
if (fileStat.isSymbolicLink()) {
const target = await readlink(filePath);
// blob content must be `target`, mode 120000
}
```

...plus a decision about whether links escaping the repo should be rejected,
which interacts with `validatePathWithinRepo` (`src/mcp/path-validation.ts`).
`validatePathWithinRepo` already resolves symlinks and rejects ones landing
outside the repo root, so an in-repo link is the only case that reaches here.

## Suggested fix

Two options, in order of preference:

1. **Minimal and honest** - drop the unreachable branch and document that
symlinks are intentionally dereferenced:

```ts
// stat() follows symlinks, and readFile() below does too, so a symlinked
// path is committed as a regular blob holding the target's content. That is
// deliberate: committing mode 120000 would require the blob content to be
// the link target path instead.
const fileStat = await stat(filePath);
if (fileStat.isDirectory()) return "040000";
if (fileStat.isFile() && fileStat.mode & constants.S_IXUSR) return "100755";
return "100644";
```

2. **Full support** - `lstat` + `readlink`, with the blob content changed in
`commit_files` at the same time, and a test asserting the committed tree entry
for a symlink. Larger change; only worth it if preserving links in commits is
actually wanted.

Either way the current state - a branch that documents behaviour the code does
not have - should not stay.

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Đánh giá

Issue này chưa được đánh giá.

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.