Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: exceptionHandler may return invalid HTTP status code #6228

Merged
merged 10 commits into from
Aug 6, 2022

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Jul 4, 2022

Description
The current implementation has undocumented behavior that use Exception code as HTTP status code,
and determine Exit code from Exception code.
But Exception code has nothing to do with HTTP status code or Exit code.

This PR introduces interfaces that explicitly show the Exception code is HTTP status code or Exit code

  • add HTTPExceptionInterface and HasExitCodeInterface
  • fix Exceptions::determineCodes()

Breaking Changes:

  • If you expect Exception code as HTTP status code, the HTTP status code will be changed.
    In that case, you need to implements HTTPExceptionInterface in the Exception.
  • If you expect Exit code based on Exception code, the Exit code will be changed.
    In that case, you need to implements HasExitCodeInterface in the Exception.

Related: #6286

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added bug Verified issues on the current code behavior or pull requests that will fix them breaking change Pull requests that may break existing functionalities labels Jul 4, 2022
@iRedds
Copy link
Collaborator

iRedds commented Jul 4, 2022

I have doubts about the implementation.
If it's an HTTP exception, then the code should be >= 400 and < 500.
In all other cases, the code should be 500.

Something like this

$statusCode = 500;
$exitStatus = EXIT_ERROR;

if (
    $exception instanceof HttpException
    && $exception->getCode() >= 400
    && $exception->getCode() < 500
) {
    $statusCode = $exception->getCode();    
}

There is no point in additional interfaces.
And to be honest, it's a little strange for me that HTTP exceptions are handled through Debug.

@kenjis
Copy link
Member Author

kenjis commented Jul 6, 2022

it's a little strange for me that HTTP exceptions are handled through Debug.

Yes. This is global exception handler, so it should return 500 Response for all cases.
HTTP exceptions should be handled before here.

@kenjis kenjis marked this pull request as draft July 6, 2022 04:15
@MGatner
Copy link
Member

MGatner commented Jul 9, 2022

Just a note that HTTP exceptions actually include 300s as well via RedirectException.

@kenjis
Copy link
Member Author

kenjis commented Jul 11, 2022

There is HTTPException, but it has nothing to do with HTTP response code.

class HTTPException extends FrameworkException

@kenjis kenjis marked this pull request as ready for review July 11, 2022 03:14
@kenjis
Copy link
Member Author

kenjis commented Jul 11, 2022

RedirectException is caught in CodeIgniter::run():

} catch (RedirectException $e) {
$logger = Services::logger();
$logger->info('REDIRECTED ROUTE at ' . $e->getMessage());
// If the route is a 'redirect' route, it throws
// the exception with the $to as the message
$this->response->redirect(base_url($e->getMessage()), 'auto', $e->getCode());
$this->sendResponse();
$this->callExit(EXIT_SUCCESS);

@kenjis
Copy link
Member Author

kenjis commented Jul 11, 2022

I think this PR is ready for reviews.
I think this is the best we can do at the moment.

@kenjis kenjis force-pushed the fix-exceptionHandler-statusCode branch 2 times, most recently from d414c1e to 0581338 Compare July 11, 2022 07:06
@MGatner
Copy link
Member

MGatner commented Jul 11, 2022

RedirectException is caught in CodeIgniter::run()

Why not make RedirectException and PageNotFoundException extend HTTPException and then catch all HTTPException in run() and handle them there? These two are currently special cases but I could see a developer wanting to force any HTTP code response at any point via an exception.

@MGatner
Copy link
Member

MGatner commented Jul 11, 2022

Actually I just read this:

The exception code has nothing to do with HTTP status code.
So it is impossible to determine HTTP status code from exception code.

So maybe the exception type is what defines these two special cases and we don't want more robust handling.

@kenjis
Copy link
Member Author

kenjis commented Jul 12, 2022

Why not make RedirectException and PageNotFoundException extend HTTPException and then catch all HTTPException in run() and handle them there?

404 is handled in the exceptionHandler() now.
If we catch all HTTPException in run(), we need to change the current code more.

@kenjis kenjis marked this pull request as draft July 12, 2022 00:48
@kenjis
Copy link
Member Author

kenjis commented Jul 12, 2022

I've changed my mind.
This PR changes too much. If we add interfaces or change the parent class, it should go 4.3.

I sent another PR #6254 with minimum changes for the bug fix.

@paulbalandan
Copy link
Member

With the merge of #6254 , is this still needed?

@kenjis
Copy link
Member Author

kenjis commented Jul 22, 2022

Yes, #6254 is a workaround.

I think the current behavior is broken. Because Exception code has nothing to do with HTTP status code/Exit code.
If the Exception code means HTTP status code/Exit code, devs should make it explicit.

@kenjis kenjis force-pushed the fix-exceptionHandler-statusCode branch 2 times, most recently from a2277d7 to e62b283 Compare July 22, 2022 06:08
@kenjis kenjis changed the base branch from develop to 4.3 July 22, 2022 06:08
@kenjis kenjis added the 4.3 label Jul 22, 2022
@kenjis kenjis force-pushed the fix-exceptionHandler-statusCode branch from e62b283 to c7bee50 Compare July 22, 2022 22:15
@kenjis
Copy link
Member Author

kenjis commented Jul 22, 2022

It seems to me that it is impossible to use Exception code to represent Exit code.
If you have already used an Exception code, you cannot specify an Exit code.

I feel it would be better to have getExitCode() in ExitExceptionInterface.

@kenjis kenjis force-pushed the fix-exceptionHandler-statusCode branch from c7bee50 to dc51f1e Compare July 24, 2022 00:48
@kenjis
Copy link
Member Author

kenjis commented Jul 24, 2022

I renamed ExitExceptionInterface to hasExitCodeInterface and added getExitCode() method.

@kenjis kenjis marked this pull request as ready for review July 24, 2022 01:05
@kenjis kenjis force-pushed the fix-exceptionHandler-statusCode branch from 9468981 to 3bac9cb Compare July 24, 2022 11:15
Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good by me!

@kenjis kenjis force-pushed the fix-exceptionHandler-statusCode branch from 3bac9cb to 5358b14 Compare August 4, 2022 01:23
@kenjis
Copy link
Member Author

kenjis commented Aug 4, 2022

Added the docs.

@kenjis kenjis force-pushed the fix-exceptionHandler-statusCode branch from 5358b14 to cb1b570 Compare August 4, 2022 01:27
Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Some proofing suggestions.

user_guide_src/source/changelogs/v4.3.0.rst Outdated Show resolved Hide resolved
user_guide_src/source/changelogs/v4.3.0.rst Outdated Show resolved Hide resolved
user_guide_src/source/general/errors.rst Outdated Show resolved Hide resolved
user_guide_src/source/general/errors.rst Outdated Show resolved Hide resolved
user_guide_src/source/installation/upgrade_430.rst Outdated Show resolved Hide resolved
user_guide_src/source/installation/upgrade_430.rst Outdated Show resolved Hide resolved
kenjis and others added 6 commits August 5, 2022 16:25
Co-authored-by: MGatner <mgatner@icloud.com>
Co-authored-by: MGatner <mgatner@icloud.com>
Co-authored-by: MGatner <mgatner@icloud.com>
Co-authored-by: MGatner <mgatner@icloud.com>
Co-authored-by: MGatner <mgatner@icloud.com>
Co-authored-by: MGatner <mgatner@icloud.com>
@kenjis kenjis requested a review from MGatner August 5, 2022 07:26
@kenjis kenjis merged commit 9bdd9f5 into codeigniter4:4.3 Aug 6, 2022
@kenjis kenjis deleted the fix-exceptionHandler-statusCode branch August 6, 2022 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.3 breaking change Pull requests that may break existing functionalities bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
4 participants