From 2579ab3bdd787d325ba3a4784e03ba0bc4b291cf Mon Sep 17 00:00:00 2001 From: Alexander Yastrebov Date: Mon, 7 Aug 2023 15:21:29 +0200 Subject: [PATCH] filters/cookie: allow expiration (#2495) Fixes #2494 Signed-off-by: Alexander Yastrebov --- docs/reference/filters.md | 10 +++--- filters/cookie/cookie.go | 37 ++++++++++++++------- filters/cookie/cookie_test.go | 62 ++++++++++++++++++++++++----------- 3 files changed, 74 insertions(+), 35 deletions(-) diff --git a/docs/reference/filters.md b/docs/reference/filters.md index e5b0f7c502..f2478bb5b5 100644 --- a/docs/reference/filters.md +++ b/docs/reference/filters.md @@ -1765,10 +1765,11 @@ requestCookie("test-session", "abc") ### responseCookie -Appends cookies to responses in the "Set-Cookie" header. The response cookie -accepts an optional argument to control the max-age property of the cookie, -of type `int`, in seconds. The response cookie accepts an optional fourth -argument, "change-only", to control if the cookie should be set on every +Appends a cookie to the response via "Set-Cookie" header. +It derives cookie domain by removing one subdomain from the request hostname domain. +The filter accepts an optional argument to set the `Max-Age` attribute of the cookie, of type `int`, in seconds. +Use zero to expire the cookie immediately. +An optional fourth argument, "change-only", controls if the cookie should be set on every response, or only if the request does not contain a cookie with the provided name and value. @@ -1778,6 +1779,7 @@ Example: responseCookie("test-session", "abc") responseCookie("test-session", "abc", 31536000), responseCookie("test-session", "abc", 31536000, "change-only") +responseCookie("test-session", "deleted", 0), ``` ### jsCookie diff --git a/filters/cookie/cookie.go b/filters/cookie/cookie.go index 5fcc11b029..1b82500506 100644 --- a/filters/cookie/cookie.go +++ b/filters/cookie/cookie.go @@ -38,7 +38,6 @@ import ( "net" "net/http" "strings" - "time" "github.com/zalando/skipper/filters" ) @@ -72,7 +71,7 @@ type filter struct { typ direction name string value string - ttl time.Duration + maxAge int changeOnly bool } @@ -216,8 +215,21 @@ func (s *spec) CreateFilter(args []interface{}) (filters.Filter, error) { } if len(args) >= 3 { - if ttl, ok := args[2].(float64); ok { - f.ttl = time.Duration(ttl) * time.Second + if maxAge, ok := args[2].(float64); ok { + // https://pkg.go.dev/net/http#Cookie uses zero to omit Max-Age attribute: + // > MaxAge=0 means no 'Max-Age' attribute specified. + // > MaxAge<0 means delete cookie now, equivalently 'Max-Age: 0' + // > MaxAge>0 means Max-Age attribute present and given in seconds + // + // Here we know user specified Max-Age explicitly, so we interpret zero + // as a signal to delete the cookie similar to what user would expect naturally, + // see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#max-agenumber + // > A zero or negative number will expire the cookie immediately. + if maxAge == 0 { + f.maxAge = -1 + } else { + f.maxAge = int(maxAge) + } } else { return nil, filters.ErrInvalidFilterParameters } @@ -241,7 +253,7 @@ func (f *filter) Request(ctx filters.FilterContext) { } func (f *filter) Response(ctx filters.FilterContext) { - var set func(filters.FilterContext, string, string, time.Duration) + var set func(filters.FilterContext, string, string, int) switch f.typ { case request: return @@ -256,7 +268,7 @@ func (f *filter) Response(ctx filters.FilterContext) { ctx.StateBag()["CookieSet:"+f.name] = f.value if !f.changeOnly { - set(ctx, f.name, f.value, f.ttl) + set(ctx, f.name, f.value, f.maxAge) return } @@ -270,10 +282,10 @@ func (f *filter) Response(ctx filters.FilterContext) { return } - set(ctx, f.name, f.value, f.ttl) + set(ctx, f.name, f.value, f.maxAge) } -func setCookie(ctx filters.FilterContext, name, value string, ttl time.Duration, jsEnabled bool) { +func setCookie(ctx filters.FilterContext, name, value string, maxAge int, jsEnabled bool) { var req = ctx.Request() if ctx.OriginalRequest() != nil { req = ctx.OriginalRequest() @@ -286,14 +298,15 @@ func setCookie(ctx filters.FilterContext, name, value string, ttl time.Duration, Secure: true, Domain: d, Path: "/", - MaxAge: int(ttl.Seconds())} + MaxAge: maxAge, + } ctx.Response().Header.Add(SetCookieHttpHeader, c.String()) } -func configSetCookie(jscookie bool) func(filters.FilterContext, string, string, time.Duration) { - return func(ctx filters.FilterContext, name, value string, ttl time.Duration) { - setCookie(ctx, name, value, ttl, jscookie) +func configSetCookie(jscookie bool) func(filters.FilterContext, string, string, int) { + return func(ctx filters.FilterContext, name, value string, maxAge int) { + setCookie(ctx, name, value, maxAge, jscookie) } } diff --git a/filters/cookie/cookie_test.go b/filters/cookie/cookie_test.go index 51f19af66e..f65b66122e 100644 --- a/filters/cookie/cookie_test.go +++ b/filters/cookie/cookie_test.go @@ -3,7 +3,6 @@ package cookie import ( "net/http" "testing" - "time" "github.com/zalando/skipper/filters" "github.com/zalando/skipper/filters/filtertest" @@ -65,7 +64,7 @@ func TestCreateFilter(t *testing.T) { filter{}, true, }, { - "wrong ttl type", + "wrong Max-Age type", response, []interface{}{"test-cookie", "A", "42"}, filter{}, @@ -89,7 +88,7 @@ func TestCreateFilter(t *testing.T) { filter{}, true, }, { - "wrong ttl type, JS", + "wrong Max-Age type, JS", responseJS, []interface{}{"test-cookie", "A", "42"}, filter{}, @@ -110,19 +109,19 @@ func TestCreateFilter(t *testing.T) { "response persistent cookie", response, []interface{}{"test-cookie", "A", 42.0}, - filter{typ: response, name: "test-cookie", value: "A", ttl: 42 * time.Second}, + filter{typ: response, name: "test-cookie", value: "A", maxAge: 42}, false, }, { "response persistent cookie, not change only, explicit", response, []interface{}{"test-cookie", "A", 42.0, "always"}, - filter{typ: response, name: "test-cookie", value: "A", ttl: 42 * time.Second}, + filter{typ: response, name: "test-cookie", value: "A", maxAge: 42}, false, }, { "response persistent cookie, change only", response, []interface{}{"test-cookie", "A", 42.0, ChangeOnlyArg}, - filter{typ: response, name: "test-cookie", value: "A", ttl: 42 * time.Second, changeOnly: true}, + filter{typ: response, name: "test-cookie", value: "A", maxAge: 42, changeOnly: true}, false, }, { "response session cookie, JS", @@ -134,19 +133,19 @@ func TestCreateFilter(t *testing.T) { "response persistent cookie, JS", responseJS, []interface{}{"test-cookie", "A", 42.0}, - filter{typ: response, name: "test-cookie", value: "A", ttl: 42 * time.Second}, + filter{typ: response, name: "test-cookie", value: "A", maxAge: 42}, false, }, { "response persistent cookie, not change only, explicit, JS", responseJS, []interface{}{"test-cookie", "A", 42.0, "always"}, - filter{typ: response, name: "test-cookie", value: "A", ttl: 42 * time.Second}, + filter{typ: response, name: "test-cookie", value: "A", maxAge: 42}, false, }, { "response persistent cookie, change only, JS", responseJS, []interface{}{"test-cookie", "A", 42.0, ChangeOnlyArg}, - filter{typ: response, name: "test-cookie", value: "A", ttl: 42 * time.Second, changeOnly: true}, + filter{typ: response, name: "test-cookie", value: "A", maxAge: 42, changeOnly: true}, false, }} { var s filters.Spec @@ -177,8 +176,8 @@ func TestCreateFilter(t *testing.T) { t.Error(ti.msg, "value", ff.value, ti.check.value) } - if ff.ttl != ti.check.ttl { - t.Error(ti.msg, "ttl", ff.ttl, ti.check.ttl) + if ff.maxAge != ti.check.maxAge { + t.Error(ti.msg, "Max-Age", ff.maxAge, ti.check.maxAge) } } } @@ -214,7 +213,7 @@ func TestSetCookie(t *testing.T) { Domain: domain, Path: "/"}, }, { - "response cookie, with ttl", + "response cookie, with Max-Age", response, []interface{}{"test-cookie", "A", 42.0}, "", @@ -226,7 +225,20 @@ func TestSetCookie(t *testing.T) { Path: "/", MaxAge: 42}, }, { - "response cookie, with non-sliding ttl", + "delete response cookie", + response, + []interface{}{"test-cookie", "deleted", 0.0}, + "", + &http.Cookie{ + Name: "test-cookie", + Value: "deleted", + HttpOnly: true, + Domain: domain, + Path: "/", + MaxAge: -1, + }, + }, { + "response cookie, with non-sliding Max-Age", response, []interface{}{"test-cookie", "A", 42.0, ChangeOnlyArg}, "", @@ -238,7 +250,7 @@ func TestSetCookie(t *testing.T) { Path: "/", MaxAge: 42}, }, { - "response cookie, with non-sliding ttl, request contains different value", + "response cookie, with non-sliding Max-Age, request contains different value", response, []interface{}{"test-cookie", "A", 42.0, ChangeOnlyArg}, "B", @@ -250,7 +262,7 @@ func TestSetCookie(t *testing.T) { Path: "/", MaxAge: 42}, }, { - "response cookie, with non-sliding ttl, request contains the same value", + "response cookie, with non-sliding Max-Age, request contains the same value", response, []interface{}{"test-cookie", "A", 42.0, ChangeOnlyArg}, "A", @@ -266,7 +278,7 @@ func TestSetCookie(t *testing.T) { Domain: domain, Path: "/"}, }, { - "response cookie, with ttl, JS", + "response cookie, with Max-Age, JS", responseJS, []interface{}{"test-cookie", "A", 42.0}, "", @@ -277,7 +289,19 @@ func TestSetCookie(t *testing.T) { Path: "/", MaxAge: 42}, }, { - "response cookie, with non-sliding ttl, JS", + "delete response js cookie", + responseJS, + []interface{}{"test-cookie", "deleted", 0.0}, + "", + &http.Cookie{ + Name: "test-cookie", + Value: "deleted", + Domain: domain, + Path: "/", + MaxAge: -1, + }, + }, { + "response cookie, with non-sliding Max-Age, JS", responseJS, []interface{}{"test-cookie", "A", 42.0, ChangeOnlyArg}, "", @@ -288,7 +312,7 @@ func TestSetCookie(t *testing.T) { Path: "/", MaxAge: 42}, }, { - "response cookie, with non-sliding ttl, request contains different value, JS", + "response cookie, with non-sliding Max-Age, request contains different value, JS", responseJS, []interface{}{"test-cookie", "A", 42.0, ChangeOnlyArg}, "B", @@ -299,7 +323,7 @@ func TestSetCookie(t *testing.T) { Path: "/", MaxAge: 42}, }, { - "response cookie, with non-sliding ttl, request contains the same value, JS", + "response cookie, with non-sliding Max-Age, request contains the same value, JS", responseJS, []interface{}{"test-cookie", "A", 42.0, ChangeOnlyArg}, "A",