Harden JSON-RPC message classification (#6759)

Author: tim-smart

.changeset/tough-taxis-own.md

@@ -0,0 +1,5 @@
+---
+"effect": patch
+---
+
+Harden JSON-RPC wire message classification against inherited properties.

packages/effect/src/unstable/rpc/RpcClient.ts

@@ -1049,11 +1049,12 @@ export const makeProtocolSocket = (options?: {
                 pinger.onPong()
                 return Effect.void
               }
-              if ("requestId" in response) {
-                const clientId = requestClientMap.get(response.requestId)
+              if (Object.hasOwn(response, "requestId")) {
+                const requestId = (response as FromServerEncoded & { readonly requestId: string | number }).requestId
+                const clientId = requestClientMap.get(requestId)
                 if (clientId !== undefined) {
                   if (response._tag === "Exit") {
-                    requestClientMap.delete(response.requestId)
+                    requestClientMap.delete(requestId)
                   }
                   return writeResponse(clientId, response)
                 }

packages/effect/src/unstable/rpc/RpcSerialization.ts

@@ -212,12 +212,13 @@ function decodeJsonRpcRaw(
 }
 
 function decodeJsonRpcMessage(decoded: JsonRpcMessage): RpcMessage.FromClientEncoded | RpcMessage.FromServerEncoded {
-  if ("method" in decoded) {
-    if (Predicate.isNullish(decoded.id) && decoded.method.startsWith("@effect/rpc/")) {
-      const tag = decoded.method.slice("@effect/rpc/".length) as
+  if (Object.hasOwn(decoded, "method")) {
+    const request = decoded as JsonRpcRequest
+    if (Predicate.isNullish(request.id) && request.method.startsWith("@effect/rpc/")) {
+      const tag = request.method.slice("@effect/rpc/".length) as
         | RpcMessage.FromServerEncoded["_tag"]
         | Exclude<RpcMessage.FromClientEncoded["_tag"], "Request">
-      const requestId = (decoded as any).params?.requestId
+      const requestId = (request as any).params?.requestId
       return requestId ?
         {
           _tag: tag,
@@ -227,46 +228,49 @@ function decodeJsonRpcMessage(decoded: JsonRpcMessage): RpcMessage.FromClientEnc
     }
     return {
       _tag: "Request",
-      id: decoded.id ?? "",
-      tag: decoded.method,
-      payload: decoded.params ?? null,
-      headers: decoded.headers ?? [],
-      ...(decoded.spanId ?
+      id: request.id ?? "",
+      tag: request.method,
+      payload: request.params ?? null,
+      headers: request.headers ?? [],
+      ...(request.spanId ?
         {
-          traceId: decoded.traceId,
-          spanId: decoded.spanId!,
-          sampled: decoded.sampled!
+          traceId: request.traceId,
+          spanId: request.spanId!,
+          sampled: request.sampled!
         } :
         {})
     }
-  } else if (decoded.error && decoded.error._tag === "Defect") {
+  }
+  const response = decoded as JsonRpcResponse
+  const hasError = Object.hasOwn(response, "error")
+  if (hasError && response.error && response.error._tag === "Defect") {
     return {
       _tag: "Defect",
-      defect: decoded.error.data
+      defect: response.error.data
     }
-  } else if (decoded.chunk === true) {
+  } else if (Object.hasOwn(response, "chunk") && response.chunk === true) {
     return {
       _tag: "Chunk",
-      requestId: decoded.id ?? "",
-      values: decoded.result as any
+      requestId: response.id ?? "",
+      values: response.result as any
     }
   }
   return {
     _tag: "Exit",
-    requestId: decoded.id ?? "",
-    exit: decoded.error != null ?
+    requestId: response.id ?? "",
+    exit: hasError && response.error != null ?
       {
         _tag: "Failure",
-        cause: decoded.error._tag === "Cause" ?
-          decoded.error.data as any :
+        cause: response.error._tag === "Cause" ?
+          response.error.data as any :
           [{
             _tag: "Die",
-            defect: decoded.error
+            defect: response.error
           }]
       } :
       {
         _tag: "Success",
-        value: decoded.result
+        value: response.result
       }
   }
 }

packages/effect/src/unstable/rpc/RpcServer.ts

@@ -22,7 +22,6 @@ import { reportCauseUnsafe } from "../../internal/effect.ts"
 import * as Latch from "../../Latch.ts"
 import * as Layer from "../../Layer.ts"
 import * as Option from "../../Option.ts"
-import * as Predicate from "../../Predicate.ts"
 import * as Pull from "../../Pull.ts"
 import * as Queue from "../../Queue.ts"
 import * as Schedule from "../../Schedule.ts"
@@ -688,7 +687,7 @@ export const make: <Rpcs extends Rpc.Any>(
 
     switch (request._tag) {
       case "Request": {
-        const tag = Predicate.hasProperty(request, "tag") ? (request.tag as string) : ""
+        const tag = Object.hasOwn(request, "tag") ? (request.tag as string) : ""
         let requestId: RequestId
         switch (typeof request.id) {
           case "number":

packages/effect/test/rpc/RpcSerialization.test.ts

@@ -1,4 +1,4 @@
-import { assert, describe, it } from "@effect/vitest"
+import { afterEach, assert, describe, it } from "@effect/vitest"
 import { RpcSerialization } from "effect/unstable/rpc"
 
 const responseExitSuccess = (requestId: string | number, value: unknown) => ({
@@ -10,7 +10,60 @@ const responseExitSuccess = (requestId: string | number, value: unknown) => ({
   }
 })
 
+const objectPrototype = Object.prototype as Record<string, unknown>
+
+const polluteObjectPrototype = (key: string, value: unknown) => {
+  Object.defineProperty(objectPrototype, key, {
+    configurable: true,
+    value
+  })
+}
+
+const decodeJsonRpcSuccess = () =>
+  RpcSerialization.jsonRpc().makeUnsafe().decode("{\"jsonrpc\":\"2.0\",\"id\":1,\"result\":\"ok\"}")
+
+const expectedJsonRpcSuccess = [{
+  _tag: "Exit",
+  requestId: 1,
+  exit: {
+    _tag: "Success",
+    value: "ok"
+  }
+}]
+
 describe("RpcSerialization", () => {
+  describe.sequential("jsonRpc inherited properties", () => {
+    afterEach(() => {
+      delete objectPrototype["method"]
+      delete objectPrototype["error"]
+      delete objectPrototype["chunk"]
+    })
+
+    it("decodes a success response with a clean prototype", () => {
+      assert.deepStrictEqual(decodeJsonRpcSuccess(), expectedJsonRpcSuccess)
+    })
+
+    it("ignores an inherited method", () => {
+      polluteObjectPrototype("method", "attacker.evil")
+      assert.deepStrictEqual(decodeJsonRpcSuccess(), expectedJsonRpcSuccess)
+    })
+
+    it("ignores an inherited defect error", () => {
+      polluteObjectPrototype("error", { _tag: "Defect", data: "pwn" })
+      assert.deepStrictEqual(decodeJsonRpcSuccess(), expectedJsonRpcSuccess)
+    })
+
+    it("ignores an inherited chunk marker", () => {
+      polluteObjectPrototype("chunk", true)
+      assert.deepStrictEqual(decodeJsonRpcSuccess(), expectedJsonRpcSuccess)
+    })
+
+    it("ignores an inherited exit error", () => {
+      polluteObjectPrototype("error", { _tag: "Cause", data: [] })
+      assert.deepStrictEqual(decodeJsonRpcSuccess(), expectedJsonRpcSuccess)
+    })
+  })
+
   it("json decode keeps array payloads flat", () => {
     const parser = RpcSerialization.json.makeUnsafe()
     const decoded = parser.decode("[1,2,3]")