Harden AuthXMLRequest.setPrincipal against arbitrary class instantiation (same pattern as GHSA-wg5r-wc3x-39vc)

[BarakSrour]

What

AuthXMLRequest.setPrincipal(String, String) resolves and instantiates a class named by its className argument:

public void setPrincipal(String className, String principalValue) {
    try {
        Class clName = Class.forName(className);
        principal = (Principal) clName.newInstance();
    } catch (ClassNotFoundException ce) {
    } catch (IllegalAccessException ia) {
    } catch (InstantiationException ie) {
    } catch (Exception e) {
    }
}

This is the same shape AuthXMLUtils.createCustomCallback had before edcf968 (GHSA-wg5r-wc3x-39vc / CVE-2026-62379). This PR applies the same hardening you used there: resolve with initialize=false, and confirm the class is a Principal before newInstance().

Why the existing cast is not a control

The (Principal) cast is evaluated only after newInstance() returns, so the named class's static initializer and no-arg constructor have both already run by the time the cast can reject it. The one-argument Class.forName also defaults to initialize=true, so a static initializer runs at resolution time even when no instance is ever created. Checked in an isolated JVM:

Class.forName(name) then (Iface) c.newInstance()       ->  [static initializer RAN]
                                                          [constructor RAN]
                                                          ClassCastException
Class.forName(name, false, loader) + isAssignableFrom  ->  rejected, neither ran

Two details specific to this method:

• Every exception is swallowed by empty catch blocks, including the ClassCastException. Side effects from loading a caller-named class would leave no trace in the logs at all — unlike createCustomCallback, which at least reached debug.error.
• principalValue is never used. It appears in the signature and the Javadoc, and nowhere in the body.

This is not a vulnerability report and needs no advisory

Stating this plainly so it is not routed as a security disclosure:

• AuthXMLRequest is referenced by five files, all in openam-core — itself, AuthXMLRequestParser, AuthXMLResponse, AuthXMLHandler and AuthUtils — and none calls setPrincipal. I checked the whole monorepo: the other setPrincipal matches are com.iplanet.ums.* setPrincipal(Principal) plus unrelated single-argument methods in openam-federation, openam-sts and openam-auth-windowsdesktopsso.
• With no reachable path there is no attacker model, so no CVE and no advisory are warranted, and I am not requesting either. This is defence in depth on a public method that is one caller away from being live, in a class that reached Class.forName(caller_string).newInstance() under CVE-2026-62379.
• Limits of my own claim: reachability was assessed statically against this repository and the published openam-core 16.1.1 / 16.1.2 artifacts. I have no visibility into downstream integrations, and setPrincipal is public on a public class, so an external caller may exist.

You may prefer to delete it instead

The method is unreachable in-tree and ignores its second parameter, so removing it outright is arguably the better change and I am happy to switch this PR to a deletion. I proposed hardening because deleting a public method on a public class is a breaking change for any downstream caller, and that trade-off is yours. Say which you would prefer.

Testing

Adds AuthXMLRequestSecurityTest, following the structure of the AuthXMLUtilsSecurityTest added in edcf968 (TestNG + AssertJ, probe class with static flags). It asserts a non-Principal class is neither initialized nor instantiated, and that a legitimate Principal still resolves.

One design note worth flagging, because the obvious version of this test silently passes against the unhardened code: the probe's flags live in a separate holder class. Reading or writing a static field triggers that class's initialization, so flags stored on the probe itself get set by the test's own reset, and the "was it initialized" assertion then passes no matter what forName does. With the flags held elsewhere the probe is only ever named by a class literal, which does not trigger initialization. I verified both assertions fail against the 1-argument Class.forName shape and pass with this change.

I have not been able to run the module's full test suite locally (the build needs JDK 17 and this host has 16), so please treat CI as the authority on that.

[maximthomas]

LGTM

[tsujiguchitky]

Nice hardening. The ordering looks right to me.

One small thing about the comment though. It opens with "className is read from the /authservice (PLL) request XML", but as your commit message points out, this method has no in-tree callers, and AuthXMLTags.PRINCIPAL is only declared -- no parser actually feeds it into setPrincipal. I'm guessing it got carried over from the AuthXMLUtils one? As written it reads like there's a reachable path, so it'd be great if you could reword it to say what's really going on: no callers, but it's public, so we hold it to the same contract anyway.

[BarakSrour]

Nice hardening. The ordering looks right to me.

One small thing about the comment though. It opens with "className is read from the /authservice (PLL) request XML", but as your commit message points out, this method has no in-tree callers, and AuthXMLTags.PRINCIPAL is only declared -- no parser actually feeds it into setPrincipal. I'm guessing it got carried over from the AuthXMLUtils one? As written it reads like there's a reachable path, so it'd be great if you could reword it to say what's really going on: no callers, but it's public, so we hold it to the same contract anyway.

Good catch, thanks - fixed

[tsujiguchitky]

Thanks for the quick turnaround! The reworded comment says exactly what I was hoping for 🙏

Approving.