Skip to content

Fix CodeQL warning-level Java correctness findings - #207

Open
vharseko wants to merge 5 commits into
OpenIdentityPlatform:masterfrom
vharseko:fix-codeql-warnings-java-correctness
Open

vharseko wants to merge 5 commits into
OpenIdentityPlatform:masterfrom
vharseko:fix-codeql-warnings-java-correctness

Conversation

@vharseko

@vharseko vharseko commented Sep 18, 2026

Copy link
Copy Markdown
Member

Summary

First of the warning-level CodeQL batches: the Java findings that touch correctness (null dereferences, resource leaks, switch coverage, equality, serialization). 18 alerts fixed here; #510 (ConnectorReference) was dismissed as a false positive (StringUtil.isBlank(null) handles the null before the dereference), and #515/#518 (AbstractScriptedService) are already fixed by #205.

Three of these are real bugs (bug below); the rest are cleanups that also remove the guards which misled the analysis.

Alerts File Change
#506–#508 dereferenced-value-may-be-nullbug ObjectMapping If a SynchronizationException was raised before op/event were created (e.g. bad params), the op != null && … guard meant it was neither logged nor rethrown, and setLogEntryMessage(null, se) then threw an NPE. Now op == null || op.action != EXCEPTION reports it like any other failure, and the audit block requires both op and event.
#516 dereferenced-value-may-be-nullbug DateUtil.getDateDifferenceInDays Integer result = null; … return result; unboxed to an NPE when either date was null. Now an explicit IllegalArgumentException, computed as int throughout.
#495/#496 missing-no-arg-constructor-on-serializablebug JsonUser, JsonGroup Both are Serializable through Activiti's User/Group, but JsonValue has no no-arg constructor, so Java deserialization failed with InvalidClassException: no valid constructor. They now implement Externalizable (the JSON map is written/read); JsonUser.cryptoService is transient (it is an OSGi service — re-attach with setCryptoService). CodeQL keeps flagging the fixed classes (#926/#927): the query only looks for a no-arg constructor on the non-serializable super-class, while Externalizable deserialization uses the class's own public no-arg constructor — dismissed as a false positive.
#500–#502 input-resource-leak SourceGenerator Three unclosed BufferedReaders over classpath resources → one readResource() helper with try-with-resources.
#503 / #504 input/output-resource-leak ConnectorInfoProviderService, JSONConfigInstaller JarInputStream and FileOutputStream are now closed on every path, including the one where the JarInputStream constructor itself fails while parsing the manifest — the underlying FileInputStream is a resource of its own.
#469 non-short-circuit-evaluation ManagedObjectSet ||| in the name validation.
#489 reference-equality-on-strings Link.targetEquals In the null branch normalizedTargetId == normalizedCompId is just normalizedCompId == null; written that way.
#487 / #488 missing-case-in-switch SyncOperation, RepoJobStore default: for the inner action switch (only reachable for the outer cases) and for CUSTOM cluster events (debug log).
#505, #509, #511–#514, #517 dereferenced-value-may-be-null / useless-null-check JSONConfigInstaller, SyncOperation, ConnectorUtil, ObjectClassResourceProvider, IdentityProviderService Removed null checks on values that cannot be null (pid, sourceClass = source.getClass(), a final map field) and made the reauthCreds != null ↔ runAsAttributes correlation explicit.
#708 field-masks-super-field ServiceTrackerNotifier Dropped the context field that shadowed ServiceTracker's own protected final context.
#709 unsafe-get-resource AuditServiceImpl getClass().getResourceAsStreamAuditServiceImpl.class.getResourceAsStream.

Note: fix-codeql-error-alerts (#205) is merged into this branch, so the error-level java/missing-clone-method alert on JsonUser (master #491, re-reported here as #928 because this PR touches the class declaration) is fixed here as well. Its commits drop out of this diff as soon as #205 lands on master.

The four CodeQL alerts raised on this PR (#925–#928) are analysed and dispositioned in #220.

Test plan

  • DateUtilTest +2 (missing start / end → IllegalArgumentException; failed with NPE before)
  • New ActivitiIdentitySerializationTestJsonUser and JsonGroup survive an ObjectOutputStream/ObjectInputStream round trip (failed with InvalidClassException / NotSerializableException before)
  • New ConnectorInfoProviderServiceJarListingTest — listing a jar with a truncated manifest (the JarInputStream constructor fails) leaves no open file descriptor, measured through UnixOperatingSystemMXBean and skipped on Windows JVMs; before 5417702 it leaked one descriptor per call
  • Suites of the touched modules: config 11, core 79, provisioner-openicf 136, util 61, quartz-fragment 8, workflow-activiti 3, audit 36, identity-provider 1 — 0 failures; custom-scripted-connector-bundler compiles (no tests)
  • After the Fix CodeQL error-level findings: dead branches, log arguments, locking, clone #205 merge: mvn -pl openidm-workflow-activiti,openidm-provisioner-openicf -am test -Dtest=JsonUserTest,ActivitiIdentitySerializationTest — 4/4 pass (clone() and the serialization round trip), both modules recompile
  • CodeQL on this PR closes the alerts listed in the commit message

…g, clone

- ReconciliationService: drop the unreachable ScheduledThreadPoolExecutor
  branches (it is a ThreadPoolExecutor)
- Fix SLF4J placeholder/argument mismatches in ClusterManager,
  ConnectorInfoProviderService, Id, GenericTableHandler,
  MappedTableHandler and PostgreSQLMappedTableHandler
- ExplicitResultSetMapper: the "total" guard compared a String against a
  List<ColumnMapping> and never matched; check the mapped DB column names
- AbstractScriptedService: synchronize on a dedicated lock instead of the
  (possibly null) registration field, make it volatile and clear it after
  unregistering; the old code threw NPE on every REGISTERED script event
- JsonUser / ChecksumFile: override clone() so the copy keeps its type
  (and, for JsonUser, its CryptoService)
- OrientDBRepoService: back off between pool-acquire retries outside the
  pool lock
- Remove the write-only containers in OperationHelperImpl and RepoJobStore

Resolves CodeQL alerts #470-#486, #490-#494, #497-#499.
- ObjectMapping: a SynchronizationException raised before the operation
  was set up was silently swallowed and then dereferenced a null audit
  event; report it like any other failure and guard the audit block
- DateUtil.getDateDifferenceInDays: reject missing dates explicitly
  instead of failing with an unboxing NullPointerException
- JsonUser / JsonGroup: implement Externalizable so the Serializable
  contract inherited from Activiti's User/Group actually works
- Close the readers/streams leaked in SourceGenerator,
  ConnectorInfoProviderService and JSONConfigInstaller
- ManagedObjectSet: short-circuit the name validation; Link: drop the
  reference comparison of strings
- Add default branches to the SyncOperation and RepoJobStore switches
- Remove the null guards that could never trigger (and misled the
  analysis), the field shadowing ServiceTracker.context, and use
  AuditServiceImpl.class for the resource lookup

Resolves CodeQL alerts #469, #487-#489, #495, #496, #500-#509,
#511-#514, #516, #517, #708, #709.
@vharseko vharseko added java Pull requests that update Java code test Tests and test infrastructure (unit, e2e, smoke) bug Something isn't working refactor Code refactoring without behavior change workflow Activiti workflow engine / scripting connector OpenICF connectors / provisioner labels Sep 18, 2026
…correctness

Brings in the JsonUser.clone() override from OpenIdentityPlatform#205 so the error-level
CodeQL alert java/missing-clone-method no longer fires on the class
declaration line this branch touches.

# Conflicts:
#	openidm-workflow-activiti/src/main/java/org/forgerock/openidm/workflow/activiti/impl/JsonUser.java
try-with-resources closes only the resource variable, so an IOException
from the JarInputStream constructor (it reads and parses the manifest)
left the FileInputStream open - CodeQL alert #503 survived the change and
was re-reported on the new line as #925. Declare both streams as
resources, the way JSONConfigInstaller already does in this branch, and
drop the explicit close that try-with-resources makes redundant.
@vharseko vharseko added repo Storage / repository layer (OrientDB, JDBC, HSQLDB) concurrency Thread-safety, locking and synchronization issues labels Sep 19, 2026
…rService

A jar whose manifest entry is truncated makes the JarInputStream
constructor fail. The test lists it ten times and checks through
UnixOperatingSystemMXBean that no file descriptor stays open; it is
skipped on JVMs that do not expose the count (Windows). Before
5417702 it failed with one leaked descriptor per call (OpenIdentityPlatform#220).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working concurrency Thread-safety, locking and synchronization issues connector OpenICF connectors / provisioner java Pull requests that update Java code refactor Code refactoring without behavior change repo Storage / repository layer (OrientDB, JDBC, HSQLDB) test Tests and test infrastructure (unit, e2e, smoke) workflow Activiti workflow engine / scripting

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants