Censor error messages for all non-connect errors

[fubhy]

connectErrorFromReason is currently implemented as:

/**
 * Convert any value - typically a caught error into a ConnectError,
 * following these rules:
 * - If the value is already a ConnectError, return it as is.
 * - If the value is an AbortError from the fetch API, return the message
 *   of the AbortError with code Canceled.
 * - For other Errors, return the error message with code Unknown by default.
 * - For other values, return the values String representation as a message,
 *   with the code Unknown by default.
 */
export declare function connectErrorFromReason(reason: unknown, code?: Code): ConnectError;

I'd suggest to not forward the error message for arbitrary Errors by default. That could lead to leaking sensitive information.

[timostamm]

You mean in the context of a server? We definitely want this to be useful in a production environment, where you would differentiate between expected errors raised intentionally by an implementation, and unexpected errors, for example from reading a file from a sensitive location.

[timostamm]

we should ensure that, when captured by the encapsulating Connect Node.js server, any messages from non-Connect errors (so unless I'm explicitly throwing new ConnectError(...)) are never surfaced to the API consumer.

Agree with that 👍 I don't think we're there yet, and I believe we need tests to ensure that this is upheld.

[fubhy]

I see. As mentioned in my original post, I think I'd side with a blanket solution for that:

If there's any unhandled errors, which includes absolutely any errors that are not already handled & explicitly (re-)thrown as ConnectError by the implementation, that reach the encapsulating Connect Node.js server, I'd not surface them in the response at all. In such a case, I'd simply return new ConnectError("Internal server error", Code.Internal).

Only return the error message if the user / implemenation chose to explicitly throw it as a ConnectError imho.

[hdevalence]

Kind of a late comment here (cc @turbocrime), but we ran across this in the context of penumbra-zone/web#323 (we're using a custom connect transport to use a browser extension as a grpc server). Our expectation was that our grpc service could return errors and that those would be visible to the client that made the request.

Failed to connect using "secret-user:secret-password@secret-db-host:12345"

Surely the expectation is that servers should not put passwords in responses they return to clients, correct? If a server does that, how does censoring the error message in the connect client specifically help? The server is still returning the sensitive data over the network. A non-default client can still get the leaked information.

How is this supposed to work for services that might be using connect or might be using grpc-web? A standard grpc-web server won't be doing any connect-specific errors...

[timostamm]

@hdevalence, this is probably a misunderstanding. Clients do not censor error messages sent over the wire.

The issue discussed here was that any error raised in a handler (a server written with connect-es) would be surfaced over the wire and to the peer in the early alpha versions of connect-es. For example (based on this guide):

export default (router: ConnectRouter) =>
  router.service(ElizaService, {
    async say(req: SayRequest, context: HandlerContext) {
      return new SayResponse({
        sentence: getResponse(),
      });
    }
  });

function getResponse(): string {
  throw new DatabaseConnectionError(`Failed to connect using "secret-user:secret-password@secret-db-host:12345"`);
}

This would send the error string over the wire to the user.

The behavior was changed to send an error with code INTERNAL and the message "internal error" instead, which seems like a reasonable default behavior. To intentionally send an error with details over the wire, users have to raise a ConnectError instead.

[hdevalence]

Got it, thanks for the explanation, and sorry about the misunderstanding!