Make WordPress Core

Opened 3 months ago

Closed 3 weeks ago

Last modified 3 weeks ago

#60014 closed enhancement (fixed)

REST endpoint: Output "server errors" if WP_DEBUG = true (register_rest_route)

Reported by: ecc's profile ecc Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.5 Priority: normal
Severity: normal Version: 6.4
Component: REST API Keywords: has-patch commit needs-dev-note
Focuses: rest-api Cc:

Description

While developing a custom REST endpoint (with WP_DEBUG = true), an "internal server error" JSON response is not really helpful for the developer.

It does not show the actual error message, which makes it useless.

(Info: In my case, the error is a wrong method return type)

{
    "code": "internal_server_error",
    "message": "<p>There has been a critical error on this website.<\/p>",
    "data": {
        "status": 500
    },
    "additional_errors": []
}

Idea:

If WP_DEBUG = true, output the full error message in the data array. (see below)

{
    "code": "internal_server_error",
    "message": "<p>There has been a critical error on this website.<\/p>",
    "data": {
        "status": 500,
        "error": "Uncaught TypeError: PluginConfig::getWpOptionStorageName(): Return value must be of type array, string returned in ..."
    },
    "additional_errors": []
}

Observation

In _wp_die_process_input(), functions.php line 4255, the errors are resetted.

If the errors aren't resetted here, the 'additional_errors' will contain the error too.

Maybe it's possible to output them only if WP_DEBUG = true.

// unset( $errors[0] );
$args['additional_errors'] = array_values( $errors );

Change History (16)

#1 @spacedmonkey
3 months ago

Thanks for the idea @ecc. However, when designing the fatal error handle, showing errors like this was omitted by design. Showing error like this would leak information about the plugins installed. This could result in bad actors being about attack sites and know where the attack generated an error.

We would need either new const or maybe use the new DEV mode to show these errors.

Last edited 3 months ago by spacedmonkey (previous) (diff)

#2 @ecc
3 months ago

Hi @spacedmonkey. Thank you for your quick reply. You're right. This is data for developer's eyes only. :)

I also thing, a new constant whould be useful.

define('WP_DEVELOPER_MODE', true);

Only if the constant is set and true, all errors are visible for the developers.

IDEA:

If WP_DEVELOPER_MODE is on, this should be highlighted in the WordPress Admin-Interface very prominent.

Something like: "Attention: WP_DEVELOPER_MODE is active. (Never activate it on your production site)"

What you do you think?

#3 @TimothyBlynJacobs
3 months ago

We could use the existing WP_DEBUG_DISPLAY constant instead of introducing a new one. But I'm not sure we really want to encourage folks to display errors on their site. It's better to check the error log.

#4 @ecc
3 months ago

Hi, @TimothyBlynJacobs. WP_DEBUG_DISPLAY is a good idea. I haven't thought about it.

About the debug.log:

While developing, an instant error output is key, in my humble opinion. If a site crashes, I want instant feedback if possible. If the API crashes in DEV, i need feedback too instantly.

Digging through debug logs to find an obscure error is possible, but not fun at all. :)

I just tried it in the WordPress REST API for my custom endpoints, and because of this experience, I wrote this ticket. :D

#5 @spacedmonkey
3 months ago

@ecc To be clear, developer mode already exists, it was added in WP 6.3. See https://make.wordpress.org/core/2023/07/14/configuring-development-mode-in-6-3/

#6 @ecc
3 months ago

@spacedmonkey I looked at the new 6.3 developer mode. Looks very interesting.

But i think, for the "server errors" output feature, checking WP_DEBUG and WP_DEBUG_DISPLAY maybe enough already.

Edit: Posted and implementation example in the reply.

Last edited 3 months ago by ecc (previous) (diff)

#7 @ecc
3 months ago

I updated the _wp_die_process_input() method (functions.php, line 4214) in order to output the errors, if debug & display of errors is activated. This will output the exception/error in the JSON field "additional_errors". (see below) This works for me.

The changed lines are:

Before

unset( $errors[0] );

After:

// Skip error reset, if debug & display of errors is activated
if( (WP_DEBUG && ! WP_DEBUG_DISPLAY) || ! WP_DEBUG ) {
  unset( $errors[0] );
}

Full updated code of _wp_die_process_input() :

function _wp_die_process_input( $message, $title = '', $args = array() ) {
  $defaults = array(
    'response'          => 0,
    'code'              => '',
    'exit'              => true,
    'back_link'         => false,
    'link_url'          => '',
    'link_text'         => '',
    'text_direction'    => '',
    'charset'           => 'utf-8',
    'additional_errors' => array(),
  );

  $args = wp_parse_args( $args, $defaults );

  if ( function_exists( 'is_wp_error' ) && is_wp_error( $message ) ) {
    if ( ! empty( $message->errors ) ) {
      $errors = array();
      foreach ( (array) $message->errors as $error_code => $error_messages ) {
        foreach ( (array) $error_messages as $error_message ) {
          $errors[] = array(
            'code'    => $error_code,
            'message' => $error_message,
            'data'    => $message->get_error_data( $error_code ),
          );
        }
      }

      $message = $errors[0]['message'];
      if ( empty( $args['code'] ) ) {
        $args['code'] = $errors[0]['code'];
      }
      if ( empty( $args['response'] ) && is_array( $errors[0]['data'] ) && ! empty( $errors[0]['data']['status'] ) ) {
        $args['response'] = $errors[0]['data']['status'];
      }
      if ( empty( $title ) && is_array( $errors[0]['data'] ) && ! empty( $errors[0]['data']['title'] ) ) {
        $title = $errors[0]['data']['title'];
      }

      // Skip error reset, if debug & display of errors is activated
      if( (WP_DEBUG && ! WP_DEBUG_DISPLAY) || ! WP_DEBUG ) {
        unset( $errors[0] );
      }

      $args['additional_errors'] = array_values( $errors );
    } else {
      $message = '';
    }
  }

  $have_gettext = function_exists( '__' );

  // The $title and these specific $args must always have a non-empty value.
  if ( empty( $args['code'] ) ) {
    $args['code'] = 'wp_die';
  }
  if ( empty( $args['response'] ) ) {
    $args['response'] = 500;
  }
  if ( empty( $title ) ) {
    $title = $have_gettext ? __( 'WordPress &rsaquo; Error' ) : 'WordPress &rsaquo; Error';
  }
  if ( empty( $args['text_direction'] ) || ! in_array( $args['text_direction'], array( 'ltr', 'rtl' ), true ) ) {
    $args['text_direction'] = 'ltr';
    if ( function_exists( 'is_rtl' ) && is_rtl() ) {
      $args['text_direction'] = 'rtl';
    }
  }

  if ( ! empty( $args['charset'] ) ) {
    $args['charset'] = _canonical_charset( $args['charset'] );
  }

  return array( $message, $title, $args );
}

JSON response, if debug and debug_display is on:

{
    "code": "internal_server_error",
    "message": "<p>There has been a critical error on this website.<\/p><p><a href=\"https:\/\/wordpress.org\/documentation\/article\/faq-troubleshooting\/\">Learn more about troubleshooting WordPress.<\/a><\/p>",
    "data": {
        "status": 500
    },
    "additional_errors": [
        {
            "code": "internal_server_error",
            "message": "<p>There has been a critical error on this website.<\/p><p><a href=\"https:\/\/wordpress.org\/documentation\/article\/faq-troubleshooting\/\">Learn more about troubleshooting WordPress.<\/a><\/p>",
            "data": {
                "error": {
                    "type": 1,
                    "message": "Uncaught InvalidArgumentException: Unexpected input for key ...",
                    "line": 120
                }
            }
        }
    ]
}

This ticket was mentioned in PR #5840 on WordPress/wordpress-develop by @spacedmonkey.


2 months ago
#8

  • Keywords has-patch added

#9 @spacedmonkey
2 months ago

  • Milestone changed from Awaiting Review to 6.5
  • Owner set to spacedmonkey
  • Status changed from new to assigned

I put together my own PR.

This is a simply PR that just shows error data if WP_DEBUG_DISPLAY is defined.

The output looks like this.

{
    "code": "internal_server_error",
    "message": "<p>There has been a critical error on this website.</p><p><a href=\"https://wordpress.org/documentation/article/faq-troubleshooting/\">Learn more about troubleshooting WordPress.</a></p>",
    "data": {
        "status": 500,
        "error": {
            "type": 4,
            "message": "syntax error, unexpected '}'",
            "file": "/var/www/src/wp-content/mu-plugins/test.php",
            "line": 5
        }
    },
    "additional_errors": []
}

This seems like a simple and clean solution. @ecc what you think?

#10 @ecc
2 months ago

@spacedmonkey I just added your PR to my WordPress installation and tested it.

Your implementation works fine.

If i send my API request with Content-Type application/json, the error looks like the one below.

{
    "code": "internal_server_error",
    "message": "<p>There has been a critical error on this website.<\/p><p><a href=\"https:\/\/wordpress.org\/documentation\/article\/faq-troubleshooting\/\">Learn more about troubleshooting WordPress.<\/a><\/p>",
    "data": {
        "status": 500,
        "error": {
            "type": 1,
            "message": "Uncaught TypeError: PluginConfig::getWpOptionStorageName(): Return value must be of type array, string returned in ...",
            "file": "PluginConfig.php",
            "line": 124
        }
    },
    "additional_errors": []
}

#11 @spacedmonkey
5 weeks ago

@TimothyBlynJacobs Do you mind reviewing this ticket? It is pretty simple but adds a lot of value.

#12 @TimothyBlynJacobs
5 weeks ago

  • Keywords commit added

This approach looks good to me!

#13 @rcorrales
3 weeks ago

I applied the patch from https://patch-diff.githubusercontent.com/raw/WordPress/wordpress-develop/pull/5840.diff locally, forced an error, and confirmed that the details appeared as a data.error object when setting Content-Type: application/json and WP_DEBUG_DISPLAY is enabled.

@spacedmonkey commented on PR #5840:


3 weeks ago
#14

Commited

#15 @spacedmonkey
3 weeks ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 57610:

REST API: Provide detailed error data in REST API response.

When the fatal error handler is triggered within a REST API request, it currently utilizes wp_die to display a specially formatted error response. However, crucial information captured by the fatal error handler, such as the exact line where the error occurred, is not included in the response due to potential security concerns, such as leaking file paths.

To address this limitation and aid developers in debugging, this enhancement introduces the inclusion of error data in the response when the WP_DEBUG_DISPLAY constant is set to true. This additional data, appended under the new key error_data, will facilitate more thorough debugging for REST API errors.

Props ecc, spacedmonkey, TimothyBlynJacobs, rcorrales.
Fixes #60014.

#16 @spacedmonkey
3 weeks ago

  • Keywords needs-dev-note added
Note: See TracTickets for help on using tickets.