fix(sdk): resolve KI-003 — log warning for skipped symlinks in _safe_extract_tar
_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): <name> -> <target>" 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.
This commit is contained in:
parent
19653f85c8
commit
d55b2b951c
@ -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): <name> -> <target>
|
||||
```
|
||||
|
||||
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
|
||||
|
||||
@ -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:
|
||||
|
||||
@ -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
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
Loading…
Reference in New Issue
Block a user