Fix the JSONSerialization crash risk before release
JSONSerialization.data(withJSONObject:) looks like a normal throwing API, but for a genuinely invalid top-level object (Double.nan/ .infinity anywhere in the object graph, or a non-bridgeable type) it raises an Objective-C exception instead of a catchable Swift Error — neither try nor try? protects against that, and the process crashes. Logged as a priority roadmap item after being found while building Apple Intelligence tool calling; fixing it now before release per Rune's ask. SafeJSONEncoding (new) checks JSONSerialization.isValidJSONObject first — a plain, safe, non-throwing Bool check — before ever calling the crash-prone encode path, returning nil instead of crashing for invalid input. Verified the check itself can't be fooled (a standalone script confirmed it correctly predicts NaN, Infinity, nested NaN, and non-bridgeable-type cases, all without crashing) before wiring it in. Applied to every call site that encodes data the app doesn't fully control — tool results (ChatViewModel.generateAIResponseWithTools, DynamicMCPTool.encodeResult, MCPService.serializeToolResult for research sub-agents) and MessageRow's tool-call-detail pretty-printer. Left outbound request-body construction in the provider files alone — those dictionaries are built entirely from known Swift types the app already controls (validated settings, string content), not from tool/model/external-server output, so the same crash class isn't realistically reachable there. Also restored a test that previously had to be deliberately skipped because it reproducibly crashed the whole test process on this exact bug (documented at the time in feature_apple_intelligence_provider memory) — now passes cleanly, directly confirming the fix rather than just the new code compiling. 436 tests total, stable across two consecutive full runs.
This commit is contained in:
@@ -114,7 +114,7 @@ struct DynamicMCPTool: FoundationModels.Tool {
|
||||
/// Pure — JSON-encodes an executeTool result dict the same way `ChatViewModel`'s manual tool loop
|
||||
/// already does, including the same truncation cap.
|
||||
nonisolated static func encodeResult(_ result: [String: Any]) -> String {
|
||||
guard let data = try? JSONSerialization.data(withJSONObject: result),
|
||||
guard let data = SafeJSONEncoding.data(withJSONObject: result),
|
||||
let str = String(data: data, encoding: .utf8) else {
|
||||
return "{\"error\": \"Failed to serialize result\"}"
|
||||
}
|
||||
|
||||
@@ -1079,7 +1079,7 @@ class MCPService {
|
||||
}
|
||||
|
||||
private func serializeToolResult(_ result: [String: Any], maxBytes: Int = 20_000) -> String {
|
||||
guard let data = try? JSONSerialization.data(withJSONObject: result),
|
||||
guard let data = SafeJSONEncoding.data(withJSONObject: result),
|
||||
let str = String(data: data, encoding: .utf8) else {
|
||||
return "{\"error\": \"Failed to serialize result\"}"
|
||||
}
|
||||
|
||||
@@ -0,0 +1,46 @@
|
||||
//
|
||||
// SafeJSONEncoding.swift
|
||||
// Confab
|
||||
//
|
||||
// A crash-safe drop-in for JSONSerialization.data(withJSONObject:) when the input isn't fully
|
||||
// controlled by the app (tool results, sub-agent results, and similar).
|
||||
//
|
||||
// SPDX-License-Identifier: PolyForm-Noncommercial-1.0.0
|
||||
// Copyright (C) 2026 Rune Olsen
|
||||
//
|
||||
// This file is part of Confab.
|
||||
//
|
||||
// Confab is licensed under the PolyForm Noncommercial License 1.0.0.
|
||||
// You may use, study, modify, and share it for any noncommercial
|
||||
// purpose. Commercial use — including selling Confab or any part of
|
||||
// it, standalone or bundled into another product or service —
|
||||
// requires a separate commercial license from the copyright holder.
|
||||
//
|
||||
// See the LICENSE file or
|
||||
// <https://polyformproject.org/licenses/noncommercial/1.0.0> for
|
||||
// the full license text. For commercial licensing, contact Rune
|
||||
// Olsen via <https://confab.no>.
|
||||
|
||||
|
||||
import Foundation
|
||||
|
||||
/// `JSONSerialization.data(withJSONObject:)` looks like a normal throwing API, but for a genuinely
|
||||
/// invalid top-level object (e.g. a `Double.nan`/`Double.infinity` value anywhere in the object
|
||||
/// graph, or a value that isn't bridgeable to a JSON type at all) it can raise an **Objective-C
|
||||
/// exception** instead of throwing a catchable Swift `Error` — neither `try?` nor `try` protects
|
||||
/// against that, and the process crashes via `abort()`.
|
||||
///
|
||||
/// Confirmed live 2026-08-28 via a real crash report (not guessed): a test exercising exactly this
|
||||
/// path took down the whole process. `JSONSerialization.isValidJSONObject(_:)` is a plain, safe,
|
||||
/// non-throwing `Bool` check that correctly predicts every case that would otherwise crash — checking
|
||||
/// it first avoids ever calling the crash-prone path on bad data. See
|
||||
/// `project_roadmap_priorities`/`feature_apple_intelligence_provider` in project memory for the full
|
||||
/// investigation this came out of.
|
||||
nonisolated enum SafeJSONEncoding {
|
||||
/// Returns the JSON-encoded data, or nil if `object` isn't valid JSON — never crashes, unlike
|
||||
/// calling `JSONSerialization.data(withJSONObject:)` directly on unvalidated input.
|
||||
nonisolated static func data(withJSONObject object: Any, options: JSONSerialization.WritingOptions = []) -> Data? {
|
||||
guard JSONSerialization.isValidJSONObject(object) else { return nil }
|
||||
return try? JSONSerialization.data(withJSONObject: object, options: options)
|
||||
}
|
||||
}
|
||||
@@ -1849,7 +1849,7 @@ Don't narrate future actions ("Let me...") - just use the tools.
|
||||
|
||||
let result = await mcp.executeTool(name: tc.functionName, arguments: tc.arguments, agentProvider: provider, agentModelId: effectiveModelId)
|
||||
let resultJSON: String
|
||||
if let data = try? JSONSerialization.data(withJSONObject: result),
|
||||
if let data = SafeJSONEncoding.data(withJSONObject: result),
|
||||
let str = String(data: data, encoding: .utf8) {
|
||||
// Cap tool results at 50 KB to avoid HTTP 413 on the next API call
|
||||
let maxBytes = 50_000
|
||||
|
||||
@@ -382,7 +382,7 @@ struct MessageRow: View {
|
||||
private func prettyJSON(_ raw: String) -> String {
|
||||
guard let data = raw.data(using: .utf8),
|
||||
let obj = try? JSONSerialization.jsonObject(with: data),
|
||||
let pretty = try? JSONSerialization.data(withJSONObject: obj, options: [.prettyPrinted, .sortedKeys]),
|
||||
let pretty = SafeJSONEncoding.data(withJSONObject: obj, options: [.prettyPrinted, .sortedKeys]),
|
||||
let str = String(data: pretty, encoding: .utf8) else {
|
||||
return raw
|
||||
}
|
||||
|
||||
@@ -110,15 +110,14 @@ struct AppleDynamicToolTests {
|
||||
#expect(json.contains("truncated"))
|
||||
}
|
||||
|
||||
// encodeResult's "non-JSON-serializable input" guard-clause path is deliberately NOT unit-tested
|
||||
// here. Confirmed via a real crash report (Confab-2026-08-28-094748.ips, faulting thread's
|
||||
// top frames: NSJSONSerialization → _writeJSONObject → objc_exception_throw → abort()) that
|
||||
// JSONSerialization.data(withJSONObject:) raises an Objective-C exception — not a catchable
|
||||
// Swift Error — for genuinely invalid top-level objects (tried both Double.nan and a plain
|
||||
// non-bridgeable Swift struct; both crash identically). `try?` can never convert an NSException,
|
||||
// so any test that actually reaches this path terminates the whole process instead of failing
|
||||
// one assertion. This is a real, pre-existing risk shared by every provider's tool loop, not
|
||||
// something new here — ChatViewModel.generateAIResponseWithTools() does the exact same
|
||||
// `try? JSONSerialization.data(withJSONObject:)` pattern for tool results today. Recorded in
|
||||
// memory (feature_apple_intelligence_provider.md) rather than silently worked around.
|
||||
@Test("encodeResult falls back to an error payload for non-JSON-serializable input, without crashing")
|
||||
func encodeResultHandlesUnserializableInput() {
|
||||
// This used to reproducibly crash the whole test process (a real .ips crash report,
|
||||
// Confab-2026-08-28-094748.ips, traced it to JSONSerialization.data(withJSONObject:)
|
||||
// raising an uncatchable Objective-C exception for invalid input — try? can't stop that).
|
||||
// Fixed by routing encodeResult through SafeJSONEncoding, which checks
|
||||
// isValidJSONObject first. Safe to test directly now.
|
||||
let json = DynamicMCPTool.encodeResult(["bad": Double.nan])
|
||||
#expect(json.contains("Failed to serialize result"))
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,54 @@
|
||||
//
|
||||
// SafeJSONEncodingTests.swift
|
||||
// oAITests
|
||||
//
|
||||
// SPDX-License-Identifier: PolyForm-Noncommercial-1.0.0
|
||||
// Copyright (C) 2026 Rune Olsen
|
||||
|
||||
import Testing
|
||||
import Foundation
|
||||
@testable import Confab
|
||||
|
||||
@Suite("SafeJSONEncoding")
|
||||
struct SafeJSONEncodingTests {
|
||||
|
||||
@Test("Encodes a normal, valid JSON object")
|
||||
func encodesValidObject() {
|
||||
let data = SafeJSONEncoding.data(withJSONObject: ["ok": true, "value": "hello"])
|
||||
#expect(data != nil)
|
||||
let str = String(data: data!, encoding: .utf8)
|
||||
#expect(str?.contains("\"ok\"") == true)
|
||||
}
|
||||
|
||||
@Test("Returns nil instead of crashing for a NaN value — the exact case that took down the whole process before this fix")
|
||||
func returnsNilForNaN() {
|
||||
let data = SafeJSONEncoding.data(withJSONObject: ["bad": Double.nan])
|
||||
#expect(data == nil)
|
||||
}
|
||||
|
||||
@Test("Returns nil instead of crashing for an Infinity value")
|
||||
func returnsNilForInfinity() {
|
||||
let data = SafeJSONEncoding.data(withJSONObject: ["bad": Double.infinity])
|
||||
#expect(data == nil)
|
||||
}
|
||||
|
||||
@Test("Returns nil instead of crashing for a non-bridgeable Swift type")
|
||||
func returnsNilForNonBridgeableType() {
|
||||
struct NotJSONSerializable {}
|
||||
let data = SafeJSONEncoding.data(withJSONObject: ["bad": NotJSONSerializable()])
|
||||
#expect(data == nil)
|
||||
}
|
||||
|
||||
@Test("Returns nil for a NaN value nested inside an array, not just at the top level")
|
||||
func returnsNilForNestedNaN() {
|
||||
let data = SafeJSONEncoding.data(withJSONObject: ["arr": [1.0, 2.0, Double.nan]])
|
||||
#expect(data == nil)
|
||||
}
|
||||
|
||||
@Test("Passes through writing options (pretty-printed output is human-readable with newlines)")
|
||||
func passesThroughOptions() {
|
||||
let data = SafeJSONEncoding.data(withJSONObject: ["a": 1, "b": 2], options: [.prettyPrinted, .sortedKeys])
|
||||
let str = String(data: data!, encoding: .utf8)
|
||||
#expect(str?.contains("\n") == true)
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user