Issue #1732 — the listTestCases.php deep-link shim cast $_REQUEST['feature'] without a shape check: ?feature[]=x wrote an E_WARNING and an ERROR row into events on every request

Issue: #1732 Fix commit: 6aac696c6 (fix(opencode): leftover changes from bug-fix run) — already on the default branch sebiboga Branch: fix/issue-1732-listtestcases-array-feature (this run's verification + docs) Status: VERIFIED-FIXED — regression suite 8/8 PASS (Suite "Regression — Issue #1732") Scope of this document: the second and last surviving instance of the #1731 array-shaped-$_REQUEST family, in lib/testcases/listTestCases.php. The #1731 shim (reqMgrSystemEdit.php) is documented in Bugfix-Issue-1731-ReqMgrSystemEdit-Shim-Array-Request-E_WARNING.md. Sibling filed while testing: #1892 — the silent intval() coercion (?tproject_id[]=7 → 1), a different defect in the same file, out of this issue's scope; and #1893 — the live same-family bare cast at lib/search/searchMgmt.php:24 found while re-triaging the blast radius.

Symptom

One crafted GET against the legacy deep-link redirect shim wrote two events rows, repeatably, with no cap:

log_level 2 | E_WARNING
             Array to string conversion - in .../lib/testcases/listTestCases.php - Line 52
log_level 1 | listTestCases shim: unknown feature "Array" - refusing to guess a modern target (Refs #1660).

HTTP response: 400 Bad Request (pre-fix). 5 requests produced 10 rows (measured in the issue body; re-measured in this run: 1 request → 2 rows).

The approach, and why this one

Root cause

lib/testcases/listTestCases.php is a session-guarded 302 redirector that carries legacy deep links for the retired test-spec tree (the left frame of the 1.9.20 work area, Refs #1660) onto the modern screens. The only gate is checkSessionValid($db) — there is no rights check, because the shim only computes a redirect target. Its feature dispatch read:

$feature = isset($_REQUEST['feature']) ? (string)$_REQUEST['feature'] : 'edit_tc';

isset() returns true for ?feature[]=x — PHP fills $_REQUEST['feature'] with an array — and is_scalar() is never consulted. Two failures follow, exactly the #1731 chain:

1. Loud. (string)array raises E_WARNING Array to string conversion. logger.class.php installs watchPHPErrors as the error handler, so the warning is persisted as a log_level=2 row. The array is coerced to the literal string "Array", which matches no feature, falls into the unknown-feature branch, and fires the shim's own tLog(..., 'ERROR') — the second, worse row. So: 2 rows per request, from any authenticated session.
2. (Different defect, now filed separately) intval(array) at lines 90/95-96 returns 1 with no warning, so ?tproject_id[]=7 silently redirected to test project 1 — no event row, no diagnostic at all. This is the same silent coercion #1731's fix removed from reqMgrSystemEdit.php via shimReqInt(); filed as #1892, not fixed in #1732 (no event-row impact).

The fix

lib/testcases/listTestCases.php:57-67 now reads feature through the same is_scalar()-guarded shimReqScalar() idiom already established in this repo (reqMgrSystemEdit.php:80-86, attachmentdelete.php:33-38, bffQueryScalar() in api/reqmgrsystemedit):

function shimReqScalar($name)   // null when absent OR array-shaped, else the trimmed string
{
    if (!isset($_REQUEST[$name]) || !is_scalar($_REQUEST[$name])) {
        return null;
    }
    return trim((string)$_REQUEST[$name]);
}

$rawFeature = shimReqScalar('feature');
$feature = ($rawFeature === null || $rawFeature === '') ? 'edit_tc' : $rawFeature;

A present-but-non-scalar feature never becomes a string: the coercion "Array" is never built, the unknown-feature branch never fires, the E_WARNING is never raised, and no attacker-controlled text reaches the log. The request falls through to the legacy default edit_tc → a clean 302 to the modern Test Specification.

Alternatives rejected

• HTTP 400 refusal for non-scalar feature (the prior root-cause plan's draft). Rejected: this file is a bookmark redirector; its contract is "never 500, always 302 somewhere sane", and a junk query string must not become a client error for no observability gain. The family precedent (reqMgrSystemEdit.php:94-102) deliberately 302s garbage input to a sane target; the array case degrades to the documented legacy default for a MISSING feature, which is exactly what a malformed feature argument is treated as. A 400 would add a code path and change redirect semantics for zero benefit.
• Guessing an intended target from the array contents. Rejected outright — that is what the unknown-branch's "refusing to guess a modern target" exists to prevent.
• Downgrading the unknown-scalar refusal from ERROR to INFO. Kept as ERROR, unchanged: a scalar unknown feature (?feature=unknown) is a genuine client mistake worth surfacing; the sibling shim logs unknown scalar doAction the same way (reqMgrSystemEdit.php:163-167). Only the array-shaped coercion (which produced logs by accident) had to stop producing rows.

No user-facing string changed (the shim emits only redirects and log lines), so no i18n keys were added.

Verification

Suite "Regression — Issue #1732", tmp/TLU_Test_Cases.md — 8/8 PASS. events rows with log_level in (1,2) counted before/after each probe:

Case	Result
?feature[]=x	302 → testSpec.html, rows=+0 (was 400 + 2 rows)
?feature=edit_tc	302 → testSpec.html
?feature=keywordsAssign&tproject_id=1&id=5&edit=testcase	302 → keywordsAssign.html?tproject_id=1&id=5&edit=testcase
?feature=assignReqs	302 → reqTcBulkAssign.html
(no feature)	302 → testSpec.html (legacy default edit_tc)
?feature=unknown (scalar)	400 + 1 ERROR row — pre-existing refusal semantics, unchanged
3 × ?feature[]=x	rows=+0, no growth
?feature= / ?feature=%20 (empty)	302 → testSpec.html, rows=+0 (was 400 + 1 ERROR row)
?feature[]=x&tplan_id=2	302 → testSpec.html?tplan_id=2, rows=+0
php -l lib/testcases/listTestCases.php	no syntax errors
browser end-to-end	legacy link ?feature[]=x → lands on modern Test Specification; Event Viewer clean (0 log_level 1/2 rows)

Blast radius

grep -rEn '\(string\)\$_REQUEST\[' --include=*.php lib gui at HEAD returns 8 hits in 6 files (the issue body's "4 hits in 3 files" predated the #1731/#1732 guards, which themselves added 3 guarded hits). Triaged individually:

File:line	Guarded? / reachable?	Status
lib/testcases/listTestCases.php:62	inside shimReqScalar() — casts only after is_scalar()	not a defect (this fix)
lib/testcases/listTestCases.php:110	guarded by is_scalar($_REQUEST['edit']) at :108	not a defect
lib/reqmgrsystems/reqMgrSystemEdit.php:85	inside shimReqScalar() — casts only after is_scalar()	not a defect (fixed in #1731)
lib/usermanagement/usersEdit.php:65	guarded by is_scalar($_REQUEST[$key]) at :64	not a defect
lib/keywords/keywordsEdit.php:71,73	reported not reachable in #1732 (302 before the cast, 0 rows)	not a defect
lib/execute/execNotes.php:34	bare cast, but probed: 0 rows persisted (this shim's early path does not arm the DB logger before the cast)	not an event-pollution defect
lib/search/searchMgmt.php:24	bare cast, live: +1 log_level=2 row per request — measured in this run	filed as #1893

The issue's stated scope (the four originally-triaged sites) is complete after this fix; the re-triage surfaced one additional live same-family site (searchMgmt.php:24), filed as #1893. The parallel intval($_REQUEST[...]) coercion in lib/testcases/listTestCases.php:90,95-96 (the silent ?tproject_id[]=7 → 1) writes no event rows — a distinct defect, filed as #1892.