Wapp

Fix cookies parsing
Login

Fix cookies parsing

(1) By Oleg (oleg4tcltk) on 2020-06-04 15:47:04 [link] [source]

Hi.

There is a bug in cookies parsing. If cookie value has '=' char(e.g. base64 value), then it corrupted on parsing. This patch makes cookies parsing more rfc6265 compliant(but be more liberal to cookie value, allowing any chars except dquote in a quoted value and any char except semicolon in a nonquoted value).

--- wapp.tcl    2020-06-02 17:57:49.379477220 +0300
+++ wapp.new.tcl    2020-06-04 18:29:34.071369424 +0300
@@ -18,6 +18,30 @@
 #
 package require Tcl 8.6
 
+proc cookies_parse {cstr} {
+  set c [dict create]
+
+  while {$cstr ne ""} {
+    set idxs [regexp -indices -inline {^\s*([^\s=]+)\s*=\s*(?:"([^"]*)"|([^";][^;]*)|)(?:\s*;\s*)?} $cstr]
+    if {[llength $idxs] == 0} {
+      puts stderr "cookie parse error for: '$cstr'"
+      return $c
+    }
+    set name [string range $cstr [lindex $idxs 1 0] [lindex $idxs 1 1]]
+    if {[lindex $idxs 2 0] >= 0} {
+      set val [string range $cstr [lindex $idxs 2 0] [lindex $idxs 2 1]]
+    } elseif {[lindex $idxs 3 0] >= 0} {
+      set val [string range $cstr [lindex $idxs 3 0] [lindex $idxs 3 1]]
+      set val [string trimright $val]
+    } else {
+      set val ""
+    }
+    dict set c $name $val
+    set cstr [string range $cstr [lindex $idxs 0 1]+1 end]
+  }
+  return $c
+}
+
 # Add text to the end of the HTTP reply.  No interpretation or transformation
 # of the text is performs.  The argument should be enclosed within {...}
 #
@@ -691,13 +715,12 @@
   # POST data
   #
   if {[dict exists $wapp HTTP_COOKIE]} {
-    foreach qterm [split [dict get $wapp HTTP_COOKIE] {;}] {
-      set qsplit [split [string trim $qterm] =]
-      set nm [lindex $qsplit 0]
-      if {[regexp {^[a-z][-a-z0-9_]*$} $nm]} {
-        dict set wapp $nm [wappInt-decode-url [lindex $qsplit 1]]
+    set cparsed [cookies_parse [dict get $wapp HTTP_COOKIE]]
+    dict for {cname cval} $cparsed {
+      if {[regexp {^[a-z][-a-z0-9_]*$} $cname]} {
+        dict set wapp $cname $cval
       }
     }
   }

(2) By D. Richard Hipp (drh) on 2020-06-04 16:13:56 [link] [source] in reply to 1

Thanks for the patch. But is this something that really needs "fixing". Perhaps Wapp does not accept the full range of allowed cookie values. But the application should be under full control of what cookies it sets. In fact, iirc, doesn't Wapp automatically encode cookies when you set them so that they can be properly decoded by the existing decoder?

In other words, what problem are you attempting to solve here?

(3) By Oleg (oleg4tcltk) on 2020-06-05 07:21:43 and edited on 2020-06-05 07:22:32 [history] [link] [source] in reply to 2

what problem are you attempting to solve here?

I use base64 value for a cookie and it don't work properly. On parsing, name-value pair is splited by '=' and values like "c29tZSB2YWx1ZQ==" are corrupted. So, i was resolving this problem and along the day i tried to do the parsing of cookies more rfc6265 compliant.

Now i think that cookie_parse proc should better be named wappInt-cookie-parse to avoid name clash due to 'source' command.

Earlier this patch was small:

--- wapp.tcl    2020-06-02 17:57:49.379477220 +0300
+++ wapp.new.tcl    2020-06-02 18:05:34.195467060 +0300
@@ -692,10 +692,9 @@
   #
   if {[dict exists $wapp HTTP_COOKIE]} {
     foreach qterm [split [dict get $wapp HTTP_COOKIE] {;}] {
-      set qsplit [split [string trim $qterm] =]
-      set nm [lindex $qsplit 0]
+      regexp {^([^=]+)=(.+)$} [string trim $qterm] _ nm val
       if {[regexp {^[a-z][-a-z0-9_]*$} $nm]} {
-        dict set wapp $nm [wappInt-decode-url [lindex $qsplit 1]]
+        dict set wapp $nm [wappInt-decode-url $val]
       }
     }
   }
But it didn't properly handle quoted cookie value and etc. So i remade it.

(5) By Oleg (oleg4tcltk) on 2020-06-05 08:58:17 [link] [source] in reply to 2

doesn't Wapp automatically encode cookies when you set them so that they can be properly decoded by the existing decoder?

My app set cookie on client side with help of js. This is auth token. And cookie just a one way beside others to deliver it to a server side. So to not have problems(other transports for auth token have restrictions on chars too) i choose the more strict form of data encoding(from my point of view) for this - base64.

imho, we shouldn't do encoding/decoding of cookie value by ourselves, because this doesn't covered by the standard and can cause problems when interacting with external world. Better, we can give wapp-encode-url/wapp-decode-url to a user and he explicitly do wapp-set-cookie atoken [wapp-encode-url $TOKEN] and wapp-decode-url [wapp-param atoken] or wapp-set-cookie atoken [base64::encode $TOKEN] and etc. On cookie set, we can check it value:

  • if it has wrong chars(from rfc:
    cookie-octet      = %x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E
                       ; US-ASCII characters excluding CTLs,
                       ; whitespace DQUOTE, comma, semicolon,
                       ; and backslash
    )
    
    then error "cookie value contains bad char: try to encode it"

(6) By Oleg (oleg4tcltk) on 2020-06-05 11:46:46 [source] in reply to 5

On cookie set, we can check it value

Something like this:

diff --git a/wapp.tcl b/wapp.tcl
index 2bdae72..f876aa6 100644
--- a/wapp.tcl
+++ b/wapp.tcl
@@ -240,6 +240,9 @@ proc wapp-reply-code {x} {
 #
 proc wapp-set-cookie {name value} {
   global wapp
+  if {![regexp {^[]!#$%&'()*+./:0-9<=>?@A-Z[^_`a-z{|}~-]+$} $value]} {
+    error "Bad cookie value: '$value'"
+  }
   dict lappend wapp .new-cookies $name $value
 }
 
@@ -808,7 +811,7 @@ proc wappInt-handle-request-unsafe {chan} {
         if {$val==""} {
           puts $chan "Set-Cookie: $nm=; HttpOnly; Path=/; Max-Age=1\r"
         } else {
-          set val [wappInt-enc-url $val]
+          set val $val
           puts $chan "Set-Cookie: $nm=$val; HttpOnly; Path=/\r"
         }
       }