fix: protect user skills from dream writes

fix: protect user skills from dream writes in HKUDS/nanobot.

Open

hamb1ywants to merge intofrom

3 commits5 files changed+175-5

Summary

add a Dream-only write guard for workspace skills
require dream_managed: true frontmatter before Dream can modify existing skills
allow Dream-created skills only when the initial SKILL.md carries that marker

Fixes #4075

Verification

uv run pytest tests/agent/test_memory_store.py::TestMemoryStoreBasicIO::test_dream_write_tools_block_unmarked_user_skill tests/agent/test_memory_store.py::TestMemoryStoreBasicIO::test_dream_write_tools_allow_marked_skill tests/agent/test_memory_store.py::TestMemoryStoreBasicIO::test_dream_can_create_only_marked_skill -q
uv run ruff check nanobot/agent/memory.py nanobot/agent/tools/filesystem.py nanobot/agent/tools/apply_patch.py tests/agent/test_memory_store.py

Activity

fix: protect user skills from dream writes

fac00f6Jul 2

test: update dream skill ownership expectations

c55b7e7Jul 2

hamb1y-bot-hkuds-nanobot4mo ago

Review — fix: protect user skills from dream writes

What this PR does: Adds a write_guard: Callable[[Path, str | None], str | None] | None parameter to WriteFileTool, EditFileTool, and ApplyPatchTool. The guard returns None to allow or a string message to deny (raised as PermissionError). MemoryStore._dream_skill_write_guard enforces:

If target skill's SKILL.md already exists: must have dream_managed: true in YAML frontmatter.
If creating new SKILL.md: the content being written must have dream_managed: true frontmatter.
Path must be inside a named skill directory (len(parts) >= 2).

Guard is checked in all three tools' execute paths, plus ApplyPatchTool checks each write in the patch.

What's good

Ownership enforced at the write sink, not in prompts. This is the core fix #4075 asked for. Even if Dream's prompt is bypassed or the model hallucinates, the tool itself rejects the write.
Frontmatter parsing is conservative. Requires --- on first line, scans until closing ---, looks for dream_managed: true (lowercase). BOM-stripped. Doesn't accept quoted "true" or True — strict.
Allows Dream to create new marked skills. test_dream_can_create_only_marked_skill verifies unmarked creation is blocked, marked creation succeeds. Correct positive path.
All three write tools guarded. No bypass via apply_patch or edit_file.
ApplyPatchTool checks each write in the patch (line 268: for path, content in writes.items(): self._check_write_guard(path, content)). Catches multi-file patches that include skill files.
Existing dream tests updated to include dream_managed: true in their fixture content. Backwards-compatible with the test suite.

Concerns / questions

dream_managed: "true" (quoted) is NOT recognized. _skill_content_is_dream_managed does value.strip().lower() == "true" — quoted values would be '"true"' after strip, which fails the check. Users writing YAML frontmatter with quoted strings would be surprised. Either document "must be unquoted true" or accept quoted forms.
dream_managed: True (YAML boolean) IS recognized because value.strip().lower() == "true" accepts case-insensitively. Good.
No support for dream_managed: 1 or other truthy values. Probably fine — strictness is good for a security marker.
_dream_skill_write_guard returns None (allow) if path can't be resolved relative to skills_dir. This is safe because the tool's own allowed_dir check still applies — the guard is layered ON TOP of existing path restrictions, not a replacement. But it means the guard doesn't FAIL CLOSED on unexpected errors. For a security boundary, failing closed (return "could not verify ownership") might be safer. Worth discussing.
Frontmatter parser doesn't handle multi-line values or nested keys. For the current single-key marker, this is fine. If the marker schema grows, switch to a real YAML parser.
No test for the BOM case. The lstrip("\ufeff") is defensive but untested. Minor.
No test for traversal-style attacks like skills/../user-skill/SKILL.md. The target.relative_to(skills_dir) would resolve this correctly, but a test would lock it in.

Verdict

Approve with follow-up. The security boundary is correctly enforced. The quoted-"true" and fail-closed questions are worth a follow-up issue but don't block merge.

hamb1y-bot-hkuds-nanobot4mo ago

Review — fix: protect user skills from dream writes

If target skill's SKILL.md already exists: must have dream_managed: true in YAML frontmatter.
If creating new SKILL.md: the content being written must have dream_managed: true frontmatter.
Path must be inside a named skill directory (len(parts) >= 2).

Guard is checked in all three tools' execute paths, plus ApplyPatchTool checks each write in the patch.

What's good

Ownership enforced at the write sink, not in prompts. This is the core fix #4075 asked for. Even if Dream's prompt is bypassed or the model hallucinates, the tool itself rejects the write.
Frontmatter parsing is conservative. Requires --- on first line, scans until closing ---, looks for dream_managed: true (lowercase). BOM-stripped. Doesn't accept quoted "true" or True — strict.
Allows Dream to create new marked skills. test_dream_can_create_only_marked_skill verifies unmarked creation is blocked, marked creation succeeds. Correct positive path.
All three write tools guarded. No bypass via apply_patch or edit_file.
ApplyPatchTool checks each write in the patch (line 268: for path, content in writes.items(): self._check_write_guard(path, content)). Catches multi-file patches that include skill files.
Existing dream tests updated to include dream_managed: true in their fixture content. Backwards-compatible with the test suite.

Concerns / questions

dream_managed: "true" (quoted) is NOT recognized. _skill_content_is_dream_managed does value.strip().lower() == "true" — quoted values would be '"true"' after strip, which fails the check. Users writing YAML frontmatter with quoted strings would be surprised. Either document "must be unquoted true" or accept quoted forms.
dream_managed: True (YAML boolean) IS recognized because value.strip().lower() == "true" accepts case-insensitively. Good.
No support for dream_managed: 1 or other truthy values. Probably fine — strictness is good for a security marker.
_dream_skill_write_guard returns None (allow) if path can't be resolved relative to skills_dir. This is safe because the tool's own allowed_dir check still applies — the guard is layered ON TOP of existing path restrictions, not a replacement. But it means the guard doesn't FAIL CLOSED on unexpected errors. For a security boundary, failing closed (return "could not verify ownership") might be safer. Worth discussing.
Frontmatter parser doesn't handle multi-line values or nested keys. For the current single-key marker, this is fine. If the marker schema grows, switch to a real YAML parser.
No test for the BOM case. The lstrip("\ufeff") is defensive but untested. Minor.
No test for traversal-style attacks like skills/../user-skill/SKILL.md. The target.relative_to(skills_dir) would resolve this correctly, but a test would lock it in.

Verdict

Approve with follow-up. The security boundary is correctly enforced. The quoted-"true" and fail-closed questions are worth a follow-up issue but don't block merge.

chengyongrureviewed4mo ago

chengyongru4mo ago

Automated PR review by nanobot. This is not a human maintainer review or approval.

I found one ownership-boundary issue worth addressing.

hamb1yreviewed4mo ago

fix: require top-level dream ownership marker

4f2677fJul 2

hamb1yreviewed4mo ago