log: serialize objects with JSON.stringify so errors show content in logs#1207
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the logging utility by ensuring that all arguments passed to log functions, particularly objects, are properly serialized into a human-readable JSON string format. This change significantly improves the debuggability and clarity of log messages, making it easier to understand the state and content of objects at the time of logging, especially for error scenarios. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- JSON.stringify can throw on circular or otherwise unserializable objects; consider wrapping the per-arg stringify in a try/catch and falling back to String(a) so logging never fails.
- Serializing generic objects with JSON.stringify loses useful Error-specific details (e.g., stack) and often returns {}; consider special-casing Error instances to log message and stack in a structured way.
- Changing console.error from console.error(args) to console.error(str) removes the browser/console’s native object inspection (e.g., clickable Error stacks); consider logging both the stringified message and the original args (e.g., console.error(str, ...args)).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- JSON.stringify can throw on circular or otherwise unserializable objects; consider wrapping the per-arg stringify in a try/catch and falling back to String(a) so logging never fails.
- Serializing generic objects with JSON.stringify loses useful Error-specific details (e.g., stack) and often returns {}; consider special-casing Error instances to log message and stack in a structured way.
- Changing console.error from console.error(args) to console.error(str) removes the browser/console’s native object inspection (e.g., clickable Error stacks); consider logging both the stringified message and the original args (e.g., console.error(str, ...args)).
## Individual Comments
### Comment 1
<location path="src/helpers/log.js" line_range="15-16" />
<code_context>
this.logger = cockpit.file(LOG_FILE);
}
+ _stringifyArgs (args) {
+ return args.map(a => typeof a === "object" && a !== null ? JSON.stringify(a) : String(a)).join(" ");
+ }
+
</code_context>
<issue_to_address>
**issue:** Consider guarding JSON.stringify to avoid runtime errors on non-serializable values
JSON.stringify can throw for some inputs (e.g. circular references, BigInt), which would turn logging into a runtime failure. Consider wrapping the JSON.stringify call in try/catch and falling back to String(a) so logging can’t break the main code path.
</issue_to_address>
### Comment 2
<location path="src/helpers/log.js" line_range="41-43" />
<code_context>
}
error (...args) {
+ const str = this._stringifyArgs(args);
// eslint-disable-next-line no-console
- console.error(args);
+ console.error(str);
this._write("ERROR", args);
this._write_to_journal("err", args);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using a single string for console.error changes its semantics compared to passing raw args
Passing the pre-stringified value removes richer console formatting (e.g., Error stacks, inspectable objects) that devtools provide for non-string arguments. If you need stringified output for file/journal logs, consider keeping `console.error(...args)` for the console while still using `_stringifyArgs(args)` only for `_write` / `_write_to_journal`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request introduces a helper function to stringify log arguments, serializing objects using JSON.stringify for improved log visibility. While this enhances debugging, it introduces several security risks, including the potential leakage of sensitive information (passwords, keys, PII) into logs, argument injection, and log injection vulnerabilities. The use of a fixed path in /tmp for log storage also remains an insecure practice. Furthermore, a critical issue was identified where logging an object with a circular reference could crash the logger, and a medium-severity performance issue exists in the error method.
b1d3a5f to
5aa538a
Compare
|
We need to fix this being shown in logs: Mar 05 10:02:17 localhost-live anaconda-webui[40761]: ErrorBoundary caught an error: [object Object] https://bugzilla-attachments.redhat.com/attachment.cgi?id=2132138 |
5aa538a to
319ad7c
Compare
319ad7c to
25decda
Compare
KKoukiou
left a comment
There was a problem hiding this comment.
Error objects, which are printed in case of exceptions are not enumerable to JSON.stringify does not print them.
I needed to create a overrride method for that.
25decda to
f4bad12
Compare
…logs Also log error after they are parsed, so that the stacktrace if it exists contains the source map.
f4bad12 to
82c620b
Compare
No description provided.