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

HTTP headers should be seperated by CRLF #58

Open
nickshanks opened this issue Jan 24, 2018 · 5 comments
Open

HTTP headers should be seperated by CRLF #58

nickshanks opened this issue Jan 24, 2018 · 5 comments
Assignees

Comments

@nickshanks
Copy link

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.

@nickshanks nickshanks self-assigned this Jan 24, 2018
@nevali
Copy link
Member

nevali commented Jan 25, 2018

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
Copy link
Author

nickshanks commented Jan 25, 2018

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
Copy link
Author

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
Copy link
Member

nevali commented Jan 26, 2018

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
Copy link
Author

nickshanks commented Jan 26, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants