fix: strip CR/LF from logged values#42
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the sanitizeForLog function to strip ASCII control characters, including carriage return (CR) and line feed (LF), to prevent log-line injection, and adds corresponding unit tests. The review feedback suggests extending this protection to Unicode line and paragraph separators (U+2028 and U+2029) and adding tests to verify their removal.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // Remove all ASCII control characters except tab (0x09). | ||
| return value.replace(/[\x00-\x08\x0A-\x1F\x7F]/g, ''); |
There was a problem hiding this comment.
While the updated regular expression successfully strips ASCII control characters like CR (\\r) and LF (\\n), it does not account for Unicode line and paragraph separators such as \\u2028 (Line Separator) and \\u2029 (Paragraph Separator). In environments or log viewers that support Unicode, these characters can also be interpreted as line breaks, potentially allowing log injection/forging.\n\nTo ensure robust protection against log injection, update the regular expression to also strip \\u2028 and \\u2029.
| // Remove all ASCII control characters except tab (0x09). | |
| return value.replace(/[\x00-\x08\x0A-\x1F\x7F]/g, ''); | |
| // Remove all ASCII control characters except tab (0x09), plus Unicode line/paragraph separators.\n return value.replace(/[\\x00-\\x08\\x0A-\\x1F\\x7F\\u2028\\u2029]/g, ''); |
There was a problem hiding this comment.
Agreed — and already covered in this PR’s regex.
sanitizeForLog() strips \u2028 and \u2029 in addition to ASCII controls (except tab). The regression test asserts CR/LF plus both Unicode separators are removed while printable Unicode is preserved.
Latest push only clarifies the comment so it matches the implemented behavior.
| test('sanitizeForLog prevents CR/LF log-line injection and preserves printable Unicode', () => { | ||
| assert.equal( | ||
| sanitizeForLog('combo\n[ERROR] forged\rrewritten'), | ||
| 'combo[ERROR] forgedrewritten', | ||
| ); | ||
| assert.equal(sanitizeForLog('Modell ✓ — 日本語'), 'Modell ✓ — 日本語'); | ||
| }); |
There was a problem hiding this comment.
Add test coverage for Unicode line and paragraph separators (\\u2028 and \\u2029) to verify that they are also successfully stripped by sanitizeForLog to prevent log injection.
test('sanitizeForLog prevents CR/LF log-line injection and preserves printable Unicode', () => {\n assert.equal(\n sanitizeForLog('combo\\n[ERROR] forged\\rrewritten\\u2028forged2\\u2029rewritten2'),\n 'combo[ERROR] forgedrewrittenforged2rewritten2',\n );\n assert.equal(sanitizeForLog('Modell ✓ — 日本語'), 'Modell ✓ — 日本語');\n});There was a problem hiding this comment.
Addressed in the latest push: sanitizeForLog() now strips U+2028/U+2029 as well, and the forged-line regression test covers both separators.
Code Review Roast 🔥Verdict: No Issues Found | Recommendation: Merge Oh wait, this PR is actually clean. I need to sit down. I had my flamethrower warmed up and everything. 📊 Overall: Like finding a unicorn in production — I didn't think clean PRs existed anymore, but here we are. Files Reviewed (1 file)
Reviewed with model: kilo/kilo-auto/free Previous Review Summaries (2 snapshots, latest commit f44a918)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit f44a918)Verdict: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)
🏆 Best part: The exhaustive test coverage for all 32 ASCII control codes is the kind of thoroughness that makes QA engineers weep tears of joy. 💀 Worst part: The comment on line 13 is a documentation time bomb — it says "ASCII control characters" while the code also strips U+2028/U+2029. 📊 Overall: Like a Heimlich maneuver that saves the patient but the doctor forgets to mention the Heimlich maneuver in the chart. Files Reviewed (2 files)
Fix these issues in Kilo Cloud Reviewed with model: kilo/kilo-auto/free Previous review (commit 8013867)Verdict: No Issues Found | Recommendation: Merge Oh wait, this PR is actually clean. I need to sit down. I had my flamethrower warmed up and everything. 📊 Overall: Like finding a unicorn in production — I didn't think clean PRs existed anymore, but here we are. Files Reviewed (2 files)
Reviewed with model: kilo/kilo-auto/free Reviewed by step-3.7-flash · Input: 57.7K · Output: 4.3K · Cached: 193.5K |
8013867 to
f44a918
Compare
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| export function sanitizeForLog(value: string): string { | ||
| // Remove all control characters except tab (0x09) | ||
| return value.replace(/[\x00-\x08\x0B\x0C\x0E-\x1F\x7F]/g, ''); | ||
| // Remove all ASCII control characters except tab (0x09). |
There was a problem hiding this comment.
🔥 The Roast: This comment says "ASCII control characters" but the regex silently strips \u2028 and \u2029 too. Future maintainers will read this, think only ASCII is being sanitized, and then wonder why their Unicode log-line injection is getting silently consumed. Documentation that lies is just tech debt with better marketing.
🩹 The Fix:
| // Remove all ASCII control characters except tab (0x09). | |
| // Remove all ASCII control characters except tab (0x09), plus Unicode line/paragraph separators. |
📏 Severity: suggestion
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Addressed in the latest push.
Comment now states explicitly that ASCII controls (except tab) and Unicode line/paragraph separators (U+2028 / U+2029) are stripped, matching the regex.
The regex already removes U+2028/U+2029; update the comment so it matches the implemented behavior called out in review.
Review follow-up
Happy to adjust further if anything else stands out. |
1 participant
Summary
sanitizeForLog()while preserving tabRoot cause
The previous regular expression skipped
0x0Aand0x0D, allowing untrusted identifiers to create forged physical log lines despite passing through the sanitizer.Verification
npm test: 68 passed, 0 failednpm testgit diff --checkCloses #41