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.

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

ClearSessionCookie doesn't work for splitted cookies #69

Closed
einfachchr opened this issue Feb 21, 2019 · 1 comment
Closed

ClearSessionCookie doesn't work for splitted cookies #69

einfachchr opened this issue Feb 21, 2019 · 1 comment

Comments

@einfachchr
Copy link
Contributor

Session cookies will be splitted if they're too large. On logout ClearSessionCookies should remove
all cookies. That does not work if cookies are splitted due to naming

Expected Behavior

ClearSessionCookies should remove all cookies set on login.

Current Behavior

When a user logs in SetSessionCookie() is responsible for setting an cookie with user information. The cookie is
created by MakeSessionCookie(...) with provider specific information. If the length of the cookie is to large, it
will be split into several cookies which are named <cookie_name>_0, <cookie_name>_1, ...
-> SET: _oauth2_proxy_0, _oauth2_proxy_1

Cookies are deleted by creating an empty cookie with be same name that has already expired. The browser will remove the
old one. The ClearSessionCookie(...) calls MakeSessionCookie(...) to create the "delete"-cookie. This time is is called
without value (to create that empty cookie) resulting in an unsplit delete-cookie.
-> UNSET: _oauth2_proxy

Possible Solution

The ClearSessionCookie() method should search the request cookies for cookies that start with OAuthProxy.CookieName and
remove them

Steps to Reproduce (for bugs)

We added a test to oauthproxy_test.go:

func TestClearSplittedCookie(t *testing.T) {
	p := OAuthProxy{CookieName: "oauth2"}
	var rw = httptest.NewRecorder()
	req := httptest.NewRequest("get", "/", nil)

	req.AddCookie(&http.Cookie{
		Name:  "test1",
		Value: "test1",
	})
	req.AddCookie(&http.Cookie{
		Name:  "oauth2-0",
		Value: "oauth2-0",
	})
	req.AddCookie(&http.Cookie{
		Name:  "oauth2-1",
		Value: "oauth2-1",
	})

	p.ClearSessionCookie(rw, req)
	header := rw.Header()

	assert.Equal(t, 3, len(header["Set-Cookie"]), "should have 3 set-cookie header entries")
}

func TestClearNotSplittedCookie(t *testing.T) {
	p := OAuthProxy{CookieName: "oauth2", CookieDomain: "abc"}
	var rw = httptest.NewRecorder()
	req := httptest.NewRequest("get", "/", nil)

	req.AddCookie(&http.Cookie{
		Name:  "test1",
		Value: "test1",
	})
	req.AddCookie(&http.Cookie{
		Name:  "oauth2",
		Value: "oauth2",
	})

	p.ClearSessionCookie(rw, req)
	header := rw.Header()

	fmt.Printf("%#v\n", header)
	assert.Equal(t, 1, len(header["Set-Cookie"]), "should have 1 set-cookie header entry")
}

Context

This bug permits the logout. If the user reloads the page she will be logged in again.

Your Environment

  • Version used:

The bug works in current versions

JoelSpeed pushed a commit that referenced this issue Mar 15, 2019
* fixes deletion of splitted cookies

* three minor adjustments to improve the tests

* changed cookie name matching to regex

* Update oauthproxy.go

Co-Authored-By: einfachchr <[email protected]>

* removed unused variable

* Changelog
@JoelSpeed
Copy link
Member

Fixed in #70 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants