bbcarchdev / quilt

Linked Open Data publication engine
Apache License 2.0
6 stars 1 forks source link

HTTP headers should be seperated by CRLF #58

Open nickshanks opened 6 years ago

nickshanks commented 6 years ago

All of the calls in libquilt and patchwork to quilt_request_headers() include a LF character at the end. Firstly, this detail should be handled by the implementations, not repeated across the codebase, and secondly it is incorrect. The HTTP specs define the header line seperator (and headers—body—footers seperators) to be CRLF (twice, for seperating envelope components).

The seperator should be moved to the implementations: servers/cli/cli.c and servers/fcgi/fcgi.c, and should be changed to \r\n. Likewise, all the places where data->headers_sent is set to 1, and a \n emitted, should call a single new function which emits \r\n.

nevali commented 6 years ago

Good spot, although it should only be the FastCGI server; the CLI technically doesn't emit HTTP (and indeed should probably not emit the headers at all unless -v is given)

nickshanks commented 6 years ago

But would we want the cli and fcgi output to differ? What if the cli server was used to pipe ‘HTTP’ to some other process, in lieu of curl localhost? Perhaps we could make the CLI emit headers on -i and -I (capital i) to match curl? The CLI could even determine if the output environment is a terminal window vs. a redirect/pipe.

nickshanks commented 6 years ago

Actually, we should probably introduce a new API, quilt_request_header() that only takes a single header line, and document quilt_request_headers() to say it needs CRLF. quilt_request_headers() can currently take any number of whole or partial header lines as a single string, and we don't want to break the ABI.

nevali commented 6 years ago

We should add a specific CGI SAPI for the 'pipe HTTP' case, as that would do exactly as you do describe.

quilt_request_header() would be even better if it specifically took (name, value, append|replace) and buffered accordingly within libquilt (with case-insensitivity on name handled properly)

nickshanks commented 6 years ago

I feel this feature speculation is starting to detract from the issue of incorrect line endings. Fixing the bug and improving the API ought to be separate issues, with the former a higher priority — I'll create a new ticket for the latter.