Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
## unreleased

- **Access logs now go to stdout.** SQLPage now writes the single per-request completion log line to stdout with the target `sqlpage::access`, matching common application-server and container logging conventions. Diagnostic logs, warnings, and internal errors still go to stderr. If your `LOG_LEVEL` or `RUST_LOG` filter is scoped to a specific old target such as `sqlpage::webserver::http=info`, add `sqlpage::access=info` so request-completion logs are still emitted. If your log pipeline only collects stderr, update it to collect stdout too.
- **OIDC login no longer restarts on a stale callback after login has already completed.** If an OIDC callback URL is received again after its temporary `sqlpage_oidc_state_*` cookie has been consumed, but the request already has a valid SQLPage session, SQLPage now treats it as a stale callback and redirects home instead of starting a new login flow.

## v0.44.1

Expand Down
21 changes: 21 additions & 0 deletions src/webserver/oidc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -539,6 +539,18 @@ fn handle_oidc_callback_error(
request: ServiceRequest,
e: &anyhow::Error,
) -> ServiceResponse {
if is_missing_tmp_login_flow_state_cookie_error(e)
&& matches!(
get_authenticated_user_info(oidc_state, &request),
Ok(Some(_))
)
{
log::warn!("Ignoring replayed OIDC callback for an already authenticated session: {e:#}");
let mut resp = build_redirect_response("/".to_string());

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Respect site_prefix when ignoring replayed callbacks

When site_prefix is configured and SQLPage is mounted only under that prefix, this new recovery path sends the browser to the host root instead of the SQLPage app. A replayed callback for /my-app/sqlpage/oidc_callback would therefore redirect to /, which may be handled by another app or by the reverse proxy rather than reaching SQLPage's prefix redirect; use the configured prefixed home URL for this fallback.

Useful? React with 👍 / 👎.

clear_redirect_count_cookie(&mut resp);
return request.into_response(resp);
}

let redirect_count = get_redirect_count(&request);
if redirect_count >= MAX_OIDC_REDIRECTS {
return handle_max_redirect_count_reached(request, e, redirect_count);
Expand Down Expand Up @@ -909,6 +921,15 @@ fn build_oidc_error_response(request: &ServiceRequest, e: &anyhow::Error) -> Htt
)
}

fn is_missing_tmp_login_flow_state_cookie_error(e: &anyhow::Error) -> bool {
e.chain().any(|cause| {
let msg = cause.to_string();
msg.starts_with("No ")
&& msg.contains(SQLPAGE_TMP_LOGIN_STATE_COOKIE_PREFIX)
&& msg.ends_with(" cookie found")
})
}

/// Returns the claims from the ID token in the `SQLPage` auth cookie.
fn get_authenticated_user_info(
oidc_state: &OidcState,
Expand Down
42 changes: 42 additions & 0 deletions tests/oidc/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,48 @@ async fn test_oidc_happy_path() {
assert_eq!(final_resp.status(), StatusCode::OK);
}

#[actix_web::test]
async fn test_oidc_replayed_callback_after_login_does_not_restart_login() {
let (app, provider) = setup_oidc_test(|_| {}).await;
let mut cookies: Vec<Cookie<'static>> = Vec::new();

let resp = request_with_cookies!(app, test::TestRequest::get().uri("/"), cookies);
assert_eq!(resp.status(), StatusCode::SEE_OTHER);
let auth_url = Url::parse(resp.headers().get("location").unwrap().to_str().unwrap()).unwrap();

let state = get_query_param(&auth_url, "state");
let nonce = get_query_param(&auth_url, "nonce");
let redirect_uri = get_query_param(&auth_url, "redirect_uri");
provider.store_auth_code("test_auth_code".to_string(), nonce);

let callback_uri = format!(
"{}?code=test_auth_code&state={}",
Url::parse(&redirect_uri).unwrap().path(),
state
);
let callback_resp =
request_with_cookies!(app, test::TestRequest::get().uri(&callback_uri), cookies);
assert_eq!(callback_resp.status(), StatusCode::SEE_OTHER);

// This is the smallest deterministic reproduction of the log pattern from
// the bug report: the first callback completed the login and removed the
// temporary sqlpage_oidc_state_* cookie; the same callback URL is then
// received again while the browser already carries the final authenticated
// session cookies. That stale callback must not start another OIDC flow.
let replay_resp =
request_with_cookies!(app, test::TestRequest::get().uri(&callback_uri), cookies);
assert_eq!(replay_resp.status(), StatusCode::SEE_OTHER);
assert_eq!(
replay_resp
.headers()
.get("location")
.unwrap()
.to_str()
.unwrap(),
"/"
);
}

async fn assert_oidc_login_fails(
provider_mutator: impl FnOnce(&mut ProviderState),
state_override: Option<String>,
Expand Down