From 1e0b2c2edd17e87d307054e16e5c289df285a2ad Mon Sep 17 00:00:00 2001 From: arika0093 Date: Tue, 14 Apr 2026 04:47:00 +0000 Subject: [PATCH 01/10] feat: support allow-insecure HTTP dependencies - add install/config handling for HTTP dependencies behind explicit opt-in - preserve apm.yml manifest consistency by using git: entries - update tests and documentation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/src/content/docs/guides/dependencies.md | 10 +- .../content/docs/reference/cli-commands.md | 19 +++- .../.apm/skills/apm-usage/commands.md | 2 +- src/apm_cli/commands/config.py | 95 +++++++++++----- src/apm_cli/commands/install.py | 86 +++++++++++++- src/apm_cli/config.py | 18 +++ src/apm_cli/deps/github_downloader.py | 11 +- src/apm_cli/models/dependency/reference.py | 47 +++++++- tests/unit/test_canonicalization.py | 106 ++++++++++++++++++ tests/unit/test_config_command.py | 54 +++++++++ tests/unit/test_install_command.py | 83 ++++++++++++++ 11 files changed, 488 insertions(+), 43 deletions(-) diff --git a/docs/src/content/docs/guides/dependencies.md b/docs/src/content/docs/guides/dependencies.md index 91b37104..267a924e 100644 --- a/docs/src/content/docs/guides/dependencies.md +++ b/docs/src/content/docs/guides/dependencies.md @@ -141,7 +141,15 @@ dependencies: alias: review # local alias (controls install directory name) ``` -Fields: `git` (required), `path`, `ref`, `alias` (all optional). The `git` value is any HTTPS or SSH clone URL. +Fields: `git` (required), `path`, `ref`, `alias` (all optional). The `git` value is any HTTPS, HTTP or SSH clone URL. + +:::caution +Use HTTP dependencies only on trusted private networks. Declare them with +`git: http://...` and `allow_insecure: true` in `apm.yml`. Installing them +still requires `apm install --allow-insecure`, unless +`apm config set allow-insecure true` is enabled globally. +::: + > **Nested groups (GitLab, Gitea, etc.):** APM treats all path segments after the host as the repo path, so `gitlab.com/group/subgroup/repo` resolves to a repo at `group/subgroup/repo`. Virtual paths on simple 2-segment repos work with shorthand (`gitlab.com/owner/repo/file.prompt.md`). But for **nested-group repos + virtual paths**, use the object format — the shorthand is ambiguous: > diff --git a/docs/src/content/docs/reference/cli-commands.md b/docs/src/content/docs/reference/cli-commands.md index 438ded4f..9c0a922f 100644 --- a/docs/src/content/docs/reference/cli-commands.md +++ b/docs/src/content/docs/reference/cli-commands.md @@ -96,6 +96,7 @@ apm install [PACKAGES...] [OPTIONS] - `--trust-transitive-mcp` - Trust self-defined MCP servers from transitive packages (skip re-declaration requirement) - `--dev` - Add packages to [`devDependencies`](../manifest-schema/#5-devdependencies) instead of `dependencies`. Dev deps are installed locally but excluded from `apm pack --format plugin` bundles - `-g, --global` - Install to user scope (`~/.apm/`) instead of the current project. Primitives deploy to `~/.copilot/`, `~/.claude/`, etc. +- `--allow-insecure` - Allow HTTP (insecure) dependencies. Required when adding or installing dependencies that use an `http://` URL. **Behavior:** - `apm install` (no args): Installs **all** packages from `apm.yml` and deploys the project's own `.apm/` content @@ -1363,6 +1364,7 @@ apm config get [KEY] **Arguments:** - `KEY` (optional) - Configuration key to retrieve. Supported keys: - `auto-integrate` - Whether to automatically integrate `.prompt.md` files into AGENTS.md + - `allow-insecure` - Whether HTTP (insecure) dependencies are allowed globally If `KEY` is omitted, displays all configuration values. @@ -1371,6 +1373,9 @@ If `KEY` is omitted, displays all configuration values. # Get auto-integrate setting apm config get auto-integrate +# Get allow-insecure setting +apm config get allow-insecure + # Show all configuration apm config get ``` @@ -1386,6 +1391,7 @@ apm config set KEY VALUE **Arguments:** - `KEY` - Configuration key to set. Supported keys: - `auto-integrate` - Enable/disable automatic integration of `.prompt.md` files + - `allow-insecure` - Allow HTTP (insecure) dependencies globally - `VALUE` - Value to set. For boolean keys, use: `true`, `false`, `yes`, `no`, `1`, `0` **Configuration Keys:** @@ -1398,6 +1404,11 @@ apm config set KEY VALUE - Set to `false` if you want to manually manage which prompts are compiled - Set to `true` to ensure all prompts are always included in the context +**`allow-insecure`** - Allow HTTP (insecure) dependencies globally +- **Type:** Boolean +- **Default:** `false` +- **Description:** When enabled, APM skips the requirement to pass `--allow-insecure` at install time for HTTP dependencies. The per-dependency `allow_insecure: true` field in apm.yml is still required. Use this for private network environments where all servers use HTTP. + **Examples:** ```bash # Enable auto-integration (default) @@ -1406,9 +1417,11 @@ apm config set auto-integrate true # Disable auto-integration apm config set auto-integrate false -# Using alternative boolean values -apm config set auto-integrate yes -apm config set auto-integrate 1 +# Allow HTTP dependencies globally (skips --allow-insecure flag requirement) +apm config set allow-insecure true + +# Disable globally (default) +apm config set allow-insecure false ``` ## Runtime Management (Experimental) diff --git a/packages/apm-guide/.apm/skills/apm-usage/commands.md b/packages/apm-guide/.apm/skills/apm-usage/commands.md index 029891bb..06bf25d3 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/commands.md +++ b/packages/apm-guide/.apm/skills/apm-usage/commands.md @@ -10,7 +10,7 @@ | Command | Purpose | Key flags | |---------|---------|-----------| -| `apm install [PKGS...]` | Install packages | `--update` refresh refs, `--force` overwrite, `--dry-run`, `--verbose`, `--only [apm\|mcp]`, `--target`, `--dev`, `-g` global, `--trust-transitive-mcp`, `--parallel-downloads N` | +| `apm install [PKGS...]` | Install packages | `--update` refresh refs, `--force` overwrite, `--dry-run`, `--verbose`, `--only [apm\|mcp]`, `--target`, `--dev`, `-g` global, `--trust-transitive-mcp`, `--parallel-downloads N`, `--allow-insecure` | | `apm uninstall PKGS...` | Remove packages | `--dry-run`, `-g` global | | `apm prune` | Remove orphaned packages | `--dry-run` | | `apm deps list` | List installed packages | `-g` global, `--all` both scopes | diff --git a/src/apm_cli/commands/config.py b/src/apm_cli/commands/config.py index 7794ce20..b7fb36da 100644 --- a/src/apm_cli/commands/config.py +++ b/src/apm_cli/commands/config.py @@ -14,6 +14,48 @@ # Restore builtin since a subcommand is named ``set`` set = builtins.set +_BOOLEAN_TRUE_VALUES = {"true", "1", "yes"} +_BOOLEAN_FALSE_VALUES = {"false", "0", "no"} +_CONFIG_KEY_DISPLAY_NAMES = { + "auto_integrate": "auto-integrate", + "allow_insecure": "allow-insecure", +} + + +def _parse_bool_value(value: str) -> bool: + """Parse a CLI boolean value.""" + normalized = value.lower() + if normalized in _BOOLEAN_TRUE_VALUES: + return True + if normalized in _BOOLEAN_FALSE_VALUES: + return False + raise ValueError(f"Invalid value '{value}'. Use 'true' or 'false'.") + + +def _get_config_setters(): + """Return config setters keyed by CLI option name.""" + from ..config import set_auto_integrate, set_allow_insecure + + return { + "auto-integrate": (set_auto_integrate, "Auto-integration"), + "allow-insecure": (set_allow_insecure, "Allow-insecure"), + } + + +def _get_config_getters(): + """Return config getters keyed by CLI option name.""" + from ..config import get_auto_integrate, get_allow_insecure + + return { + "auto-integrate": get_auto_integrate, + "allow-insecure": get_allow_insecure, + } + + +def _valid_config_keys() -> str: + """Return valid config keys for messages.""" + return ", ".join(_get_config_getters().keys()) + @click.group(help="Configure APM CLI", invoke_without_command=True) @click.pass_context @@ -115,28 +157,32 @@ def set(key, value): Examples: apm config set auto-integrate false apm config set auto-integrate true + apm config set allow-insecure true """ - from ..config import set_auto_integrate - logger = CommandLogger("config set") - if key == "auto-integrate": - if value.lower() in ["true", "1", "yes"]: - set_auto_integrate(True) - logger.success("Auto-integration enabled") - elif value.lower() in ["false", "0", "no"]: - set_auto_integrate(False) - logger.success("Auto-integration disabled") - else: - logger.error(f"Invalid value '{value}'. Use 'true' or 'false'.") - sys.exit(1) - else: + setters = _get_config_setters() + config_entry = setters.get(key) + if config_entry is None: logger.error(f"Unknown configuration key: '{key}'") - logger.progress("Valid keys: auto-integrate") + logger.progress(f"Valid keys: {_valid_config_keys()}") logger.progress( "This error may indicate a bug in command routing. Please report this issue." ) sys.exit(1) + try: + enabled = _parse_bool_value(value) + except ValueError as exc: + logger.error(str(exc)) + sys.exit(1) + + setter, label = config_entry + setter(enabled) + if enabled: + logger.success(f"{label} enabled") + else: + logger.success(f"{label} disabled") + @config.command(help="Get a configuration value") @click.argument("key", required=False) @@ -145,29 +191,28 @@ def get(key): Examples: apm config get auto-integrate + apm config get allow-insecure apm config get """ - from ..config import get_config, get_auto_integrate + from ..config import get_config logger = CommandLogger("config get") + getters = _get_config_getters() if key: - if key == "auto-integrate": - value = get_auto_integrate() - click.echo(f"auto-integrate: {value}") - else: + getter = getters.get(key) + if getter is None: logger.error(f"Unknown configuration key: '{key}'") - logger.progress("Valid keys: auto-integrate") + logger.progress(f"Valid keys: {_valid_config_keys()}") logger.progress( "This error may indicate a bug in command routing. Please report this issue." ) sys.exit(1) + value = getter() + click.echo(f"{key}: {value}") else: # Show all config config_data = get_config() logger.progress("APM Configuration:") for k, v in config_data.items(): - # Map internal keys to user-friendly names - if k == "auto_integrate": - click.echo(f" auto-integrate: {v}") - else: - click.echo(f" {k}: {v}") + display_key = _CONFIG_KEY_DISPLAY_NAMES.get(k, k) + click.echo(f" {display_key}: {v}") diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index 4e39a810..a6225cd4 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -62,7 +62,53 @@ # --------------------------------------------------------------------------- -def _validate_and_add_packages_to_apm_yml(packages, dry_run=False, dev=False, logger=None, manifest_path=None, auth_resolver=None, scope=None): +def _check_insecure_dependencies( + deps, allow_insecure_flag: bool, logger=None +) -> None: + """Check APM dependencies for HTTP (insecure) URLs and enforce security policy. + + Two conditions must BOTH be true for an HTTP dep to be allowed: + 1. The dep entry in apm.yml must have allow_insecure: true + 2. Either --allow-insecure flag is set OR apm config allow-insecure is true + + Args: + deps: List of DependencyReference objects to check. + allow_insecure_flag: True if --allow-insecure was passed on the command line. + """ + from ..config import get_allow_insecure + config_allow_insecure = get_allow_insecure() + + for dep in deps: + dep_is_insecure = getattr(dep, "is_insecure", False) is True + if not dep_is_insecure: + continue + identity = dep.get_identity() + dep_allow_insecure = getattr(dep, "allow_insecure", False) is True + if not dep_allow_insecure: + message = ( + f"Dependency '{identity}' uses HTTP (insecure) but " + f"'allow_insecure: true' is not set in its apm.yml entry. " + f"Add 'allow_insecure: true' to the dependency, or use a HTTPS URL instead." + ) + if logger: + logger.error(message) + else: + _rich_error(message) + sys.exit(1) + if not (allow_insecure_flag or config_allow_insecure): + message = ( + f"Dependency '{identity}' uses HTTP (insecure). " + f"Pass '--allow-insecure' to apm install, or run " + f"'apm config set allow-insecure true' to allow HTTP dependencies globally." + ) + if logger: + logger.error(message) + else: + _rich_error(message) + sys.exit(1) + + +def _validate_and_add_packages_to_apm_yml(packages, dry_run=False, dev=False, logger=None, manifest_path=None, auth_resolver=None, scope=None, allow_insecure=False): """Validate packages exist and can be accessed, then add to apm.yml dependencies section. Implements normalize-on-write: any input form (HTTPS URL, SSH URL, FQDN, shorthand) @@ -126,6 +172,7 @@ def _validate_and_add_packages_to_apm_yml(packages, dry_run=False, dev=False, lo valid_outcomes = [] # (canonical, already_present) tuples invalid_outcomes = [] # (package, reason) tuples _marketplace_provenance = {} # canonical -> {discovered_via, marketplace_plugin_name} + _apm_yml_entries = {} # canonical -> apm.yml entry (str or dict for HTTP deps) if logger: logger.validation_start(len(packages)) @@ -196,6 +243,19 @@ def _validate_and_add_packages_to_apm_yml(packages, dry_run=False, dev=False, lo logger.validation_fail(package, reason) continue + # Reject HTTP deps unless --allow-insecure is set + if dep_ref.is_insecure: + if not allow_insecure: + reason = ( + f"'{canonical}' uses HTTP (insecure). " + f"Pass '--allow-insecure' to allow HTTP dependencies." + ) + invalid_outcomes.append((package, reason)) + if logger: + logger.validation_fail(package, reason) + continue + dep_ref.allow_insecure = True + # Reject local packages at user scope -- relative paths resolve # against cwd during validation but against $HOME during copy, # causing silent failures. @@ -223,6 +283,7 @@ def _validate_and_add_packages_to_apm_yml(packages, dry_run=False, dev=False, lo if not already_in_deps: validated_packages.append(canonical) + _apm_yml_entries[canonical] = dep_ref.to_apm_yml_entry() existing_identities.add(identity) # prevent duplicates within batch if marketplace_provenance: _marketplace_provenance[identity] = marketplace_provenance @@ -267,7 +328,8 @@ def _validate_and_add_packages_to_apm_yml(packages, dry_run=False, dev=False, lo # Add validated packages to dependencies (already canonical) dep_label = "devDependencies" if dev else "apm.yml" for package in validated_packages: - current_deps.append(package) + entry = _apm_yml_entries.get(package, package) + current_deps.append(entry) if logger: logger.verbose_detail(f"Added {package} to {dep_label}") @@ -622,8 +684,15 @@ def _check_repo_fallback(token, git_env): default=False, help="Install to user scope (~/.apm/) instead of the current project", ) +@click.option( + "--allow-insecure", + "allow_insecure", + is_flag=True, + default=False, + help="Allow HTTP (insecure) dependencies. Required when dependencies use http:// URLs.", +) @click.pass_context -def install(ctx, packages, runtime, exclude, only, update, dry_run, force, verbose, trust_transitive_mcp, parallel_downloads, dev, target, global_): +def install(ctx, packages, runtime, exclude, only, update, dry_run, force, verbose, trust_transitive_mcp, parallel_downloads, dev, target, global_, allow_insecure): """Install APM and MCP dependencies from apm.yml (like npm install). This command automatically detects AI runtimes from your apm.yml scripts and installs @@ -643,6 +712,7 @@ def install(ctx, packages, runtime, exclude, only, update, dry_run, force, verbo apm install --update # Update dependencies to latest Git refs apm install --dry-run # Show what would be installed apm install -g org/pkg1 # Install to user scope (~/.apm/) + apm install --allow-insecure http://my-server.example.com/owner/repo """ try: # Create structured logger for install output early so exception @@ -702,7 +772,7 @@ def install(ctx, packages, runtime, exclude, only, update, dry_run, force, verbo validated_packages, outcome = _validate_and_add_packages_to_apm_yml( packages, dry_run, dev=dev, logger=logger, manifest_path=manifest_path, auth_resolver=auth_resolver, - scope=scope, + scope=scope, allow_insecure=allow_insecure, ) # Short-circuit: all packages failed validation — nothing to install if outcome.all_failed: @@ -735,6 +805,12 @@ def install(ctx, packages, runtime, exclude, only, update, dry_run, force, verbo has_any_apm_deps = bool(apm_deps) or bool(dev_apm_deps) mcp_deps = apm_package.get_mcp_dependencies() + # Enforce HTTP security policy before any installation proceeds + all_apm_deps = list(apm_deps) + list(dev_apm_deps) + _check_insecure_dependencies( + all_apm_deps, allow_insecure, logger=logger + ) + # Convert --only string to InstallMode enum if only is None: install_mode = InstallMode.ALL @@ -2702,5 +2778,3 @@ def _collect_descendants(node, visited=None): raise RuntimeError(f"Failed to resolve APM dependencies: {e}") - - diff --git a/src/apm_cli/config.py b/src/apm_cli/config.py index 0d453221..30f8d4ff 100644 --- a/src/apm_cli/config.py +++ b/src/apm_cli/config.py @@ -92,3 +92,21 @@ def set_auto_integrate(enabled: bool) -> None: enabled: Whether to enable auto-integration. """ update_config({"auto_integrate": enabled}) + + +def get_allow_insecure() -> bool: + """Get the allow-insecure setting. + + Returns: + bool: Whether HTTP (insecure) dependencies are allowed globally (default: False). + """ + return get_config().get("allow_insecure", False) + + +def set_allow_insecure(enabled: bool) -> None: + """Set the allow-insecure setting. + + Args: + enabled: Whether to allow HTTP (insecure) dependencies globally. + """ + update_config({"allow_insecure": enabled}) diff --git a/src/apm_cli/deps/github_downloader.py b/src/apm_cli/deps/github_downloader.py index ee5714f7..91be661b 100644 --- a/src/apm_cli/deps/github_downloader.py +++ b/src/apm_cli/deps/github_downloader.py @@ -619,11 +619,18 @@ def _build_repo_url(self, repo_ref: str, use_ssh: bool = False, dep_ref: Depende else: # Determine if this host should receive a GitHub token is_github = is_github_hostname(host) + # Use http:// only for dependencies explicitly marked as insecure. + is_insecure = False + if dep_ref is not None: + is_insecure = bool(getattr(dep_ref, "is_insecure", False)) if use_ssh: return build_ssh_url(host, repo_ref) - elif is_github and github_token: - # Only send GitHub tokens to GitHub hosts + elif is_github and github_token and not is_insecure: + # Only send GitHub tokens to GitHub HTTPS hosts return build_https_clone_url(host, repo_ref, token=github_token) + elif is_insecure: + # HTTP (insecure) dep: build plain http:// URL + return f"http://{host}/{repo_ref}" else: # Generic hosts: plain HTTPS, let git credential helpers handle auth return build_https_clone_url(host, repo_ref, token=None) diff --git a/src/apm_cli/models/dependency/reference.py b/src/apm_cli/models/dependency/reference.py index 1c6df16b..9c153240 100644 --- a/src/apm_cli/models/dependency/reference.py +++ b/src/apm_cli/models/dependency/reference.py @@ -56,6 +56,10 @@ class DependencyReference: None # e.g., "artifactory/github" (repo key path) ) + # HTTP (insecure) dependency fields + is_insecure: bool = False # True when the dependency URL uses http:// + allow_insecure: bool = False # True if this HTTP dep is explicitly allowed + # Supported file extensions for virtual packages VIRTUAL_FILE_EXTENSIONS = ( ".prompt.md", @@ -198,8 +202,9 @@ def to_canonical(self) -> str: - Virtual paths are appended -> owner/repo/path/to/thing - Refs are appended with # -> owner/repo#v1.0 - Local paths are returned as-is -> ./packages/my-pkg + - HTTP deps preserve the http:// prefix -> http://host/owner/repo - No .git suffix, no https://, no git@ -- just the canonical identifier. + No .git suffix, no git@ -- just the canonical identifier. Returns: str: Canonical dependency string @@ -208,6 +213,14 @@ def to_canonical(self) -> str: return self.local_path host = self.host or default_host() + + # HTTP deps always include the full http://host/path form + if self.is_insecure: + result = f"http://{host}/{self.repo_url}" + if self.reference: + result = f"{result}#{self.reference}" + return result + is_default = host.lower() == default_host().lower() # Start with optional host prefix @@ -470,6 +483,7 @@ def parse_from_dict(cls, entry: dict) -> "DependencyReference": sub_path = entry.get("path") ref_override = entry.get("ref") alias_override = entry.get("alias") + allow_insecure = bool(entry.get("allow_insecure", False)) # Validate sub_path if provided if sub_path is not None: @@ -483,6 +497,7 @@ def parse_from_dict(cls, entry: dict) -> "DependencyReference": # Parse the git URL using the standard parser dep = cls.parse(git_url) + dep.allow_insecure = allow_insecure # Apply overrides from the object fields if ref_override is not None: @@ -986,8 +1001,29 @@ def parse(cls, dependency_str: str) -> "DependencyReference": ado_project=ado_project, ado_repo=ado_repo, artifactory_prefix=artifactory_prefix, + is_insecure=dependency_str.startswith("http://"), ) + def to_apm_yml_entry(self): + """Return the entry to store in apm.yml. + + For HTTP (insecure) deps, returns a dict with 'git' and 'allow_insecure' keys. + For all other deps, returns the canonical string (same as to_canonical()). + + Returns: + str or dict: String for HTTPS/SSH/local deps; dict for HTTP deps. + """ + if self.is_insecure: + host = self.host or default_host() + entry = {"git": f"http://{host}/{self.repo_url}"} + if self.reference: + entry["ref"] = self.reference + if self.alias: + entry["alias"] = self.alias + entry["allow_insecure"] = True + return entry + return self.to_canonical() + def to_github_url(self) -> str: """Convert to full repository URL. @@ -1000,16 +1036,17 @@ def to_github_url(self) -> str: host = self.host or default_host() + scheme = "http" if self.is_insecure else "https" + if self.is_azure_devops(): # ADO format: https://dev.azure.com/org/project/_git/repo project = urllib.parse.quote(self.ado_project, safe="") repo = urllib.parse.quote(self.ado_repo, safe="") - return f"https://{host}/{self.ado_organization}/{project}/_git/{repo}" + return f"{scheme}://{host}/{self.ado_organization}/{project}/_git/{repo}" elif self.artifactory_prefix: - return f"https://{host}/{self.artifactory_prefix}/{self.repo_url}" + return f"{scheme}://{host}/{self.artifactory_prefix}/{self.repo_url}" else: - # GitHub format: https://github.com/owner/repo - return f"https://{host}/{self.repo_url}" + return f"{scheme}://{host}/{self.repo_url}" def to_clone_url(self) -> str: """Convert to a clone-friendly URL (same as to_github_url for most purposes).""" diff --git a/tests/unit/test_canonicalization.py b/tests/unit/test_canonicalization.py index 75a8b81e..27411965 100644 --- a/tests/unit/test_canonicalization.py +++ b/tests/unit/test_canonicalization.py @@ -464,3 +464,109 @@ def test_filter_no_cross_host_match(self): dep = DependencyReference.parse("gitlab.com/microsoft/apm-sample-package") filter_ref = DependencyReference.parse("microsoft/apm-sample-package") assert dep.get_identity() != filter_ref.get_identity() + + +# ── HTTP (allow_insecure) ──────────────────────────────────────────────────── + +class TestHttpInsecureDeps: + """Tests for HTTP (insecure) dependency parsing and serialization.""" + + def test_http_url_sets_insecure_flag(self): + """Parsing an http:// URL marks the ref as insecure.""" + dep = DependencyReference.parse("http://my-server.example.com/owner/repo") + assert dep.is_insecure is True + assert dep.host == "my-server.example.com" + assert dep.repo_url == "owner/repo" + + def test_https_url_is_not_insecure(self): + """Parsing an https:// URL does not mark the ref as insecure.""" + dep = DependencyReference.parse("https://gitlab.com/owner/repo.git") + assert dep.is_insecure is False + + def test_shorthand_is_not_insecure(self): + """Parsing shorthand owner/repo does not mark the ref as insecure.""" + dep = DependencyReference.parse("owner/repo") + assert dep.is_insecure is False + + def test_http_allow_insecure_default_false(self): + """Freshly parsed HTTP dep has allow_insecure=False by default.""" + dep = DependencyReference.parse("http://my-server.example.com/owner/repo") + assert dep.allow_insecure is False + + def test_http_to_canonical_preserves_scheme(self): + """to_canonical() for HTTP dep includes http:// prefix.""" + dep = DependencyReference.parse("http://my-server.example.com/owner/repo") + canonical = dep.to_canonical() + assert canonical.startswith("http://") + assert "my-server.example.com/owner/repo" in canonical + + def test_http_to_canonical_with_ref(self): + """to_canonical() for HTTP dep with ref includes #ref.""" + dep = DependencyReference.parse("http://my-server.example.com/owner/repo#main") + canonical = dep.to_canonical() + assert canonical == "http://my-server.example.com/owner/repo#main" + + def test_http_to_apm_yml_entry_returns_dict(self): + """to_apm_yml_entry() for HTTP dep returns a dict with git key.""" + dep = DependencyReference.parse("http://my-server.example.com/owner/repo") + dep.allow_insecure = True + entry = dep.to_apm_yml_entry() + assert isinstance(entry, dict) + assert entry["git"] == "http://my-server.example.com/owner/repo" + assert entry["allow_insecure"] is True + + def test_http_to_apm_yml_entry_includes_ref(self): + """to_apm_yml_entry() includes ref when present.""" + dep = DependencyReference.parse("http://my-server.example.com/owner/repo#v1.0") + dep.allow_insecure = True + entry = dep.to_apm_yml_entry() + assert entry.get("ref") == "v1.0" + assert "http://my-server.example.com/owner/repo" in entry["git"] + + def test_https_to_apm_yml_entry_returns_string(self): + """to_apm_yml_entry() for HTTPS dep returns canonical string (not dict).""" + dep = DependencyReference.parse("owner/repo") + entry = dep.to_apm_yml_entry() + assert isinstance(entry, str) + assert entry == "owner/repo" + + def test_parse_from_dict_git_http(self): + """parse_from_dict() supports git: http://... for HTTP deps.""" + entry = {"git": "http://my-server.example.com/owner/repo", "allow_insecure": True} + dep = DependencyReference.parse_from_dict(entry) + assert dep.is_insecure is True + assert dep.allow_insecure is True + assert dep.repo_url == "owner/repo" + assert dep.host == "my-server.example.com" + + def test_parse_from_dict_git_http_with_ref(self): + """parse_from_dict() reads ref from dict with git key.""" + entry = {"git": "http://my-server.example.com/owner/repo", "ref": "main", "allow_insecure": True} + dep = DependencyReference.parse_from_dict(entry) + assert dep.reference == "main" + + def test_parse_from_dict_git_http_allow_insecure_default_false(self): + """parse_from_dict() with git http URL defaults allow_insecure to False.""" + entry = {"git": "http://my-server.example.com/owner/repo"} + dep = DependencyReference.parse_from_dict(entry) + assert dep.allow_insecure is False + + def test_http_to_github_url_uses_http_scheme(self): + """to_github_url() uses http:// for HTTP deps.""" + dep = DependencyReference.parse("http://my-server.example.com/owner/repo") + url = dep.to_github_url() + assert url.startswith("http://") + assert "my-server.example.com/owner/repo" in url + + def test_https_to_github_url_uses_https_scheme(self): + """to_github_url() still uses https:// for HTTPS deps.""" + dep = DependencyReference.parse("https://gitlab.com/owner/repo.git") + url = dep.to_github_url() + assert url.startswith("https://") + + def test_http_identity_scheme_agnostic(self): + """HTTP and HTTPS deps to the same host/repo have the same identity.""" + http_dep = DependencyReference.parse("http://gitlab.com/owner/repo") + https_dep = DependencyReference.parse("https://gitlab.com/owner/repo.git") + # Identity includes host but not scheme, so they are the same package + assert http_dep.get_identity() == https_dep.get_identity() diff --git a/tests/unit/test_config_command.py b/tests/unit/test_config_command.py index d9944e76..352617f0 100644 --- a/tests/unit/test_config_command.py +++ b/tests/unit/test_config_command.py @@ -197,6 +197,20 @@ def test_set_auto_integrate_case_insensitive(self): assert result.exit_code == 0 mock_set.assert_called_once_with(True) + def test_set_allow_insecure_true(self): + """Enable allow-insecure.""" + with patch("apm_cli.config.set_allow_insecure") as mock_set: + result = self.runner.invoke(config, ["set", "allow-insecure", "true"]) + assert result.exit_code == 0 + mock_set.assert_called_once_with(True) + + def test_set_allow_insecure_false(self): + """Disable allow-insecure.""" + with patch("apm_cli.config.set_allow_insecure") as mock_set: + result = self.runner.invoke(config, ["set", "allow-insecure", "false"]) + assert result.exit_code == 0 + mock_set.assert_called_once_with(False) + class TestConfigGet: """Tests for `apm config get [key]`.""" @@ -218,6 +232,13 @@ def test_get_auto_integrate_disabled(self): assert result.exit_code == 0 assert "auto-integrate: False" in result.output + def test_get_allow_insecure(self): + """Get the allow-insecure setting.""" + with patch("apm_cli.config.get_allow_insecure", return_value=True): + result = self.runner.invoke(config, ["get", "allow-insecure"]) + assert result.exit_code == 0 + assert "allow-insecure: True" in result.output + def test_get_unknown_key(self): """Reject an unknown key.""" result = self.runner.invoke(config, ["get", "nonexistent"]) @@ -274,3 +295,36 @@ def test_set_auto_integrate_false_calls_update_config(self): with patch.object(cfg_module, "update_config") as mock_update: cfg_module.set_auto_integrate(False) mock_update.assert_called_once_with({"auto_integrate": False}) + +class TestAllowInsecureFunctions: + """Unit tests for get_allow_insecure / set_allow_insecure in config.py.""" + + def test_get_allow_insecure_default_false(self): + """get_allow_insecure() returns False when not configured.""" + import apm_cli.config as cfg_module + + with patch.object(cfg_module, "get_config", return_value={}): + assert cfg_module.get_allow_insecure() is False + + def test_get_allow_insecure_true(self): + """get_allow_insecure() returns True when configured.""" + import apm_cli.config as cfg_module + + with patch.object(cfg_module, "get_config", return_value={"allow_insecure": True}): + assert cfg_module.get_allow_insecure() is True + + def test_set_allow_insecure_calls_update_config(self): + """set_allow_insecure() delegates to update_config.""" + import apm_cli.config as cfg_module + + with patch.object(cfg_module, "update_config") as mock_update: + cfg_module.set_allow_insecure(True) + mock_update.assert_called_once_with({"allow_insecure": True}) + + def test_set_allow_insecure_false_calls_update_config(self): + """set_allow_insecure(False) passes False to update_config.""" + import apm_cli.config as cfg_module + + with patch.object(cfg_module, "update_config") as mock_update: + cfg_module.set_allow_insecure(False) + mock_update.assert_called_once_with({"allow_insecure": False}) diff --git a/tests/unit/test_install_command.py b/tests/unit/test_install_command.py index e9bbe47a..17e0451e 100644 --- a/tests/unit/test_install_command.py +++ b/tests/unit/test_install_command.py @@ -862,3 +862,86 @@ def test_global_rejects_tilde_local_path(self): assert "not supported at user scope" in result.output finally: os.chdir(self.original_dir) + + +class TestAllowInsecureFlag: + """Tests for --allow-insecure flag and HTTP dependency security checks.""" + + def setup_method(self): + self.runner = CliRunner() + + def test_http_dep_rejected_without_allow_insecure_flag(self): + """Adding http:// package without --allow-insecure is rejected.""" + with tempfile.TemporaryDirectory() as tmp_dir: + os.chdir(tmp_dir) + with patch("apm_cli.commands.install._validate_package_exists", return_value=True): + result = self.runner.invoke( + cli, ["install", "http://my-server.example.com/owner/repo"] + ) + assert result.exit_code != 0 or "allow-insecure" in result.output.lower() or "HTTP" in result.output + + def test_http_dep_validation_check(self): + """_check_insecure_dependencies blocks HTTP dep without allow_insecure flag.""" + from apm_cli.commands.install import _check_insecure_dependencies + from apm_cli.models.dependency.reference import DependencyReference + + dep = DependencyReference.parse("http://my-server.example.com/owner/repo") + dep.allow_insecure = True # dep-level OK, but no flag + + with patch("apm_cli.config.get_allow_insecure", return_value=False): + with pytest.raises(SystemExit) as exc_info: + _check_insecure_dependencies([dep], allow_insecure_flag=False) + assert exc_info.value.code == 1 + + def test_http_dep_passes_with_allow_insecure_flag(self): + """_check_insecure_dependencies passes when flag is set and dep has allow_insecure.""" + from apm_cli.commands.install import _check_insecure_dependencies + from apm_cli.models.dependency.reference import DependencyReference + + dep = DependencyReference.parse("http://my-server.example.com/owner/repo") + dep.allow_insecure = True + + with patch("apm_cli.config.get_allow_insecure", return_value=False): + # Should not raise + _check_insecure_dependencies([dep], allow_insecure_flag=True) + + def test_http_dep_passes_with_config_allow_insecure(self): + """_check_insecure_dependencies passes when global config allow_insecure is true.""" + from apm_cli.commands.install import _check_insecure_dependencies + from apm_cli.models.dependency.reference import DependencyReference + + dep = DependencyReference.parse("http://my-server.example.com/owner/repo") + dep.allow_insecure = True + + with patch("apm_cli.config.get_allow_insecure", return_value=True): + # Should not raise + _check_insecure_dependencies([dep], allow_insecure_flag=False) + + def test_http_dep_without_dep_level_allow_insecure_is_blocked(self): + """_check_insecure_dependencies blocks HTTP dep missing allow_insecure=True on dep.""" + from apm_cli.commands.install import _check_insecure_dependencies + from apm_cli.models.dependency.reference import DependencyReference + + dep = DependencyReference.parse("http://my-server.example.com/owner/repo") + # allow_insecure is False by default + + with patch("apm_cli.config.get_allow_insecure", return_value=False): + with pytest.raises(SystemExit) as exc_info: + _check_insecure_dependencies([dep], allow_insecure_flag=True) + assert exc_info.value.code == 1 + + def test_https_dep_passes_without_flag(self): + """_check_insecure_dependencies does not block HTTPS deps.""" + from apm_cli.commands.install import _check_insecure_dependencies + from apm_cli.models.dependency.reference import DependencyReference + + dep = DependencyReference.parse("owner/repo") + with patch("apm_cli.config.get_allow_insecure", return_value=False): + # Should not raise for HTTPS dep + _check_insecure_dependencies([dep], allow_insecure_flag=False) + + def test_empty_deps_list_passes(self): + """_check_insecure_dependencies handles empty dep list.""" + from apm_cli.commands.install import _check_insecure_dependencies + with patch("apm_cli.config.get_allow_insecure", return_value=False): + _check_insecure_dependencies([], allow_insecure_flag=False) From 7d4718d17037795620a84363e4acb26c09dcfc25 Mon Sep 17 00:00:00 2001 From: arika0093 Date: Tue, 14 Apr 2026 05:18:15 +0000 Subject: [PATCH 02/10] fix: avoid SSH fallback for insecure HTTP deps Also honor global allow-insecure config when adding HTTP dependencies. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../content/docs/reference/cli-commands.md | 2 +- src/apm_cli/bundle/plugin_exporter.py | 3 + src/apm_cli/commands/_helpers.py | 5 ++ src/apm_cli/commands/install.py | 19 ++++-- src/apm_cli/deps/github_downloader.py | 39 +++++++++-- src/apm_cli/deps/lockfile.py | 13 ++++ src/apm_cli/drift.py | 15 +++-- tests/unit/test_auth_scoping.py | 33 +++++++++ tests/unit/test_install_command.py | 67 +++++++++++++++++-- tests/unit/test_install_update.py | 47 +++++++++++++ 10 files changed, 221 insertions(+), 22 deletions(-) diff --git a/docs/src/content/docs/reference/cli-commands.md b/docs/src/content/docs/reference/cli-commands.md index 9c0a922f..c73b6433 100644 --- a/docs/src/content/docs/reference/cli-commands.md +++ b/docs/src/content/docs/reference/cli-commands.md @@ -74,7 +74,7 @@ apm init my-plugin --plugin ### `apm install` - Install dependencies and deploy local content -Install APM package and MCP server dependencies from `apm.yml` and deploy the project's own `.apm/` content to target directories (like `npm install`). Auto-creates minimal `apm.yml` when packages are specified but no manifest exists. +Install APM package and MCP server dependencies from `apm.yml` and deploy the project's own `.apm/` content to target directories (like `npm install`). Auto-creates minimal `apm.yml` when packages are specified but no manifest exists. For `http://` dependencies, use `--allow-insecure` or enable the global `allow-insecure` config. ```bash apm install [PACKAGES...] [OPTIONS] diff --git a/src/apm_cli/bundle/plugin_exporter.py b/src/apm_cli/bundle/plugin_exporter.py index 096840ff..65734513 100644 --- a/src/apm_cli/bundle/plugin_exporter.py +++ b/src/apm_cli/bundle/plugin_exporter.py @@ -391,8 +391,11 @@ def _dep_install_path(dep: LockedDependency, apm_modules_dir: Path) -> Path: host=dep.host, virtual_path=dep.virtual_path, is_virtual=dep.is_virtual, + artifactory_prefix=dep.registry_prefix, is_local=(dep.source == "local"), local_path=dep.local_path, + is_insecure=dep.is_insecure, + allow_insecure=dep.allow_insecure, ) return dep_ref.get_install_path(apm_modules_dir) diff --git a/src/apm_cli/commands/_helpers.py b/src/apm_cli/commands/_helpers.py index 6f220455..f444c589 100644 --- a/src/apm_cli/commands/_helpers.py +++ b/src/apm_cli/commands/_helpers.py @@ -140,6 +140,11 @@ def _build_expected_install_paths(declared_deps, lockfile, apm_modules_dir: Path host=dep.host, virtual_path=dep.virtual_path, is_virtual=dep.is_virtual, + artifactory_prefix=dep.registry_prefix, + is_local=(dep.source == "local"), + local_path=dep.local_path, + is_insecure=dep.is_insecure, + allow_insecure=dep.allow_insecure, ) install_path = dep_ref.get_install_path(apm_modules_dir) try: diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index a6225cd4..06f521f5 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -131,7 +131,10 @@ def _validate_and_add_packages_to_apm_yml(packages, dry_run=False, dev=False, lo import tempfile from pathlib import Path + from ..config import get_allow_insecure + apm_yml_path = manifest_path or Path(APM_YML_FILENAME) + config_allow_insecure = get_allow_insecure() # Read current apm.yml try: @@ -245,15 +248,18 @@ def _validate_and_add_packages_to_apm_yml(packages, dry_run=False, dev=False, lo # Reject HTTP deps unless --allow-insecure is set if dep_ref.is_insecure: - if not allow_insecure: + if not (allow_insecure or config_allow_insecure): reason = ( f"'{canonical}' uses HTTP (insecure). " - f"Pass '--allow-insecure' to allow HTTP dependencies." + f"Pass '--allow-insecure' or run " + f"'apm config set allow-insecure true' to allow HTTP dependencies." ) invalid_outcomes.append((package, reason)) if logger: logger.validation_fail(package, reason) continue + # Persist the HTTP allowance in apm.yml so future installs can pass + # _check_insecure_dependencies() before the lockfile is consulted. dep_ref.allow_insecure = True # Reject local packages at user scope -- relative paths resolve @@ -631,7 +637,7 @@ def _check_repo_fallback(token, git_env): @click.command( - help="Install APM and MCP dependencies (auto-creates apm.yml when installing packages)" + help="Install APM and MCP dependencies (auto-creates apm.yml; use --allow-insecure for http:// packages)" ) @click.argument("packages", nargs=-1) @click.option("--runtime", help="Target specific runtime only (copilot, codex, vscode)") @@ -699,6 +705,9 @@ def install(ctx, packages, runtime, exclude, only, update, dry_run, force, verbo MCP servers for all detected and available runtimes. It also installs APM package dependencies from GitHub repositories. + HTTP dependencies require `allow_insecure: true` in apm.yml and either + `--allow-insecure` or `apm config set allow-insecure true`. + The --only flag filters by dependency type (apm or mcp). Internally converted to an InstallMode enum for type-safe dispatch. @@ -776,7 +785,7 @@ def install(ctx, packages, runtime, exclude, only, update, dry_run, force, verbo ) # Short-circuit: all packages failed validation — nothing to install if outcome.all_failed: - return + sys.exit(1) # Note: Empty validated_packages is OK if packages are already in apm.yml # We'll proceed with installation from apm.yml to ensure everything is synced @@ -2776,5 +2785,3 @@ def _collect_descendants(node, visited=None): except Exception as e: raise RuntimeError(f"Failed to resolve APM dependencies: {e}") - - diff --git a/src/apm_cli/deps/github_downloader.py b/src/apm_cli/deps/github_downloader.py index 91be661b..87b3a442 100644 --- a/src/apm_cli/deps/github_downloader.py +++ b/src/apm_cli/deps/github_downloader.py @@ -624,15 +624,16 @@ def _build_repo_url(self, repo_ref: str, use_ssh: bool = False, dep_ref: Depende if dep_ref is not None: is_insecure = bool(getattr(dep_ref, "is_insecure", False)) if use_ssh: + # Method 2: SSH fallback return build_ssh_url(host, repo_ref) elif is_github and github_token and not is_insecure: - # Only send GitHub tokens to GitHub HTTPS hosts + # Method 1: Only send GitHub tokens to GitHub HTTPS hosts return build_https_clone_url(host, repo_ref, token=github_token) elif is_insecure: - # HTTP (insecure) dep: build plain http:// URL - return f"http://{host}/{repo_ref}" + # HTTP direct only: _clone_with_fallback() returns before Method 1/2/3. + return f"http://{host}/{repo_ref}.git" else: - # Generic hosts: plain HTTPS, let git credential helpers handle auth + # Method 3: Plain HTTPS, let git credential helpers handle auth return build_https_clone_url(host, repo_ref, token=None) def _clone_with_fallback(self, repo_url_base: str, target_path: Path, progress_reporter=None, dep_ref: DependencyReference = None, verbose_callback=None, **clone_kwargs) -> Repo: @@ -674,6 +675,36 @@ def _clone_with_fallback(self, repo_url_base: str, target_path: Path, progress_r _debug(f"_clone_with_fallback: repo={repo_url_base}, is_ado={is_ado}, is_generic={is_generic}, has_token={has_token is not None}") + # HTTP direct only: return before Method 1/2/3 and keep credential helpers enabled. + if dep_ref and getattr(dep_ref, "is_insecure", False): + insecure_env = { + k: v + for k, v in self.git_env.items() + if k not in ("GIT_ASKPASS", "GIT_CONFIG_GLOBAL", "GIT_CONFIG_NOSYSTEM") + } + insecure_env["GIT_TERMINAL_PROMPT"] = "0" + try: + http_url = self._build_repo_url( + repo_url_base, use_ssh=False, dep_ref=dep_ref + ) + repo = Repo.clone_from( + http_url, + target_path, + env=insecure_env, + progress=progress_reporter, + **clone_kwargs, + ) + if verbose_callback: + verbose_callback(f"Cloned from: {http_url}") + return repo + except GitCommandError as e: + sanitized_error = self._sanitize_git_error(str(e)) + raise RuntimeError( + f"Failed to clone insecure HTTP repository {repo_url_base}. " + f"HTTP dependencies do not fall back to SSH or HTTPS. " + f"Last error: {sanitized_error}" + ) from e + # When APM has a token for this host, use the locked-down env (APM manages auth). # When no token is available, relax the env so git credential helpers (gh auth, # macOS Keychain, etc.) can provide credentials -- regardless of host. diff --git a/src/apm_cli/deps/lockfile.py b/src/apm_cli/deps/lockfile.py index e4782280..cc71e47b 100644 --- a/src/apm_cli/deps/lockfile.py +++ b/src/apm_cli/deps/lockfile.py @@ -38,6 +38,8 @@ class LockedDependency: is_dev: bool = False # True for devDependencies discovered_via: Optional[str] = None # Marketplace name (provenance) marketplace_plugin_name: Optional[str] = None # Plugin name in marketplace + is_insecure: bool = False # True when the locked source was http:// + allow_insecure: bool = False # True when the manifest explicitly allowed HTTP def get_unique_key(self) -> str: """Returns unique key for this dependency.""" @@ -84,6 +86,10 @@ def to_dict(self) -> Dict[str, Any]: result["discovered_via"] = self.discovered_via if self.marketplace_plugin_name: result["marketplace_plugin_name"] = self.marketplace_plugin_name + if self.is_insecure: + result["is_insecure"] = True + if self.allow_insecure: + result["allow_insecure"] = True return result @classmethod @@ -122,6 +128,8 @@ def from_dict(cls, data: Dict[str, Any]) -> "LockedDependency": is_dev=data.get("is_dev", False), discovered_via=data.get("discovered_via"), marketplace_plugin_name=data.get("marketplace_plugin_name"), + is_insecure=data.get("is_insecure", False), + allow_insecure=data.get("allow_insecure", False), ) @classmethod @@ -167,6 +175,8 @@ def from_dependency_ref( source="local" if dep_ref.is_local else None, local_path=dep_ref.local_path if dep_ref.is_local else None, is_dev=is_dev, + is_insecure=dep_ref.is_insecure, + allow_insecure=dep_ref.allow_insecure, ) @@ -337,8 +347,11 @@ def get_installed_paths(self, apm_modules_dir: Path) -> List[str]: host=dep.host, virtual_path=dep.virtual_path, is_virtual=dep.is_virtual, + artifactory_prefix=dep.registry_prefix, is_local=(dep.source == "local"), local_path=dep.local_path, + is_insecure=dep.is_insecure, + allow_insecure=dep.allow_insecure, ) install_path = dep_ref.get_install_path(apm_modules_dir) try: diff --git a/src/apm_cli/drift.py b/src/apm_cli/drift.py index 5950dc28..6fb965ff 100644 --- a/src/apm_cli/drift.py +++ b/src/apm_cli/drift.py @@ -33,12 +33,12 @@ formatting-only change that produces the same unique key and is correctly treated as no drift. -* **Source/host changes** — *not* detected. If a user changes the host of - an otherwise identical package (e.g. adding an enterprise FQDN prefix), the - unique key (``repo_url``, host-blind for the default host) may not change - and ``detect_ref_change()`` will not signal a re-download. Host-level - changes require the user to ``apm remove`` + ``apm install`` the package, or - use ``--update``. +* **Source/host/scheme changes** — *not* detected. If a user changes the host + or scheme of an otherwise identical package (for example, switching an + insecure HTTP dependency to HTTPS), the unique key may not change and + ``detect_ref_change()`` will not signal a re-download. Source-level changes + require the user to ``apm remove`` + ``apm install`` the package, or use + ``--update``. """ from __future__ import annotations @@ -215,6 +215,9 @@ def build_download_ref( elif isinstance(getattr(locked_dep, "host", None), str) and locked_dep.host != dep_ref.host: overrides["host"] = locked_dep.host + if getattr(locked_dep, "is_insecure", False) is True: + overrides["is_insecure"] = True + # Use locked commit SHA for byte-for-byte reproducibility. if locked_dep.resolved_commit and locked_dep.resolved_commit != "cached": overrides["reference"] = locked_dep.resolved_commit diff --git a/tests/unit/test_auth_scoping.py b/tests/unit/test_auth_scoping.py index 8d595547..549f0999 100644 --- a/tests/unit/test_auth_scoping.py +++ b/tests/unit/test_auth_scoping.py @@ -233,6 +233,39 @@ def test_github_host_with_token_tries_method1_first(self): assert "ghp_TESTTOKEN" in first_url assert _url_host(first_url) == "github.com" + def test_insecure_http_dep_does_not_fallback_to_ssh(self): + """HTTP deps should clone with HTTP only and never try SSH fallback.""" + dl = _make_downloader(github_token="ghp_TESTTOKEN") + dep = _dep("http://gitlab.company.internal/acme/rules.git") + + dl.auth_resolver._cache.clear() + with patch.dict(os.environ, {"GITHUB_APM_PAT": "ghp_TESTTOKEN"}, clear=True), \ + patch( + "apm_cli.core.token_manager.GitHubTokenManager.resolve_credential_from_git", + return_value=None, + ), \ + patch('apm_cli.deps.github_downloader.Repo') as MockRepo: + MockRepo.clone_from.side_effect = GitCommandError("clone", "failed") + target = Path(tempfile.mkdtemp()) + try: + with pytest.raises( + RuntimeError, + match="do not fall back to SSH or HTTPS", + ): + dl._clone_with_fallback(dep.repo_url, target, dep_ref=dep) + finally: + import shutil + shutil.rmtree(target, ignore_errors=True) + + assert MockRepo.clone_from.call_count == 1 + first_url = MockRepo.clone_from.call_args_list[0][0][0] + env_used = MockRepo.clone_from.call_args_list[0][1]["env"] + assert first_url == "http://gitlab.company.internal/acme/rules.git" + assert "GIT_ASKPASS" not in env_used + assert "GIT_CONFIG_GLOBAL" not in env_used + assert "GIT_CONFIG_NOSYSTEM" not in env_used + assert env_used.get("GIT_TERMINAL_PROMPT") == "0" + def test_generic_host_error_message_mentions_credential_helpers(self): """When all methods fail for a generic host, the error suggests credential helpers.""" dl = _make_downloader(github_token="ghp_TESTTOKEN") diff --git a/tests/unit/test_install_command.py b/tests/unit/test_install_command.py index 17e0451e..2f9a9f23 100644 --- a/tests/unit/test_install_command.py +++ b/tests/unit/test_install_command.py @@ -869,16 +869,73 @@ class TestAllowInsecureFlag: def setup_method(self): self.runner = CliRunner() + try: + self.original_dir = os.getcwd() + except FileNotFoundError: + self.original_dir = str(Path(__file__).parent.parent.parent) + os.chdir(self.original_dir) def test_http_dep_rejected_without_allow_insecure_flag(self): """Adding http:// package without --allow-insecure is rejected.""" with tempfile.TemporaryDirectory() as tmp_dir: - os.chdir(tmp_dir) - with patch("apm_cli.commands.install._validate_package_exists", return_value=True): - result = self.runner.invoke( - cli, ["install", "http://my-server.example.com/owner/repo"] + try: + os.chdir(tmp_dir) + with patch("apm_cli.commands.install._validate_package_exists", return_value=True), \ + patch("apm_cli.config.get_allow_insecure", return_value=False): + result = self.runner.invoke( + cli, ["install", "http://my-server.example.com/owner/repo"] + ) + finally: + os.chdir(self.original_dir) + assert result.exit_code != 0 + assert "allow-insecure" in result.output or "insecure" in result.output.lower() + + def test_install_help_mentions_allow_insecure_for_http_deps(self): + """Install help should mention the HTTP allow-insecure flow.""" + result = self.runner.invoke(cli, ["install", "--help"]) + + assert result.exit_code == 0 + normalized = " ".join(result.output.split()) + assert "use --allow-insecure for http:// packages" in normalized + + @patch("apm_cli.commands.install._validate_package_exists", return_value=True) + @patch("apm_cli.commands.install.APM_DEPS_AVAILABLE", True) + @patch("apm_cli.commands.install.APMPackage") + @patch("apm_cli.commands.install._install_apm_dependencies") + def test_http_dep_addition_passes_with_config_allow_insecure( + self, mock_install_apm, mock_apm_package, mock_validate + ): + """Global config allows adding an HTTP dependency without the CLI flag.""" + with tempfile.TemporaryDirectory() as tmp_dir: + try: + os.chdir(tmp_dir) + mock_pkg_instance = MagicMock() + mock_pkg_instance.get_apm_dependencies.return_value = [] + mock_pkg_instance.get_mcp_dependencies.return_value = [] + mock_apm_package.from_apm_yml.return_value = mock_pkg_instance + mock_install_apm.return_value = InstallResult( + diagnostics=MagicMock( + has_diagnostics=False, has_critical_security=False + ) ) - assert result.exit_code != 0 or "allow-insecure" in result.output.lower() or "HTTP" in result.output + + with patch("apm_cli.config.get_allow_insecure", return_value=True): + result = self.runner.invoke( + cli, ["install", "http://my-server.example.com/owner/repo"] + ) + + assert result.exit_code == 0 + with open("apm.yml", encoding="utf-8") as f: + config = yaml.safe_load(f) + + assert config["dependencies"]["apm"] == [ + { + "git": "http://my-server.example.com/owner/repo", + "allow_insecure": True, + } + ] + finally: + os.chdir(self.original_dir) def test_http_dep_validation_check(self): """_check_insecure_dependencies blocks HTTP dep without allow_insecure flag.""" diff --git a/tests/unit/test_install_update.py b/tests/unit/test_install_update.py index 05bac905..7670aa2e 100644 --- a/tests/unit/test_install_update.py +++ b/tests/unit/test_install_update.py @@ -11,6 +11,7 @@ from unittest.mock import Mock +from apm_cli.deps.lockfile import LockFile, LockedDependency from apm_cli.drift import build_download_ref, detect_config_drift, detect_orphans, detect_ref_change from apm_cli.models.apm_package import DependencyReference @@ -229,6 +230,29 @@ def test_no_host_produces_plain_repo_url(self): assert ref.repo_url == "owner/repo" assert ref.reference == "abc123def456" + def test_http_lockfile_restores_insecure_scheme(self): + """HTTP deps should restore the locked insecure scheme on replay.""" + dep = DependencyReference( + repo_url="acme/rules", + host="git.company.internal", + reference=None, + ) + lockfile = LockFile() + lockfile.add_dependency( + LockedDependency( + repo_url="acme/rules", + host="git.company.internal", + resolved_commit="abc123def456", + is_insecure=True, + allow_insecure=True, + ) + ) + + ref = build_download_ref(dep, lockfile, update_refs=False, ref_changed=False) + assert ref.host == "git.company.internal" + assert ref.reference == "abc123def456" + assert ref.is_insecure is True + class TestPreDownloadRefLockfileOverride: """Same as TestDownloadRefLockfileOverride but for the parallel pre-download path. @@ -270,6 +294,29 @@ def test_pre_download_lockfile_override_without_update(self): assert ref.reference == "abc123def456" +class TestLockedDependencyHttpRoundTrip: + """Tests for lockfile preservation of HTTP dependency metadata.""" + + def test_to_yaml_round_trip_preserves_http_fields(self): + lockfile = LockFile() + lockfile.add_dependency( + LockedDependency( + repo_url="acme/rules", + host="git.company.internal", + resolved_commit="abc123def456", + is_insecure=True, + allow_insecure=True, + ) + ) + + parsed = LockFile.from_yaml(lockfile.to_yaml()) + dep = parsed.get_dependency("acme/rules") + + assert dep is not None + assert dep.is_insecure is True + assert dep.allow_insecure is True + + class TestRefChangedDetection: """Tests for detect_ref_change() in drift.py. From 3a28c8c88e0b8addb2e5e472dff2cc61421a4612 Mon Sep 17 00:00:00 2001 From: arika0093 Date: Tue, 14 Apr 2026 07:31:39 +0000 Subject: [PATCH 03/10] refactor: simplify SSH and HTTPS handling for GitHub URLs --- src/apm_cli/deps/github_downloader.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/apm_cli/deps/github_downloader.py b/src/apm_cli/deps/github_downloader.py index 87b3a442..8aa6690c 100644 --- a/src/apm_cli/deps/github_downloader.py +++ b/src/apm_cli/deps/github_downloader.py @@ -624,16 +624,15 @@ def _build_repo_url(self, repo_ref: str, use_ssh: bool = False, dep_ref: Depende if dep_ref is not None: is_insecure = bool(getattr(dep_ref, "is_insecure", False)) if use_ssh: - # Method 2: SSH fallback return build_ssh_url(host, repo_ref) - elif is_github and github_token and not is_insecure: - # Method 1: Only send GitHub tokens to GitHub HTTPS hosts + elif is_github and github_token: + # # Only send GitHub tokens to GitHub hosts return build_https_clone_url(host, repo_ref, token=github_token) elif is_insecure: - # HTTP direct only: _clone_with_fallback() returns before Method 1/2/3. + # HTTP direct only: _clone_with_fallback() returns. return f"http://{host}/{repo_ref}.git" else: - # Method 3: Plain HTTPS, let git credential helpers handle auth + # Generic hosts: plain HTTPS, let git credential helpers handle auth return build_https_clone_url(host, repo_ref, token=None) def _clone_with_fallback(self, repo_url_base: str, target_path: Path, progress_reporter=None, dep_ref: DependencyReference = None, verbose_callback=None, **clone_kwargs) -> Repo: From 5260fbd84d376de6021dbe757dc7e2d94f69c82c Mon Sep 17 00:00:00 2001 From: Arika Ishinami Date: Tue, 14 Apr 2026 23:34:57 +0900 Subject: [PATCH 04/10] fix: trim whitespace in boolean value parsing --- src/apm_cli/commands/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/apm_cli/commands/config.py b/src/apm_cli/commands/config.py index b7fb36da..4bf6e366 100644 --- a/src/apm_cli/commands/config.py +++ b/src/apm_cli/commands/config.py @@ -24,7 +24,7 @@ def _parse_bool_value(value: str) -> bool: """Parse a CLI boolean value.""" - normalized = value.lower() + normalized = value.strip().lower() if normalized in _BOOLEAN_TRUE_VALUES: return True if normalized in _BOOLEAN_FALSE_VALUES: From fe7179f4cead0401feb356751a5dc0aa7f47c2ba Mon Sep 17 00:00:00 2001 From: Arika Ishinami Date: Tue, 14 Apr 2026 23:35:05 +0900 Subject: [PATCH 05/10] fix: remove redundant comment character in GitHub token handling --- src/apm_cli/deps/github_downloader.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/apm_cli/deps/github_downloader.py b/src/apm_cli/deps/github_downloader.py index 8aa6690c..595cbbbb 100644 --- a/src/apm_cli/deps/github_downloader.py +++ b/src/apm_cli/deps/github_downloader.py @@ -626,7 +626,7 @@ def _build_repo_url(self, repo_ref: str, use_ssh: bool = False, dep_ref: Depende if use_ssh: return build_ssh_url(host, repo_ref) elif is_github and github_token: - # # Only send GitHub tokens to GitHub hosts + # Only send GitHub tokens to GitHub hosts return build_https_clone_url(host, repo_ref, token=github_token) elif is_insecure: # HTTP direct only: _clone_with_fallback() returns. From 47b87ab1e3ffc2975dfba90c7e72737a85fd39e2 Mon Sep 17 00:00:00 2001 From: Arika Ishinami Date: Tue, 14 Apr 2026 23:35:38 +0900 Subject: [PATCH 06/10] fix: enforce boolean type for allow_insecure field in DependencyReference --- src/apm_cli/models/dependency/reference.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/apm_cli/models/dependency/reference.py b/src/apm_cli/models/dependency/reference.py index 9c153240..ef7ad798 100644 --- a/src/apm_cli/models/dependency/reference.py +++ b/src/apm_cli/models/dependency/reference.py @@ -483,7 +483,9 @@ def parse_from_dict(cls, entry: dict) -> "DependencyReference": sub_path = entry.get("path") ref_override = entry.get("ref") alias_override = entry.get("alias") - allow_insecure = bool(entry.get("allow_insecure", False)) + allow_insecure = entry.get("allow_insecure", False) + if not isinstance(allow_insecure, bool): + raise ValueError("'allow_insecure' field must be a boolean") # Validate sub_path if provided if sub_path is not None: From 61b83403ff422e75bcf2a4efc38fe60d15f263a8 Mon Sep 17 00:00:00 2001 From: Arika Ishinami Date: Tue, 14 Apr 2026 23:37:08 +0900 Subject: [PATCH 07/10] fix: normalize URL checks to be case-insensitive for dependency references --- src/apm_cli/models/dependency/reference.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/apm_cli/models/dependency/reference.py b/src/apm_cli/models/dependency/reference.py index ef7ad798..178bdc4b 100644 --- a/src/apm_cli/models/dependency/reference.py +++ b/src/apm_cli/models/dependency/reference.py @@ -540,7 +540,7 @@ def _detect_virtual_package(cls, dependency_str: str): virtual_path = None validated_host = None - if temp_str.startswith(("git@", "https://", "http://")): + if temp_str.lower().startswith(("git@", "https://", "http://")): return is_virtual_package, virtual_path, validated_host check_str = temp_str @@ -690,7 +690,9 @@ def _parse_standard_url( repo_url = repo_part.strip() # For virtual packages, extract just the owner/repo part (or org/project/repo for ADO) - if is_virtual_package and not repo_url.startswith(("https://", "http://")): + repo_url_lower = repo_url.lower() + + if is_virtual_package and not repo_url_lower.startswith(("https://", "http://")): parts = repo_url.split("/") if "_git" in parts: @@ -724,7 +726,7 @@ def _parse_standard_url( repo_url = "/".join(parts[:2]) # Normalize to URL format for secure parsing - if repo_url.startswith(("https://", "http://")): + if repo_url_lower.startswith(("https://", "http://")): parsed_url = urllib.parse.urlparse(repo_url) host = parsed_url.hostname or "" else: From 3c0122cb48f9ee994e529ae5fa0bf7beb5f39edd Mon Sep 17 00:00:00 2001 From: Arika Ishinami Date: Tue, 14 Apr 2026 23:37:37 +0900 Subject: [PATCH 08/10] fix: improve HTTP scheme detection for is_insecure field in DependencyReference --- src/apm_cli/models/dependency/reference.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/apm_cli/models/dependency/reference.py b/src/apm_cli/models/dependency/reference.py index 178bdc4b..0474fc0f 100644 --- a/src/apm_cli/models/dependency/reference.py +++ b/src/apm_cli/models/dependency/reference.py @@ -1005,7 +1005,7 @@ def parse(cls, dependency_str: str) -> "DependencyReference": ado_project=ado_project, ado_repo=ado_repo, artifactory_prefix=artifactory_prefix, - is_insecure=dependency_str.startswith("http://"), + is_insecure=urllib.parse.urlparse(dependency_str).scheme.lower() == "http", ) def to_apm_yml_entry(self): From cd8bed0c07c5e9592dde69047954eebe0969b6e3 Mon Sep 17 00:00:00 2001 From: Arika Ishinami Date: Tue, 14 Apr 2026 23:38:15 +0900 Subject: [PATCH 09/10] fix: use allow_insecure field value in DependencyReference serialization --- src/apm_cli/models/dependency/reference.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/apm_cli/models/dependency/reference.py b/src/apm_cli/models/dependency/reference.py index 0474fc0f..93c433ad 100644 --- a/src/apm_cli/models/dependency/reference.py +++ b/src/apm_cli/models/dependency/reference.py @@ -1024,7 +1024,7 @@ def to_apm_yml_entry(self): entry["ref"] = self.reference if self.alias: entry["alias"] = self.alias - entry["allow_insecure"] = True + entry["allow_insecure"] = self.allow_insecure return entry return self.to_canonical() From f6c2a3874686a8daac32e104ae0cd59bd5f6c47b Mon Sep 17 00:00:00 2001 From: Arika Ishinami Date: Tue, 14 Apr 2026 23:38:23 +0900 Subject: [PATCH 10/10] test: add case-insensitive HTTP scheme detection and whitespace handling for allow-insecure config --- tests/unit/test_canonicalization.py | 21 +++++++++++++++++++++ tests/unit/test_config_command.py | 7 +++++++ tests/unit/test_install_command.py | 3 ++- 3 files changed, 30 insertions(+), 1 deletion(-) diff --git a/tests/unit/test_canonicalization.py b/tests/unit/test_canonicalization.py index 27411965..6cf3acc4 100644 --- a/tests/unit/test_canonicalization.py +++ b/tests/unit/test_canonicalization.py @@ -471,6 +471,11 @@ def test_filter_no_cross_host_match(self): class TestHttpInsecureDeps: """Tests for HTTP (insecure) dependency parsing and serialization.""" + def test_http_scheme_detection_is_case_insensitive(self): + """Parsing an uppercase HTTP scheme still marks the ref as insecure.""" + dep = DependencyReference.parse("HTTP://my-server.example.com/owner/repo") + assert dep.is_insecure is True + def test_http_url_sets_insecure_flag(self): """Parsing an http:// URL marks the ref as insecure.""" dep = DependencyReference.parse("http://my-server.example.com/owner/repo") @@ -515,6 +520,13 @@ def test_http_to_apm_yml_entry_returns_dict(self): assert entry["git"] == "http://my-server.example.com/owner/repo" assert entry["allow_insecure"] is True + def test_http_to_apm_yml_entry_preserves_allow_insecure_false(self): + """to_apm_yml_entry() preserves an explicit False opt-in state.""" + dep = DependencyReference.parse("http://my-server.example.com/owner/repo") + entry = dep.to_apm_yml_entry() + assert isinstance(entry, dict) + assert entry["allow_insecure"] is False + def test_http_to_apm_yml_entry_includes_ref(self): """to_apm_yml_entry() includes ref when present.""" dep = DependencyReference.parse("http://my-server.example.com/owner/repo#v1.0") @@ -551,6 +563,15 @@ def test_parse_from_dict_git_http_allow_insecure_default_false(self): dep = DependencyReference.parse_from_dict(entry) assert dep.allow_insecure is False + def test_parse_from_dict_rejects_non_boolean_allow_insecure(self): + """parse_from_dict() rejects non-boolean allow_insecure values.""" + entry = { + "git": "http://my-server.example.com/owner/repo", + "allow_insecure": "false", + } + with pytest.raises(ValueError, match="'allow_insecure' field must be a boolean"): + DependencyReference.parse_from_dict(entry) + def test_http_to_github_url_uses_http_scheme(self): """to_github_url() uses http:// for HTTP deps.""" dep = DependencyReference.parse("http://my-server.example.com/owner/repo") diff --git a/tests/unit/test_config_command.py b/tests/unit/test_config_command.py index 352617f0..bba3a757 100644 --- a/tests/unit/test_config_command.py +++ b/tests/unit/test_config_command.py @@ -211,6 +211,13 @@ def test_set_allow_insecure_false(self): assert result.exit_code == 0 mock_set.assert_called_once_with(False) + def test_set_allow_insecure_strips_whitespace(self): + """Allow surrounding whitespace in boolean config values.""" + with patch("apm_cli.config.set_allow_insecure") as mock_set: + result = self.runner.invoke(config, ["set", "allow-insecure", " true "]) + assert result.exit_code == 0 + mock_set.assert_called_once_with(True) + class TestConfigGet: """Tests for `apm config get [key]`.""" diff --git a/tests/unit/test_install_command.py b/tests/unit/test_install_command.py index 2f9a9f23..cfa63503 100644 --- a/tests/unit/test_install_command.py +++ b/tests/unit/test_install_command.py @@ -842,7 +842,8 @@ def test_global_rejects_absolute_local_path(self): cli, ["install", "--global", str(local_pkg)] ) - assert "not supported at user scope" in result.output + normalized = " ".join(result.output.split()) + assert "not supported at user scope" in normalized finally: os.chdir(self.original_dir)