Threshold validation: Address remaining code review findings

[aaronlippold]

Context

Following the threshold refactoring (#2196), comprehensive code reviews by specialized agents identified several medium/low priority improvements. The critical and high severity issues have been addressed, but these items remain for future enhancement.

---

Security Issues (Medium Priority)

1. Enhanced JSON/HDF Structure Validation

Severity: MEDIUM
Location: src/commands/validate/threshold.ts:183-186

Current:

if (!parsedExecJSON.contains || parsedExecJSON.contains.length === 0) {
  this.error('Invalid HDF file: No profiles found')
}
profile = parsedExecJSON.contains[0] as ContextualizedProfile

Issue: No validation that contains[0] has required fields before type assertion.

Recommendation:

const profileCandidate = parsedExecJSON.contains[0]

// Validate required fields exist
if (!profileCandidate || 
    typeof profileCandidate !== 'object' ||
    !profileCandidate.data ||
    !Array.isArray(profileCandidate.contains)) {
  this.error('Invalid HDF file: Profile structure is malformed')
}

profile = profileCandidate as ContextualizedProfile

2. Command Injection Protection in Inline Format

Severity: MEDIUM
Location: src/commands/validate/threshold.ts:139-145

Current: Basic string parsing without sanitization validation

Recommendation: Add validation for dangerous property names before using parsed keys:

for (const flattenedObject of flattenedObjects) {
  const [key, value] = flattenedObject.split(':')
  const trimmedKey = key.trim()
  
  // Check for dangerous property names
  if (trimmedKey.includes('__proto__') || 
      trimmedKey.includes('constructor') || 
      trimmedKey.includes('prototype')) {
    this.error('Invalid threshold key: dangerous property name')
  }
  
  toUnpack[trimmedKey] = Number.parseInt(value.trim(), 10)
}

---

DRY Issues (Medium Priority)

3. Path Splitting Pattern Duplication

Severity: MEDIUM
Location: src/utils/threshold/validators.ts (lines 160, 184, 209, 255)

Current: Repeated path.split('.') pattern 4 times

Recommendation: Create helper function:

interface ParsedThresholdPath {
  statusName: ThresholdStatus
  severity?: Severity | 'total' | 'none'
  type?: 'min' | 'max' | 'controls'
}

function parseThresholdPath(path: string): ParsedThresholdPath {
  const parts = path.split('.')
  const [statusName, severity, type] = parts
  return {
    statusName: statusName as ThresholdStatus,
    severity: severity as Severity | 'total' | 'none',
    type: type as 'min' | 'max' | 'controls',
  }
}

4. Error Message Formatting Duplication

Severity: MEDIUM
Location: src/commands/validate/threshold.ts (3 occurrences)

Current:

error instanceof Error ? error.message : String(error)

Recommendation: Extract to BaseCommand helper:

protected formatErrorMessage(error: unknown): string {
  return error instanceof Error ? error.message : String(error)
}

---

Maintainability Issues (Medium Priority)

5. Missing Architecture Documentation

Severity: MEDIUM

Recommendation: Create src/utils/threshold/README.md with:

• Module dependency diagram
• Data flow diagram
• "Adding new features" guide
• Domain concept explanations (what is compliance, why exclude Not Applicable, etc.)

6. Function Naming Improvements

Severity: LOW

Current naming is confusing:

• renameStatusName() - doesn't clearly indicate direction
• reverseStatusName() - reverse of what?

Better names:

• toHdfStatusName() - clear conversion direction
• toThresholdStatusName() - clear conversion direction

---

UX Polish (Low Priority)

7. Improve Recommendation Text

Severity: LOW
Location: src/utils/threshold/output-formatter.ts:355-376

Current: Generic text like "Address 1 controls that exceed failure thresholds"

Better: Specific actionable steps:

💡 Recommendations
  • Fix 1 high severity no_impact control (reduce from 3 to ≤2)
  • Update threshold file if current results are acceptable:
    $ saf generate threshold -i results.json -o updated-threshold.yaml

8. Display Filter Clarity

Severity: LOW

When using --display-* filters, make it clearer that full validation still occurred:

✗ Validation failed (1/26 total checks)

Filtered display (showing critical,high only):
  • no_impact.high.max: 3 > 2 (exceeds by 1)

Note: Showing 10/26 checks. Validation included all checks.

---

Implementation Priority

Recommended Order:

1. Enhanced JSON/HDF validation (Security)
2. Path splitting helper (DRY)
3. Architecture README (Maintainability)
4. Error formatting helper (DRY)
5. Function renaming (Maintainability)
6. Recommendation improvements (UX)
7. Display filter clarity (UX)

Estimated Effort: 3-4 hours total

---

Notes

All critical issues from code review have been addressed in the refactoring PR. These remaining items are enhancements that improve quality but are not blockers for shipping the refactoring.

Related: #2196 (threshold refactoring), #5041 (OCLIF README markers)