Skip to content

Commit a1636e1

Browse files
committed
fix(port): Phase 3 — Data integrity regressions
Restore file-key credential migration at startup, WorkspaceNeedsBinding collision_client_id alongside space_locked, and OAuth refresh dedup singleton. Signed-off-by: crimsonsunset <jsangio1@gmail.com>
1 parent 12a2acd commit a1636e1

11 files changed

Lines changed: 239 additions & 2 deletions

File tree

apps/desktop/src-tauri/src/commands/gateway.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -792,6 +792,7 @@ fn map_domain_event_to_ui(event: &DomainEvent) -> (&'static str, serde_json::Val
792792
session_id,
793793
space_id,
794794
workspace_root,
795+
collision_client_id,
795796
space_locked,
796797
} => (
797798
"workspace-needs-binding",
@@ -800,6 +801,7 @@ fn map_domain_event_to_ui(event: &DomainEvent) -> (&'static str, serde_json::Val
800801
"session_id": session_id,
801802
"space_id": space_id,
802803
"workspace_root": workspace_root,
804+
"collision_client_id": collision_client_id,
803805
"space_locked": space_locked,
804806
}),
805807
),

apps/desktop/src-tauri/src/state/mod.rs

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ use mcpmux_storage::{
2121
use std::path::PathBuf;
2222
use std::sync::Arc;
2323
use tokio::sync::Mutex;
24-
use tracing::info;
24+
use tracing::{info, warn};
2525

2626
/// Global application state accessible from commands.
2727
pub struct AppState {
@@ -89,6 +89,14 @@ impl AppState {
8989
info!("Opening database at {:?}", db_path);
9090

9191
let db = Database::open(&db_path)?;
92+
93+
// Re-encrypt any credentials written under the file fallback key (macOS keychain prompt dismissed)
94+
if let Err(e) =
95+
mcpmux_storage::migrate_file_key_encrypted_fields(&db, &data_dir, &encryptor)
96+
{
97+
warn!("File-key credential migration skipped: {}", e);
98+
}
99+
92100
let db = Arc::new(Mutex::new(db));
93101

94102
// Initialize repositories

apps/desktop/src/lib/api/gateway.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -335,12 +335,17 @@ export interface RefreshResult {
335335
refresh_failed: number;
336336
}
337337

338+
let refreshOAuthOnStartupPromise: Promise<RefreshResult> | null = null;
339+
338340
/**
339341
* Refresh OAuth tokens on startup for all installed HTTP servers.
340342
* This should be called during app initialization before connecting to servers.
341343
*/
342344
export async function refreshOAuthTokensOnStartup(): Promise<RefreshResult> {
343-
return apiCall('refresh_oauth_tokens_on_startup');
345+
if (!refreshOAuthOnStartupPromise) {
346+
refreshOAuthOnStartupPromise = apiCall<RefreshResult>('refresh_oauth_tokens_on_startup');
347+
}
348+
return refreshOAuthOnStartupPromise;
344349
}
345350

346351
/**

apps/desktop/src/lib/backend/events/useWorkspaceEvents.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,8 @@ export interface WorkspaceNeedsBindingPayload {
3434
workspace_root: string;
3535
/** Set when another client's scoped binding blocked the global route. */
3636
collision_client_id?: string | null;
37+
/** When true, the Space picker is locked to `space_id`. */
38+
space_locked?: boolean;
3739
}
3840

3941
/** Payload map for type-safe subscriptions. */

crates/mcpmux-core/src/domain/event.rs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -378,6 +378,11 @@ pub enum DomainEvent {
378378
session_id: String,
379379
space_id: Uuid,
380380
workspace_root: String,
381+
/// When set, a scoped binding for another OAuth client blocked the
382+
/// global binding on this path — the UI should offer to create a
383+
/// client-scoped binding for `client_id`.
384+
#[serde(default, skip_serializing_if = "Option::is_none")]
385+
collision_client_id: Option<String>,
381386
/// The folder is scoped to `space_id` by a Space base directory, so the
382387
/// mapping popup locks its Space field to it (the user only picks the
383388
/// FeatureSet). `false` for an ordinary unmapped folder, where the user
@@ -754,6 +759,7 @@ mod tests {
754759
session_id: "sess-1".to_string(),
755760
space_id: Uuid::new_v4(),
756761
workspace_root: "/proj/foo".to_string(),
762+
collision_client_id: None,
757763
space_locked: false,
758764
};
759765
assert!(!e.affects_mcp_capabilities());
@@ -780,10 +786,12 @@ mod tests {
780786
session_id: "s".into(),
781787
space_id: Uuid::nil(),
782788
workspace_root: "/r".into(),
789+
collision_client_id: Some("other-client".into()),
783790
space_locked: true,
784791
};
785792
let json = serde_json::to_string(&needs).unwrap();
786793
assert!(json.contains("\"type\":\"workspace_needs_binding\""));
787794
assert!(json.contains("\"session_id\":\"s\""));
795+
assert!(json.contains("\"collision_client_id\":\"other-client\""));
788796
}
789797
}

crates/mcpmux-gateway/src/admin/ui_events.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -406,6 +406,7 @@ pub fn map_domain_event_to_ui(event: &DomainEvent) -> (&'static str, Value) {
406406
session_id,
407407
space_id,
408408
workspace_root,
409+
collision_client_id,
409410
space_locked,
410411
} => (
411412
"workspace-needs-binding",
@@ -414,6 +415,7 @@ pub fn map_domain_event_to_ui(event: &DomainEvent) -> (&'static str, Value) {
414415
"session_id": session_id,
415416
"space_id": space_id,
416417
"workspace_root": workspace_root,
418+
"collision_client_id": collision_client_id,
417419
"space_locked": space_locked,
418420
}),
419421
),

crates/mcpmux-gateway/src/mcp/handler.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,7 @@ impl McpMuxGatewayHandler {
164164
session_id: sid.to_string(),
165165
space_id,
166166
workspace_root: root.to_string(),
167+
collision_client_id: resolved.collision_client_id.clone(),
167168
space_locked,
168169
},
169170
);

crates/mcpmux-gateway/src/services/feature_set_resolver.rs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,9 @@ pub struct ResolvedFeatureSet {
150150
/// Resolved Space id. Used by the routing layer when filtering features.
151151
pub space_id: Option<Uuid>,
152152
pub source: ResolutionSource,
153+
/// When `source == Deny` because a global binding was blocked by another
154+
/// client's scoped binding on the same path, holds that client's id.
155+
pub collision_client_id: Option<String>,
153156
}
154157

155158
impl ResolvedFeatureSet {
@@ -268,6 +271,7 @@ impl FeatureSetResolverService {
268271
feature_set_ids: vec![fs.id],
269272
space_id: Some(space_id),
270273
source: ResolutionSource::SpaceDefault,
274+
collision_client_id: None,
271275
});
272276
}
273277
debug!(
@@ -278,6 +282,7 @@ impl FeatureSetResolverService {
278282
feature_set_ids: vec![],
279283
space_id: Some(space_id),
280284
source: ResolutionSource::Deny,
285+
collision_client_id: None,
281286
})
282287
}
283288

@@ -307,6 +312,7 @@ impl FeatureSetResolverService {
307312
feature_set_ids: vec![],
308313
space_id: None,
309314
source: ResolutionSource::Deny,
315+
collision_client_id: None,
310316
});
311317
}
312318
};
@@ -360,6 +366,7 @@ impl FeatureSetResolverService {
360366
feature_set_ids: binding.feature_set_ids,
361367
space_id: Some(binding.space_id),
362368
source: ResolutionSource::WorkspaceBinding,
369+
collision_client_id: None,
363370
});
364371
}
365372
// Tier 1b: had roots, no binding. The folder is unmapped, so
@@ -412,6 +419,7 @@ impl FeatureSetResolverService {
412419
feature_set_ids: vec![],
413420
space_id: Some(default_space_id),
414421
source: ResolutionSource::PendingRoots,
422+
collision_client_id: None,
415423
});
416424
}
417425
// Grace lapsed with no root in sight — settle on the Space
@@ -453,6 +461,7 @@ impl FeatureSetResolverService {
453461
feature_set_ids: grants,
454462
space_id: Some(default_space_id),
455463
source: ResolutionSource::ClientGrant,
464+
collision_client_id: None,
456465
});
457466
}
458467
}
Lines changed: 197 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,197 @@
1+
//! Re-encrypt data that was written under the file-based fallback key.
2+
//!
3+
//! On macOS/Linux, `create_key_provider` can briefly fall back to `keys/master.key`
4+
//! when the OS keychain prompt is dismissed. Credentials encrypted during that
5+
//! window cannot be read once the keychain key is used again. This module detects
6+
//! those rows and re-encrypts them with the active key.
7+
8+
use std::path::Path;
9+
10+
#[cfg(not(windows))]
11+
use anyhow::Context;
12+
use anyhow::Result;
13+
#[cfg(not(windows))]
14+
use tracing::info;
15+
16+
use crate::crypto::FieldEncryptor;
17+
#[cfg(not(windows))]
18+
use crate::keychain::MasterKeyProvider;
19+
#[cfg(not(windows))]
20+
use crate::keychain_file::FileKeyProvider;
21+
use crate::Database;
22+
23+
/// Re-encrypt credential and installed-server fields that were encrypted with the
24+
/// legacy file fallback key so they work under the active keychain key.
25+
#[cfg(not(windows))]
26+
pub fn migrate_file_key_encrypted_fields(
27+
db: &Database,
28+
data_dir: &Path,
29+
active_encryptor: &FieldEncryptor,
30+
) -> Result<u32> {
31+
let file_provider = FileKeyProvider::new(data_dir)?;
32+
if !file_provider.key_exists() {
33+
return Ok(0);
34+
}
35+
36+
let file_key = file_provider.get_or_create_key()?;
37+
let file_encryptor = FieldEncryptor::new(&file_key)?;
38+
39+
let mut migrated = 0u32;
40+
migrated += migrate_credentials(db, active_encryptor, &file_encryptor)?;
41+
migrated += migrate_installed_server_inputs(db, active_encryptor, &file_encryptor)?;
42+
43+
if migrated > 0 {
44+
info!(
45+
"Re-encrypted {} credential/input field(s) from file fallback key to active master key",
46+
migrated
47+
);
48+
}
49+
50+
Ok(migrated)
51+
}
52+
53+
/// Windows uses DPAPI file storage only; file-keychain fallback migration is Unix-only.
54+
#[cfg(windows)]
55+
pub fn migrate_file_key_encrypted_fields(
56+
_db: &Database,
57+
_data_dir: &Path,
58+
_active_encryptor: &FieldEncryptor,
59+
) -> Result<u32> {
60+
Ok(0)
61+
}
62+
63+
/// Migrate encrypted credential values.
64+
#[cfg(not(windows))]
65+
fn migrate_credentials(
66+
db: &Database,
67+
active: &FieldEncryptor,
68+
file: &FieldEncryptor,
69+
) -> Result<u32> {
70+
let conn = db.connection();
71+
let mut stmt = conn.prepare(
72+
"SELECT rowid, credential_value FROM credentials WHERE credential_value IS NOT NULL",
73+
)?;
74+
let rows: Vec<(i64, String)> = stmt
75+
.query_map([], |row| Ok((row.get(0)?, row.get(1)?)))?
76+
.filter_map(|r| r.ok())
77+
.collect();
78+
79+
let mut migrated = 0u32;
80+
for (rowid, value) in rows {
81+
if active.decrypt(&value).is_ok() {
82+
continue;
83+
}
84+
let plaintext = file
85+
.decrypt(&value)
86+
.with_context(|| format!("credential rowid={rowid} is not readable with either key"))?;
87+
let reencrypted = active
88+
.encrypt(&plaintext)
89+
.context("failed to re-encrypt credential with active key")?;
90+
conn.execute(
91+
"UPDATE credentials SET credential_value = ?1 WHERE rowid = ?2",
92+
rusqlite::params![reencrypted, rowid],
93+
)?;
94+
migrated += 1;
95+
}
96+
Ok(migrated)
97+
}
98+
99+
/// Migrate encrypted installed-server input_values blobs.
100+
#[cfg(not(windows))]
101+
fn migrate_installed_server_inputs(
102+
db: &Database,
103+
active: &FieldEncryptor,
104+
file: &FieldEncryptor,
105+
) -> Result<u32> {
106+
let conn = db.connection();
107+
let mut stmt = conn.prepare(
108+
"SELECT rowid, input_values FROM installed_servers WHERE input_values IS NOT NULL AND input_values != ''",
109+
)?;
110+
let rows: Vec<(i64, String)> = stmt
111+
.query_map([], |row| Ok((row.get(0)?, row.get(1)?)))?
112+
.filter_map(|r| r.ok())
113+
.collect();
114+
115+
let mut migrated = 0u32;
116+
for (rowid, value) in rows {
117+
if active.decrypt(&value).is_ok() {
118+
continue;
119+
}
120+
let plaintext = match file.decrypt(&value) {
121+
Ok(p) => p,
122+
Err(_) => continue,
123+
};
124+
let reencrypted = active
125+
.encrypt(&plaintext)
126+
.context("failed to re-encrypt input_values with active key")?;
127+
conn.execute(
128+
"UPDATE installed_servers SET input_values = ?1 WHERE rowid = ?2",
129+
rusqlite::params![reencrypted, rowid],
130+
)?;
131+
migrated += 1;
132+
}
133+
Ok(migrated)
134+
}
135+
136+
#[cfg(all(test, not(windows)))]
137+
mod tests {
138+
use super::*;
139+
use crate::crypto::generate_master_key;
140+
use chrono::Utc;
141+
use uuid::Uuid;
142+
143+
#[test]
144+
fn migrate_file_key_credentials_to_active_key() {
145+
let tmp = tempfile::tempdir().unwrap();
146+
let data_dir = tmp.path();
147+
148+
let file_provider = FileKeyProvider::new(data_dir).unwrap();
149+
let file_key = file_provider.get_or_create_key().unwrap();
150+
let file_encryptor = FieldEncryptor::new(&file_key).unwrap();
151+
152+
let active_key = generate_master_key().unwrap();
153+
let active_encryptor = FieldEncryptor::new(&active_key).unwrap();
154+
155+
let db_path = data_dir.join("mcpmux.db");
156+
let db = Database::open(&db_path).unwrap();
157+
158+
let space_id = Uuid::new_v4();
159+
let conn = db.connection();
160+
conn.execute(
161+
"INSERT INTO spaces (id, name, created_at, updated_at) VALUES (?1, 'test', ?2, ?2)",
162+
rusqlite::params![space_id.to_string(), Utc::now().to_rfc3339()],
163+
)
164+
.unwrap();
165+
166+
let token = "test-oauth-token-value";
167+
let encrypted = file_encryptor.encrypt(token).unwrap();
168+
conn.execute(
169+
"INSERT INTO credentials (space_id, server_id, credential_type, credential_value, created_at, updated_at)
170+
VALUES (?1, 'demo', 'access_token', ?2, ?3, ?3)",
171+
rusqlite::params![space_id.to_string(), encrypted, Utc::now().to_rfc3339()],
172+
)
173+
.unwrap();
174+
175+
let stored_before: String = conn
176+
.query_row(
177+
"SELECT credential_value FROM credentials WHERE server_id = 'demo'",
178+
[],
179+
|r| r.get(0),
180+
)
181+
.unwrap();
182+
assert!(active_encryptor.decrypt(&stored_before).is_err());
183+
184+
let migrated = migrate_file_key_encrypted_fields(&db, data_dir, &active_encryptor).unwrap();
185+
assert_eq!(migrated, 1);
186+
187+
let stored_after: String = conn
188+
.query_row(
189+
"SELECT credential_value FROM credentials WHERE server_id = 'demo'",
190+
[],
191+
|r| r.get(0),
192+
)
193+
.unwrap();
194+
assert_eq!(active_encryptor.decrypt(&stored_after).unwrap(), token);
195+
assert!(file_encryptor.decrypt(&stored_after).is_err());
196+
}
197+
}

crates/mcpmux-storage/src/lib.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@
5353
5454
pub mod crypto;
5555
mod database;
56+
mod key_migration;
5657
pub mod keychain;
5758
#[cfg(windows)]
5859
pub mod keychain_dpapi;
@@ -62,6 +63,7 @@ mod repositories;
6263

6364
pub use crypto::{generate_master_key, FieldEncryptor, KEY_SIZE};
6465
pub use database::Database;
66+
pub use key_migration::migrate_file_key_encrypted_fields;
6567
pub use keychain::{
6668
generate_jwt_secret, JwtSecretProvider, KeychainJwtSecretProvider, KeychainKeyProvider,
6769
MasterKeyProvider, JWT_SECRET_SIZE,

0 commit comments

Comments
 (0)