Skip to content

Conversation

@wlandry
Copy link
Contributor

@wlandry wlandry commented Apr 18, 2016

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.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 81.476% when pulling 85848b9 on wlandry:large_POST into 496458f on etr:master.

@wlandry wlandry mentioned this pull request Apr 18, 2016
@coveralls
Copy link

Coverage Status

Coverage remained the same at 81.476% when pulling 369707b on wlandry:large_POST into 496458f on etr:master.

@wlandry
Copy link
Contributor Author

wlandry commented Apr 18, 2016

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.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 81.476% when pulling 2bdbda9 on wlandry:large_POST into 496458f on etr:master.

void set_arg(const std::string& key, const std::string& value)
{
this->args[key] = value;
this->args[key] = value.substr(0,content_size_limit);
Copy link
Owner

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.

Copy link
Contributor Author

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.

Copy link
Owner

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.

Copy link
Contributor Author

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.

wlandry added 2 commits April 19, 2016 16:34
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
@coveralls
Copy link

Coverage Status

Coverage remained the same at 81.476% when pulling 1fe733f on wlandry:large_POST into e396fe4 on etr:master.

@etr etr merged commit ee5de74 into etr:master Jul 10, 2016
@etr
Copy link
Owner

etr commented Jul 10, 2016

Thanks. All looks good overall.

@wlandry wlandry deleted the large_POST branch July 14, 2016 23:25
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.

3 participants