Issue with requests with invalid HTTP method

[kennymalac]

This was horrendous to debug!
I managed to destroy wookie's HTTP parsing: in my js, I had a for loop of XHR requests over and over that send error responses when trying to access a session that is either 1) nonexistent or 2) unavailable (locked)
At some point in this, the browser sends over an invalid HTTP request (i'm not entirely sure about this, it could be something else entirely) and wookie incorrectly handles the fast-http invalid-method condition. It's relevant that I'm using with-chunks :t
My traceback is:

  1: ((FLET "H0" :IN WOOKIE::READ-DATA) #<FAST-HTTP.ERROR:INVALID-METHOD {10050F4283}>)
  2: (SB-KERNEL::%SIGNAL #<FAST-HTTP.ERROR:INVALID-METHOD {10050F4283}>)
  3: (ERROR FAST-HTTP.ERROR:INVALID-METHOD)
  4: (FAST-HTTP.PARSER:PARSE-REQUEST #<unavailable argument> #<unavailable argument> #<unavailable argument> :START #<unavailable argument> :END #<unavailable argument>)
  5: (SB-DEBUG::TRACE-CALL #<SB-DEBUG::TRACE-INFO FAST-HTTP.PARSER:PARSE-REQUEST> #<FUNCTION FAST-HTTP.PARSER:PARSE-REQUEST> #S(FAST-HTTP.HTTP:HTTP-REQUEST :METHOD NIL :MAJOR-VERSION 0 :MINOR-VERSION 9 :ST..
  6: ((LAMBDA (FAST-HTTP::DATA &KEY :START :END) :IN FAST-HTTP:MAKE-PARSER) #(8 83 108 77 55 238 ...) :START #<unavailable argument> :END #<unavailable argument>)
  7: (WOOKIE::READ-DATA #<CL-ASYNC:TCP-SOCKET {100554DB33}> #(8 83 108 77 55 238 ...))
  8: (CL-ASYNC-UTIL:CALL-WITH-CALLBACK-RESTARTS #<CLOSURE (LAMBDA NIL :IN CL-ASYNC::STREAMISH-READ-CB) {1004BBE07B}> :CONTINUE-FN #<CLOSURE (LAMBDA NIL :IN CL-ASYNC::STREAMISH-READ-CB) {1004BBE0DB}> :ABORT..
  9: (CL-ASYNC::STREAMISH-READ-CB #.(SB-SYS:INT-SAP #X7FFFE80536B0) 26774 #<unused argument>)
 10: ((LAMBDA (SB-ALIEN::ARGS-POINTER SB-ALIEN::RESULT-POINTER FUNCTION) :IN "/home/ken/quicklisp/dists/quicklisp/software/cl-async-20160825-git/src/dns.lisp") #<unavailable argument> #<unavailable argumen..
 11: ("foreign function: funcall_alien_callback")
 12: ("foreign function: callback_wrapper_trampoline")
 13: ("foreign function: #x2010059C")
 14: (CL-ASYNC:START-EVENT-LOOP #<FUNCTION (LAMBDA NIL :IN SLAVE:RUN-DEV-SERVER) {22B1BF6B}> :CATCH-APP-ERRORS NIL :SEND-ERRORS-TO-EVENTCB T)
 15: (SLAVE:RUN-DEV-SERVER)
 16: (SB-INT:SIMPLE-EVAL-IN-LEXENV (SLAVE:RUN-DEV-SERVER) #<NULL-LEXENV>)

The applicable code:
https://github.com/kennymalac/simple-content-host/blob/master/src/program/slave.lisp#L71

I might just use hutchentoot. I have tried debugging this for a few hours now. It sucks because I really like the with-chunking macro that wookie has, but this error is really bad. It's hard to reproduce as well, I'm just trying to send an error response JSON if I don't want to chunk the response request. At first I thought, maybe with-chunking HAS to be called even if I want to reject the request? But some of the errors get returned, it's only when stress testing it, that it breaks entirely it seems. This is the request that wookie is trying to pass into http-parse:

#<FAST-HTTP.HTTP:HTTP-REQUEST {1004B3FEC3}>
--------------------
The object is a STRUCTURE-OBJECT of type FAST-HTTP.HTTP:HTTP-REQUEST.
METHOD: NIL
MAJOR-VERSION: 0
MINOR-VERSION: 9
STATUS: 0
CONTENT-LENGTH: NIL
CHUNKED-P: NIL
UPGRADE-P: NIL
HEADERS: NIL
HEADER-READ: 0
MARK: 0
STATE: 0
RESOURCE: NIL

Here's the relevant method where it ends up trying to use a blank request object:

wookie/parser.lisp

Line 282 in 288d108

(defun read-data (sock data)

Here's the route expanded:

(LET ((#:G975
       (WOOKIE::MAKE-ROUTE :POST "/upload-chunked/SESSION/([0-9]+)"
                           (LAMBDA (REQ RES &REST ARGS)
                             ""
                             (DECLARE (IGNORABLE REQ))
                             (SETF (REQUEST-STORE-BODY REQ) NIL)
                             (LET* ((SESSION-ID
                                     (CONCATENATE 'STRING "SESSION-"
                                                  (CAR ARGS)))
                                    (SESSION
                                     (GETHASH (INTERN SESSION-ID)
                                              *UPLOAD-SESSIONS*)))
                               (IF (AND SESSION (NOT (IN-PROGRESS SESSION)))
                                   (PROGN
                                    (SETF (IN-PROGRESS SESSION) T)
                                    (LET ((BUCKET
                                           (GET-BUCKET SESSION
                                                       (PTR
                                                        (INFO
                                                         (CONTENT-FILE
                                                          SESSION))))))
                                      (UPLOAD-CONTENT BUCKET SESSION
                                                      (LAMBDA
                                                          (HANDLE
                                                           FINISH-UPLOAD)
                                                        (FORMAT T
                                                                "handling upload...")
                                                        (WITH-CHUNKING REQ
                                                            (CHUNK LAST-CHUNK-P)
                                                          (WRITE-SEQUENCE CHUNK
                                                                          HANDLE)
                                                          (FORCE-OUTPUT HANDLE)
                                                          (WHEN LAST-CHUNK-P
                                                            (FORMAT T
                                                                    "completed chunked upload, finishing...")
                                                            (SEND-JSON-RESPONSE
                                                             RES :BODY
                                                             (FUNCALL
                                                              FINISH-UPLOAD
                                                              HANDLE))
                                                            (DELETE-PTR
                                                             SESSION)
                                                            (REMHASH
                                                             (INTERN
                                                              SESSION-ID)
                                                             *UPLOAD-SESSIONS*)))))))
                                   (PROGN
                                    (SEND-JSON-RESPONSE RES :BODY
                                                        (PLIST-HASH-TABLE
                                                         '("error" "no-session"
                                                           "message"
                                                           "Upload session expired or does not exist"))
                                                        :HEADERS
                                                        '(:CONTENT-TYPE
                                                          "text/json")
                                                        :OTHER
                                                        (:STATUS 400))))))
                           :REGEX T :CASE-SENSITIVE T :ALLOW-CHUNKING T
                           :BUFFER-BODY T :SUPPRESS-100 T :VHOST
                           WOOKIE::*DEFAULT-VHOST* :PRIORITY 0)))
  (IF T
      (WOOKIE::UPSERT-ROUTE #:G975)
      (WOOKIE::ADD-ROUTE #:G975)))

[kennymalac]

On second thought, I'm going to try putting the session check in :pre-route like I should have and see if that fixes it.

[kennymalac]

OK turns out that by moving the check to pre-route, it werrrkzz, and I'm glad that I can keep using wookie!
So it looks like any route with :chunk t cannot send a response up until all the data has been read. I think it's a good idea to add information to the documentation that specifies it would be very dangerous to do so, so that no one suffers the same embarrassing mistake that I made.

issue still exists even as pre-route

[kennymalac]

I managed to fix this by putting :close on my (send-response) error response.

[orthecreedence]

Hey, thanks for reporting this in such detail, and sorry for the lag in response. I was out of town.

If wookie is handling the condition incorrectly, then maybe the issue should remain open, although is it possible this is a fast-http bug?

[kennymalac]

@orthecreedence I don't think this is a fast-http bug because wookie ends up sending a blank request object (i.e. the initial request object it recreates) as a response. The issue is that the chunking is triggered when the request handlers are being parsed which happens even before the pre-route. This is problematic because that would mean chunking is started before the server can even make sure the user has permission to upload.