86 lines
11 KiB
Markdown
86 lines
11 KiB
Markdown
# 21 — تدقيق سيرو: العيوب الأمنية والمشاكل المرصودة
|
||
|
||
> سجلّ **قابل للمعالجة** لكل ما رُصد في كود سيرو الحيّ (`~/development/App/Siro`) أثناء بنائنا لـTripz. الغرض مزدوج: (1) ألّا نكرّرها في Tripz، (2) قائمة إصلاحات جاهزة لسيرو نفسه حين نعالجه.
|
||
>
|
||
> **كل ما يلي في سيرو (PHP/MySQL) لا في Tripz.** حيث أصلحنا المكافئ في Tripz، أُشير إليه. الخطورة: 🔴 حرجة · 🟠 متوسطة · 🟡 منخفضة/جودة.
|
||
|
||
آخر تحديث: 2026-07-24 (S18 تُصحّح: ليست عيباً، هي سلوك صحيح).
|
||
|
||
---
|
||
|
||
## المدفوعات والسحب (`payment_server/` · `request_payout.php` · `finalizePayout`)
|
||
|
||
| # | الخطورة | العيب | الإصلاح المقترح لسيرو | حالنا في Tripz |
|
||
|---|---------|-------|----------------------|----------------|
|
||
| S1 | 🔴 | **IDOR في السحب**: `request_payout.php` يأخذ `driverId`/`phone` من **جسم الطلب** لا من التوكن — سائق يسحب رصيد غيره إلى هاتفه. | اشتقاق هوية السائق من JWT الموقَّع حصراً. | ✅ نظيف — `payouts.request` يستعمل `user.userId` من التوكن. |
|
||
| S2 | 🔴 | **لا حجز رصيد عند طلب السحب** → طلبات متزامنة تمرّ كلها = double-spend. | حجز ذرّي عند التأكيد قبل أي تحويل. | ✅ خصم ذرّي عند `confirm` (I1/I4). |
|
||
| S3 | 🔴 | **رسم 3500 يناقض نفسه**: الفحص يتطلب `amount + 3500` والإنهاء يخصم `amount - 3500` = تسرّب مال. الرقم مكتوب يدوياً مرّتين. | ثابت واحد + اختبار يطابق الفحص بالخصم. | ✅ لا رسم مكرّر؛ العمولة من الرصيد التشغيلي (docs/18). |
|
||
| S4 | 🔴 | **`finalizePayout` يعمل 5 كتابات بلا معاملة/تراجع** → فشل في المنتصف يترك سحباً نصف مُسوّى. | لفّ الكتابات في transaction واحدة. | ✅ الخصم والقيد في معاملة واحدة (I1). |
|
||
| S5 | 🟠 | **`driverWallet.amount` نوعه `varchar(10)`** — المال مخزَّن نصّاً، و`SUM()` على نصوص. | تحويله `DECIMAL(12,3)`. | ✅ `numeric(12,3)` + `CHECK (balance >= 0)`. |
|
||
| S6 | 🟠 | **لا OTP على السحب** — `phone_verification` موجود لكنه مربوط بالتسجيل/الدخول فقط. | OTP + تحقّق قبل التحويل. | ✅ OTP على السحب عبر المُوزِّع (I4). |
|
||
| S7 | 🟡 | **جدولان متداخلان لنفس المفهوم**: `payout_requests` و`driver_withdrawal_requests`. | توحيدهما في جدول واحد. | ✅ جدول `pay_payouts` واحد. |
|
||
|
||
### ✅ إعادة فحص سيرو — 2026-07-24 (بطلب المالك: «العيوب في المدفوعات عدّلناها»)
|
||
|
||
فُحص الكود مباشرة. **4 من 7 مُصلَحة فعلاً وبجودة جيدة، و3 ما زالت مفتوحة + عيب جديد.**
|
||
|
||
| # | الحالة اليوم في سيرو | الدليل |
|
||
|---|---|---|
|
||
| S1 IDOR | ✅ **مُصلَح** | `request_payout.php:9` → `$driverId = $decodedToken->sub`، والهاتف يُقرأ من `driver` بالـid لا من الطلب |
|
||
| S2 حجز الرصيد | ✅ **مُصلَح** | `beginTransaction` + `SELECT … FOR UPDATE` + حالة `payout_reserved` + `rollBack` |
|
||
| S3 رسم 3500 | ✅ **مُصلَح** | الرقم اختفى من مسار السحب كلياً |
|
||
| S6 OTP على السحب | ✅ **مُصلَح** | `request_payout.php:28,60–72` — OTP إلزامي ويُتحقق من `token_verification_driver` |
|
||
| **S4 معاملة في الإنهاء** | ✅ **مُصلَح (2026-07-24)** | `finalize_payout.php` — الكتابات 2–6 داخل `beginTransaction`/`commit` واحدة، مع `rollBack` في `catch`. فُحصت أثناء الإصلاح: `$encryptionHelper` كان يُستعمل داخل الدالة بلا `global` (متغيّر غير مُعرَّف في نطاق الدالة) → كل استدعاء كان يفشل عند الخطوة 1 قبل أي كتابة. أُصلح بإضافة `global $encryptionHelper;` |
|
||
| **S5 `varchar(10)`** | ✅ **مُصلَح (2026-07-24)** | `driverWallet`/`siroWallet` في `WalletDB.sql` + `seferWallet` (جدول ثالث لم يكن مذكوراً صراحة، وُجد أثناء الإصلاح) في `schema_primary.sql` + `schema_ride.sql` (نسختان) ← `DECIMAL(12,3)`. هجرتان في `payment_server/migrations/` و`backend/migrations/`. **لم يُصلَح:** `backend/ride/seferWallet/add.php:28` يُدخل `$amount` بلا `is_numeric()` — بند منفصل جديد |
|
||
| **S7 الجدولان** | 🟡 **ما زال مفتوحاً** | `driver_withdrawal_requests` قائم بجانب `payout_requests` (`WalletDB.sql:132`) |
|
||
|
||
#### ✅ S18 — ليست مشكلة (توضيح تصميمي، 2026-07-24)
|
||
`finalize_payout.php` الخطوة 6:
|
||
```sql
|
||
UPDATE payments SET isGiven = TRUE WHERE driverID = :driverId AND isGiven = FALSE
|
||
```
|
||
**الملاحظة الأصلية (تُصحّح):** بدا أنه يعلّم كل الدفعات بغضّ النظر عن مبلغ السحب.
|
||
|
||
**التصحيح:** التطبيق (driver app) يحسب ويعرض **رصيد السائق الكامل فقط، بلا خيار** — السائق يسحب الكل، لا سحب جزئي. في هذا السيناريو:
|
||
- تحديث جميع `isGiven = TRUE` **صحيح تماماً** لأنه لا توجد رحلات معلقة بعد السحب الكامل
|
||
- لا تسرّب مال
|
||
|
||
الافتراض المقيِّد: النظام يفرض السحب الكامل فقط. إذا تغيّر هذا لاحقاً (السماح بسحب جزئي)، عندها يجب ربط `payments` بـ `payout_id`.
|
||
|
||
> ✅ **البوابة اجتيزت (2026-07-24):** S4 و S5 مُصلَحان. **بند جديد قبل أول استنساخ:** إضافة `is_numeric($amount)` في `backend/ride/seferWallet/add.php` — لم يكن جزءاً من S4/S5 الأصليين، اكتُشف أثناء إصلاحهما.
|
||
|
||
## المصادقة وكلمة المرور (`auth/`)
|
||
|
||
| # | الخطورة | العيب | الإصلاح المقترح لسيرو | حالنا في Tripz |
|
||
|---|---------|-------|----------------------|----------------|
|
||
| S8 | 🔴 | **مقارنة كلمة مرور نصّية**: `loginUsingCredentialsWithoutGoogle.php` سطر 114 — `password_verify(...) || $password === $data['password']`. الشقّ الثاني يتجاوز الـhash كلياً، ويطابق كلمة المرور المخزَّنة نصّاً لو كانت كذلك. | حذف المقارنة النصّية نهائياً؛ `password_verify` وحده. | ✅ لا كلمة مرور إطلاقاً (D5، قرار المالك) — المسار غير موجود عندنا. |
|
||
| S9 | 🟠 | **كلمة مرور مشتركة ثابتة** `'SiroPassenger2026!'` للحسابات التجريبية في الكود. | نقلها لمتغيّر بيئة على الأقل، والأفضل إزالة المسار. | ✅ لا مسار كلمة مرور. |
|
||
| S10 | 🟡 | **`password = hash($email)` وهمي** عند التسجيل — حقل بلا معنى يوحي بأمان غير موجود. | إزالة العمود إن لم يُستعمل. | ✅ لا عمود كلمة مرور. |
|
||
| S11 | 🟠 | **`user_type` يُؤخذ من الطلب بلا تحقّق توقيع** (تعليقهم صريح: "JWT not trusted without signature verification") — عميل يدّعي `admin`. يُلطَّف جزئياً بقائمة `ADMIN_PHONE_NUMBERS`. | التحقق من توقيع JWT واعتماد الدور منه. | ✅ الدور من التوكن الموقَّع (`RolesGuard`). |
|
||
|
||
## الـOTP (`auth/otp/`)
|
||
|
||
| # | الخطورة | العيب | الإصلاح المقترح لسيرو | حالنا في Tripz |
|
||
|---|---------|-------|----------------------|----------------|
|
||
| S12 | 🟠 | **رمز OTP من 3 خانات** (1000 احتمال فقط) — قابل للتخمين. | 6 خانات + قفل بعد المحاولات. | ✅ 4 خانات (قابل للرفع) + قفل 5 محاولات (D4). عند النقل نرفعه لـ6. |
|
||
| S13 | 🟠 | **مسح جدول كامل وفكّ تشفير كل صف** للتحقق من الرمز (`verify.php`: `SELECT * ... foreach decrypt`) — بطيء ويتسرّب توقيتاً. | فهرس أعمى (blind index) على الهاتف. | ✅ `phone_bidx` (HMAC حتمي) للبحث المباشر (docs/16). |
|
||
| S14 | 🟡 | **مساران متوازيان للـOTP**: `request.php`/`verify.php` (قاعدة، مشفّر) و`OtpService.php` (Redis) — تكرار وتضارب. | توحيد على مسار واحد. | ✅ مسار Redis واحد. |
|
||
| S15 | 🟡 | **`echo` لأخطاء المزوّد الخام** في `sendIntaleqOtp` ("Temporarily echo the raw error") — تسريب تشخيصي في الإنتاج. | إزالة الـecho، لوغ فقط. | ✅ لا echo؛ لوغ خادمي. |
|
||
| S16 | 🟡 | **تخزين OTP مشفَّراً بـGCM في القاعدة** ثم مسح الجدول لفكّه — تكلفة بلا فائدة (الرمز قصير العمر، الأنسب Redis + hash). | Redis + `password_hash` كما في `OtpService`. | ✅ Redis مباشرةً. |
|
||
|
||
## التشفير (`core/Security/EncryptionHelper.php`)
|
||
|
||
| # | الخطورة | العيب | الإصلاح المقترح لسيرو | حالنا في Tripz |
|
||
|---|---------|-------|----------------------|----------------|
|
||
| S17 | 🔴 | **AES-CBC بـIV ثابت** (موثَّق سابقاً في docs/16) — نفس النص ينتج نفس الشيفرة، ويسرّب الأنماط. | AES-256-GCM + IV عشوائي لكل قيمة. | ✅ منفَّذ (docs/16). |
|
||
|
||
## حدّ الطلبات (`core/Auth/RateLimiter.php`) — **جيد، للنسخ لا للإصلاح**
|
||
تصميمه سليم: حدود مسمّاة لكل نوع + **fallback بملف عند تعطّل Redis (fail-closed)**. النقطة الوحيدة: يتتبّع بـ`IP:userId` — جيد، لكن نقاط ما قبل الدخول بالـIP وحده (لا مفرّ). **ليس عيباً — مرجع نتعلّم منه.**
|
||
|
||
## نقاط تحتاج تأكيد المالك (لم أجزم بها)
|
||
- هل `raw_sms_log` + Gemini (تسوية الدفع من رسائل المزوّد) يُدقَّق ضد إعادة الاستعمال (نفس الرسالة تُسوّى مرتين)؟ — يحتاج فحص `webhook_sms/webhook.php` عند معالجة سيرو.
|
||
- هل توكنات Nabeh/مفاتيح Kazumi في متغيّرات بيئة على كل الخوادم أم مكتوبة في مكان ما؟ — يُتحقَّق عند النقل.
|
||
|
||
---
|
||
← ذو صلة: [16-encryption](16-encryption.md) · [17-backend-backlog](17-backend-backlog.md) · [18-driver-credit-commission](18-driver-credit-commission.md)
|