Skip to content

Security hardening across query builder, storage, auth, mail and crypto #430

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
papac merged 1 commit into from
Oct 4, 2026
Merged

Security hardening across query builder, storage, auth, mail and crypto #430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/Auth/Guards/SessionGuard.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ public function attempts(array $credentials, bool $remember = false): bool
$password = $credentials[$fields['password']];

if (Hash::check($password, $user->{$fields['password']})) {
// Rotate the session ID on privilege elevation (anti session
// fixation). regenerate() preserves data, so the stored user below
// lives under the new ID.
$this->getSession()->regenerate();
$this->getSession()->put($this->session_key, $user);

if ($remember) {
Expand Down Expand Up @@ -127,6 +131,10 @@ public function guest(): bool
*/
public function login(Authentication $user, bool $remember = false): bool
{
// Rotate the session ID on privilege elevation (anti session fixation).
// regenerate() preserves data, so the stored user below lives under the
// new ID.
$this->getSession()->regenerate();
$this->getSession()->add($this->session_key, $user);

if ($remember) {
Expand Down
78 changes: 63 additions & 15 deletions src/Database/QueryBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,8 @@ public function where(
);
}

$column = $this->assertSafeIdentifier($column, 'where');

if ($value instanceof QueryBuilder) {
$indicator = "(" . $value->toSql() . ")";
} else {
Expand Down Expand Up @@ -429,11 +431,17 @@ private static function isComparisonOperator(mixed $comparator): bool
* @return string
* @throws QueryBuilderException
*/
private static function assertSafeIdentifier(string $identifier, string $clause): string
private function assertSafeIdentifier(string $identifier, string $clause, bool $allowWildcard = false): string
{
$trimmed = trim($identifier);

if (!preg_match('/^[A-Za-z_][A-Za-z0-9_]*(\.[A-Za-z_][A-Za-z0-9_]*)?$/', $trimmed)) {
// Aggregates and SELECT lists may legitimately use "*" (and
// "table.*"); WHERE/JOIN/ORDER BY/GROUP BY identifiers may not.
$pattern = $allowWildcard
? '/^(\*|[A-Za-z_][A-Za-z0-9_]*(\.(\*|[A-Za-z_][A-Za-z0-9_]*))?)$/'
: '/^[A-Za-z_][A-Za-z0-9_]*(\.[A-Za-z_][A-Za-z0-9_]*)?$/';

if (!preg_match($pattern, $trimmed)) {
throw new QueryBuilderException(
"Unsafe identifier passed to {$clause}: [{$identifier}]. "
. "Only a plain or table-qualified column name is allowed."
Expand Down Expand Up @@ -565,6 +573,8 @@ public function setTable(string $table): QueryBuilder
*/
public function whereNull(string $column): QueryBuilder
{
$column = $this->assertSafeIdentifier($column, 'whereNull');

if (is_null($this->where)) {
$this->where = $column . ' is null';
} else {
Expand All @@ -584,6 +594,8 @@ public function whereNull(string $column): QueryBuilder
*/
public function whereNotNull(string $column): QueryBuilder
{
$column = $this->assertSafeIdentifier($column, 'whereNotNull');

if (is_null($this->where)) {
$this->where = $column . ' is not null';
} else {
Expand All @@ -602,15 +614,18 @@ public function whereNotNull(string $column): QueryBuilder
*/
public function whereNotBetween(string $column, array $range): QueryBuilder
{
$column = $this->assertSafeIdentifier($column, 'whereNotBetween');
$range = (array) $range;
$between = implode(' and ', $range);

if (is_null($this->where)) {
$this->where = $column . ' not between ' . $between;
$this->where = $column . ' not between ? and ?';
} else {
$this->where .= ' and ' . $column . ' not between ' . $between;
$this->where .= ' and ' . $column . ' not between ? and ?';
}

$this->where_data_binding[] = $range[0];
$this->where_data_binding[] = $range[1];

return $this;
}

Expand All @@ -625,15 +640,18 @@ public function whereNotBetween(string $column, array $range): QueryBuilder
*/
public function whereBetween(string $column, array $range): QueryBuilder
{
$column = $this->assertSafeIdentifier($column, 'whereBetween');
$range = (array) $range;
$between = implode(' and ', $range);

if (is_null($this->where)) {
$this->where = $column . ' between ' . $between;
$this->where = $column . ' between ? and ?';
} else {
$this->where .= ' and ' . $column . ' between ' . $between;
$this->where .= ' and ' . $column . ' between ? and ?';
}

$this->where_data_binding[] = $range[0];
$this->where_data_binding[] = $range[1];

return $this;
}

Expand Down Expand Up @@ -675,6 +693,8 @@ public function whereNotIn(string $column, array $range)
$in = (string) $range;
}

$column = $this->assertSafeIdentifier($column, 'whereNotIn');

if (is_null($this->where)) {
$this->where = $column . ' not in (' . $in . ')';
} else {
Expand Down Expand Up @@ -708,6 +728,8 @@ public function whereIn(string $column, array $range): QueryBuilder
$in = (string) $range;
}

$column = $this->assertSafeIdentifier($column, 'whereIn');

if (is_null($this->where)) {
$this->where = $column . ' in (' . $in . ')';
} else {
Expand All @@ -732,7 +754,7 @@ public function join(
mixed $comparator = '=',
?string $second = null
): QueryBuilder {
$table = $this->getPrefix() . $table;
$table = $this->getPrefix() . $this->assertSafeIdentifier($table, 'join');

if (is_null($this->join)) {
$this->join = '';
Expand All @@ -746,6 +768,9 @@ public function join(
$comparator = '=';
}

$first = $this->assertSafeIdentifier($first, 'join');
$second = $this->assertSafeIdentifier($second, 'join');

// Building the join query
$this->join .= 'inner join ' . $table . ' on ' . $first . ' ' . $comparator . ' ' . $second;

Expand Down Expand Up @@ -791,7 +816,7 @@ public function leftJoin(
mixed $comparator = '=',
?string $second = null
): QueryBuilder {
$table = $this->getPrefix() . $table;
$table = $this->getPrefix() . $this->assertSafeIdentifier($table, 'join');

if (is_null($this->join)) {
$this->join = '';
Expand All @@ -805,6 +830,9 @@ public function leftJoin(
$comparator = '=';
}

$first = $this->assertSafeIdentifier($first, 'join');
$second = $this->assertSafeIdentifier($second, 'join');

// Building the join query
$this->join .= 'left join ' . $table . ' on ' . $first . ' ' . $comparator . ' ' . $second . ' ';

Expand All @@ -827,7 +855,7 @@ public function rightJoin(
mixed $comparator = '=',
?string $second = null
): QueryBuilder {
$table = $this->getPrefix() . $table;
$table = $this->getPrefix() . $this->assertSafeIdentifier($table, 'join');

if (is_null($this->join)) {
$this->join = '';
Expand All @@ -841,6 +869,9 @@ public function rightJoin(
$comparator = '=';
}

$first = $this->assertSafeIdentifier($first, 'join');
$second = $this->assertSafeIdentifier($second, 'join');

$this->join .= 'right join ' . $table . ' on ' . $first . ' ' . $comparator . ' ' . $second;

return $this;
Expand Down Expand Up @@ -925,7 +956,7 @@ public function group(string $column)
public function groupBy(string $column): QueryBuilder
{
if (is_null($this->group)) {
$this->group = static::assertSafeIdentifier($column, 'groupBy');
$this->group = $this->assertSafeIdentifier($column, 'groupBy');
}

return $this;
Expand All @@ -952,7 +983,7 @@ public function having(
$comparator = '=';
}

$column = static::assertSafeIdentifier($column, 'having');
$column = $this->assertSafeIdentifier($column, 'having');

// Bind the value with a placeholder, exactly like where(). A subquery
// is inlined; any scalar is parameterised so it can never be injected.
Expand Down Expand Up @@ -985,7 +1016,7 @@ public function orderBy(string $column, string $type = 'asc'): QueryBuilder
$type = 'asc';
}

$column = static::assertSafeIdentifier($column, 'orderBy');
$column = $this->assertSafeIdentifier($column, 'orderBy');

if (is_null($this->order)) {
$this->order = 'order by ' . $column . ' ' . strtolower($type);
Expand Down Expand Up @@ -1016,6 +1047,7 @@ public function max(string $column): int|float
*/
private function aggregate($aggregate, $column): mixed
{
$column = $this->assertSafeIdentifier($column, 'aggregate', true);
$sql = 'select ' . $aggregate . '(' . $column . ') from ' . $this->table;

// Adding the join clause
Expand Down Expand Up @@ -1344,6 +1376,8 @@ public function select(array $select = ['*'])
foreach ($select as $key => $value) {
if ($value instanceof QueryBuilder) {
$select[$key] = '(' . $value->toSql() . ')';
} else {
$select[$key] = $this->assertSafeIdentifier((string) $value, 'select', true);
}
}

Expand Down Expand Up @@ -1386,8 +1420,14 @@ public function jump(int $offset = 0): QueryBuilder
*/
public function update(array $data = []): int
{
$columns = array_keys($data);

foreach ($columns as $column) {
$this->assertSafeIdentifier((string) $column, 'update');
}

$sql = 'update ' . $this->table . ' set ';
$sql .= implode(' = ?, ', array_keys($data)) . ' = ?';
$sql .= implode(' = ?, ', $columns) . ' = ?';

if (!is_null($this->where)) {
$sql .= ' where ' . $this->where;
Expand Down Expand Up @@ -1481,6 +1521,7 @@ public function decrement(string $column, int $step = 1): int
*/
private function incrementAction(string $column, int $step = 1, string $direction = '+')
{
$column = $this->assertSafeIdentifier($column, 'incrementAction');
$sql = 'update ' . $this->table . ' set ' . $column . ' = ' . $column . ' ' . $direction . ' ' . $step;

if (!is_null($this->where)) {
Expand All @@ -1505,6 +1546,8 @@ private function incrementAction(string $column, int $step = 1, string $directio
*/
public function distinct(string $column)
{
$column = $this->assertSafeIdentifier($column, 'distinct', true);

if (!is_null($this->select)) {
$this->select .= ", distinct $column";
} else {
Expand Down Expand Up @@ -1619,6 +1662,11 @@ public function insert(array $values): int
private function insertOne(array $values): int
{
$fields = array_keys($values);

foreach ($fields as $field) {
$this->assertSafeIdentifier((string) $field, 'insertOne');
}

$column = implode(', ', $fields);

$sql = 'insert into ' . $this->table . '(' . $column . ') values';
Expand Down
9 changes: 8 additions & 1 deletion src/Http/Response.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,9 @@ public function getCode(): int
/**
* Download the given file as an argument
*
* Note: $file is expected to be a trusted, caller-controlled path. Never
* pass unsanitized user input as $file, as it is read directly from disk.
*
* @param string $file
* @param ?string $filename
* @param array $headers
Expand All @@ -124,9 +127,13 @@ public function download(
$filename = basename($file);
}

// Sanitize the filename used in the Content-Disposition header to prevent
// header injection and path disclosure (strip directory, CR/LF and quotes).
$filename = str_replace(["\r", "\n", '"'], '', basename($filename));

$disposition = $headers["disposition"] ?? 'attachment';

$this->withHeader('Content-Disposition', $disposition . '; filename=' . $filename);
$this->withHeader('Content-Disposition', $disposition . '; filename="' . $filename . '"');
$this->withHeader('Content-Type', $type);

$file_size = filesize($file);
Expand Down
4 changes: 4 additions & 0 deletions src/Http/UploadedFile.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,10 @@ public function moveTo(string $to, ?string $filename = null): bool

if (is_null($filename)) {
$filename = $this->getHashName();
} else {
// Strip any directory component from an explicit filename to prevent
// path traversal outside of the destination directory.
$filename = basename($filename);
}

if (!is_dir($to)) {
Expand Down
Loading
Loading