|
| 1 | +=== ADR-013: Security Mitigations - Tier 2 Implementation |
| 2 | + |
| 3 | +*Status:* Accepted (2026-02-11) |
| 4 | + |
| 5 | +*Deciders:* Development Team + Claude Code |
| 6 | + |
| 7 | +*Context:* |
| 8 | + |
| 9 | +Following the Tier 2 risk classification for both dacli CLI and dacli-mcp modules (see ADR-011 and ADR-012), the project requires implementation of comprehensive security and quality assurance measures. The Risk Radar framework mandates cumulative mitigations: all Tier 1 measures plus all Tier 2 measures. |
| 10 | + |
| 11 | +Both modules share the same codebase (`src/dacli/`), so mitigations are applied repository-wide rather than per module. |
| 12 | + |
| 13 | +*Decision:* |
| 14 | + |
| 15 | +Implement **all required Tier 1 and Tier 2 mitigation measures** as repository-wide protections: |
| 16 | + |
| 17 | +**Tier 1 — Automated Gates:** |
| 18 | + |
| 19 | +. **Linter & Formatter:** Ruff configured in `pyproject.toml` with enforced rules (E, F, I, N, W, UP) and 100-character line length |
| 20 | +. **Pre-Commit Hooks:** Configured via `.pre-commit-config.yaml` (commit 68d6ae4) with Ruff checks |
| 21 | +. **Dependency Vulnerability Scanning:** `pip-audit` integrated in CI pipeline (commit fee56b6) |
| 22 | +. **CI Build & Unit Tests:** GitHub Actions workflow (`.github/workflows/test.yml`) running 713 automated tests with coverage reporting |
| 23 | + |
| 24 | +**Tier 2 — Extended Assurance:** |
| 25 | + |
| 26 | +. **SAST (Static Application Security Testing):** CodeQL workflow with `security-extended` query suite (commit fead47e), runs on upstream repository only |
| 27 | +. **AI-Assisted Code Review:** Claude Code review workflow (`.github/workflows/claude-code-review.yml`) for automated PR analysis |
| 28 | +. **Property-Based Testing:** Hypothesis framework (commit 87a965d) with 11 property-based tests generating 1,100+ test cases (`tests/test_property_based.py`) |
| 29 | +. **Code Quality Gate:** SonarCloud integration (commit fb4c8ad) via `.github/workflows/sonarcloud.yml` and `sonar-project.properties` |
| 30 | +. **PR Review Policy with Sampling:** Risk-based review policy documented in `.github/PR_REVIEW_POLICY.md` (commit efb868f): |
| 31 | + * 100% review for security-sensitive changes, breaking changes, architecture changes |
| 32 | + * 20-30% sampling for bug fixes, refactoring, tests, documentation |
| 33 | + * Auto-merge eligible: non-security dependency updates, formatting fixes, PATCH version bumps |
| 34 | + |
| 35 | +**Security Fixes Applied:** |
| 36 | + |
| 37 | +* `cryptography` upgraded from 46.0.3 → 46.0.5 (CVE-2026-26007 mitigation) |
| 38 | +* `pip` upgraded from 24.0 → 26.0.1 |
| 39 | +* Commit: 7766e90 |
| 40 | +
|
| 41 | +*Consequences:* |
| 42 | + |
| 43 | +**Positive:** |
| 44 | + |
| 45 | +* **100% mitigation coverage** for both Tier 1 (4/4 measures) and Tier 2 (5/5 measures) |
| 46 | +* **Zero known vulnerabilities** (pip-audit clean) |
| 47 | +* **Comprehensive test coverage:** 713 unit/integration tests + 11 property-based tests (1,100+ generated cases) |
| 48 | +* **Continuous quality monitoring:** SonarCloud provides ongoing code quality metrics and technical debt tracking |
| 49 | +* **Automated security scanning:** CodeQL runs on every push to main, catching potential vulnerabilities before production |
| 50 | +* **Efficient review process:** Sampling policy (20-30%) balances thoroughness with development velocity |
| 51 | +* **Developer experience:** Pre-commit hooks catch issues locally before CI, reducing feedback loop time |
| 52 | +
|
| 53 | +**Negative:** |
| 54 | + |
| 55 | +* **CI pipeline duration increase:** ~2-3 minutes added for SAST (CodeQL) and quality gate (SonarCloud) checks |
| 56 | +* **Developer onboarding overhead:** New contributors must understand pre-commit hooks, review policy, and quality standards |
| 57 | +* **Maintenance burden:** |
| 58 | + - CodeQL query suite updates needed for new Python versions |
| 59 | + - SonarCloud project configuration requires manual setup and token management |
| 60 | + - Hypothesis tests may need strategy refinement as edge cases are discovered |
| 61 | +* **External service dependencies:** |
| 62 | + - SonarCloud outages block PRs (mitigated by making check non-blocking in fork workflow) |
| 63 | + - CodeQL only runs on upstream repository (not on fork PRs) |
| 64 | +* **False positive handling:** SAST tools may flag intentional patterns (e.g., dynamic code in MCP server), requiring suppression annotations |
| 65 | +
|
| 66 | +==== Pugh Matrix: Mitigation Strategy |
| 67 | + |
| 68 | +[cols="3,1,1,1"] |
| 69 | +|=== |
| 70 | +| Criterion | Repository-wide (Baseline) | Module-specific | Tier 1 Only |
| 71 | + |
| 72 | +| Implementation Simplicity | 0 | - | + |
| 73 | +| Coverage Completeness | 0 | 0 | - |
| 74 | +| Maintenance Burden | 0 | - | + |
| 75 | +| Risk Mitigation Effectiveness | 0 | 0 | - |
| 76 | +| Compliance with Tier 2 | 0 | 0 | - |
| 77 | +| **Total** | **0** | **-2** | **-1** |
| 78 | +|=== |
| 79 | + |
| 80 | +_Legend: + better than baseline, 0 same as baseline, - worse than baseline_ |
| 81 | + |
| 82 | +**Module-specific approach rejected:** Both modules share the same codebase (`src/dacli/`). Applying mitigations per module would duplicate CI checks, complicate maintenance, and provide no additional risk reduction. |
| 83 | + |
| 84 | +**Tier 1 only rejected:** Insufficient coverage for Tier 2 classification. Missing SAST, property-based testing, and quality gates would leave critical gaps in security and quality assurance. |
| 85 | + |
| 86 | +**Repository-wide Tier 1+2 selected:** Simplest implementation, consistent protection across all entry points (CLI and MCP), compliant with Risk Radar tier requirements. |
| 87 | + |
| 88 | +==== Alternative Mitigation Measures Considered |
| 89 | + |
| 90 | +**Static Type Checking (mypy):** |
| 91 | + |
| 92 | +* **Rejected for Tier 1:** Python project without strict typing. Retrofitting type annotations to 64+ files would be high effort with moderate benefit. FastMCP framework uses dynamic features that complicate type checking. |
| 93 | +* **Future consideration:** May be added incrementally as codebase matures, but not required for current Tier 2 classification. |
| 94 | + |
| 95 | +**Fuzzing (AFL, cargo-fuzz):** |
| 96 | + |
| 97 | +* **Not required for Tier 2:** Fuzzing is a Tier 3 measure. The project's internal tool deployment context and recoverable data loss blast radius don't justify the complexity and CI time cost of continuous fuzzing. |
| 98 | + |
| 99 | +**Branch Protection:** |
| 100 | + |
| 101 | +* **Not required for Tier 2:** Branch protection (required status checks, mandatory reviews) is a Tier 3 measure. Current PR review policy with sampling (20-30%) provides adequate oversight for internal tool risk profile. |
| 102 | + |
| 103 | +==== Implementation Timeline |
| 104 | + |
| 105 | +All mitigations implemented between 2026-02-09 and 2026-02-11 as part of PR #279: |
| 106 | + |
| 107 | +. Pre-commit hooks: commit 68d6ae4 |
| 108 | +. Dependency vulnerability scanning (pip-audit): commit fee56b6 |
| 109 | +. Security fixes (cryptography, pip): commit 7766e90 |
| 110 | +. CodeQL SAST workflow: commit fead47e |
| 111 | +. Property-based tests (Hypothesis): commit 87a965d |
| 112 | +. SonarCloud quality gate: commit fb4c8ad |
| 113 | +. PR review policy: commit efb868f |
| 114 | + |
| 115 | +**Verification:** All 713 tests passing, CI green, pip-audit clean, CodeQL and SonarCloud integrated. |
| 116 | + |
| 117 | +==== Module-Specific Notes |
| 118 | + |
| 119 | +**PR Review Policy - Differential Application:** |
| 120 | + |
| 121 | +While most mitigations are truly repository-wide, the PR review policy applies differentially based on change type (not module): |
| 122 | + |
| 123 | +* **100% mandatory review:** |
| 124 | + - Security-sensitive changes (auth, crypto, file system ops with user paths) |
| 125 | + - Breaking changes (public API, CLI interface, configuration format) |
| 126 | + - Architecture changes (new components, core parsers, data model) |
| 127 | + - Release preparation (MINOR/MAJOR version bumps) |
| 128 | + |
| 129 | +* **20-30% sampling review:** |
| 130 | + - Bug fixes (prioritize critical bugs) |
| 131 | + - Internal refactoring (prioritize complex changes) |
| 132 | + - Test additions (prioritize property-based/integration tests) |
| 133 | + - Documentation updates (prioritize user-facing docs) |
| 134 | + |
| 135 | +* **Auto-merge eligible:** |
| 136 | + - Dependency updates (PATCH, non-security, passing CI) |
| 137 | + - Formatting/linting fixes (no logic changes) |
| 138 | + - PATCH version bumps (small fixes, no API changes) |
| 139 | + |
| 140 | +This differential approach ensures critical changes receive thorough review while maintaining development velocity for lower-risk changes. |
0 commit comments