Gavel doesn't support $ref in schemas

[ecordell]

Gavel uses Amanda for validating payloads against a JSON Schema. Unfortunately Amanda doesn't support $ref in schemas, and doesn't look like it will be added any time soon (see this issue from two years ago).

This is a feature I would like to have in Gavel. It seems to me that the options are:

• Fork Amanda and add $ref support
• Use an alternative to Amanda for schema validation:
  • https://github.com/tdegrunt/jsonschema
  • https://github.com/akidee/schema.js
  • https://github.com/garycourt/JSV
  • https://github.com/zaggino/z-schema
  • Probably many more

[Almad]

@ecordell Do you have any experience with the other implementations?

@Baggz is actually with us at @apiaryio, but I do think we might migrate away from Amanda, but we've yet to done our research homework. Feedback on this would be appreciated.

[ecordell]

I saw that @apiaryio had a fork of Amanda, but didn't realize @Baggz was there as well.

I have no experience with the other implementations, but a couple of them looked like they supported more of the schema spec (and some support draf4 features, thought it's not yet been published afaik).

It's worth noting that none of them have plans to support other schema languages, so far as I can tell, so if that's the end goal it may make sense to just get Amanda to support the full spec rather than jumping ship.

Of the projects I listed, jsonschema supports most of the spec (all of draft3 and some of draft4) and is actively developed (last commit was a few days ago). Z-Schema supports all of draft4 and looks actively developed. JSV looks like a good project and supports all of draft three, but looks unmaintained as of July.

But I haven't tried using any of these so I'd be wary to suggest anything based simply on what it says in the Readmes!

[cjha]

Are there any plans to address this? Z-Schema and https://github.com/natesilva/jayschema seem like decent alternatives for draft4 support.

[Almad]

@cjha Yes, however it is a bit deeper in queue, because we do have a diff viewer that is fairly dependent on gavel's output...and that is fairly dependent on Amanda's.

Are you guys willing to give it a shot?

[joaosa]

@Almad any updates on this? :)

[Almad]

@honzajavorek It's yours now :P

[honzajavorek]

@joaosa I think Gavel should be able to currently validate both draft v3 (by Amanda?) and draft v4 (by tv4?), but to be honest, I have currently very little insight on what actually happens under the hood and would have to check.

Amanda has last commit 2 years ago and no new development should be expected there. Not sure what are @Baggz's plans with it.

As I said, I'd have to check, but

• if Gavel supports v4 and $ref works there, my best advice is that you should consider migrating to v4.
• If $ref is not supported in Gavel's v4 implementation, then I would like us to do something about it.
• If v4 is not supported in Gavel at all, then 😱

My bet is that the second list item is true (again, this is not verified, it's from the top of my head). So if that's really true and you want to help us to get faster to a solution, you may want to contribute a failing test. Would that work for you?

[joaosa]

@honzajavorek Thank you for the prompt response :)

I think I managed to produce two failing tests and added the corresponding schemas:

• added a test that should throw when invalid data is passed a valid $ref schema
  • the rationale is to ensure $ref schema validation works as expected.
  • https://github.com/joaosa/gavel.js/blob/master/test/unit/validators/json-schema-test.coffee#L193
• added a test that should throw when a schema with an invalid $ref is passed data
  • I added the data check as an attempt to hint at the data format not matching the schema expectations, as according to this, there might not be an error if the $ref URI can't be resolved (as {} will match anything).
  • https://github.com/joaosa/gavel.js/blob/master/test/unit/validators/json-schema-test.coffee#L206

If needed, I can also place the tests in a more appropriate location within the test suite.
Also, wondering if the second test makes sense.

Hope it helps!

[honzajavorek]

@joaosa Thanks! This is great. Both tests make sense to me.

[joaosa]

@honzajavorek Any updates? :)

[honzajavorek]

@joaosa I'm sorry, not yet. Some priority stuff stepped in.

[joaosa]

@honzajavorek checking in again. I could potentially look into this myself :)

[honzajavorek]

@joaosa Thanks for pinging and thanks for the effort! I'd be glad to advise you in case you want to work on this 👍 I got back to your tests and I have several comments:

• You added tests for scenarios which prove Gavel.js behaves correctly when invalid schema/ref is passed. I don't know why I didn't realize it previously, but that's only testing negative cases. There should be also tests for some positive cases, i.e. what happens when valid ref is passed, how Gavel.js would be expected to behave. (In code, that would be describe 'when valid v4 $ref schema provided', with valid data assumed.)
• I think location of your tests is all right.
• We need to make sure (means there should be tests for it) user cannot reference an external file in the $ref or that only JSON Schema files are supported when referenced. Otherwise, it could lead to security issues. I'd say let's first disable referencing of files and if someone needs it, we can discuss how to approach it in a safe way.

When there are all necessary tests (failing tests, because the feature isn't there yet), we can start to implement the feature and make the tests green. Then let's make a Pull Request and strive for releasing the feature.

Gavel uses tv4 to validate Draft4 schemas and according to its README, it should support $ref without any problems. It would be up to you to find out why this doesn't work in case of Gavel.js. Maybe just playing with tv4 settings or upgrading to the latest version will add the feature.

Good luck! Please get back here if you make any progress or if you get stuck, I'll try to help (and be more responsive). I hope what I wrote will help you to proceed.

Note: Since Gavel.js uses Semantic Release, please read this section and make sure your commits are formatted accordingly. Only in that case the automatic releasing will work when your PR gets accepted and merged. Thanks!

[joaosa]

@honzajavorek Thank you for the feedback :)

I have some updates on this:

• Gavel's code initially validates a given schema against a meta schema and only validates the data against that schema if it is considered valid. Consequently, tv4's missing schema detection is only triggered on that stage (validatePrivate). To account for this I moved the new tests to the validPrivate's describe.

• The validation of data against a ref to a valid schema was working properly already. Either way, now there's a test to ensure it doesn't break.

• Invalid schema ref errors weren't being caught and I reformatted tv4's missing ref error output (e.g. ['', '': '']) to accommodate the expected error format.

• Updated the new fixtures to include no self-referencing JSON Pointer fragment paths, as before I had foo pointing to another definition of foo. This doesn't seem to be a real issue, but it makes the fixtures simpler.

• I added the missing positive test case, but it's pretty much only testing for the absence of an error.

Finally, I squashed some of my old commits and hopefully followed the Semantic Release principles. I submitted a PR. Let me know if this is looking okay :)

As for the external file refs, those would be especially useful in my use-case. I could work around this if there's a way to use local schema refs to a global definitions object inside an apib file.

[honzajavorek]

@joaosa Great! 👍 🎉 💕

• I think we're testing validateSchemaV4() now and in a pure sense of unit testing, it would be probably nicer to reflect that fact in the tests. However, looking at the current code, I think what you did (attaching the tests to validatePrivate()) was the best approach at the moment since testing just validateSchemaV4() couldn't be done in so straightforward way. I think we would need to refactor of some of the existing code first. I like it the way you did it 👍
• "hopefully followed the Semantic Release" - looks good to me! 👍

Everything looks really great. Thank you!

---

Regarding external file refs, I think it's okay to support it in Gavel, but we really need to test that it will interpret only valid JSON Schema files, not files like /etc/passwd (would disclose sensitive data), /dev/zero (can eat all memory), etc. People run Dredd on their CIs, we need to prevent any vulnerabilities. Would you mind adding tests for this?

• when referencing external file with valid content, Gavel reads the file and correctly validates according to the schema
• when referencing external file with (possibly sensitive) plain text content, Gavel refuses to read the file and doesn't disclose the file's contents
• when referencing huge or infinitely large external file (not sure how to test this on Windows very well), Gavel refuses to read the file and doesn't eat all computer memory or doesn't crash

If you're not sure about it, I can help with the test cases.

[joaosa]

@honzajavorek thank you! Glad to have contributed :)

Thanks for describing the test scenarios. I'll look into it next!

[honzajavorek]

@joaosa I merged the first PR meanwhile. Thanks again! Let me know if you get any far with the additional test cases.

[honzajavorek]

Closing this in favor of #89. Thanks @joaosa! 🎉