-
Notifications
You must be signed in to change notification settings - Fork 1
fix(security): isolate Requirements fixture discovery #725
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,26 @@ | ||
| # TDD Evidence: security-requirements-evidence-exclusive-discovery | ||
|
|
||
| ## Failing-before | ||
|
|
||
| - **Command**: `hatch run pytest tests/unit/registry/test_module_discovery.py::test_exclusive_module_roots_reject_project_and_user_shadowing tests/unit/scripts/test_requirements_evidence_delivery_gate.py::test_failed_command_writes_missing_diagnostic_reports_and_exports_fixture_roots tests/unit/workflows/test_requirements_evidence_delivery_workflow.py::test_required_requirements_context_is_pull_request_only -q` | ||
| - **Result**: 2 failed, 1 passed. | ||
| - **Evidence**: discovery selected the repository-local same-identity module instead of the explicit fixture, and the adapter did not set an exclusive-discovery control. | ||
|
|
||
| ## Passing-after | ||
|
|
||
| - **Command**: `hatch run pytest tests/unit/registry/test_module_discovery.py tests/unit/scripts/test_requirements_evidence_delivery_gate.py tests/unit/workflows/test_requirements_evidence_delivery_workflow.py -q` | ||
| - **Result**: 49 passed, 4 skipped because the optional pinned fixture checkout is unavailable. | ||
| - **Evidence**: exclusive discovery retained the bundled root and verified explicit root while excluding project, user, marketplace, custom, and legacy roots; adapter and workflow contracts require the control. | ||
|
|
||
| ## Quality gates | ||
|
|
||
| - `hatch run format`, `hatch run type-check`, and `hatch run lint` passed. BasedPyright reported the repository's existing warning baseline with 0 errors; Ruff reported no findings. | ||
| - `hatch run yaml-lint` completed with pre-existing errors in archived Requirements evidence and the active Requirements 07 evidence ledger; no changed YAML file produced a finding. | ||
| - `hatch run openspec validate security-requirements-evidence-exclusive-discovery --strict`, `hatch run python scripts/check_reproducible_delivery.py`, and `uv lock --check` passed. | ||
| - Semgrep SAST and its baseline gate passed with 0 findings; Bandit reported no medium/high findings. | ||
| - Changed-scope `specfact code review run` passed with no findings after installing its declared BasedPyright and Pylint review tools. | ||
| - `hatch run contract-test` used the valid cached baseline. `hatch run smart-test` ran the full suite but reported two unrelated missing fixture-package imports (`specfact_backlog` and `specfact_spec`); the 49-test focused affected-scope suite remained green. | ||
|
|
||
| ## Internal wiki follow-up | ||
|
|
||
| The sibling `specfact-cli-internal` checkout is unavailable in this environment. After it is available, add or update `wiki/sources/security-requirements-evidence-exclusive-discovery.md` and run `python3 scripts/wiki_rebuild_graph.py` from that repository root. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| # Proposal: security-requirements-evidence-exclusive-discovery | ||
|
|
||
| ## Why | ||
|
|
||
| The Requirements evidence delivery gate verifies a pinned external module fixture, but normal module discovery still examines repository-local and user-controlled roots before or alongside that fixture. A pull request can therefore shadow the trusted Requirements package with an unsigned project module and execute attacker-controlled Python while fabricating a passing report. | ||
|
|
||
| ## What Changes | ||
|
|
||
| - Add an explicit module-discovery mode that limits dynamic package discovery to `SPECFACT_MODULES_ROOTS` (while retaining bundled core modules). | ||
| - Require both the local adapter and pull-request evidence workflow to enable that mode when invoking the pinned Requirements fixture. | ||
| - Add regressions proving project, user, marketplace, custom, and legacy roots cannot shadow an exclusive fixture invocation. | ||
|
|
||
| ## Capabilities | ||
|
|
||
| ### New Capabilities | ||
|
|
||
| - `trusted-module-fixture-discovery` | ||
|
|
||
| ## Impact | ||
|
|
||
| - Affected code: module discovery and the Requirements evidence adapter. | ||
| - Affected workflow: `.github/workflows/requirements-evidence.yml`. | ||
| - Affected tests: focused discovery, adapter, and workflow contract tests. | ||
| - Compatibility: ordinary CLI discovery remains unchanged; exclusivity is opt-in for trusted fixture execution. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| ## ADDED Requirements | ||
|
|
||
| ### Requirement: Trusted fixture discovery is exclusive | ||
|
|
||
| Security-sensitive delivery gates that execute a verified external module fixture SHALL restrict dynamic module discovery to the explicitly configured fixture roots while retaining bundled core commands. | ||
|
|
||
| #### Scenario: Prevent untrusted module shadowing | ||
|
|
||
| - **GIVEN** a verified Requirements fixture and an untrusted module with the same identity in a project, user, marketplace, custom, or legacy discovery root | ||
| - **WHEN** local or CI requirements evidence enforcement invokes the released command | ||
| - **THEN** dynamic module discovery uses only the explicit verified fixture root | ||
| - **AND** bundled core commands remain available | ||
| - **AND** no untrusted same-identity module is imported or executed. | ||
|
|
||
| #### Scenario: Preserve ordinary module discovery | ||
|
|
||
| - **GIVEN** an ordinary CLI invocation that does not request exclusive discovery | ||
| - **WHEN** SpecFact discovers dynamic modules | ||
| - **THEN** existing project, explicit, user, marketplace, custom, and legacy discovery behavior remains unchanged. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| # Tasks: security-requirements-evidence-exclusive-discovery | ||
|
|
||
| ## 1. Specification and failing evidence | ||
|
|
||
| - [x] 1.1 Add the exclusive-discovery security delta. | ||
| - [x] 1.2 Add focused regressions for project-module shadowing and gate environment isolation. | ||
| - [x] 1.3 Run the focused regressions against HEAD and record failing-before evidence. | ||
|
|
||
| ## 2. Implementation | ||
|
|
||
| - [x] 2.1 Make exclusive discovery retain bundled modules and explicit fixture roots only. | ||
| - [x] 2.2 Enable exclusive discovery in the local evidence adapter and pull-request workflow. | ||
|
|
||
| ## 3. Verification | ||
|
|
||
| - [x] 3.1 Run focused tests and record passing-after evidence. | ||
| - [x] 3.2 Run required formatting, typing, linting, YAML, security, and changed-scope review gates. | ||
| - [x] 3.3 Validate the OpenSpec change strictly. | ||
| - [x] 3.4 Update the internal wiki mirror and graph when the sibling checkout is available; otherwise record the follow-up. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -143,6 +143,39 @@ def test_explicit_module_roots_take_priority_over_user_installs( | |
| assert sources["code-review"] == "custom" | ||
|
|
||
|
|
||
| def test_exclusive_module_roots_reject_project_and_user_shadowing( | ||
| tmp_path: Path, monkeypatch: pytest.MonkeyPatch | ||
| ) -> None: | ||
| """Trusted fixture discovery must ignore every non-bundled implicit root.""" | ||
| repo_root = tmp_path / "repo" | ||
| project_root = repo_root / ".specfact" / "modules" | ||
| builtin_root = tmp_path / "builtin" | ||
| explicit_root = tmp_path / "verified-fixture" / "packages" | ||
| user_root = tmp_path / "user-modules" | ||
| marketplace_root = tmp_path / "marketplace-modules" | ||
| custom_root = tmp_path / "custom-modules" | ||
| _write_manifest(builtin_root, "init") | ||
| for untrusted_root in (project_root, user_root, marketplace_root, custom_root): | ||
| _write_manifest(untrusted_root, "nold-ai-specfact-requirements") | ||
| _write_manifest(explicit_root, "nold-ai-specfact-requirements") | ||
|
Comment on lines
+158
to
+160
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 | 🟡 Minor | ⚡ Quick win Make the regression detect every excluded root. The test gives the explicit root and several untrusted roots the same module name. The explicit root is processed before user, marketplace, and custom roots, so the final source list can still pass even if those roots are scanned. The test also does not create a module in a legacy root. Use distinct module names per untrusted root, or spy on As per path instructions, tests must cover meaningful success and failure scenarios. Also applies to: 173-176 🤖 Prompt for AI AgentsSource: Path instructions |
||
| monkeypatch.chdir(repo_root) | ||
| monkeypatch.setenv("SPECFACT_MODULES_ROOTS", str(explicit_root)) | ||
| monkeypatch.setenv("SPECFACT_MODULES_EXCLUSIVE", "1") | ||
|
|
||
| discovered = discover_all_modules( | ||
| builtin_root=builtin_root, | ||
| user_root=user_root, | ||
| marketplace_root=marketplace_root, | ||
| custom_root=custom_root, | ||
| include_legacy_roots=True, | ||
| ) | ||
|
|
||
| assert [(entry.metadata.name, entry.source) for entry in discovered] == [ | ||
| ("init", "builtin"), | ||
| ("nold-ai-specfact-requirements", "custom"), | ||
| ] | ||
|
|
||
|
|
||
| def test_project_shadow_warning_is_actionable_and_emitted_once(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: | ||
| """Project-over-user shadow guidance should be user-facing but deduplicated per process.""" | ||
| repo_root = tmp_path / "repo" | ||
|
|
||
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.
This isolates only the producer's evidence and reconcile commands. The fresh consumer at line 1604 and the final verdict at line 2494 invoke
isolated_specfactwithoutSPECFACT_MODULES_EXCLUSIVE; because those processes still receiveSPECFACT_MODULES_ROOTSand run from the pull-request workspace, normal discovery checks.specfact/modulesbefore the verified fixture. A PR containing an unsigned same-identity project module can therefore shadow the Requirements fixture during the consumer and branch-protected final verification, leaving the security bypass this change is intended to close.Useful? React with 👍 / 👎.