creditkarma / thrift-typescript

Generate TypeScript from Thrift IDL files
Apache License 2.0
155 stars 32 forks source link

Don't send non-Thrift exception messages to client #187

Open markdoliner-doma opened 4 years ago

markdoliner-doma commented 4 years ago

BACKGROUND

Two kinds of exceptions can be thrown by a handler processing a Thrift request:

THIS CHANGE

Stop sending the exception message to the client. I think this is a bad idea from a security standpoint. Random exceptions raised by the server could potentially contain sensitive info like passwords. One hopes this never happens, of course, but we have no control over the code people write. It's not reasonable default behavior to send those messages to clients. The client could be untrusted or the transport could be insecure.

It could be argued that a flag should be added to enable the server to send these messages to clients. I think that's a reasonable thing to do (but I don't plan to do it).

I updated both the Apache code and the thrift-server code, and the tests for both.

Here's the diff of a ping function generated for thrift-server before and after this change:

--- before  2020-07-26 22:06:27.000000000 -0400
+++ after   2020-07-26 22:06:19.000000000 -0400
@@ -1,23 +1,23 @@
     public process_ping(requestId: number, input: thrift.TProtocol, output: thrift.TProtocol, context: Context): Promise<Buffer> {
         return new Promise<string>((resolve, reject): void => {
             try {
                 input.readMessageEnd();
                 resolve(this._handler.ping(context));
             }
             catch (err) {
                 reject(err);
             }
         }).then((data: string): Buffer => {
             const result: IPing__ResultArgs = { success: data };
             output.writeMessageBegin("ping", thrift.MessageType.REPLY, requestId);
             Ping__ResultCodec.encode(result, output);
             output.writeMessageEnd();
             return output.flush();
         }).catch((err: Error): Buffer => {
-            const result: thrift.TApplicationException = new thrift.TApplicationException(thrift.TApplicationExceptionType.UNKNOWN, err.message);
+            const result: thrift.TApplicationException = new thrift.TApplicationException(thrift.TApplicationExceptionType.UNKNOWN, "The server experienced an unexpected exception while processing the request.");
             output.writeMessageBegin("ping", thrift.MessageType.EXCEPTION, requestId);
             thrift.TApplicationExceptionCodec.encode(result, output);
             output.writeMessageEnd();
             return output.flush();
         });
     }