From 987ca1d05c2af4520d875a68500689f35ac226e6 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 23 Aug 2026 17:20:41 +0000 Subject: [PATCH 1/2] Add hook safety controls for structkit Implement comprehensive hook safety mechanisms to prevent arbitrary shell command execution when structures are generated via MCP or CI/CD. Changes: - Add --no-hooks flag and STRUCTKIT_NO_HOOKS env var to disable all hooks - Add --hooks-allowlist flag and STRUCTKIT_HOOKS_ALLOWLIST env var for command allowlisting - Implement interactive confirmation prompts before running hooks (skipped with --non-interactive) - Auto-detect .struct-hooks-allowlist in current directory - Update MCP generate_structure to skip hooks by default (no_hooks=true) - Add comprehensive test coverage for all safety features - Update documentation (hooks.md, mcp-integration.md) with safety guidance Fixes #100 Co-authored-by: Kenneth Belitzky --- docs/hooks.md | 130 +++++++++++++++++++ docs/mcp-integration.md | 37 +++++- structkit/commands/generate.py | 122 +++++++++++++++++- structkit/mcp_server.py | 8 +- tests/test_commands.py | 5 +- tests/test_commands_more.py | 2 +- tests/test_hooks.py | 226 ++++++++++++++++++++++++++++++++- 7 files changed, 518 insertions(+), 12 deletions(-) diff --git a/docs/hooks.md b/docs/hooks.md index d3e9ce5..ceba131 100644 --- a/docs/hooks.md +++ b/docs/hooks.md @@ -2,6 +2,15 @@ You can define shell commands to run before and after structure generation using the `pre_hooks` and `post_hooks` keys in your YAML configuration. These are optional and allow you to automate setup or cleanup steps. +## Safety Controls + +Hooks execute shell commands with `shell=True`, which can be powerful but also risky. StructKit provides several safety mechanisms: + +- **Interactive Confirmation**: When running interactively, StructKit prompts for confirmation before executing hooks +- **Skip Hooks**: Use `--no-hooks` flag or `STRUCTKIT_NO_HOOKS=true` to disable all hooks +- **Allowlist**: Create a `.struct-hooks-allowlist` file to restrict which commands can run +- **MCP Safety**: MCP calls skip hooks by default (`no_hooks=true`) + ## Hook Types - **pre_hooks**: List of shell commands to run before generation. If any command fails (non-zero exit), generation is aborted. @@ -170,6 +179,94 @@ files: } ``` +## Safety Features + +### Disabling Hooks + +You can disable hooks entirely using the `--no-hooks` flag or environment variable: + +```bash +# Using CLI flag +structkit generate .struct.yaml --no-hooks + +# Using environment variable +export STRUCTKIT_NO_HOOKS=true +structkit generate .struct.yaml +``` + +This is recommended for: +- CI/CD pipelines where hooks shouldn't run +- MCP/automation contexts +- Untrusted structure definitions + +### Interactive Confirmation + +By default, StructKit prompts for confirmation before running hooks in interactive mode: + +```bash +$ structkit generate .struct.yaml + +⚠️ The following pre-hooks will be executed: + - echo "Preparing environment..." + - ./scripts/prep.sh + +Do you want to run these pre-hooks? [y/N]: +``` + +To skip the prompt: +- Use `--non-interactive` flag +- Set `STRUCTKIT_NON_INTERACTIVE=true` + +**Note**: Non-interactive mode without `--no-hooks` will execute hooks without confirmation. + +### Allowlist File + +Create a `.struct-hooks-allowlist` file in your project directory to restrict which commands can run: + +```text +# .struct-hooks-allowlist +# One command per line. Lines starting with # are comments. + +echo +git +npm +python +./scripts/prep.sh +./scripts/cleanup.sh +``` + +When an allowlist exists: +- Only commands in the allowlist can run +- Both exact matches and base commands (first word) are checked +- Blocked hooks cause generation to fail + +You can also specify a custom allowlist path: + +```bash +structkit generate .struct.yaml --hooks-allowlist /path/to/allowlist.txt + +# Or via environment variable +export STRUCTKIT_HOOKS_ALLOWLIST=/path/to/allowlist.txt +structkit generate .struct.yaml +``` + +### MCP Integration Safety + +When using StructKit through MCP (Model Context Protocol), hooks are **disabled by default** for security: + +```json +{ + "name": "generate_structure", + "arguments": { + "structure_definition": "project/python", + "base_path": "/tmp/myproject", + "no_hooks": true // Default for MCP calls + } +} +``` + +To enable hooks in MCP calls (not recommended), explicitly set `no_hooks: false`. + ## Best Practices 1. **Keep hooks simple**: Use external scripts for complex operations @@ -178,6 +275,8 @@ files: 4. **Log important actions**: Use echo statements for user feedback 5. **Test independently**: Ensure hook commands work outside StructKit 6. **Consider dependencies**: Order hooks based on their requirements +7. **Use allowlists**: For production environments, always use an allowlist +8. **Disable in CI/CD**: Use `--no-hooks` in automated environments unless hooks are required and safe ## Error Handling @@ -199,6 +298,37 @@ post_hooks: - echo "Setup complete (some warnings may have occurred)" ``` +### Safe Hook Example with Allowlist + +```yaml +# .struct.yaml +pre_hooks: + - echo "Preparing environment..." + - python -c "import sys; print(sys.version)" + +post_hooks: + - echo "Generation complete!" + - git --version + +files: + - README.md: + content: | + # My Project +``` + +```text +# .struct-hooks-allowlist +echo +python +git +``` + +With this setup: +- Only `echo`, `python`, and `git` commands can run +- Interactive users will be prompted for confirmation +- Use `--no-hooks` to skip entirely +- Use `--non-interactive` to run without prompts (requires allowlist or trust) + ## Variables in Hooks You can use template variables in hook commands: diff --git a/docs/mcp-integration.md b/docs/mcp-integration.md index 8a5da7e..2f845de 100644 --- a/docs/mcp-integration.md +++ b/docs/mcp-integration.md @@ -70,6 +70,7 @@ Generate a project structure using specified definition and options. - `mappings` (optional): Variable mappings for template substitution - `structures_path` (optional): Custom path to structure definitions - `source` (optional): Named source configured with `manage_sources`. The structure definition can also use a `/` prefix. +- `no_hooks` (optional): Skip all pre/post hooks for safety (default: **true** for MCP calls) ### 4. get_structure_vars Inspect variables declared by a specific structure without generating files. @@ -296,6 +297,36 @@ if __name__ == "__main__": The MCP integration is particularly powerful for AI-assisted development workflows: +### Hook Safety in MCP + +**Important**: For security, MCP calls to `generate_structure` skip hooks by default (`no_hooks: true`). This prevents arbitrary shell command execution when structures are generated via automation or AI tools. + +```json +{ + "name": "generate_structure", + "arguments": { + "structure_definition": "project/python", + "base_path": "/tmp/myproject", + "no_hooks": true // Default for MCP - hooks are skipped + } +} +``` + +To enable hooks in MCP calls (not recommended unless you trust the structure source): + +```json +{ + "name": "generate_structure", + "arguments": { + "structure_definition": "project/python", + "base_path": "/tmp/myproject", + "no_hooks": false // Explicitly enable hooks (use with caution) + } +} +``` + +See the [Hooks documentation](hooks.md) for more information about hook safety controls. + ### Console Output Mode Using `output: "console"` with `generate_structure` allows piping structure content to stdout for subsequent AI prompts: @@ -434,7 +465,8 @@ without writing files: "dry_run": true, "mappings": { "module_name": "network-observability" - } + }, + "no_hooks": true } } ``` @@ -465,7 +497,8 @@ After approval, call the same structure with `output: "files"` and "dry_run": false, "mappings": { "module_name": "network-observability" - } + }, + "no_hooks": true } } ``` diff --git a/structkit/commands/generate.py b/structkit/commands/generate.py index 7f31d97..3d923e9 100644 --- a/structkit/commands/generate.py +++ b/structkit/commands/generate.py @@ -50,6 +50,8 @@ def __init__(self, parser): help='Path to a YAML file containing mappings to be used in templates (can be specified multiple times)') parser.add_argument('-o', '--output', type=str, choices=['console', 'file'], default=os.getenv('STRUCTKIT_OUTPUT_MODE', 'file'), help='Output mode (env: STRUCTKIT_OUTPUT_MODE)') + parser.add_argument('--no-hooks', action='store_true', help='Skip all pre/post hooks (env: STRUCTKIT_NO_HOOKS)', default=os.getenv('STRUCTKIT_NO_HOOKS', '').lower() in ('true', '1', 'yes')) + parser.add_argument('--hooks-allowlist', type=str, help='Path to hooks allowlist file (env: STRUCTKIT_HOOKS_ALLOWLIST)', default=os.getenv('STRUCTKIT_HOOKS_ALLOWLIST', None)) parser.set_defaults(func=self.execute) def _parse_template_vars(self, vars_str): @@ -91,9 +93,108 @@ def _deep_merge_dicts(self, dict1, dict2): result[key] = value return result - def _run_hooks(self, hooks, hook_type="pre"): # helper for running hooks + def _load_hooks_allowlist(self, allowlist_path): + """Load the hooks allowlist from a file. + Returns a set of allowed commands or None if file doesn't exist. + """ + if not allowlist_path: + return None + + allowlist_file = None + if os.path.isabs(allowlist_path): + allowlist_file = allowlist_path + else: + allowlist_file = os.path.join(os.getcwd(), allowlist_path) + + if not os.path.exists(allowlist_file): + # Also check for .struct-hooks-allowlist in current directory if no explicit path given + default_allowlist = os.path.join(os.getcwd(), '.struct-hooks-allowlist') + if allowlist_path == default_allowlist and not os.path.exists(default_allowlist): + return None + self.logger.warning(f"Hooks allowlist file not found: {allowlist_file}") + return None + + try: + with open(allowlist_file, 'r') as f: + lines = f.readlines() + # Parse allowlist: ignore empty lines and comments (lines starting with #) + allowlist = set() + for line in lines: + line = line.strip() + if line and not line.startswith('#'): + allowlist.add(line) + return allowlist + except Exception as e: + self.logger.error(f"Failed to read hooks allowlist: {e}") + return None + + def _check_hook_allowed(self, cmd, allowlist): + """Check if a command is allowed by the allowlist. + If allowlist is None, all commands are allowed. + If allowlist is a set, only commands in the set are allowed. + """ + if allowlist is None: + return True + + # Check exact match first + if cmd in allowlist: + return True + + # Check if the base command (first word) is allowed + base_cmd = cmd.split()[0] if cmd.split() else cmd + if base_cmd in allowlist: + return True + + return False + + def _confirm_hooks(self, hooks, hook_type="pre"): + """Ask user to confirm hook execution. + Returns True if user confirms, False otherwise. + """ if not hooks: return True + + print(f"\n⚠️ The following {hook_type}-hooks will be executed:") + for cmd in hooks: + print(f" - {cmd}") + + response = input(f"\nDo you want to run these {hook_type}-hooks? [y/N]: ").strip().lower() + return response in ('y', 'yes') + + def _run_hooks(self, hooks, hook_type="pre", skip_hooks=False, non_interactive=False, allowlist=None): + """Run pre/post hooks with safety controls. + + Args: + hooks: List of shell commands to run + hook_type: Type of hooks ("pre" or "post") + skip_hooks: If True, skip all hooks + non_interactive: If True, skip confirmation prompt + allowlist: Set of allowed commands or None to allow all + + Returns: + True if all hooks succeeded or were skipped, False if any failed + """ + if not hooks or skip_hooks: + if skip_hooks and hooks: + self.logger.info(f"Skipping {hook_type}-hooks (--no-hooks enabled)") + return True + + # Check if any hooks are blocked by allowlist + if allowlist is not None: + blocked_hooks = [cmd for cmd in hooks if not self._check_hook_allowed(cmd, allowlist)] + if blocked_hooks: + self.logger.error(f"The following {hook_type}-hooks are not in the allowlist:") + for cmd in blocked_hooks: + self.logger.error(f" - {cmd}") + self.logger.error("Hook execution blocked. Update allowlist or use --no-hooks to skip.") + return False + + # Ask for confirmation in interactive mode + if not non_interactive: + if not self._confirm_hooks(hooks, hook_type): + self.logger.info(f"User declined to run {hook_type}-hooks. Aborting.") + return False + for cmd in hooks: self.logger.info(f"Running {hook_type}-hook: {cmd}") try: @@ -201,8 +302,23 @@ def execute(self, args): pre_hooks = config.get('pre_hooks', []) post_hooks = config.get('post_hooks', []) + skip_hooks = getattr(args, 'no_hooks', False) + non_interactive = getattr(args, 'non_interactive', False) + + # Load hooks allowlist if provided (only if hooks are enabled) + allowlist = None + if not skip_hooks: + allowlist_path = getattr(args, 'hooks_allowlist', None) + if not allowlist_path: + # Check for default .struct-hooks-allowlist in current directory + default_allowlist = os.path.join(os.getcwd(), '.struct-hooks-allowlist') + if os.path.exists(default_allowlist): + allowlist_path = default_allowlist + + allowlist = self._load_hooks_allowlist(allowlist_path) if allowlist_path else None + # Run pre-hooks - if not self._run_hooks(pre_hooks, hook_type="pre"): + if not self._run_hooks(pre_hooks, hook_type="pre", skip_hooks=skip_hooks, non_interactive=non_interactive, allowlist=allowlist): self.logger.error("Aborting generation due to pre-hook failure.") return @@ -214,7 +330,7 @@ def execute(self, args): raise SystemExit(1) from None # Run post-hooks - if not self._run_hooks(post_hooks, hook_type="post"): + if not self._run_hooks(post_hooks, hook_type="post", skip_hooks=skip_hooks, non_interactive=non_interactive, allowlist=allowlist): self.logger.error("Post-hook failed.") return diff --git a/structkit/mcp_server.py b/structkit/mcp_server.py index 4d1ab31..8dbaa56 100644 --- a/structkit/mcp_server.py +++ b/structkit/mcp_server.py @@ -136,6 +136,7 @@ def _generate_structure_logic( mappings: Optional[Dict[str, str]] = None, structures_path: Optional[str] = None, source: Optional[str] = None, + no_hooks: bool = True, ) -> str: try: structures_path, structure_definition = resolve_structures_path(structures_path, source, structure_definition) @@ -161,6 +162,8 @@ class Args: args.log = "INFO" args.config_file = None args.log_file = None + args.no_hooks = no_hooks + args.hooks_allowlist = None # If mappings provided, convert to vars string consumed by GenerateCommand if mappings: @@ -452,7 +455,7 @@ async def explain_structure( self.logger.debug(f"MCP response: explain_structure len={len(result)} preview=\n{preview}") return result - @self.app.tool(name="generate_structure", description="Generate a project structure using specified definition and options") + @self.app.tool(name="generate_structure", description="Generate a project structure using specified definition and options. MCP calls skip hooks by default for safety; set no_hooks=false to enable them.") async def generate_structure( structure_definition: str, base_path: str, @@ -461,6 +464,7 @@ async def generate_structure( mappings: Optional[Dict[str, str]] = None, structures_path: Optional[str] = None, source: Optional[str] = None, + no_hooks: bool = True, ) -> str: self.logger.debug( "MCP request: generate_structure args=%s", @@ -472,6 +476,7 @@ async def generate_structure( "mappings": mappings, "structures_path": structures_path, "source": source, + "no_hooks": no_hooks, }, ) result = self._generate_structure_logic( @@ -482,6 +487,7 @@ async def generate_structure( mappings, structures_path, source, + no_hooks, ) preview = result if len(result) <= 1000 else result[:1000] + f"... [truncated {len(result)-1000} chars]" self.logger.debug(f"MCP response: generate_structure len={len(result)} preview=\n{preview}") diff --git a/tests/test_commands.py b/tests/test_commands.py index d2888f7..3368787 100644 --- a/tests/test_commands.py +++ b/tests/test_commands.py @@ -486,7 +486,10 @@ def test_multiple_mappings_files(): mappings_file=['mappings1.yaml', 'mappings2.yaml'], backup=None, output='file', - structures_path=None + structures_path=None, + no_hooks=True, + hooks_allowlist=None, + non_interactive=True ) # Mock config loading diff --git a/tests/test_commands_more.py b/tests/test_commands_more.py index 1025499..14e85f9 100644 --- a/tests/test_commands_more.py +++ b/tests/test_commands_more.py @@ -93,7 +93,7 @@ def test_generate_dry_run_diff_shows_unified_diff(parser, tmp_path): def test_generate_pre_hook_failure_aborts(parser, tmp_path): command = GenerateCommand(parser) - args = parser.parse_args(['struct-x', str(tmp_path)]) + args = parser.parse_args(['struct-x', str(tmp_path), '--non-interactive']) config = {'pre_hooks': ['exit 1'], 'files': []} diff --git a/tests/test_hooks.py b/tests/test_hooks.py index 5178526..2b5485c 100644 --- a/tests/test_hooks.py +++ b/tests/test_hooks.py @@ -1,7 +1,8 @@ import pytest -from unittest.mock import patch, MagicMock, call +from unittest.mock import patch, MagicMock, call, mock_open from structkit.commands.generate import GenerateCommand import argparse +import os @pytest.fixture def parser(): @@ -24,6 +25,14 @@ def make_args(tmp_path, pre=None, post=None): _yaml.safe_dump(config, f) return yaml_path +def make_allowlist(tmp_path, commands): + """Create a hooks allowlist file.""" + allowlist_path = tmp_path / '.struct-hooks-allowlist' + with open(allowlist_path, 'w') as f: + for cmd in commands: + f.write(f"{cmd}\n") + return allowlist_path + def test_no_hooks_runs_ok(tmp_path, parser): yaml_path = make_args(tmp_path) command = GenerateCommand(parser) @@ -38,7 +47,7 @@ def test_no_hooks_runs_ok(tmp_path, parser): def test_pre_hook_runs_and_blocks_on_failure(tmp_path, parser): yaml_path = make_args(tmp_path, pre=['exit 1']) command = GenerateCommand(parser) - args = parser.parse_args([f'file://{yaml_path}', str(tmp_path)]) + args = parser.parse_args([f'file://{yaml_path}', str(tmp_path), '--non-interactive']) with patch('subprocess.run', side_effect=__import__('subprocess').CalledProcessError(1, 'exit 1')) as mock_subproc, \ patch.object(command, '_create_structure') as mock_create_structure: command.execute(args) @@ -48,7 +57,7 @@ def test_pre_hook_runs_and_blocks_on_failure(tmp_path, parser): def test_post_hook_runs_and_blocks_on_failure(tmp_path, parser): yaml_path = make_args(tmp_path, post=['exit 1']) command = GenerateCommand(parser) - args = parser.parse_args([f'file://{yaml_path}', str(tmp_path)]) + args = parser.parse_args([f'file://{yaml_path}', str(tmp_path), '--non-interactive']) with patch('subprocess.run', side_effect=__import__('subprocess').CalledProcessError(1, 'exit 1')) as mock_subproc, \ patch.object(command, '_create_structure') as mock_create_structure: command.execute(args) @@ -59,7 +68,7 @@ def test_post_hook_runs_and_blocks_on_failure(tmp_path, parser): def test_hooks_order(tmp_path, parser): yaml_path = make_args(tmp_path, pre=['echo pre'], post=['echo post']) command = GenerateCommand(parser) - args = parser.parse_args([f'file://{yaml_path}', str(tmp_path)]) + args = parser.parse_args([f'file://{yaml_path}', str(tmp_path), '--non-interactive']) with patch('subprocess.run') as mock_subproc, \ patch.object(command, '_create_structure') as mock_create_structure: command.execute(args) @@ -67,3 +76,212 @@ def test_hooks_order(tmp_path, parser): assert mock_subproc.call_args_list[0][0][0] == 'echo pre' assert mock_subproc.call_args_list[1][0][0] == 'echo post' mock_create_structure.assert_called_once() + +def test_no_hooks_flag(tmp_path, parser): + """Test that --no-hooks skips all hooks.""" + yaml_path = make_args(tmp_path, pre=['echo pre'], post=['echo post']) + command = GenerateCommand(parser) + args = parser.parse_args([f'file://{yaml_path}', str(tmp_path), '--no-hooks']) + with patch('subprocess.run') as mock_subproc, \ + patch.object(command, '_create_structure') as mock_create_structure: + command.execute(args) + # No hooks should run + mock_subproc.assert_not_called() + mock_create_structure.assert_called_once() + +def test_interactive_confirmation_declined(tmp_path, parser): + """Test that declining hook confirmation aborts generation.""" + yaml_path = make_args(tmp_path, pre=['echo pre']) + command = GenerateCommand(parser) + args = parser.parse_args([f'file://{yaml_path}', str(tmp_path)]) + with patch('subprocess.run') as mock_subproc, \ + patch.object(command, '_create_structure') as mock_create_structure, \ + patch('builtins.input', return_value='n'): + command.execute(args) + # No hooks should run + mock_subproc.assert_not_called() + # Structure should not be created + mock_create_structure.assert_not_called() + +def test_interactive_confirmation_accepted(tmp_path, parser): + """Test that accepting hook confirmation runs hooks.""" + yaml_path = make_args(tmp_path, pre=['echo pre']) + command = GenerateCommand(parser) + args = parser.parse_args([f'file://{yaml_path}', str(tmp_path)]) + with patch('subprocess.run') as mock_subproc, \ + patch.object(command, '_create_structure') as mock_create_structure, \ + patch('builtins.input', return_value='y'): + command.execute(args) + # Hooks should run + mock_subproc.assert_called_once() + mock_create_structure.assert_called_once() + +def test_non_interactive_mode(tmp_path, parser): + """Test that --non-interactive skips confirmation.""" + yaml_path = make_args(tmp_path, pre=['echo pre']) + command = GenerateCommand(parser) + args = parser.parse_args([f'file://{yaml_path}', str(tmp_path), '--non-interactive']) + with patch('subprocess.run') as mock_subproc, \ + patch.object(command, '_create_structure') as mock_create_structure, \ + patch('builtins.input') as mock_input: + command.execute(args) + # No prompt should appear + mock_input.assert_not_called() + # Hooks should run + mock_subproc.assert_called_once() + mock_create_structure.assert_called_once() + +def test_allowlist_blocks_unlisted_command(tmp_path, parser): + """Test that allowlist blocks commands not in the list.""" + yaml_path = make_args(tmp_path, pre=['dangerous-command']) + make_allowlist(tmp_path, ['echo', 'git']) + command = GenerateCommand(parser) + args = parser.parse_args([ + f'file://{yaml_path}', + str(tmp_path), + '--non-interactive', + '--hooks-allowlist', str(tmp_path / '.struct-hooks-allowlist') + ]) + with patch('subprocess.run') as mock_subproc, \ + patch.object(command, '_create_structure') as mock_create_structure: + command.execute(args) + # Hook should be blocked + mock_subproc.assert_not_called() + # Structure should not be created + mock_create_structure.assert_not_called() + +def test_allowlist_allows_listed_command(tmp_path, parser): + """Test that allowlist allows commands in the list.""" + yaml_path = make_args(tmp_path, pre=['echo test']) + make_allowlist(tmp_path, ['echo', 'git']) + command = GenerateCommand(parser) + args = parser.parse_args([ + f'file://{yaml_path}', + str(tmp_path), + '--non-interactive', + '--hooks-allowlist', str(tmp_path / '.struct-hooks-allowlist') + ]) + with patch('subprocess.run') as mock_subproc, \ + patch.object(command, '_create_structure') as mock_create_structure: + command.execute(args) + # Hook should run + mock_subproc.assert_called_once() + mock_create_structure.assert_called_once() + +def test_allowlist_exact_match(tmp_path, parser): + """Test that allowlist supports exact command matching.""" + yaml_path = make_args(tmp_path, pre=['./scripts/prep.sh']) + make_allowlist(tmp_path, ['./scripts/prep.sh', 'echo']) + command = GenerateCommand(parser) + args = parser.parse_args([ + f'file://{yaml_path}', + str(tmp_path), + '--non-interactive', + '--hooks-allowlist', str(tmp_path / '.struct-hooks-allowlist') + ]) + with patch('subprocess.run') as mock_subproc, \ + patch.object(command, '_create_structure') as mock_create_structure: + command.execute(args) + # Hook should run + mock_subproc.assert_called_once() + mock_create_structure.assert_called_once() + +def test_allowlist_base_command_match(tmp_path, parser): + """Test that allowlist matches base command (first word).""" + yaml_path = make_args(tmp_path, pre=['git add .']) + make_allowlist(tmp_path, ['git', 'echo']) + command = GenerateCommand(parser) + args = parser.parse_args([ + f'file://{yaml_path}', + str(tmp_path), + '--non-interactive', + '--hooks-allowlist', str(tmp_path / '.struct-hooks-allowlist') + ]) + with patch('subprocess.run') as mock_subproc, \ + patch.object(command, '_create_structure') as mock_create_structure: + command.execute(args) + # Hook should run (base command 'git' is allowed) + mock_subproc.assert_called_once() + mock_create_structure.assert_called_once() + +def test_allowlist_with_comments(tmp_path, parser): + """Test that allowlist ignores comments and empty lines.""" + yaml_path = make_args(tmp_path, pre=['echo test']) + allowlist_path = tmp_path / '.struct-hooks-allowlist' + with open(allowlist_path, 'w') as f: + f.write("# This is a comment\n") + f.write("\n") + f.write("echo\n") + f.write("# Another comment\n") + f.write("git\n") + command = GenerateCommand(parser) + args = parser.parse_args([ + f'file://{yaml_path}', + str(tmp_path), + '--non-interactive', + '--hooks-allowlist', str(allowlist_path) + ]) + with patch('subprocess.run') as mock_subproc, \ + patch.object(command, '_create_structure') as mock_create_structure: + command.execute(args) + # Hook should run + mock_subproc.assert_called_once() + mock_create_structure.assert_called_once() + +def test_default_allowlist_detection(tmp_path, parser): + """Test that .struct-hooks-allowlist is auto-detected in current directory.""" + yaml_path = make_args(tmp_path, pre=['echo test']) + allowlist_path = tmp_path / '.struct-hooks-allowlist' + with open(allowlist_path, 'w') as f: + f.write("echo\n") + command = GenerateCommand(parser) + args = parser.parse_args([ + f'file://{yaml_path}', + str(tmp_path), + '--non-interactive' + ]) + # Change to tmp_path so default allowlist is found + original_cwd = os.getcwd() + try: + os.chdir(tmp_path) + with patch('subprocess.run') as mock_subproc, \ + patch.object(command, '_create_structure') as mock_create_structure: + command.execute(args) + # Hook should run (allowlist auto-detected) + mock_subproc.assert_called_once() + mock_create_structure.assert_called_once() + finally: + os.chdir(original_cwd) + +def test_post_hooks_confirmation(tmp_path, parser): + """Test that post-hooks also require confirmation.""" + yaml_path = make_args(tmp_path, post=['echo post']) + command = GenerateCommand(parser) + args = parser.parse_args([f'file://{yaml_path}', str(tmp_path)]) + with patch('subprocess.run') as mock_subproc, \ + patch.object(command, '_create_structure') as mock_create_structure, \ + patch('builtins.input', return_value='n'): + command.execute(args) + # Structure should be created + mock_create_structure.assert_called_once() + # Post-hooks should not run + mock_subproc.assert_not_called() + +def test_allowlist_blocks_post_hooks(tmp_path, parser): + """Test that allowlist blocks post-hooks too.""" + yaml_path = make_args(tmp_path, post=['dangerous-post-command']) + make_allowlist(tmp_path, ['echo', 'git']) + command = GenerateCommand(parser) + args = parser.parse_args([ + f'file://{yaml_path}', + str(tmp_path), + '--non-interactive', + '--hooks-allowlist', str(tmp_path / '.struct-hooks-allowlist') + ]) + with patch('subprocess.run') as mock_subproc, \ + patch.object(command, '_create_structure') as mock_create_structure: + command.execute(args) + # Structure should be created + mock_create_structure.assert_called_once() + # Post-hook should be blocked + mock_subproc.assert_not_called() From 60c5670270e38ce1ab184a60af88ebf1523bee35 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 23 Aug 2026 17:36:50 +0000 Subject: [PATCH 2/2] Fix trailing whitespace for pre-commit Co-authored-by: Kenneth Belitzky --- docs/hooks.md | 2 +- structkit/commands/generate.py | 28 ++++++++++++++-------------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/docs/hooks.md b/docs/hooks.md index ceba131..0c2ebc3 100644 --- a/docs/hooks.md +++ b/docs/hooks.md @@ -210,7 +210,7 @@ $ structkit generate .struct.yaml - echo "Preparing environment..." - ./scripts/prep.sh -Do you want to run these pre-hooks? [y/N]: +Do you want to run these pre-hooks? [y/N]: ``` To skip the prompt: diff --git a/structkit/commands/generate.py b/structkit/commands/generate.py index 3d923e9..16b3865 100644 --- a/structkit/commands/generate.py +++ b/structkit/commands/generate.py @@ -99,13 +99,13 @@ def _load_hooks_allowlist(self, allowlist_path): """ if not allowlist_path: return None - + allowlist_file = None if os.path.isabs(allowlist_path): allowlist_file = allowlist_path else: allowlist_file = os.path.join(os.getcwd(), allowlist_path) - + if not os.path.exists(allowlist_file): # Also check for .struct-hooks-allowlist in current directory if no explicit path given default_allowlist = os.path.join(os.getcwd(), '.struct-hooks-allowlist') @@ -113,7 +113,7 @@ def _load_hooks_allowlist(self, allowlist_path): return None self.logger.warning(f"Hooks allowlist file not found: {allowlist_file}") return None - + try: with open(allowlist_file, 'r') as f: lines = f.readlines() @@ -135,16 +135,16 @@ def _check_hook_allowed(self, cmd, allowlist): """ if allowlist is None: return True - + # Check exact match first if cmd in allowlist: return True - + # Check if the base command (first word) is allowed base_cmd = cmd.split()[0] if cmd.split() else cmd if base_cmd in allowlist: return True - + return False def _confirm_hooks(self, hooks, hook_type="pre"): @@ -153,24 +153,24 @@ def _confirm_hooks(self, hooks, hook_type="pre"): """ if not hooks: return True - + print(f"\n⚠️ The following {hook_type}-hooks will be executed:") for cmd in hooks: print(f" - {cmd}") - + response = input(f"\nDo you want to run these {hook_type}-hooks? [y/N]: ").strip().lower() return response in ('y', 'yes') def _run_hooks(self, hooks, hook_type="pre", skip_hooks=False, non_interactive=False, allowlist=None): """Run pre/post hooks with safety controls. - + Args: hooks: List of shell commands to run hook_type: Type of hooks ("pre" or "post") skip_hooks: If True, skip all hooks non_interactive: If True, skip confirmation prompt allowlist: Set of allowed commands or None to allow all - + Returns: True if all hooks succeeded or were skipped, False if any failed """ @@ -178,7 +178,7 @@ def _run_hooks(self, hooks, hook_type="pre", skip_hooks=False, non_interactive=F if skip_hooks and hooks: self.logger.info(f"Skipping {hook_type}-hooks (--no-hooks enabled)") return True - + # Check if any hooks are blocked by allowlist if allowlist is not None: blocked_hooks = [cmd for cmd in hooks if not self._check_hook_allowed(cmd, allowlist)] @@ -188,13 +188,13 @@ def _run_hooks(self, hooks, hook_type="pre", skip_hooks=False, non_interactive=F self.logger.error(f" - {cmd}") self.logger.error("Hook execution blocked. Update allowlist or use --no-hooks to skip.") return False - + # Ask for confirmation in interactive mode if not non_interactive: if not self._confirm_hooks(hooks, hook_type): self.logger.info(f"User declined to run {hook_type}-hooks. Aborting.") return False - + for cmd in hooks: self.logger.info(f"Running {hook_type}-hook: {cmd}") try: @@ -314,7 +314,7 @@ def execute(self, args): default_allowlist = os.path.join(os.getcwd(), '.struct-hooks-allowlist') if os.path.exists(default_allowlist): allowlist_path = default_allowlist - + allowlist = self._load_hooks_allowlist(allowlist_path) if allowlist_path else None # Run pre-hooks