Skip to content

Commit 0fd0bf6

Browse files
Copilotmnriem
andauthored
Catch TarError/OSError in _safe_extract_tarball; rename zip_path to archive_path in extension_update
Agent-Logs-Url: https://github.com/github/spec-kit/sessions/953d7f62-a75a-4690-90a9-98345cae824d Co-authored-by: mnriem <15701806+mnriem@users.noreply.github.com>
1 parent d00509e commit 0fd0bf6

2 files changed

Lines changed: 52 additions & 47 deletions

File tree

src/specify_cli/__init__.py

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4326,18 +4326,18 @@ def extension_update(
43264326
backup_hooks[hook_name] = ext_hooks
43274327

43284328
# 5. Download new version
4329-
zip_path = catalog.download_extension(extension_id)
4329+
archive_path = catalog.download_extension(extension_id)
43304330
try:
43314331
# 6. Validate extension ID from archive BEFORE modifying installation
43324332
# Handle both root-level and nested extension.yml (GitHub auto-generated archives)
43334333
from .extensions import _detect_archive_format
43344334
import tarfile
4335-
archive_fmt = _detect_archive_format(str(zip_path))
4335+
archive_fmt = _detect_archive_format(str(archive_path))
43364336
import yaml
43374337
manifest_data = None
43384338

43394339
if archive_fmt == "tar.gz":
4340-
with tarfile.open(zip_path, "r:gz") as tf:
4340+
with tarfile.open(archive_path, "r:gz") as tf:
43414341
# First try root-level extension.yml
43424342
try:
43434343
m = tf.getmember("extension.yml")
@@ -4354,7 +4354,7 @@ def extension_update(
43544354
with f:
43554355
manifest_data = yaml.safe_load(f.read()) or {}
43564356
else:
4357-
with zipfile.ZipFile(zip_path, "r") as zf:
4357+
with zipfile.ZipFile(archive_path, "r") as zf:
43584358
namelist = zf.namelist()
43594359

43604360
# First try root-level extension.yml
@@ -4382,7 +4382,7 @@ def extension_update(
43824382
manager.remove(extension_id, keep_config=True)
43834383

43844384
# 8. Install new version
4385-
_ = manager.install_from_zip(zip_path, speckit_version)
4385+
_ = manager.install_from_zip(archive_path, speckit_version)
43864386

43874387
# Restore user config files from backup after successful install.
43884388
new_extension_dir = manager.extensions_dir / extension_id
@@ -4428,9 +4428,9 @@ def extension_update(
44284428
hook["enabled"] = False
44294429
hook_executor.save_project_config(config)
44304430
finally:
4431-
# Clean up downloaded ZIP
4432-
if zip_path.exists():
4433-
zip_path.unlink()
4431+
# Clean up downloaded archive
4432+
if archive_path.exists():
4433+
archive_path.unlink()
44344434

44354435
# 10. Clean up backup on success
44364436
if backup_base.exists():

src/specify_cli/extensions.py

Lines changed: 44 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -171,50 +171,55 @@ def _safe_extract_tarball(
171171
"""
172172
dest_resolved = dest_dir.resolve()
173173

174-
with tarfile.open(archive_path, "r:gz") as tf:
175-
members = tf.getmembers()
176-
safe_members = []
177-
178-
# Validate every member before extracting anything.
179-
for member in members:
180-
# Reject absolute paths and any path component that is "..".
181-
if os.path.isabs(member.name) or any(
182-
part == ".." for part in member.name.replace("\\", "/").split("/")
183-
):
184-
raise error_class(
185-
f"Unsafe path in tar archive: {member.name} (potential path traversal)"
186-
)
174+
try:
175+
with tarfile.open(archive_path, "r:gz") as tf:
176+
members = tf.getmembers()
177+
safe_members = []
178+
179+
# Validate every member before extracting anything.
180+
for member in members:
181+
# Reject absolute paths and any path component that is "..".
182+
if os.path.isabs(member.name) or any(
183+
part == ".." for part in member.name.replace("\\", "/").split("/")
184+
):
185+
raise error_class(
186+
f"Unsafe path in tar archive: {member.name} (potential path traversal)"
187+
)
187188

188-
# Confirm the resolved path stays inside dest_dir.
189-
member_path = (dest_dir / member.name).resolve()
190-
try:
191-
member_path.relative_to(dest_resolved)
192-
except ValueError:
193-
raise error_class(
194-
f"Unsafe path in tar archive: {member.name} (potential path traversal)"
195-
)
189+
# Confirm the resolved path stays inside dest_dir.
190+
member_path = (dest_dir / member.name).resolve()
191+
try:
192+
member_path.relative_to(dest_resolved)
193+
except ValueError:
194+
raise error_class(
195+
f"Unsafe path in tar archive: {member.name} (potential path traversal)"
196+
)
196197

197-
# Reject symlinks and hard links.
198-
if member.issym() or member.islnk():
199-
raise error_class(
200-
f"Symlinks are not allowed in archive: {member.name}"
201-
)
198+
# Reject symlinks and hard links.
199+
if member.issym() or member.islnk():
200+
raise error_class(
201+
f"Symlinks are not allowed in archive: {member.name}"
202+
)
202203

203-
# Only allow regular files and directories.
204-
if not (member.isreg() or member.isdir()):
205-
raise error_class(
206-
f"Non-regular file in archive: {member.name}"
207-
)
204+
# Only allow regular files and directories.
205+
if not (member.isreg() or member.isdir()):
206+
raise error_class(
207+
f"Non-regular file in archive: {member.name}"
208+
)
208209

209-
safe_members.append(member)
210+
safe_members.append(member)
210211

211-
# Extract — use the "data" filter on Python 3.12+ for extra hardening.
212-
# On older versions pass only the pre-validated members so that no
213-
# unvetted entry (added concurrently or via a race) slips through.
214-
if sys.version_info >= (3, 12):
215-
tf.extractall(dest_dir, filter="data") # type: ignore[call-arg]
216-
else:
217-
tf.extractall(dest_dir, members=safe_members) # noqa: S202 — validated above
212+
# Extract — use the "data" filter on Python 3.12+ for extra hardening.
213+
# On older versions pass only the pre-validated members so that no
214+
# unvetted entry (added concurrently or via a race) slips through.
215+
if sys.version_info >= (3, 12):
216+
tf.extractall(dest_dir, filter="data") # type: ignore[call-arg]
217+
else:
218+
tf.extractall(dest_dir, members=safe_members) # noqa: S202 — validated above
219+
except error_class:
220+
raise
221+
except (tarfile.TarError, OSError) as e:
222+
raise error_class(f"Failed to read archive {archive_path}: {e}") from e
218223

219224

220225
@dataclass

0 commit comments

Comments
 (0)