added json schema validation to all incoming non-binary messages #71
Conversation
|
Overall looks good to me! Just to confirm, we're not forcing 'value' to conform to any type? My only other comment was to add a 'catch' for JSON.parse so we can log that error. |
|
I see it in the fold! Merge it! |
|
for the "value" parameter, I don't provide any specifics, so it should match anything (number, string, boolean, object...) I have only tested against a couple clients (boolean, string) and hand-crafted bad config messages. I guess until we get a testing harness integrated (WIP on https://github.com/quinkennedy/spacebrew/tree/test-driven) going live is our best test 😨 |
|
|
||
| //setup validator | ||
| var ajv = AJV(); | ||
| var validate = ajv.compile(JSON.parse(fs.readFileSync('schema.json'))); |
There was a problem hiding this comment.
hmmm, should these be inside spacebrew.createServer, so they are scoped instead of global? That probably isn't an issue with Node.js though right?
There was a problem hiding this comment.
Maybe. In working on the electron version, I'm realizing we need to have some better mgmt of things outside scripts might want to manage, e.g. having some way of closing/editing server after open. With this, it's only an issue if there's something weird about a) when the file is loaded and b) where it's loaded from. Should it be __dirname + 'shema.json'??
|
PR looks good to me with the new fix. We could: I say we go wild and do a), but up to you |
added json schema validation to all incoming non-binary messages
|
Logging into sandbox right now to test. |
addresses #70 and probably many other latent errors that may arise from malformed JSON.
The schema definition is somewhat relaxed since it does not specify
"additionalProperties":falseanywhere, which would further restrict what comes in. I focused on specifying required and common properties.I don't think I'm missing anything, so I am open to adding
"additionalProperties":falseto everything except forvalue(in messages) and possiblyoptionsobjects (in client config).