Skip to content

Commit 70cc375

Browse files
committed
Fix: Prevent infinite OIDC redirects
This commit adds a mechanism to prevent infinite redirects in the OIDC callback flow. It does this by: - Tracking the number of redirects using a cookie. - Setting a maximum number of redirects (3). - Returning an error if the maximum is exceeded.
1 parent 3ada9b4 commit 70cc375

File tree

2 files changed

+52
-4
lines changed

2 files changed

+52
-4
lines changed

src/webserver/error.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ fn error_to_html_string(app_state: &AppState, err: &anyhow::Error) -> anyhow::Re
5252
Ok(out.into_string()?)
5353
}
5454

55-
fn anyhow_err_to_actix_resp(e: &anyhow::Error, state: &AppState) -> HttpResponse {
55+
pub(super) fn anyhow_err_to_actix_resp(e: &anyhow::Error, state: &AppState) -> HttpResponse {
5656
let mut resp = HttpResponseBuilder::new(StatusCode::INTERNAL_SERVER_ERROR);
5757
resp.insert_header((header::CONTENT_TYPE, header::ContentType::plaintext()));
5858

src/webserver/oidc.rs

Lines changed: 51 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ use openidconnect::{
3434
use serde::{Deserialize, Serialize};
3535
use tokio::sync::{RwLock, RwLockReadGuard};
3636

37+
use super::error::anyhow_err_to_actix_resp;
3738
use super::http_client::make_http_client;
3839

3940
type LocalBoxFuture<T> = Pin<Box<dyn Future<Output = T> + 'static>>;
@@ -44,6 +45,8 @@ const SQLPAGE_NONCE_COOKIE_NAME: &str = "sqlpage_oidc_nonce";
4445
const SQLPAGE_TMP_LOGIN_STATE_COOKIE_PREFIX: &str = "sqlpage_oidc_state_";
4546
const OIDC_CLIENT_MAX_REFRESH_INTERVAL: Duration = Duration::from_secs(60 * 60);
4647
const OIDC_CLIENT_MIN_REFRESH_INTERVAL: Duration = Duration::from_secs(5);
48+
const SQLPAGE_OIDC_REDIRECT_COUNT_COOKIE: &str = "sqlpage_oidc_redirect_count";
49+
const MAX_OIDC_REDIRECTS: u8 = 3;
4750
const AUTH_COOKIE_EXPIRATION: awc::cookie::time::Duration =
4851
actix_web::cookie::time::Duration::days(7);
4952
const LOGIN_FLOW_STATE_COOKIE_EXPIRATION: awc::cookie::time::Duration =
@@ -374,11 +377,24 @@ async fn handle_unauthenticated_request(
374377

375378
async fn handle_oidc_callback(oidc_state: &OidcState, request: ServiceRequest) -> ServiceResponse {
376379
match process_oidc_callback(oidc_state, &request).await {
377-
Ok(response) => request.into_response(response),
380+
Ok(mut response) => {
381+
clear_redirect_count_cookie(&mut response);
382+
request.into_response(response)
383+
}
378384
Err(e) => {
379-
log::error!("Failed to process OIDC callback. Refreshing oidc provider metadata, then redirecting to home page: {e:#}");
385+
let redirect_count = get_redirect_count(&request);
386+
if redirect_count >= MAX_OIDC_REDIRECTS {
387+
log::error!(
388+
"Failed to process OIDC callback after {redirect_count} attempts. \
389+
Stopping to avoid infinite redirections: {e:#}"
390+
);
391+
let resp = build_oidc_error_response(&request, &e);
392+
return request.into_response(resp);
393+
}
394+
log::error!("Failed to process OIDC callback (attempt {redirect_count}). Refreshing oidc provider metadata, then redirecting to home page: {e:#}");
380395
oidc_state.refresh_on_error(&request).await;
381-
let resp = build_auth_provider_redirect_response(oidc_state, "/").await;
396+
let mut resp = build_auth_provider_redirect_response(oidc_state, "/").await;
397+
set_redirect_count_cookie(&mut resp, redirect_count + 1);
382398
request.into_response(resp)
383399
}
384400
}
@@ -500,6 +516,38 @@ fn build_redirect_response(target_url: String) -> HttpResponse {
500516
.body("Redirecting...")
501517
}
502518

519+
fn get_redirect_count(request: &ServiceRequest) -> u8 {
520+
request
521+
.cookie(SQLPAGE_OIDC_REDIRECT_COUNT_COOKIE)
522+
.and_then(|c| c.value().parse().ok())
523+
.unwrap_or(0)
524+
}
525+
526+
fn set_redirect_count_cookie(response: &mut HttpResponse, count: u8) {
527+
let cookie = Cookie::build(SQLPAGE_OIDC_REDIRECT_COUNT_COOKIE, count.to_string())
528+
.path("/")
529+
.http_only(true)
530+
.same_site(actix_web::cookie::SameSite::Lax)
531+
.max_age(LOGIN_FLOW_STATE_COOKIE_EXPIRATION)
532+
.finish();
533+
response.add_cookie(&cookie).ok();
534+
}
535+
536+
fn clear_redirect_count_cookie(response: &mut HttpResponse) {
537+
let cookie = Cookie::build(SQLPAGE_OIDC_REDIRECT_COUNT_COOKIE, "")
538+
.path("/")
539+
.finish()
540+
.into_owned();
541+
response.add_removal_cookie(&cookie).ok();
542+
}
543+
544+
fn build_oidc_error_response(request: &ServiceRequest, e: &anyhow::Error) -> HttpResponse {
545+
request.app_data::<web::Data<AppState>>().map_or_else(
546+
|| HttpResponse::InternalServerError().body(format!("Authentication error: {e}")),
547+
|state| anyhow_err_to_actix_resp(e, state),
548+
)
549+
}
550+
503551
/// Returns the claims from the ID token in the `SQLPage` auth cookie.
504552
async fn get_authenticated_user_info(
505553
oidc_state: &OidcState,

0 commit comments

Comments
 (0)