Regressions in the PasswordClass hash algorithm rework

Context

XWIKI-24357 ("Improve PasswordClass hash algorithm") reworked password hashing on top of the Spring Security encoders. It is released (18.4.5 on 2026-09-08 and 18.8.0-rc-1 on 2026-09-17), so the defects below are in users' hands and are reported here rather than by reopening XWIKI-24357.

They were found during a code review of the change, not by a user report. The first two have a user-visible impact; the others are correctness and diagnosability problems in the same code. Line numbers are those of master as of 2026-09-21.

1. The clear-text password check became case insensitive

BaseCollection#isPasswordValueMatching (BaseCollection.java:681) compares a clear-stored password with:

result = Strings.CI.equals(passwordValue, rawPassword);

Strings.CI is the case insensitive comparator. This branch is the legacy fallback taken when the password property is a StringProperty rather than a PasswordProperty — which is the case for every user document created before the PasswordProperty type was introduced — and it is now the single check behind both XWikiAuthServiceImpl#checkPassword and com.xpn.xwiki.api.User#checkPassword.

The code it replaced was case sensitive:

result = new PasswordClass().getEquivalentPassword(stored, password).equals(stored);

and PasswordClass#arePasswordsMatching uses Strings.CS for the very same situation. So a wiki configured with the Clear storage type now accepts SECRET where the stored password is secret.

Expected: use Strings.CS, as the surrounding code does.

2. setPasswordValue throws ClassCastException on a legacy password property

BaseCollection#setPasswordValue (BaseCollection.java:380) casts before it checks:

PasswordProperty property = (PasswordProperty) safeget(name);

if (!(property instanceof PasswordProperty)) {
    if (property != null) {
        // Make sure to delete the property if it's not the right type
        removeField(name);
    }
    property = new PasswordProperty();
}

The instanceof test is unreachable: the cast on the first line already throws when the existing property is not a PasswordProperty. The method is a copy of setStringValue, where the cast target is the wider BaseStringProperty.

The case the dead branch means to handle is exactly the one isPasswordValueMatching has a dedicated branch for — a password stored as a StringProperty — so setting a password on a pre-existing user document fails with a ClassCastException instead of migrating the property.

Expected: cast to BaseStringProperty (or drop the cast and use a pattern variable), so the replacement branch is reachable.

3. A single static exception instance is thrown from two places

PasswordClass declares (PasswordClass.java:194):

private static final IllegalArgumentException ILLEGAL_ARGUMENT_EXCEPTION_UNKNOWN_FORMAT =
    new IllegalArgumentException("The provided encoded password doesn't match any known hash encoded password format.");

and throws that same instance from getAlgorithmFromPassword (:382) and arePasswordsMatching (:451).

The stack trace an administrator sees is therefore the one captured during class initialisation, pointing at the static initialiser instead of at the call that failed, which makes any "unknown hash format" report impossible to locate. The instance is also shared across threads, so its cause and suppressed exceptions are shared state.

Expected: throw a new exception at each site, e.g. through a small private unknownFormat() factory method.

4. isPasswordHashed throws NullPointerException on a null value

PasswordClass#isPasswordHashed (PasswordClass.java:238) is public API (@Unstable, @since 18.8.0RC1, also called by PasswordPropertyParser):

return value != null && value.startsWith(HASH_IDENTIFIER + SEPARATOR) || HASH_PATTERN.matcher(value).matches();

&& binds tighter than ||, so for a null value the first operand is simply false and evaluation continues into HASH_PATTERN.matcher(null), which throws. The null check advertises a null-safety the method does not have.

Note that HASH_PATTERN already accepts the hash: prefix as one of its alternatives, so the whole body can be value != null && HASH_PATTERN.matcher(value).matches(), which both fixes the NPE and removes the redundant test.

5. The encoder is resolved twice and the deprecation warning is logged twice

PasswordClass#getPasswordHash(String, String) (PasswordClass.java:394):

PasswordEncoder passwordEncoder = getPasswordEncoder(algorithmName);
String encodedPassword = ENCODERS_MAP.get(algorithmName).encode(password);
if (passwordEncoder.upgradeEncoding(encodedPassword) || isDeprecatedEncoder(passwordEncoder.getClass())) {
    warnAboutOutdatedAlgorithm(algorithmName);
}

The encoder resolved on the first line is then ignored in favour of a second lookup in the map, and getPasswordEncoder already logs warnAboutOutdatedAlgorithm when the encoder is deprecated. Since MessageDigestPasswordEncoder (SHA-1, SHA-256, SHA-512) is deprecated, hashing with any of those logs the same warning twice per call.

Expected: use the encoder already resolved, and drop the second isDeprecatedEncoder test.

6. Smaller points in the same change

• PasswordClass#warnAboutOutdatedAlgorithm (:461) calls LOGGER.error(..., new Exception()) to obtain a stack trace, for the legitimate case of a property not yet attached to an object. That should not be an error, and should not fabricate a throwable.
• getAlgorithmFromPassword (:370) and arePasswordsMatching (:433) repeat the same three-branch dispatch (clear / legacy hash: prefix / regex match, else throw). A single private parse method returning the algorithm id, the hash and the encoder would remove the duplication.
• R180800000XWIKI24357DataMigration opens a transaction around each batch (:99) and then performs one hibernateStore.executeWrite per row inside it (:125); each row opens its own transaction, so the outer one has no effect and a batch is not atomic. The migration also hardcodes the "hash:" prefix rather than reusing the format knowledge that XWikiLegacyPasswordEncoder owns, and issues one UPDATE per password.
• XWikiLegacyPasswordEncoder#reencodePassword produces {{{} {XWikiLegacy}

  SHA-1::$argon2id$...{}}} for an unsalted legacy password, and parsing that back leaves the empty salt's separator glued to the front of the hash. It round-trips today only because Spring's Argon2EncodingUtils ignores everything before the first $. It is worth either normalising the format or recording that dependency in a comment, since nothing in the code states it.

1. 1. 7. Saving a user object/inline form writes the ******** placeholder back as the password (most security-impactful of this set)

Same XWIKI-24357 change, found in the follow-up review. Line numbers are master at 6cd8f754da6.

PasswordClass#fromString (PasswordClass.java#L216-L224) no longer ignores the form placeholder:

public BaseProperty fromString(String value) throws XWikiException
{
    BaseProperty property = newProperty();
    if (value.isEmpty() || isPasswordHashed(value)) {
        property.setValue(value);
    } else {
        property.setValue(getProcessedPassword(value));
    }
    return property;
}

The removed guard was if (FORM_PASSWORD_PLACEHODLER.equals(value)) return null;. displayEdit still pre-fills every password input with ******* (PasswordClass.java:256 and :270), and BaseClass#fromMap (BaseClass.java:408-428) skips a property only when fromString returns null. So any save of an object/inline form that contains a password field the editor did not retype stores ******* (hashed) as the new password. The validkey field is affected the same way.

Impact: an administrator editing a user through the object editor (for example to change the email), or any inline/object-form save of a user document whose password field shows the placeholder, silently changes that user's password to the known string ********; anyone can then authenticate as that user. Released regression, 18.4.5 / 18.8.0-rc-1, same as the points above.

Reproduced live on 18.9.0-SNAPSHOT: an admin opened XWiki.<user> in the object editor, changed only the email and saved; afterwards login with ******** succeeded and the original password failed (it failed before the save).

Expected: restore the placeholder check in fromString (return null for FORM_PASSWORD_PLACEHODLER), and add a unit test asserting fromString("********") yields null.