From 5a99e718c22a80bb36be7566fee937781d15676e Mon Sep 17 00:00:00 2001 From: Anthony Ronning <101225832+AnthonyRonning@users.noreply.github.com> Date: Sun, 20 Sep 2026 03:33:06 +0000 Subject: [PATCH 1/2] feat(opensecret): support per-project OAuth callback allowlists --- .agents/skills/validate-opensecret/SKILL.md | 12 +- .../scripts/disposable_db_tests.sh | 26 +- .github/workflows/sdk-integration.yml | 8 + scripts/ci/opensecret_change_detection.py | 1 + .../ci/test_opensecret_change_detection.py | 7 + services/opensecret/README.md | 7 +- services/opensecret/docs/oauth-callbacks.md | 88 +++++ services/opensecret/src/db.rs | 295 ++++++++++++++++- services/opensecret/src/main.rs | 24 -- services/opensecret/src/migrations.rs | 2 + .../opensecret/src/models/project_settings.rs | 145 +++++++++ services/opensecret/src/oauth.rs | 121 +++++++ .../opensecret/src/transport_v2/gateway.rs | 2 + .../src/transport_v2/oauth_callback_tests.rs | 307 ++++++++++++++++++ services/opensecret/src/web/oauth_routes.rs | 265 +++++++++++++-- .../opensecret/src/web/platform/common.rs | 146 +++++++-- .../src/web/platform/project_routes.rs | 3 +- 17 files changed, 1359 insertions(+), 100 deletions(-) create mode 100644 services/opensecret/docs/oauth-callbacks.md create mode 100644 services/opensecret/src/transport_v2/oauth_callback_tests.rs diff --git a/.agents/skills/validate-opensecret/SKILL.md b/.agents/skills/validate-opensecret/SKILL.md index a0c4afe36..6abba1f0a 100644 --- a/.agents/skills/validate-opensecret/SKILL.md +++ b/.agents/skills/validate-opensecret/SKILL.md @@ -62,8 +62,8 @@ OPENSECRET_DEV_POSTGRES=0 OPENSECRET_DEV_ENV=0 OPENSECRET_DEV_CONTAINERS=0 \ cargo test --locked --all-features ``` -Report passed, failed, and ignored counts. Default CI has no PostgreSQL service -and does not execute ignored tests. Do not substitute an aggregate recipe +Report passed, failed, and ignored counts. The Rust unit-test CI job has no +PostgreSQL service and does not execute ignored tests. Do not substitute an aggregate recipe unless its checked-in definition preserves the same targets, features, lockfile, and warning policy. @@ -93,8 +93,12 @@ OPENSECRET_DEV_POSTGRES=0 OPENSECRET_DEV_ENV=0 OPENSECRET_DEV_CONTAINERS=0 \ ``` The helper proves an empty-database migration and the selected local synthetic -database suites. It does not prove an OAuth provider flow, encrypted client -transport, or a data conversion from representative old rows. +database suites, including OAuth settings preservation and callback selection +through the real V1 encryption middleware and V2 gateway. The SDK integration +workflow runs this helper too. The callback fixtures use synthetic local +credentials and perform no provider exchanges; they do not prove a live OAuth +provider flow, a released SDK or application, or a data conversion from +representative old rows. For a data migration, separately build an upgrade-shaped disposable database with representative pre-change rows and verify restart, rollback, and retry diff --git a/.agents/skills/validate-opensecret/scripts/disposable_db_tests.sh b/.agents/skills/validate-opensecret/scripts/disposable_db_tests.sh index 9f357c9c0..75f72987e 100755 --- a/.agents/skills/validate-opensecret/scripts/disposable_db_tests.sh +++ b/.agents/skills/validate-opensecret/scripts/disposable_db_tests.sh @@ -95,8 +95,8 @@ cleanup() { rm -rf -- "$workdir" if [ "$status" -eq 0 ] && [ "$tests_passed" -eq 1 ]; then - printf 'Disposable-DB evidence: %s AEAD/database tests and %s OAuth database tests passed; no tests skipped; temporary cluster removed.\n' \ - "$aead_count" "$oauth_count" + printf 'Disposable-DB evidence: %s AEAD/database tests, %s OAuth database tests, and %s callback settings/transport tests passed; no tests skipped; temporary cluster removed.\n' \ + "$aead_count" "$oauth_count" "$callback_count" fi exit "$status" } @@ -200,6 +200,28 @@ fi grep -Eq "test result: ok\\. ${oauth_count} passed; 0 failed; 0 ignored;" \ "$workdir/oauth-tests.log" +callback_count=0 +for callback_filter in \ + db::tests::db_oauth_settings_ \ + transport_v2::gateway::tests::db_oauth_callback_selection_v1_v2; do + cargo test --locked --all-features "$callback_filter" \ + -- --ignored --list >"$workdir/callback-tests.list" + selected_count="$(awk -v prefix="$callback_filter" \ + 'index($0, prefix) == 1 && /: test$/ { count++ } + END { print count + 0 }' "$workdir/callback-tests.list")" + test "$selected_count" -gt 0 + + cargo test --locked --all-features "$callback_filter" \ + -- --ignored --test-threads=1 --nocapture 2>&1 | tee "$workdir/callback-tests.log" + if grep -qi 'skipping:' "$workdir/callback-tests.log"; then + printf 'OAuth callback test output contained a skip marker\n' >&2 + exit 1 + fi + grep -Eq "test result: ok\\. ${selected_count} passed; 0 failed; 0 ignored;" \ + "$workdir/callback-tests.log" + callback_count=$((callback_count + selected_count)) +done + assert_database_identity assert_migration_count tests_passed=1 diff --git a/.github/workflows/sdk-integration.yml b/.github/workflows/sdk-integration.yml index 05cb77c58..4992bc0e0 100644 --- a/.github/workflows/sdk-integration.yml +++ b/.github/workflows/sdk-integration.yml @@ -87,6 +87,14 @@ jobs: psql "${DATABASE_URL}" -X -v ON_ERROR_STOP=1 \ -f sdk/test/integration/bootstrap.sql + - name: Verify OAuth persistence and encrypted callback selection + run: | + nix develop --no-update-lock-file './services/opensecret?submodules=1' -c bash -lc ' + set -euo pipefail + cd services/opensecret + bash ../../.agents/skills/validate-opensecret/scripts/disposable_db_tests.sh + ' + - name: Start and verify OpenSecret run: | nix develop --no-update-lock-file './services/opensecret?submodules=1' -c \ diff --git a/scripts/ci/opensecret_change_detection.py b/scripts/ci/opensecret_change_detection.py index 56bd689f4..71440e699 100644 --- a/scripts/ci/opensecret_change_detection.py +++ b/scripts/ci/opensecret_change_detection.py @@ -38,6 +38,7 @@ "sdk/.npmrc", "sdk/bun.lock", "sdk/bunfig.toml", "sdk/package.json", "sdk/rust-toolchain.toml", "sdk/flake.nix", "sdk/flake.lock", ".github/workflows/sdk-integration.yml", + ".agents/skills/validate-opensecret/scripts/disposable_db_tests.sh", }) KNOWN_INDEPENDENT_PREFIXES = ( "apps/", "proxy/", "services/updates/", ".agents/", ".github/", diff --git a/scripts/ci/test_opensecret_change_detection.py b/scripts/ci/test_opensecret_change_detection.py index 3d76f20ef..9f5367002 100644 --- a/scripts/ci/test_opensecret_change_detection.py +++ b/scripts/ci/test_opensecret_change_detection.py @@ -81,6 +81,13 @@ def test_independent_components_and_docs_skip_backend_checks(self): with self.subTest(path=path): self.assert_routes([path]) + def test_disposable_database_harness_selects_integration_without_app_packaging(self): + path = ".agents/skills/validate-opensecret/scripts/disposable_db_tests.sh" + self.assert_routes([path], "integration") + self.assertEqual(research_routes(path), frozenset()) + self.assertFalse(affects_agent(path)) + self.assert_routes([".agents/skills/validate-opensecret/SKILL.md"]) + def test_submodules_and_selector_changes_select_all_backend_checks(self): for path in (".gitmodules", ".github/workflows/opensecret-change-detection.yml", "scripts/ci/opensecret_change_detection.py"): diff --git a/services/opensecret/README.md b/services/opensecret/README.md index df8714716..91407951e 100644 --- a/services/opensecret/README.md +++ b/services/opensecret/README.md @@ -55,6 +55,9 @@ OpenAI-compatible wire endpoint. Use an OpenSecret SDK or Maple for protected-route integration tests; plain `curl` is suitable only for public health probes. +For per-project callback allowlists, optional callback selection, and +old-client compatibility, see [OAuth callback selection](docs/oauth-callbacks.md). + `GET /health-check` returns HTTP 200 with `{"status":"pass","version":"v1"}` when the server can respond. It does not contact Tinfoil, other providers, or PostgreSQL; provider outages must not remove responsive enclaves from @@ -83,7 +86,9 @@ OPENSECRET_DEV_POSTGRES=0 OPENSECRET_DEV_ENV=0 OPENSECRET_DEV_CONTAINERS=0 \ cargo test --locked --all-features ``` -Default CI does not run ignored database or live-provider tests. Use the +The Rust unit-test job does not run ignored tests. The SDK integration job +also runs the disposable-database helper, including OAuth settings and +encrypted V1/V2 callback-selection checks. Live-provider tests remain separate. Use the [`validate-opensecret`](../../.agents/skills/validate-opensecret/SKILL.md) workflow for disposable PostgreSQL tests, authorized provider checks, encrypted-client smoke tests, Nix checks, and release-only EIF/PCR evidence. Report those layers diff --git a/services/opensecret/docs/oauth-callbacks.md b/services/opensecret/docs/oauth-callbacks.md new file mode 100644 index 000000000..7020a6117 --- /dev/null +++ b/services/opensecret/docs/oauth-callbacks.md @@ -0,0 +1,88 @@ +# OAuth callback selection + +OAuth callbacks are configured per project and provider. This additive API +contract lets browser clients on different origins finish sign-in on the +origin that started it. It does not change the default callback or require +existing clients to send a new field. + +## Provider settings + +The encrypted platform settings API at +`/platform/orgs/:org_id/projects/:project_id/settings/oauth` retains its +existing GET and PUT methods and organization/project authorization. Updating +settings still requires an organization owner or administrator. + +Each Google, GitHub, or Apple settings object keeps `redirect_url` and accepts +an optional `additional_redirect_urls` array. For example, a Google settings +object can contain: + +```json +{ + "client_id": "example-provider-client-id", + "redirect_url": "https://app.example.com/auth/google/callback", + "additional_redirect_urls": [ + "https://auth.example.com/auth/google/callback" + ] +} +``` + +This is a nested provider object, not a complete PUT request. The surrounding +enabled flags and other provider settings retain their existing semantics. + +- At most 16 additional URLs are accepted per provider. Each must satisfy + the existing generic callback URL validation, including its length bound. + The platform does not restrict this list to one application's hostnames. +- On PUT, omitting the new field or sending `null` preserves that provider's + stored additional list. An explicit array replaces it; `[]` clears it. +- This preservation applies when the provider settings object is supplied. + Omitting or clearing the entire provider object retains the existing + whole-object PUT behavior; it is not a patch API for other fields. +- GET and the PUT response include a stored list. Existing rows without the + field remain readable; no SQL schema migration is required. +- URL-list preservation and the settings write are serialized per project, + so an older writer that omits the field cannot overwrite a concurrently + committed list with an earlier snapshot. + +## Initiation and completion + +The decrypted request body for `/auth/github`, `/auth/google`, and +`/auth/apple` accepts an optional `redirect_url` alongside the existing +`client_id`. The same contract applies through both +Transport V1 and Transport V2. + +When absent or `null`, the provider's default `redirect_url` is used. When +present, the value must exactly match the default or one of that provider's +additional entries for the requested project. There is no wildcard, prefix, +or same-host matching. A rejected selection returns the existing bad-request +error before allocating OAuth state. + +The chosen callback is recorded in the server-validated, one-use OAuth state. +The provider authorization request and token exchange use that same callback, +including Apple's token exchange. Changing the default or additional list +does not retarget an already-started flow. Removing a list entry stops new +flows from selecting it; it does not revoke a pending flow. Existing state +expiry, one-use checks, provider checks, and V2 session/PKCE/nonce bindings +continue to apply. + +Clients must treat the returned `state` as opaque and return it unchanged. +The callback request does not accept a separate redirect override. Changing +the callback inside the returned state cannot change the server's stored +selection. + +## Compatibility and adoption + +Deploy backend support before a client selects a non-default callback. +Register each callback with its OAuth provider as well as in the project's +backend settings; these are separate requirements. Existing clients that +omit the request field continue to use the default. + +An older backend ignores the new configuration and request fields and keeps +using its default callback. Therefore a client that depends on a non-default +callback must not remain active during a backend downgrade. Older backend +settings writes may also discard the additional list; preserve configuration +outside the downgraded writer and verify it before re-enabling consumers. +OAuth state is process-local, so restarting or replacing a backend can +invalidate pending sign-ins; the user must start a new attempt. + +SDK publication, consumer upgrades, provider registration, and traffic changes +are separate from implementing this backend contract. diff --git a/services/opensecret/src/db.rs b/services/opensecret/src/db.rs index cd9a1feb2..7c8a0ce87 100644 --- a/services/opensecret/src/db.rs +++ b/services/opensecret/src/db.rs @@ -1872,22 +1872,38 @@ impl DBConnection for PostgresConnection { fn update_project_oauth_settings( &self, project_id: i32, - settings: OAuthSettings, + mut settings: OAuthSettings, ) -> Result { - let new_settings = NewProjectSetting::new_oauth_settings(project_id, settings)?; let conn = &mut self.db.get().map_err(|_| DBError::ConnectionError)?; - // Check if settings exist - if let Some(mut existing) = - ProjectSetting::get_by_project_and_category(conn, project_id, SettingCategory::OAuth)? - { - existing.settings = new_settings.settings; - existing.update(conn)?; - Ok(existing) - } else { - // Create new settings - new_settings.insert(conn).map_err(DBError::from) - } + conn.transaction::<_, DBError, _>(|conn| { + use crate::models::schema::org_projects; + + // Lock the owning project even before its first settings row exists. + // An older client omitting the additive lists must merge against the + // latest committed settings, not overwrite a concurrent update. + org_projects::table + .filter(org_projects::id.eq(project_id)) + .select(org_projects::id) + .for_update() + .first::(conn)?; + + if let Some(mut existing) = ProjectSetting::get_by_project_and_category( + conn, + project_id, + SettingCategory::OAuth, + )? { + settings.preserve_omitted_redirect_urls(&existing.get_oauth_settings()?); + existing.settings = + NewProjectSetting::new_oauth_settings(project_id, settings)?.settings; + existing.update(conn)?; + Ok(existing) + } else { + NewProjectSetting::new_oauth_settings(project_id, settings)? + .insert(conn) + .map_err(DBError::from) + } + }) } // Platform email verification implementations @@ -3003,3 +3019,256 @@ pub(crate) fn setup_db(url: String) -> Arc { info!("Connected to database with pool size: 20, min idle: 5"); Arc::new(PostgresConnection { db: pool }) } + +#[cfg(test)] +mod tests { + use super::*; + use crate::models::project_settings::{AppleOAuthSettings, OAuthProviderSettings}; + use std::{ + thread, + time::{Duration, Instant}, + }; + + struct OAuthSettingsFixture { + database_url: String, + database: Arc, + project_id: i32, + org: Org, + } + + impl OAuthSettingsFixture { + fn new() -> Self { + let database_url = std::env::var("AEAD_TAMPER_TEST_DATABASE_URL") + .expect("requires an explicitly selected disposable migrated local database"); + let parsed = url::Url::parse(&database_url).expect("database URL must parse"); + assert!(matches!( + parsed.host_str(), + Some("127.0.0.1" | "localhost" | "[::1]") + )); + let pool = Pool::builder() + .max_size(1) + .min_idle(Some(0)) + .connection_timeout(Duration::from_secs(10)) + .build(ConnectionManager::::new(database_url.clone())) + .expect("disposable database must be available"); + let database = Arc::new(PostgresConnection { db: pool }); + let (org, project_id) = { + let conn = &mut database.db.get().unwrap(); + let org = NewOrg::new(format!("oauth-settings-test-{}", Uuid::new_v4())) + .insert(conn) + .unwrap(); + let project = NewOrgProject::new(org.id, "OAuth settings test".to_string()) + .insert(conn) + .unwrap(); + (org, project.id) + }; + Self { + database_url, + database, + project_id, + org, + } + } + + fn stored(&self) -> OAuthSettings { + self.database + .get_project_oauth_settings(self.project_id) + .unwrap() + .unwrap() + } + } + + impl Drop for OAuthSettingsFixture { + fn drop(&mut self) { + if let Ok(mut conn) = self.database.db.get() { + let _ = self.org.delete(&mut conn); + } + } + } + + fn oauth_settings(additions: Option>) -> OAuthSettings { + let provider = OAuthProviderSettings { + client_id: "test-client".to_string(), + redirect_url: "https://customer.example/callback".to_string(), + additional_redirect_urls: additions.clone(), + }; + OAuthSettings { + google_oauth_enabled: true, + github_oauth_enabled: true, + apple_oauth_enabled: true, + google_oauth_settings: Some(provider.clone()), + github_oauth_settings: Some(provider), + apple_oauth_settings: Some(AppleOAuthSettings { + client_id: "test-apple-client".to_string(), + redirect_url: "https://customer.example/apple/callback".to_string(), + additional_redirect_urls: additions, + team_id: Some("ABCDEFGHIJ".to_string()), + key_id: Some("1234567890".to_string()), + }), + } + } + + fn assert_additions(settings: &OAuthSettings, expected: &[&str]) { + let expected: Vec = expected.iter().map(|value| (*value).to_string()).collect(); + for actual in [ + &settings + .google_oauth_settings + .as_ref() + .unwrap() + .additional_redirect_urls, + &settings + .github_oauth_settings + .as_ref() + .unwrap() + .additional_redirect_urls, + &settings + .apple_oauth_settings + .as_ref() + .unwrap() + .additional_redirect_urls, + ] { + assert_eq!(actual.as_ref(), Some(&expected)); + } + } + + #[test] + #[ignore = "requires AEAD_TAMPER_TEST_DATABASE_URL pointing at disposable migrated local Postgres"] + fn db_oauth_settings_insert_preserve_replace_and_clear() { + let fixture = OAuthSettingsFixture::new(); + assert!(fixture + .database + .get_project_oauth_settings(fixture.project_id) + .unwrap() + .is_none()); + let legacy = oauth_settings(None); + fixture + .database + .update_project_oauth_settings(fixture.project_id, legacy.clone()) + .unwrap(); + assert_eq!( + serde_json::to_value(fixture.stored()).unwrap(), + serde_json::to_value(legacy).unwrap() + ); + + fixture + .database + .update_project_oauth_settings( + fixture.project_id, + oauth_settings(Some(vec!["https://first.example/callback".to_string()])), + ) + .unwrap(); + for null in [false, true] { + let mut update = serde_json::to_value(oauth_settings(None)).unwrap(); + if null { + for provider in ["google", "github", "apple"] { + update[format!("{provider}_oauth_settings")]["additional_redirect_urls"] = + serde_json::Value::Null; + } + } + update["google_oauth_settings"]["redirect_url"] = + serde_json::json!("https://new-default.example/callback"); + let response = fixture + .database + .update_project_oauth_settings( + fixture.project_id, + serde_json::from_value(update).unwrap(), + ) + .unwrap(); + let stored = fixture.stored(); + assert_additions(&stored, &["https://first.example/callback"]); + assert_eq!( + stored.google_oauth_settings.as_ref().unwrap().redirect_url, + "https://new-default.example/callback" + ); + assert_eq!(response.settings, serde_json::to_value(stored).unwrap()); + } + for replacement in [vec!["https://replacement.example/callback"], vec![]] { + fixture + .database + .update_project_oauth_settings( + fixture.project_id, + oauth_settings(Some( + replacement + .iter() + .map(|value| (*value).to_string()) + .collect(), + )), + ) + .unwrap(); + assert_additions(&fixture.stored(), &replacement); + fixture + .database + .update_project_oauth_settings(fixture.project_id, oauth_settings(None)) + .unwrap(); + assert_additions(&fixture.stored(), &replacement); + } + } + + #[derive(diesel::QueryableByName)] + struct BackendPid { + #[diesel(sql_type = diesel::sql_types::Integer)] + pid: i32, + } + + #[derive(diesel::QueryableByName)] + struct WaitingForLock { + #[diesel(sql_type = diesel::sql_types::Bool)] + waiting: bool, + } + + #[test] + #[ignore = "requires AEAD_TAMPER_TEST_DATABASE_URL pointing at disposable migrated local Postgres"] + fn db_oauth_settings_omission_waits_for_latest_committed_additions() { + for initially_present in [false, true] { + let fixture = OAuthSettingsFixture::new(); + if initially_present { + fixture + .database + .update_project_oauth_settings( + fixture.project_id, + oauth_settings(Some(vec!["https://old.example/callback".to_string()])), + ) + .unwrap(); + } + let worker_pid = diesel::sql_query("SELECT pg_backend_pid() AS pid") + .get_result::(&mut fixture.database.db.get().unwrap()) + .unwrap() + .pid; + let mut writer = PgConnection::establish(&fixture.database_url).unwrap(); + let mut observer = PgConnection::establish(&fixture.database_url).unwrap(); + let worker = writer.transaction::<_, diesel::result::Error, _>(|conn| { + use crate::models::schema::org_projects; + org_projects::table.filter(org_projects::id.eq(fixture.project_id)) + .select(org_projects::id).for_update().first::(conn)?; + let updated = NewProjectSetting::new_oauth_settings( + fixture.project_id, + oauth_settings(Some(vec!["https://latest.example/callback".to_string()])), + ).unwrap(); + if let Some(mut existing) = ProjectSetting::get_by_project_and_category( + conn, fixture.project_id, SettingCategory::OAuth, + ).unwrap() { + existing.settings = updated.settings; + existing.update(conn).unwrap(); + } else { + updated.insert(conn).unwrap(); + } + + let database = fixture.database.clone(); + let project_id = fixture.project_id; + let worker = thread::spawn(move || database.update_project_oauth_settings(project_id, oauth_settings(None))); + let deadline = Instant::now() + Duration::from_secs(5); + loop { + let waiting = diesel::sql_query("SELECT COALESCE((SELECT wait_event_type = 'Lock' FROM pg_stat_activity WHERE pid = $1), false) AS waiting") + .bind::(worker_pid) + .get_result::(&mut observer).unwrap().waiting; + if waiting { break; } + assert!(Instant::now() < deadline, "older settings writer must wait for the uncommitted update"); + thread::sleep(Duration::from_millis(10)); + } + Ok(worker) + }).unwrap(); + worker.join().unwrap().unwrap(); + assert_additions(&fixture.stored(), &["https://latest.example/callback"]); + } + } +} diff --git a/services/opensecret/src/main.rs b/services/opensecret/src/main.rs index 70cc579ca..42f6295ae 100644 --- a/services/opensecret/src/main.rs +++ b/services/opensecret/src/main.rs @@ -87,7 +87,6 @@ use tokio::task::{self}; use tower_http::cors::{Any, CorsLayer}; use tracing::{debug, error, info, trace, warn}; use tracing_subscriber::{layer::SubscriberExt, util::SubscriberInitExt, EnvFilter}; -use url::Url; use uuid::Uuid; use vsock::{VsockAddr, VsockStream}; use web::attestation_routes; @@ -739,18 +738,6 @@ pub enum AppMode { Custom(String), } -impl AppMode { - fn frontend_url(&self) -> &str { - match self { - AppMode::Local => "http://127.0.0.1:5173", - AppMode::Dev => "https://dev.secretgpt.ai", - AppMode::Preview => "https://preview.opensecret.cloud", - AppMode::Prod => "https://trymaple.ai", - AppMode::Custom(_) => "https://preview.opensecret.cloud", - } - } -} - impl fmt::Display for AppMode { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { @@ -2781,17 +2768,6 @@ impl AppState { .map_err(Error::from) } - pub fn frontend_url(&self) -> String { - self.app_mode.frontend_url().to_string() - } - - pub fn oauth_callback_url(&self, provider: &str) -> Result { - let base_url = Url::parse(self.frontend_url().as_str())?; - Ok(base_url - .join(&format!("/auth/{}/callback", provider))? - .to_string()) - } - pub async fn create_account_deletion_request( &self, user_id: Uuid, diff --git a/services/opensecret/src/migrations.rs b/services/opensecret/src/migrations.rs index 835c6f439..9a8cda842 100644 --- a/services/opensecret/src/migrations.rs +++ b/services/opensecret/src/migrations.rs @@ -130,10 +130,12 @@ async fn perform_maple_settings_migration( google_oauth_settings: google_client_id.map(|client_id| OAuthProviderSettings { client_id, redirect_url: format!("{}/auth/google/callback", oauth_base_url), + additional_redirect_urls: None, }), github_oauth_settings: github_client_id.map(|client_id| OAuthProviderSettings { client_id, redirect_url: format!("{}/auth/github/callback", oauth_base_url), + additional_redirect_urls: None, }), apple_oauth_settings: None, }; diff --git a/services/opensecret/src/models/project_settings.rs b/services/opensecret/src/models/project_settings.rs index cb8a7a432..abd4408c3 100644 --- a/services/opensecret/src/models/project_settings.rs +++ b/services/opensecret/src/models/project_settings.rs @@ -52,12 +52,18 @@ impl Default for EmailSettings { pub struct OAuthProviderSettings { pub client_id: String, pub redirect_url: String, + /// Missing or null on an update preserves the stored list; an empty list clears it. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub additional_redirect_urls: Option>, } #[derive(Debug, Clone, Serialize, Deserialize, Default)] pub struct AppleOAuthSettings { pub client_id: String, pub redirect_url: String, + /// Missing or null on an update preserves the stored list; an empty list clears it. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub additional_redirect_urls: Option>, pub team_id: Option, // Apple Developer Team ID (10 chars) pub key_id: Option, // Apple Private Key ID (10 chars) } @@ -74,6 +80,41 @@ pub struct OAuthSettings { pub apple_oauth_settings: Option, } +impl OAuthSettings { + /// Preserve only the additive fields older settings clients cannot send. + /// Other fields retain the existing whole-object replacement semantics. + pub(crate) fn preserve_omitted_redirect_urls(&mut self, existing: &Self) { + for (incoming, stored) in [ + ( + &mut self.google_oauth_settings, + &existing.google_oauth_settings, + ), + ( + &mut self.github_oauth_settings, + &existing.github_oauth_settings, + ), + ] { + if let (Some(incoming), Some(stored)) = (incoming, stored) { + if incoming.additional_redirect_urls.is_none() { + incoming + .additional_redirect_urls + .clone_from(&stored.additional_redirect_urls); + } + } + } + if let (Some(incoming), Some(stored)) = ( + &mut self.apple_oauth_settings, + &existing.apple_oauth_settings, + ) { + if incoming.additional_redirect_urls.is_none() { + incoming + .additional_redirect_urls + .clone_from(&stored.additional_redirect_urls); + } + } + } +} + #[derive(Queryable, Identifiable)] #[diesel(table_name = project_settings)] pub struct ProjectSetting { @@ -166,3 +207,107 @@ impl NewProjectSetting { .map_err(ProjectSettingError::DatabaseError) } } + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::json; + + fn legacy_settings() -> Value { + json!({ + "google_oauth_enabled": true, + "github_oauth_enabled": true, + "apple_oauth_enabled": true, + "google_oauth_settings": { + "client_id": "google-client", + "redirect_url": "https://customer.example/google" + }, + "github_oauth_settings": { + "client_id": "github-client", + "redirect_url": "http://127.0.0.1:5173/github" + }, + "apple_oauth_settings": { + "client_id": "apple-client", + "redirect_url": "https://customer.example/apple", + "team_id": "ABCDEFGHIJ", + "key_id": "1234567890" + } + }) + } + + #[test] + fn legacy_oauth_settings_round_trip_without_additional_fields() { + let legacy = legacy_settings(); + let settings: OAuthSettings = serde_json::from_value(legacy.clone()).unwrap(); + assert_eq!(serde_json::to_value(settings).unwrap(), legacy); + } + + #[test] + fn omitted_and_null_lists_preserve_only_existing_provider_additions() { + let mut current_json = legacy_settings(); + for provider in ["google", "github", "apple"] { + current_json[format!("{provider}_oauth_settings")]["additional_redirect_urls"] = + json!([format!("https://auth.customer.example/{provider}")]); + } + let current: OAuthSettings = serde_json::from_value(current_json).unwrap(); + for null in [false, true] { + let mut update_json = legacy_settings(); + if null { + for provider in ["google", "github", "apple"] { + update_json[format!("{provider}_oauth_settings")]["additional_redirect_urls"] = + Value::Null; + } + } + update_json["google_oauth_settings"]["redirect_url"] = + json!("https://new.customer.example/google"); + update_json["github_oauth_enabled"] = json!(false); + update_json["github_oauth_settings"] = Value::Null; + let mut update: OAuthSettings = serde_json::from_value(update_json).unwrap(); + update.preserve_omitted_redirect_urls(¤t); + + let google = update.google_oauth_settings.unwrap(); + assert_eq!(google.redirect_url, "https://new.customer.example/google"); + assert_eq!( + google.additional_redirect_urls, + current + .google_oauth_settings + .as_ref() + .unwrap() + .additional_redirect_urls + ); + assert_eq!( + update + .apple_oauth_settings + .unwrap() + .additional_redirect_urls, + current + .apple_oauth_settings + .as_ref() + .unwrap() + .additional_redirect_urls + ); + assert!(!update.github_oauth_enabled); + assert!(update.github_oauth_settings.is_none()); + } + } + + #[test] + fn explicit_lists_replace_and_empty_lists_clear() { + let mut current_json = legacy_settings(); + for provider in ["google", "github", "apple"] { + current_json[format!("{provider}_oauth_settings")]["additional_redirect_urls"] = + json!(["https://old.customer.example/callback"]); + } + let current: OAuthSettings = serde_json::from_value(current_json).unwrap(); + for replacement in [json!([]), json!(["https://new.customer.example/callback"])] { + let mut update_json = legacy_settings(); + for provider in ["google", "github", "apple"] { + update_json[format!("{provider}_oauth_settings")]["additional_redirect_urls"] = + replacement.clone(); + } + let mut update: OAuthSettings = serde_json::from_value(update_json.clone()).unwrap(); + update.preserve_omitted_redirect_urls(¤t); + assert_eq!(serde_json::to_value(update).unwrap(), update_json); + } + } +} diff --git a/services/opensecret/src/oauth.rs b/services/opensecret/src/oauth.rs index f76b031b6..69320e299 100644 --- a/services/opensecret/src/oauth.rs +++ b/services/opensecret/src/oauth.rs @@ -33,6 +33,9 @@ pub type BasicClient = pub struct OAuthState { pub csrf_token: String, pub client_id: Uuid, + // Snapshot the selected callback, including when initiation used the default. + // Complete-state equality protects this value before callback code uses it. + pub redirect_url: String, } #[derive(Debug, Clone)] @@ -836,6 +839,7 @@ mod tests { OAuthState { csrf_token: csrf_token.to_string(), client_id: Uuid::from_u128(client_id), + redirect_url: "https://app.example/auth/callback".to_string(), } } @@ -967,6 +971,60 @@ mod tests { assert!(store.consume_at(&valid, now).await); } + #[test] + fn oauth_state_round_trip_requires_the_selected_redirect() { + let original = state("state-round-trip", 1); + let encoded = serde_json::to_value(&original).unwrap(); + assert_eq!( + serde_json::from_value::(encoded.clone()).unwrap(), + original + ); + let mut missing_redirect = encoded; + missing_redirect + .as_object_mut() + .unwrap() + .remove("redirect_url"); + assert!(serde_json::from_value::(missing_redirect).is_err()); + } + + #[tokio::test] + async fn changed_redirect_is_rejected_without_consuming_v1_or_v2_state() { + for session_id in [None, Some(SessionId::from_bytes([42; 16]))] { + let store = OAuthStateStore::with_limits(Duration::from_secs(60), 2); + let now = Instant::now(); + let valid = state("redirect-bound-state", 1); + let binding = match session_id { + Some(session_id) => OAuthStateBinding::TransportV2 { + session_id, + code_binding: new_pkce_binding().1, + }, + None => OAuthStateBinding::LegacyV1, + }; + store + .store_with_binding(&valid.csrf_token, valid.clone(), binding, now) + .await + .unwrap(); + + for changed_redirect in ["https://auth.example/auth/callback", ""] { + let mut changed = valid.clone(); + changed.redirect_url = changed_redirect.to_string(); + assert!(store + .take_with_binding(&changed, session_id, now) + .await + .is_none()); + } + let consumed = store + .take_with_binding(&valid, session_id, now) + .await + .unwrap(); + assert_eq!(consumed.state.redirect_url, valid.redirect_url); + assert!(store + .take_with_binding(&valid, session_id, now) + .await + .is_none()); + } + } + #[tokio::test] async fn oauth_state_protocol_and_v2_session_binding_are_non_consuming_on_mismatch() { let store = OAuthStateStore::with_limits(Duration::from_secs(60), 2); @@ -1045,9 +1103,20 @@ mod tests { .collect(); assert!(!legacy_parameters.contains_key("code_challenge")); assert!(!legacy_parameters.contains_key("code_challenge_method")); + assert_eq!( + legacy_parameters.get("redirect_uri").map(String::as_str), + Some("https://maple.example/callback") + ); let bound = github.generate_bound_authorize_url(&github_client).await; assert_pkce_authorization(&bound); + assert!( + url::Url::parse(&bound.auth_url) + .unwrap() + .query_pairs() + .any(|(key, value)| key == "redirect_uri" + && value == "https://maple.example/callback") + ); let google = GoogleProvider { auth_url: "https://google.example/authorize".to_string(), @@ -1065,6 +1134,16 @@ mod tests { .unwrap(); let bound = google.generate_bound_authorize_url(&google_client).await; assert_pkce_authorization(&bound); + for auth_url in [ + google.generate_authorize_url(&google_client).await.0, + bound.auth_url, + ] { + assert!(url::Url::parse(&auth_url) + .unwrap() + .query_pairs() + .any(|(key, value)| key == "redirect_uri" + && value == "https://maple.example/callback")); + } let apple = AppleProvider { auth_url: "https://apple.example/authorize".to_string(), @@ -1087,6 +1166,10 @@ mod tests { .into_owned() .collect(); assert!(!legacy_parameters.contains_key("nonce")); + assert_eq!( + legacy_parameters.get("redirect_uri").map(String::as_str), + Some("https://maple.example/callback") + ); let bound = apple.generate_bound_authorize_url(&apple_client).await; let parameters: HashMap<_, _> = url::Url::parse(&bound.auth_url) @@ -1102,6 +1185,44 @@ mod tests { parameters.get("nonce").map(String::as_str), Some(expected_nonce.as_str()) ); + assert_eq!( + parameters.get("redirect_uri").map(String::as_str), + Some("https://maple.example/callback") + ); + } + + #[tokio::test] + async fn oauth_token_exchange_sends_the_recorded_redirect_uri() { + let recorded_redirect = "https://auth.example/auth/google/callback"; + let client = OAuthBasicClient::new(ClientId::new("test-client".to_string())) + .set_client_secret(ClientSecret::new("test-secret".to_string())) + .set_auth_uri(AuthUrl::new("https://provider.example/authorize".to_string()).unwrap()) + .set_token_uri(TokenUrl::new("https://provider.example/token".to_string()).unwrap()) + .set_redirect_uri(RedirectUrl::new(recorded_redirect.to_string()).unwrap()); + let token = client + .exchange_code(oauth2::AuthorizationCode::new("test-code".to_string())) + .request_async(&|request: oauth2::HttpRequest| async move { + let parameters: HashMap<_, _> = url::form_urlencoded::parse(request.body()) + .into_owned() + .collect(); + assert_eq!( + parameters.get("redirect_uri").map(String::as_str), + Some(recorded_redirect) + ); + Ok::<_, std::convert::Infallible>( + oauth2::http::Response::builder() + .status(200) + .header("Content-Type", "application/json") + .body(br#"{"access_token":"test-token","token_type":"bearer"}"#.to_vec()) + .unwrap(), + ) + }) + .await + .unwrap(); + assert_eq!( + oauth2::TokenResponse::access_token(&token).secret(), + "test-token" + ); } #[tokio::test] diff --git a/services/opensecret/src/transport_v2/gateway.rs b/services/opensecret/src/transport_v2/gateway.rs index 596aa4f1b..f50a305ab 100644 --- a/services/opensecret/src/transport_v2/gateway.rs +++ b/services/opensecret/src/transport_v2/gateway.rs @@ -623,6 +623,8 @@ mod tests { envelope::{Credential, CredentialKind}, }; + include!("oauth_callback_tests.rs"); + struct TestSession { client: SessionSecrets, server: Arc, diff --git a/services/opensecret/src/transport_v2/oauth_callback_tests.rs b/services/opensecret/src/transport_v2/oauth_callback_tests.rs new file mode 100644 index 000000000..fe8185f14 --- /dev/null +++ b/services/opensecret/src/transport_v2/oauth_callback_tests.rs @@ -0,0 +1,307 @@ +// Included in gateway::tests to reuse the real encrypted request/response harness. +// These checks deliberately stop before an OAuth provider exchange: authorization +// URL construction and rejected selections need no provider credentials or egress. + +async fn oauth_selection_v1_request( + application: &Router<()>, + session_id: uuid::Uuid, + session_key: &[u8; 32], + target: &str, + payload: &serde_json::Value, +) -> (u16, serde_json::Value) { + use chacha20poly1305::{aead::Aead, ChaCha20Poly1305, KeyInit, Nonce}; + + let cipher = ChaCha20Poly1305::new_from_slice(session_key).unwrap(); + let nonce = crate::encrypt::generate_random::<12>(); + let mut encrypted = nonce.to_vec(); + encrypted.extend_from_slice( + &cipher + .encrypt( + Nonce::from_slice(&nonce), + serde_json::to_vec(payload).unwrap().as_slice(), + ) + .unwrap(), + ); + let request = Request::builder() + .method(Method::POST) + .uri(target) + .header(header::CONTENT_TYPE, "application/json") + .header("x-session-id", session_id.to_string()) + .body(Body::from( + serde_json::to_vec(&serde_json::json!({ "encrypted": STANDARD.encode(encrypted) })) + .unwrap(), + )) + .unwrap(); + let response = application.clone().oneshot(request).await.unwrap(); + let status = response.status().as_u16(); + let body = to_bytes(response.into_body(), 64 * 1024).await.unwrap(); + let body: serde_json::Value = serde_json::from_slice(&body).unwrap(); + if status != 200 { + // V1 retains its existing plaintext error projection. + return (status, body); + } + let encrypted = STANDARD + .decode(body["encrypted"].as_str().unwrap()) + .unwrap(); + let (nonce, ciphertext) = encrypted.split_at(12); + let plaintext = cipher + .decrypt(Nonce::from_slice(nonce), ciphertext) + .unwrap(); + (status, serde_json::from_slice(&plaintext).unwrap()) +} + +async fn oauth_selection_v2_request( + gateway: &Router<()>, + session: &TestSession, + request_number: u8, + target: &str, + payload: &serde_json::Value, +) -> (u16, serde_json::Value) { + let request_id = RequestId::from_bytes([request_number; 16]); + let envelope = RequestEnvelope::new( + request_id, + None, + None, + "POST".to_string(), + target.to_string(), + vec![ + LogicalHeader::new("content-type".to_string(), "application/json".to_string()).unwrap(), + ], + Some(serde_json::to_vec(payload).unwrap()), + ) + .unwrap(); + let ciphertext = session + .client + .encrypt_request(request_id, &envelope.encode().unwrap()) + .unwrap(); + let response = gateway + .clone() + .oneshot(outer_request( + session.server.id(), + &session.routing_key, + ciphertext, + )) + .await + .unwrap(); + let records = decrypt_records(&session.client, request_id, response).await; + let Some(ResponseRecord::Start(start)) = records.first() else { + panic!("OAuth response must begin with authenticated status"); + }; + assert!(matches!(records.last(), Some(ResponseRecord::End))); + let mut body = Vec::new(); + for record in &records[1..records.len() - 1] { + let ResponseRecord::Chunk(chunk) = record else { + panic!("OAuth response must contain only body chunks before its terminal record"); + }; + body.extend_from_slice(chunk); + } + (start.status(), serde_json::from_slice(&body).unwrap()) +} + +#[tokio::test] +#[ignore = "requires AEAD_TAMPER_TEST_DATABASE_URL pointing at disposable migrated local Postgres"] +async fn db_oauth_callback_selection_v1_v2() { + use crate::{ + db::setup_db, + models::{ + org_project_secrets::NewOrgProjectSecret, + org_projects::NewOrgProject, + orgs::NewOrg, + project_settings::{AppleOAuthSettings, OAuthProviderSettings, OAuthSettings}, + }, + web::{ + attestation_routes::SessionState, + oauth_routes, + platform::common::{ + PROJECT_APPLE_OAUTH_SECRET, PROJECT_GITHUB_OAUTH_SECRET, + PROJECT_GOOGLE_OAUTH_SECRET, + }, + }, + AppMode, AppStateBuilder, + }; + use openssl::{ + ec::{EcGroup, EcKey}, + nid::Nid, + pkey::PKey, + }; + + let database_url = std::env::var("AEAD_TAMPER_TEST_DATABASE_URL") + .expect("this test requires the disposable database harness"); + let database_host = url::Url::parse(&database_url).expect("database URL must parse"); + assert!(matches!( + database_host.host_str(), + Some("127.0.0.1" | "localhost" | "[::1]") + )); + let enclave_key = [42u8; 32]; + let app_state = Arc::new( + AppStateBuilder::default() + .app_mode(AppMode::Local) + .db(setup_db(database_url)) + .enclave_key(enclave_key.to_vec()) + .aws_credential_manager(Arc::new(tokio::sync::RwLock::new(None))) + .openai_api_base("http://127.0.0.1:9".to_string()) + .tinfoil_api_base("http://127.0.0.1:9".to_string()) + .jwt_secret([24u8; 32].to_vec()) + .build() + .await + .unwrap(), + ); + let marker = uuid::Uuid::new_v4(); + let org = app_state + .db + .create_org(NewOrg::new(format!("oauth-selection-{marker}"))) + .unwrap(); + let project = app_state + .db + .create_org_project(NewOrgProject::new(org.id, "callback-selection".to_string())) + .unwrap(); + let default_url = |provider: &str| format!("https://app.example.test/auth/{provider}/callback"); + let allowed_url = |provider: &str| { + format!("https://auth.example.test/auth/{provider}/callback?channel=hosted") + }; + let provider_settings = |provider: &str| OAuthProviderSettings { + client_id: format!("oauth-selection-{provider}"), + redirect_url: default_url(provider), + additional_redirect_urls: Some(vec![allowed_url(provider)]), + }; + app_state + .db + .update_project_oauth_settings( + project.id, + OAuthSettings { + github_oauth_enabled: true, + google_oauth_enabled: true, + apple_oauth_enabled: true, + github_oauth_settings: Some(provider_settings("github")), + google_oauth_settings: Some(provider_settings("google")), + apple_oauth_settings: Some(AppleOAuthSettings { + client_id: "oauth-selection.services".to_string(), + redirect_url: default_url("apple"), + additional_redirect_urls: Some(vec![allowed_url("apple")]), + team_id: Some("TEAM123456".to_string()), + key_id: Some("KEY1234567".to_string()), + }), + }, + ) + .unwrap(); + + // Apple builds a JWT during initiation. Generate a throwaway signing key, + // matching apple_signin's existing unit fixture, without any Apple account. + let curve = EcGroup::from_curve_name(Nid::X9_62_PRIME256V1).unwrap(); + let apple_key = PKey::from_ec_key(EcKey::generate(&curve).unwrap()).unwrap(); + let apple_secret = STANDARD.encode(apple_key.private_key_to_pem_pkcs8().unwrap()); + let secret_key = secp256k1::SecretKey::from_slice(&enclave_key).unwrap(); + for (name, value) in [ + (PROJECT_GITHUB_OAUTH_SECRET, "github-fixture-secret"), + (PROJECT_GOOGLE_OAUTH_SECRET, "google-fixture-secret"), + (PROJECT_APPLE_OAUTH_SECRET, apple_secret.as_str()), + ] { + let ciphertext = crate::encrypt::encrypt_with_key(&secret_key, value.as_bytes()).await; + app_state + .db + .create_org_project_secret(NewOrgProjectSecret::new( + project.id, + name.to_string(), + ciphertext, + )) + .unwrap(); + } + + let application = oauth_routes(Arc::clone(&app_state)); + let session_v1_id = uuid::Uuid::new_v4(); + let session_v1_key = crate::encrypt::generate_random::<32>(); + app_state + .store_session_state(session_v1_id, SessionState::new(session_v1_key)) + .await + .unwrap(); + let session_v2 = test_session(0xD1); + let sessions = Arc::new(SessionStore::new(NonZeroUsize::new(2).unwrap())); + sessions.insert(Arc::clone(&session_v2.server)).unwrap(); + let gateway = request_router(application.clone(), sessions); + let mut request_number = 0; + + for provider in ["github", "google", "apple"] { + let target = format!("/auth/{provider}"); + let default = default_url(provider); + let allowed = allowed_url(provider); + let other_provider = if provider == "github" { + "google" + } else { + "github" + }; + for (selection, expected) in [ + (None, Some(default.as_str())), + (Some(serde_json::Value::Null), Some(default.as_str())), + (Some(serde_json::json!(default)), Some(default.as_str())), + (Some(serde_json::json!(allowed)), Some(allowed.as_str())), + ( + Some(serde_json::json!("https://unlisted.example.test/callback")), + None, + ), + (Some(serde_json::json!(allowed_url(other_provider))), None), + ] { + let mut payload = + serde_json::json!({ "client_id": project.client_id, "invite_code": "" }); + if let Some(selection) = selection { + payload["redirect_url"] = selection; + } + let v1 = oauth_selection_v1_request( + &application, + session_v1_id, + &session_v1_key, + &target, + &payload, + ) + .await; + request_number += 1; + let v2 = oauth_selection_v2_request( + &gateway, + &session_v2, + request_number, + &target, + &payload, + ) + .await; + for (transport, (status, response)) in [(1, v1), (2, v2)] { + let Some(expected) = expected else { + assert_eq!( + status, 400, + "{provider} V{transport} must reject unlisted callbacks" + ); + assert!(response.get("auth_url").is_none()); + assert!(response.get("state").is_none()); + continue; + }; + assert_eq!( + status, 200, + "{provider} V{transport} initiation should succeed" + ); + let auth_url = url::Url::parse(response["auth_url"].as_str().unwrap()).unwrap(); + let query: std::collections::HashMap<_, _> = auth_url.query_pairs().collect(); + assert_eq!( + query.get("redirect_uri").map(|value| value.as_ref()), + Some(expected) + ); + let opaque_state = response["state"].as_str().unwrap(); + assert_eq!( + query.get("state").map(|value| value.as_ref()), + Some(opaque_state) + ); + let state: serde_json::Value = serde_json::from_slice( + &base64::engine::general_purpose::URL_SAFE_NO_PAD + .decode(opaque_state) + .unwrap(), + ) + .unwrap(); + assert_eq!(state["client_id"], project.client_id.to_string()); + assert_eq!(state["redirect_url"], expected); + assert!(!state["csrf_token"].as_str().unwrap().is_empty()); + if provider != "apple" { + assert_eq!(query.contains_key("code_challenge"), transport == 2); + } + } + } + } + app_state.db.delete_org_project(&project).unwrap(); + app_state.db.delete_org(&org).unwrap(); +} diff --git a/services/opensecret/src/web/oauth_routes.rs b/services/opensecret/src/web/oauth_routes.rs index 1fee380b5..9790e718c 100644 --- a/services/opensecret/src/web/oauth_routes.rs +++ b/services/opensecret/src/web/oauth_routes.rs @@ -115,6 +115,8 @@ pub struct OAuthOAuthCallbackResponse { #[derive(Deserialize, Clone)] pub struct OAuthAuthRequest { pub client_id: Uuid, + #[serde(default)] + pub redirect_url: Option, } #[derive(Deserialize, Clone)] @@ -211,10 +213,60 @@ impl AppleTokenResponse { } } +enum OAuthRedirectSelection<'a> { + Requested(Option<&'a str>), + // Only construct after the callback state matches and consumes server-stored state. + ValidatedState(&'a str), +} + +fn select_oauth_redirect_url<'a>( + settings: &'a OAuthProviderSettings, + selection: OAuthRedirectSelection<'a>, +) -> Result<&'a str, ApiError> { + match selection { + OAuthRedirectSelection::Requested(None) => Ok(&settings.redirect_url), + OAuthRedirectSelection::Requested(Some(requested)) => { + if requested == settings.redirect_url + || settings + .additional_redirect_urls + .as_ref() + .is_some_and(|urls| urls.iter().any(|url| url == requested)) + { + Ok(requested) + } else { + Err(ApiError::BadRequest) + } + } + // Configuration may change while the user authenticates. A validated + // callback must exchange its code with the same URL used at initiation. + OAuthRedirectSelection::ValidatedState(redirect_url) => Ok(redirect_url), + } +} + +fn apple_token_exchange_params<'a>( + oauth_client: &'a BasicClient, + client_id: &'a str, + client_secret: &'a str, + code: &'a str, +) -> Result<[(&'static str, &'a str); 5], ApiError> { + let redirect_uri = oauth_client + .redirect_uri() + .ok_or(ApiError::InternalServerError)? + .as_str(); + Ok([ + ("client_id", client_id), + ("client_secret", client_secret), + ("code", code), + ("grant_type", "authorization_code"), + ("redirect_uri", redirect_uri), + ]) +} + async fn get_project_oauth_client( app_state: &AppState, project_id: i32, provider_name: &str, + redirect_selection: OAuthRedirectSelection<'_>, ) -> Result { // Get project OAuth settings let oauth_settings = app_state @@ -276,6 +328,7 @@ async fn get_project_oauth_client( let standard_settings = apple_settings.as_ref().map(|apple| OAuthProviderSettings { client_id: apple.client_id.clone(), redirect_url: apple.redirect_url.clone(), + additional_redirect_urls: apple.additional_redirect_urls.clone(), }); (enabled, standard_settings, secret) @@ -298,6 +351,10 @@ async fn get_project_oauth_client( ApiError::BadRequest })?; + // Reject unlisted requested URLs before decrypting credentials, generating + // provider authorization parameters, or allocating one-time state. + let redirect_url = select_oauth_redirect_url(&provider_settings, redirect_selection)?; + // Get and decrypt client secret let secret = secret_key.ok_or_else(|| { error!("{} OAuth secret not found", provider_name); @@ -386,11 +443,7 @@ async fn get_project_oauth_client( ApiError::InternalServerError })?; - // Log the OAuth URL and redirect URL being used - debug!( - "Building Apple OAuth client with Client ID: {}, Redirect URL: {}", - client_id_with_services, apple_settings.redirect_url - ); + debug!("Building Apple OAuth client"); // Use the same client ID for the OAuth client as in the JWT's sub claim let client_id_for_client = client_id_with_services.clone(); @@ -401,11 +454,11 @@ async fn get_project_oauth_client( .build_client( client_id_for_client, // Use the same client ID as in the JWT client_secret_jwt, - apple_settings.redirect_url.clone(), + redirect_url.to_string(), ) .await - .map_err(|e| { - error!("Failed to build Apple OAuth client: {:?}", e); + .map_err(|_| { + error!("Failed to build Apple OAuth client"); ApiError::InternalServerError }) } else { @@ -414,7 +467,7 @@ async fn get_project_oauth_client( .build_client( provider_settings.client_id.clone(), client_secret, - provider_settings.redirect_url.clone(), + redirect_url.to_string(), ) .await .map_err(|_| ApiError::InternalServerError) @@ -434,7 +487,13 @@ pub async fn initiate_oauth( .map_err(|_| ApiError::BadRequest)?; // Get OAuth client for this project - let oauth_client = get_project_oauth_client(&app_state, project.id, provider_name).await?; + let oauth_client = get_project_oauth_client( + &app_state, + project.id, + provider_name, + OAuthRedirectSelection::Requested(auth_request.redirect_url.as_deref()), + ) + .await?; // Get the OAuth provider let oauth_provider = app_state @@ -483,10 +542,16 @@ pub async fn initiate_oauth( (auth_url, csrf_token, None) }; - // Create our state that includes both CSRF token and client_id + // Snapshot the actual callback even when the caller omitted redirect_url. + // Callbacks must not pick up a subsequently changed project default. let state = OAuthState { csrf_token: csrf_token.secret().clone(), client_id: project.client_id, + redirect_url: oauth_client + .redirect_uri() + .ok_or(ApiError::InternalServerError)? + .as_str() + .to_string(), }; // Store the complete state in the provider @@ -541,13 +606,13 @@ pub async fn oauth_callback( // Decode and parse the state let state_json = base64::engine::general_purpose::URL_SAFE_NO_PAD .decode(&callback_request.state) - .map_err(|e| { - error!("Could not parse state: {:?}", e); + .map_err(|_| { + error!("Could not decode OAuth state"); ApiError::BadRequest })?; debug!("Parsed state from request"); - let state: OAuthState = serde_json::from_slice(&state_json).map_err(|e| { - error!("Could not parse OAuthState: {:?}", e); + let state: OAuthState = serde_json::from_slice(&state_json).map_err(|_| { + error!("Could not parse OAuth state"); ApiError::BadRequest })?; debug!("Converted state to OAuthState"); @@ -600,7 +665,13 @@ pub async fn oauth_callback( })?; // Get OAuth client for this project - let oauth_client = get_project_oauth_client(&app_state, project.id, provider_name).await?; + let oauth_client = get_project_oauth_client( + &app_state, + project.id, + provider_name, + OAuthRedirectSelection::ValidatedState(&state.redirect_url), + ) + .await?; // Exchange the code for an access token debug!( @@ -713,14 +784,6 @@ pub async fn oauth_callback( ApiError::InternalServerError })?; - let redirect_uri = oauth_client - .redirect_uri() - .ok_or_else(|| { - error!("OAuth redirect URL not configured"); - ApiError::InternalServerError - })? - .as_str(); - // Make sure the client_id parameter matches what's in the JWT's sub claim // Apple requires these to be exactly the same // - client_id parameter must match JWT's sub claim exactly @@ -733,13 +796,12 @@ pub async fn oauth_callback( }; // Build the form data for the token request - let params = [ - ("client_id", client_id_param), - ("client_secret", &client_secret), - ("code", &callback_request.code), - ("grant_type", "authorization_code"), - ("redirect_uri", redirect_uri), - ]; + let params = apple_token_exchange_params( + &oauth_client, + client_id_param, + &client_secret, + &callback_request.code, + )?; // Create the request let request = client @@ -1594,6 +1656,147 @@ mod tests { use diesel::{ExpressionMethods, QueryDsl, RunQueryDsl}; use tokio::sync::RwLock; + fn redirect_settings() -> OAuthProviderSettings { + OAuthProviderSettings { + client_id: "provider-client".to_string(), + redirect_url: "https://app.example/auth/callback".to_string(), + additional_redirect_urls: Some(vec![ + "https://auth.example/auth/callback".to_string(), + "http://127.0.0.1:3000/auth/callback".to_string(), + ]), + } + } + + #[test] + fn oauth_initiate_request_keeps_old_payloads_compatible() { + let client_id = Uuid::from_u128(1); + for payload in [ + serde_json::json!({ "client_id": client_id }), + serde_json::json!({ "client_id": client_id, "redirect_url": null }), + ] { + let request: OAuthAuthRequest = serde_json::from_value(payload).unwrap(); + assert_eq!(request.client_id, client_id); + assert!(request.redirect_url.is_none()); + } + + let request: OAuthAuthRequest = serde_json::from_value(serde_json::json!({ + "client_id": client_id, + "redirect_url": "https://auth.example/auth/callback", + })) + .unwrap(); + assert_eq!( + request.redirect_url.as_deref(), + Some("https://auth.example/auth/callback") + ); + assert!( + serde_json::from_value::(serde_json::json!({ + "client_id": client_id, + "redirect_url": ["https://auth.example/auth/callback"], + })) + .is_err() + ); + } + + #[test] + fn oauth_redirect_selection_uses_default_or_an_exact_allowlist_match() { + let settings = redirect_settings(); + assert_eq!( + select_oauth_redirect_url(&settings, OAuthRedirectSelection::Requested(None)).unwrap(), + settings.redirect_url + ); + for requested in [ + settings.redirect_url.as_str(), + "https://auth.example/auth/callback", + "http://127.0.0.1:3000/auth/callback", + ] { + assert_eq!( + select_oauth_redirect_url( + &settings, + OAuthRedirectSelection::Requested(Some(requested)), + ) + .unwrap(), + requested + ); + } + for unlisted in [ + "https://other.example/auth/callback", + "https://auth.example/auth/other", + "https://auth.example/auth/callback/", + "https://auth.example/auth/callback?extra=1", + "https://auth.example/auth/callback#fragment", + "https://auth.example:443/auth/callback", + "https://AUTH.example/auth/callback", + "https://auth.example/auth/%63allback", + "https://auth.example.evil.example/auth/callback", + "", + ] { + assert!(matches!( + select_oauth_redirect_url( + &settings, + OAuthRedirectSelection::Requested(Some(unlisted)), + ), + Err(ApiError::BadRequest) + )); + } + } + + #[test] + fn validated_callback_keeps_selected_url_after_configuration_changes() { + let mut settings = redirect_settings(); + for request in [None, Some("https://auth.example/auth/callback")] { + let selected = + select_oauth_redirect_url(&settings, OAuthRedirectSelection::Requested(request)) + .unwrap() + .to_string(); + settings.redirect_url = "https://new-app.example/auth/callback".to_string(); + settings.additional_redirect_urls = None; + assert_eq!( + select_oauth_redirect_url( + &settings, + OAuthRedirectSelection::ValidatedState(&selected), + ) + .unwrap(), + selected + ); + assert!(matches!( + select_oauth_redirect_url( + &settings, + OAuthRedirectSelection::Requested(Some(&selected)), + ), + Err(ApiError::BadRequest) + )); + settings = redirect_settings(); + } + } + + #[test] + fn apple_token_exchange_uses_the_callback_clients_recorded_redirect() { + let selected = "https://auth.example/auth/apple/callback"; + let client = oauth2::basic::BasicClient::new(oauth2::ClientId::new( + "apple-client.services".to_string(), + )) + .set_auth_uri(oauth2::AuthUrl::new("https://apple.example/authorize".to_string()).unwrap()) + .set_token_uri(oauth2::TokenUrl::new("https://apple.example/token".to_string()).unwrap()) + .set_redirect_uri(oauth2::RedirectUrl::new(selected.to_string()).unwrap()); + let params = apple_token_exchange_params( + &client, + "apple-client.services", + "test-secret", + "test-code", + ) + .unwrap(); + assert_eq!( + params, + [ + ("client_id", "apple-client.services"), + ("client_secret", "test-secret"), + ("code", "test-code"), + ("grant_type", "authorization_code"), + ("redirect_uri", selected), + ] + ); + } + #[tokio::test] #[ignore = "requires AEAD_TAMPER_TEST_DATABASE_URL pointing at disposable migrated local Postgres"] async fn db_oauth_same_email_different_subject_is_not_linked() { diff --git a/services/opensecret/src/web/platform/common.rs b/services/opensecret/src/web/platform/common.rs index b6f33f002..69bfde0a6 100644 --- a/services/opensecret/src/web/platform/common.rs +++ b/services/opensecret/src/web/platform/common.rs @@ -189,6 +189,40 @@ pub fn validate_email_provider(provider: &str) -> Result<(), validator::Validati Ok(()) } +const MAX_ADDITIONAL_REDIRECT_URLS: usize = 16; + +fn validate_oauth_redirect_url(redirect_url: &str) -> Result<(), validator::ValidationError> { + if redirect_url.is_empty() || redirect_url.len() > 255 { + let mut error = validator::ValidationError::new("oauth_redirect_url"); + error.message = Some(format!("Redirect URL must not be empty and must not exceed 255 characters (current length: {})", redirect_url.len()).into()); + return Err(error); + } + // Preserve the existing generic URL contract, including loopback development. + if let Err(parse_err) = url::Url::parse(redirect_url) { + let mut error = validator::ValidationError::new("oauth_redirect_url_invalid"); + error.message = Some(format!("Invalid redirect URL: {}", parse_err).into()); + return Err(error); + } + Ok(()) +} + +fn validate_additional_redirect_urls( + redirect_urls: Option<&[String]>, +) -> Result<(), validator::ValidationError> { + let Some(redirect_urls) = redirect_urls else { + return Ok(()); + }; + if redirect_urls.len() > MAX_ADDITIONAL_REDIRECT_URLS { + let mut error = validator::ValidationError::new("oauth_additional_redirect_urls"); + error.message = Some("At most 16 additional redirect URLs are allowed".into()); + return Err(error); + } + for redirect_url in redirect_urls { + validate_oauth_redirect_url(redirect_url)?; + } + Ok(()) +} + pub fn validate_oauth_provider_settings( settings: &OAuthProviderSettings, ) -> Result<(), validator::ValidationError> { @@ -198,18 +232,8 @@ pub fn validate_oauth_provider_settings( error.message = Some(format!("Client ID must not be empty and must not exceed 255 characters (current length: {})", settings.client_id.len()).into()); return Err(error); } - // Validate redirect_url - if settings.redirect_url.is_empty() || settings.redirect_url.len() > 255 { - let mut error = validator::ValidationError::new("oauth_redirect_url"); - error.message = Some(format!("Redirect URL must not be empty and must not exceed 255 characters (current length: {})", settings.redirect_url.len()).into()); - return Err(error); - } - // Basic URL validation - if let Err(parse_err) = url::Url::parse(&settings.redirect_url) { - let mut error = validator::ValidationError::new("oauth_redirect_url_invalid"); - error.message = Some(format!("Invalid redirect URL: {}", parse_err).into()); - return Err(error); - } + validate_oauth_redirect_url(&settings.redirect_url)?; + validate_additional_redirect_urls(settings.additional_redirect_urls.as_deref())?; Ok(()) } @@ -223,18 +247,8 @@ pub fn validate_apple_oauth_settings( error.message = Some(format!("Client ID must not be empty and must not exceed 255 characters (current length: {})", settings.client_id.len()).into()); return Err(error); } - // Validate redirect_url - if settings.redirect_url.is_empty() || settings.redirect_url.len() > 255 { - let mut error = validator::ValidationError::new("oauth_redirect_url"); - error.message = Some(format!("Redirect URL must not be empty and must not exceed 255 characters (current length: {})", settings.redirect_url.len()).into()); - return Err(error); - } - // Basic URL validation - if let Err(parse_err) = url::Url::parse(&settings.redirect_url) { - let mut error = validator::ValidationError::new("oauth_redirect_url_invalid"); - error.message = Some(format!("Invalid redirect URL: {}", parse_err).into()); - return Err(error); - } + validate_oauth_redirect_url(&settings.redirect_url)?; + validate_additional_redirect_urls(settings.additional_redirect_urls.as_deref())?; // Validate team_id if provided if let Some(ref team_id) = settings.team_id { @@ -268,3 +282,87 @@ pub fn validate_apple_oauth_settings( Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::{json, Value}; + + fn oauth_request(provider: &str, additions: Option) -> Value { + let mut settings = json!({ + "client_id": "customer-client", + "redirect_url": "https://customer.example/callback" + }); + if let Some(additions) = additions { + settings["additional_redirect_urls"] = additions; + } + let mut request = json!({ + "google_oauth_enabled": false, + "github_oauth_enabled": false, + "apple_oauth_enabled": false + }); + request[format!("{provider}_oauth_enabled")] = json!(true); + request[format!("{provider}_oauth_settings")] = settings; + request + } + + #[test] + fn oauth_redirect_lists_accept_legacy_null_empty_and_generic_urls() { + for provider in ["google", "github", "apple"] { + for additions in [ + None, + Some(Value::Null), + Some(json!([])), + Some(json!([ + "http://127.0.0.1:5173/callback", + "https://dev.secretgpt.ai/callback", + "https://preview.opensecret.cloud/callback", + "https://customer.example/callback" + ])), + ] { + let request: UpdateOAuthSettingsRequest = + serde_json::from_value(oauth_request(provider, additions)).unwrap(); + assert!(request.validate().is_ok(), "provider: {provider}"); + } + } + } + + #[test] + fn oauth_redirect_lists_enforce_count_and_per_url_bounds() { + for provider in ["google", "github", "apple"] { + let at_limit = vec!["https://customer.example/callback"; MAX_ADDITIONAL_REDIRECT_URLS]; + let request: UpdateOAuthSettingsRequest = + serde_json::from_value(oauth_request(provider, Some(json!(at_limit)))).unwrap(); + assert!(request.validate().is_ok()); + + for additions in [ + json!(vec![ + "https://customer.example/callback"; + MAX_ADDITIONAL_REDIRECT_URLS + 1 + ]), + json!([""]), + json!(["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/relative/callback"]), + json!([format!("https://customer.example/{}", "x".repeat(256))]), + ] { + let request: UpdateOAuthSettingsRequest = + serde_json::from_value(oauth_request(provider, Some(additions))).unwrap(); + assert!(request.validate().is_err(), "provider: {provider}"); + } + } + } + + #[test] + fn oauth_redirect_lists_reject_wrong_json_types() { + for provider in ["google", "github", "apple"] { + for additions in [json!("https://customer.example/callback"), json!([42])] { + assert!( + serde_json::from_value::(oauth_request( + provider, + Some(additions) + )) + .is_err() + ); + } + } + } +} diff --git a/services/opensecret/src/web/platform/project_routes.rs b/services/opensecret/src/web/platform/project_routes.rs index c3931b82c..8b2e7b78f 100644 --- a/services/opensecret/src/web/platform/project_routes.rs +++ b/services/opensecret/src/web/platform/project_routes.rs @@ -803,7 +803,8 @@ async fn update_oauth_settings( apple_oauth_settings: update_request.apple_oauth_settings, }; - // Update settings + // The database preserves omitted additional callback lists atomically; + // every other setting retains whole-object replacement semantics. let settings = data .db .update_project_oauth_settings(project.id, oauth_settings)?; From 1ee66424cec3843f6a417ed426b83cb1ab4db537 Mon Sep 17 00:00:00 2001 From: Anthony Ronning <101225832+AnthonyRonning@users.noreply.github.com> Date: Sun, 20 Sep 2026 06:18:17 +0000 Subject: [PATCH 2/2] docs(opensecret): clarify optional callback list responses --- services/opensecret/docs/oauth-callbacks.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/services/opensecret/docs/oauth-callbacks.md b/services/opensecret/docs/oauth-callbacks.md index 7020a6117..bc7bac3bb 100644 --- a/services/opensecret/docs/oauth-callbacks.md +++ b/services/opensecret/docs/oauth-callbacks.md @@ -37,8 +37,11 @@ enabled flags and other provider settings retain their existing semantics. - This preservation applies when the provider settings object is supplied. Omitting or clearing the entire provider object retains the existing whole-object PUT behavior; it is not a patch API for other fields. -- GET and the PUT response include a stored list. Existing rows without the - field remain readable; no SQL schema migration is required. +- GET and the PUT response include `additional_redirect_urls` when a list is + stored, including `[]`. An unset or null stored value omits the field from + the response; both omission and `[]` mean no additional callbacks on read. + Existing rows without the field remain readable; no SQL schema migration + is required. - URL-list preservation and the settings write are serialized per project, so an older writer that omits the field cannot overwrite a concurrently committed list with an earlier snapshot.