SeleniumHQ / SeleniumHQ/selenium

Latent NPE risk at ~25 call sites of Json.toType() (surfaced by nullability annotation in #17802)

Open
#18,031 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Java
Stars
34.5k
Forks
8.7k
Avg merge
2d 1h
Merged PRs (30d)
92

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](https://claude.com/claude-code)

Contributor guide

Open the contributing guide

Research direction

Start by reading Json.toType(...) and JsonTypeCoercer.coerce(...), then choose one area such as grid node configuration or wire/session decoding rather than attempting all listed call sites. Inspect the named Java files and trace how null results are consumed. Done means each targeted call site has an intentional null-handling outcome and a regression test covers null input.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
backend, distributed-systems
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.