View Issue Details

IDProjectCategoryView StatusLast Update
0023706mantisbtadministrationpublic2022-04-24 11:18
Reportervboctor Assigned Tovboctor  
PrioritynormalSeveritymajorReproducibilityN/A
Status closedResolutionfixed 
Product Version2.9.0 
Target Version2.11.0Fixed in Version2.11.0 
Summary0023706: trigger_error() with errors must terminate scripts rather than being config based
Description

The MantisBT code is written with the assumption that a trigger_error() call that is passed E_ERROR and E_USER_ERROR should terminate the script. This behavior shouldn't be configurable via $g_display_errors for the following reasons:

  • This can cause security issues if administrator decides to change default configuration to continue on errors. Here is an example that doesn't use access_denied() but uses trigger errors, there is 11 of these in the code currently.
if( !config_get( 'enable_profiles' ) ) {
    trigger_error( ERROR_ACCESS_DENIED, ERROR );
}
  • As we move to exceptions, the idea of continuing after an error is not possible, since the exception will bubble up until it gets caught and at that point, the point that triggered the exception can't continue (unless it is caught in place and it makes sense to continue).

  • This can cause data integrity issues errors. For example:

function bug_ensure_exists( $p_bug_id ) {
    if( !bug_exists( $p_bug_id ) ) {
        error_parameters( $p_bug_id );
        trigger_error( ERROR_BUG_NOT_FOUND, ERROR );
    }
}

Example:
- bug_ensure_exists( $t_bug_id );
- Add a record to monitor table for a user to monitor this bug or attempt to do work on the bug.

Recommendation:

  • Move trigger errors to access denied for the 11 cases.
  • Remove support for $g_display_errors to config E_USER_ERRORS and E_ERRORS but always overwriting these to HALT and updating documentation.

We could also consider deprecating $g_display_errors all together to remove ability to continue after notices, warnings, deprecated, etc. Also this would remove control for having errors show at the bottom of the page instead of inline.

The above is a required step in our transition to exceptions. I'm thinking with core APIs we could throw exceptions instead of calling trigger_errors( ..., E_USER_ERROR ) and then have an unhandled exceptions handler that handles unhandled exceptions from UI scripts and internally calls trigger_error pulling information from the exception (e.g. error code, parameters, and stack trace). Where in case of REST/SOAP API, it is expected that APIs will catch such exceptions or have the unhandled exception handler behave differently.

TagsNo tags attached.

Relationships

related to 0023876 closeddregad Running admin/check fails 

Activities

atrol

atrol

2017-12-07 03:12

developer   ~0058330

As we move to exceptions

@vboctor I didn't have a deeper look, maybe it's worth to have a look at what dhx implemented in next branch some years ago.
https://github.com/vboctor/mantisbt/commits/next?after=4baa1c7fad8db9e8672ac112cd1626195d38600c+34

dregad

dregad

2017-12-07 04:41

developer   ~0058336

I support deprecating the ability to continue after a fatal error. IIRC, I already modified error API a few years ago, to ensure this happens for PHP E_ERRORs regardless of how $g_display_errors is set.

I'm not so keen on deprecating $g_display_errors entirely, because I believe it is a very useful feature to be able to choose the desired behavior for different error types (hide / display inline / halt), especially during development. I would only agree to that if the new Exceptions model somehow allows the same behavior (or maybe I should say functionality).

vboctor

vboctor

2017-12-08 21:40

manager   ~0058366

I have just made the change where ERROR (E_USER_ERROR) always terminate the script as part of the following PR: https://github.com/mantisbt/mantisbt/pull/1240

Related Changesets

MantisBT: master f9d45aab

2017-12-07 08:39

vboctor


Details Diff
Force E_USER_ERROR to HALT

The code assumes that such errors will halt executions, so such errors shouldn’t ever resume.

Fixes 0023706
Affected Issues
0023706
mod - core/error_api.php Diff File

MantisBT: master c816588d

2017-12-08 16:59

vboctor


Details Diff
Enforce halting of scripts on errors

Enforce for E_ERROR and E_RECOVERABLE_ERROR and not just E_USER_ERROR.

Updated documentation, config_defaults_inc.php samples and defaults.

Fixes 0023706
Affected Issues
0023706
mod - config_defaults_inc.php Diff File
mod - core/error_api.php Diff File
mod - docbook/Admin_Guide/en-US/config/logging.xml Diff File