Skip to content

Wrap malformed numeric values in a domain exception instead of a bare NumberFormatException - #139

Open
vharseko wants to merge 1 commit into
OpenIdentityPlatform:masterfrom
vharseko:codeql-uncaught-number-format
Open

vharseko wants to merge 1 commit into
OpenIdentityPlatform:masterfrom
vharseko:codeql-uncaught-number-format

Conversation

@vharseko

Copy link
Copy Markdown
Member

Closes 48 of the 54 remaining java/uncaught-number-format-exception alerts (the 8 in AttributeTypeUtil were already fixed in #134). Dismisses the remaining 6 as unreachable in practice, with the reasoning recorded on each alert.

AD/LDAP cluster — 26 of 28 sites

ADUserAccountControl, ADGroupType and ActiveDirectoryChangeLogSyncStrategy share one root cause: Active Directory bitmask attributes (userAccountControl, groupType) and sync tokens are parsed with plain Integer.parseInt, nothing around it. ADLdapUtil gets parseADInteger/parseADLong (new, unit-tested), and all three classes route through it — a corrupted AD attribute, or a sync token tampered with by whoever calls the connector's sync() operation, now fails with a ConnectorException naming the value instead of a bare NumberFormatException three frames deep with no context.

PagedSearchStrategy's paged-results cookie is <base64 cookie>:<context index>; the base64 half was already inside a try/catch(RuntimeException) reporting ConnectorException, the numeric half was one line below it, outside. Moved in.

Left alone and dismissed as unreachable (ADLdapUtil.binarySIDtoString, 2 sites): the hex strings it parses are built two lines above via String.format("%02X...") from raw bytes, so they are always valid hex.

Elsewhere — 22 sites, same idiom as what already surrounds them

  • PropertyBag.castValue (maven-plugin, 8): POM <configuration> values converted to typed connector config properties at build time; the numeric branches are wrapped, reporting through the MojoExecutionException the method already uses two lines above for the equivalent blank-value case.
  • SQLUtil.attribute2jdbcValue (dbcommon, 6): converts an attribute value to a JDBC-bindable value for a numeric SQL column; wrapped with ConnectorException, already imported and used elsewhere in the file.
  • XmlObjectDecoder (connector-framework-internal, 5): the five primitive decoders of the XML wire protocol — the one genuinely network-facing trust boundary in this batch. Wrapped with ConnectorException, matching decodeClass's existing convention two methods above.
  • RemoteWSFrameworkConnectionInfo.loadSystemProxy (1): a typo in -Dhttp.proxyPort now fails clearly at startup instead of with a bare NFE.
  • ConnectorHelper / AuthenticationApiOpTests (connector-framework-contract, 2): contract-test configuration (-DserverPort, a test-suite attribute) now reports through ContractException, the framework's own convention, instead of a bare NFE.

Dismissed as unreachable — 6

  • ADLdapUtil.binarySIDtoString ×2 (see above).
  • TstStatefulConnectorConfig (testbundlev1): parses a revision the same test connector generated two lines above via AtomicInteger.getAndIncrement()/String.valueOf() — no external input.
  • JavaScriptExecutorFactory: parses the JVM's own java.specification.version, guaranteed valid by the JDK.
  • CSVFileConnector ×2: parses a substring the regex \.[0-9]{13}$ just matched immediately before — always exactly 13 ASCII digits.

Tests

New: ADLdapUtilTest (5), ADUserAccountControlTest (2), ADGroupTypeTest (2), PagedSearchStrategyTest (1), SQLUtilTests (+3), XmlObjectDecoderTest (4, corrupts a value inside a real serialized document rather than hand-building the XML schema), RemoteWSFrameworkConnectionInfoTest (1) — 18 new/changed tests, each RED (bare NumberFormatException) before its fix and green after. PropertyBag has no test: the module has no test harness (same as in #126/#132). ConnectorHelper/AuthenticationApiOpTests are contract-test configuration parsers with no dedicated unit tests of their own; verified by compiling and by the module's existing suite.

Local runs of all six touched modules, all green: dbcommon 89, connector-framework-internal 473 (2 skipped, same as on master), connector-framework-contract 43, connector-framework-server 30, ldap-connector 169 (embedded OpenDJ), maven-plugin compiles.

… NumberFormatException

Active Directory bitmask attributes (userAccountControl, groupType) and
sync tokens share one root cause across ADUserAccountControl, ADGroupType
and ActiveDirectoryChangeLogSyncStrategy: Integer.parseInt with nothing
around it. ADLdapUtil gets parseADInteger/parseADLong, and all three
classes route through it, so a corrupted AD attribute or a tampered sync
token fails with a ConnectorException naming the value instead of a bare
NumberFormatException three frames deep. PagedSearchStrategy's cookie
parsing moves into the try/catch that already exists for the other half
of the same cookie.

The maven plugin's POM property conversion (PropertyBag), the JDBC value
conversion for numeric SQL types (SQLUtil.attribute2jdbcValue) and the
five primitive decoders of the XML wire format (XmlObjectDecoder) get the
same treatment, each in the exception type already used around them
(MojoExecutionException, ConnectorException). The proxy port system
property and two contract-test configuration parsers get a one-line wrap
with the same reasoning.

Left alone, and dismissed on GitHub as unreachable: two hex parses built
from raw bytes via String.format("%02X..."), a test connector's own
revision counter, the JVM's own java.specification.version, and two
substrings a regex just validated as exactly 13 digits - none of these
can actually fail to parse.
@vharseko vharseko added the java Pull requests that update java code label Sep 19, 2026
@vharseko vharseko added framework OpenICF-java-framework dbcommon OpenICF-dbcommon connector:ldap LDAP connector maven-plugin OpenICF-maven-plugin tests Test additions or fixes labels Sep 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

connector:ldap LDAP connector dbcommon OpenICF-dbcommon framework OpenICF-java-framework java Pull requests that update java code maven-plugin OpenICF-maven-plugin tests Test additions or fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant