fix(tests): reset EntityManager in ApiLeastPrivilegeTest to prevent stale references
This commit is contained in:
hamed
@@ -19,16 +19,22 @@ authz/headers/cors/inject) + پروبهای دستی با JWT واقعی هر
|
||||
| # | یافته | شدت | وضعیت |
|
||||
|---|-------|-----|-------|
|
||||
| 1 | `TreatmentProtocolController` هیچ گِیت مجوزی نداشت — منشیِ `services:false` میتوانست پروتکل درمان را بخواند، بازنویسی و حذف کند | 🟧 High | ✅ رفع شد |
|
||||
| 2 | `react-router` — ۵ advisory از جمله XSS و open redirect | 🟧 High | ⚠️ باز — نیازمند تصمیم |
|
||||
| 3 | `lodash-es` — code injection در `_.template` + دو prototype pollution | 🟧 High | ⚠️ باز (از آدیت قبلی) |
|
||||
| 2 | `react-router` — ۵ advisory از جمله XSS و open redirect | 🟧 High | ✅ رفع شد (مهاجرت به v8) |
|
||||
| 3 | `lodash-es` — code injection در `_.template` + دو prototype pollution | 🟧 High | ✅ رفع شد (override به 4.18.1) |
|
||||
| 4 | ۶۲ moderate در `@ckeditor/ckeditor5-build-classic` (deprecated) | 🟨 Medium | ⚠️ risk پذیرفتهشده — تصمیم ۲۰۲۶-۰۸-۰۷ |
|
||||
| 5 | `dangerouslySetInnerHTML` روی بدنهٔ بلاگ در `BlogReviewPage` | 🟨 Medium | ⚠️ باز |
|
||||
| 6 | `APP_SECRET` واقعی در `.env.test` تحت git | 🟦 Low | ⚠️ باز |
|
||||
| 7 | پسورد sandbox درگاه ملت هاردکد | ⬜ Info | باز (از آدیت قبلی، کماهمیت) |
|
||||
| 5 | `dangerouslySetInnerHTML` روی بدنهٔ بلاگ در `BlogReviewPage` | 🟨 Medium | ✅ رفع شد (sanitize هنگام ذخیره) |
|
||||
| 6 | `APP_SECRET` واقعی در `.env.test` تحت git | 🟦 Low | ✅ رفع شد |
|
||||
| 7 | پسورد sandbox درگاه ملت هاردکد | ⬜ Info | ✅ رفع شد (به env منتقل شد) |
|
||||
| 8 | ۱۰ روت `GET` که مجوزِ رجیستریشان را enforce نمیکنند | 🟨 Medium | ⚠️ باز — با تست baseline مهار شد |
|
||||
|
||||
طبق تصمیم کاربر پیش از اجرا: Critical و High **همان جلسه** رفع میشوند؛ Medium و پایینتر فقط
|
||||
گزارش میشوند. یافتهٔ ۱ رفع شد. یافتههای ۲ و ۳ High هستند ولی رفعشان **ارتقای وابستگی** است نه
|
||||
تغییر کد این repo، و شکستنِ روتینگ پنل یا ادیتور را در پی دارد — پس تصمیم به کاربر واگذار شد.
|
||||
سیاست اولیه «فقط Critical/High رفع شود» بود؛ کاربر بعداً رفعِ همهٔ یافتههای باز را خواست، پس
|
||||
یافتههای ۲، ۳، ۵، ۶ و ۷ هم بسته شدند. یافتهٔ ۴ طبق تصمیم صریح خارج از محدوده ماند و یافتهٔ ۸
|
||||
حین همین کار کشف شد.
|
||||
|
||||
```bash
|
||||
npm audit --omit=dev # قبل: high=2 moderate=62 بعد: high=0 moderate=3
|
||||
ddev exec php bin/phpunit # ۱۵۵۵ تست، ۴۸۳۲ assertion، سبز
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
@@ -183,10 +189,32 @@ npm audit --omit=dev --json | python3 -c "import json,sys; print(json.load(sys.s
|
||||
# → {'info': 0, 'low': 0, 'moderate': 62, 'high': 2, 'critical': 0, 'total': 64}
|
||||
```
|
||||
|
||||
**۵. وضعیت:** باز. پروژه روی React Router v7 است و رفع یعنی bump به نسخهای خارج از range. سه
|
||||
advisory (RSC، SSR hydration، CSRF مود RSC) به این پنل که کلاینتساید محض است مربوط نمیشوند؛ ولی
|
||||
open redirect و DoS مربوطاند. **رفع نشد** چون ارتقای major روتینگِ ۵۰ صفحهٔ پنل تست دستی
|
||||
میخواهد و در یک پاس امنیتی ریسکش بیشتر از خودِ یافته است. تسک جدا پیشنهاد میشود.
|
||||
**۵. رفع اعمالشده:** مهاجرت از `react-router-dom@7.17.0` به `react-router@8.3.0`.
|
||||
|
||||
نسخهٔ امن فقط `> 8.2.0` است و در v8 پکیج `react-router-dom` دیگر منتشر نمیشود (آخرین نسخهاش
|
||||
`7.18.2` است) — یعنی bump ساده ممکن نبود و مهاجرت اجباری بود:
|
||||
|
||||
```bash
|
||||
npm pkg delete dependencies.react-router-dom
|
||||
npm pkg set dependencies.react-router="^8.3.0"
|
||||
# ۸۱ فایل: from 'react-router-dom' → from 'react-router'
|
||||
```
|
||||
|
||||
**۶. چرا کمریسک بود:** کل APIهای مصرفشده در پنل ۱۱ تاست و همه در v8 دستنخوردهاند —
|
||||
`BrowserRouter`, `MemoryRouter`, `Routes`, `Route`, `Link`, `NavLink`, `Navigate`, `Outlet`,
|
||||
`useLocation`, `useNavigate`, `useParams`, `useSearchParams`. هیچ API حذفشدهای در پروژه استفاده
|
||||
نمیشد، پس مهاجرت فقط تغییرِ نامِ ماژول بود نه بازنویسیِ روتینگ.
|
||||
|
||||
**۷. تأیید:**
|
||||
|
||||
```
|
||||
npx tsc --noEmit # بدون خطا
|
||||
ddev exec yarn dev # webpack compiled successfully — 54 فایل
|
||||
npx vitest --run # 802 تست فرانتاند
|
||||
```
|
||||
|
||||
هفت تستِ فرانتاند در اجرای موازیِ اول timeout دادند؛ با `--no-file-parallelism` هر ۵۴ تستِ همان
|
||||
فایلها سبز شد. یعنی گرسنگی منابع بود نه رگرسیونِ روتینگ.
|
||||
|
||||
---
|
||||
|
||||
@@ -199,7 +227,27 @@ open redirect و DoS مربوطاند. **رفع نشد** چون ارتقای
|
||||
|
||||
**۳. بازتولید:** همان `npm audit` بالا — `high=2` که یکی react-router است و یکی lodash-es.
|
||||
|
||||
**۴. وضعیت:** باز. transitive است؛ باید ردیابی شود کدام پکیج آن را میکشد.
|
||||
**۴. منشأ:** ردیابی شد — کاملاً transitive و فقط از یک جا میآید:
|
||||
|
||||
```
|
||||
clinicpro
|
||||
└─┬ @ckeditor/ckeditor5-build-classic@44.3.0
|
||||
└── lodash-es@4.17.21 (در دهها زیرپکیج dedupe شده)
|
||||
```
|
||||
|
||||
**۵. رفع اعمالشده:** چون هیچ dependency مستقیمی نیست، ارتقای مستقیم ممکن نبود. نسخهٔ امن
|
||||
`4.18.1` است (range آسیبپذیر `<=4.17.23`)، پس با override اعمال شد:
|
||||
|
||||
```json
|
||||
"overrides": { "lodash": "^4.17.21", "lodash-es": "^4.18.1" }
|
||||
```
|
||||
|
||||
**۶. چرا override و نه ارتقای CKEditor:** بستنِ این یافته از راه CKEditor یعنی همان مهاجرتی که در
|
||||
یافتهٔ ۴ عمداً خارج از محدوده گذاشته شد. override همان مشکل را بدون لمسکردن ادیتور میبندد.
|
||||
`lodash-es` در بازهٔ 4.17→4.18 شکستِ API ندارد و CKEditor فقط از توابع پایهاش استفاده میکند.
|
||||
|
||||
**۷. تأیید:** `npm audit --omit=dev` دیگر `lodash-es` را گزارش نمیکند؛ build و ۸۰۲ تست فرانتاند
|
||||
سبز.
|
||||
|
||||
---
|
||||
|
||||
@@ -226,9 +274,28 @@ CKEditor بالاست: نویسندهای که HTML مخرب paste کند،
|
||||
|
||||
**۳. بازتولید:** `grep -rn "dangerouslySetInnerHTML" assets/admin/` → تنها یک hit، همین خط.
|
||||
|
||||
**۴. وضعیت:** باز طبق سیاست (Medium بدون تأیید رفع نمیشود). CSP فعلی `script-src 'self'` است، پس
|
||||
`<script>` تزریقی اجرا نمیشود؛ ولی هندلرهای inline و `javascript:` را CSP فعلی کامل نمیبندد.
|
||||
پیشنهاد: sanitize سمت سرور هنگام ذخیره، نه فقط هنگام نمایش.
|
||||
**۴. رفع اعمالشده:** پاکسازی HTML در **لحظهٔ ذخیره** با `symfony/html-sanitizer`.
|
||||
|
||||
- `config/packages/html_sanitizer.yaml` — سیاست `blog.body`: فهرست سفیدِ عناصری که CKEditor واقعاً
|
||||
تولید میکند، طرحهای مجاز فقط `http`/`https`/`mailto`، و `rel="noopener noreferrer"` اجباری روی
|
||||
هر `<a>`.
|
||||
- `src/Blog/Service/BlogBodySanitizer.php` — سرویس نازک روی همان sanitizer.
|
||||
- هر چهار نقطهٔ ورودِ بدنه: ساخت و ویرایش در `BlogController` و در `RepresentationBlogController`.
|
||||
|
||||
**۵. دو تصمیم طراحی:**
|
||||
|
||||
- **ذخیره، نه نمایش.** بدنه چند مصرفکننده دارد — پنل ادمین، سایت عمومی `nobat724_front` و فید. اگر
|
||||
پاکسازی در لایهٔ نمایش بود، هر مصرفکنندهٔ تازه دوباره آسیبپذیر شروع میکرد.
|
||||
- **`drop_elements` نه `block_elements`.** اولین پیادهسازی `block` بود و تست قرمز شد:
|
||||
`block` تگ را برمیدارد ولی متنِ داخلش را نگه میدارد، پس `<script>alert(1)</script>` به متنِ
|
||||
`alert(1)` تبدیل میشد. برای این عناصر خودِ محتوا هم باید برود.
|
||||
|
||||
**۶. پاکسازی پیش از سنجشِ خالیبودن:** بدنهای که چیزی جز markup ناامن ندارد، بعد از پاکسازی خالی
|
||||
میشود و باید همان ۴۲۲ «الزامی است» را بگیرد، نه اینکه خالی ذخیره شود.
|
||||
|
||||
**۷. تست:** `tests/Blog/BlogBodySanitizerTest.php` — پنج تست: حذف `<script>`، حذف `onclick` و
|
||||
`onerror` و `javascript:`، بقای متنِ غنیِ سالم بههمراه `noopener`، پاکسازی مسیر ویرایش، و ۴۲۲
|
||||
برای بدنهٔ کاملاً ناامن. کل `tests/Blog/`: ۵۱ تست سبز.
|
||||
|
||||
---
|
||||
|
||||
@@ -240,15 +307,76 @@ git ls-files | grep -E '^\.env' | xargs grep -nE 'APP_SECRET=.+'
|
||||
# بقیهٔ فایلها فقط placeholder دارند: CHANGE_ME…, APP_SECRET=…, APP_SECRET=...
|
||||
```
|
||||
|
||||
`.env.dev` که یافتهٔ ۳ گزارش قبلی بود، تمیز است — آن رفع پابرجاست. `.env.test` فقط محیط تست را
|
||||
امضا میکند و ارزش عملیاتی ندارد، ولی مقدارِ واقعی در git بهتر است placeholder شود.
|
||||
`.env.dev` که یافتهٔ ۳ گزارش قبلی بود، تمیز است — آن رفع پابرجاست.
|
||||
|
||||
**رفع:** مقدار به `not-a-secret-test-env-only` تغییر کرد. آشکارا غیرعملیاتی بودنِ مقدار خودش
|
||||
جلوی این را میگیرد که کسی این فایل را منبع یک secret واقعی بپندارد.
|
||||
|
||||
---
|
||||
|
||||
### 7. ⬜ INFO — پسورد sandbox درگاه ملت
|
||||
|
||||
`src/Payment/Gateway/MellatGateway.php:23` — `private const SANDBOX_PASSWORD = '17384843'`. همان
|
||||
یافتهٔ ۵ گزارش قبلی، همچنان باز، همچنان کماهمیت (اعتبارنامهٔ عمومیِ محیط تست درگاه).
|
||||
**۱. فایل:** `src/Payment/Gateway/MellatGateway.php:23` —
|
||||
`private const SANDBOX_PASSWORD = '17384843'`. همان یافتهٔ ۵ گزارش قبلی.
|
||||
|
||||
**۲. ماهیت:** اعتبارنامهٔ نمایشیِ `banktest.ir` است — عمومی و منتشرشده در مستندات خودشان، پس
|
||||
افشای secret نیست. ولی رشتهٔ پسوردمانند در `src/` هم اسکنر را روشن میکند و هم جایگزینی با ترمینال
|
||||
تستِ دیگر را نیازمند ویرایش کد میکند.
|
||||
|
||||
**۳. رفع اعمالشده:** هر سه مقدار به constructor منتقل شدند و از env میآیند:
|
||||
|
||||
```yaml
|
||||
# config/services.yaml
|
||||
$sandboxTerminalId: '%env(default::MELLAT_SANDBOX_TERMINAL_ID)%'
|
||||
$sandboxUsername: '%env(default::MELLAT_SANDBOX_USERNAME)%'
|
||||
$sandboxPassword: '%env(default::MELLAT_SANDBOX_PASSWORD)%'
|
||||
```
|
||||
|
||||
شناسهٔ ترمینال و نام کاربری پیشفرضِ درونکد دارند (شناسهاند، نه اعتبارنامه)، ولی **پسورد
|
||||
عمداً پیشفرضِ درونکد ندارد**: نبودنِ env یعنی sandbox پیکربندی نشده، و آن بهتر از نگهداشتن رشتهٔ
|
||||
پسوردمانند در `src/` است. مقدار نمایشی در `.env` با توضیح صریح نشست.
|
||||
|
||||
**۴. تأیید:** `tests/Payment/` — ۱۴ تست سبز.
|
||||
|
||||
---
|
||||
|
||||
### 8. 🟨 MEDIUM — ده روت `GET` که مجوزِ رجیستریشان را enforce نمیکنند
|
||||
|
||||
**۱. کشف چطور شد:** حین ساختِ تور ایمنیِ ساختاری (پیشنهاد ۵ همین گزارش). تست یک منشی میسازد که
|
||||
**هر** مجوزِ `PermissionCatalog` برایش خاموش است و هر روتِ `GET` بدون path parameter را با او
|
||||
میزند. ۲۹ روت پاسخ موفق دادند؛ پس از triage روی محتوای پاسخ، ۱۹ تایشان موجه بودند (دادهٔ مرجعِ
|
||||
ثابت، اندپوینت عمومی، یا دادهٔ خودِ کاربر) و ۱۰ تا نه:
|
||||
|
||||
| روت | مجوزی که باید enforce شود |
|
||||
|---|---|
|
||||
| `/api/v1/appointments/user` | `appointments.view` |
|
||||
| `/api/v1/my/appointments` | `appointments.view` |
|
||||
| `/api/v1/my/appointments/today-stats` | `appointments.view` |
|
||||
| `/api/v1/my/billing/payments` | `payments.view` |
|
||||
| `/api/v1/my/billing/payments/summary` | `payments.view` |
|
||||
| `/api/v1/billing/claims` | `payments.view` |
|
||||
| `/api/v1/billing/claims/by-patient` | `payments.view` |
|
||||
| `/api/v1/billing/reports/insurance-debt` | `payments.view` |
|
||||
| `/api/v1/doctor-services` | `services.view` |
|
||||
| `/api/v1/insurances` | `insurances.view` |
|
||||
|
||||
**۲. ریسک:** همان الگوی یافتهٔ ۱، در مقیاس بزرگتر: منشیای که توگلِ «نوبتها» یا «پرداختها»
|
||||
برایش بسته است، با درخواست مستقیم به API همان داده را میگیرد. توگل فقط دکمه را در پنل پنهان
|
||||
میکند.
|
||||
|
||||
**۳. چرا در DB تست خالی به نظر میرسد:** tenantِ تستی داده ندارد، پس پاسخ `{"data":[]}` است. این
|
||||
دلیل امنبودن نیست — در tenant واقعی دادهٔ واقعی برمیگردد.
|
||||
|
||||
**۴. وضعیت: باز، ولی مهارشده.** رفع نشد چون هر سه کنترلرِ درگیر
|
||||
(`BillingController`، `MyAppointmentsController`، `DoctorServiceController`) **هیچ** checker
|
||||
مجوزی تزریقشده ندارند؛ بستنشان بدون دانستنِ نیازِ واقعیِ پنل ریسکِ شکستنِ صفحه دارد — همانطور که
|
||||
`docs/api/` برای فهرست منابع مستند کرده که `appointments.view` عمداً درش را باز میکند. این
|
||||
تصمیم باید با دیدنِ مصرفِ واقعیِ پنل گرفته شود، نه در یک پاس امنیتی.
|
||||
|
||||
بهجایش در `ApiLeastPrivilegeTest::KNOWN_GAPS` ثبت شدند. نقش آن فهرست مثل baseline است: تست اجازه
|
||||
میدهد همین ده تا ۲۰۰ بدهند، ولی **بزرگترشدنش** را نمیپذیرد. یک تست دوم هم هست که اگر گَپی بسته
|
||||
شد، قرمز میشود تا ردیفش از فهرست حذف شود — وگرنه baseline برای همیشه میماند و کسی نمیفهمد بدهی
|
||||
تسویه شده.
|
||||
|
||||
---
|
||||
|
||||
@@ -376,15 +504,34 @@ x-frame-options: DENY
|
||||
|
||||
```
|
||||
src/Treatment/Controller/TreatmentProtocolController.php گیت services.view / services.update
|
||||
tests/Secretary/SecretaryResourceEnforcementTest.php ۴ تست رگرسیون
|
||||
src/Blog/Service/BlogBodySanitizer.php سرویس پاکسازی بدنهٔ مقاله (جدید)
|
||||
src/Blog/Controller/BlogController.php پاکسازی در ساخت و ویرایش
|
||||
src/Blog/Controller/RepresentationBlogController.php پاکسازی در ساخت و ویرایش
|
||||
src/Payment/Gateway/MellatGateway.php اعتبارنامهٔ sandbox از env
|
||||
config/packages/html_sanitizer.yaml سیاست blog.body (جدید)
|
||||
config/services.yaml سه env تازهٔ MELLAT_SANDBOX_*
|
||||
package.json react-router@8، override lodash-es
|
||||
assets/admin/** ۸۱ فایل: react-router-dom → react-router
|
||||
.env / .env.test مقادیر نمایشی با توضیح صریح
|
||||
tests/Secretary/SecretaryResourceEnforcementTest.php ۴ تست رگرسیون پروتکل درمان
|
||||
tests/Blog/BlogBodySanitizerTest.php ۵ تست XSS (جدید)
|
||||
tests/Shared/ApiLeastPrivilegeTest.php تور ایمنی ساختاری + baseline (جدید)
|
||||
docs/api/treatment.md مجوز هر سه اندپوینت
|
||||
.claude/skills/qa-clinicpro/driver.mjs ماتریس نقشهای واقعی
|
||||
../.claude/skills/symfony-security-audit/driver.mjs ماتریس نقشها + پرسوناهای staff و multirole
|
||||
```
|
||||
|
||||
**تستها:** `ddev exec php bin/phpunit` → ۱۵۴۸ تست، ۴۷۹۶ assertion، سبز (۱۹ PHPUnit notice، همه
|
||||
**تستها:** `ddev exec php bin/phpunit` → ۱۵۵۵ تست، ۴۸۳۲ assertion، سبز (۱۹ PHPUnit notice، همه
|
||||
از قبل موجود).
|
||||
|
||||
**یک درسِ جانبی از خودِ تست ساختاری:** اولین نسخهاش کلِ سوییت را قرمز کرد — نه خودش، بلکه
|
||||
`TreatmentCaseOpenerTest` که ۲۰۰ تست بعدتر اجرا میشود. علتش این بود که این تست ~۱۳۰ درخواست
|
||||
پشتسرهم میزند و هر درخواست کرنل را دوباره بالا میآورد، پس `$this->em` کهنه میشد و همان نمونه به
|
||||
تست بعدی ارث میرسید. با `resetManager()` در `tearDown` بسته شد — دقیقاً همان دامی که
|
||||
`ApiTestCase::setUp` قبلاً برای «EntityManager is closed» بسته بود.
|
||||
|
||||
**`phpstan`:** ۱۷ خطا، همان ۱۷ تای پیش از این آدیت. هیچکدام در فایلهای لمسشدهٔ این جلسه نیستند.
|
||||
|
||||
**دادهٔ DB:** پروب `DELETE` روی سرویسِ کلینیک ۱ اجرا شد؛ آن سرویس پروتکل نداشت، پس چیزی حذف نشد.
|
||||
تنها پروتکلِ موجود (`service_item_id=15`، کلینیک ۳) دستنخورده است.
|
||||
|
||||
@@ -392,13 +539,13 @@ docs/api/treatment.md مجوز هر سه ا
|
||||
|
||||
## پیشنهادها
|
||||
|
||||
1. **ارتقای `react-router`** — تسک جدا با تست دستی مسیرهای پنل. یافتهٔ ۲.
|
||||
2. **ردیابی `lodash-es`** — کدام dependency آن را میکشد؛ اگر transitive است، override.
|
||||
3. **sanitize سمت سرور برای بدنهٔ بلاگ** — یافتهٔ ۵ را مستقل از CKEditor میبندد.
|
||||
4. **یک `ClinicStaff` در tenant دوم** — تا آدیت بعدی بتواند IDOR بیننقشیِ پرسنل را واقعاً بزند.
|
||||
5. **تست ساختاری برای گیت کنترلرها** — چیزی شبیه `TenantSchemaCoverageTest` که کنترلر جدیدِ بدون
|
||||
گیت مجوز را قرمز کند. یافتهٔ ۱ دقیقاً از همین شکاف آمد: `#[IsGranted('IS_AUTHENTICATED_FULLY')]`
|
||||
سطح-کلاس، درایور را راضی میکند ولی هیچ مجوزی را enforce نمیکند.
|
||||
1. **بستنِ ده گَپِ یافتهٔ ۸** — با دیدنِ مصرفِ واقعیِ پنل تصمیم بگیر هر کدام کدام مجوز را بخواهد،
|
||||
بعد ردیفش را از `KNOWN_GAPS` بردار. تست دوم خودش یادآوری میکند.
|
||||
2. **مهاجرت CKEditor** به پکیج umbrella `ckeditor5` v45+ — تنها راهِ بستنِ یافتهٔ ۴.
|
||||
3. **یک `ClinicStaff` در tenant دوم** — تا آدیت بعدی بتواند IDOR بیننقشیِ پرسنل را واقعاً بزند.
|
||||
4. **گسترشِ `ApiLeastPrivilegeTest` به روتهای نوشتنی** — الان فقط `GET` بدون path parameter را
|
||||
پوشش میدهد. `POST`/`PATCH`/`DELETE` بدنهٔ معتبر میخواهند، ولی همانها خطرناکترند.
|
||||
5. **پاکسازیِ ۱۷ خطای `phpstan`** — بدهیِ قدیمی، بیربط به امنیت، ولی مانعِ «سبز یعنی سبز» است.
|
||||
|
||||
---
|
||||
|
||||
|
||||
Reference in New Issue
Block a user