From d4a5137ac616b19b0ac49302dc57660188106307 Mon Sep 17 00:00:00 2001 From: abose Date: Mon, 17 Aug 2026 10:33:34 +0530 Subject: [PATCH 1/2] fix(document): auto-detect declared/BOM charset instead of forcing UTF-8 Phoenix always force-decoded newly opened files as UTF-8. A file that declares a different charset (e.g. a legacy HTML file with ) or starts with a UTF-16/UTF-32 BOM got silently and irreversibly corrupted - every undecodable byte replaced with U+FFFD - both in the editor and in live preview, which serves the same in-memory text. Add EncodingDetector, wired into DocumentCommandHandlers' file-open flow, that sniffs a file's raw bytes before the first (lossy) read: - Honors a BOM for any non-binary file (previously not detected at all, even for plain .txt files). - Honors a self-declared /Content-Type for markup files, but only when the bytes aren't already valid UTF-8, so a stale/incorrect meta tag on a real UTF-8 file is never second-guessed. - Never overrides a previously user-picked encoding for a path. Adds unit tests (EncodingDetector-test.js) and extends the existing file-encoding-integ-test.js with real fixtures covering both the windows-1252 meta-charset case and the BOM auto-detection gap. --- src/document/DocumentCommandHandlers.js | 33 ++- src/document/EncodingDetector.js | 250 ++++++++++++++++ test/UnitTestSuite.js | 1 + test/spec/EncodingDetector-test.js | 275 ++++++++++++++++++ test/spec/encoding-test-files/README.md | 9 + .../meta-charset-windows1252.html | 10 + test/spec/file-encoding-integ-test.js | 93 ++++++ 7 files changed, 663 insertions(+), 8 deletions(-) create mode 100644 src/document/EncodingDetector.js create mode 100644 test/spec/EncodingDetector-test.js create mode 100644 test/spec/encoding-test-files/meta-charset-windows1252.html diff --git a/src/document/DocumentCommandHandlers.js b/src/document/DocumentCommandHandlers.js index fa50fdfeff..977264aa1e 100644 --- a/src/document/DocumentCommandHandlers.js +++ b/src/document/DocumentCommandHandlers.js @@ -41,6 +41,7 @@ define(function (require, exports, module) { FileUtils = require("file/FileUtils"), FileViewController = require("project/FileViewController"), InMemoryFile = require("document/InMemoryFile"), + EncodingDetector = require("document/EncodingDetector"), StringUtils = require("utils/StringUtils"), Async = require("utils/Async"), Metrics = require("utils/Metrics"), @@ -509,22 +510,38 @@ define(function (require, exports, module) { }); var file = FileSystem.getFileForPath(fullPath); + + function _openFileInPane() { + MainViewManager._open(paneId, file, options) + .done(function () { + result.resolve(file); + }) + .fail(function (fileError) { + _showErrorAndCleanUp(fileError, fullPath); + result.reject(); + }); + } + if (options && options.encoding) { file._encoding = options.encoding; + _openFileInPane(); } else { const encoding = PreferencesManager.getViewState("encoding", PreferencesManager.STATE_PROJECT_CONTEXT); if (encoding && encoding[fullPath]) { file._encoding = encoding[fullPath]; + _openFileInPane(); + } else { + // No explicit or previously chosen encoding for this file - see if it self-declares + // a non-UTF-8 charset (eg a legacy HTML file with ) so we + // don't silently and irreversibly corrupt it by force-decoding as UTF-8. See EncodingDetector. + EncodingDetector.detectFileEncoding(file).then(function (detectedEncoding) { + if (detectedEncoding) { + file._encoding = detectedEncoding; + } + _openFileInPane(); + }); } } - MainViewManager._open(paneId, file, options) - .done(function () { - result.resolve(file); - }) - .fail(function (fileError) { - _showErrorAndCleanUp(fileError, fullPath); - result.reject(); - }); } return result.promise(); diff --git a/src/document/EncodingDetector.js b/src/document/EncodingDetector.js new file mode 100644 index 0000000000..273f6c64ac --- /dev/null +++ b/src/document/EncodingDetector.js @@ -0,0 +1,250 @@ +/* + * GNU AGPL-3.0 License + * + * Copyright (c) 2021 - present core.ai . All rights reserved. + * + * This program is free software: you can redistribute it and/or modify it + * under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License + * for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see https://opensource.org/licenses/AGPL-3.0. + * + */ + +/*global fs*/ + +/** + * Phoenix always decodes newly opened files as UTF-8 by default (see File.js/AppshellFileSystem.js). + * That's correct for the vast majority of files, but some files - most commonly HTML/XML documents + * authored a long time ago, or exported by legacy tools (old FrontPage/Dreamweaver, Windows editors, + * etc) - are actually encoded in a legacy 8-bit charset like `windows-1252`, and self-declare that + * fact via a ``/`Content-Type` tag. Force-decoding such a file as UTF-8 doesn't just + * look wrong - the browser's `TextDecoder("utf8")` is non-fatal, so every undecodable byte is + * silently and *irreversibly* replaced with U+FFFD ("<27>") the moment the file is read. Once that + * happens the original bytes are gone; if the user then saves, the corruption is baked into the file + * on disk too. + * + * This module lets us catch that case before the first (lossy) read ever happens, so newly opened + * files get decoded with the encoding they actually declare - the same behavior a web browser + * exhibits when honoring a page's own declared charset. + * + * Two independent signals are checked, from strongest to weakest: + * 1. A byte-order-mark (BOM) - an unambiguous, extension-independent signal, so it's checked for + * any non-binary file (see detectFileEncoding's use of LanguageManager.isBinary()). This also + * means files like plain .txt with a genuine UTF-16/UTF-32 BOM now get decoded correctly on + * first open too, which - surprisingly - nothing did automatically before this module existed; + * previously that required manually picking the encoding from the status bar dropdown. + * 2. A self-declared charset (``/`Content-Type`) - only meaningful for markup file + * extensions (see SNIFFABLE_EXTENSIONS), and only trusted when the raw bytes are NOT already + * valid UTF-8. If they are, we trust that over any declaration - this avoids second-guessing + * modern UTF-8 files that simply have a stale/incorrect meta tag left over from a copy-paste. + * There is no BOM equivalent for single-byte legacy charsets like windows-1252 - a document + * declaring itself is the only signal there is. + * + * Either way, this is only ever used for a fresh, first-time open. Once a user has explicitly + * picked an encoding for a path (via the status bar dropdown), that choice always wins - see + * DocumentCommandHandlers. + */ +define(function (require, exports, module) { + + + const FileUtils = require("file/FileUtils"), + LanguageManager = require("language/LanguageManager"); + + /** + * File extensions for which we attempt to sniff a self-declared charset. + * @type {Array.} + */ + const SNIFFABLE_EXTENSIONS = ["html", "htm", "xhtml", "shtml", "php", "xml"]; + + // The HTML5 spec only requires user agents to scan the first 1024 bytes of a document for a + // charset declaration before starting to parse it; we use the same limit here. + const SNIFF_BYTE_LIMIT = 1024; + + // Matches both `` and + // `` style declarations. + const META_CHARSET_RE = /]+charset\s*=\s*["']?\s*([a-zA-Z0-9_\-:.]+)/i; + + // Charset aliases that browsers commonly treat as equivalent to a related, better-supported + // name (keyed/valued by the normalized form - see _normalizeEncodingName). Per the WHATWG + // encoding spec, content labeled iso-8859-1 is treated as windows-1252 in practice, since + // windows-1252 is a strict superset (it just assigns printable characters to the 0x80-0x9F + // range that iso-8859-1 leaves as C1 control codes, which real-world content essentially + // never intentionally uses). + const CHARSET_ALIASES = { + "latin1": "windows1252", + "iso88591": "windows1252" + }; + + /** + * @private + * Normalizes a charset name the same way Phoenix's underlying iconv-lite based fs layer does + * when looking up a codec (lower-cased, non-alphanumeric characters stripped) - eg + * "windows-1252" and "Windows_1252" both become "windows1252". `fs.SUPPORTED_ENCODINGS` is + * itself a list of already-normalized names, so declared charsets must be normalized the same + * way before being compared against it or handed back as the encoding to decode with. + * @param {string} name + * @return {string} + */ + function _normalizeEncodingName(name) { + return name.toLowerCase().replace(/[^0-9a-z]/g, ""); + } + + const BOM_SIGNATURES = [ + {bytes: [0xEF, 0xBB, 0xBF], encoding: "utf8"}, + {bytes: [0xFF, 0xFE, 0x00, 0x00], encoding: "utf32le"}, + {bytes: [0x00, 0x00, 0xFE, 0xFF], encoding: "utf32be"}, + {bytes: [0xFF, 0xFE], encoding: "utf16le"}, + {bytes: [0xFE, 0xFF], encoding: "utf16be"} + ]; + + /** + * @private + * Returns the encoding named by a recognized byte-order-mark at the start of `bytes`, or null. + * @param {Uint8Array} bytes + * @return {?string} + */ + function _detectBOM(bytes) { + for (const sig of BOM_SIGNATURES) { + if (bytes.length >= sig.bytes.length && sig.bytes.every(function (b, i) { + return bytes[i] === b; + })) { + return sig.encoding; + } + } + return null; + } + + /** + * @private + * Extracts a declared charset name from a `` tag, if present in the given bytes. The + * declaration itself is always plain ASCII per spec, so it's safe to scan for it by treating + * the raw bytes as Latin-1/ASCII regardless of the file's real encoding. + * @param {Uint8Array} bytes + * @return {?string} lower-cased charset name, or null if none found + */ + function _extractDeclaredCharset(bytes) { + const prefix = bytes.subarray(0, Math.min(bytes.length, SNIFF_BYTE_LIMIT)); + let asciiText = ""; + for (let i = 0; i < prefix.length; i++) { + asciiText += String.fromCharCode(prefix[i]); + } + const match = META_CHARSET_RE.exec(asciiText); + return match ? match[1].toLowerCase() : null; + } + + /** + * @private + * @param {Uint8Array} bytes + * @return {boolean} true if `bytes` is well-formed UTF-8 + */ + function _isValidUTF8(bytes) { + try { + new TextDecoder("utf8", {fatal: true}).decode(bytes); + return true; + } catch (e) { + return false; + } + } + + /** + * Given the raw bytes of a file and its extension, determine whether a non-default encoding + * should be used to decode it. Pure/synchronous - does no I/O. + * + * @param {string} extension lower-case file extension, no leading dot + * @param {Uint8Array} bytes raw file content + * @param {Array.=} supportedEncodings encoding names Phoenix's fs layer can decode with; + * defaults to `fs.SUPPORTED_ENCODINGS` when running in the app. + * @return {?string} the encoding name to use, or null to keep the default (utf8) + */ + function detectEncodingFromBytes(extension, bytes, supportedEncodings) { + if (!bytes || !bytes.length) { + return null; + } + + const bom = _detectBOM(bytes); + if (bom) { + // An explicit BOM is unambiguous. A utf-8 BOM just means "definitely utf-8", which is + // already our default, so nothing to override there. + return bom === "utf8" ? null : bom; + } + + if (SNIFFABLE_EXTENSIONS.indexOf(extension) === -1) { + return null; + } + + if (_isValidUTF8(bytes)) { + return null; + } + + const rawDeclared = _extractDeclaredCharset(bytes); + if (!rawDeclared) { + return null; + } + + let declared = _normalizeEncodingName(rawDeclared); + declared = CHARSET_ALIASES[declared] || declared; + + if (declared === "utf8") { + // Declared utf-8 but isn't valid utf-8 bytes - nothing sane we can substitute, so we + // just keep decoding as utf-8 (matching today's behavior) rather than guessing further. + return null; + } + + supportedEncodings = supportedEncodings || (typeof fs !== "undefined" && fs.SUPPORTED_ENCODINGS); + if (supportedEncodings && supportedEncodings.indexOf(declared) === -1) { + return null; + } + + return declared; + } + + /** + * Attempts to detect a non-default encoding for `file` before it's opened for the first time, + * by reading its raw bytes and looking for a BOM or (for markup files) a self-declared charset + * (see detectEncodingFromBytes). Never rejects - resolves with null if detection isn't + * applicable to this file, or nothing conclusive was found, in which case the caller should + * just fall back to the normal default (utf8) decode. + * + * We only skip reading the file at all when it's a known binary type (image, font, zip, etc) - + * same check the rest of the app uses (LanguageManager's isBinary()) to decide whether a file + * should ever be treated as text. Anything else is fair game for a BOM, even if its extension + * isn't one we scan for a `` declaration (see SNIFFABLE_EXTENSIONS) - a BOM is a + * cheap, unambiguous signal that doesn't depend on file type the way a meta tag scan does. + * + * @param {File} file + * @return {$.Promise} resolved with the detected encoding name, or null + */ + function detectFileEncoding(file) { + const result = new $.Deferred(); + const language = LanguageManager.getLanguageForPath(file.fullPath); + + if (language.isBinary()) { + result.resolve(null); + return result.promise(); + } + + const extension = FileUtils.getFileExtension(file.fullPath).toLowerCase(); + file.read({encoding: window.fs.BYTE_ARRAY_ENCODING, doNotCache: true}, function (err, content) { + if (err || !content) { + result.resolve(null); + return; + } + const bytes = new Uint8Array(content); + result.resolve(detectEncodingFromBytes(extension, bytes, window.fs.SUPPORTED_ENCODINGS)); + }); + + return result.promise(); + } + + exports.SNIFFABLE_EXTENSIONS = SNIFFABLE_EXTENSIONS; + exports.detectEncodingFromBytes = detectEncodingFromBytes; + exports.detectFileEncoding = detectFileEncoding; +}); diff --git a/test/UnitTestSuite.js b/test/UnitTestSuite.js index e0e8405aa2..3827d7ccd2 100644 --- a/test/UnitTestSuite.js +++ b/test/UnitTestSuite.js @@ -37,6 +37,7 @@ define(function (require, exports, module) { require("spec/EditorCommandHandlers-test"); require("spec/EditorCommandHandlers-integ-test"); require("spec/EditorManager-test"); + require("spec/EncodingDetector-test"); require("spec/EventDispatcher-test"); require("spec/EventManager-test"); require("spec/ExtensionInterface-test"); diff --git a/test/spec/EncodingDetector-test.js b/test/spec/EncodingDetector-test.js new file mode 100644 index 0000000000..58d6df29d5 --- /dev/null +++ b/test/spec/EncodingDetector-test.js @@ -0,0 +1,275 @@ +/* + * GNU AGPL-3.0 License + * + * Copyright (c) 2021 - present core.ai . All rights reserved. + * + * This program is free software: you can redistribute it and/or modify it + * under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License + * for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see https://opensource.org/licenses/AGPL-3.0. + * + */ + +/*global describe, it, expect, awaitsForDone */ +/*unittests: EncodingDetector*/ + +define(function (require, exports, module) { + + + const EncodingDetector = require("document/EncodingDetector"); + + // Builds a Uint8Array where each char code (0-255) becomes exactly one byte. Lets tests + // express arbitrary single-byte-encoded content (eg windows-1252) as a plain JS string + // without needing a real encoder: any char code above 0x7F is used as the raw byte value, + // which is exactly what windows-1252/latin1 do for the accented letters used below. + function bytesFromLatin1(str) { + const arr = new Uint8Array(str.length); + for (let i = 0; i < str.length; i++) { + // eslint-disable-next-line no-bitwise + arr[i] = str.charCodeAt(i) & 0xFF; + } + return arr; + } + + function bytesFromUTF8(str) { + return new TextEncoder().encode(str); + } + + // 0xE9 is "é" in both windows-1252 and latin1, but is not a valid standalone UTF-8 byte. + const WIN1252_E_ACUTE = String.fromCharCode(0xE9); + + // matches the normalized (non-alphanumeric-stripped) form Phoenix's fs.SUPPORTED_ENCODINGS uses + const SUPPORTED = ["utf8", "windows1252", "iso88591", "utf16le", "utf16be", "utf32le", "utf32be"]; + + describe("EncodingDetector", function () { + + describe("detectEncodingFromBytes", function () { + + it("should return null for empty/missing bytes", function () { + expect(EncodingDetector.detectEncodingFromBytes("html", new Uint8Array(0), SUPPORTED)).toBeNull(); + expect(EncodingDetector.detectEncodingFromBytes("html", null, SUPPORTED)).toBeNull(); + }); + + it("should ignore non-sniffable extensions even with a declared charset", function () { + const bytes = bytesFromLatin1( + ' caf' + WIN1252_E_ACUTE + ); + expect(EncodingDetector.detectEncodingFromBytes("js", bytes, SUPPORTED)).toBeNull(); + expect(EncodingDetector.detectEncodingFromBytes("txt", bytes, SUPPORTED)).toBeNull(); + }); + + it("should detect windows-1252 from a short-form tag", function () { + const bytes = bytesFromLatin1( + 'caf' + + WIN1252_E_ACUTE + '' + ); + expect(EncodingDetector.detectEncodingFromBytes("html", bytes, SUPPORTED)).toBe("windows1252"); + }); + + it("should detect windows-1252 from a Content-Type http-equiv meta tag", function () { + // this is exactly the shape FrontPage/legacy authoring tools emit, and what the + // original bug report's file used. + const bytes = bytesFromLatin1( + '' + + 'supermarch' + WIN1252_E_ACUTE + ); + expect(EncodingDetector.detectEncodingFromBytes("html", bytes, SUPPORTED)).toBe("windows1252"); + }); + + it("should alias iso-8859-1 and latin1 declarations to windows-1252", function () { + const isoBytes = bytesFromLatin1( + 'caf' + WIN1252_E_ACUTE + ); + const latin1Bytes = bytesFromLatin1( + 'caf' + WIN1252_E_ACUTE + ); + expect(EncodingDetector.detectEncodingFromBytes("html", isoBytes, SUPPORTED)).toBe("windows1252"); + expect(EncodingDetector.detectEncodingFromBytes("html", latin1Bytes, SUPPORTED)).toBe("windows1252"); + }); + + it("should trust valid UTF-8 content over a stale/incorrect meta declaration", function () { + // declares windows-1252, but the bytes are actually well-formed utf-8 - keep utf-8. + const bytes = bytesFromUTF8('café supermarché'); + expect(EncodingDetector.detectEncodingFromBytes("html", bytes, SUPPORTED)).toBeNull(); + }); + + it("should return null when the declared charset is utf-8 but the bytes aren't valid utf-8", function () { + const bytes = bytesFromLatin1( + 'caf' + WIN1252_E_ACUTE + ); + expect(EncodingDetector.detectEncodingFromBytes("html", bytes, SUPPORTED)).toBeNull(); + }); + + it("should return null when no charset is declared at all", function () { + const bytes = bytesFromLatin1('caf' + WIN1252_E_ACUTE + ''); + expect(EncodingDetector.detectEncodingFromBytes("html", bytes, SUPPORTED)).toBeNull(); + }); + + it("should return null when the declared charset isn't in the supported encodings list", function () { + const bytes = bytesFromLatin1( + 'caf' + WIN1252_E_ACUTE + ); + expect(EncodingDetector.detectEncodingFromBytes("html", bytes, SUPPORTED)).toBeNull(); + }); + + it("should fall back to the global fs.SUPPORTED_ENCODINGS when none is passed in", function () { + const bytes = bytesFromLatin1( + 'caf' + WIN1252_E_ACUTE + ); + // running inside the real Phoenix shell, `fs.SUPPORTED_ENCODINGS` genuinely lists + // windows-1252 (backed by the bundled iconv-lite codec table). + expect(EncodingDetector.detectEncodingFromBytes("html", bytes)).toBe("windows1252"); + }); + + it("should honor a BOM even on a non-sniffable extension, and take precedence over any meta tag", function () { + const utf16leBOM = new Uint8Array([0xFF, 0xFE, 0x61, 0x00]); // BOM + 'a' + expect(EncodingDetector.detectEncodingFromBytes("txt", utf16leBOM, SUPPORTED)).toBe("utf16le"); + + const utf16beBOM = new Uint8Array([0xFE, 0xFF, 0x00, 0x61]); + expect(EncodingDetector.detectEncodingFromBytes("html", utf16beBOM, SUPPORTED)).toBe("utf16be"); + + const utf32leBOM = new Uint8Array([0xFF, 0xFE, 0x00, 0x00, 0x61, 0x00, 0x00, 0x00]); + expect(EncodingDetector.detectEncodingFromBytes("txt", utf32leBOM, SUPPORTED)).toBe("utf32le"); + + const utf32beBOM = new Uint8Array([0x00, 0x00, 0xFE, 0xFF, 0x00, 0x00, 0x00, 0x61]); + expect(EncodingDetector.detectEncodingFromBytes("txt", utf32beBOM, SUPPORTED)).toBe("utf32be"); + }); + + it("should return null for a utf-8 BOM since that's already the default", function () { + const utf8BOM = new Uint8Array([0xEF, 0xBB, 0xBF, 0x61]); + expect(EncodingDetector.detectEncodingFromBytes("html", utf8BOM, SUPPORTED)).toBeNull(); + }); + + }); + + describe("detectFileEncoding", function () { + + function makeMockFile(fullPath, readResult) { + let readCalled = false; + return { + fullPath: fullPath, + _readCalled: function () { + return readCalled; + }, + read: function (options, callback) { + readCalled = true; + if (readResult.err) { + callback(readResult.err); + } else { + callback(null, readResult.content); + } + } + }; + } + + it("should resolve the detected encoding for a sniffable file with a declared charset", async function () { + const bytes = bytesFromLatin1( + 'caf' + + WIN1252_E_ACUTE + ); + const file = makeMockFile("/proj/eu_format_test.html", {content: bytes.buffer}); + let detected; + await awaitsForDone( + EncodingDetector.detectFileEncoding(file).done(function (result) { + detected = result; + }) + ); + expect(detected).toBe("windows1252"); + }); + + it("should resolve null without reading the file for a binary extension", async function () { + const file = makeMockFile("/proj/photo.png", {content: new ArrayBuffer(0)}); + let detected = "unset"; + await awaitsForDone( + EncodingDetector.detectFileEncoding(file).done(function (result) { + detected = result; + }) + ); + expect(detected).toBeNull(); + expect(file._readCalled()).toBe(false); + }); + + it("should still detect a BOM on a non-markup (but non-binary) extension like .txt/.js", async function () { + // Regression test: a BOM is unambiguous and extension-independent, unlike the + // scan which only makes sense for markup files - a plain .txt or .js + // file with a real UTF-16 BOM must still be auto-detected correctly. + const utf16beBOM = new Uint8Array([0xFE, 0xFF, 0x00, 0x61]); + const txtFile = makeMockFile("/proj/notes.txt", {content: utf16beBOM.buffer}); + const jsFile = makeMockFile("/proj/script.js", {content: utf16beBOM.buffer}); + + let txtDetected, jsDetected; + await awaitsForDone( + EncodingDetector.detectFileEncoding(txtFile).done(function (result) { + txtDetected = result; + }) + ); + await awaitsForDone( + EncodingDetector.detectFileEncoding(jsFile).done(function (result) { + jsDetected = result; + }) + ); + + expect(txtDetected).toBe("utf16be"); + expect(jsDetected).toBe("utf16be"); + expect(txtFile._readCalled()).toBe(true); + expect(jsFile._readCalled()).toBe(true); + }); + + it("should NOT scan for a declaration on a non-markup extension", async function () { + // a declared charset only makes sense for markup files - a .js/.txt file that + // happens to contain a "charset=windows-1252"-looking string (eg inside a comment + // or string literal) must not be reinterpreted based on it. + const bytes = bytesFromLatin1( + '// not real markup, just a comment: caf' + + WIN1252_E_ACUTE + ); + const file = makeMockFile("/proj/script.js", {content: bytes.buffer}); + let detected; + await awaitsForDone( + EncodingDetector.detectFileEncoding(file).done(function (result) { + detected = result; + }) + ); + expect(detected).toBeNull(); + expect(file._readCalled()).toBe(true); + }); + + it("should resolve null (not reject) when the file read fails", async function () { + const file = makeMockFile("/proj/broken.html", {err: "NotFound"}); + let detected = "unset", rejected = false; + await awaitsForDone( + EncodingDetector.detectFileEncoding(file) + .done(function (result) { + detected = result; + }) + .fail(function () { + rejected = true; + }) + ); + expect(detected).toBeNull(); + expect(rejected).toBe(false); + }); + + it("should resolve null for well-formed utf-8 html regardless of a stale meta charset", async function () { + const bytes = bytesFromUTF8('café'); + const file = makeMockFile("/proj/modern.html", {content: bytes.buffer}); + let detected; + await awaitsForDone( + EncodingDetector.detectFileEncoding(file).done(function (result) { + detected = result; + }) + ); + expect(detected).toBeNull(); + }); + + }); + }); +}); diff --git a/test/spec/encoding-test-files/README.md b/test/spec/encoding-test-files/README.md index d58ad89879..92cc262c5d 100644 --- a/test/spec/encoding-test-files/README.md +++ b/test/spec/encoding-test-files/README.md @@ -19,3 +19,12 @@ Only UTF8 and UTF16 is able to do all encodings. TODO: Add BOM test files + +## meta-charset-windows1252.html + +A separate fixture, unrelated to `generate.py` above. It's a legacy-style HTML file that +self-declares `charset=windows-1252` via a `` tag and is +genuinely saved in that charset, used to test that Phoenix auto-detects and honors the +declaration instead of always defaulting to UTF-8 (see `src/document/EncodingDetector.js` and +`file-encoding-integ-test.js`). It's a static checked-in fixture - edit it directly with a +windows-1252-aware tool (or `iconv -f UTF-8 -t WINDOWS-1252`) if it ever needs to change. diff --git a/test/spec/encoding-test-files/meta-charset-windows1252.html b/test/spec/encoding-test-files/meta-charset-windows1252.html new file mode 100644 index 0000000000..f5ddae0801 --- /dev/null +++ b/test/spec/encoding-test-files/meta-charset-windows1252.html @@ -0,0 +1,10 @@ + + + + +meta charset test + + +café supermarché – été + + diff --git a/test/spec/file-encoding-integ-test.js b/test/spec/file-encoding-integ-test.js index cc8bcd1f41..d13374948b 100644 --- a/test/spec/file-encoding-integ-test.js +++ b/test/spec/file-encoding-integ-test.js @@ -31,6 +31,9 @@ define(function (require, exports, module) { let FileViewController, // loaded from brackets.test, EditorManager, + DocumentManager, + PreferencesManager, + CommandManager, testWindow, brackets; @@ -44,6 +47,9 @@ define(function (require, exports, module) { brackets = testWindow.brackets; FileViewController = brackets.test.FileViewController; EditorManager = brackets.test.EditorManager; + DocumentManager = brackets.test.DocumentManager; + PreferencesManager = brackets.test.PreferencesManager; + CommandManager = brackets.test.CommandManager; await SpecRunnerUtils.loadProjectInTestWindow(testPath); }, 30000); @@ -51,6 +57,9 @@ define(function (require, exports, module) { afterAll(async function () { FileViewController = null; EditorManager = null; + DocumentManager = null; + PreferencesManager = null; + CommandManager = null; testWindow = null; brackets = null; // comment out below line if you want to debug the test window post running tests @@ -94,6 +103,69 @@ define(function (require, exports, module) { }, `${encoding} text`); } + async function verifyAutoDetectedEncoding(fileName, expectedEncoding, expectedText) { + const path = testPath + `/${fileName}`; + + // Make sure there's no leftover manually-picked encoding preference for this path from + // a previous test/run - we want a genuinely fresh, undetected-until-now open here. + const encodingPrefs = PreferencesManager.getViewState("encoding", PreferencesManager.STATE_PROJECT_CONTEXT) || {}; + delete encodingPrefs[path]; + PreferencesManager.setViewState("encoding", encodingPrefs, PreferencesManager.STATE_PROJECT_CONTEXT); + + const openDoc = DocumentManager.getOpenDocumentForPath(path); + if (openDoc) { + await awaitsForDone(CommandManager.execute("file.close", {file: openDoc.file, _forceClose: true})); + } + + // No dropdown/manual encoding selection here - this is the whole point of the test. + await awaitsForDone( + FileViewController.openAndSelectDocument(path, FileViewController.PROJECT_MANAGER)); + + await awaitsFor(function () { + return EditorManager.getActiveEditor().document.getText() === expectedText; + }, `${fileName} auto-detected as ${expectedEncoding}`); + + expect(EditorManager.getActiveEditor().document.file._encoding).toBe(expectedEncoding); + } + + it("Should auto-detect a utf16 BOM on first open, with no manual encoding selection", async function () { + // Regression test: BOM detection used to only kick in for markup file extensions + // (html/htm/xhtml/...), so a plain .txt file with a real UTF-16 BOM stayed + // force-decoded as UTF-8 (and garbled) until the user manually picked the encoding via + // the status bar dropdown, same as koi8r.txt below. See EncodingDetector.js. + await verifyAutoDetectedEncoding("utf16.txt", "utf16le", EXPECTED_TEXT_UTF16); + }); + + it("Should auto-detect a utf32le BOM on first open, with no manual encoding selection", async function () { + await verifyAutoDetectedEncoding("utf32le.txt", "utf32le", EXPECTED_TEXT_UTF16); + }); + + it("Should auto-detect a utf32be BOM on first open, with no manual encoding selection", async function () { + await verifyAutoDetectedEncoding("utf32be.txt", "utf32be", EXPECTED_TEXT_UTF16); + }); + + it("Should NOT auto-detect koi8r.txt, since single-byte legacy charsets have no BOM to detect", async function () { + // Unlike utf16/utf32, koi8r has no byte-order-mark and koi8r.txt is plain text (not + // markup with a to declare itself), so there's no signal for Phoenix to + // detect at all here - it stays on the utf8 default until the user manually picks the + // encoding (see "Should open file in koi8r encoding" below). This documents that + // boundary rather than asserting a real capability. + const path = testPath + "/koi8r.txt"; + const encodingPrefs = PreferencesManager.getViewState("encoding", PreferencesManager.STATE_PROJECT_CONTEXT) || {}; + delete encodingPrefs[path]; + PreferencesManager.setViewState("encoding", encodingPrefs, PreferencesManager.STATE_PROJECT_CONTEXT); + + const openDoc = DocumentManager.getOpenDocumentForPath(path); + if (openDoc) { + await awaitsForDone(CommandManager.execute("file.close", {file: openDoc.file, _forceClose: true})); + } + + await awaitsForDone( + FileViewController.openAndSelectDocument(path, FileViewController.PROJECT_MANAGER)); + + expect(EditorManager.getActiveEditor().document.file._encoding).toBe("utf8"); + }); + it("Should open file in utf 16 encoding", async function () { await verifyOpenEncoding("utf16", EXPECTED_TEXT_UTF16); }); @@ -109,5 +181,26 @@ define(function (require, exports, module) { it("Should open file in utf32be encoding", async function () { await verifyOpenEncoding("utf32be", EXPECTED_TEXT_UTF16); }); + + it("Should auto-detect a self-declared windows-1252 charset in an HTML file on first open", async function () { + // Regression test: a legacy HTML file that declares charset=windows-1252 via a + // tag, and is genuinely saved in that charset, used + // to always be force-decoded as UTF-8 on open, silently and irreversibly replacing + // every accented character with U+FFFD. See EncodingDetector.js. + await awaitsForDone( + FileViewController.openAndSelectDocument( + testPath + "/meta-charset-windows1252.html", + FileViewController.PROJECT_MANAGER + )); + + await awaitsFor(function () { + const text = EditorManager.getActiveEditor().document.getText(); + return text.indexOf("café supermarché") !== -1; + }, "windows-1252 html auto-detected", 5000); + + const text = EditorManager.getActiveEditor().document.getText(); + expect(text.indexOf("�")).toBe(-1); + expect(EditorManager.getActiveEditor().document.file._encoding).toBe("windows1252"); + }); }); }); From 7ff2e16c590ada2559c5a4bbb1689f68440d242f Mon Sep 17 00:00:00 2001 From: abose Date: Mon, 17 Aug 2026 10:51:00 +0530 Subject: [PATCH 2/2] fix(document): fix encoding cache poisoning and skip redundant re-detection Two follow-up correctness fixes for the encoding auto-detection added in the previous commit, plus the perf optimization that surfaced them: - Skip re-running EncodingDetector when a file's encoding is already known (File instances are cached/reused per path for the session), so reopening a tab doesn't re-read + re-detect every time. - That skip-check needed to be more careful than a plain truthiness check: several unrelated features (ProjectManager's "Download" command, AIChatPanel's image-attach, MediaViewer, etc.) read a File instance with {encoding: fs.BYTE_ARRAY_ENCODING} and no doNotCache for their own non-text purposes - which, per File.read()'s own caching, leaves that sentinel value cached in file._encoding even though the file was never opened as a document. Added EncodingDetector.isKnownTextEncoding() to tell a real text codec apart from that sentinel (notably still present in fs.SUPPORTED_ENCODINGS, so that list alone can't distinguish them). - Found a deeper, pre-existing bug while writing the regression test for the above: File.read()'s cache-hit check only compares options.encoding to this._encoding - it doesn't invalidate _contents/_stat when something bare-reassigns file._encoding (which _doOpen has always done for the explicit-option and stored-preference branches too, not just detection). So: a prior raw-byte read followed by reassigning _encoding to the correct detected value could make the next real open cache-hit on stale raw bytes instead of re-reading as text. Fixed via a _setFileEncoding() helper that clears the stale cache whenever the encoding actually changes. Extends EncodingDetector-test.js and file-encoding-integ-test.js with regression coverage for both. --- src/document/DocumentCommandHandlers.js | 44 ++++++++++++++++++----- src/document/EncodingDetector.js | 18 ++++++++++ test/spec/EncodingDetector-test.js | 28 +++++++++++++++ test/spec/file-encoding-integ-test.js | 47 +++++++++++++++++++++++++ 4 files changed, 129 insertions(+), 8 deletions(-) diff --git a/src/document/DocumentCommandHandlers.js b/src/document/DocumentCommandHandlers.js index 977264aa1e..3a418a63e9 100644 --- a/src/document/DocumentCommandHandlers.js +++ b/src/document/DocumentCommandHandlers.js @@ -522,22 +522,50 @@ define(function (require, exports, module) { }); } + // File.read() caches _contents/_stat keyed together with whatever _encoding was in + // effect at the time of that read (see File.js). Bare-reassigning file._encoding + // without invalidating that cache would let the imminent real open - which reads with + // this same newly-assigned encoding - incorrectly cache-hit and hand back stale + // content cached under the OLD encoding (worse, raw bytes, if that prior read used a + // byte-array encoding, eg via the "Download" command or an image-attach feature) + // instead of doing a real re-read. Always route encoding changes through here so that + // can never happen. + function _setFileEncoding(newEncoding) { + if (file._encoding !== newEncoding) { + file._clearCachedData(); + file._encoding = newEncoding; + } + } + if (options && options.encoding) { - file._encoding = options.encoding; + _setFileEncoding(options.encoding); _openFileInPane(); } else { const encoding = PreferencesManager.getViewState("encoding", PreferencesManager.STATE_PROJECT_CONTEXT); if (encoding && encoding[fullPath]) { - file._encoding = encoding[fullPath]; + _setFileEncoding(encoding[fullPath]); + _openFileInPane(); + } else if (EncodingDetector.isKnownTextEncoding(file._encoding)) { + // File instances are cached/reused per path for the session (FileSystem._index), + // so a known-text _encoding here means we've already read (and so already + // detected or defaulted) this exact file once before as text - eg it's being + // reopened after a close. Re-running detection would mean re-reading the whole + // file from disk a second time for no new information, so just reuse what we + // already know. (isKnownTextEncoding - rather than a plain truthiness check - + // matters here: other code paths read this same File instance for non-text + // reasons, eg downloading it or attaching it as a chat image, and can leave a + // non-text sentinel encoding cached on it even though it was never opened as a + // document - we must not mistake that for "already detected".) _openFileInPane(); } else { - // No explicit or previously chosen encoding for this file - see if it self-declares - // a non-UTF-8 charset (eg a legacy HTML file with ) so we - // don't silently and irreversibly corrupt it by force-decoding as UTF-8. See EncodingDetector. + // No explicit, previously chosen, or already-known encoding for this file - see if + // it self-declares a non-UTF-8 charset (eg a legacy HTML file with + // ) so we don't silently and irreversibly corrupt it + // by force-decoding as UTF-8. See EncodingDetector. EncodingDetector.detectFileEncoding(file).then(function (detectedEncoding) { - if (detectedEncoding) { - file._encoding = detectedEncoding; - } + // Always land on a definite, known-text value - never leave file._encoding as + // whatever a prior non-text read (see above) may have left it as. + _setFileEncoding(detectedEncoding || "utf8"); _openFileInPane(); }); } diff --git a/src/document/EncodingDetector.js b/src/document/EncodingDetector.js index 273f6c64ac..6761e4ae7d 100644 --- a/src/document/EncodingDetector.js +++ b/src/document/EncodingDetector.js @@ -244,7 +244,25 @@ define(function (require, exports, module) { return result.promise(); } + /** + * True if `encoding` is a real text codec name, as opposed to the non-text sentinel value + * (`fs.BYTE_ARRAY_ENCODING`, i.e. "byte_array" - notably still present in + * `fs.SUPPORTED_ENCODINGS`, so that list alone can't be used to tell them apart) that plenty of + * *other* call sites across the codebase pass to `File.read()` for legitimate non-text reasons + * (downloading a file, attaching an image, exporting a zip, etc) - and, unless they also pass + * `doNotCache: true`, leave cached in `file._encoding` as a side effect of File.read()'s + * caching (see File.js). A File instance touched that way before ever being opened as a + * document would otherwise look "already known" to a naive truthiness check, silently + * defeating both detection and the re-open-skip optimization in DocumentCommandHandlers. + * @param {?string} encoding + * @return {boolean} + */ + function isKnownTextEncoding(encoding) { + return !!encoding && encoding !== window.fs.BYTE_ARRAY_ENCODING; + } + exports.SNIFFABLE_EXTENSIONS = SNIFFABLE_EXTENSIONS; exports.detectEncodingFromBytes = detectEncodingFromBytes; exports.detectFileEncoding = detectFileEncoding; + exports.isKnownTextEncoding = isKnownTextEncoding; }); diff --git a/test/spec/EncodingDetector-test.js b/test/spec/EncodingDetector-test.js index 58d6df29d5..919d326dea 100644 --- a/test/spec/EncodingDetector-test.js +++ b/test/spec/EncodingDetector-test.js @@ -271,5 +271,33 @@ define(function (require, exports, module) { }); }); + + describe("isKnownTextEncoding", function () { + + it("should reject falsy values", function () { + expect(EncodingDetector.isKnownTextEncoding(null)).toBe(false); + expect(EncodingDetector.isKnownTextEncoding(undefined)).toBe(false); + expect(EncodingDetector.isKnownTextEncoding("")).toBe(false); + }); + + it("should reject the byte-array sentinel used for non-text reads", function () { + // Regression test: other code paths (download-file, attach-image-to-chat, etc) + // read a File instance with {encoding: fs.BYTE_ARRAY_ENCODING} and no + // doNotCache, which - per File.read()'s own caching - leaves that sentinel value + // cached in file._encoding even though the file was never opened as text. Note + // fs.BYTE_ARRAY_ENCODING is deliberately included in fs.SUPPORTED_ENCODINGS (so + // reads can request it), which is exactly why this needs its own explicit check + // rather than relying on SUPPORTED_ENCODINGS membership. + expect(EncodingDetector.isKnownTextEncoding(window.fs.BYTE_ARRAY_ENCODING)).toBe(false); + expect(window.fs.SUPPORTED_ENCODINGS.indexOf(window.fs.BYTE_ARRAY_ENCODING)).not.toBe(-1); + }); + + it("should accept real text codec names", function () { + expect(EncodingDetector.isKnownTextEncoding("utf8")).toBe(true); + expect(EncodingDetector.isKnownTextEncoding("windows1252")).toBe(true); + expect(EncodingDetector.isKnownTextEncoding("utf16le")).toBe(true); + }); + + }); }); }); diff --git a/test/spec/file-encoding-integ-test.js b/test/spec/file-encoding-integ-test.js index d13374948b..8a17a7436f 100644 --- a/test/spec/file-encoding-integ-test.js +++ b/test/spec/file-encoding-integ-test.js @@ -34,6 +34,7 @@ define(function (require, exports, module) { DocumentManager, PreferencesManager, CommandManager, + FileSystem, testWindow, brackets; @@ -50,6 +51,7 @@ define(function (require, exports, module) { DocumentManager = brackets.test.DocumentManager; PreferencesManager = brackets.test.PreferencesManager; CommandManager = brackets.test.CommandManager; + FileSystem = brackets.test.FileSystem; await SpecRunnerUtils.loadProjectInTestWindow(testPath); }, 30000); @@ -60,6 +62,7 @@ define(function (require, exports, module) { DocumentManager = null; PreferencesManager = null; CommandManager = null; + FileSystem = null; testWindow = null; brackets = null; // comment out below line if you want to debug the test window post running tests @@ -202,5 +205,49 @@ define(function (require, exports, module) { expect(text.indexOf("�")).toBe(-1); expect(EditorManager.getActiveEditor().document.file._encoding).toBe("windows1252"); }); + + it("Should still auto-detect correctly even if the file was previously read as raw bytes (eg via Download)", async function () { + // Regression test: several unrelated features (the project tree's "Download" command, + // attaching a file as a chat image, etc) read a File instance with + // {encoding: fs.BYTE_ARRAY_ENCODING} for their own non-text purposes, and - since they + // don't pass doNotCache - that read leaves the non-text "byte_array" sentinel cached in + // file._encoding as a side effect of File.read()'s own caching (see File.js), even + // though the file was never opened as a document. If a file gets touched that way + // *before* it's ever opened, detection/open logic must not mistake that sentinel for an + // already-known real encoding - see EncodingDetector.isKnownTextEncoding and its use in + // DocumentCommandHandlers. + const path = testPath + "/meta-charset-windows1252.html"; + + const encodingPrefs = PreferencesManager.getViewState("encoding", PreferencesManager.STATE_PROJECT_CONTEXT) || {}; + delete encodingPrefs[path]; + PreferencesManager.setViewState("encoding", encodingPrefs, PreferencesManager.STATE_PROJECT_CONTEXT); + + const openDoc = DocumentManager.getOpenDocumentForPath(path); + if (openDoc) { + await awaitsForDone(CommandManager.execute("file.close", {file: openDoc.file, _forceClose: true})); + } + + // simulate the "Download" command's raw-byte read, BEFORE this file is ever opened as + // a document - this is what poisons file._encoding with the non-text sentinel. + const file = FileSystem.getFileForPath(path); + await new Promise(function (resolve, reject) { + file.read({encoding: testWindow.fs.BYTE_ARRAY_ENCODING}, function (err) { + err ? reject(err) : resolve(); + }); + }); + expect(file._encoding).toBe(testWindow.fs.BYTE_ARRAY_ENCODING); + + await awaitsForDone( + FileViewController.openAndSelectDocument(path, FileViewController.PROJECT_MANAGER)); + + await awaitsFor(function () { + const text = EditorManager.getActiveEditor().document.getText(); + return text.indexOf("café supermarché") !== -1; + }, "windows-1252 html auto-detected despite prior raw-byte read", 5000); + + const text = EditorManager.getActiveEditor().document.getText(); + expect(text.indexOf("�")).toBe(-1); + expect(EditorManager.getActiveEditor().document.file._encoding).toBe("windows1252"); + }); }); });