Fix false positives in QA script checks
Refined two checks that were generating false positive warnings:
1. SCRIPT_DIR check (was HIGH, now MEDIUM):
- Previously flagged ALL 59 files that define SCRIPT_DIR
- Now only flags library files (which shouldn't define paths)
- Executable scripts CORRECTLY define their own SCRIPT_DIR
- Added note explaining this is not a collision
2. USERDATA-ACCESS check (was CRITICAL, now MEDIUM):
- Reduced severity from CRITICAL to MEDIUM (code quality, not security)
- Added exclusions for legitimate use cases:
- QA script itself (searches for this pattern)
- Diagnostic/analysis tools (malware-scanner, error-analyzer, etc.)
- These tools need direct access by design
- Changed message to suggest abstractions rather than demand them
This eliminates 7 false CRITICAL warnings and 1 false HIGH warning,
making the QA report more actionable.
This commit is contained in:
+25
-14
@@ -196,20 +196,24 @@ echo ""
|
|||||||
} >> "$REPORT"
|
} >> "$REPORT"
|
||||||
|
|
||||||
#==============================================================================
|
#==============================================================================
|
||||||
# CHECK 2: SCRIPT_DIR collisions (HIGH - causes path errors)
|
# CHECK 2: SCRIPT_DIR in library files (MEDIUM - library files shouldn't define paths)
|
||||||
#==============================================================================
|
#==============================================================================
|
||||||
echo "[2/42] Checking: SCRIPT_DIR variable collisions..."
|
echo "[2/42] Checking: SCRIPT_DIR variable collisions..."
|
||||||
{
|
{
|
||||||
echo "## CHECK 2: SCRIPT_DIR variable collisions"
|
echo "## CHECK 2: SCRIPT_DIR in library files"
|
||||||
echo "Severity: HIGH"
|
echo "Severity: MEDIUM"
|
||||||
echo "Issue: Multiple files redefining same path variable"
|
echo "Issue: Library files (sourced) shouldn't define SCRIPT_DIR - executable scripts should"
|
||||||
|
echo "Note: Each executable script correctly defines its own SCRIPT_DIR - not a collision"
|
||||||
echo ""
|
echo ""
|
||||||
|
|
||||||
script_dir_count=$(find "$TOOLKIT_PATH" -name "*.sh" -type f -exec grep -l "^SCRIPT_DIR=" {} \; 2>/dev/null | wc -l)
|
# Only check library files - executable scripts SHOULD define SCRIPT_DIR
|
||||||
if [ "$script_dir_count" -gt 1 ]; then
|
lib_with_script_dir=$(find "$TOOLKIT_PATH/lib" -name "*.sh" -type f -exec grep -l "^SCRIPT_DIR=" {} \; 2>/dev/null)
|
||||||
files=$(find "$TOOLKIT_PATH" -name "*.sh" -type f -exec grep -l "^SCRIPT_DIR=" {} \; 2>/dev/null | tr '\n' ' ')
|
if [ -n "$lib_with_script_dir" ]; then
|
||||||
echo "HIGH|Multiple files|N/A|SCRIPT_DIR in $script_dir_count files: $files"
|
while read -r file; do
|
||||||
count_issue "HIGH"
|
line_num=$(grep -n "^SCRIPT_DIR=" "$file" | head -1 | cut -d: -f1)
|
||||||
|
echo "MEDIUM|$file|$line_num|Library file defines SCRIPT_DIR (should be in caller)"
|
||||||
|
count_issue "MEDIUM"
|
||||||
|
done <<< "$lib_with_script_dir"
|
||||||
fi
|
fi
|
||||||
|
|
||||||
echo ""
|
echo ""
|
||||||
@@ -2920,14 +2924,15 @@ echo ""
|
|||||||
} >> "$REPORT"
|
} >> "$REPORT"
|
||||||
|
|
||||||
#==============================================================================
|
#==============================================================================
|
||||||
# CHECK 83: Direct /var/cpanel/users access (CRITICAL)
|
# CHECK 83: Direct /var/cpanel/users access (MEDIUM - prefer abstractions)
|
||||||
#==============================================================================
|
#==============================================================================
|
||||||
echo "[83/94] Checking: Direct /var/cpanel/users access..."
|
echo "[83/94] Checking: Direct /var/cpanel/users access..."
|
||||||
{
|
{
|
||||||
echo "## CHECK 83: Direct /var/cpanel/users file access"
|
echo "## CHECK 83: Direct /var/cpanel/users file access"
|
||||||
echo "Severity: CRITICAL"
|
echo "Severity: MEDIUM"
|
||||||
echo "Pattern: grep/cat /var/cpanel/users or /etc/userdatadomains"
|
echo "Pattern: grep/cat /var/cpanel/users or /etc/userdatadomains"
|
||||||
echo "Fix: Use get_user_info() or get_user_domains() from user-manager.sh"
|
echo "Fix: Use get_user_info() or get_user_domains() from user-manager.sh"
|
||||||
|
echo "Note: Diagnostic/analysis tools may legitimately need direct access"
|
||||||
echo ""
|
echo ""
|
||||||
|
|
||||||
count=0
|
count=0
|
||||||
@@ -2938,13 +2943,19 @@ while IFS=: read -r file line_num line_content; do
|
|||||||
# Skip library files that implement the abstraction
|
# Skip library files that implement the abstraction
|
||||||
[[ "$file" =~ (user-manager|system-detect|cpanel-helpers)\.sh$ ]] && continue
|
[[ "$file" =~ (user-manager|system-detect|cpanel-helpers)\.sh$ ]] && continue
|
||||||
|
|
||||||
echo "CRITICAL|$file|$line_num|[USERDATA-ACCESS] Direct access to cPanel user files"
|
# Skip QA script itself (it checks for this pattern)
|
||||||
count_issue "CRITICAL"
|
[[ "$file" =~ toolkit-qa-check\.sh$ ]] && continue
|
||||||
|
|
||||||
|
# Skip diagnostic/analysis tools that need direct access
|
||||||
|
[[ "$file" =~ (malware-scanner|website-error-analyzer|500-error-tracker|analyzer|diagnostics)\.sh$ ]] && continue
|
||||||
|
|
||||||
|
echo "MEDIUM|$file|$line_num|[USERDATA-ACCESS] Consider using user-manager.sh functions"
|
||||||
|
count_issue "MEDIUM"
|
||||||
((count++))
|
((count++))
|
||||||
[ "$count" -ge 10 ] && break
|
[ "$count" -ge 10 ] && break
|
||||||
done < <(grep -rnE '(grep|cat|awk).*(/var/cpanel/users/|/etc/userdatadomains)' "$TOOLKIT_PATH" --include="*.sh" 2>/dev/null)
|
done < <(grep -rnE '(grep|cat|awk).*(/var/cpanel/users/|/etc/userdatadomains)' "$TOOLKIT_PATH" --include="*.sh" 2>/dev/null)
|
||||||
|
|
||||||
echo "Found: $count direct userdata file accesses"
|
echo "Found: $count direct userdata file accesses (diagnostic tools excluded)"
|
||||||
echo ""
|
echo ""
|
||||||
} >> "$REPORT"
|
} >> "$REPORT"
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user