Skip to content

fix(merge-chunks): validate untrusted subagent chunk JSON before merging#1953

Closed
TPAteeq wants to merge 1 commit into
Graphify-Labs:v8from
TPAteeq:fix/validate-merge-chunks
Closed

fix(merge-chunks): validate untrusted subagent chunk JSON before merging#1953
TPAteeq wants to merge 1 commit into
Graphify-Labs:v8from
TPAteeq:fix/validate-merge-chunks

Conversation

@TPAteeq

@TPAteeq TPAteeq commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Problem

graphify merge-chunks concatenates agent-written .graphify_chunk_*.json files into one graph with only a JSON-decode guard around each file. Those chunks are untrusted output from semantic subagents, so an oversized or runaway payload, or a crafted node/edge id containing a path traversal (../../etc/passwd), flowed straight into the merged graph with nothing stopping it. The project already has a security-capping validator for exactly this threat model (#825 — memory exhaustion and chunk-directory escape via crafted IDs), but it was wired only into the skill merge path, not into the in-process merge-chunks command that feeds build_from_json.

Fix

Route each chunk through load_validated_semantic_fragment, which stats the file size before reading it (so a multi-gigabyte chunk can't blow up memory at read time), parses the JSON, and validates the byte/count caps plus the node/edge/hyperedge id charset that blocks path separators and ... An invalid chunk is skipped with a warning (filter semantics — one bad chunk never aborts the merge). This validation is deliberately not wired into build_from_json / load_graph_json: those must keep loading valid, pre-existing graphs whose AST node ids predate the stricter semantic-id charset, and gating them would reject legitimate graphs.

While wiring this in, review caught that the shared validator was too strict in two ways that would have silently dropped whole legitimate chunks, so both were relaxed (a relaxation only — never a new rejection):

The stale module docstring, which named the wrong skills, was corrected.

Tests

tests/test_merge_chunks_validation.py and an update to tests/test_semantic_cleanup.py: path-escape id rejected; malformed shape rejected; valid chunks merge; a file_type: "markdown" / "tool" chunk now merges (regression guard for the data-loss blocker); a Unicode-id chunk merges; and the validator relaxation plus the still-active path-escape guard at the unit level. Full test suite green (3290 passed).

`graphify merge-chunks` concatenates agent-written `.graphify_chunk_*.json`
files with only a JSON-decode guard, so an oversized payload or a crafted
node/edge id (e.g. `../../etc/passwd`) flowed straight into the merged graph.

Route each chunk through `load_validated_semantic_fragment`, which stats the
file size BEFORE reading it (a multi-GB chunk can't blow up memory), parses the
JSON, and validates the byte/count caps + the node/edge id charset that blocks
path traversal (Graphify-Labs#825). An invalid chunk is skipped with a warning (filter
semantics; never abort). Left OUT of build_from_json/load_graph_json on purpose:
those must keep loading valid pre-existing graphs.

Also relax two over-strict checks in the shared validator that would otherwise
silently drop whole legitimate chunks (a relaxation — never a new rejection):
- file_type is no longer gated: build coerces any value via _FILE_TYPE_SYNONYMS
  (unknown -> "concept", Graphify-Labs#840), so synonyms like "markdown"/"tool" the loader
  maps must not fail validation.
- the id charset now allows Unicode word chars (build's normalize_id preserves
  CJK/Cyrillic/accented-Latin ids); the explicit path-separator/".." check still
  blocks directory escape.
Corrects the stale module docstring (the validator serves the devin skill path
and merge-chunks, not skill-opencode/codex).
safishamsi added a commit that referenced this pull request Jul 16, 2026
…#1953)

An untrusted chunk with a non-numeric input_tokens/output_tokens would abort
the whole merge with a TypeError after other chunks had already merged. Coerce
to 0 so a bad token field can't defeat the per-chunk validation guard.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
safishamsi added a commit that referenced this pull request Jul 16, 2026
…angelog

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@safishamsi

Copy link
Copy Markdown
Collaborator

Landed on v8 (ships in 0.9.18), cherry-picked so your authorship is preserved. Thanks @TPAteeq — merge-chunks now validates each untrusted chunk (reusing the #825 fragment validator) and skips a bad one with a warning instead of aborting the whole merge or passing a path-escape id through. Small follow-up added on top: non-numeric token counts are coerced so they cannot TypeError the merge after other chunks have merged.

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