Make WordPress Core

Opened 4 weeks ago

Last modified 4 weeks ago

#66080 new defect (bug)

Send 500 (or other non-200) status code when using db-error.php

Reported by: bradleyt Owned by:
Priority: normal Milestone: 7.2
Component: Database Version: 2.5
Severity: normal Keywords: has-patch has-unit-tests
Cc: Focuses:

Description

Site owners can use db-error.php to show a custom error page when the database is unavailable. This replaces the usual message shown with wp_die().

wp_die() contains the following code:

header( "Content-Type: text/html; charset={$parsed_args['charset']}" );
status_header( $parsed_args['response'] );
nocache_headers();

However, this code isn't present when using db-error.php.

I propose that when using a drop-in error page, WordPress should output status and no-cache headers by default.

A client site that I work on recently had 1 minute of database downtime, but the error page continued to be shown for several hours. This is because it had been cached by Varnish. It is unintuitive, and not clearly documented, that a developer would need to send the status headers themselves when using db-error.php.

The same likely applies to the maintenance.php dropin.

Attachments (1)

66080.patch​ (5.1 KB ) - added by dineshinau 4 weeks ago.
Fixed the code formatting using phpcbf

Download all attachments as: .zip

Change History (13)

#1 follow-up: @westonruter
4 weeks ago

  • Keywords needs-patch added
  • Milestone Awaiting Review → 7.2
  • Type enhancement → defect (bug)
  • Version → 2.5

This was introduced in r6447 to fix #5500.

Apparently it presumes the db-error.php file itself would send the non-200 status.

The maintenance functionality was added much later in 3.0.0 via r47623 to fix #49959. It similarly lacks setting the appropriate header.

It seems a 503 Service Unavailable status is the appropriate one to send, based on wp_maintenance(). Maybe dead_db() should get header( 'Retry-After: 600' ) as well.

This ticket was mentioned in ​PR #13471 on ​WordPress/wordpress-develop by ​@dineshinau.


4 weeks ago
#2

  • Keywords has-patch added; needs-patch removed

Currently, when a site uses a db-error.php or maintenance.php drop-in to show a custom outage page, WordPress skips the status_header() and nocache_headers() calls it normally sends via wp_die(). This means a 500/503 page can get cached as a 200 by a CDN or reverse proxy (e.g. Varnish), leaving a stale error page served long after the outage is resolved.

This adds status_header( 500 ) + nocache_headers() before loading db-error.php, and status_header( 503 ) + nocache_headers() + the existing Retry-After header before loading maintenance.php.

Note there are two separate places that load db-error.php: the dead_db() function in wp-includes/functions.php (used when a query fails mid-request), and a duplicated inline check in WPDB::db_connect() (used when the initial connection attempt fails). Both needed the same fix, since they don't share code.

Tested by forcing a connection failure via invalid wp-config.php credentials (exercises class-wpdb.php) and by creating a .maintenance file (exercises wp_maintenance()), confirming via curl -I that the correct status code and Cache-Control: no-cache headers are now sent in both cases.

Opened from the WordPress Contributor Toolkit.

@dineshinau
4 weeks ago

Fixed the code formatting using phpcbf

This ticket was mentioned in ​PR #13474 on ​WordPress/wordpress-develop by ​@dineshinau.


4 weeks ago
#3

  • Keywords has-unit-tests added

Due to an PHPCS error in PR: #13471, creating a new PR after fixing the errors using phpcbf

Opened from the WordPress Contributor Toolkit.

#4 @westonruter
4 weeks ago

@dineshinau Thanks for working on a patch.

FYI: No need to attach a patch file to the ticket if you have a PR. The PR is always preferred since it includes the automated tests. Core committers can and do use the PR directly when preparing the commit for SVN, so no need to bother with patch files at all.

​@westonruter commented on ​PR #13474:


4 weeks ago
#5

Not sure why this second PR is needed. Closing in favor of keeping all the work on the previous PR: ​https://github.com/WordPress/wordpress-develop/pull/13471

#6 @dineshinau
4 weeks ago

Thanks for the feedback, @westonruter — noted on both counts.

For future patches on a ticket, I'll stick with the PR only and skip attaching a .patch file.

The second PR came from the Contributor Toolkit not picking up my local phpcbf fix before opening a new one, rather than an intentional decision — I'll make sure the existing PR branch is updated first next time instead of creating a fresh PR for the same ticket.

​@dineshinau commented on ​PR #13471:


4 weeks ago
#7

@westonruter Addressed your requested changes — fixed the PHPCS issues and corrected the status header from 500 to 503 in the comment above wp_maintenance(). Ready for another look whenever you have time.

#8 @bradleyt
4 weeks ago

The suggested status codes and subsequent PR look good to me 👍

#9 in reply to: ↑ 1 @sabernhardt
4 weeks ago

Apparently it presumes the db-error.php file itself would send the non-200 status.

[8092] documented the need to add headers with the dead_db() function (though that is not very clear or intuitive when the drop-in file is used in multiple places):

The WordPress DB error sets the HTTP status header to 500 to try to prevent search engines from caching the message. Custom DB messages should do the same.

PR 13471 needs testing with ​db-error.php and ​maintenance.php files that either

  1. include header() for status and cache control,
  2. use the status_header() and nocache_headers() functions, or
  3. do not set any headers.

Those cases seemed to work properly with the PR's edits to WPDB::db_connect() (when WP_DEBUG was false), but I did not test with dead_db() or wp_maintenance().

​@dineshinau commented on ​PR #13471:


4 weeks ago
#10

@sabernhardt I’ve addressed the comment and updated the text and formatting as suggested. Please re-review when you get a chance.

#11 @dineshinau
4 weeks ago

@sabernhardt Tested both dead_db() and wp_maintenance() now, with WP_DEBUG on, against all three header scenarios you listed.

For wp_maintenance(), swapped wp-content/maintenance.php between three versions — one that sets its own status/cache headers, one that sets nothing, and one that sets a custom Retry-After. All three return a clean single 503 with no duplicate headers or output. (Also moved the require_once for functions.php up above the drop-in check in the patch, since status_header()/nocache_headers() need to be available on that branch too, not just the existing fallback path — one require now covers both.)

dead_db() needed a different setup to reach, since bad DB credentials in wp-config.php actually hit WPDB::db_connect()'s own bail-out, not dead_db() itself. dead_db() only fires from WPDB::bail() when a query fails after the connection already succeeded. Used a small mu-plugin to call it directly instead of trying to simulate a real connection failure:

<?php
add_action( 'init', function () {
        if ( isset( $_GET['test_dead_db'] ) ) {
                dead_db();
        }
} );

Hit it with ?test_dead_db=1, ran the same three header variants through db-error.php, and got a clean single 500 with no duplicates and no PHP warnings across all three.

​@dineshinau commented on ​PR #13471:


4 weeks ago
#12

@westonruter Noted, I’ll avoid merging trunk into this PR branch again unless there’s a merge conflict or a related change that requires it.

I also fixed an issue in wp_maintenance() where status_header() wasn’t available when setting the 503 status, so I moved the functions.php include above the maintenance file existence check.

Note: See TracTickets for help on using tickets.