From d55b2b951ca6b27023108478f9beead9930b7ad4 Mon Sep 17 00:00:00 2001 From: Molecule AI SDK-Dev Date: Tue, 21 Apr 2026 22:03:13 +0000 Subject: [PATCH] =?UTF-8?q?fix(sdk):=20resolve=20KI-003=20=E2=80=94=20log?= =?UTF-8?q?=20warning=20for=20skipped=20symlinks=20in=20=5Fsafe=5Fextract?= =?UTF-8?q?=5Ftar?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _symlink entries in plugin tarballs are skipped (security posture, correct) but now emit a logger.warning so operators can audit what was dropped: "skipping symlink in plugin tarball (not supported for security): -> " Added test_safe_extract_logs_warning_for_skipped_symlink asserting the warning is present in caplog records at WARNING level. All 211 tests pass (+1 new). known-issues.md updated. --- known-issues.md | 23 +++++++++++++---------- molecule_agent/client.py | 14 ++++++++++++-- tests/test_remote_agent.py | 20 ++++++++++++++++++++ 3 files changed, 45 insertions(+), 12 deletions(-) diff --git a/known-issues.md b/known-issues.md index fb7ca8b..dce21a2 100644 --- a/known-issues.md +++ b/known-issues.md @@ -83,19 +83,22 @@ the server that needs to honor it. ## KI-003 — `_safe_extract_tar` silently skips all symlinks **File:** `molecule_agent/client.py:_safe_extract_tar` -**Status:** By design (security posture) +**Status:** ✅ Resolved **Severity:** Low (misleading behavior) -### Symptom -When extracting plugin tarballs, `_safe_extract_tar` silently skips any entry -that is a symlink. This means plugin tarballs that legitimately use symlinks -for shared assets (e.g., `assets/logo.png -> ../shared/logo.png`) will be -silently omitted from the extracted plugin directory with no error or warning. +### Resolution +`_safe_extract_tar` now emits a `logger.warning` for every skipped symlink: -### Impact -Some valid plugins may appear to install successfully but be missing files at -runtime. This can manifest as confusing "file not found" errors that are hard to -trace to the install step. +``` +skipping symlink in plugin tarball (not supported for security): -> +``` + +The file is still skipped (symlinks are a security risk in untrusted tarballs). +The warning lets operators audit what was dropped without changing the security +posture. + +Added `test_safe_extract_logs_warning_for_skipped_symlink` in +`tests/test_remote_agent.py` asserting the warning is emitted. ### Suggested fix Emit a `logger.warning()` for each skipped symlink so operators can see what diff --git a/molecule_agent/client.py b/molecule_agent/client.py index 2ae0bc6..8f4263e 100644 --- a/molecule_agent/client.py +++ b/molecule_agent/client.py @@ -60,7 +60,7 @@ _RETRY_JITTER_FRAC = 0.25 # ±25% jitter around base delay def _safe_extract_tar(tf: tarfile.TarFile, dest: Path) -> None: """Extract a tarfile, refusing entries that would escape `dest` - and silently skipping symlinks/hardlinks. + and logging skipped symlinks/hardlinks. Tar archives can include ``..`` and absolute paths. Without explicit rejection we'd risk the classic "tar slip" CVE — a malicious plugin @@ -68,12 +68,22 @@ def _safe_extract_tar(tf: tarfile.TarFile, dest: Path) -> None: entry's target to an absolute path, verify it lives inside dest, and extract one-by-one so the symlink-skip actually takes effect (a bare ``extractall`` would still write the symlinks we marked as skipped). + + Symlinks and hardlinks are skipped and a ``logger.warning`` is emitted + for each one so operators can audit what was dropped. """ dest_abs = dest.resolve() for member in tf.getmembers(): # Symlinks and hardlinks could point outside the staged tree; - # skip them entirely (matches the platform-side tar producer). + # skip them entirely (matches the platform-side tar producer) but + # warn so the operator knows what was dropped. if member.issym() or member.islnk(): + link_target = member.linkname + logger.warning( + "skipping symlink in plugin tarball (not supported for security): %s -> %s", + member.name, + link_target, + ) continue target = (dest / member.name).resolve() try: diff --git a/tests/test_remote_agent.py b/tests/test_remote_agent.py index f54ec81..e33ca1b 100644 --- a/tests/test_remote_agent.py +++ b/tests/test_remote_agent.py @@ -755,6 +755,26 @@ def test_safe_extract_skips_symlinks_silently(tmp_path: Path): assert not (tmp_path / "link.lnk").exists() +def test_safe_extract_logs_warning_for_skipped_symlink(tmp_path: Path, caplog): + buf = io.BytesIO() + with tarfile.open(fileobj=buf, mode="w") as tf: + sym = tarfile.TarInfo(name="link.lnk") + sym.type = tarfile.SYMTYPE + sym.linkname = "/etc/passwd" + tf.addfile(sym) + info = tarfile.TarInfo(name="real.md") + info.size = 2 + tf.addfile(info, io.BytesIO(b"ok")) + buf.seek(0) + with tarfile.open(fileobj=buf, mode="r") as tf: + _safe_extract_tar(tf, tmp_path) + assert (tmp_path / "real.md").exists() + assert not (tmp_path / "link.lnk").exists() + # A warning must be emitted so operators know what was dropped. + assert any("link.lnk" in r.message and "/etc/passwd" in r.message for r in caplog.records) + assert any(r.levelname == "WARNING" for r in caplog.records) + + # --------------------------------------------------------------------------- # verify_plugin_sha256 + content integrity # ---------------------------------------------------------------------------