Secure Logging Solution - #1590
Conversation
huss
left a comment
huss
left a comment
There was a problem hiding this comment.
@Zach-O-Bates Thank you for working on this. I've made some comments to consider. If some seem off then I'm sorry for my lack of understanding. I have not run this yet (am waiting on any changes). If anything is not clear or you have thoughts then please just let me know.
| @@ -0,0 +1,12 @@ | |||
| function canonicalize(input){ | |||
There was a problem hiding this comment.
There should be JSDoc. Also, formatting is a little off.
Finally, do you view this as a general function or part of sanitizeForLog. If the latter then maybe both should be in a single file.
| } | ||
| // Force string | ||
| let value = String(input); | ||
| // Normalize Unicode |
There was a problem hiding this comment.
Could this comment expand on how (specifically why NFKC was used) and why it is normalizing the string?
| @@ -0,0 +1,11 @@ | |||
| const { canonicalize } = require('./canonicalize'); | |||
|
|
|||
| function sanitizeForLog(input) { | |||
| function sanitizeForLog(input) { | ||
| const normalized = canonicalize(input); | ||
|
|
||
| return normalized |
There was a problem hiding this comment.
Could you please have a comment to describe what the sequence of replace are doing and the end result. Also, I prefer to have the result in a variable and then returned because it is easier to debug the value.
| * License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||
|
|
||
| /* Run in OED Docker web container terminal/shell: |
There was a problem hiding this comment.
This applies to all tests so could the comment be removed. This is normally only done for debugging and this is run via the entire test suite in most cases.
| } | ||
| log.info(s); | ||
|
|
||
| log.info('Handling Obvius STATUS request', { |
There was a problem hiding this comment.
I'm not going to mark all of these but see above about seeming to change response.
| res.json(rows.map(row => formatMeterForResponse(row, isAuthorizedCSV))); | ||
| } catch (err) { | ||
| log.error(`Error while performing GET all meters query: ${err}`, err); | ||
| const safeErrorMessage = sanitizeForLog(err?.message || String(err)); |
There was a problem hiding this comment.
See obvius.js for question on changing result.
| res.json(formatMeterForResponse(meter, true)); | ||
| } catch (err) { | ||
| log.error(`Error while editing a meter with detail "${err['detail']}"`, err); | ||
| const safeDetail = sanitizeForLog(err?.detaul?.toString() || ``) |
| res.json(formatMeterForResponse(newMeter, true)); | ||
| } catch (err) { | ||
| log.error(`Error while inserting new meter with detail "${err['detail']}"`, err); | ||
| const safeDetail = sanitizeForLog(err?.detaul?.toString() || ``) |
| .catch(err => { | ||
| log.error(`Failed to remove the file ${csvFilepath}.`, err); | ||
| const safeCsvPath = sanitizeForLog(csvFilepath); | ||
| const safeErrorMessage = sanitizeForLog(err?.message || String(err)); |
There was a problem hiding this comment.
See obvius.js for question on changing result.
|
@Zach-O-Bates Thank you for your work on this issue. I know it is after the time you originally intended to work on OED. Given OED wants to finish this up & it has been several months since my last comment, I'm going to get someone else to complete this work unless you indicate by 6/15/26 that you intend to work on it and it is completed in the near future. |
2 participants
This PR updates the server-side logging to sanitize user input before it’s written to the logs. It removes or escapes control characters like newlines and carriage returns to prevent log injection and keep log entries from being manipulated. The changes were applied across the relevant routes to keep logging consistent, and existing functionality is unchanged.
Developed and implemented by:
Zachary Bates - https://github.com/Zach-O-Bates
Type of change
Checklist
Limitations
This solution addresses specified routes and some that were found during development. This solution should be implemented for any new routes so that the issue is not reintroduced into the codebase.