Project

General

Profile

Bug #2986

Cookie format specifier is broken

Added by xoneca about 1 month ago. Updated about 1 month ago.

Status:
Patch Pending
Priority:
Normal
Assignee:
-
Category:
mod_accesslog
Target version:
Start date:
2019-10-11
Due date:
% Done:

0%

Estimated time:
Missing in 1.5.x:

Description

The %{name}C format specifier doesn't log cookie value correctly.

Example request with header:

Cookie: test1=1; test2=2; test3=3

Using following configuration:

accesslog.format = "[%{test1}C][%{test2}C][%{test3}C]" 

test1 cookie value gets logged, but test2 and test3 don't:

[1][][]

History

#2

Updated by xoneca about 1 month ago

Sorry, I overlooked the removal of the last line in the patch. I've reworked the patch.

#3

Updated by gstrauss about 1 month ago

Thanks. I'll try to review this patch later this week.

#4

Updated by gstrauss about 1 month ago

This appears to preserve the existing behavior of how whitespace is handled, and also fixes the issue you are seeing:

--- a/src/mod_accesslog.c
+++ b/src/mod_accesslog.c
@@ -1098,7 +1098,7 @@ REQUESTDONE_FUNC(log_access_write) {
                                                        buffer_free(bstr);
                                                        break;
                                                } else {
-                                                       do { ++str; } while (*str != ' ' && *str != '\t' && *str != '\0');
+                                                       do { ++str; } while (*str != ';' && *str != ' ' && *str != '\t' && *str != '\0');
                                                }
                                                while (*str == ' ' || *str == '\t') ++str;
                                        } while (*str++ == ';');

#5

Updated by xoneca about 1 month ago

Sure, fix as you wish.

Your proposed change has a couple edge cases, though: an empty cookie (i.e. two semicolons in a row), skips the next cookie. And if the cookie value starts with semicolon and space, no cookies are read. These are very edge cases, so I can live with them :)

You don't have to be concernet with the witespace behaviour of my patch: whitespace is implicitly skipped as we only care to find the next semicolon. Indeed, could have used strchrnul to find the next semicolon, instead of the while loop.

#6

Updated by xoneca about 1 month ago

I wasn't sure, but it seems empty cookies are allowed by the RFC [1]:

   4.  Serialize the cookie-list into a cookie-string by processing each
       cookie in the cookie-list in order:

       1.  Output the cookie's name, the %x3D ("=") character, and the
           cookie's value.

-->    2.  If there is an unprocessed cookie in the cookie-list, output
-->        the characters %x3B and %x20 ("; ").

[1] https://tools.ietf.org/html/rfc6265#section-5.4

#7

Updated by xoneca about 1 month ago

Patch using strchrnul instead of a while loop. Tested with all discussed edge cases.

#8

Updated by gstrauss about 1 month ago

strchrnul() is a GNU extension and is not as portable as C or POSIX standard library calls.

#9

Updated by gstrauss about 1 month ago

--- a/src/mod_accesslog.c
+++ b/src/mod_accesslog.c
@@ -1098,7 +1098,7 @@ REQUESTDONE_FUNC(log_access_write) {
                                                        buffer_free(bstr);
                                                        break;
                                                } else {
-                                                       do { ++str; } while (*str != ' ' && *str != '\t' && *str != '\0');
+                                                       while (*str != ';' && *str != ' ' && *str != '\t' && *str != '\0') ++str;
                                                }
                                                while (*str == ' ' || *str == '\t') ++str;
                                        } while (*str++ == ';');
#10

Updated by gstrauss about 1 month ago

  • Status changed from New to Patch Pending
  • Target version changed from 1.4.x to 1.4.55

Also available in: Atom