add option for max request content length #221
Conversation
|
For now I've only updated the uws_generic_method_handler, but I believe we'd need to pass the content length in the uws_generic_method_handler_with_extension as well. |
|
Opening this again, closed the other after fixing a local mess hehe |
src/socketify/socketify.py
Outdated
| @@ -1001,6 +1001,8 @@ def uws_generic_method_handler(res, req, user_data): | |||
| response = AppResponse(res, app) | |||
| request = AppRequest(req, app) | |||
|
|
|||
| response._content_length = int(request.get_header("content-length")) | |||
There was a problem hiding this comment.
we should not trust the content-length header in this case, we need to check the total data received because the user may be using chunked encoding.
There was a problem hiding this comment.
cool, that makes sense, thanks for the review. And I even checked bun's code 😅
if ((HTTP.Method.which(req.method()) orelse HTTP.Method.OPTIONS).hasRequestBody()) {
const req_len: usize = brk: {
if (req.header("content-length")) |content_length| {
break :brk std.fmt.parseInt(usize, content_length, 10) catch 0;
}
break :brk 0;
};
if (req_len > this.config.max_request_body_size) {
resp.writeStatus("413 Request Entity Too Large");
resp.endWithoutBody(true);
this.finalize();
return null;
}There was a problem hiding this comment.
Oh yeah but we should not trust this value and check the same when chunked encoding, this can only be used to short circuit
src/socketify/socketify.py
Outdated
| @@ -2035,6 +2038,11 @@ def on_aborted(self, handler): | |||
| return self | |||
|
|
|||
| def on_data(self, handler): | |||
| if self.app.max_content_length is not None and self._content_length is not None: | |||
There was a problem hiding this comment.
We would need to add the extra config so we check this here: https://github.com/cirospaciari/uWebSockets/blob/27848ed1bae7a51044dfb05047d192a869e01105/src/HttpContext.h#L218-L236 Today uWS has a maxPayloadLength but looks like is not applied to Http only to WS
There was a problem hiding this comment.
one question: how do I rebuild it locally so I can test with the new changes to this HttpContext.h?
There was a problem hiding this comment.
sure you can find the steps here https://github.com/cirospaciari/socketify.py?tab=readme-ov-file#hammer-building-from-source
after this if you change the local code and rebuild it will use the updated code
There was a problem hiding this comment.
I am facing an error building it locally on linux (ubuntu), do you know why this is happening?
# build uWebSockets
cd ../uWebSockets/uSockets && cc -I src -I boringssl/include -DUWS_WITH_PROXY -DLIBUS_USE_OPENSSL -DLIBUS_USE_LIBUV -pthread -fPIC -std=c11 -O3 -c src/*.c src/eventing/*.c src/crypto/*.c
In file included from src/internal/internal.h:41,
from src/bsd.c:23:
src/internal/eventing/libuv.h:23:10: fatal error: uv.h: No such file or directory
23 | #include <uv.h>
| ^~~~~~
compilation terminated.
In file included from src/internal/internal.h:41,
from src/context.c:21:
src/internal/eventing/libuv.h:23:10: fatal error: uv.h: No such file or directory
23 | #include <uv.h>
| ^~~~~~
compilation terminated.
In file included from src/internal/internal.h:41,
from src/loop.c:21:
There was a problem hiding this comment.
thank you, that fixed the issue.
Now I'd like to confirm how I should add this extra config... I was initially planning to add a new parameter (max_content_length) to the us_socket_context_options_t struct (AppOptions), but noticed it is used for SSL.
There was a problem hiding this comment.
@CarlosEduR you can create a function on TemplateApp to set this option and avoid us_socket_context_options_t since us_socket_context_options_t is usockets not uWS and this option is only for the HttpContext.h aka you should put this option in HttpContextData.h
There was a problem hiding this comment.
@cirospaciari thanks for clarifying that, learning a lot here.
How can I open a pull request for the changes in the uWebSockets code? I just forked your fork, but I am not sure about which branch I should compare against when opening the PR.
There was a problem hiding this comment.
You can just open a PR to my fork of uWebSockets and I will review and merge and after that we can update socketify to point to the right version + necessary changes the branch is capi-complete-with-ssl-and-samples you can find https://github.com/cirospaciari/socketify.py/blob/main/.gitmodules
There was a problem hiding this comment.
I forgot I could check it there, thanks!! PR opened
…o feat/max-content-length
Description
This PR fixes #120