-
Notifications
You must be signed in to change notification settings - Fork 1
fix(security): constrain marketplace dependency installs #722
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| # TDD Evidence | ||
|
|
||
| ## Failing before implementation | ||
|
|
||
| 2026-09-06T23:50:00Z | ||
|
|
||
| `hatch run pytest -q tests/unit/registry/test_dependency_resolver.py tests/unit/registry/test_module_installer.py -k 'non_index or named_pep508 or excludes_discovered or verifies_artifact_before'` | ||
|
|
||
| Result: **failed as expected** (6 failed, 1 passed). Unsafe requirement forms reached the pip mock, discovered project metadata reached resolution, and dependency processing ran before integrity rejection. | ||
|
|
||
| ## Passing after implementation | ||
|
|
||
| 2026-09-06T23:51:00Z | ||
|
|
||
| `hatch run pytest -q tests/unit/registry/test_dependency_resolver.py tests/unit/registry/test_module_installer.py -k 'non_index or named_pep508 or excludes_discovered or verifies_artifact_before'` | ||
|
|
||
| Result: **passed** (7 passed, 45 deselected). | ||
|
|
||
| `hatch run pytest -q tests/unit/registry/test_dependency_resolver.py tests/unit/registry/test_module_installer.py tests/unit/registry/test_dependency_resolver_properties.py tests/unit/specfact_cli/registry/test_dependency_resolver_pip_free.py` | ||
|
|
||
| Result: **passed** (66 passed). | ||
|
|
||
| ## Quality gates | ||
|
|
||
| - `openspec validate fix-untrusted-module-pip-install --strict`: passed. | ||
| - `hatch run format`, `hatch run lint`, and `hatch run type-check`: passed; type check reported no errors. | ||
| - `hatch run contract-test`: passed from the cached contract result. | ||
| - `hatch run security-audit`: passed with no unreviewed vulnerabilities. | ||
| - `hatch run semgrep-sast --json-output=/tmp/specfact-semgrep.json` and the baseline gate: passed with zero findings. | ||
| - `hatch run bandit-scan`: passed with no medium/high findings. | ||
| - `hatch run verify-modules-signature`: passed for all four manifests. | ||
| - Frozen-delivery checks, `uv lock --check`, and the authoritative BasedPyright JSON run passed. | ||
| - `hatch run smart-test`: full-suite execution reached 3,078 collected tests but failed on pre-existing missing external module imports; its initial stale-lock/version assertion was corrected and passes in the focused rerun. | ||
| - `hatch run specfact code review run --scope full --json --out .specfact/code-review.json`: produced zero findings but returned UNKNOWN because all OCI analyzer capsules reported `verified cache entry is missing`. Independent Ruff, BasedPyright, Semgrep, Bandit, contract, and focused test gates passed; this environment limitation remains explicitly recorded rather than misrepresented as PASS. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| ## Context | ||
|
|
||
| Module discovery intentionally includes project modules. That metadata is useful for diagnostics, but it is not trusted installation input. Both pip's resolver and installer can execute build hooks, so the boundary must be enforced before either subprocess is reached. | ||
|
|
||
| ## Goals / Non-Goals | ||
|
|
||
| **Goals:** verify the selected artifact first, constrain automatic dependencies to index-hosted PEP 508 named requirements, and exclude discovered project metadata from pip subprocess input. | ||
|
|
||
| **Non-Goals:** add a direct-URL allowlist, replace pip's resolver, or change module discovery precedence. | ||
|
|
||
| ## Decisions | ||
|
|
||
| 1. Parse every automatic-install requirement with `packaging.requirements.Requirement` and reject URLs. Invalid PEP 508 strings thereby reject pip options and local paths. | ||
| 2. Resolve only the selected marketplace artifact's requirements. Existing discovered modules remain discoverable and available to non-install diagnostics, but their declarations never reach pip during an unrelated install. | ||
| 3. Verify the extracted artifact before recursive bundle dependency installation or pip resolution. Atomic placement retains its existing verification as defense in depth. | ||
|
|
||
| ## Rollback | ||
|
|
||
| Revert the change as one unit. Partial rollback is unsafe because syntax validation alone does not fix the cross-module trust-boundary violation. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| ## Why | ||
|
|
||
| Marketplace installation currently feeds pip dependency declarations from every discovered module, including repository-controlled project modules, into pip while installing an unrelated trusted module. Pip resolution and installation can execute package build hooks before the downloaded marketplace artifact is integrity verified. | ||
|
|
||
| ## What Changes | ||
|
|
||
| - Verify the selected marketplace artifact before resolving or installing any dependency. | ||
| - Resolve and install pip requirements only from that selected, publisher-trusted artifact. | ||
| - Reject pip options, local paths, VCS references, and direct URLs before invoking pip. | ||
| - Preserve discovered-module dependency conflict visibility without treating discovered declarations as install input. | ||
|
|
||
| ## Capabilities | ||
|
|
||
| ### New Capabilities | ||
|
|
||
| - `trusted-module-dependency-installation`: Defines the trust boundary and accepted requirement syntax for marketplace dependency installation. | ||
|
|
||
| ### Modified Capabilities | ||
|
|
||
| - `module-installation`: Marketplace modules remain installable, but dependency side effects occur only after artifact verification. | ||
|
|
||
| ## Impact | ||
|
|
||
| - Affects `registry/dependency_resolver.py`, `registry/module_installer.py`, and their unit tests. | ||
| - Direct URL, VCS, local-path, and pip-option dependency declarations become invalid for automatic marketplace installation. | ||
| - No user-facing command syntax changes; README, `docs/`, `docs/index.md`, and navigation require review but no content update because this restores the documented trust model. | ||
| - Rollback is the single security-fix commit, though rollback would reopen arbitrary code execution from repository metadata. | ||
|
|
||
| ## Source Tracking | ||
|
|
||
| - **Security report**: Aardvark, "Project module manifests trigger untrusted pip installation" | ||
| - **Repository**: nold-ai/specfact-cli | ||
| - **Public issue**: Not created to avoid disclosing an unpatched critical vulnerability. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| ## ADDED Requirements | ||
|
|
||
| ### Requirement: Marketplace module installation verifies before side effects | ||
|
|
||
| Marketplace installation SHALL validate publisher policy and artifact integrity before recursively installing bundle dependencies or invoking pip dependency resolution or installation. | ||
|
|
||
| #### Scenario: Unverified marketplace archive is rejected first | ||
|
|
||
| - **GIVEN** a marketplace archive with dependency declarations and invalid integrity metadata | ||
| - **WHEN** module installation runs | ||
| - **THEN** installation rejects the archive without dependency installation side effects |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| ## ADDED Requirements | ||
|
|
||
| ### Requirement: Automatic pip installation uses only verified selected metadata | ||
|
|
||
| The marketplace installer SHALL verify the selected artifact before dependency processing and SHALL use only that artifact's pip dependency declarations as resolver and installer input. | ||
|
|
||
| #### Scenario: Project module cannot inject a dependency | ||
|
|
||
| - **GIVEN** discovery includes a project module with a pip dependency | ||
| - **WHEN** an unrelated marketplace module is installed | ||
| - **THEN** the project dependency is not passed to dependency resolution or pip installation | ||
|
|
||
| #### Scenario: Integrity failure has no dependency side effects | ||
|
|
||
| - **GIVEN** a downloaded marketplace artifact fails integrity verification | ||
| - **WHEN** installation is attempted | ||
| - **THEN** neither bundle dependencies nor pip dependencies are installed | ||
|
|
||
| ### Requirement: Automatic requirements exclude executable pip input forms | ||
|
|
||
| Automatic marketplace dependency installation SHALL accept only valid PEP 508 named requirements without direct URL or VCS references and SHALL reject pip options and local paths before invoking pip. | ||
|
|
||
| #### Scenario: Unsafe requirement is rejected | ||
|
|
||
| - **GIVEN** a selected artifact declares a direct URL, VCS URL, local path, or pip option | ||
| - **WHEN** dependency handling begins | ||
| - **THEN** installation fails before any pip resolver or installer subprocess receives the requirement | ||
|
|
||
| #### Scenario: Named requirement remains supported | ||
|
|
||
| - **GIVEN** a selected verified artifact declares a named requirement with extras, markers, and version constraints | ||
| - **WHEN** dependency handling begins | ||
| - **THEN** the requirement may be resolved and installed normally |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| ## 1. Branch and specification | ||
|
|
||
| - [x] 1.1 Confirm work occurs on the dedicated `work` worktree branch. | ||
| - [x] 1.2 Add trust-boundary and requirement-policy spec deltas. | ||
| - [x] 1.3 Validate the OpenSpec change strictly. | ||
|
|
||
| ## 2. Test-first proof | ||
|
|
||
| - [x] 2.1 Add unit tests derived from every security scenario. | ||
| - [x] 2.2 Run focused tests before production edits and record failing evidence. | ||
|
|
||
| ## 3. Implementation | ||
|
|
||
| - [x] 3.1 Add PEP 508 named-requirement validation before pip subprocesses. | ||
| - [x] 3.2 Restrict resolution/install input to selected marketplace metadata. | ||
| - [x] 3.3 Verify marketplace artifacts before dependency side effects. | ||
| - [x] 3.4 Record passing focused-test evidence. | ||
|
|
||
| ## 4. Verification and delivery | ||
|
|
||
| - [x] 4.1 Review README, `docs/`, `docs/index.md`, and navigation impact; no update required because CLI syntax and documented workflows are unchanged. | ||
| - [x] 4.2 Run formatting, typing, lint, YAML, contract, smart-test, Semgrep, Bandit, and module-signature gates; record pre-existing/environment limitations. | ||
| - [x] 4.3 Refresh `.specfact/code-review.json`; it contains zero findings but UNKNOWN analyzer evidence because the verified OCI cache is unavailable, documented in TDD evidence. | ||
| - [x] 4.4 Bump the patch version in all four authorities and add a changelog security entry. | ||
| - [ ] 4.5 Commit the completed change. | ||
| - [ ] 4.6 Create the pull request to `dev` without publicly reproducing exploit details. | ||
| - [ ] 4.7 After merge, archive with `openspec archive fix-untrusted-module-pip-install` and clean up the worktree. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ | |
|
|
||
| from beartype import beartype | ||
| from icontract import ensure, require | ||
| from packaging.requirements import InvalidRequirement, Requirement | ||
|
|
||
| from specfact_cli.common import get_bridge_logger | ||
| from specfact_cli.models.module_package import ModulePackageMetadata | ||
|
|
@@ -29,6 +30,25 @@ class PipDependencyInstallError(Exception): | |
| """Raised when installation of resolved pip requirements fails.""" | ||
|
|
||
|
|
||
| def _unsafe_requirement_reason(requirement: str) -> str | None: | ||
| """Return why a requirement is unsafe for automatic pip execution, if applicable.""" | ||
| try: | ||
| parsed = Requirement(requirement) | ||
| except InvalidRequirement: | ||
| return "not a valid PEP 508 named requirement" | ||
| if parsed.url is not None: | ||
| return "direct and VCS URLs are not approved for automatic installation" | ||
| return None | ||
|
|
||
|
|
||
| def _validate_index_requirements(requirements: list[str]) -> None: | ||
| """Reject pip options, paths, and URL requirements before invoking pip tooling.""" | ||
| for requirement in requirements: | ||
| reason = _unsafe_requirement_reason(requirement) | ||
| if reason is not None: | ||
| raise ValueError(f"unsafe pip requirement {requirement!r}: {reason}") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win Keep unsafe-input failures out of the
Suggested exception split+class UnsafePipRequirementError(ValueError):
+ """Raised when a requirement is unsafe for automatic installation."""
+
- raise ValueError(f"unsafe pip requirement {requirement!r}: {reason}")
+ raise UnsafePipRequirementError(f"unsafe pip requirement {requirement!r}: {reason}")
+
+ except UnsafePipRequirementError:
+ raise
except ValueError as exc:
raise DependencyConflictError(str(exc)) from exc🤖 Prompt for AI Agents |
||
|
|
||
|
|
||
| @beartype | ||
| def _pip_tools_available() -> bool: | ||
| """Return True if pip-compile is available.""" | ||
|
|
@@ -159,6 +179,10 @@ def resolve_dependencies( | |
| constraints = _collect_constraints(modules) | ||
| if not constraints: | ||
| return [] | ||
| try: | ||
| _validate_index_requirements(constraints) | ||
| except ValueError as exc: | ||
| raise DependencyConflictError(str(exc)) from exc | ||
|
Comment on lines
+182
to
+185
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎. |
||
| if _pip_tools_available(): | ||
| return _run_pip_compile(constraints) | ||
| return _run_basic_resolver(constraints, allow_unvalidated=allow_unvalidated) | ||
|
|
@@ -174,6 +198,10 @@ def install_resolved_pip_requirements(pinned: list[str]) -> None: | |
| """ | ||
| if not pinned: | ||
| return | ||
| try: | ||
| _validate_index_requirements(pinned) | ||
| except ValueError as exc: | ||
| raise PipDependencyInstallError(str(exc)) from exc | ||
| if not _pip_module_available(): | ||
| logger.warning( | ||
| "pip is not available in this environment; skipping install of %s marketplace pip " | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -36,7 +36,6 @@ | |||||||||||||||||||||||
| from specfact_cli.registry.module_discovery import ( | ||||||||||||||||||||||||
| MARKETPLACE_MODULES_ROOT as DISCOVERY_MARKETPLACE_MODULES_ROOT, | ||||||||||||||||||||||||
| USER_MODULES_ROOT as DISCOVERY_USER_MODULES_ROOT, | ||||||||||||||||||||||||
| discover_all_modules, | ||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||
| from specfact_cli.registry.module_security import assert_module_allowed, ensure_publisher_trusted | ||||||||||||||||||||||||
| from specfact_cli.runtime import is_debug_mode | ||||||||||||||||||||||||
|
|
@@ -919,9 +918,10 @@ def _install_bundle_dependencies_for_module(module_id: str, ctx: _BundleDepsInst | |||||||||||||||||||||||
| dependency.version_specifier, | ||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||
| all_metas = [e.metadata for e in discover_all_modules()] | ||||||||||||||||||||||||
| all_metas.append(ctx.metadata_obj) | ||||||||||||||||||||||||
| resolved = resolve_dependencies(all_metas, allow_unvalidated=True) | ||||||||||||||||||||||||
| # Discovery includes repository-controlled project modules. They remain | ||||||||||||||||||||||||
| # available to diagnostics, but must never become pip execution input | ||||||||||||||||||||||||
| # while installing this separately selected marketplace artifact. | ||||||||||||||||||||||||
| resolved = resolve_dependencies([ctx.metadata_obj], allow_unvalidated=True) | ||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an installed marketplace module requires a package such as Useful? React with 👍 / 👎. |
||||||||||||||||||||||||
| except DependencyConflictError as dep_err: | ||||||||||||||||||||||||
| if not ctx.force: | ||||||||||||||||||||||||
| raise ValueError( | ||||||||||||||||||||||||
|
|
@@ -1016,6 +1016,16 @@ def install_module( | |||||||||||||||||||||||
| ) | ||||||||||||||||||||||||
| metadata_obj = _metadata_obj_from_install_dict(metadata, manifest_module_name) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| # Both pip resolution and recursive module installation can execute | ||||||||||||||||||||||||
| # code. Establish artifact integrity before either side effect. Atomic | ||||||||||||||||||||||||
| # placement verifies again to defend against staging-time mutation. | ||||||||||||||||||||||||
| if not verify_module_artifact( | ||||||||||||||||||||||||
| extracted_module_dir, | ||||||||||||||||||||||||
| metadata_obj, | ||||||||||||||||||||||||
| allow_unsigned=os.environ.get("SPECFACT_ALLOW_UNSIGNED", "").strip().lower() in {"1", "true", "yes"}, | ||||||||||||||||||||||||
|
Comment on lines
+1022
to
+1025
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an archive contains integrity metadata that fails Useful? React with 👍 / 👎. |
||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||
|
Comment on lines
+1022
to
+1026
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win Require integrity metadata before dependency side effects.
Proposed implementation change if not verify_module_artifact(
extracted_module_dir,
metadata_obj,
allow_unsigned=os.environ.get("SPECFACT_ALLOW_UNSIGNED", "").strip().lower() in {"1", "true", "yes"},
+ require_integrity=True,
):📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||
| raise ValueError("Downloaded module failed integrity verification") | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if not o.skip_deps: | ||||||||||||||||||||||||
| _install_bundle_dependencies_for_module( | ||||||||||||||||||||||||
| module_id, | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: nold-ai/specfact-cli
Length of output: 20168
🤖 get_repo_knowledge executed:
get_repo_knowledge nold-ai/specfact-cli /tmp/coderabbit-repo-knowledge/nold-ai-specfact-cli-93d7aac2Length of output: 4078
🏁 Script executed:
Repository: nold-ai/specfact-cli
Length of output: 20686
🌐 Web query:
site:docs.astral.sh/uv uv export --output-file atomic failure existing file💡 Result:
The uv tool's export command (uv export) is designed to write dependency information to output files [1][2]. When using the --output-file option, uv typically overwrites the existing file to keep the output in sync with the current lockfile [1][3]. There is no native atomic failure mode associated with the presence of an existing file; rather, the tool's standard behavior is to update or regenerate the specified file [4][5]. If you are encountering a failure when writing to an existing file, it is likely due to standard filesystem permissions or locking issues external to uv, rather than a built-in "atomic failure" safety feature [6][7]. If you wish to ensure that previous pins do not interfere with the current generation process, you can use the --upgrade or -U flag, which instructs uv to ignore pinned versions in any existing output file and perform a fresh resolution [4][5]. For continuous integration or automated workflows (such as pre-commit), the recommended practice is to use the uv-export hook, which manages these updates reliably [3].
Citations:
🏁 Script executed:
Repository: nold-ai/specfact-cli
Length of output: 20840
Use the atomic lock refresh command.
uv export --output-file requirements/ci/locked.txtoverwrites the destination without atomic replacement on failure. A failed export can leave the tracked lockfile incomplete. Documentpython scripts/refresh_reproducible_delivery.pyinstead, or export to a temporary file and callos.replaceonly after success.🤖 Prompt for AI Agents