Skip to content

Commit b8dd23a

Browse files
n0nag0nn0nag0n
authored
Merge pull request #692 from flightphp/security-fixes
security fixes
2 parents 6718fed + 924f231 commit b8dd23a

5 files changed

Lines changed: 81 additions & 16 deletions

File tree

‎flight/Engine.php‎

Lines changed: 28 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -204,10 +204,12 @@ public function init(): void
204204
$this->set('flight.case_sensitive', false);
205205
$this->set('flight.handle_errors', true);
206206
$this->set('flight.log_errors', false);
207+
$this->set('flight.debug', false);
207208
$this->set('flight.views.path', './views');
208209
$this->set('flight.views.extension', '.php');
209210
$this->set('flight.content_length', true);
210211
$this->set('flight.v2.output_buffering', false);
212+
$this->set('flight.allow_method_override', true);
211213

212214
// Startup configuration
213215
$this->before('start', function () use ($self) {
@@ -225,6 +227,8 @@ public function init(): void
225227
// which causes a lot of problems. This will be removed
226228
// in v4
227229
$self->response()->v2_output_buffering = $this->get('flight.v2.output_buffering');
230+
// Propagate method override setting to Request
231+
$self->request()::$allowMethodOverride = (bool) $self->get('flight.allow_method_override');
228232
});
229233

230234
$this->initialized = true;
@@ -678,16 +682,24 @@ public function _start(): void
678682
public function _error(Throwable $e): void
679683
{
680684
$this->triggerEvent('flight.error', $e);
681-
$msg = sprintf(
682-
<<<'HTML'
683-
<h1>500 Internal Server Error</h1>
684-
<h3>%s (%s)</h3>
685-
<pre>%s</pre>
686-
HTML, // phpcs:ignore
687-
$e->getMessage(),
688-
$e->getCode(),
689-
$e->getTraceAsString()
690-
);
685+
686+
if ($this->get('flight.debug') === true) {
687+
$msg = sprintf(
688+
<<<'HTML'
689+
<h1>500 Internal Server Error</h1>
690+
<h3>%s (%s)</h3>
691+
<pre>%s</pre>
692+
HTML, // phpcs:ignore
693+
htmlspecialchars($e->getMessage(), ENT_QUOTES, 'UTF-8'),
694+
$e->getCode(),
695+
htmlspecialchars($e->getTraceAsString(), ENT_QUOTES, 'UTF-8')
696+
);
697+
} else {
698+
if ($this->get('flight.log_errors') === true) {
699+
error_log($e->getMessage() . "\n" . $e->getTraceAsString());
700+
}
701+
$msg = '<h1>500 Internal Server Error</h1>';
702+
}
691703

692704
try {
693705
$this->response()
@@ -890,7 +902,7 @@ public function _redirect(string $url, int $code = 303): void
890902
}
891903

892904
// Append base url to redirect url
893-
if ($base !== '/' && strpos($url, '://') === false) {
905+
if ($base !== '/' && strpos($url, '://') === false) {
894906
$url = $base . preg_replace('#/+#', '/', '/' . $url);
895907
}
896908

@@ -1001,7 +1013,11 @@ public function _jsonp(
10011013
int $option = 0
10021014
): void {
10031015
$json = $encode ? Json::encode($data, $option) : $data;
1004-
$callback = $this->request()->query[$param];
1016+
$callback = (string) $this->request()->query[$param];
1017+
1018+
if ($callback !== '' && !preg_match('/^[A-Za-z_$][\w$.]{0,127}$/', $callback)) {
1019+
throw new Exception('Invalid JSONP callback name.');
1020+
}
10051021

10061022
$this->response()
10071023
->status($code)

‎flight/commands/ControllerCommand.php‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,13 @@ public function execute(string $controller): void
5050
return;
5151
}
5252

53+
$controller = basename($controller);
54+
55+
if (!preg_match('/^[A-Za-z_][A-Za-z0-9_]*$/', str_replace('Controller', '', $controller))) {
56+
$io->error('Controller name must contain only letters, numbers, and underscores.', true);
57+
return;
58+
}
59+
5360
if (!preg_match('/Controller$/', $controller)) {
5461
$controller .= 'Controller';
5562
}

‎flight/database/SimplePdo.php‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,17 @@ public function __construct(
5555
}
5656
}
5757

58+
/**
59+
* Validates that an SQL identifier (table or column name) is safe for interpolation.
60+
* Throws PDOException on invalid identifier to prevent SQL injection.
61+
*/
62+
protected function requireSafeIdentifier(string $identifier): void
63+
{
64+
if (!preg_match('/^[A-Za-z_][A-Za-z0-9_]*$/', $identifier)) {
65+
throw new PDOException("Unsafe SQL identifier: '$identifier'");
66+
}
67+
}
68+
5869
/**
5970
* Pulls one row from the query
6071
*
@@ -319,6 +330,8 @@ public function transaction(callable $callback)
319330
*/
320331
public function insert(string $table, array $data): string
321332
{
333+
$this->requireSafeIdentifier($table);
334+
322335
// Detect if this is a bulk insert (array of arrays)
323336
$isBulk = isset($data[0]) && is_array($data[0]);
324337

@@ -333,6 +346,10 @@ public function insert(string $table, array $data): string
333346
$columns = array_keys($firstRow);
334347
$columnCount = count($columns);
335348

349+
foreach ($columns as $col) {
350+
$this->requireSafeIdentifier((string) $col);
351+
}
352+
336353
// Validate all rows have same columns
337354
foreach ($data as $index => $row) {
338355
if (count($row) !== $columnCount) {
@@ -363,6 +380,11 @@ public function insert(string $table, array $data): string
363380
} else {
364381
// Single insert
365382
$columns = array_keys($data);
383+
384+
foreach ($columns as $col) {
385+
$this->requireSafeIdentifier((string) $col);
386+
}
387+
366388
$placeholders = array_fill(0, count($data), '?');
367389

368390
$sql = sprintf(
@@ -396,8 +418,11 @@ public function insert(string $table, array $data): string
396418
*/
397419
public function update(string $table, array $data, string $where, array $whereParams = []): int
398420
{
421+
$this->requireSafeIdentifier($table);
422+
399423
$sets = [];
400424
foreach (array_keys($data) as $column) {
425+
$this->requireSafeIdentifier((string) $column);
401426
$sets[] = "$column = ?";
402427
}
403428

@@ -426,6 +451,7 @@ public function update(string $table, array $data, string $where, array $wherePa
426451
*/
427452
public function delete(string $table, string $where, array $whereParams = []): int
428453
{
454+
$this->requireSafeIdentifier($table);
429455
$sql = "DELETE FROM $table WHERE $where";
430456
$stmt = $this->runQuery($sql, $whereParams);
431457
return $stmt->rowCount();

‎flight/net/Request.php‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,12 @@ class Request
137137
*/
138138
public string $servername;
139139

140+
/**
141+
* Whether to allow HTTP method override via X-HTTP-Method-Override header or _method POST field.
142+
* Controlled by the flight.allow_method_override engine setting.
143+
*/
144+
public static bool $allowMethodOverride = true;
145+
140146
/**
141147
* Stream path for where to pull the request body from
142148
*/
@@ -282,10 +288,12 @@ public static function getMethod(): string
282288
{
283289
$method = self::getVar('REQUEST_METHOD', 'GET');
284290

285-
if (self::getVar('HTTP_X_HTTP_METHOD_OVERRIDE') !== '') {
286-
$method = self::getVar('HTTP_X_HTTP_METHOD_OVERRIDE');
287-
} elseif (isset($_REQUEST['_method']) === true) {
288-
$method = $_REQUEST['_method'];
291+
if (self::$allowMethodOverride === true) {
292+
if (self::getVar('HTTP_X_HTTP_METHOD_OVERRIDE') !== '') {
293+
$method = self::getVar('HTTP_X_HTTP_METHOD_OVERRIDE');
294+
} elseif (isset($_REQUEST['_method']) === true) {
295+
$method = $_REQUEST['_method'];
296+
}
289297
}
290298

291299
return strtoupper($method);

‎tests/EngineTest.php‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,14 @@ public function testHandleErrorWithException(): void
8787
public function testHandleException(): void
8888
{
8989
$engine = new Engine();
90+
$this->expectOutputRegex('~\<h1\>500 Internal Server Error\</h1\>~');
91+
$engine->handleException(new Exception('thrown exception message', 20));
92+
}
93+
94+
public function testHandleExceptionDebugMode(): void
95+
{
96+
$engine = new Engine();
97+
$engine->set('flight.debug', true);
9098
$this->expectOutputRegex('~\<h1\>500 Internal Server Error\</h1\>[\s\S]*\<h3\>thrown exception message \(20\)\</h3\>~');
9199
$engine->handleException(new Exception('thrown exception message', 20));
92100
}

0 commit comments

Comments
 (0)