-
Notifications
You must be signed in to change notification settings - Fork 1
fix(security): confine IDE prompt exports #721
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,40 @@ | ||
| # TDD Evidence | ||
|
|
||
| ## Failing before implementation | ||
|
|
||
| - Timestamp: 2026-09-06T23:20Z | ||
| - Command: `hatch run pytest -q tests/unit/utils/test_ide_setup.py -k 'symlinked_export_root or preserves_unrelated_directory'` | ||
| - Result: **failed as expected** (`1 failed, 1 passed`). The production path | ||
| removed the external `core` directory and wrote `specfact.test.md` through | ||
| the repository-controlled `.cursor/commands` symlink instead of raising. | ||
|
|
||
| ## Passing after implementation | ||
|
|
||
| - Timestamp: 2026-09-06T23:22Z | ||
| - Command: `hatch run pytest -q tests/unit/utils/test_ide_setup.py -k 'symlinked_export_root or preserves_unrelated_directory'` | ||
| - Result: **passed** (`2 passed, 28 deselected`). | ||
| - Command: `hatch run pytest -q tests/unit/utils/test_ide_setup.py` | ||
| - Result: **passed** (`30 passed`). | ||
| - Command: `hatch run pytest -q tests/unit/utils/test_ide_setup.py tests/unit/security/test_release_promotion_security_gates.py::test_patch_release_uses_next_version_in_all_sources tests/unit/scripts/test_reproducible_delivery.py::test_reproducible_delivery_checker_verifies_hashed_export` | ||
| - Result: **passed** (`32 passed`). | ||
|
|
||
| ## Quality gates | ||
|
|
||
| - `hatch run format`: passed; 987 files unchanged. | ||
| - `hatch run type-check`: passed with 0 errors (repository baseline warnings were reported). | ||
| - `hatch run lint`: passed with 0 errors and 0 warnings. | ||
| - `hatch run yaml-lint`: command completed but reported pre-existing errors in archived and unrelated active evidence YAML. | ||
| - `openspec validate security-01-confine-ide-prompt-exports --strict`: passed. | ||
| - `hatch run python scripts/check_reproducible_delivery.py`: passed. | ||
| - `uv lock --check`: passed. | ||
| - `hatch run check-pypi-ahead`: passed; 0.55.5 is ahead of PyPI 0.55.4. | ||
| - `hatch run semgrep-sast --json --output /tmp/specfact-semgrep.json` and gate: passed with 0 findings. | ||
| - `hatch run bandit-scan`: passed with no medium/high findings. | ||
| - `hatch run verify-modules-signature`: passed for all four manifests. | ||
| - `hatch run smart-test`: 3043 passed and 34 skipped; four unrelated failures came from missing external bundle packages plus release/lock assertions that were subsequently corrected. The two corrected release/delivery tests pass in the focused 32-test run above. | ||
| - `hatch run specfact code review run --json --out .specfact/code-review.json`: produced no findings, but failed closed because the installed review module lacked verified OCI analyzer cache entries; all required analyzer evidence was `UNKNOWN`. | ||
|
|
||
| ## Documentation and internal wiki review | ||
|
|
||
| - Reviewed `README.md`, `docs/`, `docs/index.md`, and navigation impact. No documentation change is needed for this repository-bound security correction. | ||
| - The sibling `specfact-cli-internal` checkout is unavailable. Follow-up: add/update `wiki/sources/security-01-confine-ide-prompt-exports.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,33 @@ | ||
| ## Context | ||
|
|
||
| IDE export paths are repository-relative configuration values, but resolving an | ||
| attacker-controlled symlink turns the effective cleanup and write target into | ||
| an external directory. Child-relative checks cannot establish repository | ||
| containment after the root has already escaped. | ||
|
|
||
| ## Decision | ||
|
|
||
| Validate the unresolved export root before any filesystem mutation. The root | ||
| must not be a symlink and its resolved path must be strictly beneath the | ||
| resolved repository path. Cleanup helpers return without side effects when the | ||
| root is unsafe, while export entry points raise a clear error before creating | ||
| or writing files. | ||
|
|
||
| Cleanup remains narrowly limited to existing SpecFact legacy naming and flat | ||
| `specfact*` output patterns; unrelated directories are not ownership evidence | ||
| and remain untouched. | ||
|
Comment on lines
+16
to
+18
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. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
printf '%s\n' '--- changed specification ---'
cat -n openspec/changes/security-01-confine-ide-prompt-exports/design.md | sed -n '1,120p'
printf '%s\n' '--- changed files ---'
git diff --stat
printf '%s\n' '--- relevant symbols and callers ---'
rg -n --glob '!*.lock' 'init_ide|ide_setup|prompt|specfact\*|cleanup|rmtree|unlink|remove' src tests openspec | head -240Repository: nold-ai/specfact-cli Length of output: 32911 🤖 get_repo_knowledge executed:
Length of output: 24747 🏁 Script executed: #!/bin/bash
set -u
printf '%s\n' '--- IDE-related source files ---'
git ls-files | grep -Ei '(^|/)([^/]*ide[^/]*|[^/]*prompt[^/]*)\.(py|pyi)$' | head -120
printf '%s\n' '--- init and export definitions ---'
rg -n -g '*.py' 'def (init_ide|.*export.*prompt|.*cleanup.*)|SPECFACT_COMMANDS|prompt_template|prompt_sources|shutil\.(rmtree|copytree)|\.unlink\(' src tests | head -260
printf '%s\n' '--- OpenSpec change files ---'
find openspec/changes/security-01-confine-ide-prompt-exports -maxdepth 3 -type f -print -exec sh -c 'echo "--- $1"; sed -n "1,220p" "$1"' _ {} \;Repository: nold-ai/specfact-cli Length of output: 27163 🏁 Script executed: #!/bin/bash
sed -n '1,320p' src/specfact_cli/utils/ide_setup.py
printf '%s\n' '--- tests bound to ide_setup ---'
rg -n -C 5 'ide_setup|cleanup|unsafe|symlink|legacy|specfact.*prompt|output root' tests src/specfact_cli/utils/ide_setup.pyRepository: nold-ai/specfact-cli Length of output: 50377 🏁 Script executed: #!/bin/bash
printf '%s\n' '--- containment and cleanup ---'
sed -n '430,535p' src/specfact_cli/utils/ide_setup.py
printf '%s\n' '--- export path and copy flow ---'
sed -n '680,825p' src/specfact_cli/utils/ide_setup.py
printf '%s\n' '--- output naming and settings cleanup ---'
sed -n '825,905p' src/specfact_cli/utils/ide_setup.py
sed -n '1040,1120p' src/specfact_cli/utils/ide_setup.py
printf '%s\n' '--- focused regression tests ---'
rg -n -C 12 'preserves_unrelated|symlinked_export_root|legacy.*segment|specfact.*prompt|unrelated' tests/unit/utils/test_ide_setup.pyRepository: nold-ai/specfact-cli Length of output: 29195 Define an exact cleanup ownership boundary.
🤖 Prompt for AI AgentsSource: Path instructions |
||
|
|
||
| ## Alternatives Considered | ||
|
|
||
| - Persist ownership markers for every exported path. This would support more | ||
| exact cleanup but adds migration state beyond the minimal security fix. | ||
| - Resolve the root and check only its children. Rejected because it validates | ||
| containment relative to an already escaped root rather than the repository. | ||
|
|
||
| ## Risks and Rollback | ||
|
|
||
| Repositories that intentionally symlink IDE export roots outside themselves | ||
| will now fail closed. That compatibility break is required to preserve the | ||
| repository boundary. A rollback must not occur without an alternative | ||
| ownership- and containment-safe export design. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| ## Why | ||
|
|
||
| An attacker-controlled repository can make an IDE prompt export root, such as | ||
| `.github/prompts`, a symlink to a writable location outside the repository. | ||
| Prompt cleanup and export currently follow that root, which can delete legacy | ||
| directories or write generated prompts outside the requested repository. | ||
|
|
||
| ## What Changes | ||
|
|
||
| - Require every IDE prompt export root to be a real directory path contained | ||
| beneath the resolved target repository before cleanup or export. | ||
| - Refuse symlinked or out-of-repository export roots without deleting or | ||
| writing through them. | ||
| - Preserve unrelated team-owned directories while retaining normal cleanup of | ||
| legacy SpecFact export directories inside a repository. | ||
| - Add focused security regression tests for cleanup and export behavior. | ||
|
|
||
| ## Capabilities | ||
|
|
||
| ### Modified Capabilities | ||
|
|
||
| - `init-ide-prompt-source-selection`: IDE prompt exports remain confined to the | ||
| selected repository and do not traverse repository-controlled export-root | ||
| symlinks. | ||
|
|
||
| ## Impact | ||
|
|
||
| - Affected code: `src/specfact_cli/utils/ide_setup.py`. | ||
| - Affected tests: `tests/unit/utils/test_ide_setup.py`. | ||
| - Compatibility: normal in-repository IDE exports and cleanup remain unchanged; | ||
| unsafe symlinked export roots are rejected. | ||
| - Documentation: review `README.md`, `docs/`, `docs/index.md`, and navigation; | ||
| no user-facing documentation change is expected because this restores the | ||
| existing repository-bound safety contract. | ||
| - Rollback: revert the helper and call-site guard together; doing so would | ||
| reopen the external filesystem deletion/write vulnerability. | ||
|
|
||
| ## Source Tracking | ||
|
|
||
| <!-- source_repo: nold-ai/specfact-cli --> | ||
| - **Parent Feature**: [#375](https://github.com/nold-ai/specfact-cli/issues/375) | ||
| - **Parent Epic**: [#285](https://github.com/nold-ai/specfact-cli/issues/285) | ||
| - **GitHub Issue**: [#720](https://github.com/nold-ai/specfact-cli/issues/720) | ||
| - **Issue URL**: https://github.com/nold-ai/specfact-cli/issues/720 | ||
| - **Repository**: nold-ai/specfact-cli | ||
| - **Blocked By**: none | ||
| - **Last Synced Status**: issue open, Todo, assigned, labels and parent verified on 2026-09-06 | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| ## ADDED Requirements | ||
|
|
||
| ### Requirement: IDE prompt exports are repository confined | ||
|
|
||
| The CLI SHALL perform IDE prompt cleanup and export only through a non-symlink | ||
| export root whose resolved path is contained beneath the resolved target | ||
| repository. It SHALL NOT delete or write through an unsafe export root and | ||
| SHALL preserve unrelated team-owned directories. | ||
|
|
||
| #### Scenario: Repository-controlled export root targets an external directory | ||
|
|
||
| - **GIVEN** an IDE prompt export root in the target repository is a symlink to a writable external directory | ||
| - **WHEN** IDE prompt initialization attempts cleanup and export | ||
| - **THEN** the CLI rejects the unsafe export root before deleting or writing any external content | ||
|
Comment on lines
+10
to
+14
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 Add a regression scenario for non-symlink paths that resolve outside the repository. A repository-controlled ancestor such as 🤖 Prompt for AI Agents |
||
|
|
||
| #### Scenario: Normal repository-contained export | ||
|
|
||
| - **GIVEN** an IDE prompt export root is a real directory beneath the target repository | ||
| - **WHEN** IDE prompt initialization cleans legacy SpecFact exports and writes selected prompts | ||
| - **THEN** the CLI completes the export while leaving unrelated team-owned directories intact | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| ## 1. Worktree and specification | ||
|
|
||
| - [x] 1.1 Confirm implementation runs on the dedicated non-protected `work` branch/worktree. | ||
| - [x] 1.2 Add the repository-confined IDE prompt export specification and design. | ||
| - [x] 1.3 Create and link public issue #720 with parent, labels, assignment, project status, and blocker metadata. | ||
|
|
||
| ## 2. Tests and failing evidence | ||
|
|
||
| - [x] 2.1 Add tests derived from both specification scenarios: external export-root symlinks fail without mutation, and normal exports preserve unrelated directories. | ||
| - [x] 2.2 Run the focused tests before production changes and record failing output in `TDD_EVIDENCE.md`. | ||
|
|
||
| ## 3. Implementation | ||
|
|
||
| - [x] 3.1 Add a small internal containment guard for IDE export roots. | ||
| - [x] 3.2 Apply the guard before cleanup and prompt export filesystem mutations. | ||
|
|
||
| ## 4. Verification and delivery | ||
|
|
||
| - [x] 4.1 Re-run focused tests and record passing output in `TDD_EVIDENCE.md`. | ||
| - [x] 4.2 Run formatting, type-checking, lint, YAML lint, contract tests, smart tests, independent static analysis, and SpecFact code review gates; record unrelated/environment limitations in `TDD_EVIDENCE.md`. | ||
| - [x] 4.3 Review `README.md`, `docs/`, `docs/index.md`, and navigation for documentation impact; no update is required. | ||
| - [x] 4.4 Bump the patch version in all canonical files and add the security fix to `CHANGELOG.md`. | ||
| - [x] 4.5 Verify signed module manifests remain valid; no signed module asset changed. | ||
| - [x] 4.6 Record the internal wiki mirror/rebuild follow-up because the sibling checkout is unavailable. | ||
| - [ ] 4.7 Commit the completed change and create a pull request to `dev`. | ||
|
|
||
| ## 5. Post-merge cleanup | ||
|
|
||
| - [ ] 5.1 After merge, run `openspec archive security-01-confine-ide-prompt-exports` and perform worktree cleanup. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -457,10 +457,23 @@ def _merge_prompt_export_outputs_by_basename( | |
| return out | ||
|
|
||
|
|
||
| def _safe_ide_export_dir(repo_path: Path, ide: str) -> Path: | ||
| """Return the IDE export directory only when it cannot escape the repository.""" | ||
| export_dir = repo_path / str(IDE_CONFIG[ide]["folder"]) | ||
| resolved_repo = repo_path.resolve() | ||
| resolved_export = export_dir.resolve() | ||
| try: | ||
| resolved_export.relative_to(resolved_repo) | ||
| except ValueError as exc: | ||
| raise ValueError(f"IDE export directory must remain inside the repository: {export_dir}") from exc | ||
| if export_dir.is_symlink(): | ||
| raise ValueError(f"IDE export directory must not be a symlink: {export_dir}") | ||
|
Comment on lines
+468
to
+470
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. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n -C 8 'copy_prompts_by_source_to_ide|except ValueError|ClickException|init ide' src testsRepository: nold-ai/specfact-cli Length of output: 50377 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- IDE export call sites ---'
rg -n -C 12 'copy_prompts_by_source_to_ide|_safe_ide_export_dir|def .*ide|@.*ide|init ide' src/specfact_cli tests \
-g '*.py' -g '*.pyi' | head -n 320
printf '%s\n' '--- CLI exception handling ---'
rg -n -C 10 'Typer|typer\.|ClickException|BadParameter|Exit|except \(?ValueError|except ValueError|main\(' src/specfact_cli \
-g '*.py' | head -n 320Repository: nold-ai/specfact-cli Length of output: 50376 🏁 Script executed: #!/bin/bash
set -euo pipefail
files=$(rg -l 'copy_prompts_by_source_to_ide|_safe_ide_export_dir' src/specfact_cli tests -g '*.py')
printf '%s\n' "$files"
while IFS= read -r file; do
printf '\n--- %s ---\n' "$file"
rg -n -C 18 'copy_prompts_by_source_to_ide|_safe_ide_export_dir' "$file"
done <<< "$files"
printf '\n--- command registration and main entry points ---\n'
rg -l 'init.*ide|ide.*force|def main|typer\.Typer|Typer\(' src/specfact_cli -g '*.py' |
while IFS= read -r file; do
if rg -q 'init.*ide|ide.*force|def main|typer\.Typer|Typer\(' "$file"; then
printf '\n--- %s ---\n' "$file"
rg -n -C 10 'init.*ide|ide.*force|def main|typer\.Typer|Typer\(' "$file"
fi
doneRepository: nold-ai/specfact-cli Length of output: 50376 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- init_ide implementation ---'
sed -n '588,760p' src/specfact_cli/modules/init/src/commands.py
printf '%s\n' '--- CLI exception dispatch ---'
sed -n '600,655p' src/specfact_cli/cli.py
sed -n '905,935p' src/specfact_cli/cli.py
printf '%s\n' '--- init IDE tests for CLI behavior ---'
rg -n -C 12 'CliRunner|init_ide|init ide|raises\(|exit_code|exception' tests/unit/modules/init tests -g '*.py' | head -n 260Repository: nold-ai/specfact-cli Length of output: 34705 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- CLI entrypoint and exception policy ---'
rg -n -C 18 'def cli_main|if __name__|app\(|pretty_exceptions|standalone_mode|ClickException|Exception' src/specfact_cli/cli.py src/specfact_cli -g '*.py' | tail -n 260
printf '%s\n' '--- exact init command registration ---'
sed -n '1,75p' src/specfact_cli/modules/init/src/commands.py
sed -n '575,705p' src/specfact_cli/modules/init/src/commands.pyRepository: nold-ai/specfact-cli Length of output: 30243 Convert unsafe IDE export errors into CLI errors. When 🤖 Prompt for AI AgentsSource: Linked repositories |
||
| return resolved_export | ||
|
|
||
|
|
||
| def _cleanup_legacy_multisource_segment_dirs(repo_path: Path, ide: str) -> None: | ||
| """Remove per-source subfolders from older multi-source exports (layout is now flat under the IDE root).""" | ||
| config = IDE_CONFIG[ide] | ||
| base = (repo_path / str(config["folder"])).resolve() | ||
| base = _safe_ide_export_dir(repo_path, ide) | ||
|
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. 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift Restrict legacy cleanup to SpecFact-owned directories. The root is validated, but As per path instructions, 🤖 Prompt for AI AgentsSource: Path instructions |
||
| if not base.is_dir(): | ||
| return | ||
| for child in list(base.iterdir()): | ||
|
|
@@ -498,7 +511,7 @@ def _prune_flat_specfact_exports_not_in_expected( | |
| """Remove prior flat ``specfact*`` exports that are not part of this merged export.""" | ||
| config = IDE_CONFIG[ide] | ||
| format_type = str(config["format"]) | ||
| base = (repo_path / str(config["folder"])).resolve() | ||
| base = _safe_ide_export_dir(repo_path, ide) | ||
|
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 attacker-controlled repository has a real export root but places symlinks below it, this root-only guard still permits external mutation. For skill-based IDEs, a path such as Useful? React with 👍 / 👎. |
||
| if not base.is_dir(): | ||
| return | ||
| pattern = _flat_export_glob_pattern_for_prune(format_type) | ||
|
|
@@ -539,13 +552,12 @@ def _copy_template_files_to_ide( | |
| ) -> tuple[list[Path], Path | None]: | ||
| """Copy a concrete list of prompt template files to the IDE target location.""" | ||
| config = IDE_CONFIG[ide] | ||
| ide_folder = str(config["folder"]) | ||
| format_type = str(config["format"]) | ||
| settings_file = config.get("settings_file") | ||
| if settings_file is not None and not isinstance(settings_file, str): | ||
| settings_file = None | ||
|
|
||
| ide_dir = repo_path / ide_folder | ||
| ide_dir = _safe_ide_export_dir(repo_path, ide) | ||
|
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 Reject destination symlinks before force writes. When 🤖 Prompt for AI Agents |
||
| if source_segment is not None: | ||
| ide_dir = ide_dir / source_segment | ||
| ide_dir.mkdir(parents=True, exist_ok=True) | ||
|
|
@@ -646,8 +658,7 @@ def _copy_skill_bundles_to_ide( | |
| force: bool = False, | ||
| ) -> tuple[list[Path], None]: | ||
| """Copy source/module prompt groups to skill-based IDE targets.""" | ||
| config = IDE_CONFIG[ide] | ||
| ide_dir = repo_path / str(config["folder"]) | ||
| ide_dir = _safe_ide_export_dir(repo_path, ide) | ||
| ide_dir.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| expected = {_skill_output_name_for_source(source_id) for source_id in prompts_by_source} | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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: 29724
🤖 get_repo_knowledge executed:
get_repo_knowledge nold-ai/specfact-cli /tmp/coderabbit-repo-knowledge/nold-ai-specfact-cli-93d7aac2/conventionsLength of output: 24161
Record a final full-suite result.
The recorded
hatch run smart-testresult includes failures from before the release and lockfile fixes. The later evidence covers only 32 focused tests. Run and recordhatch run smart-test-fullafter all fixes, or list remaining failures as blockers.🤖 Prompt for AI Agents
Source: Coding guidelines