Skip to content

Commit e04239c

Browse files
noelsaw1claude
andcommitted
Resolve deferred FP items: admin-only hook whitelist and N+1 loop containment
- spo-004: Add admin-only hook whitelist to downgrade inherently-admin hooks (admin_notices, admin_init, admin_menu, etc.) to INFO severity instead of flagging as missing capability checks - N+1: Replace line-range heuristic in find_meta_in_loop_line() with brace-depth tracking so get_*_meta calls after a loop's closing brace are no longer false-positived as N+1 patterns - Update FEEDBACK-CR-SELF-SERVICE.md, CHANGELOG.md, and 4X4.md Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent b990bd8 commit e04239c

File tree

4 files changed

+69
-16
lines changed

4 files changed

+69
-16
lines changed

4X4.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ WP Code Check is a zero-dependency static analysis toolkit for WordPress perform
4747
- [x] Added Path B observability for aggregated magic-string patterns - phase timing and quality counters are now visible in text and JSON output.
4848
- [x] Fixed stale-registry fallback behavior - eliminated one apparent hang path in the pattern loader and guarded empty search patterns.
4949
- [x] Fixed high-noise direct-pattern false positives - reduced `php-shell-exec-functions`, `spo-002-superglobals`, and `php-dynamic-include` noise with targeted scanner and pattern fixes.
50+
- [x] Cleared all deferred items from CR self-service feedback review — added admin-only hook whitelist for `spo-004` (downgrade to INFO) and strengthened N+1 loop detection with brace-depth lexical containment in `find_meta_in_loop_line()`.
5051
- [ ] Phase 0b observability remains incomplete - heartbeat output and slow-check rollups are still deferred and need a focused pass.
5152

5253
---

CHANGELOG.md

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,14 @@
22

33
All notable changes to this project will be documented in this file.
44

5+
## [Unreleased]
6+
7+
### Changed
8+
9+
- Admin-only hook whitelist for `spo-004-missing-cap-check`: `add_action()` calls using inherently-admin-only hooks (`admin_notices`, `admin_init`, `admin_menu`, `admin_head`, `admin_footer`, `admin_enqueue_scripts`, `admin_print_styles`, `admin_print_scripts`, `network_admin_menu`, `user_admin_menu`, `network_admin_notices`, `admin_bar_init`, `admin_action_*`, `load-*`) are now downgraded to INFO severity instead of HIGH, reducing false positives for capability check findings
10+
11+
- N+1 loop detection (`find_meta_in_loop_line`) now uses brace-depth tracking to verify `get_*_meta` calls are lexically inside a loop body, not just within 80 lines of a loop keyword. Eliminates false positives from sequential meta calls after loop closure
12+
513
## [2.2.9] - 2026-03-23
614

715
### Added

PROJECT/2-WORKING/FEEDBACK-CR-SELF-SERVICE.md

Lines changed: 17 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -43,22 +43,24 @@
4343
**File to fix:** `dist/bin/check-performance.sh` ~line 5970 (simple pattern runner grep call)
4444
**Verified impact:** `unsanitized-superglobal-read` dropped from **30 → 19** findings in the follow-up scan. The remaining 19 are mostly other classes of reads that still require separate tuning, especially the dedicated `unsanitized-superglobal-isset-bypass` rule.
4545

46-
---
46+
- [x] **FIX admin-only hook whitelist for capability check false positives***Implemented in scanner*
47+
**Finding:** `credit-registry-forms.php:48``add_action('admin_notices', ...)` flagged for missing capability check. `admin_notices` only fires for authenticated admin users.
48+
**Reviewer recommendation:** Whitelist inherently-admin-only hooks (`admin_notices`, `admin_init`, `admin_menu`, etc.)
49+
**Fix implemented:** Added admin-only hook whitelist in the `spo-004-missing-cap-check` section of `check-performance.sh` (~line 4261). When `add_action()` uses a whitelisted hook, the finding is still recorded but downgraded to INFO severity instead of HIGH. Whitelisted hooks: `admin_notices`, `admin_init`, `admin_menu`, `admin_head`, `admin_footer`, `admin_enqueue_scripts`, `admin_print_styles`, `admin_print_scripts`, `network_admin_menu`, `user_admin_menu`, `network_admin_notices`, `admin_bar_init`, `admin_action_*` (glob), `load-*` (glob).
50+
**File changed:** `dist/bin/check-performance.sh` ~line 4261
51+
**FPs eliminated:** 1+ per scan (downgraded to INFO)
4752

48-
### 📋 Deferred — Investigate Further Before Implementing
53+
---
4954

50-
- [ ] **DEFERRED: Add admin-only hook whitelist for capability check false positives**
51-
**Finding:** `credit-registry-forms.php:48``add_action('admin_notices', ...)` flagged for missing capability check. `admin_notices` only fires for authenticated admin users.
52-
**Reviewer recommendation:** Whitelist inherently-admin-only hooks (`admin_notices`, `admin_init`, `admin_menu`, etc.)
53-
**Our assessment:** Correct diagnosis. Not fixable with regex alone — requires a hook whitelist in the scanner logic. Downgrade severity to INFO as interim.
54-
**Effort:** Low–Medium | **FPs eliminated:** 1 per occurrence
55+
### ✅ Previously Deferred — Now Implemented
5556

56-
- [ ] **DEFERRED: Strengthen N+1 loop detection to verify lexical containment**
57-
**Finding 1:** `check-user-meta.php:23``get_user_meta()` called sequentially for a single user, not inside a user loop.
58-
**Finding 2:** `class-cr-business-rest-api.php:245` — single `get_user_meta()` re-read after processing.
59-
**Reviewer recommendation:** Confirm the meta call is lexically inside a loop body (`{...}`), not just nearby by line count.
60-
**Our assessment:** The scanner has `is_iterating_over_multiple_objects()` heuristics. These may be gaps in that logic. Review and tighten the "loop containment" check.
61-
**Effort:** Medium | **FPs eliminated:** 2
57+
- [x] **FIX N+1 loop detection now verifies lexical containment***Implemented in scanner*
58+
**Finding 1:** `check-user-meta.php:23``get_user_meta()` called sequentially for a single user, not inside a user loop.
59+
**Finding 2:** `class-cr-business-rest-api.php:245` — single `get_user_meta()` re-read after processing.
60+
**Root cause:** `find_meta_in_loop_line()` used a simple line-range check (loop line + 80 lines forward) without verifying the meta call was lexically inside the loop's braces. Sequential `get_*_meta()` calls after a loop's closing `}` were incorrectly flagged.
61+
**Fix implemented:** Replaced the line-range `awk` in `find_meta_in_loop_line()` with brace-depth tracking. The new `awk` counts `{` and `}` from the loop line forward, only matching `get_(post|term|user)_meta` while depth > 0. Once the loop body closes (depth returns to 0), scanning stops — meta calls after the loop are no longer flagged.
62+
**File changed:** `dist/bin/check-performance.sh` ~line 5413 (`find_meta_in_loop_line()`)
63+
**FPs eliminated:** 2+ per scan
6264

6365
---
6466

@@ -93,7 +95,7 @@
9395
| `exclude_if_file_contains` + `wp eval-file` | `check-performance.sh` + `php-dynamic-include.json` | Medium | 2 verified | ✅ Done |
9496
| Single-quote inline spo-002 grep | `check-performance.sh` ~L3723 | 1 line | 28 verified | ✅ Done |
9597
| Apply `exclude_patterns` in simple runner | `check-performance.sh` ~L5970 | Medium | 11 verified | ✅ Done |
96-
| Admin-only hook whitelist | `check-performance.sh` | Medium | 1+ per scan | 📋 Deferred |
97-
| N+1 loop containment tightening | `check-performance.sh` | Medium | 2+ per scan | 📋 Deferred |
98+
| Admin-only hook whitelist | `check-performance.sh` ~L4261 | Low | 1+ per scan (→ INFO) | ✅ Done |
99+
| N+1 loop containment (brace-depth) | `check-performance.sh` ~L5413 | Medium | 2+ per scan | ✅ Done |
98100

99101
**Latest measured totals:** 99 findings before scanner fixes → **88 findings after first round****86 findings after dynamic-include fix**.

dist/bin/check-performance.sh

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4258,6 +4258,24 @@ if [ -n "$ADMIN_MATCHES" ]; then
42584258
continue
42594259
fi
42604260

4261+
# Admin-only hook whitelist: these hooks only fire for authenticated admin
4262+
# users, so a missing capability check is informational, not a real risk.
4263+
# Extract the hook name from add_action('hook_name', ...) patterns.
4264+
if echo "$code" | grep -qE "add_action[[:space:]]*\\("; then
4265+
hook_name=$(echo "$code" | sed -n "s/.*add_action[[:space:]]*([[:space:]]*['\"]\\([^'\"]*\\)['\"].*/\\1/p" | head -1)
4266+
case "$hook_name" in
4267+
admin_notices|admin_init|admin_menu|admin_head|admin_footer| \
4268+
admin_enqueue_scripts|admin_print_styles|admin_print_scripts| \
4269+
network_admin_menu|user_admin_menu|network_admin_notices| \
4270+
admin_bar_init|admin_action_*|load-*)
4271+
# Downgrade to INFO — the hook itself implies admin context
4272+
ADMIN_SEEN_KEYS="${ADMIN_SEEN_KEYS}${key}"
4273+
group_and_add_finding "spo-004-missing-cap-check" "info" "INFO" "$file" "$lineno" "$code" "Admin-only hook '$hook_name' — implicit capability via hook context"
4274+
continue
4275+
;;
4276+
esac
4277+
fi
4278+
42614279
# Enhancement v1.0.93: Parse capability parameter from add_*_page() functions
42624280
# add_submenu_page() 4th parameter is capability
42634281
# add_menu_page() 4th parameter is capability
@@ -5409,7 +5427,31 @@ find_meta_in_loop_line() {
54095427
window_end="$scope_end"
54105428
fi
54115429

5412-
if awk -v s="$lineno" -v e="$window_end" 'NR>=s && NR<=e { if ($0 ~ /get_(post|term|user)_meta[[:space:]]*\(/) { print NR; exit } }' "$file"; then
5430+
# Verify lexical containment: only match get_*_meta while brace
5431+
# depth > 0 (i.e. actually inside the loop body, not after it).
5432+
local meta_line
5433+
meta_line=$(awk -v s="$lineno" -v e="$window_end" '
5434+
BEGIN { depth = 0; started = 0 }
5435+
NR >= s && NR <= e {
5436+
# Count braces on this line (simple char count — good enough for PHP)
5437+
n = length($0)
5438+
for (i = 1; i <= n; i++) {
5439+
c = substr($0, i, 1)
5440+
if (c == "{") { depth++; started = 1 }
5441+
if (c == "}") { depth-- }
5442+
}
5443+
# Only match meta calls while inside the loop body
5444+
if (started && depth > 0 && $0 ~ /get_(post|term|user)_meta[[:space:]]*\(/) {
5445+
print NR
5446+
exit
5447+
}
5448+
# If we opened and then fully closed the loop, stop looking
5449+
if (started && depth <= 0 && NR > s) exit
5450+
}
5451+
' "$file")
5452+
5453+
if [ -n "$meta_line" ]; then
5454+
echo "$meta_line"
54135455
return 0
54145456
fi
54155457
done <<< "$loop_matches"

0 commit comments

Comments
 (0)