Bug #1628

Lighttpd glues response headers together. Setting more than one cookie impossible.

Added by Anonymous over 6 years ago. Updated over 4 years ago.

Status:FixedStart date:
Priority:HighDue date:
Assignee:-% Done:

100%

Category:core
Target version:-
Missing in 1.5.x:No

Description

Lighttpd 1.19 seems to glue response headers with the same header name together.
This breaks all scripts that try to set more than one cookie at the same time.

PHP script to reproduce:

setcookie('cookie1', 'value1');
setcookie('cookie2', 'value2');

Intended behavior:

$ php cookietest.php
X-Powered-By: PHP/5.2.5
Set-Cookie: cookie1=value1
Set-Cookie: cookie2=value2
Content-type: text/html

Headers sent by Lighty:

HTTP/1.x 200 OK
X-Powered-By: PHP/5.2.5
Set-Cookie: cookie1=value1, cookie2=value2
Content-Type: text/html
Content-Length: 0
Date: Fri, 04 Apr 2008 17:31:17 GMT
Server: lighttpd/1.4.19

-- mail

Associated revisions

Revision 2344
Added by stbuehler about 6 years ago

Fix ajp13 response header handling (fixes #1628)

History

#1 Updated by jwmcglynn over 6 years ago

Combining multiple Set-Cookie headers into one seems to be standard practice, and the syntax used conforms to the Set-Cookie RFC: http://www.ietf.org/rfc/rfc2109.txt (section 4.2.1 and 4.2.2).

Perhaps your user agent is handling the response incorrectly?

#2 Updated by stbuehler over 6 years ago

  • Status changed from New to Fixed
  • Resolution set to invalid

#3 Updated by Anonymous over 6 years ago

  • Status changed from Fixed to Need Feedback
  • Resolution deleted (invalid)

We are experiencing problems with both Firefox and Internet Explorer.
In previous version of Lighty our websites worked fine.

Be aware that joining Set-Cookie headers with a comma is not allowed under the old Netscape specification: http://wp.netscape.com/newsref/std/cookie_spec.html
Multiple "Set-Cookie" headers should be issued.

If you specify an expiry date using PHP's setcookie() method, it produce an old-style Set-Cookie header with an "Expires" field according to the Netscape specification.
Not a new-style Set-Cookie header with "Max-Age" as described in the RFC you quote.

Perhaps this is what confuses browsers?

#4 Updated by Anonymous over 6 years ago

Better example to reproduce:

setcookie('cookie1', 'value1', time()+86400);
setcookie('cookie2', 'value2', time()+86400);

Standard PHP output:

$ php cookietest.php
X-Powered-By: PHP/5.2.5
Set-Cookie: cookie1=value1; expires=Sun, 06-Apr-2008 04:29:53 GMT
Set-Cookie: cookie2=value2; expires=Sun, 06-Apr-2008 04:29:53 GMT
Content-type: text/html

With lighttpd:

HTTP/1.x 200 OK
X-Powered-By: PHP/5.2.5
Set-Cookie: cookie1=value1; expires=Sun, 06-Apr-2008 04:31:04 GMT, cookie2=value2; expires=Sun, 06-Apr-2008 04:31:04 GMT
Content-Type: text/html
Content-Length: 0
Date: Sat, 05 Apr 2008 04:31:04 GMT
Server: lighttpd/1.4.19

As you can see lightly glues "cookie2" to the old-style "expires" field.
Both Firefox and Internet Explorer parse this incorrectly and only remember the first cookie.

-- mail

#5 Updated by stbuehler over 6 years ago

  • Status changed from Need Feedback to Fixed
  • Resolution set to invalid

HTTP/1.1 - http://tools.ietf.org/html/rfc2616#section-4.2

Multiple message-header fields with the same field-name MAY be
present in a message if and only if the entire field-value for that
header field is defined as a comma-separated list #(values).
It MUST be possible to combine the multiple header fields into one
"field-name: field-value" pair, without changing the semantics of the
message, by appending each subsequent field-value to the first, each
separated by a comma. The order in which header fields with the same
field-name are received is therefore significant to the
interpretation of the combined field value, and thus a proxy MUST NOT
change the order of these field values when a message is forwarded.

HTTP/1.0 - http://tools.ietf.org/html/rfc1945#page-22

Multiple HTTP-header fields with the same field-name may be present
in a message if and only if the entire field-value for that header
field is defined as a comma-separated list #(values). It must
be possible to combine the multiple header fields into one "field-
name: field-value" pair, without changing the semantics of the
message, by appending each subsequent field-value to the first, each
separated by a comma.

I think the HTTP specs are pretty clear, and i couldn't find something specific about multiple Set-Cookie headers in http://tools.ietf.org/html/rfc1945#page-22, only

Multiple Set-Cookie headers can be issued in a single server response.

And that does not forbid combining multiple headers, which would violate the http-specs anyway.

That this makes it difficult for browsers to parse the Set-Cookie header is not lighty's fault; perhaps you should ask the php developers to use max-age instead.

Ah, and btw: I tried cgi, fastcgi and proxy and my Set-Cookie headers were not merged, so even if this behaviour is wrong i don't think it is lighty's fault.

#6 Updated by Anonymous over 6 years ago

  • Status changed from Fixed to Need Feedback
  • Resolution deleted (invalid)

Ah, and btw: I tried cgi, fastcgi and proxy and my Set-Cookie headers were not
merged, so even if this behaviour is wrong i don't think it is lighty's fault.

I experimented a bit. When running Lighty on a seperate test server without any load, it indeed performs correctly.

However on our production webserver running an Alexa top-5000 website, it starts joining headers after running for a few minutes.
I suspect this is caused by the way Lighty recycles response headers in mod_fastcgi.c:


/* don't forward Status: */
if (0 != strncasecmp(key, "Status", key_len)) {
    if (NULL == (ds = (data_string *)array_get_unused_element(con->response.headers, TYPE_STRING))) {
    ds = data_response_init();
    }
    buffer_copy_string_len(ds->key, key, key_len);
    buffer_copy_string(ds->value, value);

    array_insert_unique(con->response.headers, (data_unset *)ds);
}

If array_get_unused_element() returns an element that was previously initialized by using data_string_init() instead of data_response_init() it joins the headers.
One place data_string_init() is used for setting a header is in the X-LIGHTTPD-send-file code, a feature used heavily on the website:


    if (host->allow_xsendfile &&
        (NULL != (ds = (data_string *) array_get_element(con->response.headers, "X-LIGHTTPD-send-file"))
          || NULL != (ds = (data_string *) array_get_element(con->response.headers, "X-Sendfile")))) {
        stat_cache_entry *sce;

        if (HANDLER_ERROR != stat_cache_get_entry(srv, con, ds->value, &sce)) {
            data_string *dcls = data_string_init();
            /* found */
            http_chunk_append_file(srv, con, ds->value, 0, sce->st.st_size);
            hctx->send_content_body = 0; /* ignore the content */
            joblist_append(srv, con);

            buffer_copy_string_len(dcls->key, "Content-Length", sizeof("Content-Length")-1);
            buffer_copy_long(dcls->value, sce->st.st_size);
            dcls = (data_string*) array_replace(con->response.headers, (data_unset *)dcls);
            if (dcls) dcls->free((data_unset*)dcls);

I replaced data_string_init() with data_response_init() and now it seems to work correctly so far.
I will wait a couple days to see if it keeps working, before calling it a solution.

-- mail

#7 Updated by stbuehler over 6 years ago

  • Status changed from Need Feedback to Fixed
  • Resolution set to fixed

Good catch!

I searched for data_string_init in the source, and all other calls should be ok.

Fixed in r2143. The original "problem" should be solved too, so we don't have to discuss whether to fix it or not^^

#8 Updated by Anonymous over 6 years ago

  • Status changed from Fixed to Need Feedback
  • Resolution deleted (fixed)

Folks, it seems that this problem also exists in 1.5 version.

Example of the raw headers:

Set-Cookie: JSESSIONID=6E11B57AEFE49DCF47004981994B526D; Path=/alp-portal

Set-Cookie: REMEMBER_USERNAME_VIEWER=; Expires=Thu, 01-Jan-1970 00:00:10 GMT; Path=/

And this is after Lighty combines two headers into one:

Set-Cookie: JSESSIONID=981CA5CCF2EC218BFF7CB148B97A0405; Path=/alp-portal, REMEMBER_USERNAME_VIEWER=; Expires=Thu, 01-Jan-1970 00:00:10 GMT; Path=/

I've verified that the second one doesn't work in FF2/FF3 nor in IE6.

-- dashin

#9 Updated by stbuehler about 6 years ago

  • Status changed from Need Feedback to Fixed
  • % Done changed from 0 to 100

Applied in changeset r2344.

#10 Updated by almost 5 years ago

  • Status changed from Fixed to Reopened
  • Assignee changed from jan to stbuehler
  • Target version changed from 1.5.0 to 1.4.x
  • % Done changed from 100 to 0

Problem with "two set-cookies" was not solved in version 1.4.x.

#11 Updated by stbuehler almost 5 years ago

  • Status changed from Reopened to Fixed
  • Assignee deleted (stbuehler)
  • % Done changed from 0 to 100
  • Missing in 1.5.x set to No

Wrong.

#12 Updated by stbuehler over 4 years ago

  • Target version deleted (1.4.x)

#13 Updated by boris over 4 years ago

Well, this bug hit me too, and it took me 2 days to analyse it to exact this point -- just to be able to use the right key words in the search bar to find this ticket :)

I have a suggestion:

data_response is a loose "subtype" of data_string, they only differ in having their insert_dup property linked to static data_response_insert_dup instead of static data_string_insert_dup. Moreover, array_get_unused_element can't validate for this subtype. I think it is very likely that someone might put the wrong kind of data_string into the response header list again in some place of the code, and then headers would be merged bad again.

What's more: Apart from the above discussion, the lighttpd documentation explicitly states that lighttpd does not merge response headers by ", " regardless of what the spec says (see Changelog, 30.05.2004 14:13 - lighttpd 1.2.1). As we have seen, this is for a reason, and I think it's the only reason why the data_response subtype was introduced in the first place.

So, here is my suggestion: let's change response_header_insert to:

int response_header_insert(server *srv, connection *con, const char *key, size_t keylen, const char *value, size_t vallen) {
        data_string *ds;

        UNUSED(srv);

        if (NULL == (ds = (data_string *)array_get_unused_element(con->response.headers, TYPE_STRING))) {
                ds = data_response_init();
+       } else {
+               ds->insert_dup = data_response_insert_dup;
        }
        buffer_copy_string_len(ds->key, key, keylen);
        buffer_copy_string_len(ds->value, value, vallen);

        array_insert_unique(con->response.headers, (data_unset *)ds);

        return 0;
}

This should be possible because data_response_insert_dup is static.

So the data_string is explicitly "type casted" to data_response, and "misuse" of the response headers list won't break the final output again.

Regards,
Boris

Also available in: Atom