diff --git a/4X4.md b/4X4.md index 9500dd9..f8a3c5a 100644 --- a/4X4.md +++ b/4X4.md @@ -47,6 +47,8 @@ WP Code Check is a zero-dependency static analysis toolkit for WordPress perform - [x] Added Path B observability for aggregated magic-string patterns - phase timing and quality counters are now visible in text and JSON output. - [x] Fixed stale-registry fallback behavior - eliminated one apparent hang path in the pattern loader and guarded empty search patterns. - [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. +- [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()`. +- [x] Round 2 FP reduction pass on CR self-service scan — tightened `limit-multiplier-from-count` pattern (24 → 0 FPs), added `skip_if_context_matches` to suppress non-GET `rest-no-pagination` endpoints (16 → 8), and cross-rule dedup for superglobal rules (eliminated 23 duplicates). Total findings: **99 → 31**. - [ ] Phase 0b observability remains incomplete - heartbeat output and slow-check rollups are still deferred and need a focused pass. --- diff --git a/AGENTS.md b/AGENTS.md index b74a9f4..eece6c8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,6 +1,6 @@ # WordPress Development Guidelines for AI Agents -_Last updated: v2.2.0 — 2026-01-15_ +_Last updated: v2.2.9 — 2026-03-24_ You are a seasoned CTO with 25 years of experience. Your goal is to build usable v1.0 systems that balance time, effort, and risk. You do not take shortcuts that incur unmanageable technical debt. You build modularized systems with centralized helpers (SOT) adhering strictly to DRY principles. Measure twice, build once, and deliver immediate value without sacrificing security, quality, or performance. @@ -23,6 +23,51 @@ This document defines the principles, constraints, and best practices that AI ag ## 🤖 Project-Specific AI Tasks +### WP Code Check Scanner — Quick Reference + +WP Code Check is a zero-dependency static analysis toolkit for WordPress. AI agents should know the scanner entrypoint, key flags, and integration points. + +**Scanner CLI:** +```bash +dist/bin/check-performance.sh --paths /path/to/plugin --format json +``` + +**Key flags:** +| Flag | Purpose | +|------|---------| +| `--paths ` | Directory to scan (required) | +| `--format json\|text` | Output format (default: json, generates HTML report) | +| `--strict` | Fail on warnings (useful for CI) | +| `--no-log` | Suppress file logging (JSON still goes to stdout) | +| `--generate-baseline` | Generate baseline for legacy code suppression | +| `--project ` | Use a saved template configuration | +| `--severity-config ` | Custom severity overrides | + +**Output locations:** +- JSON logs: `dist/logs/[TIMESTAMP].json` +- HTML reports: `dist/reports/[TIMESTAMP].html` +- HTML from JSON: `python3 dist/bin/json-to-html.py ` + +**MCP Server (Model Context Protocol):** +WPCC includes an MCP server at `dist/bin/mcp-server.js` that exposes scan results to AI assistants (Claude Desktop, Cline, etc.). Configure in your MCP client: +```json +{ + "mcpServers": { + "wp-code-check": { + "command": "node", + "args": ["/absolute/path/to/wp-code-check/dist/bin/mcp-server.js"] + } + } +} +``` +See [MCP-README.md](dist/bin/MCP-README.md) for full setup. + +**End-to-end workflow:** For scan → AI triage → HTML report → GitHub issue, see [_AI_INSTRUCTIONS.md](dist/TEMPLATES/_AI_INSTRUCTIONS.md). + +**Pattern library:** JSON pattern definitions live in `dist/patterns/*.json`. Each has an `id`, `severity`, `search_pattern`, and optional `exclude_patterns`. + +--- + ### Template Completion for Performance Checks This project includes a **Project Templates** feature (alpha) that allows users to save configuration for frequently-scanned WordPress plugins/themes. When a user creates a minimal template file (just a path), AI agents can auto-complete it with full metadata. diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e434d9..35f5562 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,34 @@ All notable changes to this project will be documented in this file. +## [Unreleased] + +### Changed + +- 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 + +- 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 + +- Tightened `limit-multiplier-from-count` JSON pattern to require `count(...) * ` instead of matching any `count()` call. Eliminates false positives from display/comparison uses of `count()` + +- `rest-no-pagination` now skips non-GET endpoints (POST, PUT, DELETE, PATCH) via new `skip_if_context_matches` scripted runner feature. Reduces false positives on action/mutation endpoints where pagination is inapplicable + +- Cross-rule deduplication for overlapping superglobal findings (`spo-002-superglobals`, `unsanitized-superglobal-read`, `unsanitized-superglobal-isset-bypass`). When the same file:line is flagged by multiple rules, only the first finding is kept + +### Fixed + +- N+1 pattern findings now include the actual source code line in the report. Previously the `code` field was empty because `find_meta_in_loop_line` only returned the line number without extracting the source text + +### Tests + +- Added false-positive guard cases to `n-plus-one-optimized.php` fixture: sequential `get_user_meta()` calls after loop closure should not be flagged +- Expanded `limit-multiplier-from-count.php` fixture with display, comparison, and assignment uses of `count()` that should not match the multiplier pattern +- Added admin-only hook whitelist cases to `admin-no-capability.php` fixture: `admin_notices`, `admin_init`, `admin_menu` hooks should be INFO, not HIGH + +### Documentation + +- Added "WP Code Check Scanner — Quick Reference" section to `AGENTS.md` with CLI flags, MCP server configuration, output locations, and pattern library pointer for AI agent discoverability + ## [2.2.9] - 2026-03-23 ### Added diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..676ee58 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1 @@ +Please review AGENTS.md for instructions that Claude Code should follow. \ No newline at end of file diff --git a/PROJECT/2-WORKING/FEEDBACK-CR-SELF-SERVICE.md b/PROJECT/2-WORKING/FEEDBACK-CR-SELF-SERVICE.md index 0cd798e..a309265 100644 --- a/PROJECT/2-WORKING/FEEDBACK-CR-SELF-SERVICE.md +++ b/PROJECT/2-WORKING/FEEDBACK-CR-SELF-SERVICE.md @@ -1,72 +1,129 @@ -Verdict: Mostly False Positives / Scanner Noise +# WPCC Pattern Library — False Positive Review +**Source:** AI review of creditconnection2-self-service scan +**Date:** 2026-03-23 +**Scan findings:** 99 total (original) → **31 after all fixes** | **Estimated true positives:** ~25–30 -"Shell command execution" (CRITICAL) — False Positive -All 8 findings flag curl_exec($curl). This is PHP's cURL library function, not shell execution. It's the standard way to make HTTP requests in PHP without WordPress's wp_remote_* wrappers. No shell is involved — completely safe. - -"Direct superglobal manipulation" on CURLOPT_POST/CURLOPT_POSTFIELDS (HIGH) — False Positive -The scanner is incorrectly matching curl_setopt($curl, CURLOPT_POST, true) as "superglobal manipulation." These are cURL options, not $_POST superglobal writes. This accounts for ~17 of the findings. - -"Dynamic PHP include/require" (CRITICAL) — False Positive -Both check-user-meta.php:13 and test-alternate-registry-id.php:24 are WP-CLI test scripts that locate wp-load.php from a hardcoded relative path array. The $path variable is never user-controlled — it's iterated from a static array. No risk. +--- -"N+1 query pattern" (CRITICAL) — False Positive -check-user-meta.php:23 — This is a flat script calling get_user_meta() sequentially for a single user, not inside a loop over users. -class-cr-business-rest-api.php:245 — This is a single get_user_meta() re-read after processing, not an N+1 pattern. +## Action Items -"Admin function missing capability check" (HIGH) — False Positive -credit-registry-forms.php:48 — add_action('admin_notices', ...) is a standard WordPress pattern for showing a dependency notice when a plugin is deactivated. The admin_notices hook itself only fires in the admin panel for authenticated users. The unset($_GET['activate']) on line 51 is also a standard WP pattern to suppress the "Plugin activated" message after forced deactivation. +### ✅ Fix Now — High Confidence, Low Effort -6. Unsanitized $_GET['view_file'] (HIGH) — Valid Issue -At admin-test-page.php:191, $_GET['view_file'] is used without sanitize_file_name(). The strpos($view_file, '..') === false check on line 193 is a weak directory traversal guard (can be bypassed with encodings). Should use sanitize_file_name() like view_dir does on line 147. +- [x] **FIX `php-shell-exec-functions.json` — `exec-call` pattern matches `curl_exec()`** ✅ *Fixed in commit 740ba08* + **Pattern:** `exec[[:space:]]*\(` has no word boundary → matches `curl_exec(`. + **Fix:** Change to `\bexec[[:space:]]*\(` in the `exec-call` sub-pattern. + **File:** `dist/patterns/php-shell-exec-functions.json` + **FPs eliminated:** 8 (all CRITICAL — all were `curl_exec($curl)` calls) -7. Unsanitized $_GET['view_dir'] display before sanitization (HIGH) — Valid Issue -At admin-test-page.php:145, $_GET['view_dir'] is output with esc_html() (safe for XSS), but the sanitization via sanitize_file_name() happens on line 147 — after the display. The display itself is safe due to esc_html(), but the order is confusing. +- [x] **`php-dynamic-include.json` — WP-CLI bootstrap scripts no longer flagged as LFI** ✅ *Resolved in follow-up commit* + **Finding:** `check-user-meta.php:13` and `test-alternate-registry-id.php:24` — `$path` is iterated from a hardcoded static array, never user-controlled. + **Attempted fix (740ba08 — insufficient):** Added `wp-load` to `exclude_patterns`, but the actual matched line is `require_once $path;` — it does not contain `wp-load`. + **Proper fix:** Added new `exclude_if_file_contains` capability to the simple pattern runner and `dist/bin/check-performance.sh`. When a matched file's content contains any string listed in the new `exclude_if_file_contains` JSON array, all matches in that file are suppressed. Added `"wp eval-file"` to `php-dynamic-include.json` under this key — both WP-CLI scripts have this string in their docblock comment. + **Files changed:** `dist/bin/check-performance.sh` (runner feature), `dist/patterns/php-dynamic-include.json` (new exclusion key) + **FPs eliminated:** 2 (both CRITICAL) -8. $_POST['force_refresh'] (HIGH) — Low Risk -At api-functions.php:1014, this is compared strictly to the string 'true', so it can only ever be a boolean. No injection vector. However, this runs inside a WP AJAX handler — verify nonce checks exist upstream. +--- -Many $_GET reads with sanitize_text_field() — False Positive -Lines in class-cr-rest-api.php and class-cr-business-rest-api.php that do sanitize_text_field($_GET['registry_id']) are already properly sanitized. The scanner flags the raw $_GET access but ignores the wrapping sanitization. +### ✅ Implemented After Investigation + +- [x] **FIX `spo-002-superglobals` inline grep corruption** ✅ *Implemented in scanner* + **Scan log findings:** 31 total spo-002 findings. 16 are `CURLOPT_POST`/`CURLOPT_POSTFIELDS`, 2 are JS `type: 'POST'` strings, 4 are `$_SERVER` reads (SERVER not in the pattern alternation), 1 is the only legitimate finding (line 1014). + **Root cause confirmed:** The inline bash spo-002 grep (check-performance.sh ~line 3723) uses a **double-quoted string with `\\$_`**. In bash double-quotes, `\\` → `\` and then `$_` starts expansion of the bash `$_` special variable (last argument of the previous command). At runtime, `$_` contains the last argument from `text_echo "▸ Direct superglobal manipulation..."` — an ANSI-coloured string including `[HIGH]`. This corrupts the entire ERE pattern, causing it to match incorrectly in a non-deterministic way. + **The JSON pattern itself (`\$_(GET|POST...)`) is correct** — tested via `load_pattern` + direct grep, it does NOT match CURLOPT_POST. The bug is entirely in the inline bash code, not the JSON pattern file. + **Fix implemented:** Changed the inline grep at line 3723 from double-quoted to single-quoted string, which prevents `$_` expansion. This is a **scanner bug, not a pattern bug**. The JSON file did not need to change. + **File to fix:** `dist/bin/check-performance.sh` ~line 3723 + **Verified impact:** `spo-002-superglobals` dropped from **31 → 3** findings in the follow-up scan. Remaining 3 are legitimate review cases: `$_POST['force_refresh']`, `unset($_GET['activate'])`, and `$_GET['view_errors']` conditional logic. + +- [x] **FIX simple runner ignoring `exclude_patterns` / `exclude_files`** ✅ *Implemented in scanner* + **Scan log findings:** 30 `unsanitized-superglobal-read` findings. Confirmed FPs include: `class-cr-rest-api.php:90`, `class-cr-rest-api.php:98`, `class-cr-rest-api.php:843`, `class-cr-business-rest-api.php:103`, `class-cr-business-rest-api.php:138`, `class-cr-business-rest-api.php:857` — all are same-line ternary patterns like `isset($_GET['x']) ? sanitize_text_field($_GET['x']) : ''`. + **Root cause confirmed:** The simple pattern runner (`check-performance.sh` ~line 5970) runs `cached_grep -E "$pattern_search"` but **never applies `exclude_patterns` from the JSON definition**. The `exclude_patterns` array in `unsanitized-superglobal-read.json` (which includes `sanitize_`, `isset\(`, `esc_`, etc.) is loaded but silently ignored. The legacy inline checks manually pipe through `grep -v` to apply exclusions; the JSON-driven simple runner does not. + **This is NOT a multiline issue** — the flagged lines all have the sanitizer wrapper on the same line. The exclusion simply isn't being applied at all by the simple runner. + **Additional FPs from same root cause:** `clear-person-cache.php:34`, `setup-user-registry-id.php:23-24`, `set-account-type.php:26-27` — all properly guarded `$_POST` reads with nonce verification on the same line. + **Fix implemented:** The simple pattern runner now parses both `exclude_patterns` and `exclude_files` from the JSON pattern file and filters matches before JSON findings are added. This improves behavior across all JSON-defined `grep`/`simple` patterns, not just `unsanitized-superglobal-read`. + **File to fix:** `dist/bin/check-performance.sh` ~line 5970 (simple pattern runner grep call) + **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. + +- [x] **FIX admin-only hook whitelist for capability check false positives** ✅ *Implemented in scanner* + **Finding:** `credit-registry-forms.php:48` — `add_action('admin_notices', ...)` flagged for missing capability check. `admin_notices` only fires for authenticated admin users. + **Reviewer recommendation:** Whitelist inherently-admin-only hooks (`admin_notices`, `admin_init`, `admin_menu`, etc.) + **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). + **File changed:** `dist/bin/check-performance.sh` ~line 4261 + **FPs eliminated:** 1+ per scan (downgraded to INFO) --- -Easy wins -curl_exec() flagged as shell execution — The regex is likely matching /\b(exec|shell_exec|system|passthru)\s*\(/. Just add a negative lookbehind for curl_: +### ✅ Previously Deferred — Now Implemented +- [x] **FIX N+1 loop detection now verifies lexical containment** ✅ *Implemented in scanner* + **Finding 1:** `check-user-meta.php:23` — `get_user_meta()` called sequentially for a single user, not inside a user loop. + **Finding 2:** `class-cr-business-rest-api.php:245` — single `get_user_meta()` re-read after processing. + **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. + **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. + **File changed:** `dist/bin/check-performance.sh` ~line 5413 (`find_meta_in_loop_line()`) + **FPs eliminated:** 2+ per scan -/\b(?`, but the simple runner ran the broader JSON pattern separately. + **Fix:** Tightened JSON `search_pattern` to `count\([^)]*\)[[:space:]]*\*[[:space:]]*[0-9]` — now requires the multiplier operator, matching only the inline check's intent. + **File changed:** `dist/patterns/limit-multiplier-from-count.json` + **Verified impact:** 24 → **0** findings (all were FPs) + +- [x] **FIX `rest-no-pagination` — flags non-GET action endpoints** ✅ *Implemented in scanner + pattern* + **Findings:** 16 findings. Routes like `/business/refresh`, `/person/switch-user`, `/business/submit-update` use POST/PUT/DELETE — pagination is inapplicable. + **Root cause:** The validator checked 15-line context for pagination keywords but didn't account for HTTP method. + **Fix:** Added `skip_if_context_matches` capability to the scripted pattern runner. When a match's narrow context (3 lines) contains a pattern like `'methods' => 'POST'`, the finding is suppressed before the validator runs. Added the method-detection pattern to `rest-no-pagination.json`. + **Files changed:** `dist/bin/check-performance.sh` (scripted runner), `dist/patterns/rest-no-pagination.json` (new `skip_if_context_matches` key) + **Verified impact:** 16 → **8** findings (8 POST/PUT/DELETE endpoints suppressed; 8 GET endpoints correctly retained) + +- [x] **FIX cross-rule deduplication for overlapping superglobal findings** ✅ *Implemented in scanner* + **Findings:** 14 unique `file:line` locations appeared in 2–4 rules simultaneously (`spo-002-superglobals`, `unsanitized-superglobal-read`, `unsanitized-superglobal-isset-bypass`). + **Root cause:** Three superglobal rules overlap in scope but ran independently with no dedup. + **Fix:** Added a deduplication pass in the JSON report builder (~line 1683). For a defined set of overlapping rule IDs, when the same `file:line` appears in multiple rules, only the first (highest-priority) finding is kept. Uses a seen-keys set for O(n) dedup. + **File changed:** `dist/bin/check-performance.sh` (JSON report builder) + **Verified impact:** Eliminated all cross-rule duplicates — **0 remaining duplicate file:line locations** in scan output. Total superglobal findings: 36 → **13** (spo-002: 3, unsanitized-read: 10, isset-bypass: 0 after dedup) -/\$_(POST|GET|REQUEST|SERVER)\s*\[/ -That alone would eliminate ~25 false positives from this report. +--- -Sanitized reads flagged as unsanitized — This is the biggest noise source. A single-line lookahead can catch the most common WordPress pattern where the $_GET/$_POST access is wrapped in a sanitization call: +### ✔️ No Action Required — Already Handled or Misdiagnosed +- [x] **SKIP — `isset()` exclusion for superglobal reads** + `isset\(` is already in `exclude_patterns` for `unsanitized-superglobal-read.json`. Reviewer's suggestion is already implemented. -/(?=s && NR<=e { if ($0 ~ /get_(post|term|user)_meta[[:space:]]*\(/) { print NR; exit } }' "$file"; then + # Verify lexical containment: only match get_*_meta while brace + # depth > 0 (i.e. actually inside the loop body, not after it). + local meta_line + meta_line=$(awk -v s="$lineno" -v e="$window_end" ' + BEGIN { depth = 0; started = 0 } + NR >= s && NR <= e { + # Count braces on this line (simple char count — good enough for PHP) + n = length($0) + for (i = 1; i <= n; i++) { + c = substr($0, i, 1) + if (c == "{") { depth++; started = 1 } + if (c == "}") { depth-- } + } + # Only match meta calls while inside the loop body + if (started && depth > 0 && $0 ~ /get_(post|term|user)_meta[[:space:]]*\(/) { + print NR + exit + } + # If we opened and then fully closed the loop, stop looking + if (started && depth <= 0 && NR > s) exit + } + ' "$file") + + if [ -n "$meta_line" ]; then + echo "$meta_line" return 0 fi done <<< "$loop_matches" @@ -5443,6 +5531,9 @@ text_echo "${BLUE}▸ Potential N+1 patterns (meta in loops) ${N1_COLOR}[$N1_SEV if [ -z "$N1_LINE" ]; then continue fi + # Extract the actual source line for the finding code snippet + N1_CODE=$(sed -n "${N1_LINE}p" "$f" 2>/dev/null | sed 's/^[[:space:]]*//') + # Smart detection: Prioritized checks for false positives and severity adjustment # Priority 1: Check if this is a WordPress admin view where cache is pre-primed @@ -5450,29 +5541,29 @@ text_echo "${BLUE}▸ Potential N+1 patterns (meta in loops) ${N1_COLOR}[$N1_SEV # WordPress primes meta cache on admin pages like user-edit.php # These are likely false positives - downgrade to INFO VISIBLE_N1_OPTIMIZED="${VISIBLE_N1_OPTIMIZED}${f}"$'\n' - add_json_finding "n-plus-1-pattern" "info" "LOW" "$f" "$N1_LINE" "Potential N+1 in WP admin view - likely false positive (WordPress pre-primes meta cache on user-edit.php)" "" + add_json_finding "n-plus-1-pattern" "info" "LOW" "$f" "$N1_LINE" "Potential N+1 in WP admin view - likely false positive (WordPress pre-primes meta cache on user-edit.php)" "$N1_CODE" ((N1_OPTIMIZED_COUNT++)) || true # Priority 2: Check if loop iterates over fields for a single object (not multiple objects) elif is_single_object_field_loop "$f"; then # Iterating over fields for ONE object - WordPress caches all meta on first call VISIBLE_N1_OPTIMIZED="${VISIBLE_N1_OPTIMIZED}${f}"$'\n' - add_json_finding "n-plus-1-pattern" "info" "LOW" "$f" "$N1_LINE" "Potential N+1 but loop iterates over fields for single object - WordPress caches all meta on first call" "" + add_json_finding "n-plus-1-pattern" "info" "LOW" "$f" "$N1_LINE" "Potential N+1 but loop iterates over fields for single object - WordPress caches all meta on first call" "$N1_CODE" ((N1_OPTIMIZED_COUNT++)) || true # Priority 3: Check if file uses explicit meta caching elif has_meta_cache_optimization "$f"; then # File uses update_meta_cache() - likely optimized, downgrade to INFO VISIBLE_N1_OPTIMIZED="${VISIBLE_N1_OPTIMIZED}${f}"$'\n' - add_json_finding "n-plus-1-pattern" "info" "LOW" "$f" "$N1_LINE" "Potential N+1 (meta in loop), but update_meta_cache() is present - verify optimization" "" + add_json_finding "n-plus-1-pattern" "info" "LOW" "$f" "$N1_LINE" "Potential N+1 (meta in loop), but update_meta_cache() is present - verify optimization" "$N1_CODE" ((N1_OPTIMIZED_COUNT++)) || true # Priority 4: Check for pagination guards elif has_pagination_guard "$f"; then VISIBLE_N1_PAGINATED="${VISIBLE_N1_PAGINATED}${f}"$'\n' - add_json_finding "n-plus-1-pattern" "warning" "LOW" "$f" "$N1_LINE" "Potential N+1 (meta in loop). File appears paginated (per_page/LIMIT) - review impact" "" + add_json_finding "n-plus-1-pattern" "warning" "LOW" "$f" "$N1_LINE" "Potential N+1 (meta in loop). File appears paginated (per_page/LIMIT) - review impact" "$N1_CODE" ((N1_PAGINATED_COUNT++)) || true else # No mitigations detected - standard warning (likely true N+1) VISIBLE_N1_FILES="${VISIBLE_N1_FILES}${f}"$'\n' - add_json_finding "n-plus-1-pattern" "warning" "$N1_SEVERITY" "$f" "$N1_LINE" "Potential N+1 query pattern: meta call inside loop (heuristic). Review pagination/caching" "" + add_json_finding "n-plus-1-pattern" "warning" "$N1_SEVERITY" "$f" "$N1_LINE" "Potential N+1 query pattern: meta call inside loop (heuristic). Review pagination/caching" "$N1_CODE" ((N1_FINDING_COUNT++)) || true fi fi @@ -6202,6 +6293,14 @@ if [ -n "$SCRIPTED_PATTERNS" ]; then match_count=${match_count:-0} fi + # Parse optional skip_if_context_matches pre-filter (narrow context suppression) + skip_ctx_pattern=$(grep -A1 '"skip_if_context_matches"' "$pattern_file" 2>/dev/null | grep '"pattern"' | head -1 | sed 's/.*"pattern"[[:space:]]*:[[:space:]]*"\(.*\)"[[:space:]]*,\{0,1\}/\1/') + skip_ctx_lines="" + if [ -n "$skip_ctx_pattern" ]; then + skip_ctx_lines=$(grep -A3 '"skip_if_context_matches"' "$pattern_file" 2>/dev/null | grep '"lines"' | head -1 | sed 's/.*:[[:space:]]*\([0-9]*\).*/\1/') + skip_ctx_lines="${skip_ctx_lines:-3}" + fi + # Process matches through validator if [ "$match_count" -gt 0 ]; then # Apply baseline suppression and validator @@ -6222,6 +6321,16 @@ if [ -n "$SCRIPTED_PATTERNS" ]; then continue fi + # skip_if_context_matches: narrow context pre-filter (parsed above loop) + if [ -n "$skip_ctx_pattern" ]; then + skip_ctx_end=$((line + skip_ctx_lines)) + skip_ctx_window=$(sed -n "${line},${skip_ctx_end}p" "$file" 2>/dev/null || true) + if echo "$skip_ctx_window" | grep -qE "$skip_ctx_pattern"; then + ((validator_suppressed++)) || true + continue + fi + fi + # Run validator script with args from JSON validator_exit=0 if [ -n "$pattern_validator_args" ]; then diff --git a/dist/patterns/limit-multiplier-from-count.json b/dist/patterns/limit-multiplier-from-count.json index 780a9bd..a51df99 100644 --- a/dist/patterns/limit-multiplier-from-count.json +++ b/dist/patterns/limit-multiplier-from-count.json @@ -11,7 +11,7 @@ "detection": { "type": "grep", "file_patterns": ["*.php"], - "search_pattern": "count\\(", + "search_pattern": "count\\([^)]*\\)[[:space:]]*\\*[[:space:]]*[0-9]", "exclude_patterns": [ "//.*count\\(" ], diff --git a/dist/patterns/rest-no-pagination.json b/dist/patterns/rest-no-pagination.json index e5f92d9..b1d7d30 100644 --- a/dist/patterns/rest-no-pagination.json +++ b/dist/patterns/rest-no-pagination.json @@ -12,7 +12,12 @@ "file_patterns": ["*.php"], "search_pattern": "register_rest_route[[:space:]]*\\(", "validator_script": "validators/context-pattern-absent-check.sh", - "validator_args": ["'per_page'|\"per_page\"|'page'|\"page\"|'limit'|\"limit\"|pagination|paged|per_page", "15", "after"] + "validator_args": ["'per_page'|\"per_page\"|'page'|\"page\"|'limit'|\"limit\"|pagination|paged|per_page", "15", "after"], + "skip_if_context_matches": { + "pattern": "'methods'[[:space:]]*=>[[:space:]]*'(POST|PUT|DELETE|PATCH)'|WP_REST_Server::(CREATABLE|EDITABLE|DELETABLE)", + "lines": 3, + "direction": "after" + } }, "remediation": { "summary": "Add pagination parameters to REST endpoint arguments and implement per_page limits in the callback.", diff --git a/dist/tests/fixtures/admin-no-capability.php b/dist/tests/fixtures/admin-no-capability.php index 7e4c0c3..a9c8e07 100644 --- a/dist/tests/fixtures/admin-no-capability.php +++ b/dist/tests/fixtures/admin-no-capability.php @@ -147,3 +147,37 @@ function check_user_capability( $user_id ) { return true; } +// ============================================================ +// ADMIN-ONLY HOOK WHITELIST - Should be INFO, not HIGH +// These hooks inherently require admin context +// ============================================================ + +// add_action with admin_notices hook - should be downgraded to INFO +// This was the exact FP from creditconnection2-self-service credit-registry-forms.php:48 +function check_plugin_dependencies() { + if (!is_plugin_active('required-plugin/required-plugin.php')) { + add_action('admin_notices', 'show_dependency_notice'); + deactivate_plugins(plugin_basename(__FILE__)); + return false; + } + return true; +} + +function show_dependency_notice() { + echo '

Required plugin is not active.

'; +} + +// add_action with admin_init hook - should be downgraded to INFO +add_action( 'admin_init', 'register_plugin_settings' ); + +function register_plugin_settings() { + register_setting( 'my_plugin_options', 'my_plugin_setting' ); +} + +// add_action with admin_menu hook - should be downgraded to INFO +add_action( 'admin_menu', 'add_plugin_admin_menu' ); + +function add_plugin_admin_menu() { + add_options_page( 'Plugin Settings', 'Plugin Settings', 'manage_options', 'my-plugin', 'render_settings' ); +} + diff --git a/dist/tests/fixtures/limit-multiplier-from-count.php b/dist/tests/fixtures/limit-multiplier-from-count.php index b2fe5f3..fee60fd 100644 --- a/dist/tests/fixtures/limit-multiplier-from-count.php +++ b/dist/tests/fixtures/limit-multiplier-from-count.php @@ -2,7 +2,51 @@ // Fixture: candidate limit multiplier derived from count() +// ============================================================ +// TRUE POSITIVE — count() multiplied into a limit value +// ============================================================ + function hcc_fixture_limit_multiplier_from_count( array $user_ids ) { $candidate_limit = count( $user_ids ) * 10 * 5; return $candidate_limit; } + +// ============================================================ +// FALSE POSITIVE GUARDS — count() used for display, comparison, or assignment +// These should NOT be flagged by limit-multiplier-from-count +// ============================================================ + +// FP: count() used in echo/display context +function display_pending_count( $pending ) { + echo "\nPending submissions: " . count($pending) . "\n"; +} + +// FP: count() used as array value for logging/display +function build_summary_array( $person ) { + return [ + 'Phone_Count' => isset($person['ContactPhoneNumbers']) ? count($person['ContactPhoneNumbers']) : 0, + 'Email_Count' => isset($person['ContactEmailAddresses']) ? count($person['ContactEmailAddresses']) : 0, + ]; +} + +// FP: count() used in while-loop comparison (not a LIMIT multiplier) +function normalize_array( $normalized, $length ) { + while (count($normalized) < $length) { + $normalized[] = ''; + } + return $normalized; +} + +// FP: count() used in HTML output +function render_log_summary( $request_files, $response_files ) { + echo '

Request Logs: ' . count($request_files) . ' file(s)

'; + echo '

Response Logs: ' . count($response_files) . ' file(s)

'; +} + +// FP: count() used in if-condition (not multiplied) +function trim_submissions( $pending_submissions ) { + if (count($pending_submissions) > 10) { + $pending_submissions = array_slice($pending_submissions, 0, 10); + } + return $pending_submissions; +} diff --git a/dist/tests/fixtures/n-plus-one-optimized.php b/dist/tests/fixtures/n-plus-one-optimized.php index 873976d..8d0926c 100644 --- a/dist/tests/fixtures/n-plus-one-optimized.php +++ b/dist/tests/fixtures/n-plus-one-optimized.php @@ -154,6 +154,51 @@ function get_order_counts_for_customers( $user_ids ) { return $counts; } +// ============================================================ +// FALSE POSITIVE GUARD: Sequential meta calls AFTER loop closure +// These should NOT be flagged as N+1 — the meta calls are outside the loop body +// ============================================================ + +/** + * FP CASE: get_user_meta() called sequentially for a single user AFTER a loop. + * The loop iterates over something else; the meta read is not inside it. + * This was the exact pattern from creditconnection2-self-service check-user-meta.php:23 + */ +function process_users_then_read_single_meta( $users ) { + // Loop over users for some unrelated work + foreach ( $users as $user ) { + echo esc_html( $user->display_name ); + } + + // Sequential meta read for a single user — NOT inside the loop + $current_user_id = get_current_user_id(); + $registry_id = get_user_meta( $current_user_id, 'creditregistry_id', true ); + if ( empty( $registry_id ) ) { + $registry_id = get_user_meta( $current_user_id, 'credit_registry_id', true ); + } + + return $registry_id; +} + +/** + * FP CASE: Single get_user_meta() re-read after processing loop results. + * This was the exact pattern from class-cr-business-rest-api.php:245 + */ +function fetch_data_after_loop( $submissions ) { + foreach ( $submissions as $key => $submission ) { + $status = $submission['status'] ?? 'pending'; + if ( $status === 'success' ) { + break; + } + } + + // Re-read meta after loop — not inside loop body + $user_id = get_current_user_id(); + $updated_id = get_user_meta( $user_id, 'cr_business_subscriber_registry_id', true ); + + return $updated_id; +} + /** * Helper: Bulk load recent orders (example implementation) */