Changed auth/encryption key prevents the signed in user from accessing the webpage again

[inmylo]

Describe the bug

When a single pair of an authentication key and encryption key is used - changing it prevents the signed in user from accessing the webpage again.

Versions

Go version: 1.16.2 linux/amd64
package version: 15ff351

Steps to Reproduce

I create a sessions Mongostore with:

store, err := mongostore.NewStore(
	db.Sessions,
	http.Cookie{
		Path:     "/",
		Domain:   "",
		MaxAge:   3600,
		Secure:   true,
		HttpOnly: true,
		SameSite: http.SameSiteStrictMode,
	},
	securecookie.GenerateRandomKey(64),
	securecookie.GenerateRandomKey(32),
)

When user requests the webpage check whether he is signed in:

fmt.Println(1)
session, err := store.Get(r, "CookieName")
fmt.Println(2)

store.Get calls a function from your package:

return sessions.GetRegistry(r).Get(s, name)

I've added a debug printing to this function:

func (s *Registry) Get(store Store, name string) (session *Session, err error) {
	fmt.Println(200, name)
	if !isCookieNameValid(name) {
		return nil, fmt.Errorf("sessions: invalid character in cookie name: %s", name)
	}
	if info, ok := s.sessions[name]; ok {
		session, err = info.s, info.e
	} else {
		fmt.Println(201)
		session, err = store.New(s.request, name)
		fmt.Printf("202 %#v\n", session)
		fmt.Printf("203 %#v\n", err)
		fmt.Printf("204 %#v\n", session.name)
		session.name = name
		fmt.Println(205)
		s.sessions[name] = sessionInfo{s: session, e: err}
		fmt.Println(206)
	}
	session.store = store
	return
}

I sign in to the website. When service is restarted - new pair of an authentication key and encryption key is generated. I refresh the webpage - server returns literally nothing, empty page, no Go errors. In a console I see only:

1
200 sessionID
201
202 (*sessions.Session)(nil)
203 &fmt.wrapError{msg:"[ERROR] decoding cookie: securecookie: the value is not valid", err:securecookie.MultiError{securecookie.cookieError{typ:2, msg:"the value is not valid", cause:error(nil)}}}

.. that means fmt.Printf("204") and the later code is never called for some reason, feels like current goroutine is stuck or being dropped.

The only thing that helps - clearing browser's cookies for this website manually. Please make func (s *Registry) Get() to return something even if it can't decode cookies.

Expected behavior

When service is restarted - new pair of an authentication key and encryption key is generated, sessions package informs that user is not signed in and returns an error that can't decode cookies. User has to sign in again.

[elithrar]

What does *your* code do here? It’s the handler code that decides how to handle the error returned from store.Get.

[inmylo]

@elithrar , my code here is to let you know that I use mongostore, not securecookies/sessions directly. The issue is that this package's sessions.GetRegistry(r).Get(s, name) doesn't even return and I can't do anything on my side - there are no errors I could check

[elithrar]

The store shouldn't matter here: your handler code determines how to handle the error when a store (any store!) can't decode an existing session due to key changes.

You posted the below, but nothing else. Where is the error handling for store.Get here?

fmt.Println(1)
session, err := store.Get(r, "CookieName")
fmt.Println(2)

[inmylo]

@elithrar , please check the console output I posted. 2 is never printed. That means I can't check the error as nothing was returned, like execution is stuck

[elithrar]

Is there no stack trace printed at all?

[inmylo]

Nothing at all. Only these 5 lines I've put into description. In a line with 204 there is accessing a field of a nil object - maybe this causes some problems. But no errors in a console. If I request the page 3 times - these 5 lines are printed 3 times.

If you have ideas I can add more debugging things into my code.

[inmylo]

Found a reason why panics were ignored in my case, fixed it. Now I see the errors form the code above:

2021/12/13 16:24:04 http2: panic serving 127.0.0.1:49810: runtime error: invalid memory address or nil pointer dereference
goroutine 81 [running]:
net/http.(*http2serverConn).runHandler.func1(0xc0000e60e8, 0xc00085df8e, 0xc0002e0600)
	/path/go/src/net/http/h2_bundle.go:5716 +0x193
panic(0xf66320, 0x161f0a0)
	/path/go/src/runtime/panic.go:965 +0x1b9
github.com/gorilla/sessions.(*Registry).Get(0xc0008550e0, 0x11e0ce0, 0xc0001b2820, 0xc0002733b0, 0x9, 0x9, 0x9, 0xc000577010)
	/path/pkg/mod/github.com/gorilla/sessions@v1.2.1/sessions.go:140 +0x14a
github.com/go-stuff/mongostore.(*Store).Get(0xc0001b2820, 0xc000160e00, 0xc0002733b0, 0x9, 0xc00057701a, 0x5, 0x0)
	/path/pkg/mod/github.com/go-stuff/mongostore@v1.0.0/mongostore.go:100 +0x67

That means store.Get(r, "CookieName") panics, because nil session object is created, but package tries to access its fields:

session, err = store.New(s.request, name)
session.name = name

[inmylo]

On the other hand, the mongostore package returns nil session if can't decode it. Should I move this issue to that package or you'll fix on your side somehow?

[fednelpat]

@inmylo As the session store states, "Note that New should never return a nil session, even in the case of an error if using the Registry infrastructure to cache the session." which means that it's mongostore's fault. However the fact that the error is ignored is debatable, that is, should the session be stored if its creation returned an error?

About that, what I don't understand @elithrar is why we don't immediately return the error and instead save both the session and the error into the registry's sessions. Wouldn't the best way to handle this be to save the session only, but only if there are no errors?

[elithrar]

I didn't write the original code, but it seems like a short-circuit return and/or better nil handling from store implementations would solve this.

[inmylo]

@deltarays , should the session be stored if its creation returned an error? - in this case error and nil session were returned, because it was impossible to decode existing cookie (because the only enc. key was changed). So I believe it's Ok to create a fresh new session and user will just be forced to sign in again

[fednelpat]

Alright, once I get home I'm going to make a pull request for that

[aeneasr]

I'd suggest moving this to the mongo library, as it is a problem on their end not following the contracts of this library's interfaces!

[stale]

This issue has been automatically marked as stale because it hasn't seen a recent update. It'll be automatically closed in a few days.

[inmylo]

@deltarays , @elithrar any updates?

[fednelpat]

Hi @inmylo, sorry for the late response but I ended up closing the pull request since as per the discussion with aeneasr my change would've had the possibility to break code in certain cases.