Prevent panic in NewSession function#140
Conversation
|
Where does this actually cause a panic? |
|
Here is an example : s := sessions.NewSession(store, "session-name")
s.Save(r, w)On Line 112 in // 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
|
|
Hm interesting that nobody noticed this before, I guess it only affects CookieStore. Do you think you can add a test for this? |
|
Yes , Maybe tomorrow
On Jan 15, 2018 22:26, Kamil Kisiel <notifications@github.com> 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>.
|
|
Here is a simple test function. |
|
LGTM. @elithrar for a sanity check? |
elithrar
left a comment
There was a problem hiding this comment.
- 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
|
|
||
| err = session.Save(req, w) | ||
| if err != nil { | ||
| t.Fatal("failed to save session", err) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The test should "fail" in the current revision
Your right , But in my PR, there is no failure because I fixed bug.
|
Okay , Here you are |
|
Hello ? |
Hi gorilla team !