Skip to content

fix: detect and decode UTF-16 BOM in read_file (#610) #621

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
PiedPiper911 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: detect and decode UTF-16 BOM in read_file (#610) #621

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 64 additions & 5 deletions src/tools/filesystem.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,20 @@ import { isPdfFile } from "./mime-types.js";
import { parsePdfToMarkdown, editPdf, PdfOperations, PdfMetadata, parseMarkdownToPdf } from './pdf/index.js';
import { isBinaryFile } from 'isbinaryfile';

/**
* Detect UTF-16 BOM (Byte Order Mark) in a buffer.
* Returns the encoding name if a BOM is found, or null otherwise.
* - 0xFF 0xFE → UTF-16 LE (common on Windows, e.g. PowerShell 5.1 redirection)
* - 0xFE 0xFF → UTF-16 BE
*/
function detectUtf16Bom(buffer: Buffer): 'utf-16le' | 'utf-16be' | null {
if (buffer.length >= 2) {
if (buffer[0] === 0xFF && buffer[1] === 0xFE) return 'utf-16le';
if (buffer[0] === 0xFE && buffer[1] === 0xFF) return 'utf-16be';
}
return null;
}

// CONSTANTS SECTION - Consolidate all timeouts and thresholds
const FILE_OPERATION_TIMEOUTS = {
PATH_VALIDATION: 10000, // 10 seconds
Expand Down Expand Up @@ -503,6 +517,38 @@ export async function readFileFromDisk(
// If we can't stat the file, continue anyway and let the read operation handle errors
}

// Detect UTF-16 BOM before delegating to the text handler.
// PowerShell 5.1 on Windows writes UTF-16 LE by default when using redirection
// (e.g., `echo "hello" > file.txt`). The readline-based text handler assumes UTF-8
// and would produce garbled output with embedded NUL characters. By detecting the
// BOM early, we can decode the file correctly.
try {
const rawBuffer = await fs.readFile(validPath);
Comment on lines +525 to +526

@coderabbitai coderabbitai Bot Aug 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Keep BOM probing bounded and cancellable.

fs.readFile(validPath) at Line [526] reads the entire file before the existing timeout starts. It does this for every file without a BOM, and the normal handler reads non-UTF-16 files again. A large or stalled file can consume excessive memory and keep the request pending past the three-minute timeout, even when the caller requests a small line range.

Read only the first two bytes under runWithAbortableTimeout. Read the full file only after a BOM is confirmed, and keep that full read cancellable.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/tools/filesystem.ts` around lines 525 - 526, Update the BOM-detection
flow around fs.readFile(validPath) to read only the first two bytes using
runWithAbortableTimeout with the existing three-minute cancellation behavior.
Only perform a full-file read when the BOM confirms UTF-16, and ensure that read
also runs through runWithAbortableTimeout; preserve the normal handler path
without rereading non-UTF-16 files.

const utf16Encoding = detectUtf16Bom(rawBuffer);
if (utf16Encoding) {
const decoder = new TextDecoder(utf16Encoding);
let decoded = decoder.decode(rawBuffer);
Comment on lines +529 to +530

@coderabbitai coderabbitai Bot Aug 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== file outline =="
ast-grep outline src/tools/filesystem.ts --match TextDecoder --view expanded || true

echo "== relevant lines =="
sed -n '500,545p' src/tools/filesystem.ts
printf '\n---\n'
sed -n '650,685p' src/tools/filesystem.ts

echo "== TextDecoder construction sites =="
rg -n "new TextDecoder|decoder.decode|utf16Encoding|replace\(" src/tools/filesystem.ts

echo "== deterministic JS probe for default TextDecoder behavior =="
node - <<'JS'
for (const inputHex of [
  'd800',
  'dfff',
  'fffd',
  'd8000061',
]) {
  const buffer = Uint8Array.from(inputHex.split(/(..)/).filter(Boolean).map(h => parseInt(h, 16)));
  console.log(inputHex, new TextDecoder().decode(buffer));
}
JS

Repository: wonderwhy-er/DesktopCommanderMCP

Length of output: 4371

Use fatal UTF-16 decoders when decoding errors must be handled explicitly.

Both decoder calls use replacement mode by default, so malformed UTF-16 decoding produces U+FFFD instead of failing and being handled explicitly. Use { fatal: true } at these sites, or reject malformed content before internal edit operations.

  • src/tools/filesystem.ts#L529-L530
  • src/tools/filesystem.ts#L671-L672
📍 Affects 1 file
  • src/tools/filesystem.ts#L529-L530 (this comment)
  • src/tools/filesystem.ts#L671-L672
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/tools/filesystem.ts` around lines 529 - 530, Update both TextDecoder
constructions in src/tools/filesystem.ts at lines 529-530 and 671-672 to use
fatal UTF-16 decoding, so malformed input throws and is handled explicitly
before internal edit operations.

// Strip the BOM character if present at the start
if (decoded.charCodeAt(0) === 0xFEFF) {
decoded = decoded.slice(1);
}

// Apply offset/length in terms of lines, consistent with the text handler
const lines = TextFileHandler.splitLinesPreservingEndings(decoded);
const selectedLines = lines.slice(offset, offset + length);
const content = selectedLines.join('');

return {
content,
mimeType: 'text/plain',
metadata: {}
};
}
} catch (error) {
// If BOM detection fails, fall through to the normal handler path
console.error('UTF-16 BOM detection failed, falling back to default handler:', error);
}

// Read under an abortable timeout so a hung/stalled read is cancelled
// (fd/thread freed) rather than leaked until the OS call returns.
const readOperation = async (signal: AbortSignal) => {
Expand Down Expand Up @@ -610,15 +656,28 @@ export async function readFileInternal(filePath: string, offset: number = 0, len
// preserve exact file content including original line endings.
// We cannot use readline-based reading as it strips line endings.

// Read entire file content preserving line endings, under a 3-minute,
// cancellable timeout so an edit on a stalled/cloud path can't hang forever
// (previously this read had no timeout at all).
const content = await runWithAbortableTimeout(
(signal) => fs.readFile(validPath, { encoding: 'utf8', signal }),
// Detect UTF-16 BOM before reading. PowerShell 5.1 on Windows writes UTF-16 LE
// by default when using redirection (e.g., `echo "hello" > file.txt`).
// Reading such files as UTF-8 would produce garbled text with embedded NUL characters.
const rawBuffer: Buffer = await runWithAbortableTimeout(
(signal) => fs.readFile(validPath, { signal }),
READ_OPERATION_TIMEOUT_MS,
`Internal read for ${filePath}`
);

let content: string;
const utf16Encoding = detectUtf16Bom(rawBuffer);
if (utf16Encoding) {
const decoder = new TextDecoder(utf16Encoding);
content = decoder.decode(rawBuffer);
// Strip the BOM character if present at the start
if (content.charCodeAt(0) === 0xFEFF) {
content = content.slice(1);
}
} else {
content = rawBuffer.toString('utf8');
}

// If we need to apply offset/length, do it while preserving line endings
if (offset === 0 && length >= Number.MAX_SAFE_INTEGER) {
// Most common case for edit operations: read entire file
Expand Down