fix(xml): XmlDocument::Load(filename) rejects an undeclared entity too (#2361)

#2082 rejects an undeclared entity reference by scanning the RAW text, because
tinyxml2 decodes the five predefined entities during parsing and "&nope;" and
"&nope;" are indistinguishable afterwards. LoadXml(string) has the raw text and ran
the check; Load(filename) handed the path straight to tinyxml2::LoadFile and never
held the bytes, so it did not.

So the identical document got two answers depending on which door it came through
-- and the file door's answer LOST DATA. Accepting the reference was recoverable;
reinterpreting it as literal text AND re-escaping it meant that loading and saving
a document changed the document, with no diagnostic anywhere.

Load now reads the file itself and parses the buffer, which is what
tinyxml2::LoadFile does internally anyway (read whole file, call Parse). The read
is BINARY: a text-mode read on Windows would collapse CRLF and move the offsets the
scanner walks.

THE FAILURE PATH DELIBERATELY STILL GOES THROUGH LoadFile. It is what produces
XML_ERROR_FILE_NOT_FOUND / FILE_COULD_NOT_BE_OPENED / FILE_READ_ERROR and the
ErrorStr() this exception message has always carried. Reproducing those categories
from an ifstream failure would be inventing diagnostics rather than keeping them,
so when the read fails the code lets tinyxml2 categorise its own failure and throws
the unchanged message.

TWO MUTATIONS ARE NOT MUTATIONS, and are recorded rather than counted as passes.

  * Text mode instead of binary is a no-op ON THIS PLATFORM. Linux makes the two
    modes identical, so the mutation is unobservable here by construction; the flag
    is a correctness measure for a platform the gate does not run on.

  * Parse(c_str()) without the length is SEMANTICALLY EQUIVALENT, and that was
    verified by probe rather than assumed. Three NUL placements were measured
    through both doors -- NUL mid-document, NUL after a complete document, and NUL
    followed by more markup containing an undeclared entity -- and all six results
    agree, because tinyxml2 stops at the NUL either way. The explicit length is kept
    because it is the clearer spelling, not because a test defends it. The probe
    binaries were deleted afterwards, per the build-resource policy.

Three real mutations caught: drop the entity check from Load; treat an unreadable
file as empty content; ReadWholeFile keeps only the first line.

Two cases added (Xml 507 -> 509), including the door-equivalence property itself
and an invariance case covering a legal document, a missing file, malformed
content, and the undeclared-PREFIX check (#2083) that always ran at this door.

Downstream, measured per SA-2 condition 5: neither cna nor mobile-eggbert
references XmlDocument outside their audit notes -- zero live sites in both.

Gate: 17,308 run, 17,308 passed, 0 failed, 0 skipped across 38 executables, GREEN.

docs/Migration-XmlDocumentLoadEntityCheck.md

Author: robertvokac

CLAUDE.md

@@ -0,0 +1,85 @@
+<!-- SPDX-License-Identifier: MIT -->
+<!-- Copyright (c) Robert Vokac and contributors -->
+
+# Migration — `XmlDocument::Load(filename)` rejects an undeclared entity too (ticket #2361)
+
+*2026-08-18.* `XmlDocument::Load` now runs the same undeclared-entity check that
+`XmlDocument::LoadXml` has run since #2082. A document loaded **from a file** used to accept
+`&nope;` and then silently rewrite it as `&amp;nope;` on save.
+
+Landed under `docs/StandingApprovals.md` SA-5. No signature, layout or `noexcept` change.
+
+---
+
+## 1. What changed
+
+| Call | Was | Is |
+|---|---|---|
+| `Load(path)` on a file containing `&nope;` | accepted; saved back as `&amp;nope;` | `XmlException` |
+| `LoadXml("<r>&nope;</r>")` | `XmlException` | `XmlException` (unchanged, since #2082) |
+| `Load(path)` on any legal document | — | **unchanged** |
+| `Load(path)` on a missing file | `XmlException` | `XmlException`, same text |
+| `Load(path)` on malformed content | `XmlException` | `XmlException`, same text |
+| `Load(path)` on an undeclared **prefix** | `XmlException` | unchanged — #2083 always ran here |
+
+**The rewrite is the half that lost data.** Accepting the reference was recoverable; reinterpreting
+it as literal text *and re-escaping it* meant loading and saving a document changed the document.
+
+## 2. Why the check could not run here before
+
+`ThrowIfUndeclaredEntityReference` scans the **raw** text, because tinyxml2 decodes the five
+predefined entities during parsing — after which `&amp;nope;` and `&nope;` are indistinguishable.
+`LoadXml(std::string)` has the raw text. `Load(filename)` handed the path straight to
+`tinyxml2::XMLDocument::LoadFile` and never held the bytes.
+
+`Load` now reads the file itself and parses the buffer, which is what `LoadFile` does internally
+anyway (read whole file, call `Parse`).
+
+## 3. The failure path deliberately still goes through `LoadFile`
+
+`LoadFile` is what produces `XML_ERROR_FILE_NOT_FOUND`, `XML_ERROR_FILE_COULD_NOT_BE_OPENED` and
+`XML_ERROR_FILE_READ_ERROR`, and the `ErrorStr()` this exception message has always carried.
+Reproducing those categories from an `ifstream` failure would be **inventing diagnostics** rather
+than keeping them, so when the read fails the code lets tinyxml2 categorise its own failure and
+throws the unchanged message.
+
+The file is read in **binary** mode: a text-mode read on Windows would collapse CRLF and move the
+offsets the scanner walks.
+
+## 4. Two mutations that are not mutations
+
+Recorded rather than counted as passes.
+
+**Text mode instead of binary** is a no-op *on this platform*. On Linux the two modes are
+identical, so the mutation is unobservable here by construction; the flag is a correctness measure
+for a platform the gate does not run on.
+
+**`Parse(c_str())` without the length** is semantically equivalent, and that was verified by probe
+rather than assumed. Three NUL placements were measured through both doors:
+
+| Document | `LoadXml` | `Load` |
+|---|---|---|
+| `<r>a\0b</r>` — NUL mid-document | throws | throws |
+| `<r>a</r>\0junk` — NUL after a complete document | accepted, `<r>a</r>` | accepted, `<r>a</r>` |
+| `<r>a</r>\0<x>&nope;</x>` | throws | throws |
+
+The two doors agree in every shape, with and without the explicit length, because tinyxml2 stops
+at the NUL either way. The explicit length is kept because it is the clearer spelling, not because
+a test defends it. (The probe binaries were deleted afterwards, per the build-resource policy.)
+
+| Real mutation | Caught |
+|---|---|
+| Drop the entity check from `Load` (the pre-#2361 code) | ✅ |
+| Treat an unreadable file as empty content | ✅ (3 tests) |
+| `ReadWholeFile` keeps only the first line | ✅ |
+
+## 5. To migrate
+
+A file containing an undeclared entity reference was never valid XML — .NET rejects it with
+`XmlException("Reference to undeclared entity '{0}'.")` (`XmlTextReaderImpl.cs:3829`). Declare the
+entity in a `<!DOCTYPE>` internal subset, or escape the ampersand as `&amp;`.
+
+## 6. Downstream, measured
+
+Per SA-2 condition 5: neither `cna` nor `mobile-eggbert` references `XmlDocument` outside their
+audit notes — **zero live sites in both**. Neither repository was modified.

docs/Migration-XmlDocumentLoadEntityCheck.md

@@ -2,6 +2,8 @@
 // Copyright (c) Robert Vokac and contributors
 // Portions based on .NET runtime API (MIT License, Copyright .NET Foundation and Contributors)
 #include "System/Xml/XmlDocument.hpp"
+#include <sstream>
+#include <fstream>
 
 #include <cctype>
 #include <vector>
@@ -97,6 +99,26 @@ namespace System::Xml {
          *       This ticket is about the **undeclared** case, where .NET throws and this port
          *       silently rewrote the document's own text.
          */
+        /**
+         * @brief Reads @p filename whole, in binary, into @p contents.
+         *
+         * Ticket #2361. `XmlDocument::Load` needs the raw bytes so that
+         * ThrowIfUndeclaredEntityReference can run at that door too, and binary mode matters:
+         * a text-mode read on Windows would collapse CRLF and change offsets the scanner walks.
+         *
+         * @return @c false if the file cannot be opened or read; the caller then lets tinyxml2
+         *         categorise the failure so the exception text is unchanged.
+         */
+        bool ReadWholeFile(const std::string& filename, std::string& contents) {
+            std::ifstream in(filename, std::ios::binary);
+            if (!in) return false;
+            std::ostringstream buffer;
+            buffer << in.rdbuf();
+            if (in.bad()) return false;
+            contents = buffer.str();
+            return true;
+        }
+
         void ThrowIfUndeclaredEntityReference(const std::string& xml) {
             static const char* const predefined[] = {"amp", "lt", "gt", "quot", "apos"};
             const std::vector<std::string> declaredNames = DeclaredEntityNames(xml);
@@ -548,13 +570,31 @@ namespace System::Xml {
 
     void XmlDocument::Load(const std::string& filename) {
         nodeCache_.clear();
-        if (doc_.LoadFile(filename.c_str()) != tinyxml2::XML_SUCCESS)
+
+        // #2361 (2026-08-18) closed the asymmetry #2082 left here. The entity check needs the
+        // RAW text -- tinyxml2 decodes the five predefined entities during parsing, after which
+        // "&amp;nope;" and "&nope;" are indistinguishable -- and this door used to hand the path
+        // straight to LoadFile without ever holding the bytes. So a document loaded FROM A FILE
+        // accepted an undeclared entity and then silently rewrote it on save, while the identical
+        // text through LoadXml was rejected. One door, two answers.
+        //
+        // This reads the file itself and then parses the buffer, which is what tinyxml2::LoadFile
+        // does internally anyway (read whole file, call Parse). The FAILURE path deliberately
+        // still goes through LoadFile: it is the thing that produces XML_ERROR_FILE_NOT_FOUND /
+        // FILE_COULD_NOT_BE_OPENED / FILE_READ_ERROR and the ErrorStr() this message has always
+        // carried, and reproducing those categories by hand would be inventing diagnostics rather
+        // than keeping them.
+        std::string contents;
+        if (!ReadWholeFile(filename, contents)) {
+            (void)doc_.LoadFile(filename.c_str());   // let tinyxml2 categorise its own failure
+            throw XmlException("XmlDocument::Load: failed to load '" + filename + "': " +
+                               (doc_.ErrorStr() ? doc_.ErrorStr() : "unknown error"));
+        }
+        ThrowIfUndeclaredEntityReference(contents);   // #2082, now at BOTH doors
+        if (doc_.Parse(contents.c_str(), contents.size()) != tinyxml2::XML_SUCCESS)
             throw XmlException("XmlDocument::Load: failed to load '" + filename + "': " +
                                (doc_.ErrorStr() ? doc_.ErrorStr() : "unknown error"));
-        // #2083. The entity check (#2082) cannot run here: it needs the RAW text, and this door
-        // hands the file straight to tinyxml2 without ever holding it. That asymmetry between
-        // the two doors is recorded rather than hidden -- see ticket #2361.
-        ThrowIfUndeclaredPrefix(doc_.RootElement(), {});
+        ThrowIfUndeclaredPrefix(doc_.RootElement(), {});   // #2083, always ran here
         // tinyxml2::XMLDocument::LoadFile() clears and frees every previously-allocated node
         // (including detachedHolder_ from the constructor, or a prior Load/LoadXml call) before
         // parsing; recreate it now, or IsDetached()/getParentNodeProperty() etc. would compare

modules/xml/src/System/Xml/XmlDocument.cpp

@@ -20,6 +20,8 @@
 #include <gtest/gtest.h>
 
 #include <cstddef>
+#include <filesystem>
+#include <fstream>
 #include <memory>
 #include <optional>
 #include <string>
@@ -255,6 +257,72 @@ TEST(XmlContractPinTests, Fix2082_WhereAnAmpersandIsNotMarkupIsSkipped) {
     EXPECT_NO_THROW(d.LoadXml("<?pi &nope; ?><r/>"));
 }
 
+// ===========================================================================
+// #2361 RESOLVED — the entity check now runs at BOTH load doors
+// ===========================================================================
+//
+// #2082 could only reach LoadXml(string), because the check needs the RAW text and Load(filename)
+// handed the path straight to tinyxml2::LoadFile without ever holding the bytes. So the identical
+// document was rejected through one door and accepted through the other -- and then silently
+// rewritten on save, which is the half that loses data.
+
+namespace {
+// Writes @p xml to a temporary file and returns the path. Kept file-local because only these two
+// cases need a file at all; the repository's TMPDIR is redirected to build-tmp/ by the gate.
+std::string writeTempXml(const char* stem, const std::string& xml) {
+    const std::filesystem::path path =
+        std::filesystem::temp_directory_path() / (std::string("sr2361_") + stem + ".xml");
+    std::ofstream out(path, std::ios::binary);
+    out << xml;
+    out.close();
+    return path.string();
+}
+} // namespace
+
+TEST(XmlContractPinTests, Fix2361_LoadFromAFileRejectsAnUndeclaredEntityToo) {
+    const std::string path = writeTempXml("undeclared", "<r>&nope;</r>");
+    XmlDocument d;
+    EXPECT_THROW(d.Load(path), System::Xml::XmlException);
+
+    // The door-equivalence property, which is the actual finding: the same text must get the same
+    // answer through both doors. Before #2361, this file loaded cleanly and then saved itself as
+    // "<r>&amp;nope;</r>" -- the document's own text changed with no diagnostic anywhere.
+    XmlDocument viaString;
+    EXPECT_THROW(viaString.LoadXml("<r>&nope;</r>"), System::Xml::XmlException);
+    std::filesystem::remove(path);
+}
+
+TEST(XmlContractPinTests, Fix2361_LoadFromAFileKeepsEveryOtherAnswerItHad) {
+    // The invariance rows. Reading the bytes here rather than letting tinyxml2 read them must not
+    // change what a legal document does, and must not change what a MISSING one does.
+    const std::string path = writeTempXml(
+        "legal",
+        "<!DOCTYPE r [<!ENTITY greeting \"hello\">]>"
+        "<p:r xmlns:p='urn:x'><!-- &nope; --><![CDATA[&nope;]]>&greeting;&amp;&#65;</p:r>");
+    XmlDocument d;
+    ASSERT_NO_THROW(d.Load(path));
+    EXPECT_NE(d.getDocumentElementProperty(), nullptr);
+
+    // A file that does not exist still reports the same failure, because the failure path
+    // deliberately still runs through tinyxml2 so it keeps categorising its own errors.
+    XmlDocument missing;
+    EXPECT_THROW(missing.Load(path + ".no-such-file"), System::Xml::XmlException);
+
+    // ...and so does a file whose CONTENT is malformed, which is a different failure again.
+    const std::string bad = writeTempXml("malformed", "<r><unclosed></r>");
+    XmlDocument m;
+    EXPECT_THROW(m.Load(bad), System::Xml::XmlException);
+
+    // The undeclared-PREFIX check (#2083) always ran at this door and still does.
+    const std::string prefixed = writeTempXml("prefix", "<p:r/>");
+    XmlDocument pfx;
+    EXPECT_THROW(pfx.Load(prefixed), System::Xml::XmlException);
+
+    std::filesystem::remove(path);
+    std::filesystem::remove(bad);
+    std::filesystem::remove(prefixed);
+}
+
 // ===========================================================================
 // #2083 RESOLVED — an undeclared namespace prefix is rejected
 // ===========================================================================