orthecreedence / wookie

Asynchronous HTTP server in common lisp
http://wookie.lyonbros.com/
MIT License
189 stars 19 forks source link

Potential issue with cookies (set-cookie) - The value NIL is not of type HASH-TABLE. #84

Closed JordanPowell closed 4 years ago

JordanPowell commented 6 years ago

I seem to be getting this error when calling set-cookie. I've looked through the code but I couldn't find anything that explicitly sets up response-headers as a hash table. Am I missing something?

I may have done something stupid so I'm sorry if that's the case. This code reproduces it for me:

(defun start()
  ;; load Wookie's core plugins
  (load-plugins)

  (defroute (:get "/") (req res)
    (wookie-plugin-export:set-cookie res "test" "test")
    (send-response res :body "Welcome"))

  ;; start serving requests!
  (as:with-event-loop ()
    (start-server (make-instance 'listener :port 5000))))

(start)

then

curl -v localhost:5000/

gives

Error running hooks: PRE-ROUTE: (socket #<TCP-SOCKET {10114DA933}>): The value NIL is not of type HASH-TABLE.

Adding a (setf (response-headers res) (make-hash-table)) before set-cookie seems to cheer it up without any issues but I'm pretty sure that's not what I'm supposed to do.

Any help is appreciated.

orthecreedence commented 6 years ago

I think I know what's going on. A LOOONG time ago, someone did a PR to switch the headers from a plist to a hash table because of some kind of memory issue...I think the cookie plugin may have gotten left behind in this change, although it's odd that it wouldn't use getf.

Either way, seems like a bug in either wookie or the cookie plugin.

Try this as a workaround:

(add-hook :pre-route
          (lambda (req res)
            (declare (ignore req))
            (unless (hash-table-p (response-headers res))
              (setf (response-headers res) (make-hash-table :test $'equal)))))
JordanPowell commented 6 years ago

Thanks for the quick response!

Your workaround works nicely, thanks. I found the PR you mentioned ( https://github.com/orthecreedence/wookie/pull/65/files) but I couldn't find any discussion about memory issues.

Do you know which of hash table or plist is the 'correct' data type for the response-headers? The wookie code converts the hash table to a plist in https://github.com/orthecreedence/wookie/blob/master/request-response.lisp#L148 so it looks like it prefers plists, but if there's a memory issue with using plists perhaps the whole thing should be converted to use hash tables.

If there's a clear direction forward would you be interested in pull requests along these lines? If not I'm happy to keep using the pre-route hook workaround.

orthecreedence commented 6 years ago

From what I remember (this is a looong time ago) we moved from plists to hashes. However, I seem to remember that being a vulnerability from request headers (meaning, a client sending a 2mb header could crash the runtime or something) but response headers were ambiguous. I think overall it makes sense to use hash tables, probably, just to keep things consistent.

I'd be interested in PRs! I'm not actively doing much with Wookie anymore, but certainly welcome improvements from others.