feat: support allow-insecure HTTP dependencies

[arika0093]

Description

This PR adds explicit support for HTTP (insecure) APM dependencies behind an opt-in flow, and makes lockfile replays preserve that HTTP source information.

When adding a package, you can explicitly allow HTTP dependencies by providing the --allow-insecure flag.

$ apm install --allow-insecure http://my-server.example.com/owner/repo

# if not provided, it will error like this:
# [*] Validating 1 package...
# [x] http://my-server.example.com/owner/repo -- 'http://my-server.example.com/owner/repo' uses HTTP (insecure). Pass '--allow-insecure' or run 'apm config set allow-insecure true' to allow HTTP dependencies.
# All packages failed validation. Nothing to install.

When restoring packages, you also need to provide the --allow-insecure flag unless the global config is enabled. If there are HTTP URLs in the dependency chain, it will error without --allow-insecure or apm config set allow-insecure true.

$ apm install --allow-insecure

# if not provided, it will error like this:
# [>] Installing dependencies from apm.yml...
# [x] Dependency 'my-server.example.com/owner/repo' uses HTTP (insecure). Pass '--allow-insecure' to apm install, or run 'apm config set allow-insecure true' to allow HTTP dependencies globally.

The apm.yml file will record it in the following format:

dependencies:
  apm:
    - git: http://my-server.example.com/owner/repo
      allow_insecure: true

HTTP dependencies require two explicit signals:

• allow_insecure: true on the dependency entry in apm.yml
• either apm install --allow-insecure or apm config set allow-insecure true

The global configuration only removes the need to pass --allow-insecure on the CLI. It does not remove the allow_insecure: true requirement in apm.yml.

apm config set allow-insecure true

Fixes #636

TODO

• Support for --allow-insecure flag when adding dependencies
• Support for --allow-insecure flag when installing dependencies
• Support for allow_insecure: true field in apm.yml and lockfile
• Support for allow-insecure global config

Type of change

• Bug fix
• New feature
• Documentation
• Maintenance / refactor

Testing

• Tested locally
• All existing tests pass
• Added tests for new functionality (if applicable)

[arika0093]

@microsoft-github-policy-service agree

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR introduces an explicit opt-in flow for allowing HTTP (insecure) APM dependencies, including persisting that decision in apm.yml and preserving HTTP metadata during lockfile replays.

Changes:

• Add --allow-insecure flag + global allow-insecure config to gate installation of http:// dependencies.
• Persist and replay HTTP dependency metadata (is_insecure, allow_insecure) via manifest and lockfile.
• Update docs and add unit tests covering HTTP parsing, lockfile round-trip, CLI behaviors, and cloning behavior.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
tests/unit/test_install_update.py	Adds lockfile replay/round-trip tests for insecure HTTP metadata.
tests/unit/test_install_command.py	Adds CLI tests for --allow-insecure and install-time security checks.
tests/unit/test_config_command.py	Adds config command + config function tests for allow-insecure.
tests/unit/test_canonicalization.py	Adds parsing/serialization tests for HTTP dependency handling.
tests/unit/test_auth_scoping.py	Ensures HTTP deps don’t fall back to SSH during clone attempts.
src/apm_cli/models/dependency/reference.py	Introduces is_insecure/allow_insecure, HTTP canonicalization, manifest serialization, and scheme-aware URLs.
src/apm_cli/drift.py	Preserves insecure scheme info during lockfile replay.
src/apm_cli/deps/lockfile.py	Persists is_insecure/allow_insecure in lockfile read/write and dependency refs.
src/apm_cli/deps/github_downloader.py	Adds HTTP-only clone path and URL building for insecure deps.
src/apm_cli/config.py	Adds get_allow_insecure / set_allow_insecure.
src/apm_cli/commands/install.py	Adds --allow-insecure, manifest writing behavior, and install-time HTTP enforcement.
src/apm_cli/commands/config.py	Generalizes config get/set to support allow-insecure.
src/apm_cli/commands/_helpers.py	Propagates HTTP/artifactory/local fields when computing install paths.
src/apm_cli/bundle/plugin_exporter.py	Propagates HTTP/artifactory/local fields for exported install paths.
packages/apm-guide/.apm/skills/apm-usage/commands.md	Documents --allow-insecure in the command reference table.
docs/src/content/docs/reference/cli-commands.md	Documents --allow-insecure and new config key.
docs/src/content/docs/guides/dependencies.md	Documents HTTP dependency manifest format + caution guidance.

[sergio-sisternes-epam]

Solid feature -- good security model with dual opt-in (dep-level + CLI/config), thorough test coverage (30+ tests), clean lockfile round-trip, and well-written docs. Nice work!

One blocking issue: to_apm_yml_entry() hardcodes allow_insecure = True, which could silently escalate security posture. See inline comment.

Also: this PR silently fixes a bug where artifactory_prefix was missing from DependencyReference construction in _helpers.py, plugin_exporter.py, and lockfile.py -- directly related to #614. Consider referencing that issue in the PR description.

[sergio-sisternes-epam]

Blocking: This hardcodes allow_insecure = True for all HTTP deps, ignoring self.allow_insecure. If this method is reused in a code path that hasn't applied the opt-in check, it silently grants insecure access.

Suggested change

	entry["allow_insecure"] = True
	entry["allow_insecure"] = self.allow_insecure

[arika0093]

addressed in commit cd8bed0.

[sergio-sisternes-epam]

Suggestion: startswith("http://") is case-sensitive, but URL schemes are case-insensitive per RFC 3986. HTTP://host/repo would bypass insecure detection.

Suggested change

	is_insecure=dependency_str.startswith("http://"),
	is_insecure=urllib.parse.urlparse(dependency_str).scheme.lower() == "http",

[arika0093]

addressed in commit 3c0122c.

[sergio-sisternes-epam]

Nit: Double # — typo from the refactor commit.

Suggested change

	# # Only send GitHub tokens to GitHub hosts
	# Only send GitHub tokens to GitHub hosts

[arika0093]

addressed in fe7179f.

[sergio-sisternes-epam]

Suggestion: bool(entry.get("allow_insecure", False)) treats any non-empty string (e.g., "false") as True. Since this is a security-relevant field, consider validating the type explicitly:

Suggested change

	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")

[arika0093]

addressed in commit 47b87ab.

[arika0093]

@sergio-sisternes-epam
Thanks for the review. I addressed the points you raised, so could you please take another look?

On #614: this PR now explicitly passes artifactory_prefix=dep.registry_prefix when reconstructing DependencyReference, which fixes a related missing-prefix propagation path. But I do not think it resolves the core #614 issue around _parse_artifactory_base_url() ignoring PROXY_REGISTRY_URL; that still appears to be unresolved.

[sergio-sisternes-epam]

All findings addressed — nice work, @arika0093! Each fix is clean and well-tested.

Thanks for the clarification on #614 — you're right that the artifactory_prefix propagation fix here is a partial improvement but the core _parse_artifactory_base_url() issue remains separate.

LGTM!

[danielmeppiel]

First — thanks for tackling this, @arika0093. The use case is real (internal mirrors without HTTPS exist in the wild), the dual opt-in design (per-dep allow_insecure: true + CLI flag) is the right shape, and the test coverage is thorough. Sergio's APPROVE isn't wrong on the implementation quality.

I'm requesting changes from a security/UX lens because APM is a supply-chain tool, and a few aspects of the current design create footguns that I don't think we can ship to the broader community. None of these are deal-breakers — they're scope tightening. If we land them, this becomes a feature I'd be proud to ship.

Blockers

1. Drop apm config set allow-insecure true (the persistent global config)

This is the biggest single risk in the PR and, IMO, the only true blocker.

Per-invocation --allow-insecure is the right friction ceiling. A persistent global setting:

• Means a developer enables it once for one internal project and then forever silently allows HTTP for every subsequent apm install on that machine, including in unrelated repos.
• Turns shared CI runners into a network-MITM surface for any repo that lands allow_insecure: true in apm.yml — the runner doesn't need to opt in per-job, the maintainer who set the config months ago already did.
• Is invisible at install time. There's no per-install reminder. Users forget they set it.
• Converts a per-action security decision into ambient permission, which is the textbook supply-chain footgun.

The per-invocation flag is the correct model. It's friction, but the friction is the security feature: every HTTP install requires an active, conscious choice. Please drop the apm config set allow-insecure path entirely (and the corresponding get_allow_insecure / set_allow_insecure config functions).

2. Be loud at install time about which URLs are insecure

Today, the user only sees output about HTTP when --allow-insecure is missing (the error path). On the success path there's no equivalent loud diagnostic listing exactly which URLs are about to be fetched over an unauthenticated channel.

Compare to pip's behavior on insecure indexes: a clear warning per fetch. APM should do the same.

Concretely:

• Print one [!] line per HTTP dependency at install-prepare time, with the full URL: [!] Fetching insecurely (no transport auth): http://internal.example.com/team/foo
• Make this print whether or not --allow-insecure was passed (i.e. the warning is informational; the flag is the gate).
• This is what makes Alice notice when Bob's apm.yml grew an unexpected HTTP entry between git pulls.

3. Transitive HTTP dependencies need a separate gate

This is the most subtle one and the one I'd most appreciate your thinking on. Scenario:

• Bob adds a top-level dep http://internal.bob.example/foo/bar with allow_insecure: true to the project's apm.yml. Alice can see this in the diff and consents by passing --allow-insecure.
• But Bob's package itself has an apm.yml that pulls a transitive dep http://attacker-controlled.example/lib/x over HTTP, also with allow_insecure: true.
• When Alice runs apm install --allow-insecure, that transitive dep gets fetched too. Alice never saw it in the project's own apm.yml. Her consent at the root extends silently to a graph she didn't review.

The lockfile makes this auditable after it's pinned, but the first install (and any update) doesn't surface this distinction.

Suggested approach (open to alternatives):

• Track per-dep whether the HTTP+allow_insecure choice originated in the root apm.yml or in a transitive dep.
• For transitive HTTP, require an additional explicit acknowledgment: either a separate --allow-insecure-transitive flag, or an interactive prompt listing the transitive HTTP URLs and their introducing parent, with --yes to skip in CI.
• At minimum, the per-URL warning from #2 should annotate (transitive, introduced by <parent-dep>) so users can see it.

Without this, the dual-opt-in only protects the immediate dep — the transitive surface is a single root flag away from being silently expanded.

Non-blocking but please consider

• Threat-model docs: the new HTTP section in dependencies.md documents the mechanism but not the why. A short paragraph explaining that HTTP has no transport auth (so an MITM can substitute the package contents — the SHA in the lockfile is itself MITM-able when fetched over HTTP) would help users make informed choices instead of copy-pasting the example.
• Audit affordance: there's no way to ask "list every insecure dep in my dependency graph" before running install. With the lockfile already preserving is_insecure, an apm deps list --insecure (or apm install --dry-run listing them) would be cheap and high-value. Could be a follow-up.
• CHANGELOG entry: this is a security-sensitive feature. The CHANGELOG entry should be explicit that this introduces an opt-in HTTP path with the threat-model implications, not just "feat: support http deps."
• Hostname allowlist (future, not now): a future iteration could let apm.yml declare an insecure_hosts: [...] allowlist so that HTTP is only permitted from explicitly-named hosts. Not for this PR.

What I'd merge

If you land #1 (drop global config) and #2 (loud per-URL output), I'd merge. #3 (transitive gating) is the one I feel strongest about from a supply-chain perspective; if you'd prefer to defer it, I'd accept that as a follow-up issue only if the per-URL warning from #2 explicitly annotates transitive HTTP deps so the information is at least visible.

If you'd rather not drop the global config, I'd respectfully decline this PR and recommend users set up HTTPS on their internal mirrors. APM is a supply-chain tool and persistent ambient "allow insecure" is incompatible with that role.

Thanks again for the thoughtful design and the thorough tests — this is salvageable and worth landing well.

[arika0093]

Thanks for the thoughtful review. I think your concerns are reasonable, especially given that APM is a supply-chain tool.

I agree on the first two blockers:

• I will drop the persistent global allow-insecure config.
• I will make insecure fetches loud on the success path as well, with one warning per HTTP dependency showing the full URL.

On transitive HTTP dependencies: I agree this is the trickiest part. A root-level opt-in should not silently bless an unreviewed transitive graph.

After thinking more about it, I do not think a simple --allow-insecure-transitive flag is the right long-term shape, because it broadens trust based on graph position rather than which host is actually being trusted.

For this PR, my plan is:

• a root HTTP dependency allowed via --allow-insecure will also allow transitive HTTP dependencies from the same host
• transitive HTTP dependencies from a different host will still be blocked by default
• for non-interactive environments, I will add an explicit host-level opt-in for additional hosts (for example, a repeatable --allow-insecure-host <host> flag)
• install output will clearly warn for every HTTP dependency, including whether it is transitive and which parent introduced it

I think this is a better narrow fix for the security concern raised here, while also fitting the common internal-repository use case more naturally than a boolean transitive flag.

I will also update the docs and changelog accordingly. The audit affordance (apm deps list --insecure) also makes sense to me.

Thanks again for the careful review.

[danielmeppiel]

APM Review Panel -- PR #700

[x] Verdict: REQUEST CHANGES -- one CRITICAL credential-leak finding plus a converging architectural/security issue around HTTP identity. The dual-opt-in design is sound and Sergio's earlier blocker is correctly fixed; the items below are what stand between this and merge.

@arika0093 -- this is a substantial, security-sensitive contribution and the design instincts are right. Five reviewers (python-architect, cli-logging-expert, devx-ux-expert, supply-chain-security-expert; growth-hacker side-channel) converged on a tight set of must-fixes. Thank you for the depth here -- 1338 lines + 30+ tests on a class of feature most projects ship sloppily.

What's excellent

• Dual opt-in (manifest allow_insecure: true + CLI --allow-insecure) is the right mental model. Matches npm overrides + --legacy-peer-deps. Neither alone is sufficient -- correct.
• Persistent global config dropped per earlier review feedback. Right call; every install run should be a conscious act.
• --allow-insecure-host for transitive deps with same-host auto-propagation for direct deps is well-designed. Good FQDN validation prevents flag injection.
• parse_from_dict() validates allow_insecure is boolean -- prevents YAML type confusion.
• Doc surface coverage (cli-commands.md, dependencies.md, apm-guide skill resources, CHANGELOG) is exactly the discipline this codebase demands.

Required changes (must land in this PR)

1. [x] CRITICAL -- HTTP clone leaks credentials in plaintext. github_downloader.py _env_for(use_token=False) strips GIT_ASKPASS and GIT_CONFIG_NOSYSTEM for HTTP attempts, re-enabling system git credential helpers (macOS Keychain, Windows Credential Manager, gh auth). When git clones http://internal.example.com/owner/repo, credential helpers may resolve stored tokens for that host and embed them as plaintext Authorization headers. The existing test test_insecure_http_dep_is_strict_by_default asserts "GIT_ASKPASS" not in env_used -- that test is verifying the vulnerability.

   Fix: HTTP attempts must actively block credential resolution:

   if attempt.scheme == "http":
       env = dict(self.git_env)
       env['GIT_ASKPASS'] = 'echo'
       env['GIT_TERMINAL_PROMPT'] = '0'
       env['GIT_CONFIG_NOSYSTEM'] = '1'
       # Suppress credential helpers explicitly
       env['GIT_CONFIG_COUNT'] = '1'
       env['GIT_CONFIG_KEY_0'] = 'credential.helper'
       env['GIT_CONFIG_VALUE_0'] = ''
       return env

   Update the test to assert credential-helper suppression for HTTP rather than its absence.

2. [x] BLOCKER -- HTTP/HTTPS identity collision enables transport downgrade. get_unique_key() and get_identity() are scheme-blind, and drift.py documents "Source/host/scheme changes -- not detected." That means https://gitlab.com/team/rules and http://gitlab.com/team/rules collide as the same dep. An attacker submitting a PR that flips the scheme + adds allow_insecure: true produces zero lockfile drift signal. Combined with finding #1, this is a clean transport-downgrade exploit chain.

   Fix: Either (a) get_identity() includes scheme when is_insecure=True, or (b) drift.py treats a is_insecure flip as drift requiring --update. Option (b) is the smaller change and preserves identity stability for non-HTTP deps.

3. [x] BLOCKER -- Inverted import: install/phases/resolve.py imports from commands/install.py. resolve.py:297-300:

   from apm_cli.commands.install import (
       _check_insecure_dependencies, _collect_insecure_dependency_infos,
       _guard_transitive_insecure_dependencies, _warn_insecure_dependencies,
   )

   Domain code (install/phases/) must never import from CLI command code. Extract the ~200 lines of insecure-policy logic + _InsecureDependencyInfo dataclass into a new src/apm_cli/install/insecure_policy.py. Both commands/install.py and install/phases/resolve.py then import from the new module. Pure move, zero logic change.

4. [x] BLOCKER -- drift.py propagates is_insecure but not allow_insecure during lockfile replay. Lines +250-252: build_download_ref() copies is_insecure=True but drops allow_insecure. The replayed DependencyReference has is_insecure=True, allow_insecure=False, which _check_insecure_dependencies() rejects. Lockfile replays of HTTP deps are broken (or worse, bypass the check via a different code path).

   if getattr(locked_dep, "is_insecure", False) is True:
       overrides["is_insecure"] = True
       overrides["allow_insecure"] = getattr(locked_dep, "allow_insecure", False)

5. [x] BLOCKER -- to_canonical() no longer strips http://, breaking the canonical identity contract. Architect + DevX both flagged this. The docstring update silently removes "no https://" and HTTP deps now produce http://host/owner/repo while HTTPS still strips. Every consumer that does dep.to_canonical().split("/") or assumes no :// will silently diverge for HTTP deps. Fix: keep to_canonical() scheme-free. The transport-aware string already exists as to_apm_yml_entry() -- use that for serialization paths and any surface that needs the scheme.

6. [!] HIGH -- Add LockedDependency.to_dependency_ref() factory. Three sites reconstruct DependencyReference from LockedDependency: _helpers.py:138, plugin_exporter.py:399, lockfile.py:365. This PR already had to fix the missing artifactory_prefix (#614 root cause) AND add is_insecure/allow_insecure at all three. Per the "abstract when 3+ call sites" rule, add a factory method that maps every field once. This is the structural fix Sergio asked about -- the artifactory_prefix patch is a band-aid without it.

7. [!] HIGH -- Unify the three competing error messages into one canonical recipe. Today a user sees three different errors depending on which gate failed:

   • validation-time ("Pass --allow-insecure"),
   • install-time/manifest-missing ("set allow_insecure: true"),
   • install-time/flag-missing ("Pass --allow-insecure").

   None give the complete recipe. Collapse to one message that always names both requirements:

   [x] http://my-server.example.com/owner/repo -- HTTP dependency (no transport encryption)
       To install:
         1. Set allow_insecure: true on the dep in apm.yml
         2. Pass --allow-insecure to apm install
   

   And use the full URL (with scheme) in every error -- the user needs to see why it's insecure.

8. [!] HIGH -- Update docs/src/content/docs/enterprise/security.md. This PR introduces an entirely new transport surface and threat model; per the repo rule ("If a code change weakens or contradicts any guarantee in security.md, the doc must be updated in the same PR"), security.md needs an "HTTP (insecure) dependencies" section covering: what allow_insecure does/doesn't protect against, MITM impact on content-hash provenance, credential-helper isolation requirement, the transitive-host boundary model.

9. [!] HIGH -- Logging routing: dead else: _rich_*() fallback branches. _warn_insecure_dependencies, _guard_transitive_insecure_dependencies, _check_insecure_dependencies repeat if logger: logger.X() else: _rich_X(...) 7 times. Callers always have a logger; the else branches are dead code and bypass DiagnosticCollector if ever triggered. Make logger a required positional arg; drop every fallback branch.

Polish (same PR if cheap; follow-up otherwise)

10. [i] Library code calls sys.exit(1). _check_insecure_dependencies and _guard_transitive_insecure_dependencies should raise an exception; CLI layer catches and exits. Improves testability and reuse.

11. [i] _format_insecure_dependency_warning uses jargon ("no transport auth"). Reword:

    [!] Insecure HTTP fetch (unencrypted): http://...
    

12. [i] Transitive-block error is a policy lecture. Drop the middle sentence; lead with the command. Move the "why" to docs.

13. [i] apm deps list --insecure column label "HTTP" misleads (values are direct / via <parent>). Rename to Origin.

14. [i] Unrelated cleanups (config.py refactor, _setup_git_environment SSH cleanup) belong in separate commits for bisectability.

Worth a follow-up issue (don't block this PR)

• APM_ALLOW_INSECURE=1 env var counterpart for CI ergonomics (mirror APM_ALLOW_PROTOCOL_FALLBACK=1 pattern). Not adding this is fine for v1; tracking issue acceptable.
• Drift-detection enhancement: warn on first-time HTTP install without lockfile SHA pin (defense-in-depth against the hash-circular-trust limitation).

Merge call

This is solid first-PR work on a security-sensitive feature -- the dual-opt-in design and host scoping are correct. The blockers are concentrated in three areas: credential leakage (#1), identity model (#2, #5), and architectural cleanup (#3, #4, #6). Items #1-#5 are the merge-blockers; #6-#9 should land in the same PR; the rest is polish or follow-up. Please address #1-#9 in this PR and re-request review.

Growth angle

• Real adoption blocker unlocked: corporate Gitea/Gogs/Artifactory-on-HTTP and air-gapped enterprise users -- the segment that quietly worked around APM's "install from anywhere" claim with manual git clone. Worth a release-notes call-out.
• First-time NONE-association contributor shipping 1338 lines + 30+ tests on a security feature is the contributor signal worth amplifying once this lands. Suggested release-notes line: "APM now supports private HTTP git hosts with explicit dual-gate opt-in (manifest + CLI). Insecure by default? Never. Insecure when you need it, with a clear audit trail." Repostable hook: "APM doesn't block your infra -- it makes insecure choices visible."
• Once shipped, security.md becomes the authoritative reference for the threat model -- making the doc update in #8 above doubly load-bearing.

_{Reviewed via the apm-review-panel skill: python-architect, cli-logging-expert, devx-ux-expert, supply-chain-security-expert; arbitrated by apm-ceo with oss-growth-hacker side-channel.}

[arika0093]

Thanks for the detailed review. I addressed all requested changes from this review, rewrote the stack so the commits are easier to map back to the review items, and kept the split granularity intact.

Review item mapping:

1. HTTP git attempts now suppress credential helpers: b5ce27e
2. HTTP/HTTPS transport flips now show up as drift: c67cfde
3. Insecure policy logic was extracted out of CLI command code into install/insecure_policy.py: 9032b9e
4. Lockfile replay now preserves allow_insecure for HTTP dependencies: 1fca10b
5. to_canonical() is scheme-free again: caeb13d
6. LockedDependency.to_dependency_ref() now centralizes the mapping: 85be6d1
7. HTTP remediation errors were unified into one canonical recipe: f08a716
8. docs/src/content/docs/enterprise/security.md was updated for the HTTP threat model: 17e40a3
9. The insecure policy helpers now require a logger and no longer keep dead fallback branches: 32cb21f
10. Library-level insecure policy helpers now raise exceptions instead of calling sys.exit(1): b067ab7
11. The insecure HTTP warning wording was tightened: 2dd90af
12. The transitive HTTP remediation error was shortened and now leads with the command: 783450f
13. apm deps list --insecure now uses Origin instead of HTTP: b8a5c5f

For review item 14:

• The unrelated commands/config.py refactor was split into its own commit for bisectability: 333e8f0
• The _setup_git_environment() / GIT_SSH_COMMAND SSH cleanup mentioned in that item was not introduced by this PR stack. It predates this branch, so there was no PR-local change to split out for that part.

At this point, all review items from this review have been addressed in the branch.