This is an automated email from the ASF dual-hosted git repository. robertlazarski pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/axis-axis2-java-rampart.git
commit c8071bad73aa1e66aefcdaa4afffefc262afed27 Author: Robert Lazarski <[email protected]> AuthorDate: Wed Jun 10 03:58:00 2026 -1000 Address RAMPART-427 review: document init trade-off, guard config parsing Follow-up to the Gemini review of the RAMPART-427 fix (both findings are pre-existing in RampartMessageData, not introduced by the race fix): - Document the performance-vs-correctness trade-off of running the WSS4J / Santuario / OpenSAML one-time initialisers in the per-message constructor: the calls are idempotent guards kept there to avoid the OpenSAML-5-migration unmarshallerFactory ordering bug; moving them to a once-per-lifecycle init is a separate, riskier change. No behaviour change. - Wrap the timestampTTL / timestampMaxSkew Integer.parseInt calls so a non-numeric RampartConfig value throws a clear RampartException naming the offending parameter and value, instead of an unhandled NumberFormatException. Adds the invalidRampartConfigValue message. (The reviewer's third, JUnit-version, note is skipped to stay consistent with the existing test suite style.) Verified with a full clean -Papache-release verify across all modules including the nine policy samples on JDK 25. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> --- .../org/apache/rampart/RampartMessageData.java | 30 +++++++++++++++++++--- .../resources/org/apache/rampart/errors.properties | 1 + 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/modules/rampart-core/src/main/java/org/apache/rampart/RampartMessageData.java b/modules/rampart-core/src/main/java/org/apache/rampart/RampartMessageData.java index 8ba45a90..66ea13ac 100644 --- a/modules/rampart-core/src/main/java/org/apache/rampart/RampartMessageData.java +++ b/modules/rampart-core/src/main/java/org/apache/rampart/RampartMessageData.java @@ -236,8 +236,20 @@ public class RampartMessageData { try { - // CRITICAL FIX: Initialize WSS4J before creating WSSConfig to ensure OpenSAML integration works - // This prevents OpenSAMLUtil.unmarshallerFactory from being null when processing SAML assertions + // PERFORMANCE vs CORRECTNESS: + // The WSS4J / Santuario / OpenSAML providers below are one-time, process-wide + // initialisers. Invoking them in this per-message constructor is intentional but + // is a correctness-over-performance trade-off: + // * Correctness: it guarantees the SAML stack is initialised before any + // assertion is processed. Without it OpenSAMLUtil.unmarshallerFactory could be + // null (an initialisation-ordering problem surfaced during the OpenSAML 5 / + // Jakarta migration), causing SAML processing to fail intermittently. + // * Performance: all of these calls are idempotent guards, so the steady-state + // cost is only the guard checks rather than real re-initialisation - but they + // still run on every message, which is wasteful under high throughput. + // The proper fix is to run this once per application lifecycle (e.g. in the + // Rampart/Rahas module init), which is tracked separately; do not move it without + // re-verifying the unmarshallerFactory ordering issue does not return. if (log.isDebugEnabled()) { log.debug("WSS4J initialization starting"); } @@ -391,12 +403,22 @@ public class RampartMessageData { if (policyDataRampartConfig != null) { String timeToLiveString = policyDataRampartConfig.getTimestampTTL(); if (timeToLiveString != null && !timeToLiveString.equals("")) { - this.setTimeToLive(Integer.parseInt(timeToLiveString)); + try { + this.setTimeToLive(Integer.parseInt(timeToLiveString)); + } catch (NumberFormatException e) { + throw new RampartException("invalidRampartConfigValue", + new String[]{"timestampTTL", timeToLiveString}, e); + } } String maxSkewString = policyDataRampartConfig.getTimestampMaxSkew(); if (maxSkewString != null && !maxSkewString.equals("")) { - this.setTimestampMaxSkew(Integer.parseInt(maxSkewString)); + try { + this.setTimestampMaxSkew(Integer.parseInt(maxSkewString)); + } catch (NumberFormatException e) { + throw new RampartException("invalidRampartConfigValue", + new String[]{"timestampMaxSkew", maxSkewString}, e); + } } } diff --git a/modules/rampart-core/src/main/resources/org/apache/rampart/errors.properties b/modules/rampart-core/src/main/resources/org/apache/rampart/errors.properties index cf5ed2b4..b4df6ed2 100644 --- a/modules/rampart-core/src/main/resources/org/apache/rampart/errors.properties +++ b/modules/rampart-core/src/main/resources/org/apache/rampart/errors.properties @@ -16,6 +16,7 @@ missingConfiguration = Missing or malformed configuration: \"{0}\" +invalidRampartConfigValue = Invalid value \"{1}\" for Rampart configuration parameter \"{0}\" expectedParameterMissing = Expected parameter missing : \"{0}\" missingScopeValue = Missing or incorrect scope value canotFindContextIdentifier = Cannot find context identifier
