anthropics / anthropics/claude-code-action

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

Abierto
#1,768 0 comentarios 0 reacciones 0 asignados Ver en GitHub
bug p3
Lenguaje dominante
TypeScript
Estrellas
8.9k
Forks
2.1k
Métricas de merge de PR
Métricas de PR pendientes

Descripción

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

Guía de contribución

Abrir la guía de contribución

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.