fix: add error logging to JsonException catch blocks#10253
Conversation
|
Hi there, gr8man! 👋 Thank you for sending this PR! We expect the following in all Pull Requests (PRs).
Important We expect all code changes or bug-fixes to be accompanied by one or more tests added to our test suite to prove the code works. If pull requests do not comply with the above, they will likely be closed. Since we are a team of volunteers, we don't have any more time to work See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md Sincerely, the mergeable bot 🤖 |
patel-vansh
left a comment
patel-vansh
left a comment
There was a problem hiding this comment.
Some comments. First of all, I think this PR is trying to solve multiple different things (like Cache error logging, JSONException logging inside CSRF, and some database BaseResult fix), however generally its recommended to have one PR per one fix. Also, I think the PR description should also be changed to reflect the changes made in the code.
That said, I think these error logging can be nice enhancement (not a bug fix), but I would more lean towards these handlers throwing errors themselves instead of just logging. This would let users react to them as per their preferences. Throwing specific errors would let them pin point the exact problem without guessing, but again, it would be an enhancement and potentially a breaking change.
| public function delete(string $key): bool | ||
| { | ||
| $key = static::validateKey($key, $this->prefix); | ||
| $key = static::validateKey($key, $this->prefix); | ||
| $file = $this->path . $key; | ||
|
|
||
| if (! is_file($file)) { | ||
| return false; | ||
| } | ||
|
|
||
| $result = @unlink($file); | ||
|
|
||
| return is_file($this->path . $key) && unlink($this->path . $key); | ||
| if ($result === false) { | ||
| log_message('error', 'Failed to delete cache file: ' . $file); | ||
| } | ||
|
|
||
| return $result; | ||
| } |
There was a problem hiding this comment.
I am not sure if we actually need this logging or not. Since the delete() method already returns bool, whether to log or not would be user's preference rather than framework responsibility. If they decide to not log and just pass on, this log_message() call would be redundant in that case
| if ($content === false) { | ||
| log_message('error', 'Failed to read cache file: ' . $this->path . $filename); | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
Again, same as above, since we're already returning false, that would already mean the read operation failed, so we can just return false instead of logging.
michalsn
left a comment
michalsn
left a comment
There was a problem hiding this comment.
Please keep this PR focused on a single issue. If you want to address another problem, please open a separate PR for that.
| try { | ||
| $json = json_decode($body, flags: JSON_THROW_ON_ERROR); | ||
| } catch (JsonException) { | ||
| log_message('error', 'Invalid JSON in request body during CSRF token removal'); |
There was a problem hiding this comment.
An error level failure is logged by default in the production environment, which allows a requester to cheaply generate disk-backed log entries. I don't believe we need to log this at all.
michalsn
changed the title
4 participants
Description
Added
log_message('error', ...)in bothcatch (JsonException)blocks. Previously, these exceptions were silently swallowed, which could mask invalid JSON payloads and potential attacks.Checklist: