jhthorsen / mojolicious-plugin-openapi

OpenAPI / Swagger plugin for Mojolicious
54 stars 44 forks source link

OpenAPI v3: Request body validation problems #128

Closed NGEfreak closed 5 years ago

NGEfreak commented 5 years ago

Validation of a request body that requires a object does not work as expected in these cases:

  1. request body contains "null"
  2. request body contains invalid JSON
  3. request body contains an array

Here is a test script:

#!/usr/bin/env perl

use 5.010;
use strict;
use warnings;
use utf8;

use Test::More;
use Test::Mojo;

use Mojolicious::Lite;

post '/test' => sub {
    my $c = shift;
    $c->openapi->valid_input or return;
    $c->render( json => undef, status => 200 );
  },
  'test';

plugin OpenAPI => {
    url    => 'data:///api.yml',
    schema => 'v3',
};

my $t = Test::Mojo->new();

# Valid request should be ok
$t->post_ok( '/test', json => { foo => 'bar' } )
  ->status_is(200);

# Missing property should fail
$t->post_ok( '/test', json => {} )
  ->status_is(400)
  ->json_is( '/errors/0/message', 'Missing property.' );

# Array should fail
$t->post_ok( '/test', json => [] )
  ->status_is(400)
  ->json_is( '/errors/0/message', 'Expected object - got array.' );

# Null should fail
$t->post_ok( '/test', json => undef )
  ->status_is(400)
  ->json_is( '/errors/0/message', 'Expected object - got null.' );

# Invalid JSON should fail
$t->post_ok( '/test', { 'Content-Type' => 'application/json' } => 'invalid_json' )
  ->status_is(400)
  ->json_is( '/errors/0/message', 'Expected object - got null.' );

done_testing;

__DATA__

@@ api.yml
openapi: 3.0.0
info:
  title: Test
  version: 0.0.0
paths:
  /test:
    post:
      x-mojo-name: test
      requestBody:
        required: true
        content:
          application/json:
            schema:
              properties:
                foo:
                  type: string
              required:
                - foo
      responses:
        '200':
          description: ok

Regarding 1. and 2.

The null and invalid JSON testcases fail because of line 308.

Before this line the value of $body_schema is

\ {
    content    {
        application/json   {
            schema   {
                properties   {
                    foo   {
                        type   "string"
                    }
                },
                required     [
                    [0] "foo"
                ]
            }
        }
    },
    required   true
}

Afterwards the value of the variable is

\ {
    schema   {
        properties   {
            foo   {
                type   "string"
            }
        },
        required     [
            [0] "foo"
        ]
    }
}

The "required" key is lost at this point with the result that the method _validate_request_value returns too early on line 322.

Regarding 2.

After this problem is fixed, invalid JSON and Null would still produce the same error: Expected object - got null. Invalid JSON is treated as Null/Undef because the method json of Mojo::Message::Request returns undef in both cases. IMHO, it would be more helpful when there is a different error message in the case of invalid JSON.

Regarding 3.

The array testcase fails because the schema does not explicitly state that the type is an object. When I add type: object to the request body schema in the spec it works.

I could not find in the OpenAPI specification if the type key is optional or required. However, Swagger UI/Editor and JSON::Validator do not complain that the type key is missing. Also, it looks like Swagger UI/Editor sets the type to object implicitly in this case.

jhthorsen commented 5 years ago

From the way I read it, non of the cases works as you expect, since you don't have "type". It's really weird, but detection of "type" only applies if the input data is matching the rules. Look here for an example acceptance test: https://github.com/mojolicious/json-validator/blob/master/t/draft4-tests/minProperties.json#L22

Please let me know where in the spec where this has changed, if that is the case.

jhthorsen commented 5 years ago

When it comes to the "null" cases, I just fixed that in the commit above.

The reason why this issue has so many labels is because you reported so many bugs in one issue.