From dd5216e4a839a730d1497b8d714beb1c3a756273 Mon Sep 17 00:00:00 2001 From: Franck DAKIA Date: Fri, 17 Jul 2026 13:42:17 +0000 Subject: [PATCH 1/7] try to fix --- src/Database/Connection/AbstractConnection.php | 6 +++--- src/Database/Connection/Adapters/MysqlAdapter.php | 4 ++-- src/Database/Connection/Adapters/PostgreSQLAdapter.php | 4 ++-- src/Database/Connection/Adapters/SqliteAdapter.php | 4 ++-- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/Database/Connection/AbstractConnection.php b/src/Database/Connection/AbstractConnection.php index 4f5984d5..00a65166 100644 --- a/src/Database/Connection/AbstractConnection.php +++ b/src/Database/Connection/AbstractConnection.php @@ -67,7 +67,7 @@ abstract class AbstractConnection * * @param array $config */ - public function __construct(array $config) + public function __construct(#[\SensitiveParameter] array $config) { $this->config = $config; @@ -95,7 +95,7 @@ public function __construct(array $config) * @param array $config * @return void */ - abstract protected function validateConfig(array $config): void; + abstract protected function validateConfig(#[\SensitiveParameter] array $config): void; /** * Build a PDO instance from the given configuration. @@ -103,7 +103,7 @@ abstract protected function validateConfig(array $config): void; * @param array $config * @return PDO */ - abstract protected function makePdo(array $config): PDO; + abstract protected function makePdo(#[\SensitiveParameter] array $config): PDO; /** * Build (eagerly) the write connection. diff --git a/src/Database/Connection/Adapters/MysqlAdapter.php b/src/Database/Connection/Adapters/MysqlAdapter.php index b8bb4910..f32c2498 100644 --- a/src/Database/Connection/Adapters/MysqlAdapter.php +++ b/src/Database/Connection/Adapters/MysqlAdapter.php @@ -30,7 +30,7 @@ class MysqlAdapter extends AbstractConnection * @param array $config * @return void */ - protected function validateConfig(array $config): void + protected function validateConfig(#[\SensitiveParameter] array $config): void { // Check the existence of database definition if (!isset($config['database'])) { @@ -44,7 +44,7 @@ protected function validateConfig(array $config): void * @param array $config * @return PDO */ - protected function makePdo(array $config): PDO + protected function makePdo(#[\SensitiveParameter] array $config): PDO { // Build of the mysql dsn if (isset($config['socket']) && !empty($config['socket'])) { diff --git a/src/Database/Connection/Adapters/PostgreSQLAdapter.php b/src/Database/Connection/Adapters/PostgreSQLAdapter.php index a6535c1a..99bb6918 100644 --- a/src/Database/Connection/Adapters/PostgreSQLAdapter.php +++ b/src/Database/Connection/Adapters/PostgreSQLAdapter.php @@ -29,7 +29,7 @@ class PostgreSQLAdapter extends AbstractConnection * @param array $config * @return void */ - protected function validateConfig(array $config): void + protected function validateConfig(#[\SensitiveParameter] array $config): void { // Check the existence of database definition if (!isset($config['database'])) { @@ -43,7 +43,7 @@ protected function validateConfig(array $config): void * @param array $config * @return PDO */ - protected function makePdo(array $config): PDO + protected function makePdo(#[\SensitiveParameter] array $config): PDO { // Build of the pgsql dsn if (isset($config['socket']) && !is_null($config['socket']) && !empty($config['socket'])) { diff --git a/src/Database/Connection/Adapters/SqliteAdapter.php b/src/Database/Connection/Adapters/SqliteAdapter.php index 3dbb5f47..45c4f257 100644 --- a/src/Database/Connection/Adapters/SqliteAdapter.php +++ b/src/Database/Connection/Adapters/SqliteAdapter.php @@ -23,7 +23,7 @@ class SqliteAdapter extends AbstractConnection * @param array $config * @return void */ - protected function validateConfig(array $config): void + protected function validateConfig(#[\SensitiveParameter] array $config): void { if (!isset($config['driver'])) { throw new InvalidArgumentException("Please select the right sqlite driver"); @@ -40,7 +40,7 @@ protected function validateConfig(array $config): void * @param array $config * @return PDO */ - protected function makePdo(array $config): PDO + protected function makePdo(#[\SensitiveParameter] array $config): PDO { // Build the PDO connection $pdo = new PDO('sqlite:' . $config['database']); From 235c06c484ec387bf0099ff7c34929d702bb239b Mon Sep 17 00:00:00 2001 From: Franck DAKIA Date: Sat, 18 Jul 2026 03:16:22 +0000 Subject: [PATCH 2/7] sec(): fix many security issues --- src/Database/QueryBuilder.php | 75 +++++++++++++++++++++++++++-- src/Queue/Adapters/QueueAdapter.php | 23 ++++++++- src/Security/Tokenize.php | 12 ++--- tests/Queue/EventQueueTest.php | 2 + tests/Queue/MailQueueTest.php | 2 + tests/Queue/NotifierQueueTest.php | 2 + tests/Queue/QueueTest.php | 3 ++ 7 files changed, 108 insertions(+), 11 deletions(-) diff --git a/src/Database/QueryBuilder.php b/src/Database/QueryBuilder.php index 7b54926c..b352478f 100644 --- a/src/Database/QueryBuilder.php +++ b/src/Database/QueryBuilder.php @@ -70,6 +70,17 @@ class QueryBuilder implements JsonSerializable */ protected ?string $having = null; + /** + * Bound values for the having clause. + * + * Kept separate from where bindings because having placeholders appear + * after where placeholders in the assembled SQL; they are merged in the + * correct positional order when the having clause is appended. + * + * @var array + */ + protected array $having_data_binding = []; + /** * Order By statement collector * @@ -404,6 +415,34 @@ private static function isComparisonOperator(mixed $comparator): bool ], true); } + /** + * Guard a column/identifier that is interpolated straight into SQL. + * + * Clauses like order by, group by and having name a column instead of + * binding a value, so the identifier cannot be a placeholder and is + * concatenated into the statement. Restrict it to a plain (optionally + * table-qualified) identifier so it can never carry an injected fragment. + * Raw expressions are intentionally not accepted here. + * + * @param string $identifier + * @param string $clause + * @return string + * @throws QueryBuilderException + */ + private static function assertSafeIdentifier(string $identifier, string $clause): string + { + $trimmed = trim($identifier); + + if (!preg_match('/^[A-Za-z_][A-Za-z0-9_]*(\.[A-Za-z_][A-Za-z0-9_]*)?$/', $trimmed)) { + throw new QueryBuilderException( + "Unsafe identifier passed to {$clause}: [{$identifier}]. " + . "Only a plain or table-qualified column name is allowed." + ); + } + + return $trimmed; + } + /** * Formats the select request * @@ -464,6 +503,15 @@ public function toSql(): string if (!is_null($this->having)) { $sql .= ' having ' . $this->having; + + // having placeholders come after where placeholders in the SQL, + // so appending their values here keeps the positional order. + $this->where_data_binding = array_merge( + $this->where_data_binding, + $this->having_data_binding + ); + $this->having_data_binding = []; + $this->having = null; } } @@ -877,7 +925,7 @@ public function group(string $column) public function groupBy(string $column): QueryBuilder { if (is_null($this->group)) { - $this->group = $column; + $this->group = static::assertSafeIdentifier($column, 'groupBy'); } return $this; @@ -904,10 +952,21 @@ public function having( $comparator = '='; } + $column = static::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. + if ($value instanceof QueryBuilder) { + $indicator = '(' . $value->toSql() . ')'; + } else { + $indicator = '?'; + $this->having_data_binding[] = $value; + } + if (is_null($this->having)) { - $this->having = $column . ' ' . $comparator . ' ' . $value; + $this->having = $column . ' ' . $comparator . ' ' . $indicator; } else { - $this->having .= ' ' . $boolean . ' ' . $column . ' ' . $comparator . ' ' . $value; + $this->having .= ' ' . $boolean . ' ' . $column . ' ' . $comparator . ' ' . $indicator; } return $this; @@ -926,6 +985,8 @@ public function orderBy(string $column, string $type = 'asc'): QueryBuilder $type = 'asc'; } + $column = static::assertSafeIdentifier($column, 'orderBy'); + if (is_null($this->order)) { $this->order = 'order by ' . $column . ' ' . strtolower($type); } else { @@ -977,6 +1038,14 @@ private function aggregate($aggregate, $column): mixed if (!is_null($this->having)) { $sql .= ' having ' . $this->having; + + // Keep having values positionally after where values. + $this->where_data_binding = array_merge( + $this->where_data_binding, + $this->having_data_binding + ); + $this->having_data_binding = []; + $this->having = null; } } diff --git a/src/Queue/Adapters/QueueAdapter.php b/src/Queue/Adapters/QueueAdapter.php index 6b0d07ae..8277508d 100644 --- a/src/Queue/Adapters/QueueAdapter.php +++ b/src/Queue/Adapters/QueueAdapter.php @@ -5,6 +5,8 @@ namespace Bow\Queue\Adapters; use Bow\Queue\QueueTask; +use Bow\Security\Crypto; +use RuntimeException; use Throwable; abstract class QueueAdapter @@ -97,7 +99,12 @@ abstract public function push(QueueTask $task): bool; */ public function serializeProducer(QueueTask $task): string { - return serialize($task); + // Authenticate the payload (encrypt-then-MAC) so a worker will only ever + // unserialize bytes this application produced. Without this, anyone able + // to write to the queue backend could deliver a crafted serialized + // object and trigger PHP object injection (RCE via a POP chain) when the + // worker deserializes it. + return Crypto::encrypt(serialize($task)); } /** @@ -108,7 +115,19 @@ public function serializeProducer(QueueTask $task): string */ public function unserializeProducer(string $task): QueueTask { - return unserialize($task); + // Verify integrity BEFORE unserialize(). Crypto::decrypt fails closed + // (returns false) on a tampered, forged or wrong-key payload, so crafted + // bytes never reach unserialize(). Only payloads produced by + // serializeProducer() with this application's key get past this point. + $plain = Crypto::decrypt($task); + + if ($plain === false) { + throw new RuntimeException( + 'Queue payload failed integrity verification and was rejected.' + ); + } + + return unserialize($plain); } /** diff --git a/src/Security/Tokenize.php b/src/Security/Tokenize.php index a7b9b873..a51338d5 100644 --- a/src/Security/Tokenize.php +++ b/src/Security/Tokenize.php @@ -65,17 +65,17 @@ public static function makeCsrfToken(?int $time = null): bool } /** - * GGenerate an encrypted key + * Generate a cryptographically strong token. + * + * Drawn from 32 bytes (256 bits) of CSPRNG output. The previous version + * seeded only 6 random bytes mixed with a time/uniqid/rand() salt, so the + * real entropy floor was ~48 bits and the salt (non-CSPRNG) added none. * * @return string */ public static function make(): string { - $salt = date('Y-m-d H:i:s', time() - 10000) . uniqid((string)rand(), true); - - $token = base64_encode(base64_encode(openssl_random_pseudo_bytes(6)) . $salt); - - return Str::slice(hash('sha256', $token), 1, 62); + return Str::slice(hash('sha256', random_bytes(32)), 1, 62); } /** diff --git a/tests/Queue/EventQueueTest.php b/tests/Queue/EventQueueTest.php index 5a11f755..48399493 100644 --- a/tests/Queue/EventQueueTest.php +++ b/tests/Queue/EventQueueTest.php @@ -6,6 +6,7 @@ use Bow\Configuration\EnvConfiguration; use Bow\Configuration\LoggerConfiguration; use Bow\Database\DatabaseConfiguration; +use Bow\Security\CryptoConfiguration; use Bow\Event\EventQueueTask; use Bow\Mail\MailConfiguration; use Bow\Queue\Adapters\QueueAdapter; @@ -33,6 +34,7 @@ public static function setUpBeforeClass(): void QueueConfiguration::class, DatabaseConfiguration::class, EnvConfiguration::class, + CryptoConfiguration::class, LoggerConfiguration::class, MailConfiguration::class, ViewConfiguration::class, diff --git a/tests/Queue/MailQueueTest.php b/tests/Queue/MailQueueTest.php index a63a2ae0..74d3459e 100644 --- a/tests/Queue/MailQueueTest.php +++ b/tests/Queue/MailQueueTest.php @@ -6,6 +6,7 @@ use Bow\Configuration\EnvConfiguration; use Bow\Configuration\LoggerConfiguration; use Bow\Database\DatabaseConfiguration; +use Bow\Security\CryptoConfiguration; use Bow\Mail\Envelop; use Bow\Mail\MailConfiguration; use Bow\Mail\MailQueueTask; @@ -30,6 +31,7 @@ public static function setUpBeforeClass(): void QueueConfiguration::class, DatabaseConfiguration::class, EnvConfiguration::class, + CryptoConfiguration::class, LoggerConfiguration::class, MailConfiguration::class, ViewConfiguration::class, diff --git a/tests/Queue/NotifierQueueTest.php b/tests/Queue/NotifierQueueTest.php index 77c8550f..0708947e 100644 --- a/tests/Queue/NotifierQueueTest.php +++ b/tests/Queue/NotifierQueueTest.php @@ -6,6 +6,7 @@ use Bow\Configuration\EnvConfiguration; use Bow\Configuration\LoggerConfiguration; use Bow\Database\DatabaseConfiguration; +use Bow\Security\CryptoConfiguration; use Bow\Mail\MailConfiguration; use Bow\Notifier\Notifier; use Bow\Notifier\NotifierQueueTask; @@ -33,6 +34,7 @@ public static function setUpBeforeClass(): void DatabaseConfiguration::class, QueueConfiguration::class, EnvConfiguration::class, + CryptoConfiguration::class, LoggerConfiguration::class, MailConfiguration::class, ViewConfiguration::class, diff --git a/tests/Queue/QueueTest.php b/tests/Queue/QueueTest.php index 29121045..e0ef9ac8 100644 --- a/tests/Queue/QueueTest.php +++ b/tests/Queue/QueueTest.php @@ -7,6 +7,7 @@ use Bow\Configuration\LoggerConfiguration; use Bow\Database\Database; use Bow\Database\DatabaseConfiguration; +use Bow\Security\CryptoConfiguration; use Bow\Mail\Mail; use Bow\Queue\Adapters\BeanstalkdAdapter; use Bow\Queue\Adapters\DatabaseAdapter; @@ -50,6 +51,8 @@ public static function setUpBeforeClass(): void DatabaseConfiguration::class, CacheConfiguration::class, EnvConfiguration::class, + // Queue payloads are now authenticated, so the worker needs the key. + CryptoConfiguration::class, ]); $config = TestingConfiguration::getConfig(); From e8366936a8adfb14c058d9a0b75c40162563d76a Mon Sep 17 00:00:00 2001 From: Franck DAKIA Date: Sat, 18 Jul 2026 03:43:41 +0000 Subject: [PATCH 3/7] formatting --- ROADMAP.md | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 077d513e..037e93a2 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -124,18 +124,18 @@ Highlights from the latest iterations — already merged into `5.x`. Full detail | Task | Status | Priority | Notes | | ---------------------------------------------------------------- | ------ | -------- | ---------------------------------------------------------------- | -| Fix middleware attribute test (shared state between tests) | ✅ Done | - | `Router::$routes` converted to instance state | +| Fix middleware attribute test (shared state between tests) | ✅ Done | - | `Router::$routes` converted to instance state | | Fix Pagination tests calling `total()` instead of `totalPages()` | ✅ Done | - | 24 tests fixed | -| Fix Barry model `array` cast returning `stdClass` | ✅ Done | - | `Model::executeDataCasting` + `parseToJson($value, assoc: true)` | -| Fix Validator `nullable\|required` priority | ✅ Done | - | `nullable` no longer short-circuits `required` | -| Fix `EnvTest` singleton pollution between tests | ✅ Done | - | `Env::reset()` added | -| Fix `SchedulerCommand` (`routes/scheduler.php` loading) | ✅ Done | - | `loadSchedulerFile()` updated, tolerates missing Loader | -| Remove dead `Model::$soft_delete` property | ✅ Done | - | Replaced with a fully functional trait | -| Improve `addEnum` / `changeEnum` error messages | ✅ Done | - | Explicitly mention the `size` key | -| Standardize method signatures | ✅ Done | - | PHP 8.1+ nullable types | -| Fix `(double)` → `(float)` cast | ✅ Done | - | `Model.php` | -| Handle `array_key_exists` with null key | ✅ Done | - | `Console.php` | -| Create test directory if missing | ✅ Done | - | `CustomCommand.php` | +| Fix Barry model `array` cast returning `stdClass` | ✅ Done | - | `Model::executeDataCasting` + `parseToJson($value, assoc: true)`| +| Fix Validator `nullable\|required` priority | ✅ Done | - | `nullable` no longer short-circuits `required` | +| Fix `EnvTest` singleton pollution between tests | ✅ Done | - | `Env::reset()` added | +| Fix `SchedulerCommand` (`routes/scheduler.php` loading) | ✅ Done | - | `loadSchedulerFile()` updated, tolerates missing Loader | +| Remove dead `Model::$soft_delete` property | ✅ Done | - | Replaced with a fully functional trait | +| Improve `addEnum` / `changeEnum` error messages | ✅ Done | - | Explicitly mention the `size` key | +| Standardize method signatures | ✅ Done | - | PHP 8.1+ nullable types | +| Fix `(double)` → `(float)` cast | ✅ Done | - | `Model.php` | +| Handle `array_key_exists` with null key | ✅ Done | - | `Console.php` | +| Create test directory if missing | ✅ Done | - | `CustomCommand.php` | ### Documentation From f33780dc3fb0160cb662cfefe9c55dd4279de005 Mon Sep 17 00:00:00 2001 From: Franck DAKIA Date: Sat, 18 Jul 2026 04:00:37 +0000 Subject: [PATCH 4/7] feat(): config encrypt method to config --- src/Queue/Adapters/QueueAdapter.php | 66 ++++++++++++++++++++++++----- src/Security/Crypto.php | 59 ++++++++++++++++++++++++++ tests/Config/stubs/config/queue.php | 8 ++++ 3 files changed, 122 insertions(+), 11 deletions(-) diff --git a/src/Queue/Adapters/QueueAdapter.php b/src/Queue/Adapters/QueueAdapter.php index 8277508d..c21916b4 100644 --- a/src/Queue/Adapters/QueueAdapter.php +++ b/src/Queue/Adapters/QueueAdapter.php @@ -64,6 +64,14 @@ abstract class QueueAdapter */ protected static bool $suppressLogging = false; + /** + * Cached queue payload protection mode ('encrypt' | 'sign'), resolved once + * from config on first use. Null until resolved. + * + * @var ?string + */ + private ?string $payload_protection = null; + /** * Enable or disable logging suppression * @@ -99,12 +107,43 @@ abstract public function push(QueueTask $task): bool; */ public function serializeProducer(QueueTask $task): string { - // Authenticate the payload (encrypt-then-MAC) so a worker will only ever - // unserialize bytes this application produced. Without this, anyone able - // to write to the queue backend could deliver a crafted serialized - // object and trigger PHP object injection (RCE via a POP chain) when the - // worker deserializes it. - return Crypto::encrypt(serialize($task)); + // Authenticate the payload so a worker will only ever unserialize bytes + // this application produced. Without this, anyone able to write to the + // queue backend could deliver a crafted serialized object and trigger PHP + // object injection (RCE via a POP chain) when the worker deserializes it. + // + // 'encrypt' (default) also keeps the payload confidential in the broker; + // 'sign' leaves it readable for debugging while staying tamper-proof. + $serialized = serialize($task); + + return $this->payloadProtection() === 'sign' + ? Crypto::sign($serialized) + : Crypto::encrypt($serialized); + } + + /** + * Resolve the configured queue payload protection mode. + * + * 'encrypt' (confidential + tamper-proof) is the secure default; only an + * explicit config('queue.payload_protection') === 'sign' opts into the + * readable-but-signed format. Falls back to 'encrypt' when config is not + * booted, so the secure behaviour holds in every context. + * + * @return string + */ + private function payloadProtection(): string + { + if ($this->payload_protection === null) { + try { + $mode = config('queue.payload_protection'); + } catch (Throwable) { + $mode = null; + } + + $this->payload_protection = $mode === 'sign' ? 'sign' : 'encrypt'; + } + + return $this->payload_protection; } /** @@ -115,11 +154,16 @@ public function serializeProducer(QueueTask $task): string */ public function unserializeProducer(string $task): QueueTask { - // Verify integrity BEFORE unserialize(). Crypto::decrypt fails closed - // (returns false) on a tampered, forged or wrong-key payload, so crafted - // bytes never reach unserialize(). Only payloads produced by - // serializeProducer() with this application's key get past this point. - $plain = Crypto::decrypt($task); + // Verify integrity BEFORE unserialize(). Both schemes fail closed + // (return false) on a tampered, forged or wrong-key payload, so crafted + // bytes never reach unserialize(). We accept either the signed or the + // encrypted format regardless of the configured mode, so flipping + // queue.payload_protection during a rollout never drops in-flight jobs. + $plain = Crypto::verify($task); + + if ($plain === false) { + $plain = Crypto::decrypt($task); + } if ($plain === false) { throw new RuntimeException( diff --git a/src/Security/Crypto.php b/src/Security/Crypto.php index 27a2dd25..04ba6be5 100644 --- a/src/Security/Crypto.php +++ b/src/Security/Crypto.php @@ -30,6 +30,12 @@ class Crypto */ private const HEADER = 'BOW2:'; + /** + * Header tagging an authenticated-but-unencrypted (signed) payload. The ':' + * keeps it distinguishable from an encrypted (BOW2:) or base64 value. + */ + private const SIGN_HEADER = 'BOWS1:'; + /** * The authentication tag length in bytes (HMAC-SHA256). */ @@ -136,6 +142,59 @@ public static function decrypt(string $data): string|bool ); } + /** + * Produce a tamper-proof but *readable* payload. + * + * Unlike encrypt(), the data is not enciphered — only a detached + * HMAC-SHA256 tag is prepended — so the payload stays inspectable in transit + * (e.g. a queue backend) while still being protected against tampering and + * forgery. Use when integrity matters but confidentiality does not. + * + * @param string $data + * @return string + */ + public static function sign(string $data): string + { + $mac = hash_hmac('sha256', $data, static::deriveKey('sign', static::resolveKey()), true); + + return self::SIGN_HEADER . base64_encode($mac) . '.' . $data; + } + + /** + * Verify a signed payload, returning the original data or false on a bad tag. + * + * Fails closed (false) on a wrong header, truncation, tampering or wrong key, + * exactly like decrypt(). + * + * @param string $data + * @return string|bool + */ + public static function verify(string $data): string|bool + { + if (!str_starts_with($data, self::SIGN_HEADER)) { + return false; + } + + $body = substr($data, strlen(self::SIGN_HEADER)); + $dot = strpos($body, '.'); + + if ($dot === false) { + return false; + } + + $mac = base64_decode(substr($body, 0, $dot), true); + + if ($mac === false || strlen($mac) !== self::MAC_LENGTH) { + return false; + } + + $payload = substr($body, $dot + 1); + $calculated = hash_hmac('sha256', $payload, static::deriveKey('sign', static::resolveKey()), true); + + // Constant-time compare; reject before the caller trusts the payload. + return hash_equals($calculated, $mac) ? $payload : false; + } + /** * Decrypt a value produced by the legacy (static IV, unauthenticated) * format. Kept only so data encrypted before the upgrade keeps working. diff --git a/tests/Config/stubs/config/queue.php b/tests/Config/stubs/config/queue.php index 01dfb8fd..8ffe882a 100644 --- a/tests/Config/stubs/config/queue.php +++ b/tests/Config/stubs/config/queue.php @@ -6,6 +6,14 @@ */ "default" => "sync", + /** + * How queue payloads are protected on the wire: + * "encrypt" — confidential + tamper-proof (default, recommended) + * "sign" — tamper-proof but readable in the broker (easier to debug) + * Both require security.key to be configured. + */ + "payload_protection" => "encrypt", + /** * The queue drive connection */ From ee6466b3db1e67245eb2e7727f8be0bb452f30d7 Mon Sep 17 00:00:00 2001 From: papac Date: Wed, 15 Jul 2026 02:28:04 +0000 Subject: [PATCH 5/7] Update CHANGELOG --- CHANGELOG.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index bd93e1af..7095b738 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,15 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## 5.4.11 - 2026-07-15 + +### What's Changed + +* Update CHANGELOG by @papac in https://github.com/bowphp/framework/pull/425 +* fix(queue): add beanstalkd type error by @papac in https://github.com/bowphp/framework/pull/424 + +**Full Changelog**: https://github.com/bowphp/framework/compare/5.4.10...5.4.11 + ## 5.4.10 - 2026-06-22 ### What's Changed @@ -363,6 +372,7 @@ Database::transaction(fn() => $user->update(['name' => ''])); + ``` From c2cbbb0051cde74d6b96f8c086251e7919cbb4aa Mon Sep 17 00:00:00 2001 From: papac Date: Wed, 15 Jul 2026 02:50:59 +0000 Subject: [PATCH 6/7] Update CHANGELOG --- CHANGELOG.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7095b738..94819fea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,15 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## 5.4.12 - 2026-07-15 + +### What's Changed + +* Update CHANGELOG by @papac in https://github.com/bowphp/framework/pull/426 +* fix(queue): add beanstalkd type error by @papac in https://github.com/bowphp/framework/pull/427 + +**Full Changelog**: https://github.com/bowphp/framework/compare/5.4.11...5.4.12 + ## 5.4.11 - 2026-07-15 ### What's Changed @@ -373,6 +382,7 @@ Database::transaction(fn() => $user->update(['name' => ''])); + ``` From f1d961ad77f832ff87ab19e782bf71ace1f0d03d Mon Sep 17 00:00:00 2001 From: Franck DAKIA Date: Mon, 10 Aug 2026 21:11:01 +0000 Subject: [PATCH 7/7] fix(queue): fix payload deserialization --- src/Application/Application.php | 21 ++- src/Http/Request.php | 55 ++++++- src/Queue/Adapters/BeanstalkdAdapter.php | 20 --- src/Queue/Adapters/QueueAdapter.php | 63 +++++++- src/Queue/Adapters/RabbitMQAdapter.php | 115 ++++++++++++-- tests/Application/ApplicationTest.php | 6 + tests/Http/RequestTest.php | 58 +++++++ tests/Queue/QueueAdapterSerializationTest.php | 63 ++++++++ tests/Queue/RabbitMQAdapterMessageTest.php | 146 ++++++++++++++++++ tests/Queue/Stubs/FailingQueueTaskStub.php | 27 ++++ 10 files changed, 528 insertions(+), 46 deletions(-) create mode 100644 tests/Http/RequestTest.php create mode 100644 tests/Queue/QueueAdapterSerializationTest.php create mode 100644 tests/Queue/RabbitMQAdapterMessageTest.php create mode 100644 tests/Queue/Stubs/FailingQueueTaskStub.php diff --git a/src/Application/Application.php b/src/Application/Application.php index a221efa6..5f2d9122 100644 --- a/src/Application/Application.php +++ b/src/Application/Application.php @@ -146,6 +146,18 @@ public function run(): bool $this->router->setPrefix(''); + // Raised here rather than from Request::capture(), which runs during + // this class's constructor — too early for the container or the error + // handler to turn it into a response. By now both are up, so a bad + // payload is a rendered 400 instead of an uncaught PHP fatal. + $invalid_json_payload = $this->request->getInvalidJsonPayload(); + + if (!is_null($invalid_json_payload)) { + throw new BadRequestException( + "The request json payload is invalid: " . $invalid_json_payload, + ); + } + $method = $this->request->method(); // We verify the existence of the method of the request in @@ -213,12 +225,17 @@ public function send(): bool * @param int $code * @return void */ - private function sendResponse(mixed $response, int $code = 200): void + private function sendResponse(mixed $response, ?int $code = null): void { if ($response instanceof ResponseInterface) { $response->sendContent(); } else { - echo $this->response->send($response, $code); + // Carry the status the response already holds. A controller that + // returned response()->json($data, 404) has set it on this very + // instance, and passing a hardcoded 200 here overwrote it — every + // json() error answered 200 with an error body, so clients could + // not tell success from failure. + echo $this->response->send($response, $code ?? $this->response->getCode()); } } diff --git a/src/Http/Request.php b/src/Http/Request.php index 2d64c0a1..51f125fd 100644 --- a/src/Http/Request.php +++ b/src/Http/Request.php @@ -64,6 +64,13 @@ class Request */ private bool $capture = false; + /** + * Why the JSON payload could not be decoded, when it could not be. + * + * @var string|null + */ + private ?string $invalid_json_payload = null; + /** * Check if file exists * @@ -91,12 +98,30 @@ public function capture() $this->id = "req_" . sha1(uniqid() . time()); if ($this->getHeader('content-type') == 'application/json') { - try { - $data = json_decode(file_get_contents("php://input"), true, 1024, JSON_THROW_ON_ERROR); - } catch (Throwable $e) { - throw new BadRequestException( - "The request json payload is invalid: " . $e->getMessage(), - ); + $raw = (string) file_get_contents("php://input"); + + // A bodyless request is not a malformed one. Clients routinely send + // "Content-Type: application/json" on a POST/DELETE that carries no + // payload, and json_decode('') throws — a throw this early cannot be + // rendered, because capture() runs from Application::__construct + // before the container binds "response", which BadRequestException + // needs. The client would get a raw PHP fatal instead of a 400. + if (trim($raw) === '') { + $data = []; + } else { + try { + $data = json_decode($raw, true, 1024, JSON_THROW_ON_ERROR); + } catch (Throwable $e) { + // Recorded, not thrown. capture() runs from + // Application::__construct, which is too early for ANY + // exception to be rendered: the container has not bound + // "response" yet and the error handler is installed later + // in the boot, so a throw here reaches the client as a raw + // PHP fatal instead of a 400. Application::run() raises it + // once both are in place. + $this->invalid_json_payload = $e->getMessage(); + $data = []; + } } } else { $data = $_POST ?? []; @@ -116,6 +141,17 @@ public function capture() $this->capture = true; } + /** + * The JSON decoding error for this request, or null when the payload was + * absent or well-formed. + * + * @return string|null + */ + public function getInvalidJsonPayload(): ?string + { + return $this->invalid_json_payload; + } + /** * Retrieve query variables * @@ -234,7 +270,12 @@ public function get(string $key, mixed $default = null): mixed { $value = $this->input[$key] ?? $default; - if (is_callable($value)) { + // Only a callable *default* may be resolved — a closure or an invokable + // object. Never a string or an array: those can come straight from the + // request, and is_callable() is true for the name of any defined + // function, so `?field=phpinfo` would call it and return its result in + // place of the input the caller asked for. + if (is_object($value) && is_callable($value)) { return $value(); } diff --git a/src/Queue/Adapters/BeanstalkdAdapter.php b/src/Queue/Adapters/BeanstalkdAdapter.php index 6796dd7d..c80e0e59 100644 --- a/src/Queue/Adapters/BeanstalkdAdapter.php +++ b/src/Queue/Adapters/BeanstalkdAdapter.php @@ -298,26 +298,6 @@ private function resolveFailedTask(Job $job, QueueTask $task, Throwable $excepti } } - /** - * Store the failed payload for later inspection - * - * Recording is best effort: the cache is not guaranteed to be configured in - * a worker process, and a throw here would escape the failure handler and - * kill the worker before the job is deleted, making it redeliver on TTR. - * - * @param string $key - * @param mixed $payload - * @return void - */ - private function recordFailedPayload(string $key, mixed $payload): void - { - try { - cache($key, $payload); - } catch (Throwable $exception) { - $this->logError($exception); - } - } - /** * Release the task back to the queue for retry * diff --git a/src/Queue/Adapters/QueueAdapter.php b/src/Queue/Adapters/QueueAdapter.php index c21916b4..d1e4815a 100644 --- a/src/Queue/Adapters/QueueAdapter.php +++ b/src/Queue/Adapters/QueueAdapter.php @@ -171,7 +171,48 @@ public function unserializeProducer(string $task): QueueTask ); } - return unserialize($plain); + $producer = unserialize($plain); + + // The payload is authentic but names a class this process cannot load: + // the task was renamed or removed, the worker runs an older revision than + // the producer, or another application shares the broker. PHP hands back a + // __PHP_Incomplete_Class, which the QueueTask return type would surface as + // an opaque TypeError. Name the class instead so the poison payload the + // adapters record points straight at the missing task. + if (!$producer instanceof QueueTask) { + throw new RuntimeException(sprintf( + 'Queue payload does not hold a %s, got %s.', + QueueTask::class, + $this->describeProducer($producer) + )); + } + + return $producer; + } + + /** + * Describe what came out of the payload for the rejection message + * + * A __PHP_Incomplete_Class keeps the original class name in a magic property, + * which get_class() does not expose, so read it out to report the task the + * worker is missing rather than the placeholder type. + * + * @param mixed $producer + * @return string + */ + private function describeProducer(mixed $producer): string + { + if (!is_object($producer)) { + return get_debug_type($producer); + } + + if (!$producer instanceof \__PHP_Incomplete_Class) { + return $producer::class; + } + + $name = ((array) $producer)['__PHP_Incomplete_Class_Name'] ?? 'unknown'; + + return sprintf('the unloadable class %s', $name); } /** @@ -418,6 +459,26 @@ final protected function generateId(): string return md5(uniqid((string) time(), true) . bin2hex(random_bytes(10)) . str_uuid() . microtime(true)); } + /** + * Store the failed payload for later inspection + * + * Recording is best effort: the cache is not guaranteed to be configured in + * a worker process, and a throw here would escape the failure handler and + * kill the worker before the message is settled, making it redeliver. + * + * @param string $key + * @param mixed $payload + * @return void + */ + protected function recordFailedPayload(string $key, mixed $payload): void + { + try { + cache($key, $payload); + } catch (Throwable $exception) { + $this->logError($exception); + } + } + /** * Log processing task * diff --git a/src/Queue/Adapters/RabbitMQAdapter.php b/src/Queue/Adapters/RabbitMQAdapter.php index 86d0c63b..bcd950a5 100644 --- a/src/Queue/Adapters/RabbitMQAdapter.php +++ b/src/Queue/Adapters/RabbitMQAdapter.php @@ -79,22 +79,7 @@ public function push(QueueTask $task): bool public function run(?string $queue = null): void { $queue = $this->getQueue($queue); - $callback = function ($msg) { - $task = $this->unserializeProducer($msg->body); - try { - $this->logProcessingTask($task); - if (!method_exists($task, 'process')) { - throw new \RuntimeException('Task does not have a process or handle method.'); - } - $task->process(); - $this->logProcessedTask($task); - $msg->ack(); - } catch (\Throwable $e) { - $this->logFailedTask($task, $e); - // Optionally requeue: set second param to true to requeue - $msg->nack(false, false); // reject and don't requeue - } - }; + $callback = fn ($msg) => $this->processMessage($msg); $this->channel->basic_qos(null, 1, null); $this->channel->basic_consume($queue, '', false, false, false, false, $callback); while ($this->channel->is_consuming()) { @@ -109,6 +94,104 @@ public function run(?string $queue = null): void } } + /** + * Process a consumed message + * + * @param object $msg + * @return void + */ + protected function processMessage(object $msg): void + { + $task = null; + + try { + // unserializeProducer() belongs inside the try: a payload that fails + // integrity verification, or that names a task class this worker + // cannot load, throws here. Letting it escape the consumer callback + // leaves the message neither acked nor nacked, so the broker + // redelivers it and the worker crash-loops on the same poison bytes. + $task = $this->unserializeProducer($msg->body); + + $this->logProcessingTask($task); + + if (!method_exists($task, 'process')) { + throw new RuntimeException('Task does not have a process or handle method.'); + } + + $task->process(); + $this->logProcessedTask($task); + $msg->ack(); + } catch (\Throwable $e) { + $this->handleMessageFailure($msg, $task, $e); + } + } + + /** + * Settle a message whose processing failed + * + * Mirrors the beanstalkd adapter: the task decides, through onException() + * and taskShouldBeDelete(), whether the failure is terminal. A transient + * failure is requeued instead of being dropped on the first throw, and the + * throttle keeps a permanently failing task from spinning the worker hot. + * + * AMQP has no per-message delay without a delayed-exchange plugin, so + * getDelay() cannot be honoured here; the requeue is immediate. + * + * @param object $msg + * @param QueueTask|null $task + * @param \Throwable $exception + * @return void + */ + private function handleMessageFailure(object $msg, ?QueueTask $task, \Throwable $exception): void + { + $this->logFailedTask($task, $exception); + + // Poison message: the body never became a task, so there is no id to key + // on and nothing to retry — a requeue would redeliver the same bytes for + // ever. Keep the raw body for inspection and reject it for good. + if (is_null($task)) { + $this->recordFailedPayload("task:failed:body:" . md5($msg->body), $msg->body); + $msg->nack(false, false); + + return; + } + + $msg->nack(!$this->resolveFailedTask($task, $exception), false); + + $this->sleep(1); + } + + /** + * Run the task defined failure handling and decide whether to drop the message + * + * onException() and taskShouldBeDelete() are both overridable, so they are + * user code and may throw. A throw must not escape, otherwise the message is + * never settled, it redelivers and the worker crash-loops on it. + * + * @param QueueTask $task + * @param \Throwable $exception + * @return bool Whether the message should be dropped + */ + private function resolveFailedTask(QueueTask $task, \Throwable $exception): bool + { + try { + $this->recordFailedPayload( + "task:failed:" . $task->getId(), + method_exists($task, 'getData') ? $task->getData() : "" + ); + + $task->onException($exception); + + return $task->taskShouldBeDelete(); + } catch (\Throwable $taskException) { + $this->logError($taskException); + + // The task cannot handle its own failure, so retrying it would most + // likely break the same way. Drop it rather than requeue for ever. + return true; + } + } + /** * Get the queue size * diff --git a/tests/Application/ApplicationTest.php b/tests/Application/ApplicationTest.php index 0ebd2fbb..7f9e7ae1 100644 --- a/tests/Application/ApplicationTest.php +++ b/tests/Application/ApplicationTest.php @@ -44,6 +44,8 @@ private function createRequestMock(string $method = 'GET', string $path = '/'): $request->allows()->path()->andReturns($path); $request->allows()->get("_method")->andReturns(""); $request->allows()->domain()->andReturns("localhost"); + // run() asks whether the JSON body failed to decode; null = it did not. + $request->allows()->getInvalidJsonPayload()->andReturns(null); return $request; } @@ -57,6 +59,9 @@ private function createResponseMock(int $expectedStatus = 200): Response $response->allows()->withHeader('X-Powered-By', 'Bow Framework'); $response->allows()->status($expectedStatus); $response->allows()->send(Mockery::any(), Mockery::any())->andReturn(''); + // sendResponse() now reads the status already held by the response + // instead of overwriting it with a hardcoded 200. + $response->allows()->getCode()->andReturns($expectedStatus); return $response; } @@ -146,6 +151,7 @@ public function test_disable_powered_by_mention() $response->shouldNotReceive('withHeader')->with('X-Powered-By', Mockery::any()); $response->allows()->status(200); $response->allows()->send(Mockery::any(), Mockery::any())->andReturn(''); + $response->allows()->getCode()->andReturns(200); $config = $this->createConfigMock(); diff --git a/tests/Http/RequestTest.php b/tests/Http/RequestTest.php new file mode 100644 index 00000000..d0093159 --- /dev/null +++ b/tests/Http/RequestTest.php @@ -0,0 +1,58 @@ +assertTrue(is_callable($value), "fixture {$value} must be a real function"); + + $_POST = ['field' => $value]; + + $request = new Request(); + $request->capture(); + + $this->assertSame($value, $request->get('field')); + } + + /** A callable default is the feature this guard must preserve. */ + public function testCallableDefaultIsStillResolved(): void + { + $request = new Request(); + $request->capture(); + + $this->assertSame('resolved', $request->get('absent', fn () => 'resolved')); + $this->assertSame('plain', $request->get('absent', 'plain')); + } + + /** @return array */ + public static function callableLookingInput(): array + { + return [ + 'php builtin' => ['phpinfo'], + 'string function' => ['trim'], + 'array function' => ['compact'], + ]; + } +} diff --git a/tests/Queue/QueueAdapterSerializationTest.php b/tests/Queue/QueueAdapterSerializationTest.php new file mode 100644 index 00000000..4661b933 --- /dev/null +++ b/tests/Queue/QueueAdapterSerializationTest.php @@ -0,0 +1,63 @@ +adapter = new class extends QueueAdapter { + public function configure(array $config): QueueAdapter + { + return $this; + } + + public function push(QueueTask $task): bool + { + return true; + } + }; + } + + public function test_it_round_trips_a_task(): void + { + $payload = $this->adapter->serializeProducer(new BasicQueueTaskStub("round-trip")); + + $this->assertInstanceOf(BasicQueueTaskStub::class, $this->adapter->unserializeProducer($payload)); + } + + public function test_it_rejects_a_payload_whose_task_class_is_not_loadable(): void + { + // A task enqueued by a producer running code the worker does not have: + // the class was renamed, removed, or lives in another service. unserialize() + // yields a __PHP_Incomplete_Class, which must not escape as a TypeError. + $payload = Crypto::encrypt('O:35:"App\Tasks\SyncWhatsAppTemplatesTask":0:{}'); + + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage('App\Tasks\SyncWhatsAppTemplatesTask'); + + $this->adapter->unserializeProducer($payload); + } + + public function test_it_rejects_a_payload_that_does_not_hold_a_task(): void + { + $payload = Crypto::encrypt(serialize(['id' => 1])); + + $this->expectException(RuntimeException::class); + + $this->adapter->unserializeProducer($payload); + } +} diff --git a/tests/Queue/RabbitMQAdapterMessageTest.php b/tests/Queue/RabbitMQAdapterMessageTest.php new file mode 100644 index 00000000..739488a5 --- /dev/null +++ b/tests/Queue/RabbitMQAdapterMessageTest.php @@ -0,0 +1,146 @@ +adapter = new class extends RabbitMQAdapter { + public function consume(object $message): void + { + $this->processMessage($message); + } + + // The failure path throttles with a real sleep; tests must not pay it. + public function sleep(int $seconds): void + { + } + }; + } + + public function test_it_acknowledges_a_processed_message(): void + { + $message = $this->message( + $this->adapter->serializeProducer(new BasicQueueTaskStub("rabbitmq_ack")) + ); + + $this->adapter->consume($message); + + $this->assertSame(1, $message->acked); + $this->assertSame(0, $message->nacked); + } + + public function test_it_nacks_a_payload_whose_task_class_is_not_loadable(): void + { + // The unserialize failure must not escape the consumer callback: an + // unacked, unnacked message is redelivered forever and crash-loops the + // worker on the same poison payload. + $message = $this->message(Crypto::encrypt('O:35:"App\Tasks\SyncWhatsAppTemplatesTask":0:{}')); + + $this->adapter->consume($message); + + $this->assertSame(0, $message->acked); + $this->assertSame(1, $message->nacked); + $this->assertFalse($message->requeued); + } + + public function test_it_nacks_a_tampered_payload(): void + { + $message = $this->message("not-a-valid-payload"); + + $this->adapter->consume($message); + + $this->assertSame(0, $message->acked); + $this->assertSame(1, $message->nacked); + $this->assertFalse($message->requeued); + } + + public function test_it_requeues_a_task_that_failed_but_may_be_retried(): void + { + // A transient failure (broker blip, timeout) must go back on the queue + // instead of being discarded on the first throw. + $message = $this->message($this->adapter->serializeProducer( + $this->failingTask() + )); + + $this->adapter->consume($message); + + $this->assertSame(0, $message->acked); + $this->assertSame(1, $message->nacked); + $this->assertTrue($message->requeued); + } + + public function test_it_drops_a_failed_task_that_asked_to_be_deleted(): void + { + $message = $this->message($this->adapter->serializeProducer( + $this->failingTask(dropAfterFailure: true) + )); + + $this->adapter->consume($message); + + $this->assertSame(0, $message->acked); + $this->assertSame(1, $message->nacked); + $this->assertFalse($message->requeued); + } + + /** + * Build a failing task the way push() delivers one: with an id already set. + */ + private function failingTask(bool $dropAfterFailure = false): FailingQueueTaskStub + { + $task = new FailingQueueTaskStub($dropAfterFailure); + $task->setId("rabbitmq-failing-task"); + + return $task; + } + + private function message(string $body): object + { + return new class ($body) { + public int $acked = 0; + public int $nacked = 0; + public bool $requeued = false; + + public function __construct(public string $body) + { + } + + public function ack(): void + { + $this->acked++; + } + + public function nack(bool $requeue = false, bool $multiple = false): void + { + $this->nacked++; + $this->requeued = $requeue; + } + }; + } +} diff --git a/tests/Queue/Stubs/FailingQueueTaskStub.php b/tests/Queue/Stubs/FailingQueueTaskStub.php new file mode 100644 index 00000000..d7827596 --- /dev/null +++ b/tests/Queue/Stubs/FailingQueueTaskStub.php @@ -0,0 +1,27 @@ +dropAfterFailure) { + $this->deleteTask(); + } + } +}