Addresses the second automated review on PR #941. All three code findings
verified against the actual behaviour, not just patched.
[Medium] shutil.copytree dereferenced symlinks the validator never saw. The
validator checks SKILL.md, the three supporting files and chapters/*.md; the
copy then followed a link anywhere else in the tree (assets/, any subdirectory)
and baked the target's real content into a package that can go out as
--distribution shareable. _assert_no_symlinks() now walks the whole tree and
refuses, and runs BEFORE the validation branch so --skip-validation cannot
bypass it. copytree also passes symlinks=True so loosening that check later
cannot silently reintroduce dereferencing.
Verified: a symlink in assets/ pointing at a secret file is refused both with
and without --skip-validation, the secret never lands in a package, and a clean
tree still emits.
[Medium] The DOCX XXE/entity guard did not extend to EPUB's ebooklib path.
Upstream hardened DOCX only. EPUB is the same zip-of-XML shape and ebooklib —
one of the packages this skill recommends installing — parsed container.xml,
the OPF and content docs with no equivalent pre-check. The guard moved to a new
book_to_skill/zip_safety.py and now runs for both formats.
Verified: an EPUB whose OPF declares an entity is refused; a clean EPUB still
extracts and detects its chapter.
[Low] No size cap before decompressing zip members. Every archive read now goes
through safe_read(), which checks the declared uncompressed size and the
compression ratio against the central directory before decompressing, and
charges actual bytes against a per-archive budget so a lying directory cannot
get past it either.
Verified: a 200 MB / 1029x bomb is refused at ~14 MB peak RSS instead of being
materialized.
[Low] The PR body's "12 numbered items" was stale against README's list. Fixed
in the PR description; the in-repo count is synced to 18 across plugin.json,
CLAUDE.md and CHANGELOG.
Recorded as deviations 17 and 18. Counters: tools 662 -> 663 (zip_safety.py);
that module is allowlisted in smoke_exceptions.txt like its siblings.
All gates green: compileall, check_paths --all, check_dual_publish,
smoke_scripts (0 failed), derive_counters --check, check_plugin_json --all
(0 FAIL). Security auditor unchanged at 0 critical / 4 high.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017zu9Gmm9S78c2t3kDLnpPX
Addresses the automated review on PR #941.
Security (the one item flagged as wanted-before-merge): upstream defaults the
extraction workdir to a fixed `<tempdir>/book_skill_work`. On a shared host that
is CWE-377/CWE-59 — any local user can pre-create the directory in a
world-writable /tmp (the sticky bit prevents deletion, not creation) and plant a
symlink named full_text.txt or metadata.json pointing at a file the victim can
write, because Path.write_text follows symlinks. Two concurrent runs also
silently clobber each other.
- Default workdir is now a fresh `tempfile.mkdtemp(prefix="book_skill_work_")`:
unpredictable name, 0700 by construction, never shared with a concurrent run.
The path is printed and carried in metadata.json's `output_text`.
- Artifacts are written 0600, and each write refuses a symlink at the target.
- An explicit --workdir / BOOK_SKILL_WORKDIR is still honoured, but is
symlink-refused, created 0700, and chmod-tightened if it already exists.
- parsers/calibre.py no longer writes its ebook-convert scratch file to the
shared directory. That also fixes a real bug the review did not name: it read
a module-level OUTPUT_DIR constant, so the scratch file ignored --workdir
entirely and escaped the directory the caller asked for.
Verified: default workdir 0700 with 0600 artifacts and a per-invocation name;
two runs get distinct directories; a 777 --workdir is tightened to 700; a
symlinked workdir is refused; and a planted `full_text.txt -> victim` symlink is
refused with the victim file left untouched.
Also from the review:
- book_skill_validator.py and token_budget_estimator.py restated the same
BUDGETS dict. Both now import SKILL_FILE_BUDGETS / CHAPTER_TOKEN_CEILING from
book_to_skill/config.py so the two gating tools cannot drift.
- Corrected the smoke_exceptions.txt rationale: the list is "modules the G8
probe trips on", not "modules that aren't CLIs". config.py, exceptions.py,
sanitize.py, parsers/__init__.py, parsers/pdf.py and parsers/text.py are
equally not CLIs and pass only because they have no argv handling.
Recorded as deviations 15 and 16; count synced in plugin.json, CLAUDE.md and
CHANGELOG. Docs updated: the workdir path is now read from the tool's output
rather than hardcoded.
All gates green: compileall, check_paths --all, check_dual_publish,
smoke_scripts (0 failed), derive_counters --check, check_plugin_json --all
(0 FAIL). Security auditor unchanged at 0 critical / 4 high (documented).
End-to-end pipeline re-run clean: extract -> verdict -> validate -> emit.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017zu9Gmm9S78c2t3kDLnpPX
CI gate G1 (check_paths.py) failed: the agent's tool table referenced
`scripts/<tool>.py`, which resolves relative to the agent's own folder, not the
skill's. Now uses `../skills/book-to-skill/scripts/...` like cs-skill-author.
Two more issues found running the full gate set locally:
- Renamed `parsers/html.py` -> `parsers/html_text.py`. A module named `html.py`
shadows the stdlib `html` package whenever its own directory lands on
sys.path[0], and `import html.parser` then fails with "'html' is not a
package". Renaming removes the hazard rather than documenting it; two import
lines changed. Verified: HTML extraction still detects chapters, emits block
boundaries, and tab-joins table cells.
- Registered the eight vendored library modules in scripts/smoke_exceptions.txt.
They are imported as `book_to_skill.*`, never run as CLIs, so gate G8's
`--help` probe can only ever fail on them. The four real entry points are
smoke-tested normally and pass.
Recorded as deviations 13 and 14 in the plugin README; count synced in
plugin.json, CLAUDE.md and CHANGELOG.
All blocking gates green locally: compileall, check_plugin_json --all (89 OK),
check_paths --all (0 findings), check_dual_publish (0 drift), smoke_scripts
(0 failed), derive_counters --check.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017zu9Gmm9S78c2t3kDLnpPX
scripts/smoke_scripts.py runs every .py file standalone as
`python3 <file> --help`, but the vendored skillopt_sleep engine is a
package meant to be invoked only via `python -m skillopt_sleep` (already
verified working). Running any single file directly breaks two ways:
1. skillopt_sleep/types.py shadows the stdlib `types` module once the
file's own directory is prepended to sys.path, cascading into
"cannot import name 'GenericAlias'/'asdict' from partially
initialized module" for every sibling that imports
dataclasses/typing.
2. experiments/*.py use absolute `from skillopt_sleep.x import y`
imports that require the package's parent directory on sys.path,
which standalone execution doesn't provide.
Listed all 29 affected files in smoke_exceptions.txt with reasons,
following the existing autoresearch-agent/handoff precedent for
fixed-contract, non-standalone scripts.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TX374i2YGrjNV4Yi3AmaKS
