# Assessment — `eval()` in Plugin Hook Dispatch (functions.php)

**Date:** 2026-06-05
**Finding under review:** "eval() of plugin-config on every request — functions.php:217-278"
**Actual code location:** `functions.php:3451-3523` (`PluginHooksPreScriptCheck` / `PluginHooksPostScriptCheck`); inline dispatch `functions.php:3398-3435`; callsites `index.php:380,428,573`.
**Verdict:** Finding is **directionally correct and worth fixing**, but the headline severity is **overstated for the current codebase**. The `eval`'d strings are *not* tenant- or request-derived today. The genuine issues are (1) a latent RCE-by-design that one schema change would arm, (2) an unrelated but more urgent **missing authorization gate** on the writer endpoint, and (3) the GET-broadening control-flow change. Details and amendments below.

---

## 1. What actually gets `eval`'d, and where it comes from

`eval()` runs three values: `$v2['eval']` (3474/3512) and `$v2['newcall']` (3467/3501). These live in `$_SESSION['companyplugins'][n]['HOOKS'][]`.

**Trust-boundary trace:**
- The plugins **table** (`evolution.plugins`) is loaded at login (`login2save.php:122-139`) and in `plugins/controller.php:46-66`. The loaded array contains **only** `id, nav, name, pages, php_path, js_path, is_active, authenticated`. **It contains no `HOOKS`, `eval`, or `newcall` keys.**
- The `HOOKS` array is populated **exclusively at runtime** by the plugin's own `addHooks()` (`plugins/shopify/plugin.php:215-242`), passing **hardcoded PHP string literals** as `newcall`/`eval` (e.g. `'$shopify -> jobSetOrderStatus($_REQUEST);'`).
- A repo-wide grep for `'HOOKS'`/`'eval'`/`'newcall'` finds **no** assignment sourced from the DB, `$_REQUEST`, or any tenant-writable store.

**Conclusion:** The eval payloads are **developer-authored code shipped in `plugin.php`**, equivalent to `include`. A tenant or non-admin user **cannot** currently influence them. So the "if config is tenant-influenceable → RCE" precondition **is not met today**.

> **Caveat (the real risk):** This safety is *incidental*, not enforced. There is no guard preventing a future change from loading `HOOKS`/`eval` from a DB column or admin form. The moment that happens, this becomes unauthenticated/low-priv RCE with zero additional code change. The pattern is a loaded gun with the safety off.

---

## 2. The genuinely urgent issue nearby: `plugins/plugin_ajax.php` has no auth gate

While tracing the writer, I found `plugins/plugin_ajax.php` (lines 1-120) `session_start()`s but performs **no `requireAuth()` / `administrator` check** before:
- `UPDATE plugins SET status=1/0` (activate/deactivate a plugin globally),
- `UPDATE plugins SET show_nav=…`,
- `UPDATE plugins SET allowed_companies=?` / `active_companies=?`.

These are parameterized (no SQLi), but **any** session — and possibly any unauthenticated request, since nothing exits on missing session — can toggle plugin activation for **any company by id**. This is the control that decides *which* `plugin.php` (and thus which `addHooks()`/`eval` set) loads for a tenant. It is a more direct and currently-reachable problem than the eval itself. **Treat as high priority.**

---

## 3. The GET broadening (control-flow change)

`PluginHooksPreScriptCheck` defaults `method` to `GET` (3460: `($v2['method'] ?? 'GET')`) and runs from `index.php:380` on page render. Previously hook dispatch was POST-only (the inline block at 3402 defaults to `POST`). Effect: a matching **GET** hook now fires mid-page and the inline dispatcher can `die;` (3434-3435). The shopify plugin **does** register GET pre/post hooks (`jobedit`, `pickingedit`, `dispatchsave.php`, etc. — all `'GET'`), so this path is live for plugin tenants. This is behaviour-changing and must be re-tested on both verbs (see methodology).

---

## 4. Confirmed defects in the dispatch code (regardless of severity framing)

- **`functions.php:3467` — `eval($v2['newcall'])`** in the *named-var* branch. `newcall` is meant to be a **method name** (`$_REQUEST['call']` was just set to it), not a statement. Eval'ing a bare identifier like `fetchInventoryRegistry` is at best a no-op/notice, at worst executes whatever string was supplied. This line looks like a copy-paste error — the pre-hook named-var branch should `include_once` the plugin (already done on 3466) and let the plugin's own `call` dispatch run, **not** `eval` the call name. **The post-hook equivalent (3501) has the same issue but is at least wrapped in try/catch.** Pre-hook (3467) is **unwrapped** → a throw aborts the whole page.
- **Asymmetric hardening:** post-hook evals are wrapped in `try/catch` + `error_log` (3500-3504, 3511-3515); the **pre-hook evals (3467, 3474) are not** — an exception there is fatal to the request.

---

## Amendments (recommended, in priority order)

### A. [HIGH / do first] Add an authorization gate to `plugins/plugin_ajax.php`
Fail closed before any `UPDATE plugins`. Require an authenticated administrator (plugin management is an admin operation):
```php
<?php
session_start();
include_once('../functions.php');
requireAuth();                                  // 403 + exit if not logged in
if (($_SESSION['administrator'] ?? 0) != 1) {   // plugin mgmt is admin-only
    http_response_code(403);
    exit('forbidden');
}
$action = $_REQUEST["action"] ?? '';
```
(Drop the per-branch `include_once('../functions.php')` once it's included at top.) **Re-confirm** with the team that plugin enable/disable is intended to be admin-only — if a lower tier manages plugins, gate on that permission instead, but it must not be "any session".

### B. [HIGH] Assert the eval source is code-only, and document the invariant
Because the safety is incidental, make it explicit and enforced:
1. Add a header comment block above `PluginHooksPreScriptCheck` stating: *the `eval`/`newcall` strings are trusted plugin code authored in `plugins/*/plugin.php::addHooks()` and MUST NEVER be sourced from the database, request, or any tenant-writable store.*
2. Add a defensive guard so a future DB-sourced value can't silently arm RCE — e.g. only eval hooks whose `'origin'` was stamped by `addHooks()`:
```php
// in addPreHook/addPostHook/addReplaceHook, stamp each hook:
'origin' => 'code',
// in PluginHooks*ScriptCheck, before any eval:
if (($v2['origin'] ?? '') !== 'code') continue;   // refuse non-code-authored hooks
```
This is cheap, preserves current behaviour, and turns "incidentally safe" into "fails closed if someone later wires DB config in."

### C. [MEDIUM] Replace `eval` with a dispatch table (the proper fix)
The eval'd strings are almost all of one shape: *"call method M on the shopify plugin with `$_REQUEST` (and sometimes `$allvars`)."* Encode that as data, not code:
- Change hook registration to store a **callable spec** instead of a code string:
  `['handler' => 'jobSetOrderStatus', 'args' => ['request','allvars']]`.
- In the dispatcher, resolve `$shopify->{$spec['handler']}(...)` from a **whitelist of allowed handler names** per plugin, passing the named args. No `eval`.
- The few hooks that mutate `$_REQUEST` before calling (e.g. `$_REQUEST["jobid"] = $_REQUEST["id"]`) become small declarative remaps or dedicated wrapper methods on the plugin.

This removes `eval` entirely and makes B unnecessary. It's more work and touches `addHooks()` in `plugin.php`, so stage it after A/B land.

### D. [MEDIUM] Fix the pre-hook named-var branch (`functions.php:3467`)
Remove the stray `eval($v2['newcall']);`. The `include_once` on 3466 already loads the plugin; the plugin dispatches on `$_REQUEST['call']` itself. If a statement genuinely needs to run, it belongs in `$v2['eval']` (the empty-var branch), not in `newcall`. Until C lands, also wrap the **pre-hook** evals (3467→removed, 3474) in the same `try/catch + error_log` the post-hook branch uses, so a faulty hook logs instead of white-screening the page.

### E. [LOW] Justify / scope the GET broadening
Confirm the GET pre-check is intentional (the shopify GET hooks rely on it). If only specific pages need it, gate the GET path to the registered hook `script` set rather than running the full loop on every GET render. Document the decision inline at `index.php:380`.

---

## Implemented 2026-06-05 (A + B)

**A — auth gate on `plugins/plugin_ajax.php`:** added a fail-closed gate at the top of the file
(`include_once('../functions.php'); requireAuth(); if administrator != 1 → 403 + exit`) *before* any
`$action` dispatch. This now also protects the previously-ungated **arbitrary zip upload/extract**
(`upload_plugin`), `runsqlscript`, `gettablefields` (interpolated `SHOW COLUMNS`), and `validate_api`
(dynamic include) actions — all of which had no auth check. The per-branch
`include_once('../functions.php')` calls were left in place (harmless `include_once` no-ops).

> **Team note / possible follow-up:** the gate requires `administrator == 1`. Confirm no
> legitimate **non-admin** workflow calls `plugin_ajax.php` (e.g. a per-company settings save by a
> manager). If one exists it will now 403 — re-scope that specific action to the appropriate
> permission rather than weakening the gate. `gettablefields` still interpolates `$_REQUEST["table"]`
> into `SHOW COLUMNS FROM $tablename`; now admin-only, but worth whitelisting separately (out of A/B scope).

**B — origin guard on eval dispatch:** the three hook registrars in
`plugins/shopify/plugin.php::addPreHook/addPostHook/addReplaceHook` now stamp `'origin' => 'code'`.
`PluginHooksPreScriptCheck` and `PluginHooksPostScriptCheck` (`functions.php:3466,3501`) now
`continue` past any hook whose `origin !== 'code'` before reaching `eval`, with a SECURITY comment
documenting the invariant. Behaviour is unchanged for the shopify plugin (all its hooks are stamped);
any future DB- or request-sourced hook is refused instead of eval'd.

> **Note for other plugins:** any plugin that builds `HOOKS` entries by a means other than these three
> registrars must also stamp `'origin' => 'code'`, or its hooks will be silently skipped. Currently
> only the shopify plugin registers hooks, so nothing else is affected today.

Not implemented (as agreed): C (dispatch table) and D (stray `eval(newcall)` at 3467 / pre-hook
try-catch) and E (GET scoping) remain open.

## Re-test methodology (plugin tenant, both verbs)

Run as a **shopify-enabled** company so `addHooks()` registers the GET+POST hooks.

1. **Functional parity — POST hooks:** exercise each replace/pre hook via its trigger
   (`inveditsave.php?call=fetch_inventory`, `jobeditsave.php?call=filter`,
   `clienteditsave.php?call=getClients`, `invcats.php?call=getCats`) — confirm same output as before.
2. **Functional parity — GET hooks:** load `?page=jobedit&id=<job>`, `?page=pickingedit`,
   `mfshoporderdetail`, `reqedit`, `purchaseadd`, `dispatchedit` — confirm `jobSetOrderStatus`
   side-effects fire once, page renders fully, no premature `die`.
3. **Pre-hook fault isolation (post-fix D):** temporarily register a throwing hook → confirm the
   page still renders and the failure is in `error_log`, not a white screen.
4. **`plugin_ajax.php` gate (post-fix A):**
   - unauthenticated `POST action=activate&id=…` → **403**, no DB write;
   - non-admin session → **403**, no DB write;
   - admin session → succeeds. Verify `evolution.plugins` row unchanged in the deny cases.
5. **Origin guard (post-fix B):** hand-inject a `HOOKS` entry without `'origin'=>'code'` into a
   test session → confirm it is **skipped** (not eval'd).
6. **Non-plugin tenant regression:** load several core pages on GET and POST for a company with no
   active plugins → confirm `companyplugins` empty path is a no-op and nothing in control flow changed.
7. **Dispatch-table parity (post-fix C):** repeat steps 1-2 and diff behaviour against the pre-C baseline.

---

## Summary table

| # | Issue | Reachable today? | Severity | Fix |
|---|-------|------------------|----------|-----|
| A | `plugin_ajax.php` no auth gate on `UPDATE plugins` | **Yes** | High | requireAuth + admin check |
| B | eval source only *incidentally* code-only | Latent | High (design) | origin guard + documented invariant |
| C | `eval()` used at all | n/a | Medium | dispatch table |
| D | stray `eval(newcall)` 3467 + unwrapped pre-hook eval | Yes (fragile) | Medium | remove + try/catch |
| E | GET broadening changes app-wide control flow | Yes | Low/Med | scope + document |

**Bottom line:** Not currently RCE — the eval payloads are shipped code, and the plugins table carries no eval config. But the writer endpoint (A) is genuinely unprotected, and the eval design (B/C) is one careless change away from RCE. Recommend landing A and B now, D shortly after, and C/E as planned hardening.