# 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](https://github.com/sebiboga/testlink-upgraded/issues/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](Bugfix-Issue-1731-ReqMgrSystemEdit-Shim-Array-Request-E_WARNING.md).
**Sibling filed while testing:** [#1892](https://github.com/sebiboga/testlink-upgraded/issues/1892) — the *silent* `intval()` coercion (`?tproject_id[]=7` → 1), a different defect in the same file, out of this issue's scope; and [#1893](https://github.com/sebiboga/testlink-upgraded/issues/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:

```php
$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):

```php
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) |

![Legacy array-shaped feature link now lands on the modern Test Specification](issue-1732-array-feature-redirects-to-testSpec.png)

## 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](https://github.com/sebiboga/testlink-upgraded/issues/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](https://github.com/sebiboga/testlink-upgraded/issues/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](https://github.com/sebiboga/testlink-upgraded/issues/1892).