Merge pull request #692 from flightphp/security-fixes

security fixes

flight/Engine.php

@@ -204,10 +204,12 @@ class Engine
         $this->set('flight.case_sensitive', false);
         $this->set('flight.handle_errors', true);
         $this->set('flight.log_errors', false);
+        $this->set('flight.debug', false);
         $this->set('flight.views.path', './views');
         $this->set('flight.views.extension', '.php');
         $this->set('flight.content_length', true);
         $this->set('flight.v2.output_buffering', false);
+        $this->set('flight.allow_method_override', true);
 
         // Startup configuration
         $this->before('start', function () use ($self) {
@@ -225,6 +227,8 @@ class Engine
             // which causes a lot of problems. This will be removed
             // in v4
             $self->response()->v2_output_buffering = $this->get('flight.v2.output_buffering');
+            // Propagate method override setting to Request
+            $self->request()::$allowMethodOverride = (bool) $self->get('flight.allow_method_override');
         });
 
         $this->initialized = true;
@@ -678,16 +682,24 @@ class Engine
     public function _error(Throwable $e): void
     {
         $this->triggerEvent('flight.error', $e);
-        $msg = sprintf(
+
-            <<<'HTML'
+        if ($this->get('flight.debug') === true) {
-            <h1>500 Internal Server Error</h1>
+            $msg = sprintf(
-                <h3>%s (%s)</h3>
+                <<<'HTML'
-                <pre>%s</pre>
+                <h1>500 Internal Server Error</h1>
-            HTML, // phpcs:ignore
+                    <h3>%s (%s)</h3>
-            $e->getMessage(),
+                    <pre>%s</pre>
-            $e->getCode(),
+                HTML, // phpcs:ignore
-            $e->getTraceAsString()
+                htmlspecialchars($e->getMessage(), ENT_QUOTES, 'UTF-8'),
-        );
+                $e->getCode(),
+                htmlspecialchars($e->getTraceAsString(), ENT_QUOTES, 'UTF-8')
+            );
+        } else {
+            if ($this->get('flight.log_errors') === true) {
+                error_log($e->getMessage() . "\n" . $e->getTraceAsString());
+            }
+            $msg = '<h1>500 Internal Server Error</h1>';
+        }
 
         try {
             $this->response()
@@ -890,7 +902,7 @@ class Engine
         }
 
         // Append base url to redirect url
-        if ($base !== '/'   && strpos($url, '://') === false) {
+        if ($base !== '/' && strpos($url, '://') === false) {
             $url = $base . preg_replace('#/+#', '/', '/' . $url);
         }
 
@@ -1001,7 +1013,11 @@ class Engine
         int $option = 0
     ): void {
         $json = $encode ? Json::encode($data, $option) : $data;
-        $callback = $this->request()->query[$param];
+        $callback = (string) $this->request()->query[$param];
+
+        if ($callback !== '' && !preg_match('/^[A-Za-z_$][\w$.]{0,127}$/', $callback)) {
+            throw new Exception('Invalid JSONP callback name.');
+        }
 
         $this->response()
             ->status($code)

flight/commands/ControllerCommand.php

@@ -50,6 +50,13 @@ class ControllerCommand extends AbstractBaseCommand
             return;
         }
 
+        $controller = basename($controller);
+
+        if (!preg_match('/^[A-Za-z_][A-Za-z0-9_]*$/', str_replace('Controller', '', $controller))) {
+            $io->error('Controller name must contain only letters, numbers, and underscores.', true);
+            return;
+        }
+
         if (!preg_match('/Controller$/', $controller)) {
             $controller .= 'Controller';
         }

flight/database/SimplePdo.php

@@ -55,6 +55,17 @@ class SimplePdo extends PdoWrapper
         }
     }
 
+    /**
+     * Validates that an SQL identifier (table or column name) is safe for interpolation.
+     * Throws PDOException on invalid identifier to prevent SQL injection.
+     */
+    protected function requireSafeIdentifier(string $identifier): void
+    {
+        if (!preg_match('/^[A-Za-z_][A-Za-z0-9_]*$/', $identifier)) {
+            throw new PDOException("Unsafe SQL identifier: '$identifier'");
+        }
+    }
+
     /**
      * Pulls one row from the query
      *
@@ -319,6 +330,8 @@ class SimplePdo extends PdoWrapper
      */
     public function insert(string $table, array $data): string
     {
+        $this->requireSafeIdentifier($table);
+
         // Detect if this is a bulk insert (array of arrays)
         $isBulk = isset($data[0]) && is_array($data[0]);
 
@@ -333,6 +346,10 @@ class SimplePdo extends PdoWrapper
             $columns = array_keys($firstRow);
             $columnCount = count($columns);
 
+            foreach ($columns as $col) {
+                $this->requireSafeIdentifier((string) $col);
+            }
+
             // Validate all rows have same columns
             foreach ($data as $index => $row) {
                 if (count($row) !== $columnCount) {
@@ -363,6 +380,11 @@ class SimplePdo extends PdoWrapper
         } else {
             // Single insert
             $columns = array_keys($data);
+
+            foreach ($columns as $col) {
+                $this->requireSafeIdentifier((string) $col);
+            }
+
             $placeholders = array_fill(0, count($data), '?');
 
             $sql = sprintf(
@@ -396,8 +418,11 @@ class SimplePdo extends PdoWrapper
      */
     public function update(string $table, array $data, string $where, array $whereParams = []): int
     {
+        $this->requireSafeIdentifier($table);
+
         $sets = [];
         foreach (array_keys($data) as $column) {
+            $this->requireSafeIdentifier((string) $column);
             $sets[] = "$column = ?";
         }
 
@@ -426,6 +451,7 @@ class SimplePdo extends PdoWrapper
      */
     public function delete(string $table, string $where, array $whereParams = []): int
     {
+        $this->requireSafeIdentifier($table);
         $sql = "DELETE FROM $table WHERE $where";
         $stmt = $this->runQuery($sql, $whereParams);
         return $stmt->rowCount();

flight/net/Request.php

@@ -137,6 +137,12 @@ class Request
      */
     public string $servername;
 
+    /**
+     * Whether to allow HTTP method override via X-HTTP-Method-Override header or _method POST field.
+     * Controlled by the flight.allow_method_override engine setting.
+     */
+    public static bool $allowMethodOverride = true;
+
     /**
      * Stream path for where to pull the request body from
      */
@@ -282,10 +288,12 @@ class Request
     {
         $method = self::getVar('REQUEST_METHOD', 'GET');
 
-        if (self::getVar('HTTP_X_HTTP_METHOD_OVERRIDE') !== '') {
+        if (self::$allowMethodOverride === true) {
-            $method = self::getVar('HTTP_X_HTTP_METHOD_OVERRIDE');
+            if (self::getVar('HTTP_X_HTTP_METHOD_OVERRIDE') !== '') {
-        } elseif (isset($_REQUEST['_method']) === true) {
+                $method = self::getVar('HTTP_X_HTTP_METHOD_OVERRIDE');
-            $method = $_REQUEST['_method'];
+            } elseif (isset($_REQUEST['_method']) === true) {
+                $method = $_REQUEST['_method'];
+            }
         }
 
         return strtoupper($method);

tests/EngineTest.php

@@ -87,6 +87,14 @@ class EngineTest extends TestCase
     public function testHandleException(): void
     {
         $engine = new Engine();
+        $this->expectOutputRegex('~\<h1\>500 Internal Server Error\</h1\>~');
+        $engine->handleException(new Exception('thrown exception message', 20));
+    }
+
+    public function testHandleExceptionDebugMode(): void
+    {
+        $engine = new Engine();
+        $engine->set('flight.debug', true);
         $this->expectOutputRegex('~\<h1\>500 Internal Server Error\</h1\>[\s\S]*\<h3\>thrown exception message \(20\)\</h3\>~');
         $engine->handleException(new Exception('thrown exception message', 20));
     }