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

Add server setting for max HTTP request header line length. #2755

Merged
merged 3 commits into from Nov 27, 2023

Conversation

charlesreiss
Copy link
Contributor

Instead of using a hard-coded length of 4096 for the maximum size of lines in the HTTP headers (including the status line), make this configurable in HTTPServerSettings.

@s-ludwig
Copy link
Member

dub test :web appears to result in a segmentation fault with the changes. From the diff alone I'm not sure what causes it.

@charlesreiss
Copy link
Contributor Author

It looked like the problem was that createTestHTTPServerRequest didn't setup m_settings in the HTTPServerRequest, so the test used a null pointer when trying to access the new max line length settings (e.g. for form-parsing).

I believe I've fixed it (by using a default-constructed HTTPServerSettings).

@@ -657,6 +658,9 @@ final class HTTPServerSettings {
/// the url and all headers.
ulong maxRequestHeaderSize = 8192;

/// Maximum number of bytes in a single line in the request header.
ulong maxRequestHeaderLineSize = 4096;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be size_t in order to be able to be passed to parseFormData on 32-bit builds. maxRequestHeaderSize should probably also be a size_t, since it needs to fully fit into memory, too, but that's a different issue.

Minor note: the indentation here uses spaces instead of tabs.

Instead of using a hard-coded length of 4096 for the maximum size of
lines in the HTTP headers (including the status line), make this
configurable in HTTPServerSettings.
@s-ludwig s-ludwig merged commit dd9fddd into vibe-d:master Nov 27, 2023
7 checks passed
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

Successfully merging this pull request may close these issues.

None yet

2 participants