addyosmani / addyosmani/agent-skills
Security review of hooks/ — 1 critical, 2 medium, 2 low (NLPM auto-scan)
- 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.