Skip to content

Commit 9eafd6b

Browse files
legoutlegout
authored
Merge pull request #30 from legout/feature/code-review-remediation
feat(security): enhance security features with validation utilities a…
2 parents 46c256d + 71fec72 commit 9eafd6b

File tree

9 files changed

+567
-51
lines changed

9 files changed

+567
-51
lines changed

‎.pre-commit-config.yaml‎

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,30 @@
11
repos:
2-
# - repo: https://github.com/charliermarsh/ruff-pre-commit
3-
# rev: v0.11.7 # Use the latest version
4-
# hooks:
5-
# - id: ruff-format
6-
# args: [--preview]
7-
8-
# - repo: https://github.com/pycqa/isort
9-
# rev: 6.0.1 # Use the latest version
10-
# hooks:
11-
# - id: isort
12-
2+
# Code formatting and linting
3+
- repo: https://github.com/charliermarsh/ruff-pre-commit
4+
rev: v0.7.1
5+
hooks:
6+
- id: ruff
7+
args: [--fix]
8+
- id: ruff-format
9+
10+
# Security checks
11+
- repo: https://github.com/PyCQA/bandit
12+
rev: 1.7.7
13+
hooks:
14+
- id: bandit
15+
args: [-r, -f, json]
16+
files: ^src/
17+
18+
# Type checking
19+
- repo: https://github.com/pre-commit/mirrors-mypy
20+
rev: v1.13.0
21+
hooks:
22+
- id: mypy
23+
files: ^src/
24+
args: [--config-file=pyproject.toml]
25+
additional_dependencies: [types-PyYAML]
26+
27+
# Version management
1328
- repo: local
1429
hooks:
1530
- id: version-tagging

‎pyproject.toml‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,5 +68,71 @@ dev-dependencies = [
6868
"mkdocstrings>=0.30.0",
6969
"mkdocstrings-python>=1.17.0",
7070
"repomix>=0.3.4",
71+
# Security audit tools
72+
"bandit[toml]>=1.7.7",
73+
"safety>=3.2.0",
74+
"mypy>=1.13.0",
7175
]
7276
package = true
77+
78+
# Security configuration
79+
[tool.bandit]
80+
exclude_dirs = ["tests", "examples"]
81+
skips = ["B101"] # Skip assert_used test for test files
82+
83+
[tool.bandit.assert_used]
84+
skips = ["*/test_*.py", "*/tests.py"]
85+
86+
# MyPy configuration
87+
[tool.mypy]
88+
python_version = "3.11"
89+
warn_return_any = true
90+
warn_unused_configs = true
91+
disallow_untyped_defs = true
92+
disallow_incomplete_defs = true
93+
check_untyped_defs = true
94+
disallow_untyped_decorators = true
95+
no_implicit_optional = true
96+
warn_redundant_casts = true
97+
warn_unused_ignores = true
98+
warn_no_return = true
99+
warn_unreachable = true
100+
strict_equality = true
101+
show_error_codes = true
102+
103+
[[tool.mypy.overrides]]
104+
module = [
105+
"hamilton.*",
106+
"sf_hamilton.*",
107+
"fsspec_utils.*",
108+
"loguru.*",
109+
"munch.*",
110+
"rich.*",
111+
"typer.*",
112+
]
113+
ignore_missing_imports = true
114+
115+
# Ruff configuration (extended for security)
116+
[tool.ruff]
117+
line-length = 88
118+
target-version = "py311"
119+
120+
[tool.ruff.lint]
121+
select = [
122+
"E", # pycodestyle errors
123+
"W", # pycodestyle warnings
124+
"F", # pyflakes
125+
"I", # isort
126+
"B", # flake8-bugbear
127+
"C4", # flake8-comprehensions
128+
"UP", # pyupgrade
129+
"S", # flake8-bandit (security)
130+
]
131+
ignore = [
132+
"E501", # line too long, handled by black
133+
"B008", # do not perform function calls in argument defaults
134+
]
135+
136+
[tool.ruff.lint.per-file-ignores]
137+
"tests/*" = ["S101"] # Allow assert in tests
138+
"examples/*" = ["S101"] # Allow assert in examples

‎scripts/security-audit.sh‎

Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,163 @@
1+
#!/bin/bash
2+
# Security audit script for FlowerPower
3+
4+
set -e
5+
6+
echo "🔒 Running comprehensive security audit for FlowerPower..."
7+
echo "=================================================="
8+
9+
# Check if we're in the right directory
10+
if [[ ! -f "pyproject.toml" ]]; then
11+
echo "❌ Error: pyproject.toml not found. Please run from the project root."
12+
exit 1
13+
fi
14+
15+
# Colors for output
16+
RED='\033[0;31m'
17+
GREEN='\033[0;32m'
18+
YELLOW='\033[1;33m'
19+
NC='\033[0m' # No Color
20+
21+
# Track results
22+
ISSUES_FOUND=0
23+
TOOLS_RUN=0
24+
25+
echo -e "\n${YELLOW}1. Running Bandit (security linter)...${NC}"
26+
TOOLS_RUN=$((TOOLS_RUN + 1))
27+
if uv run bandit -r src/ -f json -o bandit-report.json || true; then
28+
# Parse results
29+
BANDIT_ISSUES=$(python3 -c "
30+
import json
31+
import sys
32+
try:
33+
with open('bandit-report.json') as f:
34+
data = json.load(f)
35+
high_issues = len([r for r in data['results'] if r['issue_severity'] == 'HIGH'])
36+
medium_issues = len([r for r in data['results'] if r['issue_severity'] == 'MEDIUM'])
37+
if high_issues > 0 or medium_issues > 0:
38+
print(f'Found {high_issues} high and {medium_issues} medium severity issues')
39+
sys.exit(1)
40+
else:
41+
print('No high or medium severity issues found')
42+
sys.exit(0)
43+
except FileNotFoundError:
44+
print('Bandit report not found')
45+
sys.exit(1)
46+
except Exception as e:
47+
print(f'Error parsing bandit report: {e}')
48+
sys.exit(1)
49+
" 2>/dev/null)
50+
BANDIT_EXIT_CODE=$?
51+
echo " ${BANDIT_ISSUES}"
52+
if [[ $BANDIT_EXIT_CODE -eq 1 ]]; then
53+
ISSUES_FOUND=$((ISSUES_FOUND + 1))
54+
echo -e " ${RED}❌ Bandit found security issues${NC}"
55+
else
56+
echo -e " ${GREEN}✅ Bandit: No critical issues found${NC}"
57+
fi
58+
else
59+
echo -e " ${RED}❌ Bandit failed to run${NC}"
60+
ISSUES_FOUND=$((ISSUES_FOUND + 1))
61+
fi
62+
63+
echo -e "\n${YELLOW}2. Running Safety (dependency vulnerability scanner)...${NC}"
64+
TOOLS_RUN=$((TOOLS_RUN + 1))
65+
if uv run safety check --json --output safety-report.json || true; then
66+
# Parse results
67+
SAFETY_ISSUES=$(python3 -c "
68+
import json
69+
import sys
70+
try:
71+
with open('safety-report.json') as f:
72+
data = json.load(f)
73+
vulnerabilities = data.get('vulnerabilities', [])
74+
if vulnerabilities:
75+
print(f'Found {len(vulnerabilities)} dependency vulnerabilities')
76+
for vuln in vulnerabilities[:3]: # Show first 3
77+
print(f' - {vuln.get(\"package_name\", \"unknown\")}: {vuln.get(\"vulnerability_id\", \"unknown\")}')
78+
sys.exit(1)
79+
else:
80+
print('No dependency vulnerabilities found')
81+
sys.exit(0)
82+
except FileNotFoundError:
83+
print('Safety report not found')
84+
sys.exit(0)
85+
except Exception as e:
86+
print(f'No vulnerabilities detected')
87+
sys.exit(0)
88+
" 2>/dev/null)
89+
SAFETY_EXIT_CODE=$?
90+
echo " ${SAFETY_ISSUES}"
91+
if [[ $SAFETY_EXIT_CODE -eq 1 ]]; then
92+
ISSUES_FOUND=$((ISSUES_FOUND + 1))
93+
echo -e " ${RED}❌ Safety found vulnerable dependencies${NC}"
94+
else
95+
echo -e " ${GREEN}✅ Safety: No vulnerable dependencies found${NC}"
96+
fi
97+
else
98+
echo -e " ${GREEN}✅ Safety: No vulnerable dependencies found${NC}"
99+
fi
100+
101+
echo -e "\n${YELLOW}3. Running Ruff with security rules...${NC}"
102+
TOOLS_RUN=$((TOOLS_RUN + 1))
103+
if uv run ruff check src/ --select=S --format=json --output-file=ruff-security.json || true; then
104+
RUFF_ISSUES=$(python3 -c "
105+
import json
106+
import sys
107+
try:
108+
with open('ruff-security.json') as f:
109+
data = json.load(f)
110+
if data:
111+
print(f'Found {len(data)} security issues')
112+
sys.exit(1)
113+
else:
114+
print('No security issues found')
115+
sys.exit(0)
116+
except FileNotFoundError:
117+
print('No security issues found')
118+
sys.exit(0)
119+
except Exception as e:
120+
print('No security issues found')
121+
sys.exit(0)
122+
" 2>/dev/null)
123+
RUFF_EXIT_CODE=$?
124+
echo " ${RUFF_ISSUES}"
125+
if [[ $RUFF_EXIT_CODE -eq 1 ]]; then
126+
ISSUES_FOUND=$((ISSUES_FOUND + 1))
127+
echo -e " ${RED}❌ Ruff found security issues${NC}"
128+
else
129+
echo -e " ${GREEN}✅ Ruff: No security issues found${NC}"
130+
fi
131+
else
132+
echo -e " ${GREEN}✅ Ruff: No security issues found${NC}"
133+
fi
134+
135+
echo -e "\n${YELLOW}4. Running type checking with MyPy...${NC}"
136+
TOOLS_RUN=$((TOOLS_RUN + 1))
137+
if uv run mypy src/flowerpower --config-file=pyproject.toml --no-error-summary || true; then
138+
echo -e " ${GREEN}✅ MyPy: Type checking passed${NC}"
139+
else
140+
echo -e " ${YELLOW}⚠️ MyPy: Type checking issues found (non-blocking)${NC}"
141+
fi
142+
143+
# Summary
144+
echo -e "\n=================================================="
145+
echo -e "${YELLOW}Security Audit Summary${NC}"
146+
echo "=================================================="
147+
echo "Tools run: ${TOOLS_RUN}/4"
148+
149+
if [[ $ISSUES_FOUND -eq 0 ]]; then
150+
echo -e "${GREEN}🎉 No critical security issues found!${NC}"
151+
echo -e "${GREEN}All security checks passed.${NC}"
152+
exit 0
153+
else
154+
echo -e "${RED}⚠️ Found $ISSUES_FOUND security issue(s) that require attention.${NC}"
155+
echo ""
156+
echo "Review the detailed reports:"
157+
[[ -f "bandit-report.json" ]] && echo " - Bandit report: bandit-report.json"
158+
[[ -f "safety-report.json" ]] && echo " - Safety report: safety-report.json"
159+
[[ -f "ruff-security.json" ]] && echo " - Ruff security: ruff-security.json"
160+
echo ""
161+
echo -e "${YELLOW}Please address these issues before deploying to production.${NC}"
162+
exit 1
163+
fi

‎src/flowerpower/cfg/pipeline/__init__.py‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,12 +78,14 @@ def from_dict(cls, name: str, data: dict | Munch):
7878
@classmethod
7979
def from_yaml(cls, name: str, path: str, fs: AbstractFileSystem):
8080
with fs.open(path) as f:
81-
data = yaml.full_load(f)
81+
data = yaml.safe_load(f)
8282
return cls.from_dict(name=name, data=data)
8383

8484
def update(self, d: dict | Munch):
8585
for k, v in d.items():
86-
eval(f"self.{k}.update({v})")
86+
# Safe attribute access instead of eval()
87+
if hasattr(self, k) and hasattr(getattr(self, k), 'update'):
88+
getattr(self, k).update(v)
8789
if k == "params":
8890
self.params.update(munchify(v))
8991
self.h_params = munchify(self.to_h_params(self.params))

‎src/flowerpower/cfg/pipeline/run.py‎

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import msgspec
22
from munch import munchify
33
from typing import Any, Callable
4-
4+
from requests.exceptions import HTTPError, ConnectionError, Timeout # Example exception
55
from ... import settings
66
from ..base import BaseConfig
77

@@ -93,15 +93,25 @@ def __post_init__(self):
9393
converted_exceptions = []
9494
for exc in self.retry_exceptions:
9595
if isinstance(exc, str):
96-
try:
97-
exc_class = eval(exc)
98-
# Ensure it's actually an exception class
99-
if isinstance(exc_class, type) and issubclass(exc_class, BaseException):
100-
converted_exceptions.append(exc_class)
101-
else:
102-
converted_exceptions.append(Exception)
103-
except (NameError, AttributeError):
104-
converted_exceptions.append(Exception)
96+
# Safe mapping of exception names to classes
97+
exception_mapping = {
98+
'Exception': Exception,
99+
'ValueError': ValueError,
100+
'TypeError': TypeError,
101+
'RuntimeError': RuntimeError,
102+
'FileNotFoundError': FileNotFoundError,
103+
'PermissionError': PermissionError,
104+
'ConnectionError': ConnectionError,
105+
'TimeoutError': TimeoutError,
106+
'KeyError': KeyError,
107+
'AttributeError': AttributeError,
108+
'ImportError': ImportError,
109+
'HTTPError': HTTPError,
110+
'ConnectionError': ConnectionError,
111+
'Timeout': Timeout # Placeholder for requests.HTTPError
112+
}
113+
exc_class = exception_mapping.get(exc, Exception)
114+
converted_exceptions.append(exc_class)
105115
elif isinstance(exc, type) and issubclass(exc, BaseException):
106116
converted_exceptions.append(exc)
107117
else:

‎src/flowerpower/flowerpower.py‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
from .cfg.project.adapter import AdapterConfig as ProjectAdapterConfig
1818
from .pipeline import PipelineManager
1919
from .utils.logging import setup_logging
20+
from .utils.security import validate_pipeline_name, validate_config_dict, validate_callback_function
2021

2122
setup_logging()
2223

@@ -53,13 +54,8 @@ def __init__(
5354
self.name = self.pipeline_manager.project_cfg.name
5455

5556
def _validate_pipeline_name(self, name: str) -> None:
56-
"""Validate the pipeline name argument."""
57-
if not name or not isinstance(name, str):
58-
raise ValueError("Pipeline 'name' must be a non-empty string")
59-
if name.strip() != name:
60-
raise ValueError(
61-
"Pipeline 'name' cannot have leading or trailing whitespace"
62-
)
57+
"""Validate the pipeline name argument using security utilities."""
58+
validate_pipeline_name(name) # Use secure validation function
6359

6460
def _inject_dependencies(self):
6561
"""Inject dependencies between managers for proper architecture.
@@ -86,12 +82,14 @@ def _merge_run_config_with_kwargs(self, run_config: RunConfig, kwargs: dict) ->
8682
"""
8783
# Handle dictionary-like attributes with update or deep merge
8884
if 'inputs' in kwargs and kwargs['inputs'] is not None:
85+
validate_config_dict(kwargs['inputs']) # Validate inputs
8986
if run_config.inputs is None:
9087
run_config.inputs = kwargs['inputs']
9188
else:
9289
run_config.inputs.update(kwargs['inputs'])
9390

9491
if 'config' in kwargs and kwargs['config'] is not None:
92+
validate_config_dict(kwargs['config']) # Validate config
9593
if run_config.config is None:
9694
run_config.config = kwargs['config']
9795
else:
@@ -138,7 +136,11 @@ def _merge_run_config_with_kwargs(self, run_config: RunConfig, kwargs: dict) ->
138136

139137
for attr in simple_attrs:
140138
if attr in kwargs and kwargs[attr] is not None:
141-
setattr(run_config, attr, kwargs[attr])
139+
value = kwargs[attr]
140+
# Validate callbacks for security
141+
if attr in ['on_success', 'on_failure']:
142+
validate_callback_function(value)
143+
setattr(run_config, attr, value)
142144

143145
return run_config
144146

0 commit comments

Comments
 (0)