addyosmani / addyosmani/agent-skills

Security review of hooks/ — 1 critical, 2 medium, 2 low (NLPM auto-scan)

Aperta
#106 1 commento 0 reazioni 0 assegnatari Vedi su GitHub
Lingua principale
JavaScript
Stelle
93.8k
Fork
10k
Merge medio
3g 15h
PR unite (30g)
23

Descrizione

Hi — while running an automated NL-quality audit on this repository ([NLPM](https://github.com/xiaolai/nlpm-for-claude)), the security pre-scan flagged a few patterns in the hooks directory that seemed worth surfacing. Treating this as friendly notice, not a vulnerability report — they may all be intentional.

## Findings

| # | Severity | File | Line | Pattern |
|---|----------|------|------|---------|
| 1 | Critical | `hooks/simplify-ignore-test.sh` | 34 | `eval` of code extracted from another file |
| 2 | Medium | `hooks/sdd-cache-pre.sh` | 71 | unauthenticated outbound `curl` (SSRF-shaped) |
| 3 | Medium | `hooks/sdd-cache-post.sh` | 82 | `curl -L` follows redirects to a user-supplied URL |
| 4 | Low | `hooks/session-start.sh` | 13 | `$CONTENT` interpolated into JSON heredoc without escaping |
| 5 | Low | `hooks/sdd-cache-post.sh` | 118 | arbitrary web content written to local cache without size/type validation |

### Finding #1 — the one that triggered the gate

```sh
eval "$(sed -n '/^filter_file()/,/^}/p' hooks/simplify-ignore.sh)"
```

The test runner extracts a function definition from `simplify-ignore.sh` at runtime and `eval`s it. Concrete failure mode: any future write that lands a malicious payload inside `simplify-ignore.sh` would execute in the test runner without a separate review pass. People review test runners with low suspicion because tests rarely look like an attack surface — that's exactly when this pattern bites.

Safer shape: source the file (`. hooks/simplify-ignore.sh`) so shell parses it as code, or extract the function into a sourced library. `eval` of file content has no advantage here.

### Findings #2 and #3 — outbound curl

Both `sdd-cache-pre.sh` and `sdd-cache-post.sh` issue `curl` requests against URLs the agent constructs from prior input. Without a domain allowlist, a malicious skill description could direct the request to internal services (cloud metadata endpoints, internal admin panels). `curl -L` makes #3 worse because the redirect target isn't checked.

Mitigations: validate the host against an allowlist before issuing the request; add `--max-redirs 0` or check the resolved URL.

### Findings #4 and #5 — lower priority

Cosmetic JSON-injection (a SKILL.md containing `"` would produce malformed JSON output) and an unbounded write of remote content to local disk. Worth noting but not gate-worthy on their own.

## About this report

These findings come from NLPM's security pattern matcher applied to executable artifacts (hooks, scripts, MCP configs, package manifests). The matcher errs on the side of surfacing possible issues; some flagged patterns turn out to be intentional and acceptable in context.

If any of these are by design, please close this issue. If you'd like a fix PR for any of them, let me know which and I'll send one. Either way, no further action expected unless you want it.

Guida per i contributori

Apri la guida per i contributori

Valutazione

Questa issue non è ancora stata valutata.

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.