Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error message not received by connect-node client #1404

Closed
mashiro opened this issue Jan 17, 2025 · 4 comments
Closed

Error message not received by connect-node client #1404

mashiro opened this issue Jan 17, 2025 · 4 comments
Labels
bug Something isn't working

Comments

@mashiro
Copy link

mashiro commented Jan 17, 2025

Describe the bug

When grpc-transport is used in connect client, grpc-message is lost if the gRPC server includes a space at the end of the error message.

To Reproduce

Minimum implementation is here.
https://github.com/mashiro/connect-node-test

This is the case for the server implementation of connect-node.
The space is percent encoded and the message is retrieved correctly.

node server.js
❯ node client.js "foo "
{
  code: 3,
  rawMessage: 'foo ',
  metadata: Headers {
    'grpc-message': 'foo%20',
    'grpc-status': '3',
    'content-type': 'application/grpc+proto',
    date: 'Fri, 17 Jan 2025 06:22:12 GMT',
    'grpc-encoding': 'gzip'
  }
}
❯ pnpm exec buf curl -v --protocol grpc --schema ./proto -d '{"message":"foo "}' --http2-prior-knowledge http://localhost:50051/hello.v1.HelloService/Error
buf: * Invoking RPC hello.v1.HelloService.Error
buf: * Dialing (tcp) localhost:50051...
buf: * Connected to [::1]:50051
buf: > (#1) POST /hello.v1.HelloService/Error
buf: > (#1) Accept-Encoding: identity
buf: > (#1) Content-Type: application/grpc
buf: > (#1) Grpc-Accept-Encoding: gzip
buf: > (#1) Grpc-Timeout: 119981m
buf: > (#1) Te: trailers
buf: > (#1) User-Agent: grpc-go-connect/1.17.0 (go1.22.10) buf/1.49.0
buf: > (#1)
buf: } (#1) [11 bytes data]
buf: * (#1) Finished upload
buf: < (#1) HTTP/2.0 200 OK
buf: < (#1) Content-Type: application/grpc+proto
buf: < (#1) Date: Fri, 17 Jan 2025 06:22:37 GMT
buf: < (#1) Grpc-Encoding: gzip
buf: < (#1)
buf: < (#1)
buf: < (#1) Grpc-Message: foo%20
buf: < (#1) Grpc-Status: 3
buf: * (#1) Call complete
{
   "code": "invalid_argument",
   "message": "foo "
}

Next is the case of the server implementation of grpc-ruby.
In this implementation, spaces are not percent encoded and messages are lost on connect-node clients.

ruby server.rb
❯ node client.js "foo "
{
  code: 3,
  rawMessage: '',
  metadata: Headers {
    'grpc-status': '3',
    'content-type': 'application/grpc',
    'grpc-accept-encoding': 'identity, deflate, gzip'
  }
}
❯ pnpm exec buf curl -v --protocol grpc --schema ./proto -d '{"message":"foo "}' --http2-prior-knowledge http://localhost:50051/hello.v1.HelloService/Error
buf: * Invoking RPC hello.v1.HelloService.Error
buf: * Dialing (tcp) localhost:50051...
buf: * Connected to 127.0.0.1:50051
buf: > (#1) POST /hello.v1.HelloService/Error
buf: > (#1) Accept-Encoding: identity
buf: > (#1) Content-Type: application/grpc
buf: > (#1) Grpc-Accept-Encoding: gzip
buf: > (#1) Grpc-Timeout: 119980m
buf: > (#1) Te: trailers
buf: > (#1) User-Agent: grpc-go-connect/1.17.0 (go1.22.10) buf/1.49.0
buf: > (#1)
buf: } (#1) [11 bytes data]
buf: * (#1) Finished upload
buf: < (#1) HTTP/2.0 200 OK
buf: < (#1) Content-Type: application/grpc
buf: < (#1) Grpc-Accept-Encoding: identity, deflate, gzip
buf: < (#1)
buf: < (#1)
buf: < (#1) Grpc-Message: foo
buf: < (#1) Grpc-Status: 3
buf: * (#1) Call complete
{
   "code": "invalid_argument",
   "message": "foo "
}

Space is part of Percent-Byte-Unencoded, it appears that the grpc-ruby implementation must also be able to handle it properly.

https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#responses

Environment (please complete the following information):

  • @connectrpc/connect-node version: 2.0.1
@mashiro mashiro added the bug Something isn't working label Jan 17, 2025
@timostamm
Copy link
Member

Thanks for the report. This looks like a bug in Node.js:

When a http2 server sends headers with leading or trailing whitespace:

  res.writeHead(200, {
    "msg1": "foo ",
    "msg2": "foo%20",
    "msg3": " foo",
    "msg4": "%20foo",
  });

The headers are received as expected with curl:

< HTTP/2 200 
< msg1: foo 
< msg2: foo%20
< msg3:  foo
< msg4: %20foo

But a Node.js http2 client (v22.12.0) drops them:

 [Object: null prototype] {
  ':status': 200,
  msg2: 'foo%20',
  msg4: '%20foo',
Script to reproduce
import { createServer, connect } from "node:http2";

const port = 8011;

const server = createServer(async (req, res) => {
  for await (const _ of req) {
    //
  }
  res.writeHead(200, {
    "msg1": "foo ",
    "msg2": "foo%20",
    "msg3": " foo",
    "msg4": "%20foo",
  });
  res.end();
});
server.listen(port, () => {
  // Alternatively, comment out call(), and run:
  // curl -v http://localhost:8011 --http2-prior-knowledge
  call();
});

function call() {
  const client = connect(`http://localhost:${port}`);
  client.on("error", (err) => console.error(err));
  const req = client.request({
    ":path": "/",
  });
  req.on("response", (headers, flags) => {
    console.log("client received response headers:", headers);
  });
  req.on("end", () => {
    client.close();
    server.close();
  });
  req.end();
}

@bobaaaaa
Copy link

Hi @timostamm is this the same bug I found regarding?:

  • connect-fastify plugin
  • grpc transport (http2)
  • ConnectError + metadata

I reproduced the bug here: https://github.com/bobaaaaa/connect-fastify-bug

If not, I can create a new issue.

@timostamm
Copy link
Member

@mashiro, I've filed nodejs/node#56703. This issue most likely also affects @grpc/grpc-js (https://github.com/grpc/grpc-node/).

From my understanding, leading or trailing space in a header field is either malformed or should be trimmed in HTTP/2. See details in the linked Node.js issue.

The grammar in the gRPC spec may not take this into account. I recommend not to use error messages with leading or trailing whitespace in gRPC because the behavior is implementation-specific (although Node.js certainly stands out by dropping headers completely). As a workaround, you can try to percent-encode error messages before you pass them to the gRPC implementation. Alternatively, or additionally, you can encode the error in a grpc-status-details-bin header, which should preserve any UTF-8 error message, and is typically preferred over the grpc-message header by implementations (if they support it).

I'm going to close this issue, and hope that the upstream bug is fixed soon.

@timostamm
Copy link
Member

@bobaaaaa, this looks like a separate issue, please feel free to create a new one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants