Skip to content

Harden opt-in lifecycle hooks (SSRF, cache poisoning, symlink overwrite, data loss)#294

Open
bill143 wants to merge 1 commit into
addyosmani:mainfrom
bill143:fix/harden-lifecycle-hooks
Open

Harden opt-in lifecycle hooks (SSRF, cache poisoning, symlink overwrite, data loss)#294
bill143 wants to merge 1 commit into
addyosmani:mainfrom
bill143:fix/harden-lifecycle-hooks

Conversation

@bill143

@bill143 bill143 commented Jun 17, 2026

Copy link
Copy Markdown

What

Hardens the three opt-in lifecycle hooks (sdd-cache-pre.sh, sdd-cache-post.sh, simplify-ignore.sh) against security and data-integrity issues surfaced during a review. These hooks are not enabled by hooks/hooks.json (which only wires session-start.sh), so this affects users who manually enable the WebFetch cache or simplify-ignore per the docs. session-start.sh is unchanged (reviewed clean).

Why / findings addressed

Severity Issue Fix
Critical SSRFcurl -L "$URL" on an unvalidated, redirect-following URL fires automatically on every WebFetch; a 302 can reach 169.254.169.254, localhost, RFC1918 url_is_safe() allowlist + drop -L (--proto =https --max-redirs 0)
High Cache poisoning — no TTL; any 304 re-serves the cached body forever as "unchanged" TTL cap via existing fetched_at (SDD_CACHE_MAX_AGE, default 24h)
High World-readable / tamperable cache — default perms on fetched bodies cache dir 0700, files 0600 via umask 077
High Symlink / arbitrary-file overwritecat > "$target" on untrusted file_path follows symlinks; session-long TOCTOU reject symlinks at entry + re-verify regular-file target before every write-back
Critical (integrity) Data loss — non-atomic in-place rewrite under set -e can leave a half-placeholdered file after the backup was consumed trap-guarded restore-from-backup around the rewrite
Undeclared perl dependency — abort point on minimal Git Bash replaced with pure-shell trailing-newline trim (dependency removed)
Path-identity collisionsC:\x vs /c/x hash to different IDs → stale restore normalize path before hashing file_id

How it's verified

  • bash -n clean on all four hooks.
  • Existing suites pass: 21/21 simplify-ignore-test.sh, session-start-test.sh OK.
  • Added smoke test for the new SSRF guard: 15/15 (covers https-only, loopback, RFC1918, link-local/metadata, CGNAT, IPv6 literal, user@host userinfo trick, and correctly-allowed public hosts incl. just-outside-range 172.15.0.1).

Notes / open design decisions (flagged, not silently chosen)

  • DNS rebinding is not fully closed — a public hostname resolving to a private IP still passes without resolved-IP pinning (curl --resolve), which adds platform variance on Git Bash. Left as a follow-up; relies on WebFetch's own egress controls for that case.
  • --max-redirs 0 trades cache hit-rate on redirecting origins for SSRF safety. A middle ground (--max-redirs N --proto-redir =https + re-validating %{url_effective}) is possible if hit-rate matters.
  • TTL default (24h) is a guess; tune to how stale a cached "reading" may be.
  • cat > kept (not mv) to preserve inode/permissions as the original intended; atomicity-of-outcome is provided by the trap instead.
  • file_id change orphans any pre-existing cache entries from the old scheme (one-time; Stop already tolerates orphans).

Happy to split this into per-file PRs or adjust any of the above. Related findings filed as an issue for tracking.

🤖 Generated with Claude Code

…te, data loss

sdd-cache-{pre,post}.sh:
- Add url_is_safe() allowlist: require https://, reject loopback/RFC1918/
  link-local (incl. 169.254 metadata)/CGNAT/IPv6-literal/*.internal hosts.
- Drop `curl -L`; use --proto =https --max-redirs 0 (kills redirect SSRF).
- Enforce TTL cap (SDD_CACHE_MAX_AGE, default 24h) before serving on 304.
- Create cache dir 0700 under umask 077 so entries are not world-readable.

simplify-ignore.sh:
- Reject symlinks at Read entry; re-verify regular-file target before every
  write-back (Read/Edit/Write/Stop) to close TOCTOU/arbitrary-overwrite.
- trap-guarded atomic rewrite: restore from backup on unexpected exit so an
  abort can never leave the file half-placeholdered with the backup consumed.
- Remove undeclared `perl` dependency (pure-shell trailing-newline trim).
- Normalize path before hashing file_id (C:\x and /c/x map to one identity).

Existing tests pass (21/21 simplify-ignore, session-start); added SSRF guard
smoke test (15/15).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@addyosmani

Copy link
Copy Markdown
Owner

This is security-sensitive by nature, so it gets a careful pass rather than a quick merge. SSRF and cache-poisoning hardening on the lifecycle hooks is worth doing, but these hooks run on every session start for every user, so I want to read the change against the existing no-jq fallback and the payload test before anything lands. Give me the time to go through it properly, and thanks for taking the hooks seriously.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants