plack / Plack

PSGI toolkit and server adapters
http://plackperl.org/
Other
486 stars 214 forks source link

Plack::Request restore/allow default parsing of application/json bodies with HTTP::Entity::Parser #678

Closed jwrightecs closed 2 years ago

jwrightecs commented 2 years ago

register HTTP::Entiry::Parser::JSON in Plack::Request to support JSON body parsing for ->body_parameters, for content-type application/json, similarly to HTTP::Body

miyagawa commented 2 years ago

This PR is rejected due to the following reasons:

  1. This is a breaking change. $req->body_parameters used to be empty for application/json requests and now returns the parsed data from JSON.
  2. Related to that, and more importantly, that will open a security vulnerability.

Imagine you have a following code:

my $req  = Plack::Request->new($env);
my $id   = $req->body_parameters->{item_id};
my $item = Shop::Item->lookup($id);
# this will eventually become:
# ['SELECT * FROM shop_item WHERE item_id = ?', $id]

With existing UrlEncoded and MultiPart parser, body_parameters is guaranteed to be one-level inside-out multivalue hash (Hash::MultiValue object), so$params->{item_id} ($id) is always a scalar string value, or undef, and never a reference.

If you automatically load JSON parser, now $req->body_parameters->{item_id} could become a nested structure, an array ref or some hash ref. Depending on an underlying ORM or SQL builder, your SQL could become something like:

# ['SELECT * FROM shop_item WHERE item_id IN (?,?,?)', @$id ]

depending on the inputs. This is a serious security hole and a lot of frameworks suffer from this type of errors when you allow arbitrary data types as inputs.

https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2013-0155 http://blog.kazuhooku.com/2014/07/the-json-sql-injection-vulnerability.html

To mitigate this while allowing JSON input, you have to strictly validate the incoming data structure before passing the parameters to any other models. That's a good practice anyway, but that's a breaking change, and it needs to be an opt-in.

If you do want this behavior with the caveat above understood, you could simply do:

my $req = Plack::Request->new($env);
$req->request_body_parser->register("application/json", "HTTP::Entity::Parser::JSON");
...

on your own.