Skip to content
New issue

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

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

Already on GitHub? # to your account

Do we need to verify trailers? #432

Closed
ronag opened this issue Sep 23, 2020 · 10 comments · Fixed by #433
Closed

Do we need to verify trailers? #432

ronag opened this issue Sep 23, 2020 · 10 comments · Fixed by #433
Labels
enhancement New feature or request

Comments

@ronag
Copy link
Member

ronag commented Sep 23, 2020

When a message includes a message body encoded with the chunked
transfer coding and the sender desires to send metadata in the form
of trailer fields at the end of the message, the sender SHOULD
generate a Trailer header field before the message body to indicate
which fields will be present in the trailers. This allows the
recipient to prepare for receipt of that metadata before it starts
processing the body, which is useful if the message is being streamed
and the recipient wishes to confirm an integrity check on the fly.

The way I read the spec the server/client may send trailers that are not included in the Trailers header. However, how should one handle the case where the client/server does not send a specified trailer? i.e. to we need a "trailer mismatch error"?

@ronag ronag added the question [Use a Discussion instead] label Sep 23, 2020
@ronag
Copy link
Member Author

ronag commented Sep 23, 2020

@szmarczak @mcollina

@szmarczak
Copy link
Member

szmarczak commented Sep 23, 2020

I think no. I read SHOULD as is highly recommended to, which means: optional.

@ronag
Copy link
Member Author

ronag commented Sep 23, 2020

I think no. I read SHOULD as is highly recommended to, which means: optional.

Actually I'm referring to something else. The spec says that the request header should be included. It does not say anything about what to do if the request header includes a key that is not part of the trailers.

@ronag
Copy link
Member Author

ronag commented Sep 23, 2020

Consider:

Ok

res.writeHead(200, { Trailer: 'foo' })
res.writeTrailer('foo', 'asd')
res.writeTrailer('bar', 'asd') // not specified in header
res.end()

Not ok?

res.writeHead(200, { Trailer: 'foo' })
res.writeTrailer('bar', 'asd') 
// foo is not sent?
res.end()

@szmarczak
Copy link
Member

Ah. Yeah, it makes sense to throw. IMO it's invalid.

@ronag ronag added enhancement New feature or request and removed question [Use a Discussion instead] labels Sep 23, 2020
ronag added a commit that referenced this issue Sep 23, 2020
ronag added a commit that referenced this issue Sep 24, 2020
ronag added a commit that referenced this issue Sep 24, 2020
@olizilla
Copy link

Just a note that this causes an Error to be thrown when using undici to fetch from an IPFS Gateway API.

That api always sends a trailer: X-Stream-Error header, but will only actually send that trailer if an error occurs mid-stream. Code that used to work is now failing when we switch to using undici. Originally reported here: web3-storage/web3.storage#1017 by @flea89

Would you consider a PR to make absent trailers not be an error?

@flea89
Copy link

flea89 commented Mar 1, 2022

Thanks, @olizilla for reporting this here.

@ronag, we'd really appreciate your view on this one?
Happy to help.

@ronag
Copy link
Member Author

ronag commented Mar 1, 2022

What does the spec say?

@szmarczak
Copy link
Member

https://httpwg.org/specs/rfc7230.html#rfc.section.4.1.2

It doesn't explicitly say the headers must be present, so I guess they can be omitted.

@flea89
Copy link

flea89 commented Mar 1, 2022

From what I can see from Hypertext Transfer Protocol (HTTP/1.1): Message Syntax and Routing # header.trailer and Hypertext Transfer Protocol (HTTP/1.1): Message Syntax and Routing # chunked.trailer.part the spec doesn't explicitly specify whether a trailer field should be included at the end of the chunked response or not.

Also looking at the decoding section it suggests to read until the field it's empty and append the headers that were provided in the trailers, so my guess is that they can be omitted?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants