From b4c6ff663728f160830e363874ccf3e7192ac95e Mon Sep 17 00:00:00 2001 From: Sola Date: Sun, 27 Sep 2026 01:47:24 -0600 Subject: [PATCH 01/12] Fix dead validation checks and restore deprecation reporting - app/helper/view.php: _parseTextile() now restores the previous error_reporting level in a finally block instead of suppressing E_DEPRECATED for the remainder of the request - app/controller/index.php, app/controller/admin.php: username validation compared preg_match() result with === false, which can never match (returns 0 on non-match); use !== 1 and anchor the admin pattern so it actually rejects invalid usernames - app/controller/admin.php: same dead-check fix for task_color, and validate the group change_task_color AJAX endpoint (was storing raw POST data rendered into a style attribute); returns 400 on invalid input - app/model/issue.php: due_date normalization had the same dead preg_match check; restore intended behavior with a guard so unparseable dates are left untouched --- app/controller/admin.php | 11 ++++++++--- app/controller/index.php | 2 +- app/helper/view.php | 17 ++++++++++++----- app/model/issue.php | 7 +++++-- 4 files changed, 26 insertions(+), 11 deletions(-) diff --git a/app/controller/admin.php b/app/controller/admin.php index cf7a1cbe..ecfd399d 100644 --- a/app/controller/admin.php +++ b/app/controller/admin.php @@ -331,11 +331,11 @@ public function user_save(\Base $f3): void throw new \Exception("Please enter a name."); } - if (preg_match("/#?[0-9a-f]{3,6}/i", (string) $f3->get("POST.task_color")) === false) { + if (preg_match("/^#?[0-9a-f]{3,6}$/i", (string) $f3->get("POST.task_color")) !== 1) { throw new \Exception("Please enter a valid hex color."); } - if (preg_match("/[0-9a-z_-]+/i", (string) $f3->get("POST.username")) === false) { + if (preg_match("/^[0-9a-z_-]+$/i", (string) $f3->get("POST.username")) !== 1) { throw new \Exception("Usernames can only contain letters, numbers, hyphens, and underscores."); } @@ -555,7 +555,12 @@ public function group_ajax(\Base $f3): void $this->_printJson(["changed" => 1]); break; case "change_task_color": - $group->task_color = ltrim((string) $f3->get("POST.value"), '#'); + $color = ltrim((string) $f3->get("POST.value"), '#'); + if (!ctype_xdigit($color) || (strlen($color) !== 3 && strlen($color) !== 6)) { + $f3->error(400); + return; + } + $group->task_color = $color; $group->save(); $this->_printJson(["changed" => 1]); break; diff --git a/app/controller/index.php b/app/controller/index.php index 78725ac7..0b62cc67 100644 --- a/app/controller/index.php +++ b/app/controller/index.php @@ -148,7 +148,7 @@ public function registerpost($f3): void $errors[] = "Name is required"; } - if (preg_match("/^[0-9a-z]{4,}$/i", (string) $f3->get("POST.register-username")) === false) { + if (preg_match("/^[0-9a-z]{4,}$/i", (string) $f3->get("POST.register-username")) !== 1) { $errors[] = "Usernames must be at least 4 characters and can only contain letters and numbers."; } diff --git a/app/helper/view.php b/app/helper/view.php index e9f4c482..6c267994 100644 --- a/app/helper/view.php +++ b/app/helper/view.php @@ -258,11 +258,18 @@ protected function _parseEmoticons(string $str): string */ protected function _parseTextile(string $str, bool $escape = true): string { - error_reporting(E_ALL ^ E_DEPRECATED); // Temporarily ignore deprecations because this lib is incompatible - $tex = new \Netcarver\Textile\Parser('html5'); - $tex->setDimensionlessImages(true); - $tex->setRestricted($escape); - return $tex->parse($str); + // Temporarily ignore deprecations because this lib is incompatible, + // restoring the previous level afterwards so deprecations elsewhere + // in the request are still reported. + $level = error_reporting(E_ALL ^ E_DEPRECATED); + try { + $tex = new \Netcarver\Textile\Parser('html5'); + $tex->setDimensionlessImages(true); + $tex->setRestricted($escape); + return $tex->parse($str); + } finally { + error_reporting($level); + } } /** diff --git a/app/model/issue.php b/app/model/issue.php index 9e37f1a2..fc45b742 100644 --- a/app/model/issue.php +++ b/app/model/issue.php @@ -50,8 +50,11 @@ public static function create(array $data, bool $notify = true): static } if (!empty($data["due_date"])) { - if (preg_match("/\\d{4}(-\\d{2}){2}/", (string) $data["due_date"]) === false) { - $data["due_date"] = date("Y-m-d", strtotime((string) $data["due_date"])); + if ( + preg_match("/\\d{4}(-\\d{2}){2}/", (string) $data["due_date"]) !== 1 + && ($parsed = strtotime((string) $data["due_date"])) !== false + ) { + $data["due_date"] = date("Y-m-d", $parsed); } if (empty($data["sprint_id"]) && !empty($data['due_date_sprint'])) { From c2cfb4a621261410a5765e08e6911c4dae25d287 Mon Sep 17 00:00:00 2001 From: Sola Date: Sun, 27 Sep 2026 01:47:25 -0600 Subject: [PATCH 02/12] Harden cron, migrations, Docker, and session handling - cron/base.php: load the installer-written root config.php (the cron/config.php it required was never created, so every cron job fataled); keep cron/config.php as a legacy fallback. Connect with the same MySQL/SQLite engine switch as index.php and honor db.port instead of hardcoding 3306. - app/helper/security.php: run each migration file inside a transaction, skip empty statements, and return success/failure so callers can stop instead of spinning on a broken migration. - index.php: apply all pending migrations in one request (with an iteration cap) instead of one per request; set the Secure flag on cookies when served over HTTPS. - Dockerfile: install imap (mail-to-issue crons) and pdo_sqlite; set post_max_size to match upload_max_filesize (8M default made the 1024M limit ineffective); add a HEALTHCHECK. - nginx-example.conf: align client_max_body_size with PHP limits. - uploads/.htaccess: add Apache 2.4 'Require all denied'. - composer.json: require phpstan/phpstan explicitly instead of inheriting it transitively from rector. - README.md: maintenance notes (framework fork contingency, vendored-JS accepted risk, API key header preference, log rotation, cron setup). --- Dockerfile | 8 ++++++-- README.md | 10 ++++++++++ app/helper/security.php | 41 +++++++++++++++++++++++++++++------------ composer.json | 1 + composer.lock | 36 ++++++++++++++++++------------------ cron/base.php | 21 +++++++++++++++------ index.php | 14 ++++++++++---- nginx-example.conf | 2 +- uploads/.htaccess | 5 +++++ 9 files changed, 95 insertions(+), 43 deletions(-) diff --git a/Dockerfile b/Dockerfile index 200f9868..0e62d7b1 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,10 +1,14 @@ FROM docker.io/library/php:8.4-apache COPY --from=ghcr.io/mlocati/php-extension-installer /usr/bin/install-php-extensions /usr/local/bin/ -RUN install-php-extensions exif gd zip pdo_mysql intl +RUN install-php-extensions exif gd zip pdo_mysql pdo_sqlite intl imap RUN a2enmod rewrite +# Container health check (Apache on :80) +HEALTHCHECK --interval=30s --timeout=5s --start-period=15s \ + CMD php -r "exit(@fsockopen('127.0.0.1', 80) ? 0 : 1);" + COPY --chown=33:33 . /var/www/html/ -RUN echo 'upload_max_filesize = 1024M' > /usr/local/etc/php/conf.d/limits.ini +RUN printf 'upload_max_filesize = 1024M\npost_max_size = 1024M\n' > /usr/local/etc/php/conf.d/limits.ini RUN mv "$PHP_INI_DIR/php.ini-production" "$PHP_INI_DIR/php.ini" diff --git a/README.md b/README.md index c919e39f..45a3cd5c 100644 --- a/README.md +++ b/README.md @@ -15,6 +15,16 @@ Detailed requirements and installation instructions are available at [phproject. Phproject uses [Composer](https://getcomposer.org/) for dependency management. After cloning the repository, run `composer install` to install the required packages. +## Maintenance notes + +A few things worth knowing when keeping a Phproject install running long-term: + +- **Framework contingency.** Phproject is built on `bcosca/fatfree-core`, which has a small maintainer base. A mirror is kept at [Alanaktion/fatfree-core](https://github.com/Alanaktion/fatfree-core); if upstream ever goes unmaintained, point Composer at the fork by adding a VCS repository entry and requiring the same version constraint — no code changes needed. +- **Vendored JavaScript is frozen.** The JS libraries in `js/` (Bootstrap 3.4.1, jQuery 3.6.3, bootstrap-datepicker, EasyMDE) are pinned and no longer receive upstream security fixes. This is accepted risk: they are only served as static assets, and user content rendered through them is escaped server-side. Re-vendor only if a concrete issue arises. +- **API keys.** Pass API keys via the `X-API-Key` (or `X-Redmine-API-Key`) request header. The `?key=` query-string parameter still works but is deprecated — it leaks keys into server access logs. +- **Logs.** The application appends to `log/` indefinitely. In production, mount `log/` as a volume and rotate it (e.g. with `logrotate`) so a long-running instance can't fill its disk. +- **Cron jobs** (`cron/`) read database settings from the root `config.php` written by the installer, and support both MySQL and SQLite. Schedule them with the system cron, e.g. `*/5 * * * * php /var/www/html/cron/due_alerts.php`. + ## Contributing Phproject is maintained as an open source project for use by anyone around the world under the [GNU General Public License](http://www.gnu.org/licenses/gpl-3.0.txt). If you find a bug or would like a new feature added, [open an issue](https://github.com/Alanaktion/phproject/issues/new) or [submit a pull request](https://github.com/Alanaktion/phproject/compare/) with new code. If you want to help with translation, [you can submit translations via Crowdin](https://crowdin.com/project/phproject). diff --git a/app/helper/security.php b/app/helper/security.php index 7d141eb8..076016f9 100644 --- a/app/helper/security.php +++ b/app/helper/security.php @@ -103,23 +103,40 @@ public function checkDatabaseVersion(): string|bool } /** - * Install latest core database updates + * Install a core database update + * @return bool TRUE if the update was applied, FALSE on failure */ - public function updateDatabase(string $version): void + public function updateDatabase(string $version): bool { $f3 = \Base::instance(); - if (file_exists("db/{$version}.sql")) { - $update_db = file_get_contents("db/{$version}.sql"); - $db = $f3->get("db.instance"); - foreach (explode(";", $update_db) as $stmt) { - $db->exec($stmt); - } - - \Cache::instance()->reset(); - $f3->set("success", " Database updated to version: {$version}"); - } else { + if (!file_exists("db/{$version}.sql")) { $f3->set("error", " Database file not found for version: {$version}"); + return false; } + + $update_db = file_get_contents("db/{$version}.sql"); + $db = $f3->get("db.instance"); + try { + // Note: MySQL implicitly commits around DDL statements, so a + // failed migration there can't be rolled back; the version row + // is only updated by the last statement, so a failure leaves + // the database at the previous version and surfaces an error. + $db->begin(); + foreach (explode(";", (string) $update_db) as $stmt) { + if (trim($stmt) !== "") { + $db->exec($stmt); + } + } + $db->commit(); + } catch (\Throwable $e) { + $db->rollback(); + $f3->set("error", " Database update to version {$version} failed: " . $e->getMessage()); + return false; + } + + \Cache::instance()->reset(); + $f3->set("success", " Database updated to version: {$version}"); + return true; } /** diff --git a/composer.json b/composer.json index 48309c52..6bde32b5 100644 --- a/composer.json +++ b/composer.json @@ -22,6 +22,7 @@ } }, "require-dev": { + "phpstan/phpstan": "^2.2.14", "squizlabs/php_codesniffer": "^3.13.6", "rector/rector": "^2.6.7", "phpunit/phpunit": "^10.5.64" diff --git a/composer.lock b/composer.lock index 9ce87d2a..24eaa3c0 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "d5b83eee918589c8d5a02236036dd977", + "content-hash": "7dd64d8298bcfa48fcf5e362e7ea53f7", "packages": [ { "name": "bcosca/fatfree-core", @@ -128,16 +128,16 @@ }, { "name": "league/commonmark", - "version": "2.10.1", + "version": "2.10.3", "source": { "type": "git", "url": "https://github.com/thephpleague/commonmark.git", - "reference": "9d489ab67a02960fd8ffe624d93f751daf95439e" + "reference": "6efbd9c472b91db0a3350fcd601c8332c2382e1f" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/thephpleague/commonmark/zipball/9d489ab67a02960fd8ffe624d93f751daf95439e", - "reference": "9d489ab67a02960fd8ffe624d93f751daf95439e", + "url": "https://api.github.com/repos/thephpleague/commonmark/zipball/6efbd9c472b91db0a3350fcd601c8332c2382e1f", + "reference": "6efbd9c472b91db0a3350fcd601c8332c2382e1f", "shasum": "" }, "require": { @@ -231,7 +231,7 @@ "type": "tidelift" } ], - "time": "2026-09-07T13:44:26+00:00" + "time": "2026-09-21T13:07:34+00:00" }, { "name": "league/config", @@ -1015,11 +1015,11 @@ }, { "name": "phpstan/phpstan", - "version": "2.2.14", + "version": "2.2.16", "dist": { "type": "zip", - "url": "https://api.github.com/repos/phpstan/phpstan/zipball/9c672e7a8e791dfc3d30e55f683e73fc0b63a3ac", - "reference": "9c672e7a8e791dfc3d30e55f683e73fc0b63a3ac", + "url": "https://api.github.com/repos/phpstan/phpstan/zipball/46a6d9060e5a7763adfcc21ebcb8b504ebdbcb92", + "reference": "46a6d9060e5a7763adfcc21ebcb8b504ebdbcb92", "shasum": "" }, "require": { @@ -1075,7 +1075,7 @@ "type": "github" } ], - "time": "2026-09-12T21:39:33+00:00" + "time": "2026-09-25T09:31:51+00:00" }, { "name": "phpunit/php-code-coverage", @@ -1400,16 +1400,16 @@ }, { "name": "phpunit/phpunit", - "version": "10.5.64", + "version": "10.5.65", "source": { "type": "git", "url": "https://github.com/sebastianbergmann/phpunit.git", - "reference": "0e8c1d19cea35ad97d4887f363d07c78e30fbf06" + "reference": "b4f678d4ebb686d6d3bd6a7c193e8a067d1a7f01" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/sebastianbergmann/phpunit/zipball/0e8c1d19cea35ad97d4887f363d07c78e30fbf06", - "reference": "0e8c1d19cea35ad97d4887f363d07c78e30fbf06", + "url": "https://api.github.com/repos/sebastianbergmann/phpunit/zipball/b4f678d4ebb686d6d3bd6a7c193e8a067d1a7f01", + "reference": "b4f678d4ebb686d6d3bd6a7c193e8a067d1a7f01", "shasum": "" }, "require": { @@ -1419,7 +1419,7 @@ "ext-libxml": "*", "ext-mbstring": "*", "ext-xmlwriter": "*", - "myclabs/deep-copy": "^1.13.4", + "myclabs/deep-copy": "^1.14.0", "phar-io/manifest": "^2.0.4", "phar-io/version": "^3.2.1", "php": ">=8.1", @@ -1436,7 +1436,7 @@ "sebastian/exporter": "^5.1.4", "sebastian/global-state": "^6.0.2", "sebastian/object-enumerator": "^5.0.0", - "sebastian/recursion-context": "^5.0.1", + "sebastian/recursion-context": "^5.0.2", "sebastian/type": "^4.0.0", "sebastian/version": "^4.0.1" }, @@ -1481,7 +1481,7 @@ "support": { "issues": "https://github.com/sebastianbergmann/phpunit/issues", "security": "https://github.com/sebastianbergmann/phpunit/security/policy", - "source": "https://github.com/sebastianbergmann/phpunit/tree/10.5.64" + "source": "https://github.com/sebastianbergmann/phpunit/tree/10.5.65" }, "funding": [ { @@ -1489,7 +1489,7 @@ "type": "other" } ], - "time": "2026-07-06T14:50:35+00:00" + "time": "2026-09-23T10:01:12+00:00" }, { "name": "rector/rector", diff --git a/cron/base.php b/cron/base.php index 92fd29bc..e0d67de1 100644 --- a/cron/base.php +++ b/cron/base.php @@ -23,14 +23,23 @@ ]); // Load local configuration -$f3->mset(require_once(__DIR__ . '/config.php')); +$config_file = $homedir . "config.php"; +if (!is_file($config_file) && is_file(__DIR__ . "/config.php")) { + // Fall back to legacy cron-specific config location + $config_file = __DIR__ . "/config.php"; +} +$f3->mset(require_once($config_file)); // Connect to database -$f3->set("db.instance", new \Helper\SQL( - "mysql:host=" . $f3->get("db.host") . ";port=3306;dbname=" . $f3->get("db.name"), - $f3->get("db.user"), - $f3->get("db.pass") -)); +if ($f3->get("db.engine") == "sqlite") { + $f3->set("db.instance", new \Helper\SQL("sqlite:" . $f3->get("db.name"))); +} else { + $f3->set("db.instance", new \Helper\SQL( + "mysql:host=" . $f3->get("db.host") . ";port=" . ($f3->get("db.port") ?: 3306) . ";dbname=" . $f3->get("db.name"), + $f3->get("db.user"), + $f3->get("db.pass") + )); +} // Load database-backed config \Model\Config::loadAll(); diff --git a/index.php b/index.php index b1bda3e5..2414202f 100644 --- a/index.php +++ b/index.php @@ -16,6 +16,7 @@ "CACHE" => true, "AUTOLOAD" => "app/;lib/vendor/", "JAR.samesite" => "Lax", + "JAR.secure" => $f3->get("SCHEME") == "https", "PACKAGE" => "Phproject", "TZ" => "UTC", "microtime" => microtime(true), @@ -89,10 +90,15 @@ // Load final configuration \Model\Config::loadAll(); -// Ensure database is up to date -$version = \Helper\Security::instance()->checkDatabaseVersion(); -if ($version !== true) { - \Helper\Security::instance()->updateDatabase($version); +// Ensure database is up to date, applying all pending migrations in one request +$security = \Helper\Security::instance(); +$migrations = 0; +while (($version = $security->checkDatabaseVersion()) !== true && $migrations < 100) { + if (!$security->updateDatabase($version)) { + break; + } + $f3->set("version", $version); + $migrations++; } // Set up CSRF protection diff --git a/nginx-example.conf b/nginx-example.conf index d7e34336..5959c39c 100644 --- a/nginx-example.conf +++ b/nginx-example.conf @@ -32,5 +32,5 @@ server { include fastcgi_params; } - client_max_body_size 64M; + client_max_body_size 1024M; # must be >= PHP post_max_size (see Dockerfile) } diff --git a/uploads/.htaccess b/uploads/.htaccess index 3a428827..da186912 100644 --- a/uploads/.htaccess +++ b/uploads/.htaccess @@ -1 +1,6 @@ +# Apache 2.2 syntax (ignored on 2.4 without mod_access_compat) Deny from all +# Apache 2.4 syntax + + Require all denied + From c187ec35fbc66c1d9ae3aa62857fda924cbce8ff Mon Sep 17 00:00:00 2001 From: Sola Date: Sun, 27 Sep 2026 01:47:25 -0600 Subject: [PATCH 03/12] Add public route smoke tests Mock the login page, home, opensearch.xml, and the issues API without a key, asserting none of them fatal (HTTP 500). Catches PHP-version incompatibilities in controllers/templates. Runs in CI where config.php exists; skips otherwise like the other web tests. --- tests/webFeatureTest.php | 45 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/tests/webFeatureTest.php b/tests/webFeatureTest.php index 7ddab495..f5a82abf 100644 --- a/tests/webFeatureTest.php +++ b/tests/webFeatureTest.php @@ -214,4 +214,49 @@ public function testIssueSaveCreatesIssueViaWebRoute(): void $issue->delete(false); } } + + /** + * Public routes that should respond without a fatal error. + * @return array + */ + public static function publicRouteProvider(): array + { + return [ + 'login page renders' => ['GET /login', 200], + 'anonymous home reroutes' => ['GET /', null], + 'opensearch renders' => ['GET /opensearch.xml', 200], + 'api without key is rejected' => ['GET /issues.json', 401], + ]; + } + + /** + * Smoke test: hit public routes and assert none of them fatal (HTTP 500). + * Catches PHP-version incompatibilities in controllers/templates early. + */ + #[\PHPUnit\Framework\Attributes\DataProvider('publicRouteProvider')] + public function testPublicRouteDoesNotFatal(string $route, ?int $expectedStatus): void + { + if (!$this->configured) { + $this->markTestSkipped(); + } + + // Ensure no user leaks in from other tests; these routes are public + // and the API case specifically asserts anonymous access is rejected. + $f3 = \Base::instance(); + $f3->clear('user'); + $f3->clear('user_obj'); + + $output = $this->mock($route); + $this->assertIsString($output, "Route {$route} failed to mock"); + + // The test error handler renders JSON with a status code on failure; + // anything else is a normal (HTML or empty reroute) response. + $decoded = json_decode($output, true); + $status = is_array($decoded) && isset($decoded['status']) ? (int) $decoded['status'] : 200; + + $this->assertNotSame(500, $status, "Route {$route} caused a fatal error"); + if ($expectedStatus !== null) { + $this->assertSame($expectedStatus, $status, "Route {$route} returned an unexpected status"); + } + } } From 69dfeaf1fcd55ac0a1bfc4f8a6f9abdf4698ec8f Mon Sep 17 00:00:00 2001 From: Sola Date: Sun, 27 Sep 2026 01:49:28 -0600 Subject: [PATCH 04/12] Address CodeRabbit review feedback - admin.php: task-color validation now accepts only 3- or 6-digit hex, matching the group color validator (4- and 5-digit values were accepted but treated as invalid elsewhere). - index.php: serialize the migration loop with a non-blocking file lock so concurrent requests can't execute the same migration twice; a request that can't acquire the lock skips, since another request is already migrating. - security.php: document that MySQL DDL commits implicitly, so a failed migration may leave applied DDL in place and need manual cleanup before retry (SQLite migrations remain fully atomic). - Dockerfile/nginx-example.conf: post_max_size and client_max_body_size raised to 1100M so a full-size 1024M upload plus multipart overhead isn't rejected. - tests: read the mocked route's HTTP status from F3's ERROR hive instead of parsing output, so partial output before a failure can't mask a 500; replicate index.php's global issue_types for full-page renders; add a regression test for the partial-output case. --- Dockerfile | 2 +- app/controller/admin.php | 2 +- app/helper/security.php | 7 ++++-- index.php | 29 ++++++++++++++++------- nginx-example.conf | 2 +- tests/webFeatureTest.php | 51 ++++++++++++++++++++++++++++++++++++---- 6 files changed, 76 insertions(+), 17 deletions(-) diff --git a/Dockerfile b/Dockerfile index 0e62d7b1..047863f9 100644 --- a/Dockerfile +++ b/Dockerfile @@ -10,5 +10,5 @@ HEALTHCHECK --interval=30s --timeout=5s --start-period=15s \ CMD php -r "exit(@fsockopen('127.0.0.1', 80) ? 0 : 1);" COPY --chown=33:33 . /var/www/html/ -RUN printf 'upload_max_filesize = 1024M\npost_max_size = 1024M\n' > /usr/local/etc/php/conf.d/limits.ini +RUN printf 'upload_max_filesize = 1024M\npost_max_size = 1100M\n' > /usr/local/etc/php/conf.d/limits.ini RUN mv "$PHP_INI_DIR/php.ini-production" "$PHP_INI_DIR/php.ini" diff --git a/app/controller/admin.php b/app/controller/admin.php index ecfd399d..a6058ba4 100644 --- a/app/controller/admin.php +++ b/app/controller/admin.php @@ -331,7 +331,7 @@ public function user_save(\Base $f3): void throw new \Exception("Please enter a name."); } - if (preg_match("/^#?[0-9a-f]{3,6}$/i", (string) $f3->get("POST.task_color")) !== 1) { + if (preg_match("/^#?(?:[0-9a-f]{3}|[0-9a-f]{6})$/i", (string) $f3->get("POST.task_color")) !== 1) { throw new \Exception("Please enter a valid hex color."); } diff --git a/app/helper/security.php b/app/helper/security.php index 076016f9..eac9b8b1 100644 --- a/app/helper/security.php +++ b/app/helper/security.php @@ -118,9 +118,12 @@ public function updateDatabase(string $version): bool $db = $f3->get("db.instance"); try { // Note: MySQL implicitly commits around DDL statements, so a - // failed migration there can't be rolled back; the version row + // failed migration there can't be rolled back. The version row // is only updated by the last statement, so a failure leaves - // the database at the previous version and surfaces an error. + // the database at the previous version and surfaces an error, + // but already-applied DDL (e.g. an added column) stays in place + // and may need manual cleanup before the migration can be + // retried. SQLite migrations are fully atomic. $db->begin(); foreach (explode(";", (string) $update_db) as $stmt) { if (trim($stmt) !== "") { diff --git a/index.php b/index.php index 2414202f..ab3000cd 100644 --- a/index.php +++ b/index.php @@ -90,15 +90,28 @@ // Load final configuration \Model\Config::loadAll(); -// Ensure database is up to date, applying all pending migrations in one request -$security = \Helper\Security::instance(); -$migrations = 0; -while (($version = $security->checkDatabaseVersion()) !== true && $migrations < 100) { - if (!$security->updateDatabase($version)) { - break; +// Ensure database is up to date, applying all pending migrations in one request. +// Serialized with a non-blocking file lock so concurrent requests can't run +// the same migration twice; a request that can't acquire the lock skips this, +// since another request is already migrating. +$lock = fopen($f3->get("TEMP") . "migrations.lock", "c"); +if ($lock && flock($lock, LOCK_EX | LOCK_NB)) { + try { + $security = \Helper\Security::instance(); + $migrations = 0; + while (($version = $security->checkDatabaseVersion()) !== true && $migrations < 100) { + if (!$security->updateDatabase($version)) { + break; + } + $f3->set("version", $version); + $migrations++; + } + } finally { + flock($lock, LOCK_UN); + fclose($lock); } - $f3->set("version", $version); - $migrations++; +} elseif ($lock) { + fclose($lock); } // Set up CSRF protection diff --git a/nginx-example.conf b/nginx-example.conf index 5959c39c..45c9b28a 100644 --- a/nginx-example.conf +++ b/nginx-example.conf @@ -32,5 +32,5 @@ server { include fastcgi_params; } - client_max_body_size 1024M; # must be >= PHP post_max_size (see Dockerfile) + client_max_body_size 1100M; # must be >= PHP post_max_size (see Dockerfile) } diff --git a/tests/webFeatureTest.php b/tests/webFeatureTest.php index f5a82abf..b9525d29 100644 --- a/tests/webFeatureTest.php +++ b/tests/webFeatureTest.php @@ -12,6 +12,12 @@ class WebFeatureTest extends TestCase protected bool $configured = false; protected int $initialOutputBufferLevel = 0; + /** + * HTTP status captured from F3's error handling during the last mock() + * call; null when the route completed without an error. + */ + protected ?int $lastMockStatus = null; + protected function setUp(): void { $this->initialOutputBufferLevel = ob_get_level(); @@ -48,6 +54,10 @@ protected function setUp(): void \Model\Config::loadAll(); \Helper\Security::instance()->initCsrfToken(); + // Replicate index.php's global template data so full-page renders + // (e.g. the footer's issue-type picker) behave as in production. + $f3->set('issue_types', (new \Model\Issue\Type())->find()); + $this->configured = true; } @@ -75,6 +85,11 @@ protected function mock(string $route, ?array $args = null, ?array $headers = nu // Prevent F3 from calling exit by setting HALT to false $f3->set('HALT', false); + // Capture the error status from F3's error handling rather than + // parsing output: a route that emits partial content before failing + // would otherwise be misread as a 200. + $this->lastMockStatus = null; + // Set custom error handler that won't exit $f3->set('ONERROR', function ($f3) { echo json_encode([ @@ -92,11 +107,20 @@ protected function mock(string $route, ?array $args = null, ?array $headers = nu $f3->mock($route, $args, $headers); } catch (\Throwable $e) { // Catch any uncaught exceptions + $this->lastMockStatus = 500; echo json_encode([ "status" => 500, "error" => $e->getMessage() ], JSON_THROW_ON_ERROR); } finally { + // Capture F3's recorded error status before clearing it. This + // works regardless of which ONERROR handler ran (controllers + // may install their own) and even when the route emitted partial + // output before failing. + if ($this->lastMockStatus === null) { + $code = $f3->get('ERROR.code'); + $this->lastMockStatus = $code === null ? null : (int) $code; + } // Restore original handlers $f3->set('ONERROR', $originalErrorHandler); $f3->set('ONREROUTE', $originalOnReroute); @@ -249,14 +273,33 @@ public function testPublicRouteDoesNotFatal(string $route, ?int $expectedStatus) $output = $this->mock($route); $this->assertIsString($output, "Route {$route} failed to mock"); - // The test error handler renders JSON with a status code on failure; - // anything else is a normal (HTML or empty reroute) response. - $decoded = json_decode($output, true); - $status = is_array($decoded) && isset($decoded['status']) ? (int) $decoded['status'] : 200; + // Use the status captured from F3's error handling; a route that + // emits partial content before failing must still report its 500. + $status = $this->lastMockStatus ?? 200; $this->assertNotSame(500, $status, "Route {$route} caused a fatal error"); if ($expectedStatus !== null) { $this->assertSame($expectedStatus, $status, "Route {$route} returned an unexpected status"); } } + + /** + * A route that emits partial output before failing must still report + * its 500 status instead of being misread as a 200. + */ + public function testMockCapturesStatusAfterPartialOutput(): void + { + if (!$this->configured) { + $this->markTestSkipped(); + } + + $f3 = \Base::instance(); + $f3->route('GET /_test_partial_output', function (): void { + echo 'partial output'; + throw new \Exception('boom'); + }); + + $this->mock('GET /_test_partial_output'); + $this->assertSame(500, $this->lastMockStatus); + } } From 9c24e3f8a3875600004bc84524ec747880b4c979 Mon Sep 17 00:00:00 2001 From: Sola Date: Sun, 27 Sep 2026 09:56:43 -0600 Subject: [PATCH 05/12] Use Pdo\Mysql::ATTR_INIT_COMMAND instead of deprecated PDO constant PDO::MYSQL_ATTR_INIT_COMMAND is deprecated since PHP 8.5. The Pdo\Mysql::ATTR_INIT_COMMAND equivalent has existed since PHP 8.0, so this is safe for the project's PHP >= 8.1 requirement, and all three usages are in MySQL-only branches where pdo_mysql is loaded. --- index.php | 2 +- tests/apiTest.php | 2 +- tests/webFeatureTest.php | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/index.php b/index.php index ab3000cd..c3196419 100644 --- a/index.php +++ b/index.php @@ -83,7 +83,7 @@ "mysql:host=" . $f3->get("db.host") . ";port=" . $f3->get("db.port") . ";dbname=" . $f3->get("db.name"), $f3->get("db.user"), $f3->get("db.pass"), - [\PDO::MYSQL_ATTR_INIT_COMMAND => 'SET NAMES utf8mb4;'] + [\Pdo\Mysql::ATTR_INIT_COMMAND => 'SET NAMES utf8mb4;'] )); } diff --git a/tests/apiTest.php b/tests/apiTest.php index 371f4126..01566cd9 100644 --- a/tests/apiTest.php +++ b/tests/apiTest.php @@ -60,7 +60,7 @@ protected function setUp(): void "mysql:host=" . $f3->get("db.host") . ";port=" . $f3->get("db.port") . ";dbname=" . $f3->get("db.name"), $f3->get("db.user"), $f3->get("db.pass"), - [\PDO::MYSQL_ATTR_INIT_COMMAND => 'SET NAMES utf8mb4;'] + [\Pdo\Mysql::ATTR_INIT_COMMAND => 'SET NAMES utf8mb4;'] )); } diff --git a/tests/webFeatureTest.php b/tests/webFeatureTest.php index b9525d29..9e86f9f4 100644 --- a/tests/webFeatureTest.php +++ b/tests/webFeatureTest.php @@ -47,7 +47,7 @@ protected function setUp(): void 'mysql:host=' . $f3->get('db.host') . ';port=' . $f3->get('db.port') . ';dbname=' . $f3->get('db.name'), $f3->get('db.user'), $f3->get('db.pass'), - [\PDO::MYSQL_ATTR_INIT_COMMAND => 'SET NAMES utf8mb4;'] + [\Pdo\Mysql::ATTR_INIT_COMMAND => 'SET NAMES utf8mb4;'] )); } From 6415b0856f62c0a555141c9613e2ce9a831113a3 Mon Sep 17 00:00:00 2001 From: Sola Date: Sun, 27 Sep 2026 10:08:09 -0600 Subject: [PATCH 06/12] Install pdo_mysql extension in CI PHPStan analyzes code referencing Pdo\Mysql::ATTR_INIT_COMMAND, which requires the pdo_mysql extension to be loaded for class discovery. Install it explicitly in all CI jobs so static analysis sees the real class. This also matches the production Dockerfile, which installs pdo_mysql. --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 313184e2..dcf58f3a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -47,7 +47,7 @@ jobs: - uses: shivammathur/setup-php@v2 with: php-version: ${{ matrix.php }} - extensions: ${{ !matrix.intl && ':intl' || '' }} + extensions: pdo_mysql${{ !matrix.intl && ', :intl' || '' }} - uses: actions/checkout@v7 From 13ec6793cc27bd02d4b1cf1da815699338a73c9c Mon Sep 17 00:00:00 2001 From: Sola Date: Sun, 27 Sep 2026 10:14:06 -0600 Subject: [PATCH 07/12] Guard Pdo\Mysql::ATTR_INIT_COMMAND behind PHP_VERSION_ID check Pdo\Mysql only exists on PHP 8.4+ with pdo_mysql loaded, so the unconditional reference would fatal on PHP 8.1-8.3. Resolve the attribute via a version guard instead: use Pdo\Mysql::ATTR_INIT_COMMAND on 8.4+ (required on 8.5+, where PDO::MYSQL_ATTR_INIT_COMMAND is deprecated) and fall back to PDO::MYSQL_ATTR_INIT_COMMAND on older versions (both constants equal 1002). Also add a PHPStan scan shim declaring Pdo\Mysql so static analysis resolves the class when running on PHP < 8.4 or without pdo_mysql installed. --- index.php | 3 ++- phpstan.neon | 2 ++ phpstan/scan/pdo_mysql.php | 17 +++++++++++++++++ tests/apiTest.php | 3 ++- tests/webFeatureTest.php | 3 ++- 5 files changed, 25 insertions(+), 3 deletions(-) create mode 100644 phpstan/scan/pdo_mysql.php diff --git a/index.php b/index.php index c3196419..c1bde976 100644 --- a/index.php +++ b/index.php @@ -83,7 +83,8 @@ "mysql:host=" . $f3->get("db.host") . ";port=" . $f3->get("db.port") . ";dbname=" . $f3->get("db.name"), $f3->get("db.user"), $f3->get("db.pass"), - [\Pdo\Mysql::ATTR_INIT_COMMAND => 'SET NAMES utf8mb4;'] + // Pdo\Mysql::ATTR_INIT_COMMAND exists on PHP 8.4+; PDO::MYSQL_ATTR_INIT_COMMAND is deprecated since 8.5 + [(\PHP_VERSION_ID >= 80400 ? \Pdo\Mysql::ATTR_INIT_COMMAND : \PDO::MYSQL_ATTR_INIT_COMMAND) => 'SET NAMES utf8mb4;'] )); } diff --git a/phpstan.neon b/phpstan.neon index 67386ceb..37ac9cf6 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -6,6 +6,8 @@ parameters: - tests excludePaths: - app/plugin/* + scanFiles: + - phpstan/scan/pdo_mysql.php ignoreErrors: - '#Unsafe usage of new static\(\).#' - '#Access to an undefined property Model\\Custom::.*#' diff --git a/phpstan/scan/pdo_mysql.php b/phpstan/scan/pdo_mysql.php new file mode 100644 index 00000000..a028a5a3 --- /dev/null +++ b/phpstan/scan/pdo_mysql.php @@ -0,0 +1,17 @@ +get("db.host") . ";port=" . $f3->get("db.port") . ";dbname=" . $f3->get("db.name"), $f3->get("db.user"), $f3->get("db.pass"), - [\Pdo\Mysql::ATTR_INIT_COMMAND => 'SET NAMES utf8mb4;'] + // Pdo\Mysql::ATTR_INIT_COMMAND exists on PHP 8.4+; PDO::MYSQL_ATTR_INIT_COMMAND is deprecated since 8.5 + [(\PHP_VERSION_ID >= 80400 ? \Pdo\Mysql::ATTR_INIT_COMMAND : \PDO::MYSQL_ATTR_INIT_COMMAND) => 'SET NAMES utf8mb4;'] )); } diff --git a/tests/webFeatureTest.php b/tests/webFeatureTest.php index 9e86f9f4..2caa78e7 100644 --- a/tests/webFeatureTest.php +++ b/tests/webFeatureTest.php @@ -47,7 +47,8 @@ protected function setUp(): void 'mysql:host=' . $f3->get('db.host') . ';port=' . $f3->get('db.port') . ';dbname=' . $f3->get('db.name'), $f3->get('db.user'), $f3->get('db.pass'), - [\Pdo\Mysql::ATTR_INIT_COMMAND => 'SET NAMES utf8mb4;'] + // Pdo\Mysql::ATTR_INIT_COMMAND exists on PHP 8.4+; PDO::MYSQL_ATTR_INIT_COMMAND is deprecated since 8.5 + [(\PHP_VERSION_ID >= 80400 ? \Pdo\Mysql::ATTR_INIT_COMMAND : \PDO::MYSQL_ATTR_INIT_COMMAND) => 'SET NAMES utf8mb4;'] )); } From bcff2d77c5590924208a6d933993cfd9242627e5 Mon Sep 17 00:00:00 2001 From: Sola Date: Thu, 8 Oct 2026 03:23:04 +0000 Subject: [PATCH 08/12] Tolerate replayed DDL when retrying failed MySQL migrations MySQL implicitly commits DDL, so a migration that fails partway cannot roll back the statements that already ran; retrying replayed them and died on duplicate-object errors, requiring manual cleanup first. Statements that fail with already-applied schema errors (duplicate column/key name, table already exists) are now skipped on MySQL so a retry can finish without manual intervention. Data-dependent errors (e.g. duplicate entry) still abort the migration. Tests: new tests/migrationTest.php (classifier unit tests, retry after partial failure via a MySQL-semantics stub, SQLite atomicity check); full suite green, phpcs and phpstan clean. --- app/helper/security.php | 41 ++++++++-- tests/MigrationFakeMySql.php | 68 ++++++++++++++++ tests/migrationTest.php | 152 +++++++++++++++++++++++++++++++++++ 3 files changed, 256 insertions(+), 5 deletions(-) create mode 100644 tests/MigrationFakeMySql.php create mode 100644 tests/migrationTest.php diff --git a/app/helper/security.php b/app/helper/security.php index eac9b8b1..772fa857 100644 --- a/app/helper/security.php +++ b/app/helper/security.php @@ -120,14 +120,21 @@ public function updateDatabase(string $version): bool // Note: MySQL implicitly commits around DDL statements, so a // failed migration there can't be rolled back. The version row // is only updated by the last statement, so a failure leaves - // the database at the previous version and surfaces an error, - // but already-applied DDL (e.g. an added column) stays in place - // and may need manual cleanup before the migration can be - // retried. SQLite migrations are fully atomic. + // the database at the previous version and surfaces an error. + // Already-applied DDL (e.g. an added column) stays in place, + // but replaying it on retry is tolerated (see + // isAlreadyAppliedSchemaError()), so no manual cleanup is + // needed before retrying. SQLite migrations are fully atomic. $db->begin(); foreach (explode(";", (string) $update_db) as $stmt) { if (trim($stmt) !== "") { - $db->exec($stmt); + try { + $db->exec($stmt); + } catch (\Throwable $e) { + if (!$this->isAlreadyAppliedSchemaError($db->driver(), $e)) { + throw $e; + } + } } } $db->commit(); @@ -142,6 +149,30 @@ public function updateDatabase(string $version): bool return true; } + /** + * Check whether a migration statement failure only means the statement + * was already applied by an earlier partial run. + * + * MySQL implicitly commits DDL, so when a migration fails partway + * through, the statements that already ran stay applied; retrying the + * migration replays them, and each replayed statement fails. A replayed + * statement fails with a "duplicate"/"already exists" schema error, + * which is safe to skip because the desired end state is already in + * place. Anything else — including data-dependent "Duplicate entry" + * errors — is a real failure and must still abort the migration. + * @param string $driver PDO driver name ("mysql", "sqlite", ...) + */ + protected function isAlreadyAppliedSchemaError(string $driver, \Throwable $e): bool + { + if ($driver !== "mysql") { + return false; + } + return (bool) preg_match( + "/\\bduplicate\\s+(column|key|index|foreign key constraint)\\s+name\\b|\\balready exists\\b/i", + $e->getMessage() + ); + } + /** * Initialize a CSRF token */ diff --git a/tests/MigrationFakeMySql.php b/tests/MigrationFakeMySql.php new file mode 100644 index 00000000..8db6ac09 --- /dev/null +++ b/tests/MigrationFakeMySql.php @@ -0,0 +1,68 @@ + needle => one-time failure message */ + public array $failOnce = []; + + public function driver(): string + { + return "mysql"; + } + + public function begin(): bool + { + return true; + } + + public function commit(): bool + { + return true; + } + + public function rollback(): bool + { + return true; + } + + public function exec(string $stmt) + { + $stmt = trim($stmt); + foreach ($this->failOnce as $needle => $message) { + if (str_contains($stmt, $needle)) { + unset($this->failOnce[$needle]); + // Statements applied before this one stay applied: + // MySQL implicitly committed them. + throw new \Exception("PDOStatement: " . $message); + } + } + if (in_array($stmt, $this->applied, true)) { + // Replay of already-applied DDL, exactly as F3 surfaces it. + throw new \Exception("PDOStatement: " . $this->duplicateMessage($stmt)); + } + $this->applied[] = $stmt; + return 0; + } + + private function duplicateMessage(string $stmt): string + { + if (str_contains($stmt, "ADD COLUMN")) { + return "Duplicate column name 'c1'"; + } + if (str_contains($stmt, "CREATE TABLE")) { + return "Table 't' already exists"; + } + return "Duplicate key name 'k'"; + } +} + diff --git a/tests/migrationTest.php b/tests/migrationTest.php new file mode 100644 index 00000000..26027f9a --- /dev/null +++ b/tests/migrationTest.php @@ -0,0 +1,152 @@ +migrationFile = dirname(__DIR__) . "/db/99.99.99.sql"; + } + + protected function tearDown(): void + { + if (file_exists($this->migrationFile)) { + unlink($this->migrationFile); + } + $f3 = \Base::instance(); + $f3->clear("db.instance"); + $f3->clear("error"); + $f3->clear("success"); + } + + private function writeMigration(string $sql): void + { + file_put_contents($this->migrationFile, $sql); + } + + private function isAlreadyApplied(string $driver, string $message): bool + { + $method = new \ReflectionMethod(\Helper\Security::class, "isAlreadyAppliedSchemaError"); + $method->setAccessible(true); + return $method->invoke(\Helper\Security::instance(), $driver, new \Exception($message)); + } + + public function testClassifierToleratesOnlyReplayedSchemaObjects(): void + { + // Replayed DDL on MySQL: safe to skip. + $this->assertTrue($this->isAlreadyApplied("mysql", "PDOStatement: Duplicate column name 'c1'")); + $this->assertTrue($this->isAlreadyApplied("mysql", "PDOStatement: Duplicate key name 'idx'")); + $this->assertTrue($this->isAlreadyApplied("mysql", "PDOStatement: Table 't' already exists")); + $this->assertTrue( + $this->isAlreadyApplied("mysql", "PDOStatement: Duplicate foreign key constraint name 'fk'") + ); + // Data-dependent and other errors: must still abort. + $this->assertFalse( + $this->isAlreadyApplied("mysql", "PDOStatement: Duplicate entry '1' for key 'PRIMARY'") + ); + $this->assertFalse( + $this->isAlreadyApplied("mysql", "PDOStatement: You have an error in your SQL syntax") + ); + $this->assertFalse( + $this->isAlreadyApplied("mysql", "PDOStatement: Unknown column 'nope' in 'field list'") + ); + // Other drivers: never tolerated (SQLite migrations are atomic). + $this->assertFalse($this->isAlreadyApplied("sqlite", "PDOStatement: Duplicate column name 'c1'")); + } + + public function testPartialMySqlMigrationCanBeRetriedWithoutCleanup(): void + { + $f3 = \Base::instance(); + $db = new FakeMySqlForMigrationTest(); + // First attempt fails on the second statement. + $db->failOnce = ["THIS IS NOT VALID SQL" => "You have an error in your SQL syntax near 'THIS'"]; + $f3->set("db.instance", $db); + + $this->writeMigration( + "ALTER TABLE `t` ADD COLUMN `c1` INT NULL;\n" . + "THIS IS NOT VALID SQL;\n" . + "UPDATE `config` SET `value` = '99.99.99' WHERE `attribute` = 'version';\n" + ); + + $security = \Helper\Security::instance(); + $this->assertFalse($security->updateDatabase("99.99.99")); + $this->assertStringContainsString("failed", (string) $f3->get("error")); + // The first DDL statement stayed applied (MySQL implicit commit). + $this->assertCount(1, $db->applied); + + // The fixed migration is retried as-is on the next request: the + // already-applied statement replays and must be tolerated. + $this->writeMigration( + "ALTER TABLE `t` ADD COLUMN `c1` INT NULL;\n" . + "ALTER TABLE `t` ADD COLUMN `c2` INT NULL;\n" . + "UPDATE `config` SET `value` = '99.99.99' WHERE `attribute` = 'version';\n" + ); + $f3->clear("error"); + + $this->assertTrue($security->updateDatabase("99.99.99")); + $this->assertSame(" Database updated to version: 99.99.99", $f3->get("success")); + // c1 was applied exactly once; c2 and the version update ran on retry. + $this->assertCount(3, $db->applied); + } + + public function testRealMySqlErrorsStillAbortTheMigration(): void + { + $f3 = \Base::instance(); + $db = new FakeMySqlForMigrationTest(); + // A data-dependent failure must never be mistaken for replayed DDL. + $db->failOnce = ["INSERT" => "Duplicate entry '1' for key 'PRIMARY'"]; + $f3->set("db.instance", $db); + + $this->writeMigration( + "ALTER TABLE `t` ADD COLUMN `c1` INT NULL;\n" . + "INSERT INTO `t` (`id`) VALUES (1);\n" . + "UPDATE `config` SET `value` = '99.99.99' WHERE `attribute` = 'version';\n" + ); + + $this->assertFalse(\Helper\Security::instance()->updateDatabase("99.99.99")); + $this->assertStringContainsString("Duplicate entry", (string) $f3->get("error")); + } + + public function testSqliteMigrationStaysAtomic(): void + { + $f3 = \Base::instance(); + $dbFile = $f3->get("TEMP") . "migration-test.sqlite"; + @unlink($dbFile); + $db = new \Helper\SQL("sqlite:" . $dbFile); + $db->exec("CREATE TABLE `config` (`attribute` TEXT PRIMARY KEY, `value` TEXT)"); + $db->exec("INSERT INTO `config` VALUES ('version', '00.00.00')"); + $f3->set("db.instance", $db); + + try { + // A failing migration rolls back entirely on SQLite. + $this->writeMigration( + "CREATE TABLE `migtest` (`id` INTEGER PRIMARY KEY);\n" . + "THIS IS NOT VALID SQL;\n" . + "UPDATE `config` SET `value` = '99.99.99' WHERE `attribute` = 'version';\n" + ); + $this->assertFalse(\Helper\Security::instance()->updateDatabase("99.99.99")); + $tables = $db->exec("SELECT name FROM sqlite_master WHERE type='table' AND name='migtest'"); + $this->assertSame([], $tables); + $version = $db->exec("SELECT value FROM `config` WHERE `attribute` = 'version'"); + $this->assertSame("00.00.00", $version[0]["value"]); + + // A clean migration applies and bumps the version. + $this->writeMigration( + "CREATE TABLE `migtest` (`id` INTEGER PRIMARY KEY);\n" . + "UPDATE `config` SET `value` = '99.99.99' WHERE `attribute` = 'version';\n" + ); + $this->assertTrue(\Helper\Security::instance()->updateDatabase("99.99.99")); + $version = $db->exec("SELECT value FROM `config` WHERE `attribute` = 'version'"); + $this->assertSame("99.99.99", $version[0]["value"]); + } finally { + @unlink($dbFile); + } + } +} From 8d5b2143b66a77283f652444d38ab2e13e162abd Mon Sep 17 00:00:00 2001 From: Alan Date: Fri, 9 Oct 2026 10:58:37 -0600 Subject: [PATCH 09/12] Update app/helper/security.php Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- app/helper/security.php | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/app/helper/security.php b/app/helper/security.php index 772fa857..c62b59bf 100644 --- a/app/helper/security.php +++ b/app/helper/security.php @@ -131,15 +131,23 @@ public function updateDatabase(string $version): bool try { $db->exec($stmt); } catch (\Throwable $e) { - if (!$this->isAlreadyAppliedSchemaError($db->driver(), $e)) { - throw $e; - } - } + $db->begin(); + foreach (explode(";", (string) $update_db) as $stmt) { + if (trim($stmt) !== "") { + $db->exec($stmt); } } - $db->commit(); + if ($db->pdo()->inTransaction()) { + $db->commit(); + } } catch (\Throwable $e) { - $db->rollback(); + try { + if ($db->pdo()->inTransaction()) { + $db->rollback(); + } + } catch (\Throwable) { + // Keep the original migration error. + } $f3->set("error", " Database update to version {$version} failed: " . $e->getMessage()); return false; } From 8f2080e4045fad38e5b0e5e239e92d8e08eda587 Mon Sep 17 00:00:00 2001 From: Alan Date: Fri, 9 Oct 2026 11:00:43 -0600 Subject: [PATCH 10/12] Update cron/base.php When db.name is relative, cron/base.php passes it unchanged to the SQLite DSN. Cron entrypoints do not change the working directory. A cron run outside the project root can therefore open the wrong database or fail to find the installed database. Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- cron/base.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cron/base.php b/cron/base.php index e0d67de1..228269d7 100644 --- a/cron/base.php +++ b/cron/base.php @@ -32,7 +32,11 @@ // Connect to database if ($f3->get("db.engine") == "sqlite") { - $f3->set("db.instance", new \Helper\SQL("sqlite:" . $f3->get("db.name"))); + $db_name = $f3->get("db.name"); + if ($db_name !== ":memory:" && substr($db_name, 0, 1) !== "/") { + $db_name = $homedir . $db_name; + } + $f3->set("db.instance", new \Helper\SQL("sqlite:" . $db_name)); } else { $f3->set("db.instance", new \Helper\SQL( "mysql:host=" . $f3->get("db.host") . ";port=" . ($f3->get("db.port") ?: 3306) . ";dbname=" . $f3->get("db.name"), From c3b42fb1980f685f5a22432dbdb79a1418117611 Mon Sep 17 00:00:00 2001 From: Sola Date: Fri, 9 Oct 2026 11:05:53 -0600 Subject: [PATCH 11/12] Fix due_date validation, anchor admin regexes, repair migration guards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - app/model/issue.php: Issue::create() now skips due_date normalization only when the entire value is a valid Y-m-d date; otherwise it parses with strtotime and throws on unparseable input (the old unanchored pattern check could pass through invalid values untouched). - app/controller/admin.php: user_save() username and task_color patterns now use \z instead of $, so validation reaches the true end of the value (previously a trailing newline would pass). - app/helper/security.php: repair the mangled inTransaction() edit — restore the per-statement DDL-replay tolerance alongside the commit/rollback guards so the file parses and both behaviors work. - tests/MigrationFakeMySql.php: add pdo()->inTransaction() to the fake to match the DB\SQL interface now used by updateDatabase(). --- app/controller/admin.php | 4 ++-- app/helper/security.php | 8 ++++---- app/model/issue.php | 12 ++++++++---- tests/MigrationFakeMySql.php | 10 ++++++++++ 4 files changed, 24 insertions(+), 10 deletions(-) diff --git a/app/controller/admin.php b/app/controller/admin.php index d8a9ab9e..9e7a3b69 100644 --- a/app/controller/admin.php +++ b/app/controller/admin.php @@ -349,11 +349,11 @@ public function user_save(\Base $f3): void throw new \Exception("Please enter a name."); } - if (preg_match("/^#?(?:[0-9a-f]{3}|[0-9a-f]{6})$/i", (string) $f3->get("POST.task_color")) !== 1) { + if (preg_match("/^#?(?:[0-9a-f]{3}|[0-9a-f]{6})\\z/i", (string) $f3->get("POST.task_color")) !== 1) { throw new \Exception("Please enter a valid hex color."); } - if (preg_match("/^[0-9a-z_-]+$/i", (string) $f3->get("POST.username")) !== 1) { + if (preg_match("/^[0-9a-z_-]+\\z/i", (string) $f3->get("POST.username")) !== 1) { throw new \Exception("Usernames can only contain letters, numbers, hyphens, and underscores."); } diff --git a/app/helper/security.php b/app/helper/security.php index c62b59bf..fc67890d 100644 --- a/app/helper/security.php +++ b/app/helper/security.php @@ -131,10 +131,10 @@ public function updateDatabase(string $version): bool try { $db->exec($stmt); } catch (\Throwable $e) { - $db->begin(); - foreach (explode(";", (string) $update_db) as $stmt) { - if (trim($stmt) !== "") { - $db->exec($stmt); + if (!$this->isAlreadyAppliedSchemaError($db->driver(), $e)) { + throw $e; + } + } } } if ($db->pdo()->inTransaction()) { diff --git a/app/model/issue.php b/app/model/issue.php index fc45b742..d6761c2e 100644 --- a/app/model/issue.php +++ b/app/model/issue.php @@ -50,10 +50,14 @@ public static function create(array $data, bool $notify = true): static } if (!empty($data["due_date"])) { - if ( - preg_match("/\\d{4}(-\\d{2}){2}/", (string) $data["due_date"]) !== 1 - && ($parsed = strtotime((string) $data["due_date"])) !== false - ) { + $dueDate = (string) $data["due_date"]; + $date = \DateTime::createFromFormat("Y-m-d", $dueDate); + if ($date === false || $date->format("Y-m-d") !== $dueDate) { + // Not already a valid Y-m-d date, try to parse it + $parsed = strtotime($dueDate); + if ($parsed === false) { + throw new \Exception("Please enter a valid due date."); + } $data["due_date"] = date("Y-m-d", $parsed); } diff --git a/tests/MigrationFakeMySql.php b/tests/MigrationFakeMySql.php index 8db6ac09..5ae1c640 100644 --- a/tests/MigrationFakeMySql.php +++ b/tests/MigrationFakeMySql.php @@ -35,6 +35,16 @@ public function rollback(): bool return true; } + public function pdo(): object + { + return new class { + public function inTransaction(): bool + { + return true; + } + }; + } + public function exec(string $stmt) { $stmt = trim($stmt); From c320b4bbf13af38217dd141b9bc8bdd901b405b9 Mon Sep 17 00:00:00 2001 From: Alan Date: Mon, 12 Oct 2026 17:52:44 -0600 Subject: [PATCH 12/12] Remove maintenance notes from README Removed maintenance notes section from README.md. --- README.md | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/README.md b/README.md index 45a3cd5c..c919e39f 100644 --- a/README.md +++ b/README.md @@ -15,16 +15,6 @@ Detailed requirements and installation instructions are available at [phproject. Phproject uses [Composer](https://getcomposer.org/) for dependency management. After cloning the repository, run `composer install` to install the required packages. -## Maintenance notes - -A few things worth knowing when keeping a Phproject install running long-term: - -- **Framework contingency.** Phproject is built on `bcosca/fatfree-core`, which has a small maintainer base. A mirror is kept at [Alanaktion/fatfree-core](https://github.com/Alanaktion/fatfree-core); if upstream ever goes unmaintained, point Composer at the fork by adding a VCS repository entry and requiring the same version constraint — no code changes needed. -- **Vendored JavaScript is frozen.** The JS libraries in `js/` (Bootstrap 3.4.1, jQuery 3.6.3, bootstrap-datepicker, EasyMDE) are pinned and no longer receive upstream security fixes. This is accepted risk: they are only served as static assets, and user content rendered through them is escaped server-side. Re-vendor only if a concrete issue arises. -- **API keys.** Pass API keys via the `X-API-Key` (or `X-Redmine-API-Key`) request header. The `?key=` query-string parameter still works but is deprecated — it leaks keys into server access logs. -- **Logs.** The application appends to `log/` indefinitely. In production, mount `log/` as a volume and rotate it (e.g. with `logrotate`) so a long-running instance can't fill its disk. -- **Cron jobs** (`cron/`) read database settings from the root `config.php` written by the installer, and support both MySQL and SQLite. Schedule them with the system cron, e.g. `*/5 * * * * php /var/www/html/cron/due_alerts.php`. - ## Contributing Phproject is maintained as an open source project for use by anyone around the world under the [GNU General Public License](http://www.gnu.org/licenses/gpl-3.0.txt). If you find a bug or would like a new feature added, [open an issue](https://github.com/Alanaktion/phproject/issues/new) or [submit a pull request](https://github.com/Alanaktion/phproject/compare/) with new code. If you want to help with translation, [you can submit translations via Crowdin](https://crowdin.com/project/phproject).