cookie.get() return value incorrect when called with nonexistent cookie

[mszabo-wikia]

Expected Behavior

cookie.get() should return an empty string if no such cookie was present in the request header, as documented

Current Behavior

On Varnish 6.6.0, cookie.get() returns something other than an empty string/empty value.

Steps to Reproduce (for bugs)

Simple varnishtest reproducer:

varnishtest "cookie.get() with nonexistent cookie"

server s1 {
    rxreq
    expect req.http.X-Cookie-Present == "0"
    txresp
} -start

varnish v1 -vcl+backend {
    vcl 4.1;
    import cookie;

    sub vcl_recv {
        if (req.http.Cookie) {
            cookie.parse(req.http.Cookie);

            if (cookie.get("no_such_cookie") != "") {
                set req.http.X-Cookie-Present = "1";
            } else {
                set req.http.X-Cookie-Present = "0";
            }
        }
        return(pass);
    }
} -start

client c1 {
    txreq -hdr "Cookie: some_cookie=foo;other_cookie=bar"
    rxresp
} -run

The above test also fails if the condition is changed to if (!cookie.get("no_such_cookie")). cookie.isset() however works as expected.

Context

I wanted to use cookie.get("no_such_cookie") != "" to handle both the case where the cookie is not set, or where it's set but the value is empty.

Your Environment

• Version used: 6.6.0
• Operating System and version: Debian Buster

[dridi]

That's probably because cookie.get() returns null and not an empty string.

The same would happen with a non-existent header:

varnishtest 3634

varnish v1 -vcl {
        backend be none;
        sub vcl_recv {
                return (synth(200));
        }
        sub vcl_synth {
                if (req.http.Cookie) {
                        set resp.http.X-Cookie-Present = "1";
                } else {
                        set resp.http.X-Cookie-Present = "0";
                }
                if (req.http.Cookie == "") {
                        set resp.http.X-Cookie-Empty = "1";
                } else {
                        set resp.http.X-Cookie-Empty = "0";
                }
        }
} -start

client c1 {
        txreq
        rxresp
        expect resp.http.X-Cookie-Present == 0
        expect resp.http.X-Cookie-Empty == 0
} -run

[hermunn]

This test, where a temporary header is used, passes:

varnishtest "cookie.get() with nonexistent cookie"

server s1 {
	rxreq
	expect req.http.X-Cookie-Present == "0"
	txresp
} -start

varnish v1 -vcl+backend {
	vcl 4.1;
	import cookie;

	sub vcl_recv {
		if (req.http.Cookie) {
			cookie.parse(req.http.Cookie);
			set req.http.X-tmp = cookie.get("no_such_cookie");
			if (req.http.X-tmp != "") {
				set req.http.X-Cookie-Present = "1";
			} else {
				set req.http.X-Cookie-Present = "0";
			}
		}
		return(pass);
	}
} -start

client c1 {
	txreq -hdr "Cookie: some_cookie=foo;other_cookie=bar"
	rxresp
} -run

I disagree with @dridi when it comes to closing this issue. The documentation clearly states that the return value is an empty string, and comparing the return value with the empty string seems a natural thing to do (even though it is more natural to me to store it in a header first, since you will need it soon after if it is there).

We might just keep the code but fix the documentation somehow, but this is unclear to me. The problem here is possibly present at other places in the code where "the empty string" is mentioned.

[hermunn]

This, too, passes, and it is very close to your test case, exploiting the fact that NULL, and not the empty string, is returned:

varnishtest "cookie.get() with nonexistent cookie"

server s1 {
	rxreq
	expect req.http.X-Cookie-Present == "0"
	txresp
} -start

varnish v1 -vcl+backend {
	vcl 4.1;
	import cookie;

	sub vcl_recv {
		if (req.http.Cookie) {
			cookie.parse(req.http.Cookie);
			if (cookie.get("no_such_cookie")) {
				set req.http.X-Cookie-Present = "1";
			} else {
				set req.http.X-Cookie-Present = "0";
			}
		}
		return(pass);
	}
} -start

client c1 {
	txreq -hdr "Cookie: some_cookie=foo;other_cookie=bar"
	rxresp
} -run

Would it be acceptable to change the docs to state that the NULL string is returned, and then include examples for testing if the cookie is there, both directly (like here) or via a temporary header (in my previous post)?

I am just throwing ideas around, of course.

[dridi]

I think it's conceptually fine to fix vmod_cookie to match what the manual says, because cookie.isset() already fills the gap once cookie.get() no longer returns null.

The downside is that we risk breaking existing VCL code in subtle ways, so I'd probably prefer a docfix.

[gquintard]

the establish culture is to return NULL when something isn't found, so I'd rather keep it and docfix. Empty and NULL are to different things

[mszabo-wikia]

Yeah, clarifying the behavior in the docs sounds fine to me.

[hermunn]

Just to clarify: #3647 is a PR which doc fixes this. Anyone are welcome to comment, of course.

After summer, when all the Varnish Cache contributors are back, is probably the time this ticket will be closed.

edit: typo/grammar fix

[mszabo-wikia]

Awesome, thanks a lot for jumping in on this!