Add Lax + Secured cookie options#16
Open
TheMightyFid wants to merge 3 commits into
Open
Conversation
Contributor
Reviewer's GuideAdds configurability for session cookie security attributes (Secure flag and SameSite policy), introduces builder support for those options, updates cookie usage across auth and logout flows, and improves behavior and debug logging when an already-authenticated user hits the auth route. Sequence diagram for auth route handling with existing sessionsequenceDiagram
actor User
participant Router
participant dispatch_auth
participant AuthCache as cache
participant App
User->>Router: GET /auth
Router->>dispatch_auth: dispatch_auth(configuration, cache, post_login_redirect, existing_session_id)
alt [existing_session_id is_some]
dispatch_auth->>cache: get_auth_session(session_id)
alt [Ok(Some(_))]
dispatch_auth-->>User: Redirect::to(target)
User->>App: GET target
else [not valid]
dispatch_auth->>dispatch_auth: handle_auth(configuration, cache, post_login_redirect)
dispatch_auth-->>User: auth flow response
end
else [no existing_session_id]
dispatch_auth->>dispatch_auth: handle_auth(configuration, cache, post_login_redirect)
dispatch_auth-->>User: auth flow response
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The SameSite/secure cookie configuration logic is duplicated in multiple handlers; consider extracting a small helper to construct the session cookie consistently from
OAuthConfiguration. - In
handle_callback, the session cookiemax_ageis hard-coded to 60 minutes while other flows usesession_max_age; aligning these via configuration would avoid divergent expiration behavior. - The debug logging that inspects the raw
COOKIEheader withcontains(SESSION_KEY)may produce false positives if other cookies share that substring; parsing the header into individual cookie names would be more robust.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The SameSite/secure cookie configuration logic is duplicated in multiple handlers; consider extracting a small helper to construct the session cookie consistently from `OAuthConfiguration`.
- In `handle_callback`, the session cookie `max_age` is hard-coded to 60 minutes while other flows use `session_max_age`; aligning these via configuration would avoid divergent expiration behavior.
- The debug logging that inspects the raw `COOKIE` header with `contains(SESSION_KEY)` may produce false positives if other cookies share that substring; parsing the header into individual cookie names would be more robust.
## Individual Comments
### Comment 1
<location path="src/authentication/router/handle_callback.rs" line_range="165-170" />
<code_context>
.http_only(true)
- .same_site(axum_extra::extract::cookie::SameSite::Strict)
- .secure(true),
+ .same_site(if configuration.lax_same_site {
+ axum_extra::extract::cookie::SameSite::Lax
+ } else {
</code_context>
<issue_to_address>
**suggestion:** Cookie attribute logic (SameSite/Secure) is duplicated across multiple handlers and could benefit from a shared helper.
The SameSite/secure_cookies branching here, in the logout handlers, and in the default router should be moved into a shared helper that constructs a Cookie with the correct attributes. This will reduce duplication and avoid these code paths diverging if you later add attributes like domain or adjust SameSite handling for specific flows.
Suggested implementation:
```rust
.await?;
fn build_session_cookie(
name: &'static str,
value: String,
lax_same_site: bool,
secure_cookies: bool,
max_age: Option<Duration>,
) -> Cookie<'static> {
let mut builder = Cookie::build((name, value))
.path("/")
.http_only(true)
.same_site(if lax_same_site {
axum_extra::extract::cookie::SameSite::Lax
} else {
axum_extra::extract::cookie::SameSite::Strict
})
.secure(secure_cookies);
if let Some(age) = max_age {
builder = builder.max_age(age);
}
builder
}
tracing::debug!("auth: token exchange succeeded, session stored and cookie set");
let jar = jar.add(build_session_cookie(
SESSION_KEY,
id.clone(),
configuration.lax_same_site,
configuration.secure_cookies,
Some(Duration::minutes(60)),
));
```
To fully apply the refactoring across the codebase and remove duplication:
1. Move `build_session_cookie` (or a more generic `build_cookie_with_auth_defaults`) to a shared module (e.g. `src/authentication/cookies.rs`) and adjust its visibility (`pub` if needed).
2. Replace the SameSite/secure cookie construction in the logout handlers and in the default router with calls to this shared helper, passing the appropriate cookie name, value, and `max_age` (or `None` where not used).
3. If you move the helper to a different module, update imports/usages in `handle_callback.rs` accordingly (remove the local function and call the shared helper instead).
</issue_to_address>
### Comment 2
<location path="src/authentication/router/handle_callback.rs" line_range="171" />
<code_context>
+ axum_extra::extract::cookie::SameSite::Strict
+ })
+ .secure(configuration.secure_cookies)
.max_age(Duration::minutes(60)),
);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Session cookie max_age is hard-coded to 60 minutes instead of using the configured session lifetime.
In `handle_default` the session cookie duration is taken from `session_max_age`, but here it’s fixed at 60 minutes. If sessions are meant to respect the configured lifetime, consider using `session_max_age` (or the same source of truth) here as well, or clearly document why this endpoint differs.
Suggested implementation:
```rust
.same_site(if configuration.lax_same_site {
axum_extra::extract::cookie::SameSite::Lax
} else {
axum_extra::extract::cookie::SameSite::Strict
})
.secure(configuration.secure_cookies)
.max_age(session_max_age),
```
1. Ensure `session_max_age` is defined in `handle_callback` in the same way it is in `handle_default` (e.g. derived from configuration, such as `configuration.session_max_age`, and converted to the appropriate `time::Duration` type).
2. If this handler sets any other session cookies with a hard-coded `Duration::minutes(60)`, update those `.max_age(Duration::minutes(60))` calls to `.max_age(session_max_age)` as well to keep cookie lifetimes consistent.
3. If `session_max_age` is currently local to `handle_default`, consider extracting a helper function or reusing the same computation logic in `handle_callback` to avoid duplication and ensure a single source of truth for session lifetime.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+165
to
+170
| .same_site(if configuration.lax_same_site { | ||
| axum_extra::extract::cookie::SameSite::Lax | ||
| } else { | ||
| axum_extra::extract::cookie::SameSite::Strict | ||
| }) | ||
| .secure(configuration.secure_cookies) |
Contributor
There was a problem hiding this comment.
suggestion: Cookie attribute logic (SameSite/Secure) is duplicated across multiple handlers and could benefit from a shared helper.
The SameSite/secure_cookies branching here, in the logout handlers, and in the default router should be moved into a shared helper that constructs a Cookie with the correct attributes. This will reduce duplication and avoid these code paths diverging if you later add attributes like domain or adjust SameSite handling for specific flows.
Suggested implementation:
.await?;
fn build_session_cookie(
name: &'static str,
value: String,
lax_same_site: bool,
secure_cookies: bool,
max_age: Option<Duration>,
) -> Cookie<'static> {
let mut builder = Cookie::build((name, value))
.path("/")
.http_only(true)
.same_site(if lax_same_site {
axum_extra::extract::cookie::SameSite::Lax
} else {
axum_extra::extract::cookie::SameSite::Strict
})
.secure(secure_cookies);
if let Some(age) = max_age {
builder = builder.max_age(age);
}
builder
}
tracing::debug!("auth: token exchange succeeded, session stored and cookie set");
let jar = jar.add(build_session_cookie(
SESSION_KEY,
id.clone(),
configuration.lax_same_site,
configuration.secure_cookies,
Some(Duration::minutes(60)),
));
To fully apply the refactoring across the codebase and remove duplication:
- Move
build_session_cookie(or a more genericbuild_cookie_with_auth_defaults) to a shared module (e.g.src/authentication/cookies.rs) and adjust its visibility (pubif needed). - Replace the SameSite/secure cookie construction in the logout handlers and in the default router with calls to this shared helper, passing the appropriate cookie name, value, and
max_age(orNonewhere not used). - If you move the helper to a different module, update imports/usages in
handle_callback.rsaccordingly (remove the local function and call the shared helper instead).
| axum_extra::extract::cookie::SameSite::Strict | ||
| }) | ||
| .secure(configuration.secure_cookies) | ||
| .max_age(Duration::minutes(60)), |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Session cookie max_age is hard-coded to 60 minutes instead of using the configured session lifetime.
In handle_default the session cookie duration is taken from session_max_age, but here it’s fixed at 60 minutes. If sessions are meant to respect the configured lifetime, consider using session_max_age (or the same source of truth) here as well, or clearly document why this endpoint differs.
Suggested implementation:
.same_site(if configuration.lax_same_site {
axum_extra::extract::cookie::SameSite::Lax
} else {
axum_extra::extract::cookie::SameSite::Strict
})
.secure(configuration.secure_cookies)
.max_age(session_max_age),- Ensure
session_max_ageis defined inhandle_callbackin the same way it is inhandle_default(e.g. derived from configuration, such asconfiguration.session_max_age, and converted to the appropriatetime::Durationtype). - If this handler sets any other session cookies with a hard-coded
Duration::minutes(60), update those.max_age(Duration::minutes(60))calls to.max_age(session_max_age)as well to keep cookie lifetimes consistent. - If
session_max_ageis currently local tohandle_default, consider extracting a helper function or reusing the same computation logic inhandle_callbackto avoid duplication and ensure a single source of truth for session lifetime.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add support for:
Summary by Sourcery
Add configurable session cookie security settings and improve handling of authenticated requests to the auth route.
New Features:
Bug Fixes:
Enhancements:
Tests: