…#116) * Harden installer account creation: transactions + prepared statements The user-creation path in 202-config/install.php predated the codebase's own conventions (CLAUDE.md). This brings it in line: - Wrap the user / users_pref / user_role writes in a single begin_transaction/commit/rollback so a mid-way failure can't leave a half-built account behind. - Replace string-concatenated SQL (escaped with real_escape_string) with prepared statements and bound parameters, mirroring AUTH::LOGIN_SELECT. - Check every prepare()/execute() return value and throw on failure instead of silently discarding it via the @-suppressed _mysqli_query(). - Drop the dead md5 salt_user_pass() fallback and call hash_user_pass() (password_hash/bcrypt) directly; the helper is always loaded. The non-critical auto_cron flag update is kept best-effort (logged, not fatal) since the account is already committed by that point. * docs: require full deploy-quality code review before committing Add a "Before committing" section to CLAUDE.md mandating a production-grade review of staged changes — per-file walkthrough, error-path tracing, lint/test confirmation — before every commit. * Make installer resilient with graceful fallbacks (backend + frontend) Goal: a successful install even when individual steps fail — friction loses users. Adds fallbacks on both sides without weakening the transactional core. Backend (install.php): - Wrap schema setup + the user/pref/role transaction in one guarded try (DDL stays before begin_transaction; $inTransaction guards rollback). - Classify failures: transient DB errors (lock wait, deadlock, server gone/lost) are marked retryable with a "try again" message; everything else gets an actionable message linking the setup-config and manual install fallbacks. - Treat post-commit steps (auto_cron, daily-email registration, partitioning) as best-effort: each is isolated, logged, and downgraded to a friendly note so the account still completes successfully. - Add an AJAX/JSON response path: returns the rendered success panel to swap in, or field/general errors + a retryable flag. json_encode failures are surfaced explicitly, never an empty body. Frontend (install.php): - Progressive enhancement: the plain POST form still works without JS. - With JS: instant client-side validation mirroring the server rules, AJAX submit (no full reload), inline field errors, preserved input (incl. timezone), and automatic retry with backoff on transient/network failures before showing a connection error with manual fallbacks. Reliability (functions-tracking202.php): - Add connect/exec timeouts to callAutoCron() and registerDailyEmail() so a slow or unreachable my.tracking202.com can no longer hang the page. * Clarify the already-installed copy Reassure users who reach this state (often a retry after a dropped response right after a successful install) instead of reading like an error. Lead with "your account is ready" and a single clear CTA; keep the reinstall note as a secondary line on the no-JS page. * Fix issues found in review of installer resilience changes - Retry classification: under MYSQLI_REPORT_STRICT (report-only, no ERROR bit) execute() returns false instead of throwing, so the RuntimeExceptions carried code 0 and the transient-error check ([1205,1213,2006,2013]) never matched — the auto-retry feature never fired for the common deadlock/lock-wait case. Plumb the real driver errno into each exception so the catch can classify correctly. - Reuse connect.php's $_is_ajax instead of re-deriving the same XHR detection locally (single source of truth). - Escape $_SERVER['SERVER_NAME'] in the success panel; it's the one unescaped reflection and now flows through JSON -> outerHTML on the AJAX path (latent XSS behind a misconfigured Host/proxy). * Address remaining installer review items 1. Schema failures now abort the install (functions-install.php): install_databases() previously logged SchemaInstaller errors and continued, so a partially-created schema could be reported as a successful install. It now throws after logging. All CREATE TABLE use IF NOT EXISTS, so re-running over an existing/partial schema is still error-free — only genuine failures abort. 2. CSRF protection on the install POST: the form now embeds the app-wide $_SESSION['token'] (set in connect.php, persisted across AJAX via withWritableSession) and the handler rejects any POST whose token doesn't match before doing any work. Account creation is gated on the CSRF check; AJAX gets a clear JSON error, no-JS re-renders the form with the message. 3. Client/server validation alignment: numeric field bounds now live in a single $rules array, used by the server checks and injected into the JS (json_encode) so they can't drift. The client email check is loosened to a permissive subset of the server's filter_var so it never blocks an address the server would accept. Raw $_POST reads in validation are null-coalesced to avoid undefined-array-key warnings on malformed requests. 4. Retry UX: a neutral "Connection issue — retrying… (attempt N of 3)" status is shown during the backoff before each auto-retry, so a flaky-connection user isn't staring at a frozen, silent form. * Make the live-upgrade path resilient to transient DB failures Upgrades run on live, loaded servers and use a separate path from the fresh installer (functions-upgrade.php, not SchemaInstaller). Every migration step ran through _mysqli_query(), which does @$db->query() and returns false on error; the results were overwhelmingly unchecked, so a lock-wait/deadlock under load silently skipped a migration step (or fataled on the next ->num_rows) — leaving a partially-migrated live DB with no error surfaced. Add _upgrade_query(): a drop-in resilient runner for the upgrade path. - Transient lock-wait (1205) / deadlock (1213): the statement provably did not apply, so re-run the identical SQL with exponential backoff (1s/2s/4s, up to 3 retries) — safe even for non-idempotent statements. - Transient but never clears: stop with a clear, resumable message instead of silently skipping (the upgrade resumes via its existing IF [NOT] EXISTS / existence guards on re-run). - Every other outcome preserves _mysqli_query()'s exact contract (result, or false), so the upgrade's intentional "ignore an already-applied step" calls behave exactly as before. All 416 _mysqli_query() call sites in functions-upgrade.php are routed through the wrapper. The one explicit-transaction block (1.9.57) uses $connection->query() directly and is untouched, so per-statement retry never runs inside an open transaction. _mysqli_query() itself is unchanged, so the rest of the app is unaffected. This is Option B (retry + loud failure); full resumability across all ~610 statements (per-step version markers) remains a possible follow-up. * Add tests for installer + upgrade resilience (with testability seams) The new resilience logic had no coverage. Refactor the riskiest pieces behind small testable seams and add unit + integration tests. Upgrade path (functions-upgrade.php): - Split _upgrade_query() into a pure core, _upgrade_query_run(), that takes the connection, a backoff sleeper, and a "give up" handler as parameters (no globals, no real sleep, no _die()). The production wrapper supplies sleep() and the _die() abort; behavior is unchanged. Installer (install.php / functions-install-helpers.php): - Extract the field rules, the CSRF token check, and account validation into pure, dependency-free helpers (install_default_rules, install_csrf_ok, install_validate_account) so they can be unit-tested without bootstrapping the install entry point. install.php now calls them; the validation helper mirrors the previous inline checks exactly (including the isset()/empty() password semantics). Tests: - tests/Upgrade/UpgradeQueryTest.php (9): retry on 1205/1213, backoff sequence 1s/2s/4s, permanent error -> false, exhausted -> onExhausted, custom max attempts, and the mysqli_sql_exception (REPORT_ERROR) path. - tests/Install/InstallHelpersTest.php (16): rules, CSRF accept/reject, and every validation branch incl. missing-field handling. - tests/Upgrade/UpgradeQueryIntegrationTest.php (@group integration): drives a real MySQL — forces a genuine 1205 lock-wait (a second connection holds a FOR UPDATE lock; the injected sleeper releases it) to prove the retry path works end-to-end. Self-skips without P202_TEST_DB_* env, so it's excluded from the default/CI run. Full suite: 814 tests, 0 failures (8 pre-existing skips). The new unit tests run green here; the integration test runs wherever MySQL is provisioned. * Make success-panel render + JSON encode testable, and cover them Closes the remaining testable gap in the installer. Extract two more pieces out of install.php's procedural flow into the pure helpers file: - render_install_success(): now takes the base URL and server name as parameters (no globals) so the panel markup is unit-testable. install .php passes get_absolute_url() / SERVER_NAME at both call sites. - install_encode_response(): the json_encode-or-explicit-fallback logic, returning ['ok','body']. install_json() now delegates to it. Tests (InstallHelpersTest +5): encode round-trip, encode fallback on malformed UTF-8 (CLAUDE.md #4), success panel renders account + login link, renders warnings, and escapes SERVER_NAME (the latent-XSS fix). Full suite: 819 tests, 0 failures (8 pre-existing skips). * Address code-review findings on the testability refactor - Email rule (CLAUDE.md #5): install_validate_account() now prefers the shared check_email_address() in production instead of a forked filter_var, so the installer's email rule can't drift from the rest of the app. Falls back to the identical filter_var only when the helper is loaded in isolation (unit tests). - Integration test hang guard: if the server ignores/clamps innodb_lock_wait_timeout=1, the lock-wait UPDATE would block for the server default instead of failing fast (the lock only releases inside the sleeper) and hang the test. Read the value back and skip cleanly instead, releasing the held lock first. - _upgrade_query_run(): clamp $maxAttempts to >= 1 so a bad caller can't make the first transient error skip straight to the abort handler. - Test fake: dispatch on Closure instead of is_callable() so a future string outcome that names a real function can't be invoked by accident. - Clarify render_install_success() docblock: $base/user_name are emitted as-is (trusted/pre-escaped); only $serverName is escaped here. Full suite: 819 tests, 0 failures (8 pre-existing skips). * Address Copilot PR review comments - Add rel="noopener noreferrer" to every target="_blank" link in install.php (server-side DB-error message, the welcome ReadMe link, and the client-side connection-error message) to prevent reverse tabnabbing. - Fix user-facing typo on the install success screen: "thats" -> "that's". - Integration test: restore mysqli reporting in tearDownAfterClass so the setUpBeforeClass mysqli_report(MYSQLI_REPORT_OFF) can't leak into other tests in the same process. Full suite: 819 tests, 0 failures (8 pre-existing skips). * Address second Copilot review round install.php (transactional account creation, per CLAUDE.md #1 — check every fallible return; use the statement-level error for prepared stmts): - Check $db->begin_transaction() and throw on failure (previously set $inTransaction=true regardless, so a failed start would run the writes non-atomically). - Check $db->commit() and throw on failure so the catch/rollback path handles it instead of reporting a non-committed account as success. - Capture $stmt->error (as $stmtError, not the page-level $error array) before close() and use it in the execute()-failure exceptions; $db->error can be empty/misleading for prepared statements. Applied to the user, lookup, prefs, role, and auto_cron statements; the auto_cron message now includes the errno too. (prepare/begin/commit keep $db->error — those are connection-level, with no statement.) UpgradeQueryIntegrationTest: - Restore mysqli_report to the codebase default (MYSQLI_REPORT_STRICT, per connect.php) right after the connection probe, so the REPORT_OFF state can't leak even when the connect-failure markTestSkipped() bypasses tearDown. tearDown restore changed from ERROR|STRICT to STRICT to match. Full suite: 819 tests, 0 failures (8 pre-existing skips). * Make install seeding idempotent so retries can't duplicate defaults Codex review (P2): the AJAX auto-retry re-runs install_databases() on a transient error before any user is committed (is_installed() is still false). Schema creation is idempotent (CREATE TABLE IF NOT EXISTS) and most seeds use INSERT IGNORE, but two used plain INSERT: - seedDefaultChartData() -> 202_charts (no unique key) - seedVersion() -> 202_version (read as a single row) Each retry would duplicate those rows (a doubled default dashboard chart; multiple version rows). Guard both with an existence check so re-running the seed step is a no-op once the rows exist. DataSeeder is only used by the installer, so upgrades are unaffected. Full suite: 819 tests, 0 failures (8 pre-existing skips). * Create the default chart for the committed user, not a hard-coded id Codex review (P2, follow-on): a rolled-back user INSERT still consumes the 202_users AUTO_INCREMENT id (InnoDB doesn't reuse it), so after a transient failure + retry the committed account can become user_id 2 while the seeder hard-codes the default chart to user_id 1. account_overview.php LEFT JOINs 202_charts on the user's own id, so the real account would render with no default chart. Fix: create the default chart inside the account transaction, keyed on the committed $user_id (atomic with the user, and a rolled-back attempt's chart rolls back with it). Remove seedDefaultChartData() from DataSeeder, which ran before the user existed and could only guess the id. seedVersion() keeps its idempotency guard (still seeded pre-transaction). Full suite: 819 tests, 0 failures (8 pre-existing skips). * Harden install: idempotent default chart + checked json_encode Findings from a full-branch review: - Default-chart INSERT was non-idempotent on the INSERT IGNORE "existing user" branch (202_charts has no unique key). Two concurrent same-username installs could leave one account with duplicate default charts, which account_overview.php then joins twice. Gate the chart insert on $user_created (only a freshly-inserted user gets one); the pref/role rows were already INSERT IGNORE, so they were safe. - The inline <script> echoed json_encode(get_absolute_url()) and json_encode($rules) unchecked (CLAUDE.md #1 names json_encode as a fallible call). A false return would emit `var BASE = false;` / broken JS; add a `?: '""'` / `?: '{}'` fallback so the page stays valid. Full suite: 819 tests, 0 failures (8 pre-existing skips). * Set user_dash_email on install; complete @Covers list Copilot review: - 202_users.user_dash_email is NOT NULL with no default. The user INSERT omitted it, relying on SchemaInstaller's `SET session sql_mode=''` (on the shared DB::getInstance() connection) to avoid a strict-mode 1364 error and leaving the column an empty string otherwise. Set it explicitly to the account email and bind it (type string sssssisss, 9 params) so the row is valid regardless of sql_mode. Pre-existing (the original install omitted it too), surfaced by the rewrite. - InstallHelpersTest now exercises install_encode_response, install_request_base_url and render_install_success; add them to the @Covers list so strict-coverage runs don't mark the class risky. Full suite: 832 tests, 0 failures (8 pre-existing skips). --------- Co-authored-by: Claude <noreply@anthropic.com>

Upgrade to PHP 8 #5

Description

Activity

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions