JQAjax>>#callback:json: and invalid json

[adriaon]

Pentesters had fun with JQAjax>>#callback:json: while feeding it invalid json. Shouldn't it catch the json parsing error and respond with a bad request response or provide a hook for this or something?

[adriaon]

Perhaps like this?

self
    callback: [:value | | json |
        json := [WAJsonParser parse: value] on: WAJsonSyntaxError do: [:ex | ex return: nil].
        json isNil ifTrue: [self requestContext respond: [:resp | resp badRequest]].
        aBlock value: json]
    value: (JSJson new stringify: anObject)

[adriaon]

Hmmm.. , 'null' is valid json and parses to nil. Perhaps this is better:

    self
        callback: [:value | | json |
            json :=
                [WAJsonParser parse: value]
                    on: WAJsonSyntaxError
                    do: [:ex | self requestContext respond: [:resp | resp badRequest]].
            aBlock value: json]
        value: (JSJson new stringify: anObject)

[jbrichau]

Hi @adriaon

The error should be caught by the Seaside exception handler, which will return an error 500. There should not be a need for separate exception handlers inside (ajax) callbacks.

[jbrichau]

Since the urls for a callback:json: callback are not intended to be part of a public api, the response to an invalid json can very well be an internal error (http status 500).

Though, if one wants to return a bad request (status 400), it should be possible to catch the WAJsonSyntaxError in the configured exception handler and return such a response.

As such, I don't think we should alter the behavior of the callback. Similar situations can arise in all callbacks that accept values sent from the client. If they are tampered with and an error results, it should be considered normal.

Perhaps there are other reasons but as such, I don't think we should alter the callback implementation.

[adriaon]

Hi @jbrichau ,

Thanks for your replies. Some considerations and thoughts from my end.

We see errors 500 as indications of 'bugs' or 'shortcomings' in our server. They are monitored and they are bad for our reputation.

JQAjax>>#callback:json: is the only place in Seaside where WAJsonParser is used (in my image), besides REST APIs.

JQAjax>>#callback:json: might be the only place where programmers can't do any sanity checks before the input gets processed, even-though it is 'just' parsing.

Dealing with WAJsonSyntaxError in the configured exception handler broadens the scope (enormously), which I think is undesirable. To compensate this, the configured error handler needs to be even more complex.

Those points made me think JQAjax>>#callback:json: is the best place to deal with it.

Cheers, Adriaan.

[jbrichau]

Hey @adriaon,

Well, in my opinion, when a callback:json: gets passed illegal json, then this is actually a bug. The trouble is that humans tampering with the code can trigger the error as well. So, I understand your point of view but it does mean you will need to log these errors in some other way when they occur.

I can imagine adding a new method with an optional error handler block (e.g. callback:json:onInvalidJson:) to allow handling a json parse error differently, but I would not want to change the existing behaviour of the callback and start missing programming error in the server log.

[adriaon]

Hi @jbrichau,

Fair enough. I make my own method. Probably named #callback:untrustedJson:. Because with #callback:json:onInvalidJson: all senders need to supply the same error handling block.

Cheers. Adriaan.

[jbrichau]

Hey @adriaon

I'm not trying to reject adding this possibility to Seaside. I am, however, making the point that we should make this change optional and preserve existing behavior. We can add callback:json:catchJsonParseError: with the last argument being a Boolean and make the original method delegate to this new method with false as last argument.

[adriaon]

Hi @jbrichau,

No worries. Didn't want to come across harsh. We are just pair-programming :-). Preserving existing behaviour is perfectly fine. Adding #callback:json:catchJsonParseError: is also a possibility. But the one with a block is more flexible. I thought #callback:json:onInvalidJson: is ok and then a shorthand method like #callback:untrustedJson: that calls #callback:json:onInvalidJson: with a default block. For me, either solution is fine.