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 # ---------------------------------------------------------------------------