Close out the remaining small note-tier CodeQL categories

[vharseko]

Summary

Follow-up to the unused-parameter cleanup (#141): investigated all the newly-surfaced small note-severity CodeQL categories.

• GuardedString now overrides toString() (returns "GuardedString(...)"), so it no longer inherits Object's default. Fixes call-to-object-tostring at its root instead of patching the two current call sites (SharedSecretPrincipal, ScriptOnResourceApiOpTests) individually — any future logging of a GuardedString is safe by construction. This is more than cosmetic: the inherited output printed hashCode(), which is derived from the unsalted SHA-1 hash of the clear text, so a fingerprint of the secret reached log text — through SharedSecretPrincipal.toString(), and, with OK-level logging on, through LoggingProxy, which prints every API argument and return value: a GuardedString attribute value (e.g. __PASSWORD__) passed to create/update or returned by getObject was printed via Attribute.toString().
• GuardedByteArray gets the same override ("GuardedByteArray(...)"): its hashCode() is derived from the clear bytes the same way, and it is a supported attribute type, so the same LoggingProxy → Attribute.toString() path printed it.
• Fixes a shadowed local in LdapInternalSearch.execute() (local-shadows-field).

Also dismissed on GitHub: 3 ignored-error-status-of-call (the ignored return values are already covered by a subsequent check or exception path), 8 jdk-internal-api-access (AD DirSync's com.sun.jndi.ldap.Ber*, no public JDK alternative exists), 1 confusing-method-signature (Log.log(..) is long-standing, heavily-used public API — renaming to remove the overload is too invasive for the benefit).

Covered by other PRs instead of this one:

• Fix mechanical CodeQL note findings: reflection, boxing, deprecated closes #134 (opened independently, earlier) fixes byte-for-byte the same lines in AttributeTypeUtil.java, MultiOpTests.java, PrettyStringBuilder.java, StringUtil.java, ActiveDirectoryChangeLogSyncStrategy.java, XSDAnnotationParser.java (with AttributeTypeUtilTests.java): uncaught-number-format-exception, inefficient-boxed-constructor, inefficient-empty-string-test, inefficient-key-set-iterator, unknown-javadoc-parameter, missing-space-in-concatenation. Dropped here on 2026-09-21.
• Fix the error-level CodeQL findings: dead branches, a null factory, interned-string locks, racy lazy init #132 removes the dead ContractTestFactory inner class from ContractITCase (unused-reference-type) along with more of that file's dead code; the two PRs conflicted there, so this PR no longer touches ContractITCase. Dropped here on 2026-10-04.

Update 2026-10-02 (review round 1): rebased onto the current master; added the GuardedByteArray override; replaced the toString() test, which passed even without the override, by one that pins the fix; reworded the Javadoc to say why the override matters.

Update 2026-10-04 (review round 2): rebased onto the current master (picks up #146, which fixes the docs.groovy-lang.org javadoc failure that turned the windows-latest, 11 cell red); both toString() tests now also pin the exact output; corrected the description of the GuardedByteArray leak (it was reachable, not latent); dropped the ContractITCase change in favour of #132; aligned the added 3A copyright lines to the repository's Portions Copyrighted 2026 3A Systems, LLC.

Test plan

• GuardedStringTests.testToStringDoesNotDependOnTheSecret and GuardedByteArrayTests.testToStringDoesNotDependOnTheSecret: two different secrets print the same toString(), and it is exactly "GuardedString(...)" / "GuardedByteArray(...)". Verified red with the overrides removed (round 1) and with toString() returning null / "" (round 2).
• connector-framework test suite after round 2 — 194 tests, 0 failures.
• connector-framework, connector-framework-contract, OpenICF-ldap-connector build after round 2.
• mvn install on connector-framework, connector-framework-contract, OpenICF-ldap-connector (incl. existing test suites, LDAP/OpenDJ integration tests included) — all green, 946 tests, 0 failures (before the review rounds).

[maximthomas]

praise: Fixing call-to-object-tostring in GuardedString itself, not at the two call sites, closes more than the CodeQL note.

• The inherited Object.toString() printed Integer.toHexString(hashCode()), and GuardedString.hashCode() is base64SHA1Hash.hashCode() (GuardedString.java:292), 32 bits of the unsalted SHA-1 of the clear text. SharedSecretPrincipal.toString() put that value into log text. The new toString() (GuardedString.java:299-302) removes it for every caller.
• The LdapInternalSearch.execute() rename and the ContractTestFactory removal change no behaviour. The renamed local has one read, the doSearch argument (LdapInternalSearch.java:63-65). The removed class was private and unreferenced, and all 9 build-maven jobs compile the result.

---

suggestion (non-blocking): testToStringNeverExposesTheClearText stays green with the toString() override deleted.

OpenICF-java-framework/connector-framework/src/test/java/org/identityconnectors/common/security/GuardedStringTests.java:126-131

The inherited output, org.identityconnectors.common.security.GuardedString@<hex>, can never contain "secret", so the assertion holds with or without the override. Measured: with GuardedString.java:294-302 deleted at bcf6509, GuardedStringTests runs 6 cases with 0 failures. The new behaviour — the output no longer depends on the secret — has no test. A CodeQL re-scan would catch a revert. This test would not.

    @Test
    public void testToStringDoesNotDependOnTheSecret() {
        GuardedString first = new GuardedString("secret".toCharArray());
        GuardedString second = new GuardedString("other".toCharArray());
        assertEquals(first.toString(), second.toString(),
                "toString() must not carry anything derived from the clear text");
    }

Pin: with the override removed, the two strings differ in their hashCode() suffix, so this case turns red (reasoned, not run).

---

suggestion (non-blocking): GuardedByteArray still inherits Object.toString(), which prints the same secret-derived hash.

OpenICF-java-framework/connector-framework/src/main/java/org/identityconnectors/common/security/GuardedByteArray.java:49, :270-271

GuardedByteArray.hashCode() returns base64SHA1Hash.hashCode(), and the hash comes from SecurityUtil.computeBase64SHA1Hash (:251). The PR's "safe by construction" argument applies to this class unchanged. Nothing at head stringifies a GuardedByteArray, so the leak is latent. The first log line that does will expose the fingerprint, and only then will CodeQL flag it. This can go in this PR or a follow-up.

    @Override
    public String toString() {
        return "GuardedByteArray(...)";
    }

---

nitpick (non-blocking): The new Javadoc calls the inherited output harmless, but it carried a fingerprint of the secret.

OpenICF-java-framework/connector-framework/src/main/java/org/identityconnectors/common/security/GuardedString.java:295-298

"its output is just a class name and hash code, not useful for logging" leaves out that the hash code is derived from the clear text (:292, SecurityUtil.java:136-144). A maintainer could read the override as cosmetic and remove it, and no test would fail.

    /**
     * Never prints the clear text, nor anything derived from it: the inherited
     * {@link Object#toString()} would print {@link #hashCode()}, which is computed
     * from the SHA-1 hash of the clear text.
     */

[vharseko]

@maximthomas all three points are addressed in 58bdfdb (the branch is also rebased onto the current master):

• Test that did not pin the fix: testToStringNeverExposesTheClearText is replaced by testToStringDoesNotDependOnTheSecret, which asserts that two different secrets print the same toString(). Verified red with the override removed: expected [...GuardedString@22c507a1] but found [...GuardedString@3a9f227a].
• GuardedByteArray: now overrides toString() ("GuardedByteArray(...)") too, with the same test in GuardedByteArrayTests (also verified red without the override).
• Javadoc: reworded as you suggested on both classes. It now says that the inherited output printed hashCode(), which is computed from the SHA-1 hash of the clear text.

connector-framework mvn install: 194 tests, 0 failures.

[maximthomas]

praise: Round 1's follow-ups all landed, and the new tests pin the fix.

• GuardedByteArray.toString() (GuardedByteArray.java:279-282) removes the same SHA-1 fingerprint as GuardedString.toString(), and both Javadocs now say why (GuardedString.java:295-299).
• testToStringDoesNotDependOnTheSecret in GuardedStringTests and GuardedByteArrayTests goes red with the overrides removed (Tests run: 12, Failures: 2). The PR body's "verified red" claim holds.

---

suggestion (non-blocking): testToStringDoesNotDependOnTheSecret still passes when toString() returns null, "" or any other constant.

OpenICF-java-framework/connector-framework/src/test/java/org/identityconnectors/common/security/GuardedStringTests.java:127-132, GuardedByteArrayTests.java:128-133

Both cases only compare the outputs for two different secrets. Measured at 58bdfdb: with GuardedString.toString() returning null and GuardedByteArray.toString() returning "", both classes run 12 cases with 0 failures, because assertEquals(null, null) holds. These mutants leak nothing, so the hardening is pinned and the output itself is not.

        assertEquals(first.toString(), "GuardedString(...)");

Pin: add this line to GuardedStringTests, and assertEquals(first.toString(), "GuardedByteArray(...)") to GuardedByteArrayTests. The null/"" mutants then fail.

---

suggestion (non-blocking): The description says the GuardedByteArray leak was latent. OK-level logging already reached it.

OpenICF-java-framework/connector-framework/src/main/java/org/identityconnectors/framework/common/FrameworkUtil.java:268, OpenICF-java-framework/connector-framework-internal/src/main/java/org/identityconnectors/framework/impl/api/LoggingProxy.java:61-72, OpenICF-java-framework/connector-framework/src/main/java/org/identityconnectors/framework/common/objects/Attribute.java:154-162

GuardedByteArray is an accepted attribute value (ATTR_SUPPORTED_TYPES). When OK-level logging is on, LoggingProxy appends every API argument. create/update therefore print Set<Attribute>, Attribute.toString() prints getValue(), and before this PR that printed Object.toString(), which contains hashCode(). getObject reaches it through "Return: " + ret and ConnectorObject.toString(). Round 1 of this review called the leak latent, and that was wrong. The body becomes the squash commit message, so it is worth correcting there.

- **`GuardedByteArray`** gets the same override (`"GuardedByteArray(...)"`): its `hashCode()` is derived from the clear bytes the same way. With OK-level logging on, `LoggingProxy` printed it through `Attribute.toString()` whenever a `GuardedByteArray` attribute value was passed to create/update or returned by getObject.

Or: in the 2026-09-21 update, also drop "this PR now only carries the 3 fixes unique to it", since the diff carries 4.

---

suggestion (non-blocking): ContractITCase.java conflicts with open #132, which removes the same ContractTestFactory class.

OpenICF-java-framework/connector-framework-contract/src/main/java/org/identityconnectors/contract/test/ContractITCase.java:24

#132 (head 9aa4f8d) removes the same class and overlapping imports, and adds its own 3A header line at the same spot. Each PR merges cleanly onto master on its own. Whichever merges second hits CONFLICT (content) in this file, in either order (git merge-tree 9aa4f8d 58bdfdb). #132 covers this removal, so the ContractITCase hunks could be dropped from this PR. Otherwise the second PR needs a rebase. Aligning the header line alone does not avoid the conflict, because the import hunks overlap too.

---

nitpick (non-blocking): The PR adds two off-pattern spellings of the 3A copyright line.

OpenICF-java-framework/connector-framework/src/test/java/org/identityconnectors/common/security/GuardedStringTests.java:23, OpenICF-java-framework/connector-framework-contract/src/main/java/org/identityconnectors/contract/test/ContractITCase.java:24, OpenICF-ldap-connector/src/main/java/org/identityconnectors/ldap/search/LdapInternalSearch.java:23, OpenICF-java-framework/connector-framework/src/test/java/org/identityconnectors/common/security/GuardedByteArrayTests.java:23

The first three read Portions Copyrighted 2026 3A Systems LLC., and GuardedByteArrayTests reads Portions Copyright 2026 3A Systems, LLC.. The repo's form, used 155 times and by GuardedString.java:22 and GuardedByteArray.java:22, is:

 * Portions Copyrighted 2026 3A Systems, LLC

[vharseko]

@maximthomas all five points are addressed in b9e0f90 (the branch is also rebased onto the current master, which picks up #146 — that was the docs.groovy-lang.org javadoc failure in the windows-latest, 11 cell):

• Test passing for any constant: both testToStringDoesNotDependOnTheSecret cases now also assert the exact output, "GuardedString(...)" / "GuardedByteArray(...)". Verified with the mutants you measured: GuardedString.toString() returning null and GuardedByteArray.toString() returning "" now fail (expected [GuardedString(...)] but found [null], expected [GuardedByteArray(...)] but found []).
• "Latent" was wrong: confirmed the LoggingProxy → Attribute.toString() path (FrameworkUtil.java:268, LoggingProxy.java:61-78, Attribute.java:154-162). The description now says so for GuardedByteArray — and for GuardedString too, since the same path printed e.g. a __PASSWORD__ value on create/update, beyond the SharedSecretPrincipal case the description named before.
• Conflict with Fix the error-level CodeQL findings: dead branches, a null factory, interned-string locks, racy lazy init #132: Fix the error-level CodeQL findings: dead branches, a null factory, interned-string locks, racy lazy init #132's ContractITCase change is a superset of this one, so this PR no longer touches ContractITCase; the description now lists unused-reference-type as covered by Fix the error-level CodeQL findings: dead branches, a null factory, interned-string locks, racy lazy init #132. That also settles the "3 fixes" vs 4 point: the unique fixes are again three (GuardedString, GuardedByteArray, LdapInternalSearch).
• Copyright lines: the added lines in GuardedStringTests, GuardedByteArrayTests and LdapInternalSearch now read Portions Copyrighted 2026 3A Systems, LLC.

connector-framework test suite: 194 tests, 0 failures.

[maximthomas]

praise: The fix sits where the leak was, and the tests now pin it exactly.

• GuardedString.toString() and GuardedByteArray.toString() (OpenICF-java-framework/connector-framework/src/main/java/org/identityconnectors/common/security/GuardedString.java:301, GuardedByteArray.java:280) close call-to-object-tostring at the root: no Guarded* value prints its SHA-1-derived hashCode() through LoggingProxy → Attribute.toString() any more.
• testToStringDoesNotDependOnTheSecret asserts the exact "GuardedString(...)" / "GuardedByteArray(...)" (GuardedStringTests.java:132, GuardedByteArrayTests.java:133), so removing the override or returning null, "" or another constant turns it red.
• Leaving ContractITCase to #132 keeps the PR conflict-free: it merges cleanly into master at 1abfe741.