-
Notifications
You must be signed in to change notification settings - Fork 1
fix(security): verify project module commands #724
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,32 @@ | ||
| # TDD Evidence | ||
|
|
||
| ## Failing before | ||
|
|
||
| - **Timestamp**: 2026-09-07T01:11Z | ||
| - **Command**: `hatch run pytest tests/unit/specfact_cli/registry/test_module_packages.py -q -k 'spoofed_official_bundle_identity or unsigned_project_module_requires_integrity_and_signature or explicit_unsigned_override_preserves_project_module_development'` | ||
| - **Result**: expected red; 2 failed and 1 passed. The spoofed manifest was accepted as `specfact-requirements`, and project registration called artifact verification without requiring integrity or a signature. | ||
|
|
||
| ## Passing after | ||
|
|
||
| - **Timestamp**: 2026-09-07T01:12Z | ||
| - **Command**: `hatch run pytest tests/unit/specfact_cli/registry/test_module_packages.py -q -k 'spoofed_official_bundle_identity or unsigned_project_module_requires_integrity_and_signature or explicit_unsigned_override_preserves_project_module_development or requirements_bundle_mounts_native'` | ||
| - **Result**: 4 passed. | ||
| - **Timestamp**: 2026-09-07T01:13Z | ||
| - **Command**: `hatch run pytest tests/unit/specfact_cli/registry/test_module_packages.py tests/unit/cli/test_lean_help_output.py -q` | ||
| - **Result**: 67 passed and 1 pre-existing migration skip. | ||
| - **Timestamp**: 2026-09-07T01:14Z | ||
| - **Commands**: `hatch run format`, `hatch run type-check`, `hatch run lint`, `openspec validate fix-project-module-command-trust --strict`, and `hatch run check-version-sources`. | ||
| - **Result**: passed; BasedPyright reported 0 errors with repository-existing warnings. | ||
| - **Timestamp**: 2026-09-07T01:16Z | ||
| - **Commands**: `hatch run semgrep-sast --json --output /tmp/specfact-semgrep.json`, `hatch run semgrep-sast-gate --results /tmp/specfact-semgrep.json --baseline tools/semgrep/sast-baseline.json`, `hatch run bandit-scan`, and `hatch run verify-modules-signature-pr --version-check-base HEAD~1`. | ||
| - **Result**: passed with no blocking Semgrep or Bandit findings and all four module manifests verified. | ||
| - **Timestamp**: 2026-09-07T01:17Z | ||
| - **Commands**: `uv lock`, `hatch run python scripts/check_reproducible_delivery.py`, and `uv lock --check`. | ||
| - **Result**: passed after synchronizing the project version in `uv.lock`. | ||
|
|
||
| ## Baseline and environment limitations | ||
|
|
||
| - `hatch run smart-test` completed 3,044 tests with 4 failures and 34 skips. Two failures were corrected by synchronizing the new patch version and lockfile; two unrelated command-audit/import failures require absent external `specfact-backlog` and `specfact-spec` modules. | ||
| - `hatch run yaml-lint` reports pre-existing long-line/blank-line findings in archived `requirements-08-bounded-red-green-proof` evidence and active `requirements-07-runtime-proof-delivery` evidence; it exits zero and reports no finding in this change. | ||
| - `hatch run specfact code review run --json --out .specfact/code-review.json --scope changed` produced the required local report, but its verdict is `UNKNOWN` because the released review module could not acquire its verified OCI analyzer cache. Independent local Ruff, BasedPyright, Semgrep, Bandit, and contract gates were run separately. | ||
| - The sibling `specfact-cli-internal` checkout was unavailable at `/workspace/specfact-cli-internal`; mirror `fix-project-module-command-trust` into `wiki/sources/` and run `python3 scripts/wiki_rebuild_graph.py` from that repository root as a follow-up. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| # Design: Project module command trust boundary | ||
|
|
||
| ## Context | ||
|
|
||
| Project discovery intentionally finds `.specfact/modules`, but discovery must not itself grant permission to execute repository Python. Registration is the narrow trust boundary immediately before lazy loaders become reachable from root CLI commands. | ||
|
|
||
| ## Decisions | ||
|
|
||
| 1. Project-sourced packages require both integrity metadata and an official-key-verifiable signature. The existing explicit unsigned override remains the opt-in escape hatch for local development. | ||
| 2. A recognized bundle string is insufficient evidence of official bundle ownership. Category mounting also requires the canonical module name `nold-ai/<bundle>`. | ||
| 3. Verification remains local and uses the existing artifact verifier and bundled key resolution. | ||
|
|
||
| ## Alternatives | ||
|
|
||
| - A new interactive trust prompt was rejected because non-interactive CI cannot safely prompt and publisher trust alone does not authenticate repository contents. | ||
| - Removing project module discovery was rejected because it would break the supported local module-development workflow. | ||
| - Special-casing only `requirements` was rejected because the same bundle-impersonation primitive applies to every official category mapping. | ||
|
|
||
| ## Risk and fallback | ||
|
|
||
| Unsigned project modules will no longer load by default. Their owners can explicitly opt into the existing unsigned development mode, or sign the artifact. No network-dependent fallback is introduced. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| # Change: Verify project modules before command registration | ||
|
|
||
| ## Why | ||
|
|
||
| Workspace-local module manifests are repository-controlled input. An unsigned project module can currently claim an official bundle name and become the root `requirements` command, causing Python import-time code to execute when a developer or CI runner asks for command help. | ||
|
|
||
| ## What Changes | ||
|
|
||
| - Require project-scoped modules to carry valid integrity metadata and a signature before their commands can be registered, unless the operator explicitly enables the existing unsigned-module override. | ||
| - Bind recognized official bundle names to their expected `nold-ai/<bundle>` module identity before mounting category groups. | ||
| - Add regression coverage proving an unsigned project module cannot register or execute and cannot impersonate the requirements bundle. | ||
|
|
||
| ## Capabilities | ||
|
|
||
| ### Modified Capabilities | ||
|
|
||
| - `module-security`: Project-discovered executable modules fail closed unless cryptographically verified or explicitly allowed as unsigned. | ||
| - `category-command-groups`: Official category bundle mounting requires the expected official module identity. | ||
|
|
||
| ## Impact | ||
|
|
||
| - **Code**: Module registration and category-group eligibility in `src/specfact_cli/registry/module_packages.py`. | ||
| - **Tests**: Focused registry tests for unsigned project modules and official bundle identity binding. | ||
| - **Compatibility**: Existing signed project modules continue to work. Deliberately unsigned project modules require the existing explicit unsigned override. | ||
| - **Offline-first**: Verification uses the bundled public key and local artifact contents; no network request is added. | ||
| - **Documentation**: Review `docs/reference/module-categories.md`, module security documentation, README, site index, and navigation; document the fail-closed project-module rule where module trust is explained. | ||
| - **Rollback**: Revert this change to restore the earlier permissive behavior; doing so reopens repository-triggered code execution and is not recommended. | ||
|
|
||
| ## Source Tracking | ||
|
|
||
| - **Security report**: Aardvark, “Requirements mount trusts unverified project bundle metadata” | ||
| - **Affected commit**: `b75f524750172eda7f1b38a3ca5bcd44f1a3dc09` | ||
| - **Repository**: `nold-ai/specfact-cli` | ||
| - **Last Synced Status**: confirmed against branch HEAD | ||
|
|
||
| ## Verification Exception | ||
|
|
||
| The mandatory SpecFact self-review report was generated and contains no code findings, but its assurance status is `UNKNOWN` because the released review module could not acquire its verified OCI analyzer cache in this environment. This narrowly documented environment exception does not waive analysis: repository Ruff, BasedPyright, Semgrep, Bandit, contract, and focused regression gates run independently, and the report details are preserved in `TDD_EVIDENCE.md`. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| ## ADDED Requirements | ||
|
|
||
| ### Requirement: Official bundle category groups are identity-bound | ||
|
|
||
| The CLI SHALL mount a recognized official bundle category only when an enabled discovered package declares both the recognized bundle and its expected canonical `nold-ai/<bundle>` module identity. | ||
|
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 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- target spec ---'
cat -n openspec/changes/fix-project-module-command-trust/specs/category-command-groups/spec.md
printf '%s\n' '--- change files ---'
find openspec/changes/fix-project-module-command-trust -maxdepth 3 -type f -print | sort
printf '%s\n' '--- trust-related references ---'
rg -n -i 'trust|signature|integrity|canonical|official bundle|mount|unsigned override|enabled discovered' \
openspec/changes/fix-project-module-command-trust src tests 2>/dev/null | head -240Repository: nold-ai/specfact-cli Length of output: 33571 🤖 get_repo_knowledge executed:
Length of output: 20488 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- module security spec ---'
cat -n openspec/changes/fix-project-module-command-trust/specs/module-security/spec.md
printf '%s\n' '--- grouping implementation ---'
cat -n src/specfact_cli/registry/module_grouping.py
printf '%s\n' '--- registry package models ---'
sed -n '1,230p' src/specfact_cli/models/module_package.py
printf '%s\n' '--- relevant tests ---'
rg -n -C 8 'spoofed_official_bundle_identity|unsigned_project_module_requires_integrity_and_signature|requirements_bundle_mounts_native|explicit_unsigned_override_preserves_project_module_development|mount.*category|category.*mount' tests/unit/specfact_cli/registry tests 2>/dev/null | head -320Repository: nold-ai/specfact-cli Length of output: 35259 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- registry/module package files ---'
find src/specfact_cli/registry tests/unit/specfact_cli/registry -maxdepth 2 -type f -print | sort
printf '%s\n' '--- registration and grouping symbols ---'
rg -n -i 'register.*module|module.*register|installed.*category|category_groups|bundle.*identity|canonical.*module|is_official|allow_unsigned|project_scope|source_scope' \
src/specfact_cli/registry tests/unit/specfact_cli/registry | head -320Repository: nold-ai/specfact-cli Length of output: 16972 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- module_packages: bundle discovery and mounting ---'
sed -n '330,430p' src/specfact_cli/registry/module_packages.py
sed -n '1240,1400p' src/specfact_cli/registry/module_packages.py
sed -n '1510,1690p' src/specfact_cli/registry/module_packages.py
printf '%s\n' '--- tests: identity, mounting, trust ---'
sed -n '380,430p' tests/unit/specfact_cli/registry/test_module_packages.py
sed -n '620,820p' tests/unit/specfact_cli/registry/test_module_packages.py
sed -n '806,880p' tests/unit/specfact_cli/registry/test_module_packages.pyRepository: nold-ai/specfact-cli Length of output: 30586 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- get_installed_bundles definition and callers ---'
rg -n -A70 -B15 '^def get_installed_bundles|def _mount_installed_category_groups|_mount_installed_category_groups\(' \
src/specfact_cli/registry/module_packages.py
printf '%s\n' '--- trust-failure and canonical-package test setup ---'
sed -n '560,635p' tests/unit/specfact_cli/registry/test_module_packages.py
sed -n '900,945p' tests/unit/specfact_cli/registry/test_module_packages.pyRepository: nold-ai/specfact-cli Length of output: 15330 Require trust-gate acceptance before category mounting. Line 5 makes enabled state and canonical identity sufficient. When trust verification skips a canonical project package, 🤖 Prompt for AI AgentsSource: Path instructions |
||
|
|
||
| #### Scenario: Project manifest impersonates requirements bundle | ||
|
|
||
| - **GIVEN** a discovered package is named `attacker/evil-requirements` | ||
| - **AND** its manifest declares bundle `specfact-requirements` and command `requirements` | ||
| - **WHEN** installed category groups are mounted | ||
| - **THEN** the package is not treated as the official requirements bundle | ||
| - **AND** no attacker loader is mounted as the root `requirements` command | ||
|
|
||
| #### Scenario: Canonical requirements module is eligible | ||
|
|
||
| - **GIVEN** an enabled verified package is named `nold-ai/specfact-requirements` | ||
| - **AND** it declares bundle `specfact-requirements` | ||
| - **WHEN** installed category groups are mounted | ||
| - **THEN** the requirements category remains eligible for root mounting | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| ## ADDED Requirements | ||
|
|
||
| ### Requirement: Project module code requires verified provenance | ||
|
|
||
| The CLI SHALL NOT register executable commands from a project-scoped module unless the artifact has valid integrity metadata and a signature verifiable with configured trusted key material, or the operator explicitly enables unsigned modules. | ||
|
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 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- spec ---'
cat -n openspec/changes/fix-project-module-command-trust/specs/module-security/spec.md
printf '%s\n' '--- change files ---'
find openspec/changes/fix-project-module-command-trust -maxdepth 3 -type f -print | sort
printf '%s\n' '--- relevant trust symbols ---'
rg -n -S "integrity|checksum|signature|trusted key|unsigned|project-scoped|module registration|register.*command|command.*register" src tests openspec 2>/dev/null | head -240Repository: nold-ai/specfact-cli Length of output: 30458 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- existing lifecycle trust contract ---'
sed -n '300,334p' openspec/specs/module-lifecycle-management/spec.md
printf '%s\n' '--- change proposal/design/tasks ---'
cat -n openspec/changes/fix-project-module-command-trust/proposal.md
cat -n openspec/changes/fix-project-module-command-trust/design.md
cat -n openspec/changes/fix-project-module-command-trust/tasks.md
printf '%s\n' '--- project module registration path ---'
rg -n -S "verify_module_integrity|verify_integrity|register_module_package_commands|require_signature|allow_unsigned" src/specfact_cli/registry tests | head -160Repository: nold-ai/specfact-cli Length of output: 21184 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- verification call path ---'
sed -n '1338,1390p' src/specfact_cli/registry/module_packages.py
sed -n '760,815p' src/specfact_cli/registry/module_installer.py
printf '%s\n' '--- focused registration tests ---'
sed -n '800,885p' tests/unit/specfact_cli/registry/test_module_packages.py
sed -n '900,945p' tests/unit/specfact_cli/registry/test_module_packages.py
printf '%s\n' '--- focused installer tests ---'
sed -n '520,620p' tests/unit/registry/test_module_installer.py
sed -n '820,855p' tests/unit/registry/test_module_installer.pyRepository: nold-ai/specfact-cli Length of output: 15728 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- signature-required tests ---'
rg -n -C 5 -S "missing.*signature|signature.*missing|require_signature|checksum_mismatch|checksum mismatch|tamper" \
tests/unit/specfact_cli/registry tests/unit/registry | head -220
printf '%s\n' '--- all scenarios in the change spec ---'
rg -n -A 5 -B 1 "#### Scenario|Requirement:" openspec/changes/fix-project-module-command-trust/specs/module-security/spec.mdRepository: nold-ai/specfact-cli Length of output: 22201 Add explicit project-module failure scenarios. The requirement requires valid integrity metadata and a trusted signature, but its scenarios do not define “valid” or cover a checksum mismatch and integrity metadata without a signature. Add scenarios that require both modules to be rejected before command registration when the unsigned override is disabled. Add the missing-signature registration regression and align the existing checksum-mismatch regression with this contract. 🤖 Prompt for AI AgentsSource: Path instructions |
||
|
|
||
| #### Scenario: Unsigned project module is discovered by default | ||
|
|
||
| - **GIVEN** a repository contains a project-scoped module with executable Python and no integrity metadata | ||
| - **AND** unsigned modules have not been explicitly allowed | ||
| - **WHEN** module package commands are registered | ||
| - **THEN** the module command is skipped | ||
| - **AND** resolving CLI help does not execute the project module Python | ||
|
|
||
| #### Scenario: Operator explicitly allows unsigned project development | ||
|
|
||
| - **GIVEN** a project-scoped module has no signature | ||
| - **AND** the operator explicitly enables the unsigned-module override | ||
| - **WHEN** module package commands are registered | ||
| - **THEN** existing unsigned development behavior is preserved | ||
|
|
||
| #### Scenario: Signed project module fails trusted-key verification | ||
|
|
||
| - **GIVEN** a project-scoped module carries integrity metadata signed by an untrusted key | ||
| - **WHEN** module package commands are registered without the unsigned override | ||
| - **THEN** the module command is skipped | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| # Tasks | ||
|
|
||
| - [x] 1.1 Create the dedicated bugfix worktree and branch. | ||
| - [x] 2.1 Add module-security and category-command-group spec deltas for project-module verification and official identity binding. | ||
| - [x] 3.1 Add unit and CLI-level regression tests derived from every security scenario. | ||
| - [x] 3.2 Run focused tests before production edits and record failing evidence. | ||
| - [x] 4.1 Require integrity and signature verification for project module registration while preserving the explicit unsigned override. | ||
| - [x] 4.2 Bind official bundle category eligibility to canonical module identity. | ||
| - [x] 4.3 Record passing focused-test evidence. | ||
| - [x] 5.1 Run formatting, type checking, linting, YAML linting, contract tests, smart tests, independent static analysis, and applicable signature verification. | ||
| - [x] 5.2 Refresh `.specfact/code-review.json`, resolve every finding, and record review evidence (the report has no findings but records unavailable OCI analyzer evidence as an environment limitation). | ||
| - [x] 6.1 Review `docs/`, README, docs index, and navigation; update module security guidance without adding a page unless necessary. | ||
| - [x] 6.2 If the sibling internal checkout is unavailable, record the follow-up to mirror this change and rebuild its wiki graph. | ||
| - [x] 7.1 Apply the required synchronized patch-version and changelog security entry. | ||
| - [ ] 8.1 Commit the completed change and create the pull request. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1207,7 +1207,8 @@ def get_installed_bundles( | |
| def _resolved_bundle(meta: Any) -> str | None: | ||
| bundle_name = getattr(meta, "bundle", None) | ||
| if isinstance(bundle_name, str) and bundle_name: | ||
| return bundle_name | ||
| expected_module_name = f"nold-ai/{bundle_name}" | ||
| return bundle_name if getattr(meta, "name", None) == expected_module_name else None | ||
|
Comment on lines
+1210
to
+1211
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 a manifest omits Useful? React with 👍 / 👎. |
||
| module_name = getattr(meta, "name", None) | ||
| if not isinstance(module_name, str) or "/" not in module_name: | ||
| return None | ||
|
|
@@ -1358,7 +1359,18 @@ def _register_service_bridges_safe(meta: Any, bridge_owner_map: dict[str, str], | |
|
|
||
|
|
||
| def _module_integrity_allows_load(package_dir: Path, meta: Any, ctx: _ModuleIntegrityContext) -> bool: | ||
| if verify_module_artifact(package_dir, meta, allow_unsigned=ctx.allow_unsigned): | ||
| verify_project_signature = getattr(meta, "source", None) == "project" and not ctx.allow_unsigned | ||
| if verify_project_signature: | ||
| verified = verify_module_artifact( | ||
| package_dir, | ||
| meta, | ||
| allow_unsigned=False, | ||
| require_integrity=True, | ||
| require_signature=True, | ||
| ) | ||
| else: | ||
| verified = verify_module_artifact(package_dir, meta, allow_unsigned=ctx.allow_unsigned) | ||
| if verified: | ||
| return True | ||
| if _is_builtin_module_package(package_dir): | ||
| ctx.logger.warning( | ||
|
|
||
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.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: nold-ai/specfact-cli
Length of output: 5498
🤖 get_repo_knowledge executed:
get_repo_knowledge nold-ai/specfact-cli /tmp/coderabbit-repo-knowledge/nold-ai-specfact-cli-93d7aac2/conventionsLength of output: 21240
🏁 Script executed:
Repository: nold-ai/specfact-cli
Length of output: 34523
Record the contract-gate outcome before closing validation.
TDD_EVIDENCE.mdrecords the smart-test result, including two unresolved external-module failures, but it does not identify the contract command or its result. Add the exact contract command and result, and state whether the smart-test failures are an accepted environment exception. Keep task5.1unchecked until this status is explicit.📍 Affects 2 files
openspec/changes/fix-project-module-command-trust/TDD_EVIDENCE.md#L29-L31(this comment)openspec/changes/fix-project-module-command-trust/tasks.md#L10-L11🤖 Prompt for AI Agents
Source: Path instructions