[Seaside] bad request

[Philippe Marschall]

On Sun, Dec 29, 2013 at 8:26 AM, Otto Behrens <otto at finworks.biz> wrote:
>>> This is my best conclusion - please comment:
>>> For someone providing a web application, the server implementer can either
>>> 1) be strict respond with 400 (bad request) if it finds something it
>>> dislikes in the request (either the http method + URL, headers or
>>> body),
>>> or 2) ignore inappropriate parts of the request. Of course, some parts
>>> of the request can't be ignored, so one will have to distinguish
>>> carefully.
>>
>> I generally prefer option 1) because this way when something goes
>> wrong on the server the client gets notified. Otherwise client errors
>> can be really annoying to debug.
>
> Yes, agree. I could not find "invalid cookie" kind of responses when
> searching for how others solve this kind of problem though. Having
> said this, I'd also rather respond with 'bad request' if I find
> anything I don't like in the request.
>
> I suppose we also have to distinguish between unable to parse the
> cookie header and an unexpected key or value in the cookie header. If
> the server receives a key that it does not recognise, should it
> respond with bad request?

No. Among other things it's hard to know in the adapter which cookies
the code later will accept.

>> process: aNativeRequest
>>     ^ [ self basicProcess ]
>>         on: self badRequestExceptionSet
>>         do: [ :e | self badRequestFor: e ]
>>
>> badRequestExceptionSet
>>   ^ GRInvalidUtf8Error, WAInvalidUrlSyntaxError
>>
>> badRequestFor: anError
>>   "default implementation, trigger the servers internal error handler
>>   sublcasses can answer a 'native' bad request (HTTP 400) respone"
>>   anError signal
>
> This means that every subclass of WAServerAdaptor have to implement a
> native bad request response. Is there an elegant (i.e. non monkey
> patched) way for me to customize this response?

Yes

> That's why I thought
> the seaside framework must handle it, because I can customise there.
>
>>> It seems if I want to handle any of these errors (consistently), I
>>> have to modify Comanche, Zinc and FastCGI adaptors.
>>
>> Yes
>
> Ok, would be soo nice if I can do it once in my application though
> (because I do it there already).
>
>>> I think we should allow the Seaside framework to handle these errors
>>> as well. I understand giving the WAResponse the responsibility to
>>> handle it can be tricky, but it may just be the right place.
>>> Currently, it does handle some of the 400 range of errors (eg. not
>>> found).
>>
>> WAResponse does not handle 400. Somebody somewhere creates a 400
>> response just like somebody somewhere creates a 200 response.
>
> Sorry, the request handler may create a WAResponse with code 400.
>
>>> This is my suggestion: Create the WARequestContext on the native
>>> request without parsing it.
>>
>> I'm not liking it. The propose of WARequestContext is that it holds on
>> to (among other things) a WARequest and a WAResponse. We just got an
>> exeption when trying to create a WARequest. This smells like having a
>> partially initialized object that will just cause other exceptions
>> further down the road.
>
> Yes, I share your discomfort. But, the same problem applies to the
> response. When creating the context, the response is empty. Exceptions
> raised during the construction of the response may leave it in a bad
> state if it is not properly handled, not true?

"it" meaning context or response

> It would be better then to parse it in one shot then. And set the
> WARequest to be bad could be another option. The request handler can
> then take care of a bad request.

That sounds better. It also addresses the issue of what happens when
the exception handler access the same header that signals an exception
or a different header that signals an exception as well. I guess you
could do the check in WARequestHandler >> #handle:. Would you extend
WAErrorHandler to have a #handleBadRequest: method or where would you
eventually handle it? Would be argument be the original exception, the
native request or the bad WARequest?

>>> Parsing the URL effectively comes down to handleFiltered: on
>>> WADispatcher that will try to get the url via the consumer on the
>>> request. If parsing it breaks, the request is marked as a bad request.
>>> The dispatcher could pass to the default handler. The parsing errors
>>> of the other parts of the request would then go to the handler
>>> dispatched to. And that could in turn be dispatched to an error
>>> handler.
>>
>> No, the URL is parsed in WAServerAdaptor >> #requestUrlFor:
>
> I meant that if we delay the parsing, #requestUrlFor: would be sent at
> a later stage.

Cheers
Philippe