cloudformation-resource-schema icon indicating copy to clipboard operation
cloudformation-resource-schema copied to clipboard

Validate $ref correctly

Open tobywf opened this issue 4 years ago • 0 comments

Issue

See also https://github.com/aws-cloudformation/aws-cloudformation-rpdk/issues/66

TL;DR: If we validate the schema as data, and our meta schema allows $ref, it doesn't work, as the "data" i.e. the schema is not further processed:

@Test
public void improperValidation() {
    final JSONObject definition = new JSONObject()
            .put(TYPE_NAME_KEY, EXAMPLE_TYPE_NAME)
            .put(DESCRIPTION_KEY, EXAMPLE_DESCRIPTION)
            .put(PROPERTIES_KEY, new JSONObject()
                .put("propertyA", new JSONObject()
                    .put("$ref", "#/garbage")))
            .put(PRIMARY_IDENTIFIER_KEY, Arrays.asList(EXAMPLE_PRIMARY_IDENTIFIER));

    validator.validateResourceDefinition(definition);
}

this passed, but the $ref is obviously garbage. The reason it "passes" is because we allow $refs:

https://github.com/aws-cloudformation/aws-cloudformation-resource-schema/blob/6a1ef45d612d7c90da1d77e71a9427a64cdd5237/src/main/resources/schema/provider.definition.schema.v1.json#L85-L87

however, all JSON schema mandates is:

"$ref": {
    "type": "string",
    "format": "uri-reference"
},

Solutions?

The obvious solution is to resolve all $refs before validation, and then it'd work. In Python, I'd use jsonref, or disallow circular references and simply inline all references, and disallow $ref.

It may be possible to use org.everit.json.schema.loader.SchemaLoader in some capacity. Instead of validating against draft-7 (the usual meta-schema), it may be possible to use our meta-schema instead. Since SchemaLoader handles $refs semantically, that ought to work. (I had planned to do something like this in the RPDK, where the library/dynamic nature makes it fairly straightforward to do.)

If in future we want to restrict $refs to the local or any hosted schemas, putting in our own code could be beneficial here in the long run.

tobywf avatar Jul 09 '19 02:07 tobywf