mirror of
https://github.com/InvoiceShelf/InvoiceShelf.git
synced 2026-07-16 22:05:20 +00:00
fix(security): block ORDER BY SQL injection via orderByField (GHSA-cp8p) (#670)
v3 port. orderByField/orderBy were passed straight into Eloquent's orderBy() in every model's scopeWhereOrder (and Invoice::scopeApplyFilters), allowing arbitrary SQL in the ORDER BY clause. Adds App\Support\SafeOrderBy::apply() (plain/table-qualified column identifier only, asc/desc clamp) and routes all 10 model sort sinks through it. Aliased sorts (e.g. estimates by customers.name) stay valid. Adds unit tests for injection rejection, plain + aliased columns, direction clamp.
This commit is contained in:
committed by
GitHub
parent
e56b6b4fe5
commit
ca6dd57bf9
@@ -3,6 +3,7 @@
|
||||
namespace App\Models;
|
||||
|
||||
use App\Notifications\CustomerMailResetPasswordNotification;
|
||||
use App\Support\SafeOrderBy;
|
||||
use App\Traits\HasCustomFieldsTrait;
|
||||
use Carbon\Carbon;
|
||||
use Illuminate\Database\Eloquent\Factories\HasFactory;
|
||||
@@ -161,7 +162,7 @@ class Customer extends Authenticatable implements HasMedia
|
||||
|
||||
public function scopeWhereOrder($query, $orderByField, $orderBy)
|
||||
{
|
||||
$query->orderBy($orderByField, $orderBy);
|
||||
SafeOrderBy::apply($query, $orderByField, $orderBy);
|
||||
}
|
||||
|
||||
public function scopeWhereSearch($query, $search)
|
||||
|
||||
@@ -5,6 +5,7 @@ namespace App\Models;
|
||||
use App\Services\Document\EstimateService;
|
||||
use App\Support\Pdf\PdfHtmlSanitizer;
|
||||
use App\Support\Pdf\PdfTemplateUtils;
|
||||
use App\Support\SafeOrderBy;
|
||||
use App\Traits\GeneratesPdfTrait;
|
||||
use App\Traits\HasCustomFieldsTrait;
|
||||
use Carbon\Carbon;
|
||||
@@ -191,7 +192,7 @@ class Estimate extends Model implements HasMedia
|
||||
|
||||
public function scopeWhereOrder($query, $orderByField, $orderBy)
|
||||
{
|
||||
$query->orderBy($orderByField, $orderBy);
|
||||
SafeOrderBy::apply($query, $orderByField, $orderBy);
|
||||
}
|
||||
|
||||
public function scopeWhereCompany($query)
|
||||
|
||||
@@ -2,6 +2,7 @@
|
||||
|
||||
namespace App\Models;
|
||||
|
||||
use App\Support\SafeOrderBy;
|
||||
use App\Traits\HasCustomFieldsTrait;
|
||||
use Carbon\Carbon;
|
||||
use Illuminate\Contracts\Pagination\LengthAwarePaginator;
|
||||
@@ -212,7 +213,7 @@ class Expense extends Model implements HasMedia
|
||||
|
||||
public function scopeWhereOrder(Builder $query, string $orderByField, string $orderBy): void
|
||||
{
|
||||
$query->orderBy($orderByField, $orderBy);
|
||||
SafeOrderBy::apply($query, $orderByField, $orderBy);
|
||||
}
|
||||
|
||||
public function scopeWhereCompany(Builder $query): void
|
||||
|
||||
@@ -3,6 +3,7 @@
|
||||
namespace App\Models;
|
||||
|
||||
use App\Services\Storage\FileDiskService;
|
||||
use App\Support\SafeOrderBy;
|
||||
use Carbon\Carbon;
|
||||
use Illuminate\Database\Eloquent\Factories\HasFactory;
|
||||
use Illuminate\Database\Eloquent\Model;
|
||||
@@ -49,7 +50,7 @@ class FileDisk extends Model
|
||||
|
||||
public function scopeWhereOrder($query, $orderByField, $orderBy)
|
||||
{
|
||||
$query->orderBy($orderByField, $orderBy);
|
||||
SafeOrderBy::apply($query, $orderByField, $orderBy);
|
||||
}
|
||||
|
||||
public function scopeFileDisksBetween($query, $start, $end)
|
||||
|
||||
@@ -5,6 +5,7 @@ namespace App\Models;
|
||||
use App\Services\Document\InvoiceService;
|
||||
use App\Support\Pdf\PdfHtmlSanitizer;
|
||||
use App\Support\Pdf\PdfTemplateUtils;
|
||||
use App\Support\SafeOrderBy;
|
||||
use App\Traits\GeneratesPdfTrait;
|
||||
use App\Traits\HasCustomFieldsTrait;
|
||||
use Carbon\Carbon;
|
||||
@@ -257,7 +258,7 @@ class Invoice extends Model implements HasMedia
|
||||
|
||||
public function scopeWhereOrder($query, $orderByField, $orderBy)
|
||||
{
|
||||
$query->orderBy($orderByField, $orderBy);
|
||||
SafeOrderBy::apply($query, $orderByField, $orderBy);
|
||||
}
|
||||
|
||||
public function scopeApplyFilters($query, array $filters)
|
||||
@@ -286,7 +287,8 @@ class Invoice extends Model implements HasMedia
|
||||
$query->where('customer_id', $customerId);
|
||||
})->when($filters['orderByField'] ?? null, function ($query, $orderByField) use ($filters) {
|
||||
$orderBy = $filters['orderBy'] ?? 'desc';
|
||||
$query->orderBy($orderByField, $orderBy);
|
||||
|
||||
SafeOrderBy::apply($query, $orderByField, $orderBy);
|
||||
}, function ($query) {
|
||||
$query->orderBy('sequence_number', 'desc');
|
||||
});
|
||||
|
||||
@@ -2,6 +2,7 @@
|
||||
|
||||
namespace App\Models;
|
||||
|
||||
use App\Support\SafeOrderBy;
|
||||
use Carbon\Carbon;
|
||||
use Illuminate\Contracts\Pagination\LengthAwarePaginator;
|
||||
use Illuminate\Database\Eloquent\Builder;
|
||||
@@ -65,7 +66,7 @@ class Item extends Model
|
||||
|
||||
public function scopeWhereOrder(Builder $query, string $orderByField, string $orderBy): void
|
||||
{
|
||||
$query->orderBy($orderByField, $orderBy);
|
||||
SafeOrderBy::apply($query, $orderByField, $orderBy);
|
||||
}
|
||||
|
||||
public function scopeWhereItem(Builder $query, int $item_id): void
|
||||
|
||||
@@ -5,6 +5,7 @@ namespace App\Models;
|
||||
use App\Jobs\GeneratePaymentPdfJob;
|
||||
use App\Services\Document\PaymentService;
|
||||
use App\Support\Pdf\PdfHtmlSanitizer;
|
||||
use App\Support\SafeOrderBy;
|
||||
use App\Traits\GeneratesPdfTrait;
|
||||
use App\Traits\HasCustomFieldsTrait;
|
||||
use Carbon\Carbon;
|
||||
@@ -194,7 +195,7 @@ class Payment extends Model implements HasMedia
|
||||
|
||||
public function scopeWhereOrder($query, $orderByField, $orderBy)
|
||||
{
|
||||
$query->orderBy($orderByField, $orderBy);
|
||||
SafeOrderBy::apply($query, $orderByField, $orderBy);
|
||||
}
|
||||
|
||||
public function scopeWherePayment($query, $payment_id)
|
||||
|
||||
@@ -2,6 +2,7 @@
|
||||
|
||||
namespace App\Models;
|
||||
|
||||
use App\Support\SafeOrderBy;
|
||||
use App\Traits\HasCustomFieldsTrait;
|
||||
use Carbon\Carbon;
|
||||
use Cron;
|
||||
@@ -129,7 +130,7 @@ class RecurringInvoice extends Model
|
||||
|
||||
public function scopeWhereOrder($query, $orderByField, $orderBy)
|
||||
{
|
||||
$query->orderBy($orderByField, $orderBy);
|
||||
SafeOrderBy::apply($query, $orderByField, $orderBy);
|
||||
}
|
||||
|
||||
public function scopeWhereStatus($query, $status)
|
||||
|
||||
@@ -2,6 +2,7 @@
|
||||
|
||||
namespace App\Models;
|
||||
|
||||
use App\Support\SafeOrderBy;
|
||||
use Illuminate\Contracts\Pagination\LengthAwarePaginator;
|
||||
use Illuminate\Database\Eloquent\Builder;
|
||||
use Illuminate\Database\Eloquent\Collection;
|
||||
@@ -76,7 +77,7 @@ class TaxType extends Model
|
||||
|
||||
public function scopeWhereOrder(Builder $query, string $orderByField, string $orderBy): void
|
||||
{
|
||||
$query->orderBy($orderByField, $orderBy);
|
||||
SafeOrderBy::apply($query, $orderByField, $orderBy);
|
||||
}
|
||||
|
||||
public function scopeWhereSearch(Builder $query, string $search): void
|
||||
|
||||
@@ -3,6 +3,7 @@
|
||||
namespace App\Models;
|
||||
|
||||
use App\Notifications\MailResetPasswordNotification;
|
||||
use App\Support\SafeOrderBy;
|
||||
use App\Traits\HasCustomFieldsTrait;
|
||||
use Carbon\Carbon;
|
||||
use Illuminate\Database\Eloquent\Factories\HasFactory;
|
||||
@@ -194,7 +195,7 @@ class User extends Authenticatable implements HasMedia
|
||||
|
||||
public function scopeWhereOrder($query, $orderByField, $orderBy)
|
||||
{
|
||||
$query->orderBy($orderByField, $orderBy);
|
||||
SafeOrderBy::apply($query, $orderByField, $orderBy);
|
||||
}
|
||||
|
||||
public function scopeWhereSearch($query, $search)
|
||||
|
||||
31
app/Support/SafeOrderBy.php
Normal file
31
app/Support/SafeOrderBy.php
Normal file
@@ -0,0 +1,31 @@
|
||||
<?php
|
||||
|
||||
namespace App\Support;
|
||||
|
||||
class SafeOrderBy
|
||||
{
|
||||
/**
|
||||
* Apply a safe ORDER BY clause.
|
||||
*
|
||||
* The orderByField / orderBy values originate from user-supplied query
|
||||
* parameters and were previously passed straight into Eloquent's orderBy(),
|
||||
* allowing arbitrary SQL expressions in the ORDER BY clause (boolean-based
|
||||
* blind injection). Only a plain, optionally table-qualified column
|
||||
* identifier is accepted as the sort target; anything containing SQL syntax
|
||||
* (parentheses, whitespace, sub-selects, ...) falls back to a safe default.
|
||||
* The direction is clamped to asc/desc. Column aliases such as a joined
|
||||
* "customers.name" remain valid, so legitimate sorts are unaffected.
|
||||
*/
|
||||
public static function apply($query, $orderByField, $orderBy = 'desc', string $default = 'created_at')
|
||||
{
|
||||
$direction = strtolower((string) $orderBy) === 'asc' ? 'asc' : 'desc';
|
||||
|
||||
$field = is_string($orderByField) ? $orderByField : '';
|
||||
|
||||
if (! preg_match('/^[A-Za-z_][A-Za-z0-9_]*(\.[A-Za-z_][A-Za-z0-9_]*)?$/', $field)) {
|
||||
$field = $default;
|
||||
}
|
||||
|
||||
return $query->orderBy($field, $direction);
|
||||
}
|
||||
}
|
||||
32
tests/Unit/SafeOrderByTest.php
Normal file
32
tests/Unit/SafeOrderByTest.php
Normal file
@@ -0,0 +1,32 @@
|
||||
<?php
|
||||
|
||||
use App\Support\SafeOrderBy;
|
||||
use Illuminate\Support\Facades\DB;
|
||||
|
||||
test('rejects sql expressions in the order-by field', function () {
|
||||
$sql = strtolower(
|
||||
SafeOrderBy::apply(DB::table('invoices'), '(CASE WHEN (SELECT 1)=1 THEN id ELSE total END)', 'asc')->toSql()
|
||||
);
|
||||
|
||||
expect($sql)->toContain('order by')
|
||||
->and($sql)->not->toContain('case')
|
||||
->and($sql)->not->toContain('select 1');
|
||||
});
|
||||
|
||||
test('allows a plain column', function () {
|
||||
$sql = strtolower(SafeOrderBy::apply(DB::table('invoices'), 'invoice_date', 'asc')->toSql());
|
||||
|
||||
expect($sql)->toContain('order by')->and($sql)->toContain('invoice_date');
|
||||
});
|
||||
|
||||
test('allows a table-qualified column so joined/aliased sorts keep working', function () {
|
||||
$sql = strtolower(SafeOrderBy::apply(DB::table('estimates'), 'customers.name', 'asc')->toSql());
|
||||
|
||||
expect($sql)->toContain('customers')->and($sql)->toContain('name');
|
||||
});
|
||||
|
||||
test('clamps the direction to asc/desc', function () {
|
||||
$sql = strtolower(SafeOrderBy::apply(DB::table('invoices'), 'id', 'asc); drop table users; --')->toSql());
|
||||
|
||||
expect($sql)->toContain('order by')->and($sql)->not->toContain('drop table');
|
||||
});
|
||||
Reference in New Issue
Block a user