Skip to content

Stop forcing display_errors in production and leaking DB errors - #90

Open
santichausis wants to merge 1 commit into
gea-ecobricks:new-mainfrom
santichausis:fix/error-exposure
Open

Stop forcing display_errors in production and leaking DB errors#90
santichausis wants to merge 1 commit into
gea-ecobricks:new-mainfrom
santichausis:fix/error-exposure

Conversation

@santichausis

Copy link
Copy Markdown

Summary

Two related information-disclosure issues, fixed together since they're both about errors leaking to end users:

1. Hardcoded display_errors in ~29 files. Files like 404-handler.php, en/project.php, es/add-project.php, scripts/get-training.php, api/fetch_cash_trans.php, etc. had:

ini_set('display_errors', 1);
error_reporting(E_ALL);

This forces PHP to print stack traces and internal file paths to any visitor, overriding whatever the production server's own php.ini says. Removed these overrides everywhere so the server's own production settings (which should have display_errors off) actually take effect. Where the line closed the PHP tag inline (ini_set(...);?>), preserved the ?> and only removed the ini_set/error_reporting calls, verified with php -l on every changed file.

2. Raw DB errors echoed straight to responses. E.g. api/fetch_cash_trans.php did:

echo json_encode(['error' => 'Failed to prepare database query: ' . $gobrik_conn->error]);

Anyone hitting the endpoint with a malformed request got the actual MySQL error string back. Replaced every occurrence (public /api/ endpoints, the public add-project/add-project-images forms in es/fr/id, and the /scripts/ admin migration tools) with a generic message + error_log(...) so the real error is still available to whoever has server log access, just not to the requester.

Testing

No test suite/CI in this repo, and no DB credentials available locally (ecobricks_env.php/gobrikconn_env.php intentionally not committed). Verified with:

  • php -l on all 35 changed files — no new syntax errors (pre-existing unrelated (double) cast deprecation notices in a few add-project.php files are untouched, confirmed present before this change too).
  • Manual review of every diff to confirm control flow is unchanged (e.g. $errors[]/$error_message are still populated so if (!empty($error_message)) branches still fire correctly, just with generic text instead of the raw driver error).

Recommend a quick smoke test on staging/beta.ecobricks.org: submit the add-project form and the /api/fetch_cash_trans.php endpoint with an invalid ID and confirm you get a clean generic error instead of a PHP warning or raw SQL error text.

Removes hardcoded ini_set('display_errors', 1) / error_reporting(E_ALL)
overrides from ~29 files. These forced PHP to show stack traces and
internal paths to any visitor regardless of the server's own
production php.ini settings.

Also replaces every place that echoed a raw $conn->error / $stmt->error
/ connect_error string back to the browser (public API endpoints, the
public project-submission forms, and the /scripts/ admin tools) with a
generic message plus a server-side error_log() call, so operators can
still diagnose failures without exposing DB schema/host details to
whoever is looking at the response.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant