fix(sync): enforce private profile boundaries
This commit is contained in:
@@ -251,7 +251,10 @@ impl BrowserCore {
|
||||
}
|
||||
self.bookmarks
|
||||
.iter()
|
||||
.filter(|bookmark| self.profile_allows_cloud_sync(bookmark.profile_id()))
|
||||
.filter(|bookmark| {
|
||||
self.profile_allows_cloud_sync(bookmark.profile_id())
|
||||
&& self.space_allows_sync(bookmark.space_id())
|
||||
})
|
||||
.collect()
|
||||
}
|
||||
|
||||
|
||||
@@ -90,19 +90,20 @@ impl BrowserCore {
|
||||
|
||||
#[must_use]
|
||||
pub fn cloud_sync_upload_enabled(&self) -> bool {
|
||||
matches!(
|
||||
self.sync_connection_state,
|
||||
SyncConnectionState::SignedIn
|
||||
| SyncConnectionState::AwaitingDeviceApproval
|
||||
| SyncConnectionState::SyncReady { .. }
|
||||
| SyncConnectionState::SyncError { .. }
|
||||
)
|
||||
self.active_profile_allows_sync()
|
||||
&& matches!(
|
||||
self.sync_connection_state,
|
||||
SyncConnectionState::SignedIn
|
||||
| SyncConnectionState::AwaitingDeviceApproval
|
||||
| SyncConnectionState::SyncReady { .. }
|
||||
| SyncConnectionState::SyncError { .. }
|
||||
)
|
||||
}
|
||||
|
||||
pub(crate) fn sync_space_name_for(&self, space_id: &SpaceId) -> Option<String> {
|
||||
self.spaces
|
||||
.iter()
|
||||
.find(|space| space.id() == space_id)
|
||||
.find(|space| space.id() == space_id && self.space_allows_sync(space.id()))
|
||||
.map(|space| space.name().to_string())
|
||||
}
|
||||
|
||||
@@ -180,7 +181,11 @@ impl BrowserCore {
|
||||
}
|
||||
self.tabs
|
||||
.iter()
|
||||
.filter(|tab| tab.sync_enabled() && self.profile_allows_cloud_sync(tab.profile_id()))
|
||||
.filter(|tab| {
|
||||
tab.sync_enabled()
|
||||
&& self.profile_allows_cloud_sync(tab.profile_id())
|
||||
&& self.space_allows_sync(tab.space_id())
|
||||
})
|
||||
.collect()
|
||||
}
|
||||
|
||||
@@ -188,7 +193,7 @@ impl BrowserCore {
|
||||
if self.sync_object_policy(SyncObjectKind::Spaces) == SyncObjectPolicy::Paused {
|
||||
return Vec::new();
|
||||
}
|
||||
self.spaces.iter().collect()
|
||||
self.spaces.iter().filter(|space| self.space_allows_sync(space.id())).collect()
|
||||
}
|
||||
|
||||
pub(super) fn apply_space_sync_record(
|
||||
@@ -200,11 +205,15 @@ impl BrowserCore {
|
||||
let space_id = parse_space_id(&record.id)?;
|
||||
let default_profile_id = self.sync_profile_id(&record.default_profile_id, context)?;
|
||||
let archive_policy = ArchivePolicy::from(record.archive_policy.clone());
|
||||
let existing_index =
|
||||
self.spaces.iter().position(|space| space.id() == &space_id).or_else(|| {
|
||||
self.spaces
|
||||
.iter()
|
||||
.position(|space| space.name().eq_ignore_ascii_case(record.name.trim()))
|
||||
let existing_index = self
|
||||
.spaces
|
||||
.iter()
|
||||
.position(|space| space.id() == &space_id && self.space_allows_sync(space.id()))
|
||||
.or_else(|| {
|
||||
self.spaces.iter().position(|space| {
|
||||
space.name().eq_ignore_ascii_case(record.name.trim())
|
||||
&& self.space_allows_sync(space.id())
|
||||
})
|
||||
});
|
||||
|
||||
match existing_index {
|
||||
@@ -382,20 +391,20 @@ impl BrowserCore {
|
||||
context: &SyncSnapshotApplyContext,
|
||||
) -> Result<ProfileId, SyncClientError> {
|
||||
let profile_id = ProfileId::parse(raw).map_err(snapshot_schema_error)?;
|
||||
if let Some(local_profile_id) = context.profile_alias(&profile_id) {
|
||||
let local_profile_id = context
|
||||
.profile_alias(&profile_id)
|
||||
.or_else(|| {
|
||||
self.profiles
|
||||
.iter()
|
||||
.any(|profile| profile.id() == &profile_id)
|
||||
.then_some(profile_id)
|
||||
})
|
||||
.unwrap_or_else(|| self.active_profile_id.clone());
|
||||
if self.profile_allows_sync(&local_profile_id) {
|
||||
return Ok(local_profile_id);
|
||||
}
|
||||
if self.profiles.iter().any(|profile| profile.id() == &profile_id) {
|
||||
return Ok(profile_id);
|
||||
}
|
||||
Ok(self.active_profile_id.clone())
|
||||
}
|
||||
|
||||
pub(super) fn profile_allows_cloud_sync(&self, profile_id: &ProfileId) -> bool {
|
||||
self.profiles.iter().any(|profile| {
|
||||
profile.id() == profile_id
|
||||
&& profile.allows_sync()
|
||||
&& profile.sync_policy() == ely_domain::ProfileSyncPolicy::Enabled
|
||||
Err(SyncClientError::SyncPolicy {
|
||||
reason: "sync record targets a private profile".to_string(),
|
||||
})
|
||||
}
|
||||
|
||||
@@ -405,16 +414,22 @@ impl BrowserCore {
|
||||
space_name: Option<&str>,
|
||||
) -> Result<SpaceId, SyncClientError> {
|
||||
let space_id = SpaceId::parse(raw).map_err(snapshot_schema_error)?;
|
||||
if self.spaces.iter().any(|space| space.id() == &space_id) {
|
||||
if self.space_allows_sync(&space_id) {
|
||||
return Ok(space_id);
|
||||
}
|
||||
if let Some(space_name) = space_name
|
||||
&& let Some(space) =
|
||||
self.spaces.iter().find(|space| space.name().eq_ignore_ascii_case(space_name))
|
||||
&& let Some(space) = self.spaces.iter().find(|space| {
|
||||
space.name().eq_ignore_ascii_case(space_name) && self.space_allows_sync(space.id())
|
||||
})
|
||||
{
|
||||
return Ok(space.id().clone());
|
||||
}
|
||||
Ok(self.active_space_id.clone())
|
||||
if self.space_allows_sync(&self.active_space_id) {
|
||||
return Ok(self.active_space_id.clone());
|
||||
}
|
||||
Err(SyncClientError::SyncPolicy {
|
||||
reason: "sync record targets a private space".to_string(),
|
||||
})
|
||||
}
|
||||
|
||||
fn ensure_synced_tab_indexes(
|
||||
|
||||
@@ -16,7 +16,10 @@ impl BrowserCore {
|
||||
}
|
||||
self.history_entries
|
||||
.iter()
|
||||
.filter(|entry| self.profile_allows_cloud_sync(entry.profile_id()))
|
||||
.filter(|entry| {
|
||||
self.profile_allows_cloud_sync(entry.profile_id())
|
||||
&& self.space_allows_sync(entry.space_id())
|
||||
})
|
||||
.collect()
|
||||
}
|
||||
|
||||
|
||||
@@ -17,7 +17,13 @@ impl BrowserCore {
|
||||
if self.sync_object_policy(SyncObjectKind::Notes) == SyncObjectPolicy::Paused {
|
||||
return Vec::new();
|
||||
}
|
||||
self.notes.iter().filter(|note| self.profile_allows_cloud_sync(note.profile_id())).collect()
|
||||
self.notes
|
||||
.iter()
|
||||
.filter(|note| {
|
||||
self.profile_allows_cloud_sync(note.profile_id())
|
||||
&& self.space_allows_sync(note.space_id())
|
||||
})
|
||||
.collect()
|
||||
}
|
||||
|
||||
pub(super) fn apply_note_sync_record(
|
||||
|
||||
@@ -1,10 +1,37 @@
|
||||
use ely_domain::{Profile, ProfileId, ProfileKind, SyncObjectKind, SyncObjectPolicy};
|
||||
use ely_domain::{Profile, ProfileId, ProfileKind, SpaceId, SyncObjectKind, SyncObjectPolicy};
|
||||
use ely_sync_client::SyncClientError;
|
||||
|
||||
use super::{BrowserCore, sync::snapshot_schema_error, sync_context::SyncSnapshotApplyContext};
|
||||
use crate::{sync_engine::SyncSnapshotApplySummary, sync_records::ProfileSyncRecord};
|
||||
|
||||
impl BrowserCore {
|
||||
#[must_use]
|
||||
pub fn active_profile_allows_sync(&self) -> bool {
|
||||
self.profile_allows_sync(&self.active_profile_id)
|
||||
}
|
||||
|
||||
pub(super) fn profile_allows_sync(&self, profile_id: &ProfileId) -> bool {
|
||||
self.profiles
|
||||
.iter()
|
||||
.find(|profile| profile.id() == profile_id)
|
||||
.is_some_and(|profile| profile.allows_sync())
|
||||
}
|
||||
|
||||
pub(super) fn profile_allows_cloud_sync(&self, profile_id: &ProfileId) -> bool {
|
||||
self.profiles.iter().any(|profile| {
|
||||
profile.id() == profile_id
|
||||
&& profile.allows_sync()
|
||||
&& profile.sync_policy() == ely_domain::ProfileSyncPolicy::Enabled
|
||||
})
|
||||
}
|
||||
|
||||
pub(super) fn space_allows_sync(&self, space_id: &SpaceId) -> bool {
|
||||
self.spaces
|
||||
.iter()
|
||||
.find(|space| space.id() == space_id)
|
||||
.is_some_and(|space| self.profile_allows_sync(space.default_profile_id()))
|
||||
}
|
||||
|
||||
pub(crate) fn visible_profiles_for_sync(&self) -> Vec<&Profile> {
|
||||
if self.sync_object_policy(SyncObjectKind::Profiles) == SyncObjectPolicy::Paused {
|
||||
return Vec::new();
|
||||
@@ -21,8 +48,9 @@ impl BrowserCore {
|
||||
let profile_id = ProfileId::parse(&record.id).map_err(snapshot_schema_error)?;
|
||||
let kind = ProfileKind::from(record.kind);
|
||||
if kind == ProfileKind::Private {
|
||||
summary.record_skipped();
|
||||
return Ok(());
|
||||
return Err(SyncClientError::SyncPolicy {
|
||||
reason: "snapshot contains a private profile".to_string(),
|
||||
});
|
||||
}
|
||||
let name = record.name.trim().to_string();
|
||||
|
||||
|
||||
@@ -19,7 +19,10 @@ impl BrowserCore {
|
||||
}
|
||||
self.reading_list
|
||||
.iter()
|
||||
.filter(|entry| self.profile_allows_cloud_sync(entry.profile_id()))
|
||||
.filter(|entry| {
|
||||
self.profile_allows_cloud_sync(entry.profile_id())
|
||||
&& self.space_allows_sync(entry.space_id())
|
||||
})
|
||||
.collect()
|
||||
}
|
||||
|
||||
|
||||
@@ -291,6 +291,7 @@ impl BrowserCore {
|
||||
/// UI thread does the (synchronous, cheap) serialization before
|
||||
/// handing bytes off to the worker thread.
|
||||
pub fn build_sync_snapshot_bytes(&self) -> Result<Vec<u8>, SyncClientError> {
|
||||
self.ensure_active_profile_allows_sync()?;
|
||||
let body = SyncSnapshotBody::from_core(self);
|
||||
serde_json::to_vec(&body).map_err(|error| SyncClientError::Json {
|
||||
endpoint: "snapshot".to_string(),
|
||||
@@ -302,6 +303,7 @@ impl BrowserCore {
|
||||
&mut self,
|
||||
bytes: &[u8],
|
||||
) -> Result<SyncSnapshotApplySummary, SyncClientError> {
|
||||
self.ensure_active_profile_allows_sync()?;
|
||||
let body: SyncSnapshotBody = serde_json::from_slice(bytes).map_err(|error| {
|
||||
SyncClientError::Json { endpoint: "snapshot".to_string(), source: error }
|
||||
})?;
|
||||
@@ -313,4 +315,11 @@ impl BrowserCore {
|
||||
}
|
||||
self.apply_sync_snapshot_body(body)
|
||||
}
|
||||
|
||||
fn ensure_active_profile_allows_sync(&self) -> Result<(), SyncClientError> {
|
||||
if self.active_profile_allows_sync() {
|
||||
return Ok(());
|
||||
}
|
||||
Err(SyncClientError::SyncPolicy { reason: "active profile is private".to_string() })
|
||||
}
|
||||
}
|
||||
|
||||
@@ -3,8 +3,9 @@ use std::error::Error;
|
||||
use ely_browser_core::{BrowserCore, InitialBrowserConfig};
|
||||
use ely_domain::{
|
||||
ProfileKind, ProfileSyncPolicy, SiteOrigin, SitePermissionDecision, SitePermissionFeature,
|
||||
UrlText,
|
||||
SyncConnectionState, UrlText,
|
||||
};
|
||||
use ely_sync_client::SyncClientError;
|
||||
|
||||
#[test]
|
||||
fn sync_snapshot_imports_remote_profiles_before_spaces() -> Result<(), Box<dyn Error>> {
|
||||
@@ -124,6 +125,7 @@ fn standard_profile_sync_preserves_a_same_named_private_profile() -> Result<(),
|
||||
let mut target = BrowserCore::new(InitialBrowserConfig::private_window()?)?;
|
||||
let private_profile_id = target.snapshot()?.active_profile_id;
|
||||
target.navigate_active_tab(UrlText::parse("https://private.example/secret")?)?;
|
||||
target.create_profile("Local", 0x26251e, ProfileKind::Standard)?;
|
||||
target.apply_sync_snapshot_bytes(&bytes)?;
|
||||
let snapshot = target.snapshot()?;
|
||||
|
||||
@@ -135,8 +137,11 @@ fn standard_profile_sync_preserves_a_same_named_private_profile() -> Result<(),
|
||||
&& profile.name() == "Private"
|
||||
&& profile.kind() == &ProfileKind::Standard
|
||||
}));
|
||||
let outbound = String::from_utf8(target.build_sync_snapshot_bytes()?)?;
|
||||
let outbound_bytes = target.build_sync_snapshot_bytes()?;
|
||||
let outbound = String::from_utf8(outbound_bytes.clone())?;
|
||||
assert!(!outbound.contains("https://private.example/secret"));
|
||||
assert!(!outbound.contains(private_profile_id.as_str()));
|
||||
assert!(!snapshot_contains_space_named(&outbound_bytes, "Private")?);
|
||||
assert!(outbound.contains("https://example.com/remote"));
|
||||
Ok(())
|
||||
}
|
||||
@@ -144,8 +149,17 @@ fn standard_profile_sync_preserves_a_same_named_private_profile() -> Result<(),
|
||||
#[test]
|
||||
fn standard_profile_sync_remaps_a_private_profile_id_collision() -> Result<(), Box<dyn Error>> {
|
||||
let mut target = BrowserCore::new(InitialBrowserConfig::private_window()?)?;
|
||||
let private_profile_id = target.snapshot()?.active_profile_id;
|
||||
let private_snapshot = target.snapshot()?;
|
||||
let private_profile_id = private_snapshot.active_profile_id;
|
||||
let private_space_id = private_snapshot.active_space_id;
|
||||
target.navigate_active_tab(UrlText::parse("https://private.example/id-secret")?)?;
|
||||
target.create_profile("Local", 0x26251e, ProfileKind::Standard)?;
|
||||
target.navigate_active_tab(UrlText::parse(
|
||||
"https://private-space.example/standard-profile-secret",
|
||||
)?)?;
|
||||
target.bookmark_active_tab()?;
|
||||
target.save_active_url_note("private space note")?;
|
||||
target.save_active_tab_to_reading_list()?;
|
||||
|
||||
let mut source_config = InitialBrowserConfig::ely_defaults()?;
|
||||
source_config.profile_id = Some(private_profile_id.clone());
|
||||
@@ -172,8 +186,114 @@ fn standard_profile_sync_remaps_a_private_profile_id_collision() -> Result<(), B
|
||||
.count(),
|
||||
1
|
||||
);
|
||||
let outbound = String::from_utf8(target.build_sync_snapshot_bytes()?)?;
|
||||
let outbound_bytes = target.build_sync_snapshot_bytes()?;
|
||||
let outbound = String::from_utf8(outbound_bytes.clone())?;
|
||||
assert!(!outbound.contains("https://private.example/id-secret"));
|
||||
assert!(!outbound.contains("https://private-space.example/standard-profile-secret"));
|
||||
assert!(!outbound.contains(private_profile_id.as_str()));
|
||||
assert!(!outbound.contains(private_space_id.as_str()));
|
||||
assert!(!snapshot_contains_space_named(&outbound_bytes, "Private")?);
|
||||
assert!(!outbound.contains("\"space_name\":\"Private\""));
|
||||
assert!(outbound.contains("https://example.com/id-remote"));
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn private_profile_blocks_snapshot_input_and_output() -> Result<(), Box<dyn Error>> {
|
||||
let source = BrowserCore::new(InitialBrowserConfig::ely_defaults()?)?;
|
||||
let bytes = source.build_sync_snapshot_bytes()?;
|
||||
let mut private = BrowserCore::new(InitialBrowserConfig::private_window()?)?;
|
||||
private.set_sync_connection_state(SyncConnectionState::SignedIn);
|
||||
|
||||
assert!(!private.active_profile_allows_sync());
|
||||
assert!(!private.cloud_sync_upload_enabled());
|
||||
assert!(matches!(private.build_sync_snapshot_bytes(), Err(SyncClientError::SyncPolicy { .. })));
|
||||
assert!(matches!(
|
||||
private.apply_sync_snapshot_bytes(&bytes),
|
||||
Err(SyncClientError::SyncPolicy { .. })
|
||||
));
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn sync_snapshot_rejects_records_targeting_a_private_profile() -> Result<(), Box<dyn Error>> {
|
||||
let mut target = BrowserCore::new(InitialBrowserConfig::private_window()?)?;
|
||||
let private_profile_id = target.snapshot()?.active_profile_id;
|
||||
target.create_profile("Local", 0x26251e, ProfileKind::Standard)?;
|
||||
|
||||
let mut source_config = InitialBrowserConfig::ely_defaults()?;
|
||||
source_config.profile_id = Some(private_profile_id);
|
||||
let source = BrowserCore::new(source_config)?;
|
||||
let mut document: serde_json::Value =
|
||||
serde_json::from_slice(&source.build_sync_snapshot_bytes()?)?;
|
||||
document["profiles"] = serde_json::json!([]);
|
||||
let bytes = serde_json::to_vec(&document)?;
|
||||
|
||||
assert!(matches!(
|
||||
target.apply_sync_snapshot_bytes(&bytes),
|
||||
Err(SyncClientError::SyncPolicy { .. })
|
||||
));
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn sync_snapshot_blocks_references_to_a_remote_private_profile() -> Result<(), Box<dyn Error>> {
|
||||
let mut source = BrowserCore::new(InitialBrowserConfig::ely_defaults()?)?;
|
||||
source.navigate_active_tab(UrlText::parse("https://private.example/remote-record")?)?;
|
||||
let mut document: serde_json::Value =
|
||||
serde_json::from_slice(&source.build_sync_snapshot_bytes()?)?;
|
||||
let profiles = document["profiles"].as_array_mut().ok_or("sync profiles must be an array")?;
|
||||
let profile = profiles.first_mut().ok_or("sync snapshot must include a profile")?;
|
||||
profile["kind"] = serde_json::json!("private");
|
||||
let bytes = serde_json::to_vec(&document)?;
|
||||
let mut target = BrowserCore::new(InitialBrowserConfig::ely_defaults()?)?;
|
||||
|
||||
assert!(matches!(
|
||||
target.apply_sync_snapshot_bytes(&bytes),
|
||||
Err(SyncClientError::SyncPolicy { .. })
|
||||
));
|
||||
assert!(
|
||||
target
|
||||
.snapshot()?
|
||||
.tabs
|
||||
.iter()
|
||||
.all(|tab| tab.url().as_str() != "https://private.example/remote-record")
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn sync_snapshot_rejects_records_targeting_a_private_space() -> Result<(), Box<dyn Error>> {
|
||||
let mut target = BrowserCore::new(InitialBrowserConfig::private_window()?)?;
|
||||
let private_space_id = target.snapshot()?.active_space_id;
|
||||
let standard_profile_id = target.create_profile("Local", 0x26251e, ProfileKind::Standard)?;
|
||||
|
||||
let mut source_config = InitialBrowserConfig::ely_defaults()?;
|
||||
source_config.profile_id = Some(standard_profile_id);
|
||||
let source = BrowserCore::new(source_config)?;
|
||||
let mut document: serde_json::Value =
|
||||
serde_json::from_slice(&source.build_sync_snapshot_bytes()?)?;
|
||||
document["profiles"] = serde_json::json!([]);
|
||||
document["spaces"] = serde_json::json!([]);
|
||||
for tab in document["tabs"].as_array_mut().ok_or("sync tabs must be an array")? {
|
||||
tab["space_id"] = serde_json::json!(private_space_id.as_str());
|
||||
tab["space_name"] = serde_json::json!("Private");
|
||||
}
|
||||
let bytes = serde_json::to_vec(&document)?;
|
||||
|
||||
assert!(matches!(
|
||||
target.apply_sync_snapshot_bytes(&bytes),
|
||||
Err(SyncClientError::SyncPolicy { .. })
|
||||
));
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn snapshot_contains_space_named(bytes: &[u8], name: &str) -> Result<bool, Box<dyn Error>> {
|
||||
let document: serde_json::Value = serde_json::from_slice(bytes)?;
|
||||
let spaces = document["spaces"].as_array().ok_or("sync snapshot spaces must be an array")?;
|
||||
Ok(spaces.iter().any(|space| space["name"].as_str() == Some(name)))
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user