anthropics / anthropics/claude-code-action

isAllowedBot is duplicated byte-for-byte across checkHumanActor and checkWritePermissions

Abierto
#1,765 0 comentarios 0 reacciones 0 asignados Ver en GitHub
area:permissions dev-experience p3
Lenguaje dominante
TypeScript
Estrellas
8.9k
Forks
2.1k
Métricas de merge de PR
Métricas de PR pendientes

Descripción

**Type:** maintainability / drift risk in a security control
**Severity:** medium
**Area:** `src/github/validation/actor.ts`, `src/github/validation/permissions.ts`
**Effort:** trivial

## Summary

The `allowed_bots` allow-list matcher exists twice, as two identical private
copies in two different files. Both are part of the action's authorization path.
Verified identical:

```
$ diff <(sed -n 8,26p src/github/validation/permissions.ts) \
<(sed -n 11,29p src/github/validation/actor.ts)
# (no output)
```

## Affected code

- `src/github/validation/permissions.ts:8-25` - used by `checkActorWritePermissions`
when the collaborator-permission API reports the actor "is not a user"
- `src/github/validation/actor.ts:11-28` - used by `checkHumanActor` for
non-`User` account types and for actors the Users API cannot resolve

```ts
function isAllowedBot(actor: string, allowedBots: string): boolean {
const trimmed = allowedBots.trim();
if (trimmed === "*") return true;
if (!trimmed) return false;

const allowedList = trimmed
.split(",")
.map((bot) => bot.trim().toLowerCase().replace(/\[bot\]$/, ""))
.filter((bot) => bot.length > 0);

const normalizedActor = actor.toLowerCase().replace(/\[bot\]$/, "");
return allowedList.includes(normalizedActor);
}
```

## Impact

Both call sites gate whether an automated actor may trigger Claude. A fix or
hardening applied to one copy - handling whitespace inside entries, supporting a
`*[bot]` wildcard the way `actorMatchesPattern` does, normalising a different
suffix - silently does not apply to the other. The two checks then disagree about
whether a given bot is allowed, which is the worst outcome for an authorization
predicate: one layer permits what the other denies, and which one wins depends on
which code path the event happens to take.

There is no test asserting the two stay in agreement.

## Suggested fix

Move the function to the existing actor-filter utility and import it in both
validators:

```ts
// src/github/utils/actor-filter.ts
export function isAllowedBot(actor: string, allowedBots: string): boolean { ... }
```

`src/github/utils/actor-filter.ts` already owns the neighbouring concerns
(`parseActorFilter`, `resolveActorName`, `actorMatchesPattern`,
`shouldIncludeCommentByActor`) and already has coverage in
`test/actor-filter.test.ts`, so the shared copy inherits a test home.

Worth noting while consolidating: `isAllowedBot` does exact matching after
stripping a `[bot]` suffix, while `actorMatchesPattern` in the same area supports
a `*[bot]` wildcard. `allowed_bots` documents `'*'` for "all bots" so the
behaviours are defensible, but having them in one file makes the difference
visible instead of accidental.

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.