JWT Handler Response Format Fix#56
Conversation
Do not return anything in handler in order for Coldbox's RestHandler to properly get the responseData Update test to validate the rendered content
|
Thanks so much!! |
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where the JWT Handler's refreshToken() method improperly returns the response object, causing ColdBox's RestHandler to skip its response formatting logic. The fix ensures that handlers return void/null so the RestHandler can properly invoke getDataPacket() and format the response correctly.
Key Changes:
- Removed
returnstatements that returned the response object from error handling paths inhandlers/Jwt.cfc - Updated catch blocks to use
event.getResponse()consistently instead ofprc.response - Added integration test assertions to validate the rendered content matches ColdBox RestHandler's default response format
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| handlers/Jwt.cfc | Fixed all error handling paths to avoid returning the response object, ensuring RestHandler's formatting logic executes properly |
| test-harness/tests/specs/integration/JWTSpec.cfc | Added test assertions to validate the rendered JSON response contains the expected 4 keys (data, error, pagination, messages) |
| event | ||
| .getResponse() | ||
| .setErrorMessage( | ||
| "Token Expired - #e.message#", | ||
| 400, | ||
| "Token Expired" | ||
| ); |
There was a problem hiding this comment.
[nitpick] Missing explicit return; statement for consistency. Line 24 includes an explicit return; after setting the error message. For consistency across all error paths in this function, consider adding return; after line 60.
| .addMessage( "Tokens refreshed! The passed in refresh token has been invalidated" ); | ||
| } catch ( RefreshTokensNotActive e ) { | ||
| return event.getResponse().setErrorMessage( "Refresh Tokens Not Active", 404, "Disabled" ); | ||
| event.getResponse().setErrorMessage( "Refresh Tokens Not Active", 404, "Disabled" ); |
There was a problem hiding this comment.
[nitpick] Missing explicit return; statement for consistency. Line 24 includes an explicit return; after setting the error message. For consistency across all error paths in this function, consider adding return; here as well.
| "The refresh token was not passed via the header or the rc. Cannot refresh the unrefreshable!", | ||
| 400, | ||
| "Missing refresh token" | ||
| ); |
There was a problem hiding this comment.
[nitpick] Missing explicit return; statement for consistency. Line 24 includes an explicit return; after setting the error message. For consistency across all error paths in this function, consider adding return; after line 44.
| ); | |
| ); | |
| return; |
| event | ||
| .getResponse() | ||
| .setErrorMessage( | ||
| "Invalid Token - #e.message#", | ||
| 401, | ||
| "Invalid Token" | ||
| ); |
There was a problem hiding this comment.
[nitpick] Missing explicit return; statement for consistency. Line 24 includes an explicit return; after setting the error message. For consistency across all error paths in this function, consider adding return; after line 52.
Description
Currently, the JWT Handler improperly returns a value causing it to skip ColdBox's RestHandler's response formatting logic. This results in the entire response object being returned rather than just invoking getDataPacket()
Before:
{ "STATUS_TEXTS": { "300": "Multiple Choices", "503": "Service Unavailable", "101": "Switching Protocols", "406": "Not Acceptable", "207": "Multi-Status", "424": "Failed Dependency", "203": "Non-authoritative Information", "410": "Gone", "502": "Bad Gateway", "508": "Loop Detected", "308": "Permanent Redirect", "403": "Forbidden", "400": "Bad Request", "444": "Connection Closed Without Response", "599": "Network Connect Timeout Error", "506": "Variant Also Negotiates", "404": "Not Found", "102": "Processing", "202": "Accepted", "431": "Request Header Fields Too Large", "412": "Precondition Failed", "510": "Not Extended", "418": "I'm a teapot", "411": "Length Required", "421": "Misdirected Request", "304": "Not Modified", "208": "Already Reported", "226": "IM Used", "428": "Precondition Required", "302": "Found", "416": "Requested Range Not Satisfiable", "204": "No Content", "501": "Not Implemented", "100": "Continue", "429": "Too Many Requests", "305": "Use Proxy", "402": "Payment Required", "303": "See Other", "200": "OK", "499": "Client Closed Request", "417": "Expectation Failed", "401": "Unauthorized", "423": "Locked", "505": "HTTP Version Not Supported", "205": "Reset Content", "405": "Method Not Allowed", "451": "Unavailable For Legal Reasons", "408": "Request Timeout", "413": "Payload Too Large", "307": "Temporary Redirect", "504": "Gateway Timeout", "407": "Proxy Authentication Required", "206": "Partial Content", "422": "Unprocessable Entity", "409": "Conflict", "500": "Internal Server Error", "426": "Upgrade Required", "301": "Moved Permanently", "414": "Request-URI Too Long", "201": "Created", "511": "Network Authentication Required", "507": "Insufficient Storage", "415": "Unsupported Media Type" }, "$wbMixer": true, "hasData": false, "hasPagination": false, "format": "json", "data": {}, "pagination": { "totalPages": 1, "maxRows": 0, "offset": 0, "page": 1, "totalRecords": 0 }, "error": true, "binary": false, "messages": [ "Refresh Token Endpoint Disabled" ], "location": "", "jsonCallback": "", "contentType": "", "statusCode": 404, "statusText": "Ok", "responsetime": 0, "headers": [ { "name": "x-response-time", "value": 0 } ] }After:
{ "error": true, "messages": [ "Refresh Token Endpoint Disabled" ] }I updated the test to validate the rendered content returned
Issues
Type of change
Checklist