Prevent panic in NewSession function

[ahmdrz]

Hi gorilla team !

[kisielk]

Where does this actually cause a panic?

[ahmdrz]

Here is an example :

s := sessions.NewSession(store, "session-name")
s.Save(r, w)

On Line 112 in store.go

// Save adds a single session to the response.
func (s *CookieStore) Save(r *http.Request, w http.ResponseWriter,
	session *Session) error {
	encoded, err := securecookie.EncodeMulti(session.Name(), session.Values,
		s.Codecs...)
	if err != nil {
		return err
	}
	http.SetCookie(w, NewCookie(session.Name(), encoded, session.Options))
	return nil
}

Is session.Options point to null , Panic occurred :

invalid memory address or nil pointer dereference

[kisielk]

Hm interesting that nobody noticed this before, I guess it only affects CookieStore.

Do you think you can add a test for this?

[ahmdrz]

Yes , Maybe tomorrow On Jan 15, 2018 22:26, Kamil Kisiel <[email protected]> wrote: Hm interesting that nobody noticed this before, I guess it only affects CookieStore. Do you think you can add a test for this? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#140 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AQK5JHCM4WZX4tBvQ30Usy3wg5O32it9ks5tK59sgaJpZM4RVlTo>.

[ahmdrz]

Here is a simple test function.

[kisielk]

LGTM. @elithrar for a sanity check?

[elithrar]

• Need to update the test so that it fails under the current ref first
• Update the test name to scope it further - e.g. TestCookieStoreMapPanic

[elithrar]

The test should "fail" in the current revision: the panic will cause the test not to run as it won't get to this stage. You should use a deferred recover() to "catch" any potential panic and then call t.Fatal.

[ahmdrz]

The test should "fail" in the current revision

Your right , But in my PR, there is no failure because I fixed bug.

[ahmdrz]

Okay , Here you are

[ahmdrz]

Hello ?