Skip to content

Commit 95f818a

Browse files
iamaeroplaneclaude
andcommitted
Address remaining Copilot security and logic review feedback
- Fix Zip Slip vulnerability by using relative_to() for safe path validation - Add HTTPS validation for extension download URLs - Backup both *-config.yml and *-config.local.yml files on remove - Normalize boolean values to lowercase for hook condition comparisons - Show non-default catalog warning only once per instance Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent 42b869a commit 95f818a

1 file changed

Lines changed: 37 additions & 11 deletions

File tree

src/specify_cli/extensions.py

Lines changed: 37 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -390,13 +390,18 @@ def install_from_zip(
390390

391391
# Extract ZIP safely (prevent Zip Slip attack)
392392
with zipfile.ZipFile(zip_path, 'r') as zf:
393+
# Validate all paths first before extracting anything
394+
temp_path_resolved = temp_path.resolve()
393395
for member in zf.namelist():
394-
# Resolve the target path and ensure it's within temp_path
395396
member_path = (temp_path / member).resolve()
396-
if not str(member_path).startswith(str(temp_path.resolve())):
397+
# Use is_relative_to for safe path containment check
398+
try:
399+
member_path.relative_to(temp_path_resolved)
400+
except ValueError:
397401
raise ValidationError(
398402
f"Unsafe path in ZIP archive: {member} (potential path traversal)"
399403
)
404+
# Only extract after all paths are validated
400405
zf.extractall(temp_path)
401406

402407
# Find extension directory (may be nested)
@@ -472,7 +477,10 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool:
472477
backup_dir = self.extensions_dir / ".backup" / extension_id
473478
backup_dir.mkdir(parents=True, exist_ok=True)
474479

475-
config_files = list(extension_dir.glob("*-config.yml"))
480+
# Backup both primary and local override config files
481+
config_files = list(extension_dir.glob("*-config.yml")) + list(
482+
extension_dir.glob("*-config.local.yml")
483+
)
476484
for config_file in config_files:
477485
backup_path = backup_dir / config_file.name
478486
shutil.copy2(config_file, backup_path)
@@ -982,13 +990,15 @@ def get_catalog_url(self) -> str:
982990
"Invalid SPECKIT_CATALOG_URL: must be a valid URL with a host."
983991
)
984992

985-
# Warn users when using a non-default catalog
993+
# Warn users when using a non-default catalog (once per instance)
986994
if catalog_url != self.DEFAULT_CATALOG_URL:
987-
print(
988-
"Warning: Using non-default extension catalog. "
989-
"Only use catalogs from sources you trust.",
990-
file=sys.stderr,
991-
)
995+
if not getattr(self, "_non_default_catalog_warning_shown", False):
996+
print(
997+
"Warning: Using non-default extension catalog. "
998+
"Only use catalogs from sources you trust.",
999+
file=sys.stderr,
1000+
)
1001+
self._non_default_catalog_warning_shown = True
9921002

9931003
return catalog_url
9941004

@@ -1158,6 +1168,15 @@ def download_extension(self, extension_id: str, target_dir: Optional[Path] = Non
11581168
if not download_url:
11591169
raise ExtensionError(f"Extension '{extension_id}' has no download URL")
11601170

1171+
# Validate download URL requires HTTPS (prevent man-in-the-middle attacks)
1172+
from urllib.parse import urlparse
1173+
parsed = urlparse(download_url)
1174+
is_localhost = parsed.hostname in ("localhost", "127.0.0.1", "::1")
1175+
if parsed.scheme != "https" and not (parsed.scheme == "http" and is_localhost):
1176+
raise ExtensionError(
1177+
f"Extension download URL must use HTTPS: {download_url}"
1178+
)
1179+
11611180
# Determine target path
11621181
if target_dir is None:
11631182
target_dir = self.cache_dir / "downloads"
@@ -1587,10 +1606,17 @@ def _evaluate_condition(self, condition: str, extension_id: Optional[str]) -> bo
15871606
config_manager = ConfigManager(self.project_root, extension_id)
15881607
actual_value = config_manager.get_value(key_path)
15891608

1609+
# Normalize boolean values to lowercase for comparison
1610+
# (YAML True/False vs condition strings 'true'/'false')
1611+
if isinstance(actual_value, bool):
1612+
normalized_value = "true" if actual_value else "false"
1613+
else:
1614+
normalized_value = str(actual_value)
1615+
15901616
if operator == "==":
1591-
return str(actual_value) == expected_value
1617+
return normalized_value == expected_value
15921618
else: # !=
1593-
return str(actual_value) != expected_value
1619+
return normalized_value != expected_value
15941620

15951621
# Pattern: "env.VAR_NAME is set"
15961622
if match := re.match(r'env\.([A-Z0-9_]+)\s+is\s+set', condition, re.IGNORECASE):

0 commit comments

Comments
 (0)