diff --git a/graphify/cli.py b/graphify/cli.py index 2700f68df..e51283ba9 100644 --- a/graphify/cli.py +++ b/graphify/cli.py @@ -3040,11 +3040,23 @@ def dispatch_command(cmd: str) -> None: chunk_files.extend(sorted(expanded) if expanded else [arg]) merged: dict = {"nodes": [], "edges": [], "hyperedges": [], "input_tokens": 0, "output_tokens": 0} seen_ids: set[str] = set() + # These chunk files are untrusted subagent output. load_validated_... + # stats the file size BEFORE reading it (so a multi-GB chunk can't blow up + # memory), parses the JSON, and validates the security caps + the node/ + # edge id charset that blocks path traversal (#825) — the same enforcement + # the skill merge path applies. A bad chunk is skipped with a warning + # (filter semantics; never abort). Deliberately NOT wired into + # build_from_json/load_graph_json, which must keep loading valid + # pre-existing graphs. file_type is left to build's coercion (#840). + from graphify.semantic_cleanup import load_validated_semantic_fragment for cf in chunk_files: - try: - chunk = json.loads(Path(cf).read_text(encoding="utf-8")) - except (json.JSONDecodeError, OSError) as exc: - print(f"[graphify merge-chunks] warning: skipping {cf}: {exc}", file=sys.stderr) + chunk, _chunk_errs = load_validated_semantic_fragment(Path(cf)) + if _chunk_errs: + print( + f"[graphify merge-chunks] warning: skipping invalid chunk {cf}: " + f"{'; '.join(_chunk_errs[:3])}", + file=sys.stderr, + ) continue for n in chunk.get("nodes", []): if n.get("id") not in seen_ids: diff --git a/graphify/semantic_cleanup.py b/graphify/semantic_cleanup.py index 0dd74dc76..09cac2d84 100644 --- a/graphify/semantic_cleanup.py +++ b/graphify/semantic_cleanup.py @@ -1,11 +1,13 @@ # Semantic fragment sanitizer — converts sentence-like rationale nodes into # attributes on related nodes and removes invalid file_type values. # -# Currently called from the skill merge scripts (skill-opencode.md, -# skill-codex.md) so that rationale text never leaks into the knowledge -# graph as standalone nodes. (Future: graphify.llm may wire this into -# _parse_llm_json / _merge_into for non-skill code paths; not done in -# this cycle.) +# Called from the skill merge path (see skill-devin.md) and from the in-process +# `graphify merge-chunks` command — both ingest untrusted agent-written chunk +# JSON, and validate_semantic_fragment() rejects malformed/oversized payloads and +# crafted node/edge IDs before they touch the graph. The primary build/load paths +# (build_from_json, load_graph_json) deliberately do NOT run this: they must keep +# loading valid pre-existing graphs whose AST node IDs predate the stricter +# semantic-ID charset. from __future__ import annotations import json @@ -31,7 +33,12 @@ MAX_SEMANTIC_FRAGMENT_HYPEREDGES = 10_000 MAX_SEMANTIC_HYPEREDGE_NODES = 256 MAX_SEMANTIC_ID_LENGTH = 256 VALID_SEMANTIC_FILE_TYPES = frozenset({"code", "document", "paper", "image", "rationale", "concept"}) -_SEMANTIC_ID_RE = re.compile(r"^[A-Za-z0-9._:-]+$") +# Unicode word characters are allowed: build's normalize_id preserves CJK / +# Cyrillic / accented-Latin identifiers, so an ASCII-only gate would reject valid +# ids that the loader accepts. The explicit path-separator / ".." check in +# _validate_semantic_id still blocks directory escape (#825); "/", "\\", spaces, +# "@", "#" etc. are not \w and remain rejected. +_SEMANTIC_ID_RE = re.compile(r"^[\w.:-]+$") def validate_semantic_fragment(fragment: object) -> list[str]: @@ -74,12 +81,12 @@ def validate_semantic_fragment(fragment: object) -> list[str]: errors.append(f"nodes[{i}] must be an object") continue _validate_semantic_id(errors, f"nodes[{i}].id", node.get("id")) - file_type = node.get("file_type") - if file_type is not None and file_type not in VALID_SEMANTIC_FILE_TYPES: - errors.append( - f"nodes[{i}].file_type {file_type!r} is not one of " - f"{sorted(VALID_SEMANTIC_FILE_TYPES)}" - ) # validate file_type before any sanitize path can run + # file_type is intentionally NOT rejected here. It carries no security + # risk (it can't exhaust memory or escape a directory), and + # build_from_json already coerces every value via _FILE_TYPE_SYNONYMS + # (unknown -> "concept", #840). Rejecting a whole chunk over a synonym + # like "markdown"/"tool"/"framework" that the loader would happily map is + # pure data loss, so leave file_type normalization to build. for i, edge in enumerate(edges): if not isinstance(edge, dict): diff --git a/tests/test_merge_chunks_validation.py b/tests/test_merge_chunks_validation.py new file mode 100644 index 000000000..ca9f5eabb --- /dev/null +++ b/tests/test_merge_chunks_validation.py @@ -0,0 +1,104 @@ +"""Tests that `graphify merge-chunks` validates untrusted subagent chunk JSON. + +merge-chunks concatenates agent-written `.graphify_chunk_*.json` files. Those are +untrusted output, so each is run through `validate_semantic_fragment` (caps + the +node/edge ID charset that blocks path-escape). An invalid chunk is skipped with a +warning; valid chunks still merge. +""" +import json + +import graphify.__main__ as mainmod + + +def _write(path, obj): + path.write_text(json.dumps(obj), encoding="utf-8") + + +def _run_merge(monkeypatch, argv): + monkeypatch.setattr(mainmod, "_check_skill_version", lambda _: None) + monkeypatch.setattr(mainmod.sys, "argv", argv) + mainmod.main() + + +def test_merge_chunks_skips_chunk_with_path_escape_id(tmp_path, monkeypatch, capsys): + good = tmp_path / ".graphify_chunk_0.json" + _write(good, {"nodes": [{"id": "pkg.mod.good", "label": "G"}], "edges": [], "hyperedges": []}) + bad = tmp_path / ".graphify_chunk_1.json" + # A node id with a path separator would escape the chunk directory (#825). + _write(bad, {"nodes": [{"id": "../../etc/passwd", "label": "B"}], "edges": [], "hyperedges": []}) + out = tmp_path / "merged.json" + + _run_merge(monkeypatch, ["graphify", "merge-chunks", str(good), str(bad), "--out", str(out)]) + + merged = json.loads(out.read_text()) + assert {n["id"] for n in merged["nodes"]} == {"pkg.mod.good"} + assert "skipping invalid chunk" in capsys.readouterr().err + + +def test_merge_chunks_skips_malformed_shape(tmp_path, monkeypatch, capsys): + bad = tmp_path / ".graphify_chunk_0.json" + _write(bad, {"nodes": "not-a-list", "edges": []}) + out = tmp_path / "merged.json" + + _run_merge(monkeypatch, ["graphify", "merge-chunks", str(bad), "--out", str(out)]) + + merged = json.loads(out.read_text()) + assert merged["nodes"] == [] + assert "skipping invalid chunk" in capsys.readouterr().err + + +def test_merge_chunks_accepts_synonym_file_type(tmp_path, monkeypatch): + # file_type synonyms (markdown/tool/framework/...) are coerced by build, not + # a validation failure — the chunk must merge, not be silently dropped (#840). + c = tmp_path / ".graphify_chunk_0.json" + _write(c, {"nodes": [{"id": "pkg.readme", "label": "Readme", "file_type": "markdown"}, + {"id": "pkg.tool", "label": "Tool", "file_type": "tool"}], + "edges": [], "hyperedges": []}) + out = tmp_path / "merged.json" + _run_merge(monkeypatch, ["graphify", "merge-chunks", str(c), "--out", str(out)]) + merged = json.loads(out.read_text()) + assert {n["id"] for n in merged["nodes"]} == {"pkg.readme", "pkg.tool"} + + +def test_merge_chunks_accepts_unicode_id(tmp_path, monkeypatch): + # build's normalize_id preserves Unicode identifiers; validation must not + # reject a chunk that uses them. + c = tmp_path / ".graphify_chunk_0.json" + _write(c, {"nodes": [{"id": "mod_处理数据", "label": "handler", "file_type": "code"}], + "edges": [], "hyperedges": []}) + out = tmp_path / "merged.json" + _run_merge(monkeypatch, ["graphify", "merge-chunks", str(c), "--out", str(out)]) + merged = json.loads(out.read_text()) + assert {n["id"] for n in merged["nodes"]} == {"mod_处理数据"} + + +def test_validate_semantic_fragment_accepts_synonyms_and_unicode(): + from graphify.semantic_cleanup import validate_semantic_fragment + frag = {"nodes": [{"id": "mod_处理", "file_type": "markdown"}, + {"id": "a.b::C.d", "file_type": "tool"}], + "edges": [], "hyperedges": []} + assert validate_semantic_fragment(frag) == [] + + +def test_validate_semantic_fragment_still_blocks_path_escape(): + from graphify.semantic_cleanup import validate_semantic_fragment + errs = validate_semantic_fragment({"nodes": [{"id": "../../etc/passwd"}], + "edges": [], "hyperedges": []}) + assert errs + + +def test_merge_chunks_merges_valid_chunks(tmp_path, monkeypatch): + c0 = tmp_path / ".graphify_chunk_0.json" + _write(c0, {"nodes": [{"id": "a", "label": "A"}], "edges": [], "hyperedges": [], + "input_tokens": 10, "output_tokens": 5}) + c1 = tmp_path / ".graphify_chunk_1.json" + _write(c1, {"nodes": [{"id": "b", "label": "B"}], "edges": [], "hyperedges": [], + "input_tokens": 7, "output_tokens": 3}) + out = tmp_path / "merged.json" + + _run_merge(monkeypatch, ["graphify", "merge-chunks", str(c0), str(c1), "--out", str(out)]) + + merged = json.loads(out.read_text()) + assert {n["id"] for n in merged["nodes"]} == {"a", "b"} + assert merged["input_tokens"] == 17 + assert merged["output_tokens"] == 8 diff --git a/tests/test_semantic_cleanup.py b/tests/test_semantic_cleanup.py index 49fd03398..de13bd245 100644 --- a/tests/test_semantic_cleanup.py +++ b/tests/test_semantic_cleanup.py @@ -51,11 +51,15 @@ def test_validate_semantic_fragment_rejects_path_separator_in_id(): assert any("nodes[0].id" in e for e in errors) -def test_validate_semantic_fragment_rejects_invalid_file_type(): +def test_validate_semantic_fragment_accepts_unknown_file_type(): + """An unknown/synonym file_type is NOT a validation failure: build_from_json + coerces any value via _FILE_TYPE_SYNONYMS (unknown -> "concept", #840), so + rejecting a whole chunk over it would be pure data loss. file_type carries no + security risk, so it is left to build's coercion rather than gated here.""" fragment = _valid_fragment() fragment["nodes"][0]["file_type"] = "executable" errors = sc.validate_semantic_fragment(fragment) - assert any("file_type" in e for e in errors) + assert not any("file_type" in e for e in errors) def test_validate_semantic_fragment_accepts_rationale_file_type():