657820 - Submitted marks should be checked for validity before dropping them into the database

Status: VERIFIED FIXED

(Websites Graveyard :: markup.mozilla.org, defect, P1)

Description

[Fred Wenzel [:wenzel]]

Right now, submitted marks are not checked for validity at all: I could upload a picture of my dog and the app would take it and drop it into the DB.

Since the marks are JSON, it should do at the very least:
- a check if this is actually JSON that can be parsed
- a check if the structure of the JSON is roughly what you'd expect:

{"rtl":false,"strokes":[[{"x":538,"y":322,"z":0,"time":1355,"speed":0.014142135623730952,"angle":5.497787143782138,"significance":1},{"x":539,"y":348,"z":0,"time":1389,"speed":0.028284271247461905,"angle":5.497787143782138,"significance":1},
...
]]}

Comment 1

[[:brez]]

Added ability to confirm that it is in fact JSON by attempting to load it into simplejson, an invalid string will throw an exception which is caught and set the fail message, etc.  Re. the structure,  the JSON is parsed in commons.py which would err out if its not what we're expecting..  didn't think it would be necessary to duplicate this - if not let me know and reopen:

https://github.com/mozilla/markup/commit/eba161a8973d88302fde7e1d8de827eb63234026

Comment 2

[Fred Wenzel [:wenzel]]

That code doesn't make a lot of sense to me:
- You are creating some JSON from a dict ("dumps") and make sure it is then, in fact, JSON ("loads"). Now you know the Python standard library works. You're not testing if the points_obj that the user sent you is in fact pareseable JSON.
- You are also not testing (not even spot-testing) if the JSON that the user sent you is in fact what roughly looks like a GML mark. Even if it's parseable, it could be random, JSON-encoded information.
- I don't see the structure of the mark being parsed anywhere in commons.py (unless I am overlooking it, in which case please point out the line to me, thanks). I just see some regex magic to remove whitespace.

Comment 3

[[:brez]]

Sure will update - I honestly don't see this as a 'blocker' though - there's no concept of schema validation in JSON, sure we can do some sanity checks (and we will) but there will always be some edge case where you can get that picture of you dog into the database.

Comment 4

[[:brez]]

Re. making some sense of the code - python lists translate to JSON arrays so we're can't easily parse JSON from the javascript frontend, i.e. why I have to use dict - if you have a better approach, please let me know.

Comment 5

[[:brez]]

Just documentation on what is being done:

1. Make sure its JSON
2. Make sure stroke object exists
3. Make sure a random sample is correctly formatted
4. Meets some size expectations 30k for simple 150k for raw (you can probably just test the overall size of the request isn't over 180k)

Comment 6

[[:brez]]

Disregard this - figure out a way to ensure JSON is in fact JSON

(In reply to comment #4)
> Re. making some sense of the code - python lists translate to JSON arrays so
> we're can't easily parse JSON from the javascript frontend, i.e. why I have
> to use dict - if you have a better approach, please let me know.

Comment 7

[[:brez]]

Added, commits:

https://github.com/mozilla/markup/commit/8493c5d11bbdea2b9f7875afa91713f9fd0d3b38
https://github.com/mozilla/markup/commit/439dc958a5f53414b7f11536dd36cdcc60561c7d

Comment 8

[Fred Wenzel [:wenzel]]

Do you really want that size check to be an "and"? I think the raw data being > 150k *or* the simplified > 30k should trigger an error.

Also, please test this by making a sufficiently complex mark to make sure that these limits are actually useful. Thanks!

Comment 9

[[:brez]]

Yea good point - guess I was looking at the requirement too literally - tested with a super super complex drawing at it was 40k (simplified) i.e. are those values too low?

Comment 10

[[:brez]]

[master 356f09e] used OR for length conditional bumped max values up
 1 files changed, 1 insertions(+), 1 deletions(-)


https://github.com/mozilla/markup/commit/356f09ee8bc753764cebab623abf09d253dad7d1

Comment 11

[Fred Wenzel [:wenzel]]

Yes, judging by your test, I think the values are good now. Thanks!

Comment 12

[Matt Brandt [:mbrandt]]

QA verified per comment 11 and submitting blatantly non-json is rejected.