View Issue Details

IDProjectCategoryView StatusLast Update
0023706mantisbtadministrationpublic2017-12-08 21:40
ReportervboctorAssigned Tovboctor 
PrioritynormalSeveritymajorReproducibilityN/A
Status assignedResolutionopen 
Product Version2.9.0 
Target Version2.10.0Fixed in Version 
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.

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

Issue History

Date Modified Username Field Change
2017-12-07 02:21 vboctor New Issue
2017-12-07 03:12 atrol Note Added: 0058330
2017-12-07 04:41 dregad Note Added: 0058336
2017-12-08 21:40 vboctor Assigned To => vboctor
2017-12-08 21:40 vboctor Status new => assigned
2017-12-08 21:40 vboctor Product Version => 2.9.0
2017-12-08 21:40 vboctor Target Version => 2.10.0
2017-12-08 21:40 vboctor Note Added: 0058366