Fixes microsoft#81424: Throw when a request is made with invalid arguments

alexdima · alexdima · commit 79280beee235 · 2020-03-02T22:09:19.000+01:00
diff --git a/src/vs/workbench/services/extensions/common/rpcProtocol.ts b/src/vs/workbench/services/extensions/common/rpcProtocol.ts
@@ -27,6 +27,10 @@ function safeStringify(obj: any, replacer: JSONStringifyReplacer | null): string
 	}
 }
 
+function stringify(obj: any, replacer: JSONStringifyReplacer | null): string {
+	return JSON.stringify(obj, <(key: string, value: any) => any>replacer);
+}
+
 function createURIReplacer(transformer: IURITransformer | null): JSONStringifyReplacer | null {
 	if (!transformer) {
 		return null;
@@ -412,6 +416,8 @@ export class RPCProtocol extends Disposable implements IRPCProtocol {
 			return Promise.reject<any>(errors.canceled());
 		}
 
+		const serializedRequestArguments = MessageIO.serializeRequestArguments(args, this._uriReplacer);
+
 		const req = ++this._lastMessageId;
 		const callId = String(req);
 		const result = new LazyPromise();
@@ -428,7 +434,7 @@ export class RPCProtocol extends Disposable implements IRPCProtocol {
 
 		this._pendingRPCReplies[callId] = result;
 		this._onWillSendRequest(req);
-		const msg = MessageIO.serializeRequest(req, rpcId, methodName, args, !!cancellationToken, this._uriReplacer);
+		const msg = MessageIO.serializeRequest(req, rpcId, methodName, serializedRequestArguments, !!cancellationToken);
 		if (this._logger) {
 			this._logger.logOutgoing(msg.byteLength, req, RequestInitiator.LocalSide, `request: ${getStringIdentifierForProxy(rpcId)}.${methodName}(`, args);
 		}
@@ -600,6 +606,8 @@ class MessageBuffer {
 	}
 }
 
+type SerializedRequestArguments = { type: 'mixed'; args: VSBuffer[]; argsType: ArgType[]; } | { type: 'simple'; args: string; };
+
 class MessageIO {
 
 	private static _arrayContainsBufferOrUndefined(arr: any[]): boolean {
@@ -614,7 +622,7 @@ class MessageIO {
 		return false;
 	}
 
-	public static serializeRequest(req: number, rpcId: number, method: string, args: any[], usesCancellationToken: boolean, replacer: JSONStringifyReplacer | null): VSBuffer {
+	public static serializeRequestArguments(args: any[], replacer: JSONStringifyReplacer | null): SerializedRequestArguments {
 		if (this._arrayContainsBufferOrUndefined(args)) {
 			let massagedArgs: VSBuffer[] = [];
 			let massagedArgsType: ArgType[] = [];
@@ -627,13 +635,27 @@ class MessageIO {
 					massagedArgs[i] = VSBuffer.alloc(0);
 					massagedArgsType[i] = ArgType.Undefined;
 				} else {
-					massagedArgs[i] = VSBuffer.fromString(safeStringify(arg, replacer));
+					massagedArgs[i] = VSBuffer.fromString(stringify(arg, replacer));
 					massagedArgsType[i] = ArgType.String;
 				}
 			}
-			return this._requestMixedArgs(req, rpcId, method, massagedArgs, massagedArgsType, usesCancellationToken);
+			return {
+				type: 'mixed',
+				args: massagedArgs,
+				argsType: massagedArgsType
+			};
+		}
+		return {
+			type: 'simple',
+			args: stringify(args, replacer)
+		};
+	}
+
+	public static serializeRequest(req: number, rpcId: number, method: string, serializedArgs: SerializedRequestArguments, usesCancellationToken: boolean): VSBuffer {
+		if (serializedArgs.type === 'mixed') {
+			return this._requestMixedArgs(req, rpcId, method, serializedArgs.args, serializedArgs.argsType, usesCancellationToken);
 		}
-		return this._requestJSONArgs(req, rpcId, method, safeStringify(args, replacer), usesCancellationToken);
+		return this._requestJSONArgs(req, rpcId, method, serializedArgs.args, usesCancellationToken);
 	}
 
 	private static _requestJSONArgs(req: number, rpcId: number, method: string, args: string, usesCancellationToken: boolean): VSBuffer {
diff --git a/src/vs/workbench/services/extensions/test/common/rpcProtocol.test.ts b/src/vs/workbench/services/extensions/test/common/rpcProtocol.test.ts
@@ -212,4 +212,13 @@ suite('RPCProtocol', () => {
 			assert.equal(res, 7);
 		});
 	});
+
+	test('issue #81424: SerializeRequest should throw if an argument can not be serialized', () => {
+		let badObject = {};
+		(<any>badObject).loop = badObject;
+
+		assert.throws(() => {
+			bProxy.$m(badObject, '2');
+		});
+	});
 });
