T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/review.sh:359
- Finding
- Security findings are discarded because pipeline loops execute in subshells<![CDATA[ ## Vulnerability Details **File Location**: `scripts/review.sh`, lines 359–403 **Vulnerability Type**: Security scanner fail-open behavior caused by lost subshell state **Risk Level**: High ### Vulnerable Code ```bash # SEC-001: Hardcoded secrets while IFS= read -r file; do if grep -nEi '(api[_-]?key|secret|password|token|credential)\s*[=:]\s*["\x27][^"\x27]{8,}' "$file" 2>/dev/null | head -5 | while IFS=: read -r line_num content; do add_issue "critical" "security" "SEC-001" "Possible hardcoded secret detected" "$file" "$line_num" done; then true; fi done <<< "$CHANGED_FILES" # SEC-002: SQL injection patterns if [[ "$HAS_PYTHON" == "true" ]] || [[ "$HAS_JS" == "true" ]]; then while IFS= read -r file; do if grep -nE '(execute|query)\s*\(\s*["\x27].*(%s|\$\{|" *\+)' "$file" 2>/dev/null | head -5 | while IFS=: read -r line_num content; do add_issue "critical" "security" "SEC-002" "Possible SQL injection via string interpolation" "$file" "$line_num" done; then true; fi done <<< "$CHANGED_FILES" fi # SEC-003: eval/exec usage while IFS= read -r file; do if grep -nE '\b(eval|exec)\s*\(' "$file" 2>/dev/null | head -5 | while IFS=: read -r line_num content; do add_issue "warning" "security" "SEC-003" "Use of eval/exec detected" "$file" "$line_num" done; then true; fi done <<< "$CHANGED_FILES" # SEC-004: HTTP URLs (should be HTTPS) while IFS= read -r file; do if grep -nE 'http://[^l][^o][^c]' "$file" 2>/dev/null | grep -v 'localhost\|127\.0\.0\.1\|0\.0\.0\.0' | head -5 | while IFS=: read -r line_num content; do add_issue "warning" "security" "SEC-004" "HTTP URL detected (should use HTTPS)" "$file" "$line_num" done; then true; fi done <<< "$CHANGED_FILES" # SEC-005: Disabled TLS verification while IFS= read -r file; do if grep -nEi '(verify\s*=\s*False|NODE_TLS_REJECT_UNAUTHORIZED.*0|InsecureSkipVerify.*true)' "$file" 2>/dev/null | head -5 | while IFS=: read -r line_num ...[truncated 3488 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions Replace every pipeline-fed state-mutating loop with process substitution so that the `while` loop executes in the current shell: ```bash while IFS=: read -r line_num content; do add_issue "critical" "security" "SEC-001" \ "Possible hardcoded secret detected" "$file" "$line_num" done < <( grep -nEi \ '(api[_-]?key|secret|password|token|credential)\s*[=:]\s*["\x27][^"\x27]{8,}' \ "$file" 2>/dev/null | head -5 ) ``` Apply the same structure to SEC-002 through SEC-006 and to any other check that modifies arrays or counters from a pipeline-fed loop. Additional hardening should include: 1. Add automated tests containing one positive fixture for every security check. 2. Assert that each fixture appears in Markdown, JSON, and text output. 3. Assert that every critical fixture causes exit status 1. 4. Add a negative fixture for each check to detect false positives. 5. Run the test suite under the minimum documented Bash version and current supported versions. 6. Avoid relying solely on `shopt -s lastpipe`, because its behavior depends on shell mode and environment; process substitution is explicit and more portable within the declared Bash requirement. 7. Add a CI regression test that fails if a detected issue does not increment the corresponding parent-shell counter. ]]>