-
Notifications
You must be signed in to change notification settings - Fork 191
Handle POST's larger than 1024 bytes correctly. #95
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
Conversation
…s or arguments. The default is unlimited.
|
I have added a facility to limit the size of POST's, so that attackers can not upload terabyte files that crash the server. The default is unlimited. |
| void set_arg(const std::string& key, const std::string& value) | ||
| { | ||
| this->args[key] = value; | ||
| this->args[key] = value.substr(0,content_size_limit); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, it feels like throwing an exception would be more transparent - at least the library consumer will be notified that it happened and will avoid using partial data as argument unless they want to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did think of that. However, if I threw an exception, the functions webserver::build_request_args, and webserver::post_iterator would have to catch it immediately. They are invoked by libmicrohttpd, so they can not propagate exceptions. Then we would still need a way to indicate that too many characters were POST'ed.
With the solution I gave, the user can check whether each of the args has size >= content_size_limit. We could even add a simple function to check overflow. A C++98 version would be something like.
bool overflow() const
{
for (std::map<std::string, std::string, http::arg_comparator>::const_iterator arg=args.begin();
arg!=args.end(); ++arg)
{
if (arg->second.size()>=content_size_limit)
return true;
}
return false;
}Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see.
I think, that having this kind of semantic, a utility such as the one you are describing could be very useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out to be enough to just check the size of content. I added a function content_too_large().
I also had to roll back pellucide's modification since it conflicted with checking the size.
This removes the modification done in pellucide's branch to append in set_arg. That is instead done in webserver::post_iterator and webserver::finalize_answer. Conflicts: src/httpserver/http_request.hpp
|
Thanks. All looks good overall. |
webserver::post_iterator unconditionally sets a key to the last value parsed. But if the post is larger than 1024 bytes, it will parse it incrementally. That means that the key will be set to the last part, instead of the whole POST. This fix prepends the existing value if any.