InspectorDebuggerAgent: guard m_scripts.find() with isValidKey for protocol-supplied scriptId

[robobun]

Problem

m_scripts in InspectorDebuggerAgent is an UncheckedKeyHashMap<JSC::SourceID, JSC::Debugger::Script>. With WTF's default integer hash traits, key 0 is the empty-bucket sentinel and UINT32_MAX is the deleted-bucket sentinel.

Several Debugger protocol handlers parse the client-supplied scriptId string with parseIntegerAllowingTrailingJunk<JSC::SourceID>(...).value_or(0) and pass the result straight to m_scripts.find(). When a client sends scriptId: "0" (or "-1", "not-a-number", or an overflow that value_or(0) collapses to zero):

• On ASSERT_ENABLED builds the lookup hits ASSERTION FAILED: isValidKey(*entry) at wtf/HashTable.h:692 and aborts.
• On release builds the lookup matches the first empty bucket and returns an iterator to a default-constructed Script. Debugger.getScriptSource replies with {"scriptSource": ""} for a script that does not exist; Debugger.setBreakpoint then dereferences the null sourceProvider and segfaults.

Repro (against a bun --inspect process)

ws.send(JSON.stringify({ id: 1, method: "Debugger.enable", params: {} }));
ws.send(JSON.stringify({ id: 2, method: "Debugger.getScriptSource", params: { scriptId: "0" } }));
// release: {"result":{"scriptSource":""},"id":2}
// asserts: ASSERTION FAILED: isValidKey(*entry) ... SIGABRT

ws.send(JSON.stringify({ id: 3, method: "Debugger.setBreakpoint", params: { location: { scriptId: "0", lineNumber: 0 } } }));
// release: SIGSEGV (null sourceProvider deref in resolveBreakpoint)

Fix

Guard each call site that looks up a protocol-supplied sourceID in m_scripts with m_scripts.isValidKey(sourceID), returning the existing "Missing script ..." error for the reserved keys. This follows the same pattern as InspectorDOMAgent::nodeForId.

Affected handlers: setBreakpoint, continueToLocation, searchInContent, getScriptSource, getBreakpointLocations.

Branched from 34c01d13391e (Bun's current WEBKIT_VERSION) so the preview build can be consumed directly. The same change applies cleanly on main.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 14 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: b3c3a7fe-2475-4886-ae7e-30ef7350f441

📥 Commits

Reviewing files that changed from the base of the PR and between e6e37cd and 71c8e36.

📒 Files selected for processing (1)

• Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

LGTM — straightforward input-validation guards that follow the existing InspectorDOMAgent::nodeForId pattern.

What was reviewed:

• Confirmed m_scripts is UncheckedKeyHashMap<JSC::SourceID, ...> and noSourceID == 0, so the sentinel-key hazard is real and isValidKey is the right guard.
• Each new branch mirrors the adjacent == end() handling exactly (including the continueProgram() / resumed() pair in continueToLocation), so no behavioral divergence on the error path.
• .value_or(0) → .value_or(JSC::noSourceID) is a semantic no-op; getBreakpointLocations only needs to guard startSourceID because endSourceID is already required to equal it.

Extended reasoning...

Overview

This PR adds m_scripts.isValidKey(sourceID) guards in five InspectorDebuggerAgent protocol handlers (setBreakpoint, continueToLocation, searchInContent, getScriptSource, getBreakpointLocations) before calling m_scripts.find(sourceID) with a client-supplied scriptId. It also swaps two .value_or(0) for .value_or(JSC::noSourceID) (identical value, clearer intent). Single file, ~20 added lines, purely additive early-returns.

Security risks

None introduced. The change is a hardening fix: m_scripts is an UncheckedKeyHashMap, so looking up the empty/deleted sentinel keys (0 / max) is UB — assert-abort in debug, empty-bucket match and null-sourceProvider deref in release. The new guards reject those keys with the same "Missing script..." error the handler already returns for unknown IDs. The inspector protocol is a privileged debug channel, so this is robustness rather than a security boundary, but it does close a remotely-triggerable crash for anything speaking the protocol.

Level of scrutiny

Low-to-moderate. The diff is small, mechanical, and copies an established WebKit idiom verbatim (InspectorDOMAgent::nodeForId at InspectorDOMAgent.cpp:636). Each guard sits immediately before an existing find() and duplicates the adjacent == end() branch's side effects and error string, so the valid-key path is unchanged and the invalid-key path is indistinguishable from "script not found".

Other factors

• Verified JSC::noSourceID == 0 in DebuggerPrimitives.h, so the .value_or change is a no-op.
• getBreakpointLocations only guards startSourceID; endSourceID is already constrained to equal it a few lines earlier, so a second guard would be dead code.
• continueToLocation's new branch replicates the continueProgram() + resumed() calls from the existing not-found branch, preserving the resume semantics.
• No prior reviews on the PR; the bug-hunting pass found nothing.