max / goingson
15 files changed,
+280 insertions,
-164 deletions
| @@ -217,6 +217,7 @@ const api = { | |||
| 217 | 217 | oauth: { | |
| 218 | 218 | listProviders: () => invoke('list_oauth_providers'), | |
| 219 | 219 | start: (providerId) => invoke('start_oauth', { providerId }), // Opens browser for consent | |
| 220 | + | pollResult: (port) => invoke('poll_oauth_result', { port }), // Polls the loopback callback over IPC | |
| 220 | 221 | complete: (input) => invoke('complete_oauth', { input }), // Exchanges auth code for tokens | |
| 221 | 222 | refreshTokens: (accountId) => invoke('refresh_oauth_tokens', { accountId }), | |
| 222 | 223 | disconnect: (accountId) => invoke('disconnect_oauth', { accountId }), |
| @@ -694,26 +694,17 @@ | |||
| 694 | 694 | } | |
| 695 | 695 | ||
| 696 | 696 | try { | |
| 697 | - | // Check if there's a callback response available | |
| 698 | - | // The callback server writes to a temp location we can poll | |
| 699 | - | // Poll the local OAuth callback server. Expected to fail | |
| 700 | - | // repeatedly until the user completes the browser auth flow. | |
| 701 | - | const response = await fetch(`http://127.0.0.1:${port}/result`, { | |
| 702 | - | method: 'GET', | |
| 703 | - | mode: 'cors', | |
| 704 | - | }).catch(() => null); | |
| 705 | - | ||
| 706 | - | if (response && response.ok) { | |
| 707 | - | const data = await response.json(); | |
| 708 | - | if (data.code) { | |
| 709 | - | await completeOAuth(data.code, data.state); | |
| 710 | - | return; | |
| 711 | - | } else if (data.error) { | |
| 712 | - | GoingsOn.ui.showToast('OAuth error: ' + data.error, 'error'); | |
| 713 | - | pendingOAuthState = null; | |
| 714 | - | refreshAccountsView(); | |
| 715 | - | return; | |
| 716 | - | } | |
| 697 | + | // Poll the local callback server over IPC. Stays "pending" | |
| 698 | + | // until the user completes the browser auth flow. | |
| 699 | + | const data = await GoingsOn.api.oauth.pollResult(port); | |
| 700 | + | if (data.status === 'success' && data.code) { | |
| 701 | + | await completeOAuth(data.code, data.state); | |
| 702 | + | return; | |
| 703 | + | } else if (data.status === 'error') { | |
| 704 | + | GoingsOn.ui.showToast('OAuth error: ' + (data.error || 'unknown'), 'error'); | |
| 705 | + | pendingOAuthState = null; | |
| 706 | + | refreshAccountsView(); | |
| 707 | + | return; | |
| 717 | 708 | } | |
| 718 | 709 | } catch (e) { | |
| 719 | 710 | // Ignore polling errors |
| @@ -246,37 +246,34 @@ | |||
| 246 | 246 | } | |
| 247 | 247 | ||
| 248 | 248 | try { | |
| 249 | - | const resp = await fetch(`http://127.0.0.1:${port}/result`); | |
| 250 | - | if (resp.status === 200) { | |
| 251 | - | const data = await resp.json(); | |
| 252 | - | ||
| 253 | - | // Still waiting for the browser redirect | |
| 254 | - | if (data.status === 'pending') return; | |
| 255 | - | ||
| 256 | - | clearInterval(pollInterval); | |
| 257 | - | ||
| 258 | - | if (data.code) { | |
| 259 | - | // Complete auth | |
| 260 | - | try { | |
| 261 | - | await GoingsOn.api.sync.completeAuth({ | |
| 262 | - | code: data.code, | |
| 263 | - | state: data.state, | |
| 264 | - | }); | |
| 265 | - | GoingsOn.ui.showToast('Connected to Makenot.work!'); | |
| 266 | - | refreshSyncIndicator(); | |
| 267 | - | // Force re-render: open settings overlay and show sync section. | |
| 268 | - | await GoingsOn.settings.open(); | |
| 269 | - | await GoingsOn.settings.showSection('sync'); | |
| 270 | - | } catch (err) { | |
| 271 | - | GoingsOn.ui.showToast('Auth failed: ' + GoingsOn.utils.getErrorMessage(err), 'error'); | |
| 272 | - | } | |
| 273 | - | } else if (data.error) { | |
| 274 | - | GoingsOn.ui.showToast('Auth error: ' + data.error, 'error'); | |
| 249 | + | // Poll the local callback server over IPC (no HTTP endpoint). | |
| 250 | + | const data = await GoingsOn.api.oauth.pollResult(port); | |
| 251 | + | ||
| 252 | + | // Still waiting for the browser redirect | |
| 253 | + | if (data.status === 'pending') return; | |
| 254 | + | ||
| 255 | + | clearInterval(pollInterval); | |
| 256 | + | ||
| 257 | + | if (data.status === 'success' && data.code) { | |
| 258 | + | // Complete auth | |
| 259 | + | try { | |
| 260 | + | await GoingsOn.api.sync.completeAuth({ | |
| 261 | + | code: data.code, | |
| 262 | + | state: data.state, | |
| 263 | + | }); | |
| 264 | + | GoingsOn.ui.showToast('Connected to Makenot.work!'); | |
| 265 | + | refreshSyncIndicator(); | |
| 266 | + | // Force re-render: open settings overlay and show sync section. | |
| 267 | + | await GoingsOn.settings.open(); | |
| 268 | + | await GoingsOn.settings.showSection('sync'); | |
| 269 | + | } catch (err) { | |
| 270 | + | GoingsOn.ui.showToast('Auth failed: ' + GoingsOn.utils.getErrorMessage(err), 'error'); | |
| 275 | 271 | } | |
| 272 | + | } else if (data.status === 'error') { | |
| 273 | + | GoingsOn.ui.showToast('Auth error: ' + (data.error || 'unknown'), 'error'); | |
| 276 | 274 | } | |
| 277 | - | // 204 = still waiting, continue polling | |
| 278 | 275 | } catch (_) { | |
| 279 | - | // Server not ready yet or gone, keep polling | |
| 276 | + | // Command briefly unavailable, keep polling | |
| 280 | 277 | } | |
| 281 | 278 | }, 1000); | |
| 282 | 279 | } |
| @@ -625,7 +625,6 @@ async fn send_email_inner(state: &Arc<AppState>, input: SendEmailInput) -> Resul | |||
| 625 | 625 | account.smtp_port as u16, | |
| 626 | 626 | &account.email_address, | |
| 627 | 627 | &access_token, | |
| 628 | - | true, | |
| 629 | 628 | ); | |
| 630 | 629 | smtp_client | |
| 631 | 630 | .send_message(¶ms) |
| @@ -28,19 +28,21 @@ pub(super) fn uses_oauth_imap(account: &EmailAccount) -> bool { | |||
| 28 | 28 | account.is_oauth() && !uses_jmap(account) | |
| 29 | 29 | } | |
| 30 | 30 | ||
| 31 | + | /// Resolves an account password, preferring the secure keychain over the | |
| 32 | + | /// legacy plaintext `password` column. | |
| 33 | + | /// | |
| 34 | + | /// New accounts store the secret only in the keychain (the DB column is `""`); | |
| 35 | + | /// the column is consulted solely for accounts created by older versions that | |
| 36 | + | /// have not yet been re-saved. | |
| 37 | + | fn resolve_password(keychain: Option<String>, db_column: &str) -> Option<String> { | |
| 38 | + | keychain.or_else(|| (!db_column.is_empty()).then(|| db_column.to_string())) | |
| 39 | + | } | |
| 40 | + | ||
| 31 | 41 | /// Gets the password for a password-based email account. | |
| 32 | 42 | pub(super) fn get_account_password(account: &EmailAccount) -> Result<String, ApiError> { | |
| 33 | - | if !account.password.is_empty() { | |
| 34 | - | return Ok(account.password.clone()); | |
| 35 | - | } | |
| 36 | - | ||
| 37 | - | // Migration: check keychain for passwords stored by older versions | |
| 38 | 43 | let raw_id: Uuid = account.id.into(); | |
| 39 | - | if let Some(password) = CredentialStore::get_password(raw_id) { | |
| 40 | - | return Ok(password); | |
| 41 | - | } | |
| 42 | - | ||
| 43 | - | Err(ApiError::auth("No password available")) | |
| 44 | + | resolve_password(CredentialStore::get_password(raw_id), &account.password) | |
| 45 | + | .or_api_err(|| ApiError::auth("No password available")) | |
| 44 | 46 | } | |
| 45 | 47 | ||
| 46 | 48 | /// Gets a valid access token for an OAuth account, refreshing if needed. | |
| @@ -261,6 +263,8 @@ pub async fn create_email_account(state: State<'_, Arc<AppState>>, input: EmailA | |||
| 261 | 263 | } | |
| 262 | 264 | } | |
| 263 | 265 | ||
| 266 | + | // Never write the password to the DB column; store it in the OS keychain | |
| 267 | + | // and leave the column empty (mirrors the OAuth token path). | |
| 264 | 268 | let account = state.email_accounts | |
| 265 | 269 | .create( | |
| 266 | 270 | DESKTOP_USER_ID, | |
| @@ -271,12 +275,18 @@ pub async fn create_email_account(state: State<'_, Arc<AppState>>, input: EmailA | |||
| 271 | 275 | &input.smtp_server, | |
| 272 | 276 | input.smtp_port, | |
| 273 | 277 | &input.username, | |
| 274 | - | &input.password, | |
| 278 | + | "", | |
| 275 | 279 | input.use_tls, | |
| 276 | 280 | input.archive_folder_name.as_deref(), | |
| 277 | 281 | ) | |
| 278 | 282 | .await?; | |
| 279 | 283 | ||
| 284 | + | if !input.password.is_empty() { | |
| 285 | + | let raw_id: Uuid = account.id.into(); | |
| 286 | + | CredentialStore::store_password(raw_id, &input.password) | |
| 287 | + | .map_api_err("Failed to store password", ApiError::internal)?; | |
| 288 | + | } | |
| 289 | + | ||
| 280 | 290 | Ok(account) | |
| 281 | 291 | } | |
| 282 | 292 | ||
| @@ -297,6 +307,19 @@ pub async fn update_email_account(state: State<'_, Arc<AppState>>, id: EmailAcco | |||
| 297 | 307 | } | |
| 298 | 308 | } | |
| 299 | 309 | ||
| 310 | + | // Route a changed password through the keychain. Pass `Some("")` to the | |
| 311 | + | // repo so any stale plaintext in the column is cleared; `None` leaves the | |
| 312 | + | // column untouched when the user did not change the password. | |
| 313 | + | let db_password = match input.password.as_deref() { | |
| 314 | + | Some(pwd) if !pwd.is_empty() => { | |
| 315 | + | let raw_id: Uuid = id.into(); | |
| 316 | + | CredentialStore::store_password(raw_id, pwd) | |
| 317 | + | .map_api_err("Failed to store password", ApiError::internal)?; | |
| 318 | + | Some("") | |
| 319 | + | } | |
| 320 | + | _ => None, | |
| 321 | + | }; | |
| 322 | + | ||
| 300 | 323 | state.email_accounts | |
| 301 | 324 | .update( | |
| 302 | 325 | id, | |
| @@ -308,7 +331,7 @@ pub async fn update_email_account(state: State<'_, Arc<AppState>>, id: EmailAcco | |||
| 308 | 331 | &input.smtp_server, | |
| 309 | 332 | input.smtp_port, | |
| 310 | 333 | &input.username, | |
| 311 | - | input.password.as_deref(), | |
| 334 | + | db_password, | |
| 312 | 335 | input.use_tls, | |
| 313 | 336 | input.archive_folder_name.as_deref(), | |
| 314 | 337 | ) | |
| @@ -322,6 +345,7 @@ pub async fn update_email_account(state: State<'_, Arc<AppState>>, id: EmailAcco | |||
| 322 | 345 | pub async fn delete_email_account(state: State<'_, Arc<AppState>>, id: EmailAccountId) -> Result<bool, ApiError> { | |
| 323 | 346 | let raw_id: Uuid = id.into(); | |
| 324 | 347 | let _ = CredentialStore::delete_oauth(raw_id); | |
| 348 | + | let _ = CredentialStore::delete_password(raw_id); | |
| 325 | 349 | Ok(state.email_accounts.delete(id, DESKTOP_USER_ID).await?) | |
| 326 | 350 | } | |
| 327 | 351 | ||
| @@ -450,7 +474,6 @@ async fn test_oauth_imap_account( | |||
| 450 | 474 | account.smtp_port as u16, | |
| 451 | 475 | &account.email_address, | |
| 452 | 476 | &access_token, | |
| 453 | - | true, | |
| 454 | 477 | ); | |
| 455 | 478 | ||
| 456 | 479 | let (smtp_success, smtp_message) = match smtp_client.test_connection().await { | |
| @@ -514,3 +537,32 @@ async fn test_jmap_account(account: &EmailAccount) -> Result<TestConnectionRespo | |||
| 514 | 537 | available_folders: folders, | |
| 515 | 538 | }) | |
| 516 | 539 | } | |
| 540 | + | ||
| 541 | + | #[cfg(test)] | |
| 542 | + | mod tests { | |
| 543 | + | use super::resolve_password; | |
| 544 | + | ||
| 545 | + | #[test] | |
| 546 | + | fn keychain_password_is_preferred_over_db_column() { | |
| 547 | + | // A keychain hit always wins, even when a legacy plaintext column is present. | |
| 548 | + | assert_eq!( | |
| 549 | + | resolve_password(Some("secret".to_string()), "stale_plaintext"), | |
| 550 | + | Some("secret".to_string()) | |
| 551 | + | ); | |
| 552 | + | } | |
| 553 | + | ||
| 554 | + | #[test] | |
| 555 | + | fn db_column_is_used_only_when_keychain_is_empty() { | |
| 556 | + | // Legacy accounts created before the keychain migration fall back to the column. | |
| 557 | + | assert_eq!( | |
| 558 | + | resolve_password(None, "legacy_plaintext"), | |
| 559 | + | Some("legacy_plaintext".to_string()) | |
| 560 | + | ); | |
| 561 | + | } | |
| 562 | + | ||
| 563 | + | #[test] | |
| 564 | + | fn no_password_anywhere_resolves_to_none() { | |
| 565 | + | // New accounts with the column blanked and nothing in the keychain have no password. | |
| 566 | + | assert_eq!(resolve_password(None, ""), None); | |
| 567 | + | } | |
| 568 | + | } |
| @@ -72,6 +72,49 @@ pub struct OAuthCompleteResponse { | |||
| 72 | 72 | pub provider_name: String, | |
| 73 | 73 | } | |
| 74 | 74 | ||
| 75 | + | /// Result of polling a pending OAuth callback. | |
| 76 | + | /// | |
| 77 | + | /// Mirrors the callback server's stored state. `status` is one of | |
| 78 | + | /// `pending`, `success`, or `error`. | |
| 79 | + | #[derive(Debug, Serialize)] | |
| 80 | + | #[serde(rename_all = "camelCase")] | |
| 81 | + | pub struct OAuthPollResponse { | |
| 82 | + | pub status: String, | |
| 83 | + | pub code: Option<String>, | |
| 84 | + | pub state: Option<String>, | |
| 85 | + | pub error: Option<String>, | |
| 86 | + | pub description: Option<String>, | |
| 87 | + | } | |
| 88 | + | ||
| 89 | + | impl From<crate::oauth::StoredCallback> for OAuthPollResponse { | |
| 90 | + | fn from(stored: crate::oauth::StoredCallback) -> Self { | |
| 91 | + | use crate::oauth::StoredCallback; | |
| 92 | + | match stored { | |
| 93 | + | StoredCallback::Pending => OAuthPollResponse { | |
| 94 | + | status: "pending".to_string(), | |
| 95 | + | code: None, | |
| 96 | + | state: None, | |
| 97 | + | error: None, | |
| 98 | + | description: None, | |
| 99 | + | }, | |
| 100 | + | StoredCallback::Success { code, state } => OAuthPollResponse { | |
| 101 | + | status: "success".to_string(), | |
| 102 | + | code: Some(code), | |
| 103 | + | state: Some(state), | |
| 104 | + | error: None, | |
| 105 | + | description: None, | |
| 106 | + | }, | |
| 107 | + | StoredCallback::Error { error, description } => OAuthPollResponse { | |
| 108 | + | status: "error".to_string(), | |
| 109 | + | code: None, | |
| 110 | + | state: None, | |
| 111 | + | error: Some(error), | |
| 112 | + | description, | |
| 113 | + | }, | |
| 114 | + | } | |
| 115 | + | } | |
| 116 | + | } | |
| 117 | + | ||
| 75 | 118 | // ============ Commands ============ | |
| 76 | 119 | ||
| 77 | 120 | /// Lists available OAuth providers. | |
| @@ -136,6 +179,13 @@ pub async fn start_oauth( | |||
| 136 | 179 | }); | |
| 137 | 180 | } | |
| 138 | 181 | ||
| 182 | + | // Keep the callback server alive so the frontend can poll its result over | |
| 183 | + | // IPC (see `poll_oauth_result`). | |
| 184 | + | { | |
| 185 | + | let mut servers = state.pending_oauth_servers.lock().unwrap_or_else(|e| e.into_inner()); | |
| 186 | + | servers.insert(port, callback_server); | |
| 187 | + | } | |
| 188 | + | ||
| 139 | 189 | Ok(OAuthStartResponse { | |
| 140 | 190 | auth_url: start_result.auth_url, | |
| 141 | 191 | state: start_result.state, | |
| @@ -144,6 +194,28 @@ pub async fn start_oauth( | |||
| 144 | 194 | }) | |
| 145 | 195 | } | |
| 146 | 196 | ||
| 197 | + | /// Polls the local OAuth callback server for a result. | |
| 198 | + | /// | |
| 199 | + | /// The frontend calls this on an interval after opening the browser. Delivering | |
| 200 | + | /// the authorization code over IPC (instead of an unauthenticated HTTP endpoint) | |
| 201 | + | /// keeps it out of reach of other local processes on the loopback port. | |
| 202 | + | /// | |
| 203 | + | /// Returns `{status: "pending"}` until the redirect arrives, then a `success` | |
| 204 | + | /// (with code/state) or `error` payload. An unknown port also reports `pending`. | |
| 205 | + | #[tauri::command] | |
| 206 | + | #[instrument(skip_all)] | |
| 207 | + | pub async fn poll_oauth_result( | |
| 208 | + | state: State<'_, Arc<AppState>>, | |
| 209 | + | port: u16, | |
| 210 | + | ) -> Result<OAuthPollResponse, ApiError> { | |
| 211 | + | let servers = state.pending_oauth_servers.lock().unwrap_or_else(|e| e.into_inner()); | |
| 212 | + | let stored = match servers.get(&port) { | |
| 213 | + | Some(server) => server.poll(), | |
| 214 | + | None => crate::oauth::StoredCallback::Pending, | |
| 215 | + | }; | |
| 216 | + | Ok(stored.into()) | |
| 217 | + | } | |
| 218 | + | ||
| 147 | 219 | /// Completes OAuth with an authorization code. | |
| 148 | 220 | /// | |
| 149 | 221 | /// Called after the browser redirects back with the code. | |
| @@ -167,6 +239,12 @@ pub async fn complete_oauth( | |||
| 167 | 239 | flows.remove(&input.state) | |
| 168 | 240 | }.ok_or_else(|| ApiError::bad_request("Invalid or expired OAuth state token"))?; | |
| 169 | 241 | ||
| 242 | + | // The callback has been consumed; tear down its loopback server. | |
| 243 | + | { | |
| 244 | + | let mut servers = state.pending_oauth_servers.lock().unwrap_or_else(|e| e.into_inner()); | |
| 245 | + | servers.remove(&flow.port); | |
| 246 | + | } | |
| 247 | + | ||
| 170 | 248 | let token_manager = TokenManager::from_env(); | |
| 171 | 249 | let provider = token_manager | |
| 172 | 250 | .provider(&flow.provider_id) |
| @@ -182,6 +182,13 @@ pub async fn sync_start_auth( | |||
| 182 | 182 | }); | |
| 183 | 183 | } | |
| 184 | 184 | ||
| 185 | + | // Keep the callback server alive so the frontend can poll its result over | |
| 186 | + | // IPC (`poll_oauth_result`) instead of an unauthenticated HTTP endpoint. | |
| 187 | + | { | |
| 188 | + | let mut servers = state.pending_oauth_servers.lock().unwrap_or_else(|e| e.into_inner()); | |
| 189 | + | servers.insert(port, callback_server); | |
| 190 | + | } | |
| 191 | + | ||
| 185 | 192 | Ok(SyncAuthStartResponse { | |
| 186 | 193 | auth_url, | |
| 187 | 194 | state: csrf_state, | |
| @@ -204,6 +211,12 @@ pub async fn sync_complete_auth( | |||
| 204 | 211 | flows.remove(&input.state) | |
| 205 | 212 | }.ok_or_else(|| ApiError::bad_request("Invalid or expired OAuth state token"))?; | |
| 206 | 213 | ||
| 214 | + | // The callback has been consumed; tear down its loopback server. | |
| 215 | + | { | |
| 216 | + | let mut servers = state.pending_oauth_servers.lock().unwrap_or_else(|e| e.into_inner()); | |
| 217 | + | servers.remove(&flow.port); | |
| 218 | + | } | |
| 219 | + | ||
| 207 | 220 | let (user_id, app_id) = client | |
| 208 | 221 | .authenticate_with_code(&input.code, &flow.code_verifier, flow.port, "__internal__") | |
| 209 | 222 | .await |
| @@ -131,9 +131,13 @@ pub struct ImapProvider { | |||
| 131 | 131 | ||
| 132 | 132 | impl ImapProvider { | |
| 133 | 133 | pub fn new(account: &EmailAccount) -> Self { | |
| 134 | + | // Prefer the keychain; fall back to the legacy plaintext column only | |
| 135 | + | // for accounts created before credentials moved to secure storage. | |
| 136 | + | let password = crate::oauth::CredentialStore::get_password(account.id.into()) | |
| 137 | + | .unwrap_or_else(|| account.password.clone()); | |
| 134 | 138 | Self { | |
| 135 | - | imap_client: ImapClient::with_password(account, &account.password), | |
| 136 | - | smtp_client: SmtpClient::new(account), | |
| 139 | + | imap_client: ImapClient::with_password(account, &password), | |
| 140 | + | smtp_client: SmtpClient::with_password(account, &password), | |
| 137 | 141 | } | |
| 138 | 142 | } | |
| 139 | 143 | } |
| @@ -131,7 +131,6 @@ pub struct SmtpClient { | |||
| 131 | 131 | port: u16, | |
| 132 | 132 | auth: SmtpAuth, | |
| 133 | 133 | from_address: String, | |
| 134 | - | use_tls: bool, | |
| 135 | 134 | } | |
| 136 | 135 | ||
| 137 | 136 | impl SmtpClient { | |
| @@ -148,7 +147,6 @@ impl SmtpClient { | |||
| 148 | 147 | password: account.password.clone(), | |
| 149 | 148 | }, | |
| 150 | 149 | from_address: account.email_address.clone(), | |
| 151 | - | use_tls: account.use_tls, | |
| 152 | 150 | } | |
| 153 | 151 | } | |
| 154 | 152 | ||
| @@ -164,7 +162,6 @@ impl SmtpClient { | |||
| 164 | 162 | password: password.to_string(), | |
| 165 | 163 | }, | |
| 166 | 164 | from_address: account.email_address.clone(), | |
| 167 | - | use_tls: account.use_tls, | |
| 168 | 165 | } | |
| 169 | 166 | } | |
| 170 | 167 | ||
| @@ -174,7 +171,6 @@ impl SmtpClient { | |||
| 174 | 171 | port: u16, | |
| 175 | 172 | email: &str, | |
| 176 | 173 | access_token: &str, | |
| 177 | - | use_tls: bool, | |
| 178 | 174 | ) -> Self { | |
| 179 | 175 | Self { | |
| 180 | 176 | server: server.to_string(), | |
| @@ -184,47 +180,33 @@ impl SmtpClient { | |||
| 184 | 180 | access_token: access_token.to_string(), | |
| 185 | 181 | }, | |
| 186 | 182 | from_address: email.to_string(), | |
| 187 | - | use_tls, | |
| 188 | 183 | } | |
| 189 | 184 | } | |
| 190 | 185 | ||
| 186 | + | /// Builds an SMTP transport with a mandatory encryption floor. | |
| 187 | + | /// | |
| 188 | + | /// The transport always negotiates STARTTLS, so credentials and OAuth | |
| 189 | + | /// access tokens are never transmitted over an unencrypted connection. A | |
| 190 | + | /// server that does not offer STARTTLS fails the handshake rather than | |
| 191 | + | /// silently downgrading to cleartext (the previous `builder_dangerous` | |
| 192 | + | /// path, which leaked secrets whenever the account had TLS unticked). | |
| 191 | 193 | fn build_mailer(&self) -> Result<AsyncSmtpTransport<Tokio1Executor>, String> { | |
| 192 | - | match &self.auth { | |
| 193 | - | SmtpAuth::Password { username, password } => { | |
| 194 | - | let creds = Credentials::new(username.clone(), password.clone()); | |
| 195 | - | if self.use_tls { | |
| 196 | - | Ok(AsyncSmtpTransport::<Tokio1Executor>::starttls_relay(&self.server) | |
| 197 | - | .map_err(|e| format!("SMTP relay error: {}", e))? | |
| 198 | - | .credentials(creds) | |
| 199 | - | .port(self.port) | |
| 200 | - | .build()) | |
| 201 | - | } else { | |
| 202 | - | Ok(AsyncSmtpTransport::<Tokio1Executor>::builder_dangerous(&self.server) | |
| 203 | - | .credentials(creds) | |
| 204 | - | .port(self.port) | |
| 205 | - | .build()) | |
| 206 | - | } | |
| 207 | - | } | |
| 208 | - | SmtpAuth::XOAuth2 { email, access_token } => { | |
| 209 | - | // For XOAUTH2, we use the email as username and access_token as password | |
| 210 | - | // with the Xoauth2 mechanism | |
| 211 | - | let creds = Credentials::new(email.clone(), access_token.clone()); | |
| 212 | - | if self.use_tls { | |
| 213 | - | Ok(AsyncSmtpTransport::<Tokio1Executor>::starttls_relay(&self.server) | |
| 214 | - | .map_err(|e| format!("SMTP relay error: {}", e))? | |
| 215 | - | .credentials(creds) | |
| 216 | - | .authentication(vec![Mechanism::Xoauth2]) | |
| 217 | - | .port(self.port) | |
| 218 | - | .build()) | |
| 219 | - | } else { | |
| 220 | - | Ok(AsyncSmtpTransport::<Tokio1Executor>::builder_dangerous(&self.server) | |
| 221 | - | .credentials(creds) | |
| 222 | - | .authentication(vec![Mechanism::Xoauth2]) | |
| 223 | - | .port(self.port) | |
| 224 | - | .build()) | |
| 225 | - | } | |
| 226 | - | } | |
| 227 | - | } | |
| 194 | + | let builder = AsyncSmtpTransport::<Tokio1Executor>::starttls_relay(&self.server) | |
| 195 | + | .map_err(|e| format!("SMTP relay error: {}", e))? | |
| 196 | + | .port(self.port); | |
| 197 | + | ||
| 198 | + | let mailer = match &self.auth { | |
| 199 | + | SmtpAuth::Password { username, password } => builder | |
| 200 | + | .credentials(Credentials::new(username.clone(), password.clone())) | |
| 201 | + | .build(), | |
| 202 | + | SmtpAuth::XOAuth2 { email, access_token } => builder | |
| 203 | + | // For XOAUTH2 the email is the username and the access token is | |
| 204 | + | // the secret, negotiated with the Xoauth2 mechanism. | |
| 205 | + | .credentials(Credentials::new(email.clone(), access_token.clone())) | |
| 206 | + | .authentication(vec![Mechanism::Xoauth2]) | |
| 207 | + | .build(), | |
| 208 | + | }; | |
| 209 | + | Ok(mailer) | |
| 228 | 210 | } | |
| 229 | 211 | ||
| 230 | 212 | pub async fn send_message(&self, params: &SendParams<'_>) -> Result<String, String> { |
| @@ -207,6 +207,7 @@ macro_rules! all_commands { | |||
| 207 | 207 | // OAuth | |
| 208 | 208 | $crate::commands::list_oauth_providers, | |
| 209 | 209 | $crate::commands::start_oauth, | |
| 210 | + | $crate::commands::poll_oauth_result, | |
| 210 | 211 | $crate::commands::complete_oauth, | |
| 211 | 212 | $crate::commands::refresh_oauth_tokens, | |
| 212 | 213 | $crate::commands::disconnect_oauth, |
| @@ -39,7 +39,7 @@ pub struct CallbackError { | |||
| 39 | 39 | ||
| 40 | 40 | /// Stored callback data for polling. | |
| 41 | 41 | #[derive(Debug, Clone)] | |
| 42 | - | enum StoredCallback { | |
| 42 | + | pub enum StoredCallback { | |
| 43 | 43 | Pending, | |
| 44 | 44 | Success { code: String, state: String }, | |
| 45 | 45 | Error { error: String, description: Option<String> }, | |
| @@ -49,6 +49,9 @@ enum StoredCallback { | |||
| 49 | 49 | pub struct OAuthCallbackServer { | |
| 50 | 50 | port: u16, | |
| 51 | 51 | receiver: Receiver<Result<CallbackResult, CallbackError>>, | |
| 52 | + | /// Shared callback state, surfaced to the frontend through the trusted IPC | |
| 53 | + | /// layer rather than an unauthenticated local HTTP endpoint. | |
| 54 | + | stored: Arc<Mutex<StoredCallback>>, | |
| 52 | 55 | } | |
| 53 | 56 | ||
| 54 | 57 | impl OAuthCallbackServer { | |
| @@ -81,7 +84,7 @@ impl OAuthCallbackServer { | |||
| 81 | 84 | Self::run_server(listener, sender, stored_clone); | |
| 82 | 85 | }); | |
| 83 | 86 | ||
| 84 | - | Ok(Self { port, receiver }) | |
| 87 | + | Ok(Self { port, receiver, stored }) | |
| 85 | 88 | } | |
| 86 | 89 | ||
| 87 | 90 | /// Returns the port this server is listening on. | |
| @@ -89,6 +92,14 @@ impl OAuthCallbackServer { | |||
| 89 | 92 | self.port | |
| 90 | 93 | } | |
| 91 | 94 | ||
| 95 | + | /// Returns the current callback state for delivery over the trusted IPC layer. | |
| 96 | + | /// | |
| 97 | + | /// Used instead of an HTTP `/result` endpoint so the authorization code and | |
| 98 | + | /// state are never readable by other local processes on the loopback port. | |
| 99 | + | pub fn poll(&self) -> StoredCallback { | |
| 100 | + | self.stored.lock().unwrap_or_else(|e| e.into_inner()).clone() | |
| 101 | + | } | |
| 102 | + | ||
| 92 | 103 | /// Waits for the OAuth callback with a timeout. | |
| 93 | 104 | /// | |
| 94 | 105 | /// # Arguments | |
| @@ -172,32 +183,6 @@ impl OAuthCallbackServer { | |||
| 172 | 183 | .split_whitespace() | |
| 173 | 184 | .next()?; | |
| 174 | 185 | ||
| 175 | - | let path_only = path.split('?').next().unwrap_or(path); | |
| 176 | - | ||
| 177 | - | // Handle /result endpoint for polling | |
| 178 | - | if path_only == "/result" { | |
| 179 | - | let stored_guard = stored.lock().ok()?; | |
| 180 | - | let json = match &*stored_guard { | |
| 181 | - | StoredCallback::Pending => r#"{"status":"pending"}"#.to_string(), | |
| 182 | - | StoredCallback::Success { code, state } => { | |
| 183 | - | serde_json::json!({ | |
| 184 | - | "status": "success", | |
| 185 | - | "code": code, | |
| 186 | - | "state": state, | |
| 187 | - | }).to_string() | |
| 188 | - | } | |
| 189 | - | StoredCallback::Error { error, description } => { | |
| 190 | - | serde_json::json!({ | |
| 191 | - | "status": "error", | |
| 192 | - | "error": error, | |
| 193 | - | "description": description.as_deref().unwrap_or(""), | |
| 194 | - | }).to_string() | |
| 195 | - | } | |
| 196 | - | }; | |
| 197 | - | Self::send_json_response(&mut stream, &json); | |
| 198 | - | return None; | |
| 199 | - | } | |
| 200 | - | ||
| 201 | 186 | // Parse query parameters for the callback | |
| 202 | 187 | let query = path.split('?').nth(1).unwrap_or(""); | |
| 203 | 188 | let params: std::collections::HashMap<&str, &str> = query | |
| @@ -284,8 +269,11 @@ impl OAuthCallbackServer { | |||
| 284 | 269 | } | |
| 285 | 270 | ||
| 286 | 271 | fn send_response(stream: &mut TcpStream, status: &str, content_type: &str, body: &str) { | |
| 272 | + | // No CORS header: these pages are rendered by the user's external | |
| 273 | + | // browser after the redirect, and the callback result is delivered to | |
| 274 | + | // the app over IPC, so no cross-origin fetch needs to read this. | |
| 287 | 275 | let response = format!( | |
| 288 | - | "HTTP/1.1 {}\r\nContent-Type: {}\r\nContent-Length: {}\r\nAccess-Control-Allow-Origin: *\r\nConnection: close\r\n\r\n{}", | |
| 276 | + | "HTTP/1.1 {}\r\nContent-Type: {}\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{}", | |
| 289 | 277 | status, | |
| 290 | 278 | content_type, | |
| 291 | 279 | body.len(), | |
| @@ -294,10 +282,6 @@ impl OAuthCallbackServer { | |||
| 294 | 282 | let _ = stream.write_all(response.as_bytes()); | |
| 295 | 283 | let _ = stream.flush(); | |
| 296 | 284 | } | |
| 297 | - | ||
| 298 | - | fn send_json_response(stream: &mut TcpStream, json: &str) { | |
| 299 | - | Self::send_response(stream, "200 OK", "application/json", json); | |
| 300 | - | } | |
| 301 | 285 | } | |
| 302 | 286 | ||
| 303 | 287 | /// Simple URL decoding with proper multi-byte UTF-8 support. | |
| @@ -617,56 +601,64 @@ mod tests { | |||
| 617 | 601 | } | |
| 618 | 602 | ||
| 619 | 603 | #[test] | |
| 620 | - | fn server_result_endpoint_returns_pending_initially() { | |
| 604 | + | fn poll_returns_pending_initially() { | |
| 621 | 605 | let _lock = lock_server_tests(); | |
| 622 | 606 | let server = OAuthCallbackServer::start().unwrap(); | |
| 623 | - | let port = server.port(); | |
| 624 | - | ||
| 625 | - | let response = send_request(port, "/result"); | |
| 626 | - | let body = extract_body(&response); | |
| 627 | 607 | ||
| 628 | - | assert!(body.contains(r#""status":"pending"#)); | |
| 608 | + | assert!(matches!(server.poll(), StoredCallback::Pending)); | |
| 629 | 609 | } | |
| 630 | 610 | ||
| 631 | 611 | #[test] | |
| 632 | - | fn server_result_endpoint_returns_success_after_callback() { | |
| 612 | + | fn poll_returns_success_after_callback() { | |
| 633 | 613 | let _lock = lock_server_tests(); | |
| 634 | 614 | let server = OAuthCallbackServer::start().unwrap(); | |
| 635 | 615 | let port = server.port(); | |
| 636 | 616 | ||
| 637 | - | // First, trigger the callback | |
| 617 | + | // Trigger the callback via the browser-redirect path. | |
| 638 | 618 | send_request(port, "/?code=mycode&state=mystate"); | |
| 639 | 619 | ||
| 640 | - | // Small delay to let the server process | |
| 620 | + | // Small delay to let the server process. | |
| 641 | 621 | std::thread::sleep(Duration::from_millis(200)); | |
| 642 | 622 | ||
| 643 | - | // Then poll /result | |
| 644 | - | let response = send_request(port, "/result"); | |
| 645 | - | let body = extract_body(&response); | |
| 646 | - | ||
| 647 | - | assert!(body.contains(r#""status":"success"#)); | |
| 648 | - | assert!(body.contains(r#""code":"mycode"#)); | |
| 649 | - | assert!(body.contains(r#""state":"mystate"#)); | |
| 623 | + | // The result is surfaced through poll(), not an HTTP endpoint. | |
| 624 | + | match server.poll() { | |
| 625 | + | StoredCallback::Success { code, state } => { | |
| 626 | + | assert_eq!(code, "mycode"); | |
| 627 | + | assert_eq!(state, "mystate"); | |
| 628 | + | } | |
| 629 | + | other => panic!("expected success, got {:?}", other), | |
| 630 | + | } | |
| 650 | 631 | } | |
| 651 | 632 | ||
| 652 | 633 | #[test] | |
| 653 | - | fn server_result_endpoint_returns_error_after_error_callback() { | |
| 634 | + | fn poll_returns_error_after_error_callback() { | |
| 654 | 635 | let _lock = lock_server_tests(); | |
| 655 | 636 | let server = OAuthCallbackServer::start().unwrap(); | |
| 656 | 637 | let port = server.port(); | |
| 657 | 638 | ||
| 658 | - | // Trigger an error callback | |
| 639 | + | // Trigger an error callback. | |
| 659 | 640 | send_request(port, "/?error=invalid_grant&error_description=Expired"); | |
| 660 | 641 | ||
| 661 | 642 | std::thread::sleep(Duration::from_millis(200)); | |
| 662 | 643 | ||
| 663 | - | // Then poll /result | |
| 664 | - | let response = send_request(port, "/result"); | |
| 665 | - | let body = extract_body(&response); | |
| 644 | + | match server.poll() { | |
| 645 | + | StoredCallback::Error { error, description } => { | |
| 646 | + | assert_eq!(error, "invalid_grant"); | |
| 647 | + | assert_eq!(description.as_deref(), Some("Expired")); | |
| 648 | + | } | |
| 649 | + | other => panic!("expected error, got {:?}", other), | |
| 650 | + | } | |
| 651 | + | } | |
| 652 | + | ||
| 653 | + | #[test] | |
| 654 | + | fn callback_response_omits_cors_header() { | |
| 655 | + | let _lock = lock_server_tests(); | |
| 656 | + | let server = OAuthCallbackServer::start().unwrap(); | |
| 657 | + | let port = server.port(); | |
| 666 | 658 | ||
| 667 | - | assert!(body.contains(r#""status":"error"#)); | |
| 668 | - | assert!(body.contains(r#""error":"invalid_grant"#)); | |
| 669 | - | assert!(body.contains(r#""description":"Expired"#)); | |
| 659 | + | // The loopback server must not advertise a permissive CORS policy. | |
| 660 | + | let response = send_request(port, "/?code=c&state=s"); | |
| 661 | + | assert!(!response.to_ascii_lowercase().contains("access-control-allow-origin")); | |
| 670 | 662 | } | |
| 671 | 663 | ||
| 672 | 664 | #[test] |
| @@ -12,7 +12,7 @@ pub mod provider; | |||
| 12 | 12 | pub mod providers; | |
| 13 | 13 | pub mod token_manager; | |
| 14 | 14 | ||
| 15 | - | pub use callback_server::OAuthCallbackServer; | |
| 15 | + | pub use callback_server::{OAuthCallbackServer, StoredCallback}; | |
| 16 | 16 | pub use credentials::{CredentialStore, OAuthCredentials}; | |
| 17 | 17 | pub use provider::{OAuthProvider, OAuthProviderConfig, OAuthStartResult, TokenResult}; | |
| 18 | 18 | pub use providers::*; |
| @@ -57,6 +57,10 @@ pub struct AppState { | |||
| 57 | 57 | pub token_refresh_locks: Arc<Mutex<std::collections::HashMap<uuid::Uuid, Arc<TokioMutex<()>>>>>, | |
| 58 | 58 | /// Pending OAuth flows keyed by state token (CSRF + PKCE verifier stored server-side). | |
| 59 | 59 | pub pending_oauth_flows: Arc<Mutex<std::collections::HashMap<String, PendingOAuthFlow>>>, | |
| 60 | + | /// Live OAuth callback servers keyed by loopback port. The callback result is | |
| 61 | + | /// polled through the trusted IPC layer (`poll_oauth_result`) rather than an | |
| 62 | + | /// unauthenticated local HTTP endpoint. | |
| 63 | + | pub pending_oauth_servers: Arc<Mutex<std::collections::HashMap<u16, crate::oauth::OAuthCallbackServer>>>, | |
| 60 | 64 | pub data_dir: PathBuf, | |
| 61 | 65 | } | |
| 62 | 66 | ||
| @@ -164,6 +168,7 @@ impl AppState { | |||
| 164 | 168 | email_sync_locks: Arc::new(Mutex::new(std::collections::HashSet::new())), | |
| 165 | 169 | token_refresh_locks: Arc::new(Mutex::new(std::collections::HashMap::new())), | |
| 166 | 170 | pending_oauth_flows: Arc::new(Mutex::new(std::collections::HashMap::new())), | |
| 171 | + | pending_oauth_servers: Arc::new(Mutex::new(std::collections::HashMap::new())), | |
| 167 | 172 | data_dir: app_data_dir, | |
| 168 | 173 | }) | |
| 169 | 174 | } |
| @@ -77,6 +77,7 @@ pub async fn setup_test_state() -> (Arc<AppState>, UserId) { | |||
| 77 | 77 | email_sync_locks: Arc::new(std::sync::Mutex::new(std::collections::HashSet::new())), | |
| 78 | 78 | token_refresh_locks: Arc::new(std::sync::Mutex::new(std::collections::HashMap::new())), | |
| 79 | 79 | pending_oauth_flows: Arc::new(std::sync::Mutex::new(std::collections::HashMap::new())), | |
| 80 | + | pending_oauth_servers: Arc::new(std::sync::Mutex::new(std::collections::HashMap::new())), | |
| 80 | 81 | data_dir: std::path::PathBuf::from("/tmp/goingson-test"), | |
| 81 | 82 | }; | |
| 82 | 83 |
| @@ -23,7 +23,7 @@ | |||
| 23 | 23 | } | |
| 24 | 24 | ], | |
| 25 | 25 | "security": { | |
| 26 | - | "csp": null | |
| 26 | + | "csp": "default-src 'self'; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline'; img-src 'self' data: asset: http://asset.localhost; font-src 'self'; connect-src 'self' ipc: http://ipc.localhost; object-src 'none'; base-uri 'self'; frame-ancestors 'none'; form-action 'self'" | |
| 27 | 27 | } | |
| 28 | 28 | }, | |
| 29 | 29 | "bundle": { |