Skip to content

fix(web): prevent invalid Unicode in API request logs - #4913

Merged
chrarnoldus merged 2 commits into
mainfrom
little-bosworth
Jul 31, 2026
Merged

fix(web): prevent invalid Unicode in API request logs#4913
chrarnoldus merged 2 commits into
mainfrom
little-bosworth

Conversation

@chrarnoldus

@chrarnoldus chrarnoldus commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Summary

  • repair lone UTF-16 surrogates and NUL characters before inserting JSONB request-log fields
  • use native String.prototype.isWellFormed() and toWellFormed() with an allocation-free clean-string path
  • preserve valid surrogate pairs and sanitize nested values and keys
  • add regression coverage for PostgreSQL-incompatible Unicode payloads

Verification

  • pnpm --filter web exec jest src/lib/rewriteModelResponse.test.ts src/lib/sanitize-jsonb.test.ts --runInBand --forceExit
  • pnpm --filter web run typecheck
  • pnpm exec oxlint --config .oxlintrc.json apps/web/src/lib/sanitize-jsonb.ts apps/web/src/lib/sanitize-jsonb.test.ts apps/web/src/lib/rewriteModelResponse.ts
  • pnpm format
  • git diff --check

@chrarnoldus chrarnoldus self-assigned this Jul 31, 2026
Comment thread apps/web/src/lib/sanitize-jsonb.ts Outdated
Comment thread apps/web/src/lib/sanitize-jsonb.ts Outdated
@kilo-code-bot

kilo-code-bot Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Executive Summary

The incremental change replaces the hand-rolled surrogate repair with isWellFormed()/toWellFormed() plus explicit U+0000 replacement, which resolves both previously reported findings with high confidence.

Verification of previous findings
  • Resolved (WARNING, U+0000 escape): sanitizeJsonbString now short-circuits only when the string is well-formed and contains no U+0000, and otherwise applies replaceAll to map U+0000 to U+FFFD. Both object keys and nested values go through the same function, and the test covers a key and a value containing U+0000.
  • Resolved (SUGGESTION, per-character rebuild): clean strings return the original reference after two native scans, so large request bodies are no longer copied character by character.
Notes
  • Runtime support checked: String.prototype.isWellFormed/toWellFormed require Node 20+; the repo pins "node": ">=24 <25", and the only caller (rewriteModelResponse.ts) is server-side, so there is no browser-compatibility exposure.
  • Type support checked: apps/web/tsconfig.json uses lib: ["dom", "dom.iterable", "esnext", ...], which includes the ES2024 string declarations.
  • Behavior parity confirmed: valid surrogate pairs are preserved by toWellFormed(), and the input object is not mutated (Object.fromEntries builds a new object).
  • U+0000 is the only code point PostgreSQL jsonb rejects in escaped form, so no additional characters need handling.
  • Verification commands from the PR description were not re-run (read-only review mode).
Files Reviewed (2 files)
  • apps/web/src/lib/sanitize-jsonb.ts
  • apps/web/src/lib/sanitize-jsonb.test.ts
Previous Review Summary (commit 09ea1f8)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 09ea1f8)

Status: 2 Issues Found | Recommendation: Address before merge

Executive Summary

The new JSONB sanitizer correctly repairs unpaired UTF-16 surrogates, but it does not cover an escaped NUL code point (U+0000), which PostgreSQL jsonb rejects with the same error class, so api_request_log inserts can still fail.

Overview

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 1
Issue Details (click to expand)

WARNING

File Line Issue
apps/web/src/lib/sanitize-jsonb.ts 6 NUL (U+0000) is escaped by JSON.stringify and rejected by PostgreSQL jsonb with the same unsupported Unicode escape sequence error, so the insert can still fail

SUGGESTION

File Line Issue
apps/web/src/lib/sanitize-jsonb.ts 10 Per-character rebuild runs even for already-valid strings and is then discarded; a surrogate-range regex fast path avoids copying large request bodies
Notes
  • Verified api_request_log.response is text(), not jsonb(), so leaving responseText unsanitized is correct: node-postgres encodes lone surrogates to U+FFFD when writing UTF-8 text parameters, and only the jsonb path emits the rejected escape sequence.
  • Sanitizing request.body widens the inserted value to unknown; that is compatible with the untyped jsonb() column, so there is no type-safety regression at the insert site.
  • Verification commands in the PR description were not re-run (read-only review mode).
  • The NUL literals in the inline comment on line 6 were normalized away by the GitHub API; read that comment as referring to the U+0000 escape sequence.
Files Reviewed (3 files)
  • apps/web/src/lib/sanitize-jsonb.ts - 2 issues
  • apps/web/src/lib/sanitize-jsonb.test.ts - 0 issues
  • apps/web/src/lib/rewriteModelResponse.ts - 0 issues

Fix these issues in Kilo Cloud


Reviewed by claude-opus-5 · Input: 20 · Output: 4.1K · Cached: 469.4K

Review guidance: REVIEW.md from base branch main

@chrarnoldus
chrarnoldus merged commit 48cb04e into main Jul 31, 2026
16 checks passed
@chrarnoldus
chrarnoldus deleted the little-bosworth branch July 31, 2026 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants