hansva commented on PR #8947:
URL: https://github.com/apache/hop/pull/8947#issuecomment-6193164272
## Review
Not built or tested locally.
**Must fix**
1. **XXE protection can be skipped.** In `Dom4JUtil.java:36-48`, all
features share one `try`, with `disallow-doctype-decl` set first. With
`XML_ALLOW_DOCTYPE_DECL=Y`, if a later `setFeature` fails, the failure is only
logged and the external-entity/DTD features are never set. Use one `try` per
feature, or refuse the DOCTYPE on failure.
2. **`XML_ALLOW_DOCTYPE_DECL` has opposite defaults.** Core
(`XmlParserFactoryProducer.java:65`) refuses only when the value is `"N"`, so
unset means allowed. `Dom4JUtil.java:40` allows only when it is `"Y"`, so unset
means refused. As a result, the DTD Validator and Get Data From XML disagree on
the same file. Read it through one shared helper.
3. **XSLT includes bypass VFS.** `XsltData.java:94` and the action's
`Xslt.java:389` pass the VFS URI as `systemId`, but Saxon loads
`xsl:include`/`document()` through `java.net.URL`. This fails for `s3://`,
`gs://`, `azure://` and named connections, and breaks absolute `href`s in
remote stylesheets that used to work. Set a HopVfs-backed `URIResolver`.
4. **XSLTC is not covered.** `createSecureTransformerFactory()` sets
`ACCESS_EXTERNAL_STYLESHEET=""`, which blocks includes under the JDK's XSLTC.
The JAXP leg of `XsltRelativeIncludeTest` passes only because Saxon is the JAXP
provider.
**Should fix**
5. **Element type is still stored as translated text.**
`GetXmlDataDialog.java:1844` still saves `Attributo`, `属性`, and so on. The
cross-locale lookup (`GetXmlDataField.java:202`) breaks as soon as a
translation changes. Store the code, and keep the lookup only for reading
existing files.
6. **Empty-root output (XML Output Advanced)**,
`AdvancedXmlOutput.java:497-537`:
- If the root is also the loop element, zero rows produce `<customer/>`,
which downstream reads as one empty record.
- The bare tag omits forced/default attributes and static child elements.
- If `writeEmptyRootElement()` throws, the writer, zip and stream are
never closed, and `setErrors(1)` overwrites the error count.
- The docs (`xmloutputadvanced.adoc:57`) promise an empty root, but with
*Do not open new file at start* on, no file is written.
**Minor**
7. With a DOCTYPE allowed, only the number of entity expansions is capped,
not their total size. A quadratic blowup (~400 KB input → ~4.5 GB) still causes
an OOM. The test only covers billion laughs.
8. `rootStartedInCurrentFile` (`AdvancedXmlOutputData.java:43`) duplicates
`currentGroupKey != null`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
