Refactor XmlFactories into their hardening target factories.

[garydgregory]

Refactor XmlFactories into their hardening target factories.

Missing Java 8 methods will be added in a follow up PR.

Before you push a pull request, review this list:

• Read the contribution guidelines for this project.
• Read the ASF Generative Tooling Guidance if you use Artificial Intelligence (AI).
• [ ] I used AI to create any part of, or all of, this pull request. Which AI tool was used to create this pull request, and to what extent did it contribute?
• Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself.
• Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best practice.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.

[ppkarwasz]

I have mixed feelings about using the wrapper classes as public entry points:

Pros:

• We save up on the number of classes.

Cons:

• Users can use instanceof on the implementation class of the wrapper. I am not sure if this is good or bad. Since users could always compare implementation by class name, this is not a blocker.
• This makes a Multi-Release JAR harder to produce without duplicating a lot of code. Therefore we are left with calling the Java 9 and Java 13 methods by reflection. Knowing what users do with our JARs (e.g. log4j-api), maybe it is better, since there are a lot of shaders out there that don't know how to shade an MRJ.

If we choose this model (i.e. wrapper class == public entry point and Java 9/13 methods called by reflection), I would recommend to merge this class with DocumentBuilderHardener in a follow-up PR.

[garydgregory]

without duplicating a lot of code.

That shouldn't be the case if we subclass, if that matters that much, for example a HardeningDocumentBuilderFactory13 could extend HardeningDocumentBuilderFactory9 could extend HardeningDocumentBuilderFactory but I'm not sure an MRJ would allow that.

[ppkarwasz]

This method hides DocumentBuilderFactory, which is not a problem per-se, but is a problem for the methods that we are not hiding, which will return not hardened methods.

[garydgregory]

Like what? HardeningDocumentBuilderFactory.newDocumentBuilder() returns a safe DocumentBuilder

[ppkarwasz]

I compared this design with #54 and I prefer this one: it keeps all (or nearly all) the code for a given JAXP factory type in a single class.

I would only change a few things:

• Move the instance methods of each wrapper into a nested Wrapper class, so that the public class no longer extends the JAXP factory:

  public final class HardeningDocumentBuilderFactory {
  
      public static DocumentBuilderFactory newInstance() { ... }
  
      private static final class Wrapper extends DocumentBuilderFactory {
          ... current code ...
      }
  }

  This way no JAXP static method is hidden, and users cannot accidentally obtain a non-hardened factory by calling an inherited method such as newInstance(String, ClassLoader) or newDefaultInstance().

• (follow-up PR) Add the remaining JAXP factory methods, as you proposed. For the Java 9 and Java 13 methods we can use MethodHandles.publicLookup() rather than core reflection.

• (follow-up PR) Merge DocumentBuilderHardener into HardeningDocumentBuilderFactory, and likewise for the other types.

• (optional PR) Rename the classes, e.g. SafeDocumentBuilderFactory.

I dropped the Multi-Release jar idea: users shade this library without setting Multi-Release: true, then report that methods which worked in their IDE fail in "production". We have seen plenty of such issue reports for Log4j.

[garydgregory]

Arg, I meant to open a PR in my fork, not here, apologies.