# WP11 — `ProductionOrderController`: four unscoped foreign keys (tenant isolation) — REPORT **Status:** COMPLETE **Repo:** BE `/home/moonui2/moon-erp-be` · **Branch:** `hazemdev2` · **Migration:** none · **FE change:** none **Commit:** `c209bdebd` — `fix(production): scope the production order issue/receive foreign keys to the caller's company` (not pushed, not merged) --- ## 1. What was wrong `Modules/Production/app/Http/Controllers/ProductionOrderController.php` validated four foreign keys with a bare `exists:` rule, so a signed-in user in company A could name company B's record on a **write** path and the write went through under company A: | Line (pre-fix) | Rule | Method | Where the foreign id landed | |---|---|---|---| | 695 | `'warehouse_id' => ['nullable','integer','exists:warehouses,id']` | `issueMaterials()` | `inventory_issues.warehouse_id` — materials drawn out of another company's warehouse | | 703 | `'lines.*.product_id' => [...,'exists:products,id']` | `issueMaterials()` | `inventory_issue_items.product_id` (a raw product line resolves no order-material row, so `resolveLines()` takes the id verbatim) | | 731 | `'lines.*.unit_id' => [...,'exists:units,id']` | `issueMaterials()` | `inventory_issue_items.unit_id` (same raw-product-line path — with a material row the unit is overridden, without one it survives) | | 1006 | `'warehouse_id' => ['required','integer','exists:warehouses,id']` | `receiveFg()` | `inventory_receipts.warehouse_id` — finished goods capitalised into another company's warehouse | **The comment was the worst part.** Line 690 read: > `// ISS-2026-9166 (WP4) [FIN] — the acting company scopes the per-line routing exists rules` > `// below so a line can never reference another company's warehouse or business partner.` `$companyId` was resolved on line 691 and used for `lines.*.warehouse_id` and `lines.*.owner_partner_id` — but not for the top-level `warehouse_id` on the very next line. Anyone reading this method to audit the invariant would have concluded it held. It did not. `receiveFg()` did not resolve `$companyId` at all. --- ## 2. Red-before-green New test file: `Modules/Production/tests/Feature/ProductionOrderTenantScopeTest.php` (7 tests). Helpers prefixed `wp11OrderScope*` (Pest single-process redeclare rule). ### RED — before the fix ``` cd /home/moonui2/moon-erp-be && /opt/cpanel/ea-php82/root/usr/bin/php -d memory_limit=1G vendor/bin/pest --filter='WP11' FAIL Modules\Production\tests\Feature\ProductionOrderTenantScopeTest ⨯ WP11: a top-level warehouse_id from another company is rejected on… 8.21s ⨯ WP11: a lines.*.product_id from another company is rejected on issu… 3.96s ⨯ WP11: a lines.*.unit_id from another company is rejected on issue-m… 3.72s ⨯ WP11: a warehouse_id from another company is rejected on receive-fg 3.80s ⨯ WP11: the rejection is an ordinary validation message that names no… 3.70s ✓ WP11: a legitimate same-company issue-materials still succeeds 6.69s ✓ WP11: a legitimate same-company receive-fg still succeeds 3.93s FAILED … > a top-level warehouse_id from another company is rejected on issue-materials Expected response status code [422] but received 200. Failed asserting that 200 is identical to 422. FAILED … > a lines.*.product_id from another company is rejected on issue-materials Expected response status code [422] but received 200. Failed asserting that 200 is identical to 422. FAILED … > a lines.*.unit_id from another company is rejected on issue-materials Expected response status code [422] but received 200. Failed asserting that 200 is identical to 422. FAILED … > a warehouse_id from another company is rejected on receive-fg Expected response status code [422] but received 200. Failed asserting that 200 is identical to 422. FAILED … > the rejection is an ordinary validation message that names nothing foreign Expected response status code [422] but received 200. Failed asserting that 200 is identical to 422. Tests: 5 failed, 2 passed (10 assertions) Duration: 35.48s ``` **All four cross-tenant references returned HTTP 200 today.** Not a partial guard, not a downstream rejection — the requests were accepted and the rows were written. The two same-company controls passed on the unfixed code, which proves the tests are not simply rejecting everything. > Interim note: an earlier RED run of the `lines.*.product_id` case (before an explicit `unit_id` > was added to that line's payload) died with > `SQLSTATE[23000] … NOT NULL constraint failed: inventory_issue_items.unit_id` — i.e. validation had > already passed and the request reached the **INSERT of the foreign product into company A's > inventory issue**. That is the strongest single piece of evidence in this report that the hole was > a real write path, not a theoretical one. ### GREEN — after the fix ``` cd /home/moonui2/moon-erp-be && /opt/cpanel/ea-php82/root/usr/bin/php -d memory_limit=1G vendor/bin/pest --filter='WP11' PASS Modules\Production\tests\Feature\ProductionOrderTenantScopeTest ✓ WP11: a top-level warehouse_id from another company is rejected on… 8.35s ✓ WP11: a lines.*.product_id from another company is rejected on issu… 3.87s ✓ WP11: a lines.*.unit_id from another company is rejected on issue-m… 3.63s ✓ WP11: a warehouse_id from another company is rejected on receive-fg 3.81s ✓ WP11: the rejection is an ordinary validation message that names no… 3.78s ✓ WP11: a legitimate same-company issue-materials still succeeds 4.25s ✓ WP11: a legitimate same-company receive-fg still succeeds 3.97s Tests: 7 passed (25 assertions) Duration: 33.12s ``` --- ## 3. The fix Four rules converted to the idiom the file already used — the same one WP9 landed in `BomLineRules` (`be1f6b4ed`) and the same one `lines.*.warehouse_id` two lines below already used: ```php \Illuminate\Validation\Rule::exists('', 'id')->where('company_id', $companyId) ``` - `issueMaterials()` — `warehouse_id`, `lines.*.product_id`, `lines.*.unit_id`, all against the already-resolved `$companyId` (line 691, unchanged). - `receiveFg()` — `$companyId = (int) auth()->user()->company_id;` added immediately after `authorizeCompany($order)` (the same position and the same expression `issueMaterials()` uses), then `warehouse_id` scoped. No restructuring, no trait extraction, no other rule touched. All three target tables carry `company_id` (verified in `create_warehouses_table`, `create_products_table`, `create_units_table`), so the scoped rule is well-formed on every one. --- ## 4. Acceptance criteria | # | Criterion | Result | |---|---|---| | 1 | Red-before-green on all four | ✅ all four returned **200** before, **422** after — both runs pasted above | | 2 | Legitimate same-company issue + receive-FG still succeed | ✅ both control tests green before AND after; `pest Modules/Production` shows no new failures | | 3 | Rejection is an ordinary validation message, no leak | ✅ asserted: body contains neither `FOREIGN-SECRET-WAREHOUSE` nor the string `another company`, and `errors.warehouse_id` equals `__('validation.exists', …)` **verbatim** | | 4 | Fix the comment at line 690 | ✅ rewritten — see below | | 5 | `pest Modules/Production` zero NEW failures | ✅ see §6 | ### Criterion 4 in detail — the comment, verified rather than assumed The brief asked me to verify the old claim becomes true rather than assume it. **It does not become literally true**, so I did not write it that way. I enumerated every key in `issueMaterials()`'s validate array and traced where each lands: | Key | Scoped? | By what | |---|---|---| | `warehouse_id` | ✅ now | this WP | | `lines.*.material_id` | ✅ already | `->where('production_order_id', $order->id)`, and `$order` is `authorizeCompany()`'d | | `lines.*.product_id` | ✅ now | this WP | | `lines.*.unit_id` | ✅ now | this WP | | `lines.*.warehouse_id` | ✅ already | WP4 | | `lines.*.owner_partner_id` | ✅ already | WP4 closure on `BusinessPartner` | | `lines.*.batch_ids.*.batch_id` | ✅ already | not here — `IssueMaterials::resolveBatchAllocations()` does `MfgBatch::where('company_id', $order->company_id)` and throws a 422 on a miss (verified by reading it, not by trusting the existing comment) | | **`lines.*.product_variant_id`** | ❌ **no** | `['nullable','integer']` — **no `exists` rule at all** | `product_variant_id` is the one hole an unqualified "a line can never reference another company's record" claim would still paper over. In `IssueMaterials::resolveLines()`: ```php $variantId = $material?->product_variant_id ?? (! empty($line['product_variant_id']) ? $line['product_variant_id'] : null); ``` When the line resolves an order-material row the snapshot's variant wins and the request value is discarded — but on a **raw product line with no material row** the request value is taken verbatim and stamped onto `inventory_issue_items.product_variant_id`. That is outside this WP's four-rule scope (see §5), so the comment now **names it as the exception** instead of hiding it. New comment: ```php // ISS-2026-9166 (WP4) [FIN] / WP11 — the acting company scopes EVERY `exists` rule below // (the issue-level warehouse override, the per-line routing warehouse and business partner, // and the per-line product and unit), so none of them can name another company's record. // Two line keys are scoped elsewhere, not here: `lines.*.material_id` by the order itself // (already authorizeCompany()'d), and `lines.*.batch_ids.*.batch_id` by // IssueMaterials::resolveBatchAllocations(), which looks the lot up under $order->company_id. // WP11 note — the ONE key this guarantee does NOT cover is `lines.*.product_variant_id`: // it carries no exists rule here and, on a raw product line with no order-material row, // resolveLines() takes it verbatim. Reported by WP11, not fixed by it. ``` Every clause in it is now a statement I traced to code. A reader auditing the invariant from this comment alone will reach the correct conclusion, including about the part that is still open. --- ## 5. Sweep — other unscoped `exists:` in `Modules/Production/app/Http/Controllers/` **Result: none. The four fixed here were the complete set.** - `grep -rn "exists:" Modules/Production/app/Http/Controllers/` now returns **zero** hits. - Every `Rule::exists(...)` in the directory carries a scoping `->where(...)`. Verified individually across `MpsController`, `MfgTollContractController`, `StandardCostController`, `RoutingController`, `RndProjectController`, `RndAmortizationController`, `ToolController`, `BatchController`, `ConfirmationController` and `ProductionOrderController` — all `->where('company_id', $companyId)`, except three that scope through a company-owned parent and are **correct as written**: - `ProductionOrderController:713` and `:805` — `production_order_materials` scoped by `production_order_id` (the order is `authorizeCompany()`'d). - `ConfirmationController:69` — `mfg_confirmations` scoped by `company_id` **and** `production_order_id`. - `Modules/Production/app/Http/Requests/` re-confirmed clean (WP9's finding still holds; the only `exists:` string left in that tree is inside a doc comment in `Concerns/BomLineRules.php:23` describing the bug WP9 fixed). `BackflushFromStagingRequest.php:30` left alone as instructed. ### Reported, not fixed 1. **`lines.*.product_variant_id` in `issueMaterials()` has no `exists` rule.** Detailed in §4. It is a genuine (if narrower) cross-tenant reference on the same endpoint — reachable only on a raw product line with no order-material row. Deliberately left out of this WP's minimal diff; it needs its own red-before-green and a decision about whether the variant should instead be forced to match the resolved product. Worth a follow-up ticket. 2. **`StoreBomRequest`'s duplicate line rules are not folded into the shared trait** (flagged by WP1 and WP9, out of scope here). Both copies are correct today; it is a drift risk, not a defect. Re-confirmed still true. --- ## 6. Scoped test runs | Run | Result | |---|---| | `pest --filter='WP11'` (before fix) | 5 failed / 2 passed — the four holes all returned 200 | | `pest --filter='WP11'` (after fix) | **7 passed, 25 assertions** | | `pest Modules/Production` (after fix) | **661 passed / 10 failed** (3979 assertions, 1468.67s) | Baseline after WP10 was **654 passed / 10 failed**. 654 + my 7 new tests = **661**, and the failure count is unchanged at 10 — the same pre-existing set, confirmed by name in the run output: ``` ⨯ it settle-buy under DIRECT grn_mode does NOT double-increase physic… ← ConsignmentFoundation ×1 ⨯ it F1 variance: grounding carries the REAL decomposed variance legs… ┐ ⨯ it F1 variance: the single LLM request embeds the real leg amounts… ├ CostAiAnomalyVariance ×3 ⨯ it F1 variance: endpoint runs over HTTP, perm-gated + 404s a foreig… ┘ ⨯ it BE-V: a standard order closed via HTTP records all 6 variances t… ┐ ⨯ it BE-V: each variance family reconciles to actual − std to the cen… │ ⨯ it BE-V: classification bands split normal / monitor / investigate ├ ProductionVariance ×6 ⨯ it BE-V: an actual-method order records no variances │ ⨯ it BE-V: FOVV is descoped when no budget / budgeted volume exists │ ⨯ it BE-V: GET production/variances returns and filters the records ┘ Tests: 10 failed, 661 passed (3979 assertions) Duration: 1468.67s ``` **Zero NEW failures.** --- ## 7. Housekeeping - `./vendor/bin/pint` on the two touched files only → `{"result":"pass"}`. - `chown moonui2:moonui2` on `ProductionOrderController.php`, `ProductionOrderTenantScopeTest.php`, `docs/moonstack/CHANGELOG.md`. - `bash local-deploy.sh` → completed, no new migrations. - Bilingual `[Unreleased]` bullet added to `docs/moonstack/CHANGELOG.md` (EN + AR, `{{ar}}` separator) covering both the issue-materials and the receive-FG side in end-user language. - No push, no merge. moonui2 only. ## 8. Files changed - `Modules/Production/app/Http/Controllers/ProductionOrderController.php` — 4 rules + the comment - `Modules/Production/tests/Feature/ProductionOrderTenantScopeTest.php` — new, 7 tests - `docs/moonstack/CHANGELOG.md` — 1 bilingual bullet