From d06356816046941450445a7a5c50cdced5301306 Mon Sep 17 00:00:00 2001 From: Trezy Date: Thu, 26 Mar 2026 08:54:04 -0700 Subject: [PATCH] fix: ditch redirect cookies in favor of storing redirect info in the db --- .../20260325000000_text_to_timestamptz.sql | 62 ++---------- ...0326000000_create_auth_login_redirects.sql | 11 +++ ...0326000000_create_auth_login_redirects.sql | 11 +++ src/auth/routes.rs | 96 ++++++++++++++----- 4 files changed, 101 insertions(+), 79 deletions(-) create mode 100644 migrations/postgres/20260326000000_create_auth_login_redirects.sql create mode 100644 migrations/sqlite/20260326000000_create_auth_login_redirects.sql diff --git a/migrations/postgres/20260325000000_text_to_timestamptz.sql b/migrations/postgres/20260325000000_text_to_timestamptz.sql index bad7f5a..1175cb9 100644 --- a/migrations/postgres/20260325000000_text_to_timestamptz.sql +++ b/migrations/postgres/20260325000000_text_to_timestamptz.sql @@ -1,53 +1,9 @@ --- Restore TIMESTAMPTZ columns that were converted to TEXT by uuid_to_text. --- The stored RFC 3339 strings are valid TIMESTAMPTZ literals, so the cast is safe. - --- users -ALTER TABLE users ALTER COLUMN created_at TYPE TIMESTAMPTZ USING created_at::timestamptz; -ALTER TABLE users ALTER COLUMN last_used_at TYPE TIMESTAMPTZ USING last_used_at::timestamptz; - --- user_permissions -ALTER TABLE user_permissions ALTER COLUMN granted_at TYPE TIMESTAMPTZ USING granted_at::timestamptz; - --- api_keys -ALTER TABLE api_keys ALTER COLUMN created_at TYPE TIMESTAMPTZ USING created_at::timestamptz; -ALTER TABLE api_keys ALTER COLUMN last_used_at TYPE TIMESTAMPTZ USING last_used_at::timestamptz; -ALTER TABLE api_keys ALTER COLUMN revoked_at TYPE TIMESTAMPTZ USING revoked_at::timestamptz; - --- event_logs -ALTER TABLE event_logs ALTER COLUMN created_at TYPE TIMESTAMPTZ USING created_at::timestamptz; - --- backfill_jobs -ALTER TABLE backfill_jobs ALTER COLUMN created_at TYPE TIMESTAMPTZ USING created_at::timestamptz; -ALTER TABLE backfill_jobs ALTER COLUMN started_at TYPE TIMESTAMPTZ USING started_at::timestamptz; -ALTER TABLE backfill_jobs ALTER COLUMN completed_at TYPE TIMESTAMPTZ USING completed_at::timestamptz; - --- dead_letter_hooks -ALTER TABLE dead_letter_hooks ALTER COLUMN created_at TYPE TIMESTAMPTZ USING created_at::timestamptz; - --- lexicons -ALTER TABLE lexicons ALTER COLUMN created_at TYPE TIMESTAMPTZ USING created_at::timestamptz; -ALTER TABLE lexicons ALTER COLUMN updated_at TYPE TIMESTAMPTZ USING updated_at::timestamptz; -ALTER TABLE lexicons ALTER COLUMN last_fetched_at TYPE TIMESTAMPTZ USING last_fetched_at::timestamptz; - --- records -ALTER TABLE records ALTER COLUMN created_at TYPE TIMESTAMPTZ USING created_at::timestamptz; -ALTER TABLE records ALTER COLUMN indexed_at TYPE TIMESTAMPTZ USING indexed_at::timestamptz; - --- labels -ALTER TABLE labels ALTER COLUMN cts TYPE TIMESTAMPTZ USING cts::timestamptz; -ALTER TABLE labels ALTER COLUMN exp TYPE TIMESTAMPTZ USING exp::timestamptz; - --- labeler_subscriptions -ALTER TABLE labeler_subscriptions ALTER COLUMN created_at TYPE TIMESTAMPTZ USING created_at::timestamptz; -ALTER TABLE labeler_subscriptions ALTER COLUMN updated_at TYPE TIMESTAMPTZ USING updated_at::timestamptz; - --- rate_limits -ALTER TABLE rate_limits ALTER COLUMN created_at TYPE TIMESTAMPTZ USING created_at::timestamptz; -ALTER TABLE rate_limits ALTER COLUMN updated_at TYPE TIMESTAMPTZ USING updated_at::timestamptz; - --- rate_limit_allowlist -ALTER TABLE rate_limit_allowlist ALTER COLUMN created_at TYPE TIMESTAMPTZ USING created_at::timestamptz; - --- script_variables -ALTER TABLE script_variables ALTER COLUMN created_at TYPE TIMESTAMPTZ USING created_at::timestamptz; -ALTER TABLE script_variables ALTER COLUMN updated_at TYPE TIMESTAMPTZ USING updated_at::timestamptz; +-- No-op migration. +-- +-- sqlx's AnyPool driver does not support native Postgres TIMESTAMPTZ or JSONB +-- types, so all columns must remain TEXT. Lua scripts use explicit casts +-- (e.g. col::timestamptz, col::jsonb) in queries where needed. +-- +-- This migration was originally intended to restore native types but was +-- reverted after discovering the AnyPool limitation. +SELECT 1; diff --git a/migrations/postgres/20260326000000_create_auth_login_redirects.sql b/migrations/postgres/20260326000000_create_auth_login_redirects.sql new file mode 100644 index 0000000..9930b6e --- /dev/null +++ b/migrations/postgres/20260326000000_create_auth_login_redirects.sql @@ -0,0 +1,11 @@ +-- Stores redirect URIs for cross-origin login flows (e.g., Pentaract → HappyView). +-- Keyed by the OAuth state parameter so we can look up the redirect target +-- when the PDS callback arrives, without relying on third-party cookies. +CREATE TABLE auth_login_redirects ( + state TEXT PRIMARY KEY, + redirect_uri TEXT NOT NULL, + created_at TEXT NOT NULL, + expires_at TEXT NOT NULL +); + +CREATE INDEX idx_auth_login_redirects_expires ON auth_login_redirects(expires_at); diff --git a/migrations/sqlite/20260326000000_create_auth_login_redirects.sql b/migrations/sqlite/20260326000000_create_auth_login_redirects.sql new file mode 100644 index 0000000..9930b6e --- /dev/null +++ b/migrations/sqlite/20260326000000_create_auth_login_redirects.sql @@ -0,0 +1,11 @@ +-- Stores redirect URIs for cross-origin login flows (e.g., Pentaract → HappyView). +-- Keyed by the OAuth state parameter so we can look up the redirect target +-- when the PDS callback arrives, without relying on third-party cookies. +CREATE TABLE auth_login_redirects ( + state TEXT PRIMARY KEY, + redirect_uri TEXT NOT NULL, + created_at TEXT NOT NULL, + expires_at TEXT NOT NULL +); + +CREATE INDEX idx_auth_login_redirects_expires ON auth_login_redirects(expires_at); diff --git a/src/auth/routes.rs b/src/auth/routes.rs index 3ade70f..08e5831 100644 --- a/src/auth/routes.rs +++ b/src/auth/routes.rs @@ -6,13 +6,14 @@ use axum::{ }; use axum_extra::extract::cookie::{Cookie, Key, SignedCookieJar}; use serde::Deserialize; - use crate::AppState; use crate::auth::COOKIE_NAME; -use crate::db::adapt_sql; +use crate::db::{adapt_sql, now_rfc3339}; use crate::error::AppError; -const REDIRECT_COOKIE_NAME: &str = "happyview_redirect"; +/// Legacy cookie name from the old cookie-based redirect approach. +/// Detected and removed in the callback to clean up stale cookies. +const LEGACY_REDIRECT_COOKIE: &str = "happyview_redirect"; #[derive(Deserialize)] pub struct LoginQuery { @@ -46,18 +47,35 @@ async fn login( .await .map_err(|e| AppError::Internal(format!("OAuth authorize failed: {e}")))?; - // Store the redirect URI in a cookie if provided - // Must use SameSite=None for cross-origin requests (e.g., Pentaract calling HappyView) - let jar = if let Some(redirect_uri) = query.redirect_uri { - let mut cookie = Cookie::new(REDIRECT_COOKIE_NAME, redirect_uri); - cookie.set_path("/"); - cookie.set_http_only(true); - cookie.set_same_site(axum_extra::extract::cookie::SameSite::None); - cookie.set_secure(true); // Required when SameSite=None - jar.add(cookie) - } else { - jar - }; + // Store the redirect URI in the database, keyed by the OAuth state parameter. + // This avoids third-party cookie issues when Pentaract (cross-origin) calls this endpoint. + if let Some(redirect_uri) = &query.redirect_uri { + // Extract the state param from the authorize URL query string + let oauth_state = url + .split('?') + .nth(1) + .and_then(|qs| { + qs.split('&') + .find_map(|pair| pair.strip_prefix("state=")) + }) + .map(|s| urlencoding::decode(s).unwrap_or_else(|_| s.into()).to_string()); + + if let Some(oauth_state) = oauth_state { + let now = now_rfc3339(); + let expires_at = (chrono::Utc::now() + chrono::Duration::minutes(10)).to_rfc3339(); + let sql = adapt_sql( + "INSERT INTO auth_login_redirects (state, redirect_uri, created_at, expires_at) VALUES (?, ?, ?, ?)", + state.db_backend, + ); + let _ = sqlx::query(&sql) + .bind(&oauth_state) + .bind(redirect_uri) + .bind(&now) + .bind(&expires_at) + .execute(&state.db) + .await; + } + } Ok((jar, Json(serde_json::json!({ "url": url })))) } @@ -67,6 +85,31 @@ async fn callback( jar: SignedCookieJar, Query(query): Query, ) -> Result<(SignedCookieJar, Redirect), AppError> { + // Look up the redirect URI from the database before the OAuth library consumes the state + let redirect_url = if let Some(oauth_state) = &query.state { + let sql = adapt_sql( + "SELECT redirect_uri FROM auth_login_redirects WHERE state = ? AND expires_at > ?", + state.db_backend, + ); + let now = now_rfc3339(); + let row: Option<(String,)> = sqlx::query_as(&sql) + .bind(oauth_state) + .bind(&now) + .fetch_optional(&state.db) + .await + .unwrap_or(None); + + // Clean up the row (one-time use) + if row.is_some() { + let delete_sql = adapt_sql("DELETE FROM auth_login_redirects WHERE state = ?", state.db_backend); + let _ = sqlx::query(&delete_sql).bind(oauth_state).execute(&state.db).await; + } + + row.map(|(uri,)| uri) + } else { + None + }; + let params = atrium_oauth::CallbackParams { code: query.code, state: query.state, @@ -85,11 +128,8 @@ async fn callback( .await .ok_or_else(|| AppError::Internal("no DID in OAuth session".into()))?; - // Get the redirect URL from cookie, defaulting to "/" - let redirect_url = jar - .get(REDIRECT_COOKIE_NAME) - .map(|c| c.value().to_string()) - .unwrap_or_else(|| "/".to_string()); + // Use DB-stored redirect, or default to "/" + let redirect_url = redirect_url.unwrap_or_else(|| "/".to_string()); // Set the session cookie // Must use SameSite=None for cross-origin requests (e.g., Pentaract calling HappyView) @@ -99,13 +139,17 @@ async fn callback( session_cookie.set_same_site(axum_extra::extract::cookie::SameSite::None); session_cookie.set_secure(true); // Required when SameSite=None - // Remove the redirect cookie (must match attributes from login) - let mut redirect_removal = Cookie::from(REDIRECT_COOKIE_NAME); - redirect_removal.set_path("/"); - redirect_removal.set_same_site(axum_extra::extract::cookie::SameSite::None); - redirect_removal.set_secure(true); + // Remove the legacy redirect cookie if present (old cookie-based approach) + let jar = if jar.get(LEGACY_REDIRECT_COOKIE).is_some() { + let mut removal = Cookie::from(LEGACY_REDIRECT_COOKIE); + removal.set_path("/"); + removal.set_same_site(axum_extra::extract::cookie::SameSite::None); + removal.set_secure(true); + jar.add(session_cookie).remove(removal) + } else { + jar.add(session_cookie) + }; - let jar = jar.add(session_cookie).remove(redirect_removal); Ok((jar, Redirect::to(&redirect_url))) } -- 2.51.2