# WP9 — BOM line rules company-scoped (tenant isolation) — REPORT **Status:** DONE **Commit:** `be1f6b4ed` — `fix(production): scope BOM line foreign keys to the caller's company` **Branch:** `hazemdev2` (BE, `/home/moonui2/moon-erp-be`). **Not pushed, not merged.** **Migration:** none · **FE change:** none --- ## 1. What was wrong `Modules/Production/app/Http/Requests/Concerns/BomLineRules.php` (created by WP1, `513cc5697`) validated three tenant-owned foreign keys with a bare `exists:` rule: ```php 'components.*.product_id' => ['required', 'exists:products,id'], 'components.*.unit_id' => ['required', 'exists:units,id'], 'operations.*.production_center_id' => ['required', 'exists:production_centers,id'], ``` `exists:` asks only "does this row exist anywhere in the installation" — not "does it belong to the caller's company". Because the trait is shared by `UpdateBomRequest`, `BomController::syncComponents()` and `BomController::syncOperations()`, a signed-in user of company A could attach company B's product, unit or production centre to their own BOM through any of: - `PUT /api/production/boms/{bom}` — the ordinary Save button (widened by WP1) - `PUT /api/production/boms/{bom}/components` - `PUT /api/production/boms/{bom}/operations` Creating a BOM was already safe (`StoreBomRequest.php:45,48,66`), and the substitute rule on the very next line of the same trait (`:50`) was already scoped. So this was an asymmetry inside one feature, not a deliberate design. --- ## 2. The change One file of production code, in the **one** shared trait — no second copy anywhere. ```php protected static function bomComponentRules(string $presence): array { $companyId = auth()->user()?->company_id; return [ 'components' => [$presence, 'array'], 'components.*.product_id' => ['required', Rule::exists('products', 'id')->where('company_id', $companyId)], ... 'components.*.unit_id' => ['required', Rule::exists('units', 'id')->where('company_id', $companyId)], ``` ```php protected static function bomOperationRules(string $presence): array { $companyId = auth()->user()?->company_id; return [ 'operations' => [$presence, 'array'], 'operations.*.production_center_id' => ['required', Rule::exists('production_centers', 'id')->where('company_id', $companyId)], ``` The idiom is copied verbatim from `StoreBomRequest.php` — the same `$companyId = auth()->user()?->company_id;` local, the same `Rule::exists(table, id)->where('company_id', $companyId)`. The pre-existing substitute rule was folded onto the same `$companyId` local (it was already scoped; this is only de-duplication of the `auth()->user()?->company_id` expression, no behaviour change). A docblock paragraph was added to the trait recording *why* it must stay scoped and why the message stays generic, so the next editor does not reintroduce a bare `exists:`. **Fail-closed note:** if `auth()->user()` is null, `$companyId` is null and the rule matches nothing. Both routes are behind auth middleware anyway, and this matches `StoreBomRequest`'s behaviour exactly. --- ## 3. Red-before-green — both runs pasted New test file: `Modules/Production/tests/Feature/ProductionBomLineTenantScopeTest.php` (8 tests). Helpers are prefixed `bomTenantScope*` (`bomTenantScopeFixture`, `bomTenantScopeBom`) — verified unique across the whole suite, so no Pest single-process redeclare. Test design note: fixture products use the factory default `base_unit_id = null` so `BomController::assertComponentUnitsMatchBase()` stays out of the way — these cases must fail on the *validation rule*, not on that unrelated guard. Otherwise the red run would have shown a 422 for the wrong reason. ### RED — before the fix (trait unchanged) ``` $ /opt/cpanel/ea-php82/root/usr/bin/php -d memory_limit=1G vendor/bin/pest --filter='ProductionBomLineTenantScope' FAIL Modules\Production\tests\Feature\ProductionBomLineTenantScopeTest ⨯ it rejects a component product belonging to another company on PUT… 8.29s ⨯ it rejects a component unit belonging to another company on PUT /bo… 2.30s ⨯ it rejects an operation production centre belonging to another comp… 2.23s ⨯ it rejects another company product through PUT /boms/{id}/component… 2.30s ⨯ it rejects another company unit through PUT /boms/{id}/components 2.13s ⨯ it rejects another company production centre through PUT /boms/{id}… 2.30s ⨯ it returns an ordinary validation message that does not reveal the… 2.34s ✓ it still accepts components and operations that belong to the calle… 2.25s FAILED … Expected response status code [422] but received 200. Failed asserting that 200 is identical to 422. at ProductionBomLineTenantScopeTest.php:79 75▕ 'components' => [ 76▕ ['product_id' => $ctx['partB']->id, 'quantity' => 1, 'unit_id' => $ctx['unitA']->id], 77▕ ], 78▕ ]) ➜ 79▕ ->assertStatus(422) Tests: 7 failed, 1 passed (13 assertions) Duration: 24.53s ``` **Every cross-tenant write returned `200` — the save went through.** All three fields, through both entry points. The only passing test was the control (a legitimate same-company save). ### GREEN — after the fix ``` $ /opt/cpanel/ea-php82/root/usr/bin/php -d memory_limit=1G vendor/bin/pest --filter='ProductionBomLineTenantScope' PASS Modules\Production\tests\Feature\ProductionBomLineTenantScopeTest ✓ it rejects a component product belonging to another company on PUT… 6.93s ✓ it rejects a component unit belonging to another company on PUT /bo… 2.25s ✓ it rejects an operation production centre belonging to another comp… 2.17s ✓ it rejects another company product through PUT /boms/{id}/component… 2.18s ✓ it rejects another company unit through PUT /boms/{id}/components 2.11s ✓ it rejects another company production centre through PUT /boms/{id}… 2.17s ✓ it returns an ordinary validation message that does not reveal the… 2.13s ✓ it still accepts components and operations that belong to the calle… 2.17s Tests: 8 passed (37 assertions) Duration: 23.57s ``` (Two intermediate iterations were needed on the *leak-message* assertion only — `expect()->toContain()` on a scalar, then a dotted `json()` path that Laravel splits on the dots inside the error key `components.0.product_id`. Neither touched production code; the six status assertions were green on the first post-fix run.) ### Acceptance criteria mapped to tests | Criterion | Test | |---|---| | foreign `product_id` rejected on `PUT /boms/{id}` | `rejects a component product belonging to another company on PUT /boms/{id}` | | foreign `unit_id` rejected on `PUT /boms/{id}` | `rejects a component unit belonging to another company on PUT /boms/{id}` | | foreign `production_center_id` rejected on `PUT /boms/{id}` | `rejects an operation production centre belonging to another company on PUT /boms/{id}` | | same protection via `PUT /boms/{id}/components` | two tests (product + unit) | | same protection via `PUT /boms/{id}/operations` | one test (production centre) | | no data leak in the message | `returns an ordinary validation message that does not reveal the other company record` | | legitimate same-company save unaffected | `still accepts components and operations that belong to the caller company` | Each rejection test also asserts the BOM header did **not** change and no line rows were written — the whole request is refused, nothing half-saves. --- ## 4. No-leak verification (criterion 5) The rejection is Laravel's default `validation.exists` message, asserted verbatim: ``` "The selected components.0.product_id is invalid." ``` The test additionally asserts the whole response body contains neither the foreign product's name (the fixture names it `FOREIGN-SECRET-PART`) nor the phrase `another company`. This is byte-identical to what a caller gets for a deleted or made-up id, so a probe cannot distinguish "does not exist" from "belongs to someone else". --- ## 5. Regression evidence **WP1's suite — unchanged, still 6/36:** ``` $ … vendor/bin/pest --filter='ProductionBomUpdateLines' PASS Modules\Production\tests\Feature\ProductionBomUpdateLinesTest ✓ it persists edited components and operations through PUT /boms/{id} 7.36s ✓ it keeps nested substitutes through a GET then PUT round trip 2.84s ✓ it leaves existing lines untouched when the payload omits component… 2.24s ✓ it returns 422 and saves no header when a component fails request v… 4.41s ✓ it rolls the header back when the component unit guard rejects the… 2.20s ✓ it stores supply_source and customer_qty_per_unit exactly as sent 2.29s Tests: 6 passed (36 assertions) ``` **Full Production module:** ``` $ … vendor/bin/pest Modules/Production Tests: 10 failed, 640 passed (3726 assertions) Duration: 1452.90s ``` | | passed | failed | |---|---|---| | WP1 baseline | 632 | 10 | | after WP9 | **640** (+8 = exactly the new file) | **10** (unchanged) | The 10 failures were confirmed to be the same pre-existing ones by running only the three known-bad files in isolation — they account for **all 10**, so nothing else in the module fails: ``` $ … vendor/bin/pest ConsignmentFoundationTest.php CostAiAnomalyVarianceTest.php ProductionVarianceTest.php Tests: 10 failed, 23 passed (184 assertions) ``` - ConsignmentFoundation ×1 — `settle-buy under DIRECT grn_mode …` - CostAiAnomalyVariance ×3 — the three `F1 variance:` cases - ProductionVariance ×6 — the `BE-V:` cases **Zero new failures.** --- ## 6. Other unscoped `exists:` rules found (reported, NOT changed) A sweep of `Modules/Production/app` for bare `exists:` on tenant-owned tables turned up **four**, all in `ProductionOrderController.php`, all outside this WP's scope: | File:line | Rule | Note | |---|---|---| | `ProductionOrderController.php:695` | `'warehouse_id' => ['nullable','integer','exists:warehouses,id']` | **Worth a look.** In `issueMaterials()`, whose own comment two lines above says *"the acting company scopes the per-line routing exists rules below so a line can never reference another company's warehouse or business partner"* — and the per-line rules right after it ARE scoped. This top-level warehouse override was missed by that work (ISS-2026-9166 / WP4). Same defect shape as WP9. | | `ProductionOrderController.php:703` | `'lines.*.product_id' => [… 'exists:products,id']` | same method, unscoped | | `ProductionOrderController.php:731` | `'lines.*.unit_id' => [… 'exists:units,id']` | same method, unscoped | | `ProductionOrderController.php:1006` | `'warehouse_id' => ['required','integer','exists:warehouses,id']` | `receive-fg` handler | Nothing else in `Modules/Production/app/Http/Requests/` is unscoped — after this commit the only `Rule::exists` without a company filter in that directory is `BackflushFromStagingRequest.php:30`, which is scoped by `production_order_id` instead (the order itself is already `authorizeCompany`'d, so that one is correct as written). **Also still open (out of scope, flagged by WP1 and confirmed here):** `StoreBomRequest` keeps its own full copy of the component/operation rules rather than consuming `BomLineRules`. Both copies are now correct and identical in intent, but they are two copies that can drift — exactly the risk the trait was created to remove. Tidiness, not a defect; recommend a follow-up WP that makes `StoreBomRequest` use the trait with `$presence = 'sometimes'` and `required_with:components`. --- ## 7. Housekeeping done - `./vendor/bin/pint` on the two touched PHP files only → `{"result":"pass"}`. - `chown moonui2:moonui2` on all three edited files (verified: all `moonui2 moonui2`). - `bash local-deploy.sh` → `انتهى الديبلوي بنجاح ✓`, no new migrations. - Bilingual `[Unreleased]` bullet added at the top of `docs/moonstack/CHANGELOG.md` ("A bill of materials can no longer be made to point at another company's item, unit or work centre." / "مابقاش ممكن تخلّي قائمة المكوّنات تشاور على صنف أو وحدة أو مركز إنتاج بتاع شركة تانية."). - Working tree clean after commit. **Nothing pushed, nothing merged.** No `/home/moonui` path touched, no `migrate:fresh`/`refresh`/`db:wipe`, no git remote printed. ## 8. Files touched | File | Change | |---|---| | `Modules/Production/app/Http/Requests/Concerns/BomLineRules.php` | 3 rules scoped + docblock; substitute rule de-duplicated onto `$companyId` | | `Modules/Production/tests/Feature/ProductionBomLineTenantScopeTest.php` | new, 8 tests / 37 assertions | | `docs/moonstack/CHANGELOG.md` | one bilingual `[Unreleased]` bullet | ## 9. Concerns 1. **`ProductionOrderController::issueMaterials()` (line 695) is the same defect and is still open.** Its own comment claims company scoping that the top-level `warehouse_id` does not have. Recommend a follow-up WP; not changed here because the brief limits the fix to the BOM trait. 2. The `StoreBomRequest` duplicate-rules drift risk above. 3. Nothing else. No behaviour change for legitimate single-company use; no migration; no FE impact.