alexedwards / scs

HTTP Session Management for Go
MIT License
2.05k stars 165 forks source link

Ignore GobCodec.Decode errors? #156

Closed d2ro closed 1 year ago

d2ro commented 1 year ago

If a custom type is stored in a session, and that custom type is modified, then GobCodec.Decode fails if a session contains the old type. LoadAndSave returns an "Internal Server Error" and the http handler is not even called.

A clean way to prevent this would be to never change a type used in a session, and version custom types (CustomTypeV1, CustomTypeV2 etc) instead.

But either way, getting empty session data might always better than just getting an internal server error, which lasts until the user deletes their cookies or the session expires. Thus I'm using a custom GobCodec which just drops the error:

func (TryGobCodec) Decode(b []byte) (deadline time.Time, values map[string]interface{}, err error) {
    time, values, _ := scs.GobCodec{}.Decode(b)
    return time, values, nil
}

What is your opinion on this? I could submit a pull request to add TryGobCodec. Or would it be useful to change the behavior of GobCodec?

alexedwards commented 1 year ago

I don't think we should change the behavior of GobCodec, I think it's important for it to always return an error if one is encountered.

If you want to ignore the error in specific scenarios, then I believe the right way to do this is to implement your own custom GobCodec type, just like you have with TryGobCodec. Although I'd suggest that you check for a specific expected error and choose to ignore it, rather than just ignoring all errors completely.

d2ro commented 1 year ago

Thank you very much for your reply. I absolutely agree with you that an error should always be returned if one is encountered.

I still see the risk that a developer might unwittingly modify the gob serialization format of a custom data type which is stored in a session. That would result in visitors getting internal server errors if their session still holds data in the old serialization format.

A custom ErrorFunc which clears the session (if the error is a gob error) is probably a better solution for this.

alexedwards commented 1 year ago

A custom ErrorFunc which clears the session (if the error is a gob error) is probably a better solution for this.

Good idea, I think that could be a nice approach to this.