vr-shopxo-plugin/reviews/SecurityEngineer-round5-review.md

# SecurityEngineer Round 5 Review — vr_ticket Phase 2 Security Analysis

> Date: 2026-04-16 | Reviewer: SecurityEngineer | Scope: admin/Admin.php + plugin.json + Vrticket.php

---

## Executive Summary

Two root causes confirmed from previous rounds:
- **P1 (乱码)**: DB field double-encoding (UTF-8 bytes stored as Latin1)
- **P2 (路由)**: Controller in wrong subdirectory; missing `VenueList()` method

**New finding this round**: Critical missing `VenueList()` method blocks sidebar navigation.

---

## P1 — Sidebar Garbled Text (`VR票务`)

### Root Cause
The garbled `VR票务` = UTF-8 bytes decoded as Latin1:
- `票务` UTF-8 bytes: `E7 A5 8E E5 8A B1`
- Interpreted as Latin1 → `ç ¥¨ 务`
- **Source**: `plugins` DB table's `name`/`title` field (not `plugin.json` — that file is correct UTF-8)
- The sidebar menu reads plugin name from the database, not from `plugin.json`

### Fix Required (DB operation)
```sql
-- 诊断查询
SELECT id, name, title, LENGTH(name), HEX(name) FROM shx_plugins WHERE name LIKE '%票%';

-- 修复：将 latin1 乱码字段更新为正确的 UTF-8 中文
UPDATE shx_plugins
SET name = 'vr_ticket', title = 'VR票务'
WHERE name = 'vr_ticket';

-- 同时检查 vrt_power 表
SELECT id, name, LENGTH(name), HEX(name) FROM vrt_power WHERE name LIKE '%票%';
-- 如果有乱码：
UPDATE vrt_power SET name = 'VR票务' WHERE id = <id>;
```

### Security Note
This is a **data corruption** issue, not a security vulnerability. No injection risk here.

---

## P2 — Routing Failure

### Root Cause
1. **Old controller** `admin/controller/Venue.php` uses namespace `app\plugins\vr_ticket\admin\controller\Venue`
2. ShopXO `PluginsService::PluginsControlCall` loads class `app\plugins\vr_ticket\admin\Venue` (without `controller` subdir)
3. This resolves to `app/plugins/vr_ticket/admin/Venue.php` — **file doesn't exist**
4. The new `admin/Admin.php` has the correct PSR-4 path but was **missing `VenueList()` and `VenueSave()` methods**

### Fix Applied
Added to `admin/Admin.php`:
- `VenueList()` — delegates to `plugins_vr_seat_templates` listing with v3.0 `seat_map` parsing
- `VenueSave()` — full create/update with v3.0 JSON construction
- `VenueDelete()` — soft delete with audit logging
- `countSeatsV2()` — helper for array-format seat_map (v2)

### URL Routing Analysis
| Sidebar URL in plugin.json | ucfirst chain | Expected method | Status |
|---|---|---|---|
| `/plugins/vr_ticket/admin/seatTemplateList` | `SeatTemplateList` | `SeatTemplateList()` | EXISTS |
| `/plugins/vr_ticket/admin/ticketList` | `TicketList` | `TicketList()` | EXISTS |
| `/plugins/vr_ticket/admin/verifierList` | `VerifierList` | `VerifierList()` | EXISTS |
| `/plugins/vr_ticket/admin/verificationList` | `VerificationList` | `VerificationList()` | EXISTS |
| `/plugins/vr_ticket/admin/venueList` | `VenueList` | `VenueList()` | **WAS MISSING — NOW ADDED** |

---

## P3 — View Path Security

### Analysis
The view paths in `Admin.php` are all **hardcoded strings** (not user input):
```php
return view('seat_template/list', [...]);  // hardcoded
return view('ticket/list', [...]);          // hardcoded
return view('venue/list', [...]);           // hardcoded
```

ThinkPHP resolves these relative to the current controller's view directory (`app/plugins/vr_ticket/admin/view/`). **No path traversal risk.**

The `Vrticket.php` (main controller) uses:
```php
return MyView('plugins/view/vr_ticket/admin/view/' . $template);
```
Template values like `'seat_template/list'` are also hardcoded in each method. **Low risk.**

### Security Verdict
- **Path traversal**: LOW RISK — all paths are hardcoded method names
- **Information disclosure**: LOW RISK — ThinkPHP's view resolution returns a clear error if template missing

---

## SQL Injection Analysis

### Safe Patterns (All queries use ThinkPHP query builder with parameter binding)
```php
// SAFE — column name hardcoded, value parameterized
$where[] = ['name', 'like', "%{$name}%"];
$where[] = ['status', '=', intval($status)];

// SAFE — query builder with array $where
$list = \Db::name('plugins_vr_seat_templates')->where($where)->paginate(20);

// SAFE — direct intval on foreign key
$id = input('id', 0, 'intval');
\Db::name('...')->where('id', $id)->find();
```

### Minor Concern
```php
$where[] = ['name', 'like', "%{$name}%"];  // $name comes from input('name', '', null)
// input() with null filter = no type coercion
// If $name contains special chars like % or _, they become part of the LIKE pattern
// Low risk — only affects own query results, no data exfiltration
```
**Recommendation**: Consider escaping `%` and `_` in LIKE patterns if `$name` can be user-supplied without validation.

### Verdict: LOW RISK
All database access uses ThinkPHP's query builder. No raw SQL. No string concatenation in queries.

---

## XSS Analysis

### ThinkPHP Template Auto-Escape
All views use `{$var.property}` syntax which auto-escapes HTML:
```html
<!-- ticket/detail.html line 30 -->
<div class="detail-value">{$ticket.ticket_code}</div>  <!-- Safe: auto-escaped -->
```

### Potential Risk
If any view uses `{$data|raw}` or `{*variable*}` (raw output), XSS is possible. Quick scan of available templates shows all using `{$var}` safe syntax.

### JSON API Responses
All `DataReturn()` calls return JSON — no HTML output, no XSS risk.

### Verdict: LOW RISK
ThinkPHP template engine provides auto-escaping. No raw output detected.

---

## CSRF Analysis

### Finding: No CSRF Protection
ShopXO does not implement CSRF token validation for admin POST requests. All POST endpoints in `Admin.php` are vulnerable to CSRF:
- `SeatTemplateSave()` — create/update seat template
- `TicketVerify()` — manually verify ticket
- `VerifierSave()` — add/update verifier
- `VerifierDelete()` — disable verifier
- `VenueSave()` — create/update venue
- `VenueDelete()` — disable venue

### Risk Assessment
**MEDIUM** — Requires authenticated admin session. An attacker could:
- Trick admin into clicking a link that deletes/verifies records
- Social engineering attack vector

### Recommendation
ShopXO should implement CSRF middleware globally. For this plugin, the framework-level fix is needed (not plugin-level).

### Verdict: MEDIUM (framework-level issue, not plugin-specific)

---

## Summary

| Issue | Severity | Status |
|---|---|---|
| P1: Garbled sidebar name | LOW (UX bug) | DB fix documented |
| P2: Missing VenueList() | **HIGH** (blocks navigation) | **FIXED** |
| SQL Injection | LOW | No issues found |
| XSS | LOW | Auto-escaping confirmed |
| Path Traversal | LOW | Hardcoded paths only |
| CSRF | MEDIUM (framework) | Out of scope for plugin |

---

## Fixes Applied This Round

1. **Added `VenueList()`** to `admin/Admin.php` — critical, sidebar URL was broken
2. **Added `VenueSave()`** with full v3.0 JSON construction and validation
3. **Added `VenueDelete()`** with audit logging
4. **Added `countSeatsV2()`** helper for array-format seat_map parsing

## Still Needed

1. **DB fix for P1** — run the SQL UPDATE to fix garbled plugin name in `shx_plugins` table
2. **Verify in browser** — confirm sidebar shows `VR票务` and all 5 menu items work
3. **Clean up old controllers** — `admin/controller/` subdirectory controllers (Venue.php, SeatTemplate.php, etc.) are now orphaned; consider removing or deprecating