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

Undici incorrectly rejects responses with trailer header but no matching trailer #1418

Closed
pimterry opened this issue May 6, 2022 · 2 comments · Fixed by #1419
Closed

Undici incorrectly rejects responses with trailer header but no matching trailer #1418

pimterry opened this issue May 6, 2022 · 2 comments · Fixed by #1419
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@pimterry
Copy link
Member

pimterry commented May 6, 2022

Bug Description

When a server sends a Trailer: X header, it's saying that it may send an X trailer field after the response. Undici treats this as a commitment, and throws an error if the response has no such trailer, for no apparent reason.

This seems to be an intentional check here discussed originally in #432. There's no reference to any spec there that says this is necessary though (and I can't find one) just discussion suggesting maybe it's odd. Undici should follow the spec, not reject responses that seem weird.

There are real servers (e.g. all IPFS nodes, as noted in that issue at the time) that are broken by this in many cases, since they return trailer: X-Stream-Error with all streaming responses, to indicate that they may add stream errors after the body in some cases.

Browser fetch & node HTTP don't enforce this, and no spec says this is invalid - this check should just be removed imo.

Reproducible By

const http = require('http');

http.createServer((req, res) => {
    res.writeHead(200, {
        trailer: 'test'
    });
    res.end('body');
}).listen(8008);

// Global Undici fetch from Node 18:
fetch('http://localhost:8008').then((res) => {
    console.log('got status', res.status);
    return res.text();
}).then((body) => {
    console.log('got body', body);
})
.catch(console.error);

This prints:

TypeError: terminated
    at Fetch.onAborted (/tmp/tmp.EolblgSMPd/node_modules/undici/lib/fetch/index.js:1900:49)
    at Fetch.emit (node:events:527:28)
    at Fetch.terminate (/tmp/tmp.EolblgSMPd/node_modules/undici/lib/fetch/index.js:79:10)
    at Object.onError (/tmp/tmp.EolblgSMPd/node_modules/undici/lib/fetch/index.js:2034:34)
    at Request.onError (/tmp/tmp.EolblgSMPd/node_modules/undici/lib/core/request.js:233:27)
    at errorRequest (/tmp/tmp.EolblgSMPd/node_modules/undici/lib/client.js:1728:13)
    at Socket.onSocketClose (/tmp/tmp.EolblgSMPd/node_modules/undici/lib/client.js:1002:5)
    at Socket.emit (node:events:527:28)
    at TCP.<anonymous> (node:net:715:12) {
  [cause]: TrailerMismatchError: Trailers does not match trailer header
      at Parser.onMessageComplete (/tmp/tmp.EolblgSMPd/node_modules/undici/lib/client.js:864:30)
      at wasm_on_message_complete (/tmp/tmp.EolblgSMPd/node_modules/undici/lib/client.js:383:30)
      at wasm://wasm/0002afd2:wasm-function[45]:0x8dc
      at wasm://wasm/0002afd2:wasm-function[56]:0x1ad3
      at wasm://wasm/0002afd2:wasm-function[55]:0xcd7
      at wasm://wasm/0002afd2:wasm-function[21]:0x4e4
      at Parser.execute (/tmp/tmp.EolblgSMPd/node_modules/undici/lib/client.js:515:22)
      at Parser.readMore (/tmp/tmp.EolblgSMPd/node_modules/undici/lib/client.js:484:12)
      at Socket.onSocketReadable (/tmp/tmp.EolblgSMPd/node_modules/undici/lib/client.js:925:10)
      at Socket.emit (node:events:527:28) {
    code: 'UND_ERR_TRAILER_MISMATCH'
  }
}

Expected Behavior

This should work as normal, as if the trailer field was not present.

This works correctly in Node HTTP, node-fetch & browser fetch, so this produces regressions for everybody using a fetch polyfill with Node 18.

Environment

Ubuntu 20.04, using global from Node 18.0.0 or Node 16.14.2 with Undici 5.1.1.

Additional context

Looks like this was causing errors in the past too, e.g. #880 says it broke all requests to pinterest.

@pimterry pimterry added the bug Something isn't working label May 6, 2022
@mcollina
Copy link
Member

mcollina commented May 6, 2022

Would you like to send a Pull Request to address this issue? Remember to add unit tests.

@mcollina mcollina added the good first issue Good for newcomers label May 6, 2022
@pimterry
Copy link
Member Author

pimterry commented May 6, 2022

Yep, I'll sort that now.

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

Successfully merging a pull request may close this issue.

2 participants