Skip to content

Commit b327770

Browse files
committed
fix(migration): Fix cross-platform CI failures
- Set the current user as owner when creating Windows handoff files while preserving strict ownership checks and inherited access rules. - Preserve AlreadyExists errors for nonce recovery and support long Windows paths without overwriting existing requests. - Align memory symlink tests with per-file omission behavior while retaining link rejection and strict destination validation. - Cover file ownership, long paths, and existing-content preservation.
1 parent 762bbbc commit b327770

2 files changed

Lines changed: 182 additions & 41 deletions

File tree

‎src/crates/services/legacy-migration-adapters/src/memory.rs‎

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2097,7 +2097,7 @@ mod tests {
20972097

20982098
#[cfg(unix)]
20992099
#[test]
2100-
fn file_memory_rejects_symlinked_owner_file() {
2100+
fn file_memory_omits_symlinked_source_file() {
21012101
use std::os::unix::fs::symlink;
21022102

21032103
let temp = test_tempdir("file-memory-symlink");
@@ -2109,15 +2109,12 @@ mod tests {
21092109
fs::write(&outside, "outside").unwrap();
21102110
symlink(&outside, memory_root.join("MEMORY.md")).unwrap();
21112111

2112-
assert!(matches!(
2113-
plan_file_memory(&roots),
2114-
Err(LegacyMigrationError::LinkedPath(_))
2115-
));
2112+
assert_linked_source_is_omitted(&roots);
21162113
}
21172114

21182115
#[cfg(windows)]
21192116
#[test]
2120-
fn file_memory_rejects_reparse_owner_file_when_supported() {
2117+
fn file_memory_omits_reparse_source_file_when_supported() {
21212118
use std::os::windows::fs::symlink_file;
21222119

21232120
let temp = test_tempdir("file-memory-reparse");
@@ -2131,10 +2128,29 @@ mod tests {
21312128
return;
21322129
}
21332130

2131+
assert_linked_source_is_omitted(&roots);
2132+
}
2133+
2134+
fn assert_linked_source_is_omitted(roots: &MigrationRoots) {
2135+
let source_root = source_file_memory_root(roots);
2136+
fs::write(source_root.join("memory_summary.md"), "valid summary").unwrap();
21342137
assert!(matches!(
2135-
plan_file_memory(&roots),
2138+
validate_regular_file(&source_root, &source_root.join("MEMORY.md")),
21362139
Err(LegacyMigrationError::LinkedPath(_))
21372140
));
2141+
let plan = plan_file_memory(roots).expect("skip linked source and keep valid files");
2142+
assert_eq!(plan.omitted, vec!["MEMORY.md"]);
2143+
assert_eq!(plan.files.len(), 1);
2144+
assert_eq!(
2145+
plan.files[0].source_relative,
2146+
PathBuf::from("memory_summary.md")
2147+
);
2148+
// The same unsafe entry on the destination must still block planning.
2149+
assert!(collect_memory_files(&roots.legacy_home_root, &source_root).is_err());
2150+
assert_eq!(
2151+
fs::read_to_string(roots.legacy_home_root.join("outside.md")).unwrap(),
2152+
"outside"
2153+
);
21382154
}
21392155

21402156
fn memory_selection() -> MigrationSelection {

‎src/crates/services/legacy-migration/src/handoff.rs‎

Lines changed: 159 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,9 @@ use openbitfun_product_domains::legacy_migration::{
77
use serde::{Deserialize, Serialize};
88
use sha2::{Digest, Sha256};
99
use std::ffi::OsStr;
10-
use std::fs::{self, File, OpenOptions};
10+
#[cfg(not(windows))]
11+
use std::fs::OpenOptions;
12+
use std::fs::{self, File};
1113
use std::io::Write;
1214
use std::path::{Component, Path, PathBuf};
1315

@@ -340,16 +342,21 @@ fn write_new_private_json<T: Serialize>(path: &Path, value: &T) -> LegacyMigrati
340342
let mut bytes = serde_json::to_vec_pretty(value)
341343
.map_err(|error| LegacyMigrationError::json(path, error))?;
342344
bytes.push(b'\n');
343-
let mut options = OpenOptions::new();
344-
options.create_new(true).write(true);
345-
#[cfg(unix)]
346-
{
347-
use std::os::unix::fs::OpenOptionsExt;
348-
options.mode(0o600);
349-
}
350-
let mut file = options
351-
.open(path)
352-
.map_err(|error| LegacyMigrationError::io(path, error))?;
345+
#[cfg(windows)]
346+
let mut file = create_current_user_owned_file(path)?;
347+
#[cfg(not(windows))]
348+
let mut file = {
349+
let mut options = OpenOptions::new();
350+
options.create_new(true).write(true);
351+
#[cfg(unix)]
352+
{
353+
use std::os::unix::fs::OpenOptionsExt;
354+
options.mode(0o600);
355+
}
356+
options
357+
.open(path)
358+
.map_err(|error| LegacyMigrationError::io(path, error))?
359+
};
353360
file.write_all(&bytes)
354361
.map_err(|error| LegacyMigrationError::io(path, error))?;
355362
file.sync_all()
@@ -424,26 +431,13 @@ fn ensure_path_chain_is_plain(root: &Path, target: &Path) -> LegacyMigrationResu
424431

425432
#[cfg(windows)]
426433
fn verify_current_user_owned(path: &Path) -> LegacyMigrationResult<()> {
427-
use std::ffi::c_void;
428434
use std::os::windows::ffi::OsStrExt;
429435
use windows::core::PCWSTR;
430-
use windows::Win32::Foundation::{CloseHandle, LocalFree, HANDLE, HLOCAL};
436+
use windows::Win32::Foundation::{LocalFree, HLOCAL};
431437
use windows::Win32::Security::Authorization::{GetNamedSecurityInfoW, SE_FILE_OBJECT};
432438
use windows::Win32::Security::{
433-
EqualSid, GetTokenInformation, TokenUser, OWNER_SECURITY_INFORMATION, PSECURITY_DESCRIPTOR,
434-
PSID, TOKEN_QUERY, TOKEN_USER,
439+
EqualSid, OWNER_SECURITY_INFORMATION, PSECURITY_DESCRIPTOR, PSID,
435440
};
436-
use windows::Win32::System::Threading::{GetCurrentProcess, OpenProcessToken};
437-
438-
struct OwnedHandle(HANDLE);
439-
impl Drop for OwnedHandle {
440-
fn drop(&mut self) {
441-
// SAFETY: this wrapper owns the process token handle exactly once.
442-
unsafe {
443-
let _ = CloseHandle(self.0);
444-
}
445-
}
446-
}
447441

448442
struct LocalSecurityDescriptor(PSECURITY_DESCRIPTOR);
449443
impl Drop for LocalSecurityDescriptor {
@@ -456,7 +450,9 @@ fn verify_current_user_owned(path: &Path) -> LegacyMigrationResult<()> {
456450
}
457451
}
458452

459-
let wide_path = path
453+
let canonical_path =
454+
fs::canonicalize(path).map_err(|error| LegacyMigrationError::io(path, error))?;
455+
let wide_path = canonical_path
460456
.as_os_str()
461457
.encode_wide()
462458
.chain(std::iter::once(0))
@@ -484,6 +480,35 @@ fn verify_current_user_owned(path: &Path) -> LegacyMigrationResult<()> {
484480
}
485481
let _descriptor = LocalSecurityDescriptor(descriptor);
486482

483+
with_current_user_sid(|sid| {
484+
// SAFETY: both SIDs are owned by live security descriptor/token buffers.
485+
unsafe { EqualSid(owner, sid) }.map_err(|_| {
486+
LegacyMigrationError::InvalidRequest(
487+
"handoff file is owned by another OS user".to_string(),
488+
)
489+
})
490+
})
491+
}
492+
493+
#[cfg(windows)]
494+
fn with_current_user_sid<T>(
495+
action: impl FnOnce(windows::Win32::Security::PSID) -> LegacyMigrationResult<T>,
496+
) -> LegacyMigrationResult<T> {
497+
use std::ffi::c_void;
498+
use windows::Win32::Foundation::{CloseHandle, HANDLE};
499+
use windows::Win32::Security::{GetTokenInformation, TokenUser, TOKEN_QUERY, TOKEN_USER};
500+
use windows::Win32::System::Threading::{GetCurrentProcess, OpenProcessToken};
501+
502+
struct OwnedHandle(HANDLE);
503+
impl Drop for OwnedHandle {
504+
fn drop(&mut self) {
505+
// SAFETY: this wrapper owns the process token handle exactly once.
506+
unsafe {
507+
let _ = CloseHandle(self.0);
508+
}
509+
}
510+
}
511+
487512
let mut raw_token = HANDLE::default();
488513
// SAFETY: `raw_token` is writable and becomes owned by `OwnedHandle` only
489514
// after OpenProcessToken succeeds.
@@ -516,13 +541,93 @@ fn verify_current_user_owned(path: &Path) -> LegacyMigrationResult<()> {
516541
// SAFETY: GetTokenInformation successfully initialized a TOKEN_USER at the
517542
// start of the aligned allocator buffer for the duration of this scope.
518543
let token_user = unsafe { &*(buffer.as_ptr() as *const TOKEN_USER) };
519-
// SAFETY: both SIDs are owned by live security descriptor/token buffers.
520-
if unsafe { EqualSid(owner, token_user.User.Sid) }.is_err() {
521-
return Err(LegacyMigrationError::InvalidRequest(
522-
"handoff file is owned by another OS user".to_string(),
523-
));
524-
}
525-
Ok(())
544+
action(token_user.User.Sid)
545+
}
546+
547+
#[cfg(windows)]
548+
fn create_current_user_owned_file(path: &Path) -> LegacyMigrationResult<File> {
549+
use std::os::windows::{ffi::OsStrExt, io::FromRawHandle};
550+
use windows::core::PCWSTR;
551+
use windows::Win32::Security::{
552+
InitializeSecurityDescriptor, SetSecurityDescriptorOwner, PSECURITY_DESCRIPTOR,
553+
SECURITY_ATTRIBUTES, SECURITY_DESCRIPTOR,
554+
};
555+
use windows::Win32::Storage::FileSystem::{
556+
CreateFileW, CREATE_NEW, FILE_ATTRIBUTE_NORMAL, FILE_GENERIC_WRITE, FILE_SHARE_DELETE,
557+
FILE_SHARE_READ, FILE_SHARE_WRITE,
558+
};
559+
560+
with_current_user_sid(|sid| {
561+
let mut descriptor = SECURITY_DESCRIPTOR::default();
562+
let descriptor_ptr =
563+
PSECURITY_DESCRIPTOR((&mut descriptor as *mut SECURITY_DESCRIPTOR).cast());
564+
// SAFETY: the descriptor is aligned, writable and lives through file creation.
565+
// The SID remains live inside with_current_user_sid. Leave the DACL absent
566+
// so Windows retains its normal inherited/default access policy.
567+
unsafe {
568+
InitializeSecurityDescriptor(descriptor_ptr, 1)
569+
.and_then(|()| SetSecurityDescriptorOwner(descriptor_ptr, Some(sid), false))
570+
}
571+
.map_err(|error| LegacyMigrationError::ProcessInspection(error.to_string()))?;
572+
let mut attributes = SECURITY_ATTRIBUTES {
573+
nLength: std::mem::size_of::<SECURITY_ATTRIBUTES>() as u32,
574+
lpSecurityDescriptor: descriptor_ptr.0,
575+
bInheritHandle: false.into(),
576+
};
577+
// canonicalize supplies the verbatim Windows prefix for long paths,
578+
// matching std::fs behavior without requiring the new file to exist.
579+
let parent = path
580+
.parent()
581+
.filter(|parent| !parent.as_os_str().is_empty())
582+
.unwrap_or(Path::new("."));
583+
let name = path.file_name().ok_or_else(|| {
584+
LegacyMigrationError::io(
585+
path,
586+
std::io::Error::new(
587+
std::io::ErrorKind::InvalidInput,
588+
"request path has no filename",
589+
),
590+
)
591+
})?;
592+
let absolute = fs::canonicalize(parent)
593+
.map_err(|error| LegacyMigrationError::io(path, error))?
594+
.join(name);
595+
let mut wide_path = absolute.as_os_str().encode_wide().collect::<Vec<_>>();
596+
if wide_path.contains(&0) {
597+
return Err(LegacyMigrationError::io(
598+
path,
599+
std::io::Error::new(
600+
std::io::ErrorKind::InvalidInput,
601+
"request path contains a NUL character",
602+
),
603+
));
604+
}
605+
wide_path.push(0);
606+
// SAFETY: the path, attributes, descriptor and SID remain live through
607+
// CreateFileW. CREATE_NEW never opens or changes an existing file.
608+
let handle = unsafe {
609+
CreateFileW(
610+
PCWSTR(wide_path.as_ptr()),
611+
FILE_GENERIC_WRITE.0,
612+
FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
613+
Some(&mut attributes),
614+
CREATE_NEW,
615+
FILE_ATTRIBUTE_NORMAL,
616+
None,
617+
)
618+
}
619+
.map_err(|error| {
620+
// CreateFileW reports HRESULT_FROM_WIN32; preserve the OS code so
621+
// callers can still distinguish AlreadyExists during nonce recovery.
622+
LegacyMigrationError::io(
623+
path,
624+
std::io::Error::from_raw_os_error(error.code().0 & 0xffff),
625+
)
626+
})?;
627+
// SAFETY: CreateFileW returned a valid, uniquely owned handle; File
628+
// takes responsibility for closing it, including on write failure.
629+
Ok(unsafe { File::from_raw_handle(handle.0) })
630+
})
526631
}
527632

528633
#[cfg(unix)]
@@ -1042,6 +1147,26 @@ mod tests {
10421147
}
10431148
}
10441149

1150+
#[test]
1151+
fn private_request_creation_preserves_owner_and_existing_contents() {
1152+
let temporary = tempfile::tempdir().expect("temporary directory");
1153+
let parent = temporary
1154+
.path()
1155+
.join("nested".repeat(20))
1156+
.join("nested".repeat(20));
1157+
fs::create_dir_all(&parent).expect("long request directory");
1158+
let path = parent.join("request.json");
1159+
write_new_private_json(&path, &"original").expect("create owned request");
1160+
verify_current_user_owned(&path).expect("request belongs to current user");
1161+
let original = fs::read(&path).unwrap();
1162+
assert!(matches!(
1163+
write_new_private_json(&path, &"replacement"),
1164+
Err(LegacyMigrationError::Io { source, .. })
1165+
if source.kind() == std::io::ErrorKind::AlreadyExists
1166+
));
1167+
assert_eq!(fs::read(&path).unwrap(), original);
1168+
}
1169+
10451170
#[test]
10461171
fn handoff_nonce_can_only_resume_the_same_persisted_plan() {
10471172
let temporary = tempfile::tempdir().expect("temporary directory");

0 commit comments

Comments
 (0)