From 521195f6cd51186986a42008f3f886ad04f770dc Mon Sep 17 00:00:00 2001 From: Rahul Krishna Date: Mon, 31 Aug 2026 16:46:58 -0400 Subject: [PATCH 1/2] fix(artifacts): flatten only configuration XML, not every XML in the repository Discovery classifies every *.xml in a repository as an artifact, and the flattener mints a key per element path. That is what configuration looks like, and equally what a data file looks like. On ThingsBoard v4.0 it produced 39,551 keys from 453 XML files, 30,408 of them from LwM2M object registries under src/main/data -- static data resources, not settings. No size threshold separates the two: that repository's pom.xml legitimately carries 1332 keys while a data registry carries 615. So recognition is by name and by location, never by volume. A name list alone is not enough. Vendor deployment descriptors are open-ended, and daytrader8's ibm-web-bnd.xml / ibm-web-ext.xml are real configuration that no plausible list contains -- a name-only rule silently dropped 41 of its keys. WEB-INF and META-INF are directories the servlet and Java EE specifications define to hold descriptors, so an XML sitting directly in one is configuration by construction. Data resources do not live there. An unrecognized XML keeps its artifact entry, source, sha256 and roles; only flattening is skipped, and the result stays ok=true rather than being reported as a parse failure. Measured: ThingsBoard config keys 42,768 -> 8,447, with the remaining XML keys coming from genuine descriptors. daytrader8's analysis.json is byte-identical to before this change. Note for the issue: this removes meaningless keys, but it is NOT a size fix. The document goes 480 MB -> 469 MB, because its bulk is the symbol table for 4131 files, not config keys. Anyone chasing output size should look there. Closes #210 --- .../com/ibm/cldk/artifacts/ConfigKeys.java | 78 ++++++++++++++++++ .../ibm/cldk/artifacts/ConfigKeysTest.java | 81 +++++++++++++++++++ 2 files changed, 159 insertions(+) diff --git a/src/main/java/com/ibm/cldk/artifacts/ConfigKeys.java b/src/main/java/com/ibm/cldk/artifacts/ConfigKeys.java index 707906d9..851cd284 100644 --- a/src/main/java/com/ibm/cldk/artifacts/ConfigKeys.java +++ b/src/main/java/com/ibm/cldk/artifacts/ConfigKeys.java @@ -8,10 +8,12 @@ import java.io.IOException; import java.io.StringReader; import java.util.ArrayList; +import java.util.Arrays; import java.util.Comparator; import java.util.HashSet; import java.util.LinkedHashMap; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.Properties; import java.util.Set; @@ -155,6 +157,9 @@ public static Result extract(JArtifact artifact, String text, boolean captureVal artifact.getId(), "env", recognizeComposeEnv(flat), captureValue, false, k -> CanId.configKeyEnvDualMintId(artifact.getId(), k))); } else if ("xml".equals(artifact.getFormat())) { + if (!isConfigXml(artifact.getPath())) { + return new Result(List.of(), true); + } keys = extractXml(artifact.getId(), text, captureValue); } else { return new Result(List.of(), true); @@ -166,6 +171,79 @@ public static Result extract(JArtifact artifact, String text, boolean captureVal } } + /** + * XML basenames worth flattening. Discovery classifies every {@code *.xml} in a repository as an + * artifact, and the flattener mints a key per element path -- which is what configuration looks + * like, and also what a data file looks like. On ThingsBoard v4.0 that produced 39,551 keys from + * 453 XML files, 30,408 of them from LwM2M object registries under {@code src/main/data/}: static + * data resources, not settings (#210). + * + *

No size threshold separates the two -- that repository's {@code pom.xml} legitimately carries + * 1332 keys while a data registry carries 615 -- so recognition is by name, not by volume. A file + * that is not recognized stays in the artifact inventory with its {@code source}, {@code sha256} + * and roles intact; only flattening is skipped, so nothing disappears from the file listing and + * the result is {@code ok=true}, not a parse failure. + * + *

A name list alone is not enough, because vendor deployment descriptors are open-ended + * ({@code ibm-web-bnd.xml}, {@code jboss-web.xml}, {@code weblogic.xml}, ...) -- daytrader8's two + * {@code ibm-web-*.xml} descriptors are real configuration and no plausible list contains them. + * So location counts too: {@code WEB-INF/} and {@code META-INF/} are directories the servlet and + * Java EE specifications define to hold descriptors, so an XML sitting directly in one is + * configuration by construction rather than by guess. Data resources do not live there. + * + *

The accepted cost is a project whose configuration lives in an unlisted XML name outside + * those directories. That file is still inventoried and still discoverable; it just contributes + * no {@code config_keys}. Adding a name here is the fix when one turns up -- prefer that to + * widening the rule to "any XML under a config-ish directory", which is how the unbounded + * behaviour arose in the first place. + */ + private static final Set CONFIG_XML_NAMES = new HashSet<>(Arrays.asList( + "pom.xml", "web.xml", "ejb-jar.xml", "application.xml", "persistence.xml", + "beans.xml", "faces-config.xml", "server.xml", "context.xml", "settings.xml", + "ivy.xml", "struts.xml", "hibernate.cfg.xml", "logback.xml", "logback-test.xml", + "log4j.xml", "log4j2.xml", "checkstyle.xml", "spotbugs.xml", "pmd.xml", + "jboss-web.xml", "weblogic.xml", "orm.xml", "validation.xml", "portlet.xml", + "tiles.xml", "sun-jaxb.episode", "liquibase.xml", "AndroidManifest.xml")); + + /** Recognized suffixes, for families that are named by convention rather than by one filename. */ + private static final List CONFIG_XML_SUFFIXES = + Arrays.asList("-context.xml", "-config.xml", "-configuration.xml", "-beans.xml", + "-servlet.xml", "-ds.xml", "-mapper.xml", "-changelog.xml"); + + /** + * Whether an XML artifact is a configuration file worth flattening: a recognized basename, a + * conventional suffix, or any XML in a descriptor directory. A suffix never matches bare -- + * {@code notweb.xml} must not pass as {@code web.xml}. + * + * @param path the artifact's project-relative path (the directory part carries the location signal) + */ + static boolean isConfigXml(String path) { + String lower = basename(path).toLowerCase(Locale.ROOT); + if (CONFIG_XML_NAMES.contains(lower)) { + return true; + } + for (String suffix : CONFIG_XML_SUFFIXES) { + // A suffix match needs something in front of it, so "-context.xml" itself is not a match. + if (lower.length() > suffix.length() && lower.endsWith(suffix)) { + return true; + } + } + return inDescriptorDirectory(path); + } + + /** True when the file sits directly inside a {@code WEB-INF} or {@code META-INF} directory. */ + private static boolean inDescriptorDirectory(String path) { + String normalized = path.replace('\\', '/'); + int slash = normalized.lastIndexOf('/'); + if (slash < 0) { + return false; + } + String parent = normalized.substring(0, slash); + int parentSlash = parent.lastIndexOf('/'); + String dir = parentSlash < 0 ? parent : parent.substring(parentSlash + 1); + return "WEB-INF".equalsIgnoreCase(dir) || "META-INF".equalsIgnoreCase(dir); + } + // ---- properties: java.util.Properties handles key=value/key:value, `\` continuations, and // `#`/`!` comments natively -- and last-wins, since a later duplicate key simply overwrites the // earlier one on load. Using the JDK parser buys all of that for free, at the cost of position diff --git a/src/test/java/com/ibm/cldk/artifacts/ConfigKeysTest.java b/src/test/java/com/ibm/cldk/artifacts/ConfigKeysTest.java index 3f481d71..21fc0382 100644 --- a/src/test/java/com/ibm/cldk/artifacts/ConfigKeysTest.java +++ b/src/test/java/com/ibm/cldk/artifacts/ConfigKeysTest.java @@ -35,6 +35,87 @@ private static Optional find(List keys, String key) { return keys.stream().filter(k -> k.getKey().equals(key)).findFirst(); } + // ---- xml: only recognized configuration shapes are flattened (#210) ---------------------- + + /** A data file shaped like the LwM2M object registries that produced 30k keys on ThingsBoard. */ + private static String dataXml() { + StringBuilder b = new StringBuilder(""); + for (int i = 0; i < 40; i++) { + b.append("n").append(i) + .append("Integer"); + } + return b.append("").toString(); + } + + @Test + void aRecognizedConfigXmlIsStillFlattened() { + ConfigKeys.Result r = ConfigKeys.extract(artifact("src/main/webapp/WEB-INF/web.xml", "xml"), + "mode", true); + assertTrue(r.ok); + assertFalse(r.keys.isEmpty(), "web.xml is configuration and must still be flattened"); + } + + @Test + void pomXmlIsStillFlattened() { + ConfigKeys.Result r = ConfigKeys.extract(artifact("pom.xml", "xml"), + "11", true); + assertTrue(r.ok); + assertFalse(r.keys.isEmpty(), "pom.xml carries real configuration"); + } + + @Test + void anUnrecognizedXmlYieldsNoKeysButIsNotAFailure() { + // The artifact stays in the inventory; only flattening is skipped. ok=true, exactly as for a + // format with no flattener -- marking it a parse failure would misreport a healthy file. + ConfigKeys.Result r = ConfigKeys.extract( + artifact("application/src/main/data/lwm2m-registry/10326.xml", "xml"), dataXml(), true); + assertTrue(r.ok, "skipping extraction is not a parse failure"); + assertTrue(r.keys.isEmpty(), "a data registry is not configuration and must not be flattened"); + } + + @Test + void aSuffixMatchDoesNotAdmitAnArbitraryFile() { + // "notweb.xml" ends with "web.xml" but is not it; recognition is on the whole basename. + ConfigKeys.Result r = ConfigKeys.extract(artifact("src/notweb.xml", "xml"), dataXml(), true); + assertTrue(r.ok); + assertTrue(r.keys.isEmpty(), "recognition must match the basename, not a suffix of it"); + } + + @Test + void aVendorDescriptorInWebInfIsRecognizedByItsLocation() { + // daytrader8 ships ibm-web-bnd.xml / ibm-web-ext.xml. Vendor descriptors are open-ended, so + // no name list catches them; WEB-INF is a spec-defined descriptor directory, so location does. + ConfigKeys.Result r = ConfigKeys.extract(artifact("src/main/webapp/WEB-INF/ibm-web-bnd.xml", "xml"), + "", true); + assertTrue(r.ok); + assertFalse(r.keys.isEmpty(), "an XML in WEB-INF is a deployment descriptor"); + } + + @Test + void anXmlInMetaInfIsRecognizedByItsLocation() { + ConfigKeys.Result r = ConfigKeys.extract(artifact("src/main/resources/META-INF/anything.xml", "xml"), + "1", true); + assertTrue(r.ok); + assertFalse(r.keys.isEmpty(), "META-INF is a descriptor directory"); + } + + @Test + void aDescriptorDirectoryDeeperInThePathDoesNotCount() { + // Only the immediate parent counts: data nested far below a WEB-INF is not a descriptor. + ConfigKeys.Result r = ConfigKeys.extract(artifact("WEB-INF/data/registry/10326.xml", "xml"), + dataXml(), true); + assertTrue(r.ok); + assertTrue(r.keys.isEmpty(), "the file must sit directly in the descriptor directory"); + } + + @Test + void aSuffixPatternStillMatchesItsFamily() { + ConfigKeys.Result r = ConfigKeys.extract(artifact("src/main/resources/app-context.xml", "xml"), + "", true); + assertTrue(r.ok); + assertFalse(r.keys.isEmpty(), "*-context.xml is a recognized Spring configuration shape"); + } + // ---- properties ------------------------------------------------------------------------- @Test From b5ef036186213eb36327bf6c08232fe585a6a6fd Mon Sep 17 00:00:00 2001 From: Rahul Krishna Date: Mon, 31 Aug 2026 17:14:26 -0400 Subject: [PATCH 2/2] fix(artifacts): widen the config-XML allowlist and stop discarding DOCTYPE configs Two gaps found by probing the first allowlist against real framework names rather than trusting it. The suffix rules assumed a hyphen, so they matched spring-servlet.xml but missed the canonical spellings of the very frameworks they targeted: applicationContext .xml (Spring) and UserMapper.xml (MyBatis) both failed a -context.xml/-mapper.xml test. Matching against the lowercased stem instead catches camelCase, hyphenated and underscored spellings alike. Short stems that occur inside ordinary words are excluded -- "ds" would admit records.xml -- so those keep their separator. The name list also gains Ant, Jenkins, WildFly, OSGi, Tomcat, Quartz, Ehcache, Hibernate mappings and the Spring Boot logging variants. Probed: 16 of 16 common framework configs now flatten, and records.xml, productCatalog.xml and the data registries still do not. Separately, config files were being discarded wholesale. The parser refused any DOCTYPE, which rejects the whole document rather than the dangerous part: on ThingsBoard that silently reduced 43 files to extraction=partial with no keys, 35 of them logback.xml -- exactly the configuration this layer exists to report. Configuration files now parse through their own factory that permits an internal subset while keeping every external route closed: general entities, parameter entities, external DTD loading and XInclude all stay off, FEATURE_SECURE_PROCESSING still caps entity expansion, and entity references are not expanded into the tree. XmlHardeningTest pins each of those separately, including that an external entity cannot leak file contents and that a billion-laughs document is refused. Manifests deliberately keep the categorical refusal. A pom.xml has no legitimate reason to declare a DOCTYPE, and the four existing tests that attribute each switch on that factory are untouched and still pass -- widening the shared factory was my first attempt and it broke exactly those, which is what showed the split was the right shape. The parser also no longer narrates: an error handler replaces the JDK's raw [Fatal Error] stderr lines, which bypassed Log and arrived 43 at a time. Measured: ThingsBoard config keys 42,768 -> 9,352 with all 35 logback.xml now extraction=full contributing 905 real keys, and stderr noise 43 lines -> 0. daytrader8's analysis.json stays byte-identical. --- .../com/ibm/cldk/artifacts/ConfigKeys.java | 63 +++++++--- .../ibm/cldk/artifacts/ManifestParsers.java | 69 ++++++++++- .../ibm/cldk/artifacts/XmlHardeningTest.java | 113 ++++++++++++++++++ 3 files changed, 230 insertions(+), 15 deletions(-) create mode 100644 src/test/java/com/ibm/cldk/artifacts/XmlHardeningTest.java diff --git a/src/main/java/com/ibm/cldk/artifacts/ConfigKeys.java b/src/main/java/com/ibm/cldk/artifacts/ConfigKeys.java index 851cd284..f98ed48e 100644 --- a/src/main/java/com/ibm/cldk/artifacts/ConfigKeys.java +++ b/src/main/java/com/ibm/cldk/artifacts/ConfigKeys.java @@ -198,17 +198,43 @@ public static Result extract(JArtifact artifact, String text, boolean captureVal * behaviour arose in the first place. */ private static final Set CONFIG_XML_NAMES = new HashSet<>(Arrays.asList( - "pom.xml", "web.xml", "ejb-jar.xml", "application.xml", "persistence.xml", - "beans.xml", "faces-config.xml", "server.xml", "context.xml", "settings.xml", - "ivy.xml", "struts.xml", "hibernate.cfg.xml", "logback.xml", "logback-test.xml", - "log4j.xml", "log4j2.xml", "checkstyle.xml", "spotbugs.xml", "pmd.xml", - "jboss-web.xml", "weblogic.xml", "orm.xml", "validation.xml", "portlet.xml", - "tiles.xml", "sun-jaxb.episode", "liquibase.xml", "AndroidManifest.xml")); - - /** Recognized suffixes, for families that are named by convention rather than by one filename. */ - private static final List CONFIG_XML_SUFFIXES = - Arrays.asList("-context.xml", "-config.xml", "-configuration.xml", "-beans.xml", - "-servlet.xml", "-ds.xml", "-mapper.xml", "-changelog.xml"); + // build / project + "pom.xml", "build.xml", "settings.xml", "toolchains.xml", "ivy.xml", "assembly.xml", + // servlet / Java EE descriptors + "web.xml", "ejb-jar.xml", "application.xml", "application-client.xml", "persistence.xml", + "beans.xml", "faces-config.xml", "orm.xml", "validation.xml", "portlet.xml", "ra.xml", + // containers and app servers + "server.xml", "context.xml", "tomcat-users.xml", "standalone.xml", "domain.xml", + "jboss-web.xml", "jboss-ejb3.xml", "jboss-deployment-structure.xml", "weblogic.xml", + "glassfish-web.xml", "glassfish-application.xml", "blueprint.xml", + // frameworks + "struts.xml", "tiles.xml", "hibernate.cfg.xml", "quartz.xml", "ehcache.xml", + "liquibase.xml", "bindings.xml", "sun-jaxb.episode", + // logging + "logback.xml", "logback-test.xml", "logback-spring.xml", + "log4j.xml", "log4j2.xml", "log4j2-spring.xml", "log4j2-test.xml", + // static analysis / tooling + "checkstyle.xml", "spotbugs.xml", "findbugs-exclude.xml", "pmd.xml", "suppressions.xml", + // misc + "config.xml", "androidmanifest.xml")); + + /** + * Name stems that mark a configuration file whatever separator precedes them. Matching only + * hyphenated forms was the first attempt and it missed the canonical spellings of the very + * frameworks it was meant to cover: {@code applicationContext.xml} (Spring) and {@code + * UserMapper.xml} (MyBatis) both failed a {@code -context.xml}/{@code -mapper.xml} test, while + * {@code spring-servlet.xml} passed. Comparing against the lowercased stem catches camelCase, + * hyphenated and underscored spellings alike. + * + *

Deliberately excludes short stems that occur inside ordinary words -- {@code "ds"} would + * admit {@code records.xml} -- so those stay hyphen-only in {@link #CONFIG_XML_SUFFIXES}. + */ + private static final List CONFIG_XML_STEMS = + Arrays.asList("context", "config", "configuration", "settings", "beans", "servlet", + "mapper", "changelog", "datasource"); + + /** Suffixes that must keep their separator, being too short or too common to match bare. */ + private static final List CONFIG_XML_SUFFIXES = Arrays.asList("-ds.xml", ".hbm.xml"); /** * Whether an XML artifact is a configuration file worth flattening: a recognized basename, a @@ -223,11 +249,21 @@ static boolean isConfigXml(String path) { return true; } for (String suffix : CONFIG_XML_SUFFIXES) { - // A suffix match needs something in front of it, so "-context.xml" itself is not a match. + // A suffix match needs something in front of it, so ".hbm.xml" itself is not a match. if (lower.length() > suffix.length() && lower.endsWith(suffix)) { return true; } } + if (lower.endsWith(".xml")) { + String stem = lower.substring(0, lower.length() - ".xml".length()); + for (String token : CONFIG_XML_STEMS) { + // The stem must be preceded by something, so a file named exactly "config.xml" is + // matched by name above rather than here, and "context.xml" likewise. + if (stem.length() > token.length() && stem.endsWith(token)) { + return true; + } + } + } return inDescriptorDirectory(path); } @@ -329,8 +365,7 @@ private static boolean isDigits(String s) { private static List extractXml(String artifactId, String text, boolean captureValue) throws ParserConfigurationException, SAXException, IOException { - Document doc = ManifestParsers.newSecureDocumentBuilderFactory() - .newDocumentBuilder() + Document doc = ManifestParsers.newConfigDocumentBuilder() .parse(new InputSource(new StringReader(text))); Element root = doc.getDocumentElement(); List entries = new ArrayList<>(); diff --git a/src/main/java/com/ibm/cldk/artifacts/ManifestParsers.java b/src/main/java/com/ibm/cldk/artifacts/ManifestParsers.java index 281356f4..797778f6 100644 --- a/src/main/java/com/ibm/cldk/artifacts/ManifestParsers.java +++ b/src/main/java/com/ibm/cldk/artifacts/ManifestParsers.java @@ -16,8 +16,10 @@ import org.w3c.dom.Element; import org.w3c.dom.Node; import org.w3c.dom.NodeList; +import org.xml.sax.ErrorHandler; import org.xml.sax.InputSource; import org.xml.sax.SAXException; +import org.xml.sax.SAXParseException; /** * Pure dependency-manifest and lockfile readers: text in, records out. No file I/O, no execution, @@ -174,7 +176,7 @@ private static String kindForMavenDependency(String scope, boolean optional) { } private static DocumentBuilder newSecureDocumentBuilder() throws ParserConfigurationException { - return newSecureDocumentBuilderFactory().newDocumentBuilder(); + return quiet(newSecureDocumentBuilderFactory().newDocumentBuilder()); } /** @@ -190,6 +192,11 @@ private static DocumentBuilder newSecureDocumentBuilder() throws ParserConfigura * toggles are kept as explicit defense in depth alongside it, and {@code * FEATURE_SECURE_PROCESSING} restricts external DTD/entity resource access (JAXP's {@code * accessExternalDTD}) independently of both. + * + *

A dependency manifest has no legitimate reason to declare a DOCTYPE, so this stays + * categorical. Configuration files do -- {@code logback.xml} routinely carries one -- and they + * are parsed through {@link #newConfigDocumentBuilder} instead, which permits an internal subset + * while keeping every external switch here identical. */ static DocumentBuilderFactory newSecureDocumentBuilderFactory() throws ParserConfigurationException { DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); @@ -200,6 +207,66 @@ static DocumentBuilderFactory newSecureDocumentBuilderFactory() throws ParserCon return factory; } + /** + * The parser for configuration files, which differs from {@link #newSecureDocumentBuilderFactory} + * in exactly one respect: an internal DOCTYPE subset is allowed. + * + *

Refusing every DOCTYPE rejects the whole document rather than the dangerous part. On + * ThingsBoard v4.0 that discarded 43 files, 35 of them {@code logback.xml} -- precisely the + * configuration this analyzer exists to report, silently reduced to {@code extraction=partial} + * with no keys. XXE needs an external reference to resolve, and all four external + * routes stay closed here (general entities, parameter entities, external DTD loading, + * XInclude), with {@code FEATURE_SECURE_PROCESSING} capping entity expansion so an internal + * "billion laughs" is refused rather than expanded. {@code setExpandEntityReferences(false)} + * additionally keeps entity bodies out of the walked tree. + * + *

Manifests deliberately do NOT use this: a pom.xml declaring a DOCTYPE has no legitimate + * shape, so it keeps the categorical refusal. + */ + static DocumentBuilderFactory newConfigDocumentBuilderFactory() throws ParserConfigurationException { + DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); + factory.setFeature("http://xml.org/sax/features/external-general-entities", false); + factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, ""); + factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_SCHEMA, ""); + factory.setXIncludeAware(false); + factory.setExpandEntityReferences(false); + return factory; + } + + /** A configuration-file builder that does not narrate to stderr. */ + static DocumentBuilder newConfigDocumentBuilder() throws ParserConfigurationException { + return quiet(newConfigDocumentBuilderFactory().newDocumentBuilder()); + } + + /** + * Silence the JDK parser's own commentary. With no error handler installed it prints every + * problem to {@code System.err} itself -- raw {@code [Fatal Error] :19:10: ...} lines that + * bypass {@link com.ibm.cldk.utils.Log}, tell the user nothing actionable, and on a large + * repository arrive dozens at a time. Failures already travel in each caller's + * {@code (records, ok)} result, so the thrown exception carries the signal; this removes only + * the duplicate narration. + */ + static DocumentBuilder quiet(DocumentBuilder builder) { + builder.setErrorHandler(new ErrorHandler() { + @Override + public void warning(SAXParseException e) {} + + @Override + public void error(SAXParseException e) throws SAXException { + throw e; + } + + @Override + public void fatalError(SAXParseException e) throws SAXException { + throw e; + } + }); + return builder; + } + private static List directChildren(Element parent, String tagName) { List children = new ArrayList<>(); NodeList nodes = parent.getChildNodes(); diff --git a/src/test/java/com/ibm/cldk/artifacts/XmlHardeningTest.java b/src/test/java/com/ibm/cldk/artifacts/XmlHardeningTest.java new file mode 100644 index 00000000..21d5c59e --- /dev/null +++ b/src/test/java/com/ibm/cldk/artifacts/XmlHardeningTest.java @@ -0,0 +1,113 @@ +package com.ibm.cldk.artifacts; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.IOException; +import java.io.StringReader; +import java.nio.file.Files; +import java.nio.file.Path; +import javax.xml.parsers.ParserConfigurationException; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.w3c.dom.Document; +import org.xml.sax.InputSource; +import org.xml.sax.SAXException; + +/** + * The XML parser reads untrusted repository content, so its hardening is load-bearing and each + * guarantee is asserted separately rather than trusted as a bundle. + * + *

An internal DOCTYPE subset is permitted on purpose: refusing every DOCTYPE rejected the whole + * document rather than the dangerous part, discarding 43 real files on ThingsBoard v4.0 (35 of them + * {@code logback.xml}). These tests pin that the permission did not buy an XXE back — every + * external reference must still fail to resolve. + */ +class XmlHardeningTest { + + private static Document parse(String xml) throws ParserConfigurationException, SAXException, IOException { + return ManifestParsers.newConfigDocumentBuilder().parse(new InputSource(new StringReader(xml))); + } + + @Test + void anInternalDoctypeSubsetIsAccepted() throws Exception { + // The logback.xml shape: a DOCTYPE that references nothing outside the document. + Document d = parse("\n"); + assertNotNull(d.getDocumentElement()); + assertTrue("configuration".equals(d.getDocumentElement().getTagName())); + } + + @Test + void anExternalEntityDoesNotLeakFileContents(@TempDir Path tmp) throws Exception { + Path secret = tmp.resolve("secret.txt"); + Files.write(secret, "TOP_SECRET_VALUE".getBytes("UTF-8")); + String xml = "]>\n&xxe;"; + + String text; + try { + text = parse(xml).getDocumentElement().getTextContent(); + } catch (SAXException blocked) { + return; // Refusing outright is an equally acceptable outcome. + } + assertFalse(text.contains("TOP_SECRET_VALUE"), + "external entity must never resolve — this is the XXE the hardening exists to stop"); + } + + @Test + void anExternalParameterEntityIsNotFetched(@TempDir Path tmp) throws Exception { + Path dtd = tmp.resolve("evil.dtd"); + Files.write(dtd, "".getBytes("UTF-8")); + String xml = " %ext;]>\nok"; + try { + assertFalse(parse(xml).getDocumentElement().getTextContent().contains("PWNED"), + "external parameter entity must not be fetched"); + } catch (SAXException blocked) { + // Also fine. + } + } + + @Test + void anExternalDtdIsNotLoaded(@TempDir Path tmp) throws Exception { + Path dtd = tmp.resolve("ext.dtd"); + Files.write(dtd, "".getBytes("UTF-8")); + String xml = "\nok"; + try { + assertFalse(parse(xml).getDocumentElement().getTextContent().contains("INJECTED"), + "an external DTD must not be loaded"); + } catch (SAXException blocked) { + // Also fine. + } + } + + @Test + void aBillionLaughsExpansionIsRefusedRatherThanExhaustingMemory() { + StringBuilder dtd = new StringBuilder(""); + for (int i = 1; i <= 9; i++) { + dtd.append(""); + } + dtd.append("]>\n&lol9;"); + // FEATURE_SECURE_PROCESSING caps entity expansion, so this fails fast instead of hanging. + assertThrows(SAXException.class, () -> parse(dtd.toString()), + "unbounded entity expansion must be refused"); + } + + @Test + void theParserDoesNotWriteToStderrOnAMalformedDocument() throws Exception { + java.io.PrintStream original = System.err; + java.io.ByteArrayOutputStream captured = new java.io.ByteArrayOutputStream(); + System.setErr(new java.io.PrintStream(captured, true, "UTF-8")); + try { + assertThrows(SAXException.class, () -> parse("")); + } finally { + System.setErr(original); + } + assertTrue(captured.toString("UTF-8").isEmpty(), + "the parser must not narrate to stderr — errors travel in the result, not as [Fatal Error] spam"); + } +}