Do not save session on client error

[0rzech]

IMHO, session should not be saved on client error. For instance, with always_save option it can lead to prolonging session lifetime on HTTP 401.

Maybe I'm wrong, but in general it seems weird to save session even when client made an invalid request.

But perhaps it would be better not to save on client error, but only when the session was not modified anyway, like so:

(modified || (session_config.always_save && !res.status().is_client_error())

?

Or maybe we should just stick to the Django way you showed in #220 (comment) and close this PR?

[maxcountryman]

Can you go into more detail regarding the logic here? Django only filters out server errors.

[0rzech]

I've updated PR description. Sorry for the mess.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.85%. Comparing base (66983f5) to head (1d4adb9).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #220      +/-   ##
==========================================
+ Coverage   83.80%   83.85%   +0.04%     
==========================================
  Files           5        5              
  Lines         352      353       +1     
==========================================
+ Hits          295      296       +1     
  Misses         57       57              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[maxcountryman]

I think I'd like to understand better why Django doesn't do this. (Maybe there's some historical context or discussion about it.) In a similar vein, are there examples of other established frameworks also not saving on client errors?

[0rzech]

I did some archaeology and it looks like http status code exclusions were added fairly recently to, case-by-case and for different reasons - Django session middleware had been masking exceptions / status codes without these exclusions. At first no status codes were excluded, then only 500 (https://code.djangoproject.com/ticket/3881) and then status codes >= 500 (https://code.djangoproject.com/ticket/34173).

I tried to find how other frameworks do it, but I'm not sure if they implement "always save" at all. RoR doesn't seem to have it: https://api.rubyonrails.org/v7.2.2/classes/ActionDispatch/Session/CookieStore.html . Phoenix Framework doesn't seem to have such option either: https://hexdocs.pm/plug/Plug.Session.html . Same with Spring: https://docs.spring.io/spring-session/reference/configuration/common.html . It looks like developers are supposed to implement it themselves there.

[maxcountryman]

Interesting! Thanks for looking into that. Do you think we shouldn't try to implement "always save" altogether?

[0rzech]

I think it's good to have this option, either built-in or through some Fn(...).

Perhaps RFC will help us: https://datatracker.ietf.org/doc/html/rfc6265#section-3 ?

Origin servers MAY send a Set-Cookie response header with any
response. User agents MAY ignore Set-Cookie headers contained in
responses with 100-level status codes but MUST process Set-Cookie
headers contained in other responses (including responses with 400-
and 500-level status codes).

This means that setting cookies on both 4xx and 5xx is fine according to spec.

Now, the question is whether we would like to skip refreshing the session when these errors occur and how to do it?

I think there are several options:

• Remove always_save, but add a way to easily hook into the code so that developers can implement decision process themselves. I suppose the ease of doing this in other frameworks is the reason why there's no such option there.
• Add setting with status code ranges or something to be filtered out and decide whether always filter out those status codes or only in case of always_save == true.
• Remove

  tower-sessions/src/service.rs

  Line 253 in 66983f5

  && !res.status().is_server_error() =>

  because tower-sessions does not mask errors, so the primary reason for Django's workaround is not there.
• Change < 500 to < 400. This would increase hardcoded status code range, though.
• Leave the code as is. But why should server error always prevent session from being saved and its expiration time from being prolonged?

[maxcountryman]

I kind of like the first option, since that seems like the most flexible but maybe it would be annoying to implement?

[0rzech]

I like the first option too. I haven't tried to implement it yet, but SessionConfig is both Clone and Debug, so it's a no-go for Fn field there. Also no trait for us due to orphan rule.

[maxcountryman]

If we can remove Clone and Debug or otherwise rework SessionConfig, that's fine. (There's another issue where some applications would like to mutate the config after it's been set up so it may be worthwhile to rethink that altogether.)

[0rzech]

Removing Clone and Debug would break backwards compatibility. If we're OK with that, then perhaps we could wait until it's safe to update rand 0.8 to 0.9, as this is a breaking change too? Currently tower-sessions-core depends on rand 0.8.5 directly and axum-login depends on it traisitively.