Repository navigation
Latent NPE risk at ~25 call sites of Json.toType() (surfaced by nullability annotation in #17802) #18031
Description
While auditing usages of Json.toType(...) / JsonTypeCoercer.coerce(...) for #17802 (which marks these methods @Nullable, since they can legitimately return null for a top-level JSON null), I found ~25 production call sites that dereference or pass along the result without a null guard. These are pre-existing behaviors, not regressions from #17802 — the nullability annotation just makes them visible to static analysis (e.g. IntelliJ's null-checker). None have a known repro; they'd only trigger on malformed/unexpected input (a literal null body, a corrupted stored value, etc.).
Grouped by area:
Grid node / capability parsing — feed into NodeOptions.enhanceStereotype, which dereferences immediately with no guard:
NodeOptions.java:358,472,RelayOptions.java:168,DockerOptions.java:173,KubernetesOptions.java:150
Wire/session-decode paths — no null guard, no catch for the null case:
ProtocolHandshake.java:97,RemoteScript.java:95,W3CHttpResponseCodec.java:106,160,GraphqlHandler.java:140,CustomLocatorHandler.java:111,NewNodeSession.java:48,RemoteNode.java:186,AddNode.java:57,LocalNode.java:1011,1122,1160+OneShotNode.java:360(shared code),RedisBackedNewSessionQueue.java:441,527
Lower severity (already inside broad catches elsewhere in the file, or build-time tooling):
FirefoxProfile.java:88,Preferences.java:101,InspectContainer.java:67,ListImages.java:77,CdpEndpointFinder.java:75,CdpEventTypes.java:96,SeleniumManager.java:167,CdpClientGenerator.java:108,OneShotNode.java:149
Structural — Event.java:59's getData(Type) re-exports the nullability gap: its own return type isn't annotated @Nullable, silently passing the gap to every caller.
Suggest tackling these in small, targeted follow-up PRs per area (grid node config parsing vs. wire-protocol decode vs. the Event API), each with a regression test for the null-input case.
🤖 Generated with Claude Code
Activity
jasminritik26 commentedon Sep 18, 2026
brokendenim commentedon Sep 19, 2026
darnellaakaisha26 commentedon Sep 20, 2026
yashp676 commentedon Oct 13, 2026
Hi @asolntsev, would it be OK if I took the Grid node config parsing group (NodeOptions, RelayOptions, DockerOptions, KubernetesOptions) as the first PR?
My plan is to make a top-level null there fail with a ConfigException naming the offending config entry, rather than an NPE inside enhanceStereotype, with a regression test per call site that feeds in a null stereotype/config value.
asolntsev commentedon Oct 13, 2026
@yashp676 it's a good idea. But wait, I have some uncommitted changes in my branch on my machine. Let me review them first. I will return soon.
finish marking `Json.toType` overloads as nullable; fix unsafe usages…
asolntsev commentedon Oct 13, 2026
@yashp676 I've committed my local changes to #17802
This issue #18031 was registered to mention the "other" cases not covered by PR #17802. So yes, feel free to handle the Grid node config parsing group.
yashp676 commentedon Oct 14, 2026
Thanks, I’ll open a PR for the Grid node config group, based on #17802
yashp676 commentedon Oct 16, 2026
@asolntsev I’ve opened #18185 for the Grid node config parsing group: NodeOptions (both stereotype paths), RelayOptions, DockerOptions and KubernetesOptions. A null stereotype now fails with a ConfigException naming the config entry instead of an NPE. One correction to my earlier comment: on the Relay path the NPE was in ImmutableCapabilities, not enhanceStereotype. Same fix either way.