fix(backup): reject symlinks/hardlinks/device files in Python < 3.12 extractall fallback

[xxiaoxiong]

Problem (#65556)

agent/curator_backup.py has a pre-extraction path traversal check (lines 626–630) that validates member names for .. and absolute paths before calling extractall(). The Python 3.12+ path uses filter="data" which automatically rejects symlinks, hardlinks, and device nodes. The Python < 3.12 fallback path (line 634) calls tf.extractall(str(skills)) without filter="data" and without any post-validation for member types.

A malicious .tar.gz containing a symlink that points outside the extraction directory, or a hardlink/device node, would be extracted by the fallback path even though the 3.12+ path would reject it.

Fix

Replicate the filter="data" protections manually in the Python < 3.12 fallback path. Iterate members before calling extractall() and raise TarError on:

• issym() — symlinks (could point outside the extraction root)
• islnk() — hardlinks (could reference existing system files)
• Anything that is neither isfile() nor isdir() — device nodes, FIFOs, sockets

The pre-extraction path traversal check (lines 626–630) already rejects .. and absolute paths; this change adds the missing member-type validation for the fallback path.

             try:
                 tf.extractall(str(skills), filter="data")  # type: ignore[call-arg]
             except TypeError:
-                # Python < 3.12 — no filter kwarg
+                # Python < 3.12 — no filter kwarg. Replicate the same
+                # protections manually: reject symlinks, hardlinks, and
+                # device/special files in addition to the path traversal
+                # check already done above.
+                for member in tf.getmembers():
+                    if member.issym() or member.islnk():
+                        raise tarfile.TarError(
+                            f"refusing to extract symlink/hardlink: {member.name!r}"
+                        )
+                    if not member.isfile() and not member.isdir():
+                        raise tarfile.TarError(
+                            f"refusing to extract non-regular file: {member.name!r}"
+                        )
                 tf.extractall(str(skills))

Test plan

• Manual on Python 3.11: a snapshot tarball containing a symlink is rejected with the new TarError.
• Manual on Python 3.12+: same tarball is still rejected by filter="data" (path unaffected).
• Existing snapshot restore tests pass on both 3.11 and 3.12.

[alt-glitch]

This was generated by AI during triage.

Related: the curator fallback change overlaps open #23796, while the STT hunk is also proposed by #65578. These are separate surfaces bundled in one PR; please compare or split them rather than treating this as one duplicate.

[xxiaoxiong]

STT hunk was split out — now clean.

[teknium1]

Thanks for the focused fallback hardening. The premise is live on current main: agent/curator_backup.py:624-634 validates names, then the Python <3.12 branch calls unfiltered extractall().

Problems

• The new agent/curator_backup.py:638 condition is stricter than filter="data", rather than equivalent to it. Current tarfile._get_filtered_attrs permits symlink/hardlink members whose targets remain within the destination and rejects escaping targets. This can make a valid in-tree-link snapshot restore on Python 3.12+ but fail on Python 3.11.
• The diff adds no regression test. Current coverage in tests/agent/test_curator_backup.py:236-259 only exercises unsafe path names, not a link-mediated write through the forced legacy fallback.

Suggested changes

• Define whether links are intentionally unsupported in snapshots; otherwise validate their resolved targets equivalently to filter="data".
• Add a forced-TypeError fallback test proving an escaping symlink archive is rejected and staged skills are restored. Related open PR #23796 already contains a test and broader path-form validation worth comparing.

This is an automated hermes-sweeper review.

[teknium1]

filter="data" does not reject every link: it permits symlink/hardlink members whose targets resolve inside the destination. This makes Python <3.12 stricter than Python 3.12+ and can reject a snapshot that the newer path restores; please either preserve equivalent target-aware behavior or explicitly establish and test all-link rejection as the supported contract.

[egilewski]

suggesting changes

The fallback closes the demonstrated archive-link and special-file extraction paths, but it is stricter than the data filter it claims to reproduce. Hermes snapshots preserve symlinks, and an ordinary in-tree symlink that restores through filter="data" makes the old-Python fallback reject the entire rollback. The validator should distinguish links that stay inside the extraction root from absolute or escaping links so it preserves the security boundary without making valid snapshots unrestorable.

• [P3] Allow symlinks whose resolved target stays inside the snapshot
  snapshot_skills() archives the skills tree with the default dereference=False, so legitimate skill symlinks are stored as symlink members. The data filter permits a link whose target resolves inside the extraction root, but this fallback rejects every symlink and hardlink. Focused rollback validation showed that safe/link.txt -> file.txt restores through filtered extraction, while the fallback rejects the snapshot and restores the prior tree. This makes otherwise valid snapshots containing internal links unrestorable on affected runtimes.
  Remediation: Validate symlink and hardlink targets with the same target-resolution rules as tarfile.data_filter: reject absolute or escaping targets, allow targets that resolve inside the extraction root, and continue rejecting device and other special-file members. Add cases for internal, absolute, and escaping links.

Security evidence:

• trust boundary: Rollback extracts a mutable compressed tar archive into the live skills tree. Archive member names, types, metadata, and link targets are untrusted; member validation guards filesystem extraction and link or special-file creation.
• source/sink/invariant: Before unfiltered compatibility extraction, every member path and link target must remain under the live skills root, and device or special members must be rejected. Matching the data filter also requires accepting regular files, directories, and non-escaping links.
• current-main reproduction: The pre-filter fallback accepted a regular file, an escaping symlink pivot, and a FIFO, while filtered extraction accepted a regular file and an internal symlink and rejected the escaping link and FIFO.
• PR-head or patch-replay validation: The change accepted a regular file and rejected the escaping link and FIFO, but it also rejected a non-escaping safe/link.txt -> file.txt, confirming the compatibility regression.
• positive/negative cases: Positive cases covered a regular file and an internal relative symlink. Negative cases covered an escaping symlink pivot and a FIFO, with prior-tree restoration checked after rejection.
• residual bypass search: This rollback was the only tar extraction sink found in agent code. The changed predicates cover symlinks and hardlinks and reject special files; comparison with data_filter isolated the overbroad link rejection rather than another link-type bypass.
• reviewer validation: Source inspection and focused rollback validation consistently showed the unsafe baseline, the intended unsafe-member rejection, and the valid-link regression.

Not checked:

• Full test suite
• CodeRabbit review
• Native pre-filter Python runtime

Signed: GPT-5.6-sol-xhigh in Codex

[alt-glitch]

This was generated by AI during triage.

Duplicate of #23796. The current fallback guard covers the same legacy archive-member path, while #23796 validates members and preserves safe in-root links; the latest review confirms this patch rejects those legitimate links. Please consolidate on #23796.

[GottZ]

This was generated by AI during triage.

Summary

Two open PRs address the unsafe legacy tar extraction path in agent/curator_backup.py: #60007 rejects all symlink and hardlink members in the shared pre-check, while #65579 rejects links and special files only after the Python <3.12 fallback is reached. Both close the reported escape mechanism, but their blanket link rejection differs from filter="data" semantics for safe in-tree links.

Related pull requests

• fix(curator): reject symlinks in tarball pre-check for Python < 3.12 #60007 duplicate — (+6/-0) — n/a: The diff blocks every symlink and hardlink before either extraction path, addressing link-mediated traversal but also changing filtered extraction behavior, and it adds no regression test. Despite the automated keep_open verdict, contributor evidence identifies Reject unsafe curator rollback tar members #23796 as the earlier fix for the same fallback vulnerability with the requested forced-fallback test scenario.
• fix(backup): reject symlinks/hardlinks/device files in Python < 3.12 extractall fallback #65579 related — (+13/-1) — n/a: The diff adds fallback-only rejection of all symlinks, hardlinks, and special files, but the COMMENTED keep_open review and subsequent contributor review document that this is stricter than filter="data" and rejects legitimate in-tree-link snapshots. Despite that keep_open review, the complete diff retains this design flaw, while contributor evidence identifies Reject unsafe curator rollback tar members #23796 as covering the same path with target-aware link validation.

Duplicates

#60007 and #65579 substantially duplicate each other on legacy-fallback symlink and hardlink hardening; contributor discussion identifies both as downstream duplicates of #23796, with #65579 additionally rejecting special files.

Suggested consolidation

Close #60007 and #65579 as duplicates of #23796. The cross-PR evidence identifies #23796 as the earlier implementation for the same vulnerability, with the forced-TypeError regression scenario absent from these two diffs and target-aware validation that preserves safe in-root links.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    subgraph Dup60007 ["PRs duplicating each other"]
        P60007["PR #60007 (open)"]
        P65579["PR #65579 (open)"]
    end
    class P60007 open
    class P65579 open
    class P65579 target
    click P60007 "https://github.com/NousResearch/hermes-agent/pull/60007"
    click P65579 "https://github.com/NousResearch/hermes-agent/pull/65579"

Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 2 kB of PR diffs, 4 kB of issue/PR text, 5 kB of discussion (6 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

[xxiaoxiong]

Closing stale PR — superseded by upstream work / no longer relevant.