fix(mac): guard browser.proxy JSON serialization against non-serializable values#45895
fix(mac): guard browser.proxy JSON serialization against non-serializable values#45895vernonstinebaker wants to merge 3 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR fixes a Changes:
Confidence Score: 4/5
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @Test func postRequestWithNonSerializableBodyDoesNotCrash() async throws { | ||
| actor BodyCapture { | ||
| private var body: Data? | ||
| func set(_ body: Data?) { self.body = body } | ||
| func get() -> Data? { self.body } | ||
| } | ||
|
|
||
| let capturedBody = BodyCapture() | ||
| let proxy = MacNodeBrowserProxy( | ||
| endpointProvider: { | ||
| MacNodeBrowserProxy.Endpoint( | ||
| baseURL: URL(string: "http://127.0.0.1:18791")!, | ||
| token: nil, | ||
| password: nil) | ||
| }, | ||
| performRequest: { request in | ||
| await capturedBody.set(request.httpBody) | ||
| let url = try #require(request.url) | ||
| let response = try #require( | ||
| HTTPURLResponse( | ||
| url: url, | ||
| statusCode: 200, | ||
| httpVersion: nil, | ||
| headerFields: nil)) | ||
| return (Data(#"{"ok":true}"#.utf8), response) | ||
| }) | ||
|
|
||
| // Encode a body that, after AnyCodable decoding, is fine. | ||
| // The sanitization layer ensures no crash even if the gateway | ||
| // sends something unexpected in the future. | ||
| _ = try await proxy.request( | ||
| paramsJSON: #"{"method":"POST","path":"/action","body":{"key":"val"}}"#) | ||
|
|
||
| let bodyData = try #require(await capturedBody.get()) | ||
| let parsed = try #require(JSONSerialization.jsonObject(with: bodyData) as? [String: Any]) | ||
| #expect(parsed["key"] as? String == "val") | ||
| } |
There was a problem hiding this comment.
Integration test doesn't exercise the non-serializable path
The test name postRequestWithNonSerializableBodyDoesNotCrash implies it verifies the crash fix, but the body {"key":"val"} is perfectly valid JSON and passes through sanitizeForJSON via the isValidJSONObject fast-path without ever touching the fallback serialization logic. The comment even acknowledges this: "The sanitization layer ensures no crash even if the gateway sends something unexpected in the future."
This test is essentially a duplicate of the existing postRequestSerializesNestedBodyWithoutCrash with a simpler body. The real crash scenario (an opaque __SwiftValue reaching NSJSONSerialization) is only exercised at unit level in sanitizeForJSONConvertsNonSerializableValuesToStrings. Consider either renaming this test to reflect what it actually covers (e.g. postRequestWithSimpleBodySerializesCorrectly), or extending it to inject a non-serializable value directly into the sanitizeForJSON → makeRequest path so the regression is tested end-to-end.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/macos/Tests/OpenClawIPCTests/MacNodeBrowserProxyTests.swift
Line: 145-181
Comment:
**Integration test doesn't exercise the non-serializable path**
The test name `postRequestWithNonSerializableBodyDoesNotCrash` implies it verifies the crash fix, but the body `{"key":"val"}` is perfectly valid JSON and passes through `sanitizeForJSON` via the `isValidJSONObject` fast-path without ever touching the fallback serialization logic. The comment even acknowledges this: "The sanitization layer ensures no crash even if the gateway sends something unexpected in the future."
This test is essentially a duplicate of the existing `postRequestSerializesNestedBodyWithoutCrash` with a simpler body. The real crash scenario (an opaque `__SwiftValue` reaching `NSJSONSerialization`) is only exercised at unit level in `sanitizeForJSONConvertsNonSerializableValuesToStrings`. Consider either renaming this test to reflect what it actually covers (e.g. `postRequestWithSimpleBodySerializesCorrectly`), or extending it to inject a non-serializable value directly into the `sanitizeForJSON` → `makeRequest` path so the regression is tested end-to-end.
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d93287ca33
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
apps/macos/Package.resolved
Outdated
| "location" : "https://github.com/steipete/ElevenLabsKit", | ||
| "state" : { | ||
| "revision" : "c8679fbd37416a8780fe43be88a497ff16209e2d", | ||
| "revision" : "7e3c948d8340abe3977014f3de020edf221e9269", |
There was a problem hiding this comment.
This change updates elevenlabskit to a different git revision while keeping the same 0.1.0 version, and there is no matching manifest or code change that explains a dependency upgrade. That silently swaps the source code pulled for macOS builds under the same semantic version, which weakens reproducibility and can introduce unreviewed behavior changes in production artifacts; if this was not a deliberate dependency update, the lockfile change should be dropped from this bugfix commit.
Useful? React with 👍 / 👎.
2 participants
Summary
SIGABRTwhenMacNodeBrowserProxy.makeRequestpasses non-JSON-serializable values toNSJSONSerialization. Crash reports show__SwiftValueexceptions in the serialization path.browser.proxyrequest contains unexpected value types, breaking browser control for users. Four duplicate issues confirm this is actively hitting users.sanitizeForJSON(_:)recursive sanitizer inMacNodeBrowserProxythat converts non-JSON-serializable values toString(describing:)before passing toNSJSONSerialization. Also added explicitCronJobmemberwise init to fix a pre-existing Swift 6.2 build error in the test target.AnyCodabledecoding — the sanitizer is purely a defensive layer at serialization time.Change Type
Scope
Linked Issue/PR
User-visible / Behavior Changes
None. The app previously crashed silently; it now gracefully sanitizes the request body.
Security Impact
No)No)No)No)No)Evidence
OpenClaw-2026-03-13-041109.ips(SIGABRT inMacNodeBrowserProxy.makeRequest,lastExceptionBacktraceatNSJSONSerialization dataWithJSONObject)node.invokereturnsUNAVAILABLE: Error: node disconnected (browser.proxy)at the same timestampHuman Verification (required)
sanitizeForJSONwith opaque Swift structs, nested dictionaries, arrays, JSON-safe primitives (String/Int/Bool/NSNull), full proxy request round-trip through mock HTTPCFGetTypeID, recursive nesting through mixed dict/array structuresReview Conversations
Compatibility / Migration
Yes)No)No)Failure Recovery (if this breaks)
d93287capps/macos/Sources/OpenClaw/NodeMode/MacNodeBrowserProxy.swiftRisks and Mitigations
String(describing:)may produce verbose or unexpected output for complex opaque typesisValidJSONObjectcheck.🤖 AI-assisted. Fully tested (7/7 tests passing).