Skip to content

fix: security hardening, bug fixes, and PostgreSQL compatibility#843

Open
onesyue wants to merge 1 commit intocedar2025:masterfrom
onesyue:fix/bugs-security-pg
Open

fix: security hardening, bug fixes, and PostgreSQL compatibility#843
onesyue wants to merge 1 commit intocedar2025:masterfrom
onesyue:fix/bugs-security-pg

Conversation

@onesyue
Copy link
Copy Markdown

@onesyue onesyue commented Mar 30, 2026

Summary

A batch of security, correctness, and PostgreSQL compatibility fixes found during code audit.

Security Fixes

1. SQL injection via admin filter/sort field names
Admin table endpoints (UserController, OrderController, CouponController, TicketController) accept user-controlled filter[].id / sort[].id as column names. orderBy() column names are NOT parameterized by Laravel — an attacker with admin access could inject SQL expressions. Added isValidFieldName() regex validation (/^[a-zA-Z_][a-zA-Z0-9_.]*$/) to QueryOperators trait.

2. ZipSlip in theme upload (ThemeService.php)
$zip->extractTo() without validating entry paths allows path traversal via filenames like ../../etc/cron.d/evil. Added .. detection before extraction.

3. Timing-safe token comparison (Middleware/Server.php)
Changed $value !== admin_setting('server_token') to hash_equals() to prevent timing attacks on the server communication token.

4. Hardcoded APP_KEY in .env.example
Removed the pre-set APP_KEY value. Deployments that skip php artisan key:generate would share a known encryption key.

Bug Fixes

5. Wrong constant in order event dispatch (OrderService.php:134)

// Before: uses STATUS_PROCESSING (status constant) to match order TYPE
Order::STATUS_PROCESSING => admin_setting('new_order_event_id', 0),
// After: correct constant
Order::TYPE_NEW_PURCHASE => admin_setting('new_order_event_id', 0),

Both constants happen to equal 1, so no behavioral change — but semantically wrong and fragile.

6. where('plan_id', NULL) never matches (UserService.php)
SQL WHERE plan_id = NULL is always false — must use IS NULL. Changed to whereNull('plan_id'). Same fix for expired_at.

7. Coupon race condition (CouponService.php)
lockForUpdate() was called in the constructor (outside any transaction), so the lock was immediately released. Two concurrent requests could both read limit_use = 1, both succeed, and over-consume the coupon. Moved the lock inside a DB::transaction() in the use() method.

8. Balance race condition (UserService::addBalance())
Same pattern — lockForUpdate() without a wrapping transaction. Wrapped in DB::transaction().

9. Duplicate index migration fails (2025_01_15_000002)
v2_stat_server.server_id and record_at indexes already exist from the initial migration. Running this migration on an existing database throws "duplicate index" errors. Added Schema::hasIndex() check before each $table->index().

PostgreSQL Compatibility

10. Boolean column comparisons (8 files)
PostgreSQL boolean columns reject integer comparisons (WHERE banned = 0 → error). Changed all where('banned', 0) to where('banned', false) and where('show', 1) to where('show', true). These changes are backward-compatible with MySQL.

Files Changed (20 files, +124 -73)

Category Files
Security QueryOperators.php, UserController, OrderController, CouponController, TicketController, Server.php middleware, ThemeService.php, .env.example
Bug fixes OrderService.php, UserService.php, CouponService.php, migration file
PG compat KnowledgeController, OrderController, CheckTrafficExceeded, ResetTraffic, PlanObserver, ServerRouteObserver, ServerService, TrafficResetService

Test Plan

  • All boolean fixes verified on production PostgreSQL deployment
  • Field name validation regex tested — allows column_name, relation.column, rejects ; DROP TABLE, column--, 1 OR 1=1
  • MySQL deployments unaffected (boolean true/false works on MySQL)
  • Coupon concurrent usage should now be properly serialized

🤖 Generated with Claude Code

Security:
- Validate filter/sort field names in admin controllers to prevent
  SQL injection via user-controlled column names in orderBy/where
- Add ZipSlip protection in theme upload (reject paths with "..")
- Use hash_equals() for timing-safe server token comparison
- Remove hardcoded APP_KEY from .env.example

Bug fixes:
- Fix wrong constant: Order::STATUS_PROCESSING → TYPE_NEW_PURCHASE
  in OrderService event dispatch (values happen to match, but
  semantically incorrect)
- Fix where('plan_id', NULL) → whereNull('plan_id') — SQL
  "col = NULL" never matches, must use "col IS NULL"
- Wrap CouponService::use() in DB::transaction() so lockForUpdate()
  actually holds the lock (prevents coupon race condition)
- Wrap UserService::addBalance() in DB::transaction() for the same
  reason (prevents balance race condition)
- Fix duplicate index creation in migration by checking existence
  before adding

PostgreSQL compatibility:
- Replace where('banned', 0) with where('banned', false) and
  where('show', 1) with where('show', true) across all files —
  PostgreSQL boolean columns reject integer comparisons
@onesyue onesyue force-pushed the fix/bugs-security-pg branch from af72f5f to b98c06e Compare April 11, 2026 21:46
@onesyue
Copy link
Copy Markdown
Author

onesyue commented Apr 11, 2026

@cedar2025 友情提醒:此 PR 已重新 rebase 至最新 master,并新增了今天发现的一处遗漏。

本次更新内容

新增 app/Services/TelegramService.php 修复:

- fn($q) => $q->where('is_admin', 1)
-     ->when($isStaff, fn($q) => $q->orWhere('is_staff', 1))
+ fn($q) => $q->where('is_admin', true)
+     ->when($isStaff, fn($q) => $q->orWhere('is_staff', true))

is_admin / is_staff 在 schema 里是 boolean 列,PG 严格要求 boolean 比较。where('is_admin', 1) 在 PG 上会抛 SQLSTATE[42883]: undefined function: operator does not exist: boolean = integer,导致 sendMessageWithAdmin()(管理员通知)静默失败。MySQL 因为隐式类型转换不受影响。

本 PR 总共修复:

  • SQL 注入防护(QueryOperators::isValidFieldName + 4 个 admin Controller 调用)
  • 时间侧信道(Server middleware 用 hash_equals 替代 !==
  • ZipSlip 路径穿越(ThemeService)
  • addBalance/CouponService 事务并发安全
  • 10+ 处 PG boolean = int 不兼容
  • OrderService 新订单事件类型修复

恳请审查 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant