# Bugfix — Issue #1696: the legacy requirement tree loader leaked every project's requirement doc_ids

> **Issue:** [#1696](https://github.com/sebiboga/testlink-upgraded/issues/1696)
> **Area:** Requirement Specification Tree — legacy lazy loader
> **Class:** broken access control / IDOR
> **Status:** fixed (`18c9680c2` on `fix/issue-1696`), regression suite 10/10 PASS
> **Impact:** every logged-in user — a global `<no rights>` role included — could read the
> requirement specification `doc_id`s, the requirement `req_doc_id`s, the titles and the shape of
> the requirement branch of **any** test project, and walk the whole `nodes_hierarchy`.

This is the follow-up to
[Requirement Specification Tree (modernized, Refs #1695)](Requirement-Specification-Tree-Modernized.md).
That page flagged the loader as an open hole; this page records the fix.

---

## Symptom

`lib/ajax/getrequirementnodes.php` returned the requirement tree of **any** node to **any**
authenticated user. It performed no rights check whatsoever — only a session check — and resolved
whatever node id the caller passed.

## Environment

* App `http://localhost:8082` (PHP built-in server, docroot = repo root), repo at `ba3f3ed75`.
* MariaDB `127.0.0.1:3306`, database `testlink`.
* **The freshly imported database is EMPTY** — `testprojects`, `req_specs`, `requirements` and
  `user_testproject_roles` all hold 0 rows. There is no "project B" to leak until you build one.
  Worth knowing: this class of bug is *invisible* right after an import. Fixtures used here:

```sql
INSERT INTO nodes_hierarchy (id,name,parent_id,node_type_id,node_order) VALUES
 (1,'Project B (SECRET)',NULL,1,1),      -- node_type_id 1 = testproject
 (2,'Secret Spec B',1,6,1),              -- 6 = requirement_spec
 (3,'Secret Requirement B1',2,7,1),      -- 7 = requirement
 (10,'Project A',NULL,1,2), (11,'Spec A1',10,6,1);
INSERT INTO req_specs (id,testproject_id,doc_id) VALUES (2,1,'SPEC-B-001'),(11,10,'SPEC-A-001');
INSERT INTO requirements (id,srs_id,req_doc_id) VALUES (3,2,'REQ-B-001');
-- testprojects.options must carry the serialized blob, otherwise the screen reads
-- "Requirements disabled" (parseTestProjectRecordset(), testproject.class.php:240-255):
--   O:8:"stdClass":4:{s:19:"requirementsEnabled";i:1;s:19:"testPriorityEnabled";i:1;
--                     s:17:"automationEnabled";i:1;s:16:"inventoryEnabled";i:0;}
INSERT INTO users (login,password,role_id,email,first,last,locale,active,cookie_string,auth_method)
 VALUES ('lowpriv','<bcrypt of lowpriv123>',3,'l@e.com','Low','Priv','en_GB',1,'ck-lowpriv','');
-- role 3 = '<no rights>';  user_testproject_roles left EMPTY on purpose
```

Log in the way the browser does (`login.php` posts to the BFF; the POST needs the same-origin
proof enforced by `api/_guard.php:31-40`):

```bash
curl -s -c c1696.txt -X POST http://localhost:8082/api/auth/login \
     -H 'Origin: http://localhost:8082' -d login=lowpriv -d password=lowpriv123
# -> {"status":"ok","success":true,…}
```

## Root cause — the `file:line` chain

| hop | location (pre-fix) | what happens |
|---|---|---|
| 1 | `lib/ajax/getrequirementnodes.php:20-22` | `testlinkInitPage($db)` — the **one-argument** form: boots the page, checks the SESSION only. `grep -c "hasRight\|checkRights"` on the file → **0**. |
| 2 | `:25-27` | `root_node` / `node` / `filter_node` are read straight out of the request. Nothing proves `$node` is a requirement node, and nothing ties `$node` back to `$root_node`. |
| 3 | `:33` | `display_children()` runs **before** any decision about the caller's rights. |
| 4 | `:34` | `echo json_encode($nodes)` — the payload leaves the process. |
| 5 | `display_children()` SQL | `… WHERE NHA.parent_id = intval($parent)` **only**. Any `nodes_hierarchy.id` is a valid parent — no project scope, no node-type gate on the requested node. |
| 6 | `case 'requirement_spec'` / `case 'requirement'` | the label is built as `"{$doc_id}:{$name}"` from `req_specs.doc_id` / `requirements.req_doc_id` and the whole row is handed to ExtJS — **the doc_id, the identifier that keys the requirement tracker, is disclosed before the name.** |

### Why it became reachable

1.9.20 only ever reached the file through the `reqSpecListTree` frame, and that frame had already
done its role check — so the loader's own missing check was masked *in normal use*. The endpoint
was nevertheless directly reachable all along and has authorized nothing since 1.9.20: this is
legacy code, not a regression of this cycle. **Refs #1695** removed even the indirect mask by
turning `lib/requirements/reqSpecListTree.php` itself into a 302 shim to the modern screen, which
is what made the hole obvious enough to report.

## Measured reproduction (pre-fix)

Attacker: role `3` = `<no rights>`, **empty `user_testproject_roles`** — a pure authenticated
IDOR, not a misconfigured grant.

```
$ curl -s -b c1696.txt '…/lib/ajax/getrequirementnodes.php?mode=reqspec&root_node=1'
[{"text":"SPEC-B-001:Secret Spec B (1)","id":"2",…,"href":"javascript:REQ_SPEC_MGMT(1,2)"}]   HTTP 200

$ curl -s -b c1696.txt '…/lib/ajax/getrequirementnodes.php?mode=reqspec&root_node=1&node=2'
[{"text":"REQ-B-001:Secret Requirement B1","id":"3",…,"href":"javascript:REQ_MGMT(1,3)"}]       HTTP 200

$ for n in 1 2 3 5 10 11; do curl -s "…/getrequirementnodes.php?node=$n"; done
node=1   -> SPEC-B-001:Secret Spec B (1)     href="javascript:REQ_SPEC_MGMT(,2)"
node=2   -> REQ-B-001:Secret Requirement B1  href="javascript:REQ_MGMT(,3)"
node=10  -> SPEC-A-001:Spec A1 (0)           href="javascript:REQ_SPEC_MGMT(,11)"
node=3 / node=5 / node=11 -> null
```

Two extras beyond the original report:

* `root_node` is optional and becomes an **empty string** in the emitted href — with only
  `?node=<id>` the JSON contains `REQ_SPEC_MGMT(,2)`: a node addressed with no project at all.
* **No node-type gate on the requested node**, only on the returned children
  (`NT.description NOT IN ('testcase','testsuite',…)`) — so any `nodes_hierarchy.id` is accepted
  as a parent and the endpoint doubles as a generic hierarchy probe.

Layer by layer:

| layer | measurement | verdict |
|---|---|---|
| session auth | anonymous GET → `top.location.href='../../login.php?note=expired&…'` | **works** — IDOR, not unauthenticated |
| **rights** | no `hasRight`, no role check, no project-scope check | **the bug** |
| modern replacement | `api/reqspectreelist/index.php` → `403 no_right` / `404`, rights checked **before** resolution (`:154-167`) | **properly guarded** |
| Event Viewer | `SELECT COUNT(*) FROM events` → **0** | the disclosure left **no trace at all** |

## Blast radius

```
$ grep -rn getrequirementnodes --include='*.php' --include='*.tpl' --include='*.html' \
      --include='*.js' --include='*.md' . | grep -v '^./tmp/' | wc -l
47      # 18 of those are in code (below), the rest are comments / docs / CHANGELOG history

$ grep -rn getrequirementnodes --include='*.php' --include='*.tpl' --include='*.html' \
      --include='*.js' . | grep -v '^./tmp/' | grep -v '^./lib/ajax/getrequirementnodes.php' \
      | cut -d: -f1 | sort | uniq -c | sort -rn
      4 ./api/reqspectreelist/index.php               (comments)
      2 ./gui/templates/requirements/reqSpecMgmt.html (comments)
      2 ./api/reqspec/index.php                      (comments)
      1 ./lib/functions/tlRequirementFilterControl.class.php  <-- the ONLY URL constructor
      1 ./lib/requirements/reqSpecListTree.php        (comment)
      1 ./lib/functions/common.php                   (comment)
      1 ./lib/ajax/requirements/getreqmonitors.php   (comment)
      1 ./lib/ajax/gettprojectnodes.php              (comment)
      1 ./lib/ajax/gettestcasesummary.php            (comment)
      1 ./lib/ajax/getreqcoveragenodes.php           (comment)
      1 ./api/tcprojecttree/index.php                (comment)
      1 ./api/requirements/index.php                 (comment)

$ grep -rn 'new tlRequirementFilterControl' --include='*.php' . | wc -l
0       # the class is never instantiated -> tlRequirementFilterControl.class.php:272 is DEAD code
```

* **Reachable by:** every authenticated user, any role, any project.
* **Data at risk:** `req_specs.doc_id` + titles, `requirements.req_doc_id` + titles, the
  requirement-branch shape of every project, plus a generic `nodes_hierarchy.id` probe.
* **Not at risk:** anything without a session (measured), and nothing else — the endpoint is
  strictly read-only.
* **Live callers that must keep working:** none. The modern tree is
  `api/reqspectreelist/index.php`, which never calls this file.
* **Siblings already fixed, same bug class, untouched here:** `lib/ajax/gettprojectnodes.php:31`
  (#1770) and `lib/ajax/getreqcoveragenodes.php:31` (#1765). `lib/ajax/gettestcasesummary.php:27`
  and `lib/ajax/requirements/getreqmonitors.php:14` only *mention* #1696 in a comment.

## The fix

`lib/ajax/getrequirementnodes.php` is **retired in place as a non-mutating 302 shim** — the exact
shape its two already-fixed siblings use (`gettprojectnodes.php:57-118`):

1. **Preserve** the legacy `testlinkInitPage()` session contract via `doDBConnect()` +
   `checkSessionValid($db)` — an anonymous visitor is still bounced to
   `login.php?note=expired&destination=…`. `checkSessionValid()` rather than a hand-rolled
   `header()`, because it walks up from `dirname(SCRIPT_FILENAME)` until it finds `login.php`; a
   relative `'login.php'` would resolve against `/lib/ajax/` and 404.
2. **Refuse every non-`GET`/`HEAD` verb** with `405` + JSON `{"code":"method_not_allowed", …}`
   and a `tLog(…, 'WARNING')` row, so the retirement is visible in the Event Viewer instead of a
   silent behaviour change.
3. **Delete `display_children()` and all of its SQL** — that function *was* the leak.
4. `302` a legacy `GET` to `gui/templates/requirements/reqSpecListTree.html`, carrying
   `tproject_id` from **`tproject_id` or the legacy `root_node`** (first one > 0 wins) plus
   `filter_node` when present, so an old bookmark — or the dead
   `tlRequirementFilterControl.class.php:272` URL — still lands on a working screen. `filter_node`
   is forwarded purely so such a bookmark survives intact: the modern requirement tree does **not**
   read it (`grep -ri filter_node` over `gui/templates/requirements/reqSpecListTree.html` and
   `api/reqspectreelist/` → 0 hits), unlike its test-case sibling
   `gui/templates/testcases/tcProjectTree.html:119`. The `$_SESSION['basehref']` base is honoured
   (as the *frame* shim `lib/requirements/reqSpecListTree.php:36-38` does), so a sub-directory
   installation is not redirected off the document root; the two loader siblings hardcode `/gui/…`
   instead.
5. The read is deliberately **NOT replayed**. The modern screen and its BFF decide what to
   reveal, per request, with a real rights check.

### Alternatives considered and rejected

| alternative | why rejected |
|---|---|
| **Delete the file outright** (the issue's first suggestion) | `lib/functions/tlRequirementFilterControl.class.php:272` still builds this URL into its loader. That call path is dead (`new tlRequirementFilterControl` → 0 hits), but deleting would turn it into a **404** — strictly larger and less friendly than the convention #1770/#1765 already established (kept the file, redirected it). |
| **Add the two rights checks to the existing loader** | Sound security-wise, but it leaves a legacy ExtJS surface alive and answering, reachable from a dead call path, with SQL driven by request ids. Retiring it removes the class of bug instead of guarding it, and the modern BFF (`api/reqspectreelist/index.php:154-196`) already implements both checks *plus* the node-type and project-scope proofs the loader lacked. |
| **Also strip the dead loader string from `tlRequirementFilterControl::build_tree_menu()`** | Unrelated cleanup. With the file still present and now redirecting, the dead URL resolves; there is no functional need to touch the class. |

## Verification (post-fix, `18c9680c2`) — 10/10 PASS

| # | step | result |
|---|---|---|
| R1 | anonymous `GET ?mode=reqspec&root_node=1` | HTTP 200, body has `login.php?note=expired` — **unchanged** |
| R2 | `<no rights>` `GET ?mode=reqspec&root_node=1` | HTTP 302 → `…/reqSpecListTree.html?tproject_id=1`; `grep -c 'SPEC-B-001\|REQ-B-001\|Secret'` → **0** |
| R3 | `<no rights>` `GET ?node=2` | HTTP 302 → `…/reqSpecListTree.html`, no project param; leak check → **0** |
| R4 | `POST` / `PUT` / `DELETE` | HTTP 405 + `method_not_allowed` for all three, no data |
| R5 | `admin` `GET ?mode=reqspec&root_node=1&filter_node=2` | HTTP 302 → `…?tproject_id=1&filter_node=2` — `filter_node` survives the redirect (the modern screen ignores it, see §6.4) |
| R6 | `admin` `GET /api/reqspectreelist/index.php?action=init&tproject_id=1` | HTTP 200 `{"status":"ok",…,"grant":{"view":true,"modify":true}}` — modern tree unaffected |
| R7 | `<no rights>` same endpoint | HTTP 403 `no_right` — **unchanged** |
| R8 | `GET /lib/requirements/reqSpecListTree.php?tproject_id=1` | HTTP 302 → `…/reqSpecListTree.html?tproject_id=1` — **unchanged** |
| R9 | `php -l lib/ajax/getrequirementnodes.php` | `No syntax errors detected` |
| R10 | `events` table | 3 rows at `log_level` 2 (WARNING) = exactly the R4 refusals; **no Error row** |

Browser pass (Chrome, headless, as `admin`): navigating the legacy URL
`http://localhost:8082/lib/ajax/getrequirementnodes.php?mode=reqspec&root_node=1` lands on
`gui/templates/requirements/reqSpecListTree.html?tproject_id=1`, the tree renders
(`1 specifications, 1 requirements`, state card *Requirements enabled*, project switcher
populated), expanding the project node shows `Project B (SECRET)` with its requirement count, and
**the console is clean — zero errors and zero warnings**.

Regression suite: `tmp/TLU_Test_Cases.md`, heading
`## Regression — Issue #1696: legacy requirement tree loader lib/ajax/getrequirementnodes.php retired (IDOR)`
— 10 cases, appended with `>>`, gate
`TLU_REQUIRE_SUITE="Issue #1696" bash ai/verify_test_suites.sh` → `7 PASS / 0 FAIL / 0 SKIP`.

## Code review (mandatory, subagent)

**No BLOCKER.** The fix itself was judged correct and live-verified: anon GET → `200` +
`top.location.href='../../login.php?note=expired&…'`, auth GET → `302`, `?node=2` → `302` with
no payload, `POST/PUT/DELETE/PATCH/OPTIONS` → `405`, `HEAD` → `302`, no CRLF injection, no PHP
notices, `php -l` clean on 8.3, `events` gained only the intended WARNING rows.

All 5 SHOULD-FIX and the factual NITs were fixed in commit `40008af91`:

| finding | fix |
|---|---|
| the `filter_node` comment claimed it was "a real gesture on the modern screen too" — **false**, the requirement tree never reads it | comment corrected: forwarded for bookmark fidelity only, with the `tcProjectTree.html:119` contrast |
| the pre-fix `file:line` citations were off by 1–3 lines | corrected to `:20-22`, `:25-27`, `:33`, `:34` against `git show origin/sebiboga:lib/ajax/getrequirementnodes.php` |
| the blast-radius greps were not reproducible (47, not 25; `new tlRequirementFilterControl` → 0 only with `--include='*.php'`, since the bare grep self-matches this doc) | both quoted in their reproducible form |
| "the last mask was removed" overstated when the exposure began — the endpoint was always directly reachable | reworded here, in the CHANGELOG and in the `docs/` mirror |
| "as on every sibling shim" for `basehref` — the two loader siblings hardcode `/gui/…`; it matches the *frame* shim | corrected |
| the docblock documented only `mode=reqspec` / `operation=manage` | now documents the full legacy surface: `mode=addtc`, `operation=print`, and the href the `$fn[operation][mode]` table chose per combination |
| `docs/Requirement-Specification-Tree-Modernized.md` still said "still unprotected" / "stays open", diverging from this wiki page | mirrored (rule 6) |
| `docs/MODERNIZATION-STATUS.md` and the `$actions->reqSpecListTree` comment in `lib/functions/common.php` did not record the retirement | both updated |

NITs consciously **not** actioned, with reasons: the open-redirect surface via
`$_SESSION['basehref']` (derived from `HTTP_HOST` at login, `configCheck.php:71-82`) is
**pre-existing and identical to the frame shim's**, not introduced here — the two loader siblings
are immune only because they hardcode the path; `?root_node[]=7` coerces to `tproject_id=1` with
no PHP warning and no data impact (the frame shim is stricter with `is_scalar()`, the two loader
siblings are not); the `405` omits an `Allow: GET, HEAD` header, as do both siblings; the 405 JSON
message is untranslated, as on every legacy ajax endpoint (#1770/#1765 likewise — no legacy ajax
file has ever used `lang_get()`), and **no i18n bundle was touched**, so no JSON validation was
needed.

## Screenshot

Navigating the **legacy loader URL** in Chrome now lands on the modern Requirement Specification
Tree — the redirect target, with the fixture tree rendered:

![legacy loader URL redirects to the modern requirement specification tree](1696-legacy-loader-redirects-to-modern-tree.png)

## Files changed

| file | purpose |
|---|---|
| `lib/ajax/getrequirementnodes.php` | rewritten as the session-guarded 302 shim; `display_children()` and its SQL deleted |
| `CHANGELOG` | `[KEY BUGFIX] (security) - #1696` entry under 2.0.1 |
| `docs/Bugfix-Issue-1696-Requirement-Tree-Loader-Unauthorized-IDOR.md` | the `docs/` mirror (no image lines) |
| `tmp/TLU_Test_Cases.md` | the 10-case regression suite (append-only, shared file) |

## Re-testing in one pass

```bash
php -l lib/ajax/getrequirementnodes.php
curl -s -c /tmp/c -X POST http://localhost:8082/api/auth/login \
     -H 'Origin: http://localhost:8082' -d login=admin -d password=admin -o /dev/null
curl -s -o /dev/null -w '%{http_code} %{redirect_url}\n' -b /tmp/c \
     'http://localhost:8082/lib/ajax/getrequirementnodes.php?mode=reqspec&root_node=1'
# expect: 302 http://localhost:8082/gui/templates/requirements/reqSpecListTree.html?tproject_id=1
curl -s -o /dev/null -w '%{http_code}\n' -b /tmp/c -X POST \
     'http://localhost:8082/lib/ajax/getrequirementnodes.php?root_node=1'
# expect: 405
grep -rn "display_children" lib/ajax/getrequirementnodes.php   # expect: no output
```

## Related

- [Requirement Specification Tree (modernized, Refs #1695)](Requirement-Specification-Tree-Modernized.md)
  — the BFF that replaces the loader, and the "Known limitations" bullet this fix closes.
- [Requirement Specification Management (#1026)](Requirement-Specification-Tree-reqSpecMgmt.md) —
  the legacy `req_specs.doc_id` / `child_requirements_mgmt` semantics preserved by the shim.
- The two sibling loaders of this same bug class, retired before this one:
  `lib/ajax/gettprojectnodes.php` (Refs #1770) and `lib/ajax/getreqcoveragenodes.php` (Refs #1765).