-
-
Couldn't load subscription status.
- Fork 1.5k
fix: return 401 instead of 500 for AJAX requests with expired session #21599
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
Conversation
|
No tests added for the moment as i am not sure about potential impact : need pre-review first. |
|
I'm not confident enough with new routiong system, I'll wait for @cedric-anne review. |
| // For AJAX/JSON requests, return a JSON response with 401 status | ||
| $response = new JsonResponse([ | ||
| 'error' => __('Your session has expired'), | ||
| 'message' => __('Please log in again'), | ||
| 'code' => 'ERROR_SESSION_EXPIRED', | ||
| ], 401); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throwing back a HttpException should permit to have it correctly handled by the ErrorController.
| // For AJAX/JSON requests, return a JSON response with 401 status | |
| $response = new JsonResponse([ | |
| 'error' => __('Your session has expired'), | |
| 'message' => __('Please log in again'), | |
| 'code' => 'ERROR_SESSION_EXPIRED', | |
| ], 401); | |
| // For AJAX/JSON requests, convert the error into a HttpException | |
| $http_exception = new \Glpi\Exception\Http\HttpException(403, 'Session expired.'); | |
| $http_exception->setMessageToDisplay(__('Your session has expired. Please log in again.')); | |
| throw $http_exception; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Response code shouldn't be 401 instead of 403 ? https://stackoverflow.com/questions/1653493/what-http-status-code-is-supposed-to-be-used-to-tell-the-client-the-session-has
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed here 2d6d33b but i kept 401 Response Code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the $event->setResponse($response); is wrongly conditionned
Nevermind, it's OK |
Fix AJAX requests returning HTTP 500 instead of 401 when session expires.
The AccessErrorListener had an early return for AJAX requests that prevented handling of SessionExpiredException. This has been fixed by processing SessionExpiredException before the
AJAX check.
For AJAX requests with expired sessions, the listener now returns a proper JSON 401 response:
HTML requests continue to redirect to the login page as before. Other exception handling remains unchanged.
Warning : This fix applies to all 122 AJAX endpoints in the application. Low risk of impact, though frontend handling of 401 responses should be monitored after deployment.