Skip to content

Commit 2463e4a

Browse files
dugshubclaude
andcommitted
feat(core): add comprehensive security hardening to models
Implement multiple layers of security protection in core models to prevent command injection, DoS attacks, and resource exhaustion. Security Enhancements: 1. Command Injection Prevention (BashActionConfig): - Added allow_shell_features flag (default: False) - Validates commands to block shell metacharacters - Rejects: pipes, redirects, command substitution, variable expansion - Clear error messages guide users to explicit opt-in - 13 tests for injection patterns 2. DoS Protection (SessionState): - Validates option_values and variables for depth/size - Maximum 1000 options/variables - Integration with validators module - 8 tests for DoS scenarios 3. Collection Size Limits: - BranchConfig: 100 actions, 50 options, 20 menus - WizardConfig: 100 branches - Prevents memory exhaustion from config files - 6 tests for collection limits 4. Entry Branch Validation (WizardConfig): - Ensures entry_branch exists in branches list - Helpful error messages show available branches - 3 tests for validation scenarios Test Coverage: - 30 security-focused tests in test_security.py - All existing tests updated and passing - 100% coverage of new security code Breaking Changes: - Commands with shell features now require allow_shell_features=True - Wizard configs with invalid entry_branch now fail validation - Large collections/deep nesting now rejected Migration: - Set allow_shell_features=True for commands needing pipes/redirects - Ensure entry_branch matches a branch ID - Review any configs with >100 branches or >50 options Part of security hardening (Priorities 1, 2, 3) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 7319aba commit 2463e4a

File tree

3 files changed

+556
-12
lines changed

3 files changed

+556
-12
lines changed

src/cli_patterns/core/models.py

Lines changed: 170 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,16 +10,18 @@
1010

1111
from __future__ import annotations
1212

13+
import re
1314
from typing import Any, Literal, Optional, Union
1415

15-
from pydantic import BaseModel, ConfigDict, Field
16+
from pydantic import BaseModel, ConfigDict, Field, field_validator, model_validator
1617

1718
from cli_patterns.core.types import (
1819
ActionId,
1920
BranchId,
2021
MenuId,
2122
OptionKey,
2223
)
24+
from cli_patterns.core.validators import ValidationError, validate_state_value
2325

2426
# StateValue is defined as Any for Pydantic compatibility
2527
# The actual type constraint (JSON-serializable) is enforced at serialization time
@@ -65,6 +67,11 @@ class BashActionConfig(BaseConfig):
6567
"""Configuration for bash command actions.
6668
6769
Executes a bash command with optional environment variables.
70+
71+
Security:
72+
By default, shell features (pipes, redirects, command substitution) are
73+
disabled to prevent command injection attacks. Set allow_shell_features=True
74+
only for trusted commands that require shell features.
6875
"""
6976

7077
type: Literal["bash"] = Field(
@@ -77,6 +84,47 @@ class BashActionConfig(BaseConfig):
7784
env: dict[str, str] = Field(
7885
default_factory=dict, description="Environment variables for command"
7986
)
87+
allow_shell_features: bool = Field(
88+
default=False,
89+
description=(
90+
"Allow shell features (pipes, redirects, command substitution). "
91+
"SECURITY RISK: Only enable for trusted commands. When False, "
92+
"command is executed without shell interpretation to prevent injection."
93+
),
94+
)
95+
96+
@model_validator(mode="after")
97+
def validate_command_safety(self) -> BashActionConfig:
98+
"""Validate command doesn't contain dangerous patterns.
99+
100+
This validator blocks shell injection attempts when allow_shell_features=False.
101+
102+
Returns:
103+
Validated model
104+
105+
Raises:
106+
ValueError: If command contains dangerous shell metacharacters
107+
"""
108+
if not self.allow_shell_features:
109+
# Dangerous shell metacharacters
110+
dangerous_patterns = [
111+
(r"[;&|]", "command chaining (;, &, |)"),
112+
(r"`", "command substitution (backticks)"),
113+
(r"\$\(", "command substitution ($())"),
114+
(r"[<>]", "redirection (<, >)"),
115+
(r"\$\{", "variable expansion (${})"),
116+
(r"^\s*\w+\s*=", "variable assignment"),
117+
]
118+
119+
for pattern, description in dangerous_patterns:
120+
if re.search(pattern, self.command):
121+
raise ValueError(
122+
f"Command contains {description}. "
123+
f"Set allow_shell_features=True to enable shell features "
124+
f"(SECURITY RISK: only do this for trusted commands)."
125+
)
126+
127+
return self
80128

81129

82130
class PythonActionConfig(BaseConfig):
@@ -220,6 +268,11 @@ class BranchConfig(BaseConfig):
220268
221269
A branch represents a screen/step in the wizard with actions, options,
222270
and navigation menus.
271+
272+
Limits:
273+
- Actions: 100 maximum
274+
- Options: 50 maximum
275+
- Menus: 20 maximum
223276
"""
224277

225278
id: BranchId = Field(description="Unique branch identifier")
@@ -235,6 +288,34 @@ class BranchConfig(BaseConfig):
235288
default_factory=list, description="Navigation menus in this branch"
236289
)
237290

291+
@field_validator("actions")
292+
@classmethod
293+
def validate_actions_size(
294+
cls, v: list[ActionConfigUnion]
295+
) -> list[ActionConfigUnion]:
296+
"""Validate number of actions is reasonable."""
297+
if len(v) > 100:
298+
raise ValueError("Too many actions in branch (maximum: 100)")
299+
return v
300+
301+
@field_validator("options")
302+
@classmethod
303+
def validate_options_size(
304+
cls, v: list[OptionConfigUnion]
305+
) -> list[OptionConfigUnion]:
306+
"""Validate number of options is reasonable."""
307+
if len(v) > 50:
308+
raise ValueError("Too many options in branch (maximum: 50)")
309+
return v
310+
311+
@field_validator("menus")
312+
@classmethod
313+
def validate_menus_size(cls, v: list[MenuConfig]) -> list[MenuConfig]:
314+
"""Validate number of menus is reasonable."""
315+
if len(v) > 20:
316+
raise ValueError("Too many menus in branch (maximum: 20)")
317+
return v
318+
238319

239320
# ============================================================================
240321
# Wizard Configuration
@@ -246,6 +327,9 @@ class WizardConfig(BaseConfig):
246327
247328
This is the top-level configuration that defines an entire wizard,
248329
including all branches and the entry point.
330+
331+
Limits:
332+
- Branches: 100 maximum
249333
"""
250334

251335
name: str = Field(description="Wizard name (identifier)")
@@ -256,8 +340,24 @@ class WizardConfig(BaseConfig):
256340
)
257341
branches: list[BranchConfig] = Field(description="All branches in the wizard tree")
258342

259-
# TODO: Add validator to ensure entry_branch exists in branches
260-
# This would be done with @model_validator in Pydantic v2
343+
@field_validator("branches")
344+
@classmethod
345+
def validate_branches_size(cls, v: list[BranchConfig]) -> list[BranchConfig]:
346+
"""Validate number of branches is reasonable."""
347+
if len(v) > 100:
348+
raise ValueError("Too many branches in wizard (maximum: 100)")
349+
return v
350+
351+
@model_validator(mode="after")
352+
def validate_entry_branch_exists(self) -> WizardConfig:
353+
"""Validate that entry_branch exists in branches list."""
354+
branch_ids = {b.id for b in self.branches}
355+
if self.entry_branch not in branch_ids:
356+
raise ValueError(
357+
f"entry_branch '{self.entry_branch}' not found in branches. "
358+
f"Available branches: {sorted(branch_ids)}"
359+
)
360+
return self
261361

262362

263363
# ============================================================================
@@ -270,6 +370,11 @@ class SessionState(StrictModel):
270370
271371
This model combines both wizard state (navigation, options) and
272372
parser state (mode, history) into a single unified state.
373+
374+
Security:
375+
All StateValue fields (option_values, variables) are validated for:
376+
- Maximum nesting depth (50 levels)
377+
- Maximum collection size (1000 items)
273378
"""
274379

275380
# Wizard state
@@ -295,6 +400,68 @@ class SessionState(StrictModel):
295400
default_factory=list, description="Command history for readline/recall"
296401
)
297402

403+
@field_validator("option_values")
404+
@classmethod
405+
def validate_option_values(
406+
cls, v: dict[OptionKey, StateValue]
407+
) -> dict[OptionKey, StateValue]:
408+
"""Validate all option values meet safety requirements.
409+
410+
Checks each value for:
411+
- Maximum nesting depth (50 levels)
412+
- Maximum collection size (1000 items)
413+
414+
Args:
415+
v: Option values dict to validate
416+
417+
Returns:
418+
Validated dict
419+
420+
Raises:
421+
ValueError: If any value violates safety limits
422+
"""
423+
# Check total number of options
424+
if len(v) > 1000:
425+
raise ValueError("Too many options (maximum: 1000)")
426+
427+
# Validate each value
428+
for key, value in v.items():
429+
try:
430+
validate_state_value(value)
431+
except ValidationError as e:
432+
raise ValueError(f"Invalid value for option '{key}': {e}") from e
433+
434+
return v
435+
436+
@field_validator("variables")
437+
@classmethod
438+
def validate_variables(cls, v: dict[str, StateValue]) -> dict[str, StateValue]:
439+
"""Validate all variables meet safety requirements.
440+
441+
Checks each value for:
442+
- Maximum nesting depth (50 levels)
443+
- Maximum collection size (1000 items)
444+
445+
Args:
446+
v: Variables dict to validate
447+
448+
Returns:
449+
Validated dict
450+
451+
Raises:
452+
ValueError: If any value violates safety limits
453+
"""
454+
if len(v) > 1000:
455+
raise ValueError("Too many variables (maximum: 1000)")
456+
457+
for key, value in v.items():
458+
try:
459+
validate_state_value(value)
460+
except ValidationError as e:
461+
raise ValueError(f"Invalid value for variable '{key}': {e}") from e
462+
463+
return v
464+
298465

299466
# ============================================================================
300467
# Result Types

tests/unit/core/test_models.py

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -472,17 +472,17 @@ def test_wizard_config_validates_entry_branch_exists(self) -> None:
472472
"""
473473
GIVEN: Wizard with entry_branch that doesn't exist in branches
474474
WHEN: Creating a WizardConfig
475-
THEN: Validation should succeed (validation is runtime, not construction)
475+
THEN: Validation error is raised
476476
"""
477477
branch = BranchConfig(id=make_branch_id("main"), title="Main")
478-
# Entry branch points to non-existent branch - this is allowed at construction
479-
config = WizardConfig(
480-
name="test-wizard",
481-
version="1.0.0",
482-
entry_branch=make_branch_id("nonexistent"),
483-
branches=[branch],
484-
)
485-
assert config.entry_branch == make_branch_id("nonexistent")
478+
# Entry branch points to non-existent branch - should raise validation error
479+
with pytest.raises(ValidationError, match="entry_branch.*not found"):
480+
WizardConfig(
481+
name="test-wizard",
482+
version="1.0.0",
483+
entry_branch=make_branch_id("nonexistent"),
484+
branches=[branch],
485+
)
486486

487487
def test_wizard_config_multiple_branches(self) -> None:
488488
"""

0 commit comments

Comments
 (0)