Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

filters/cookie: allow expiration #2495

Merged
merged 1 commit into from
Aug 7, 2023
Merged

filters/cookie: allow expiration #2495

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions docs/reference/filters.md
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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