Skip to content

Commit

Permalink
filters/cookie: allow expiration (#2495)
Browse files Browse the repository at this point in the history 
Fixes #2494

Signed-off-by: Alexander Yastrebov <[email protected]>
  • Loading branch information
@AlexanderYastrebov
AlexanderYastrebov authored Aug 7, 2023
1 parent b02dc97 commit 2579ab3
Show file tree
Hide file tree
Showing 3 changed files with 74 additions and 35 deletions.
10 changes: 6 additions & 4 deletions docs/reference/filters.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand All @@ -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
Expand Down
37 changes: 25 additions & 12 deletions filters/cookie/cookie.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ import (
"net"
"net/http"
"strings"
"time"

"github.com/zalando/skipper/filters"
)
Expand Down Expand Up @@ -72,7 +71,7 @@ type filter struct {
typ direction
name string
value string
ttl time.Duration
maxAge int
changeOnly bool
}

Expand Down Expand Up @@ -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
}
Expand All @@ -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
Expand All @@ -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
}

Expand All @@ -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()
Expand All @@ -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)
}
}

Expand Down
62 changes: 43 additions & 19 deletions filters/cookie/cookie_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package cookie
import (
"net/http"
"testing"
"time"

"github.com/zalando/skipper/filters"
"github.com/zalando/skipper/filters/filtertest"
Expand Down Expand Up @@ -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{},
Expand All @@ -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{},
Expand All @@ -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",
Expand All @@ -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
Expand Down Expand Up @@ -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)
}
}
}
Expand Down Expand Up @@ -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},
"",
Expand All @@ -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},
"",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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},
"",
Expand All @@ -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},
"",
Expand All @@ -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",
Expand All @@ -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",
Expand Down

0 comments on commit 2579ab3

Please sign in to comment.