From dc061017bb126452253af439ddb174b1584af260 Mon Sep 17 00:00:00 2001 From: saladday <1203511142@qq.com> Date: Wed, 23 Sep 2026 18:17:25 +0000 Subject: [PATCH 1/6] Install Files.create writes without replacement The shared workspace installer gains an explicit per-call mode. The new create mode walks existing components with the held O_PATH|O_NOFOLLOW walk, creates missing parents with mkdirat (mode 0700) only after the complete body is verified, reopens each one without following links, and installs with renameat2(RENAME_NOREPLACE), falling back to linkat plus unlinking the staging name. An existing file, a directory, a symlink or a non-directory component rejects with a fixed code and nothing is installed. The four-argument form keeps the replace mode used by initial Session files and Skills, including Core-issued installs on older Runtime images. --- packages/codex-executor/src/bin/write.rs | 72 ++++- packages/codex-executor/src/write_file.rs | 203 ++++++++++++- .../codex-executor/src/write_file_tests.rs | 276 +++++++++++++++++- 3 files changed, 533 insertions(+), 18 deletions(-) diff --git a/packages/codex-executor/src/bin/write.rs b/packages/codex-executor/src/bin/write.rs index bd3f0c92c..ccbbfbfa6 100644 --- a/packages/codex-executor/src/bin/write.rs +++ b/packages/codex-executor/src/bin/write.rs @@ -6,12 +6,22 @@ mod write_file; use serde_json::json; use std::io::{self, Write}; use std::path::Path; +use write_file::{InstallMode, Rejection}; + +/// The four-argument form keeps the replace mode of trusted initialization, +/// including Core-issued initial-file installs on older Runtime images. The +/// public Files.create writer appends `create`. +fn mode(args: &[String]) -> Option { + match args { + [_, _, _, _] => Some(InstallMode::Replace), + [_, _, _, _, mode] if mode == "create" => Some(InstallMode::Create), + _ => None, + } +} fn run() -> Result { let args: Vec<_> = std::env::args().skip(1).collect(); - if args.len() != 4 { - return Err(io::Error::from(io::ErrorKind::InvalidInput).into()); - } + let mode = mode(&args).ok_or_else(|| io::Error::from(io::ErrorKind::InvalidInput))?; let size = args[2] .parse::() .map_err(|_| io::Error::from(io::ErrorKind::InvalidInput))?; @@ -21,15 +31,25 @@ fn run() -> Result { size, io::stdin().lock(), Path::new(&args[3]), + mode, )?; Ok(size) } +fn error_code(failure: &write_file::Failure) -> &'static str { + match failure.rejection { + Some(Rejection::Directory) => "destination_directory", + Some(Rejection::Unsafe) => "unsafe_destination", + None if failure.error.kind() == io::ErrorKind::InvalidInput => "invalid_input", + None => "write_failed", + } +} + fn main() -> std::process::ExitCode { let response = match run() { Ok(size) => json!({"version": 1, "outcome": "completed", "size_bytes": size}), Err(failure) => { - json!({"version": 1, "outcome": if failure.committed { "unknown" } else { "failed" }, "error": if failure.error.kind() == io::ErrorKind::InvalidInput { "invalid_input" } else { "write_failed" }}) + json!({"version": 1, "outcome": if failure.committed { "unknown" } else { "failed" }, "error": error_code(&failure)}) } }; if writeln!(io::stdout().lock(), "{response}").is_err() { @@ -37,3 +57,47 @@ fn main() -> std::process::ExitCode { } std::process::ExitCode::SUCCESS } + +#[cfg(test)] +mod cli_tests { + use super::*; + + #[test] + fn selects_mode_explicitly_and_reports_fixed_codes() { + let args = |values: &[&str]| values.iter().map(|v| v.to_string()).collect::>(); + assert_eq!( + mode(&args(&["r", "p", "0", "s"])), + Some(InstallMode::Replace) + ); + assert_eq!( + mode(&args(&["r", "p", "0", "s", "create"])), + Some(InstallMode::Create) + ); + for invalid in [ + args(&["r", "p", "0"]), + args(&["r", "p", "0", "s", "replace"]), + args(&["r", "p", "0", "s", "create", "x"]), + ] { + assert_eq!(mode(&invalid), None); + } + let failure = |rejection, kind| write_file::Failure { + committed: false, + rejection, + error: io::Error::from(kind), + }; + for (expected, value) in [ + ( + "destination_directory", + failure(Some(Rejection::Directory), io::ErrorKind::AlreadyExists), + ), + ( + "unsafe_destination", + failure(Some(Rejection::Unsafe), io::ErrorKind::AlreadyExists), + ), + ("invalid_input", failure(None, io::ErrorKind::InvalidInput)), + ("write_failed", failure(None, io::ErrorKind::NotFound)), + ] { + assert_eq!(error_code(&value), expected); + } + } +} diff --git a/packages/codex-executor/src/write_file.rs b/packages/codex-executor/src/write_file.rs index f650b57a9..5cc681ef6 100644 --- a/packages/codex-executor/src/write_file.rs +++ b/packages/codex-executor/src/write_file.rs @@ -1,5 +1,9 @@ use crate::workspace_path::{anchor, directory}; -use rustix::fs::{AtFlags, FileType, Mode, OFlags, fstat, openat, renameat, statat, unlinkat}; +use rustix::fs::{ + AtFlags, FileType, Mode, OFlags, RenameFlags, fstat, linkat, mkdirat, openat, renameat, + renameat_with, statat, unlinkat, +}; +use rustix::io::Errno; use sha2::{Digest, Sha256}; use std::fs::File; use std::io::{self, Read, Write}; @@ -8,9 +12,30 @@ use std::path::Path; pub(crate) const MAX_BYTES: u64 = 50 * 1024 * 1024; +/// Selects the destination policy explicitly for each invocation. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum InstallMode { + /// Trusted initialization (initial Session files and Skills): the parent must + /// exist and an existing regular file is replaced by a fresh inode. + Replace, + /// Public Environment Files.create: missing parents are created without + /// following links and an existing destination is never replaced. + Create, +} + +/// A known create-mode refusal. Nothing was installed and staging is removed. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum Rejection { + /// A directory already occupies the destination. + Directory, + /// The destination exists, or a path component is a symlink or not a directory. + Unsafe, +} + #[derive(Debug)] pub(crate) struct Failure { pub committed: bool, + pub rejection: Option, pub error: io::Error, } @@ -18,17 +43,35 @@ impl From for Failure { fn from(error: io::Error) -> Self { Self { committed: false, + rejection: None, error, } } } +impl From for Failure { + fn from(error: Errno) -> Self { + io::Error::from(error).into() + } +} + +impl From for Failure { + fn from(rejection: Rejection) -> Self { + Self { + committed: false, + rejection: Some(rejection), + error: io::ErrorKind::AlreadyExists.into(), + } + } +} + pub(crate) fn install( root: &Path, relative: &str, size: u64, mut input: impl Read, staging_root: &Path, + mode: InstallMode, ) -> Result<(), Failure> { if size > MAX_BYTES || staging_root.starts_with(root) @@ -43,18 +86,31 @@ pub(crate) fn install( } let (parent, leaf) = relative.rsplit_once('/').unwrap_or(("", relative)); let root = anchor(root)?; - let parent = directory(&root, parent)?; + let (held, missing) = match mode { + InstallMode::Replace => (directory(&root, parent)?, Vec::new()), + InstallMode::Create => existing_prefix(&root, parent)?, + }; let staging_parent = directory(&anchor(staging_root)?, "")?; - if fstat(&parent).map_err(io::Error::from)?.st_dev - != fstat(&staging_parent).map_err(io::Error::from)?.st_dev - { + let device = fstat(&staging_parent)?.st_dev; + if fstat(&held)?.st_dev != device { return Err(io::Error::from(io::ErrorKind::InvalidInput).into()); } - match statat(&parent, leaf, AtFlags::SYMLINK_NOFOLLOW) { - Ok(metadata) if FileType::from_raw_mode(metadata.st_mode) == FileType::RegularFile => (), - Ok(_) => return Err(io::Error::from(io::ErrorKind::InvalidInput).into()), - Err(rustix::io::Errno::NOENT) => (), - Err(error) => return Err(io::Error::from(error).into()), + if missing.is_empty() { + match (mode, statat(&held, leaf, AtFlags::SYMLINK_NOFOLLOW)) { + (_, Err(Errno::NOENT)) => (), + (InstallMode::Replace, Ok(metadata)) + if FileType::from_raw_mode(metadata.st_mode) == FileType::RegularFile => {} + (InstallMode::Replace, Ok(_)) => { + return Err(io::Error::from(io::ErrorKind::InvalidInput).into()); + } + (InstallMode::Create, Ok(metadata)) + if FileType::from_raw_mode(metadata.st_mode) == FileType::Directory => + { + return Err(Rejection::Directory.into()); + } + (InstallMode::Create, Ok(_)) => return Err(Rejection::Unsafe.into()), + (_, Err(error)) => return Err(error.into()), + } } let staging = Staging::new(&staging_parent)?; let mut file = &staging.file; @@ -75,20 +131,99 @@ pub(crate) fn install( return Err(io::Error::from(io::ErrorKind::InvalidData).into()); } staging.file.sync_all()?; - staging.persist(&parent, leaf)?; + let parent = match mode { + InstallMode::Replace => { + staging.replace(&held, leaf)?; + held + } + InstallMode::Create => { + // Parents are created only after the complete body is verified. + let parent = create_missing(held, &missing)?; + if fstat(&parent)?.st_dev != device { + return Err(io::Error::from(io::ErrorKind::InvalidInput).into()); + } + staging.install_new(&parent, leaf)?; + parent + } + }; File::from(parent).sync_all().map_err(|error| Failure { committed: true, + rejection: None, error, })?; File::from(staging_parent) .sync_all() .map_err(|error| Failure { committed: true, + rejection: None, error, })?; Ok(()) } +const HELD: OFlags = OFlags::PATH + .union(OFlags::DIRECTORY) + .union(OFlags::NOFOLLOW) + .union(OFlags::CLOEXEC); + +/// Walks existing components below the held root without following links. It +/// returns the deepest existing directory and the components still missing. An +/// existing symlink or non-directory component rejects; nothing is created here. +fn existing_prefix<'a>( + root: &OwnedFd, + relative: &'a str, +) -> Result<(OwnedFd, Vec<&'a str>), Failure> { + let mut fd = openat( + root, + ".", + OFlags::PATH | OFlags::DIRECTORY | OFlags::CLOEXEC, + Mode::empty(), + )?; + if relative.is_empty() { + return Ok((fd, Vec::new())); + } + let parts: Vec<_> = relative.split('/').collect(); + for (index, part) in parts.iter().enumerate() { + match openat(&fd, *part, HELD, Mode::empty()) { + Ok(child) => fd = child, + Err(Errno::NOENT) => return Ok((fd, parts[index..].to_vec())), + Err(Errno::NOTDIR | Errno::LOOP) => return Err(Rejection::Unsafe.into()), + Err(error) => return Err(error.into()), + } + } + Ok((fd, Vec::new())) +} + +/// Creates each missing component with mode 0700 relative to the held parent, +/// then reopens it without following links. A component that appears +/// concurrently is accepted only if it is a real directory. +fn create_missing(mut fd: OwnedFd, missing: &[&str]) -> Result { + for part in missing { + match mkdirat(&fd, *part, Mode::from_raw_mode(0o700)) { + Ok(()) => File::from(openat( + &fd, + ".", + OFlags::RDONLY | OFlags::DIRECTORY | OFlags::CLOEXEC, + Mode::empty(), + )?) + .sync_all()?, + Err(Errno::EXIST) => (), + Err(error) => return Err(error.into()), + } + fd = match openat(&fd, *part, HELD, Mode::empty()) { + Ok(child) => child, + Err(Errno::NOTDIR | Errno::LOOP) => return Err(Rejection::Unsafe.into()), + Err(error) => return Err(error.into()), + }; + } + Ok(openat( + &fd, + ".", + OFlags::RDONLY | OFlags::DIRECTORY | OFlags::CLOEXEC, + Mode::empty(), + )?) +} + struct Staging<'a> { parent: &'a OwnedFd, name: String, @@ -113,11 +248,55 @@ impl<'a> Staging<'a> { }) } - fn persist(mut self, destination: &OwnedFd, leaf: &str) -> io::Result<()> { + fn replace(mut self, destination: &OwnedFd, leaf: &str) -> io::Result<()> { renameat(self.parent, self.name.as_str(), destination, leaf)?; self.committed = true; Ok(()) } + + /// Atomically installs the file only if the destination name is free. + fn install_new(self, destination: &OwnedFd, leaf: &str) -> Result<(), Failure> { + match renameat_with( + self.parent, + self.name.as_str(), + destination, + leaf, + RenameFlags::NOREPLACE, + ) { + Ok(()) => { + self.commit(); + Ok(()) + } + Err(Errno::EXIST) => Err(Rejection::Unsafe.into()), + // The filesystem or kernel lacks RENAME_NOREPLACE. + Err(Errno::INVAL | Errno::NOSYS) => self.link_new(destination, leaf), + Err(error) => Err(error.into()), + } + } + + /// linkat never replaces an existing name. Removing the staging name after + /// the link is best-effort cleanup of an already installed file. + fn link_new(self, destination: &OwnedFd, leaf: &str) -> Result<(), Failure> { + match linkat( + self.parent, + self.name.as_str(), + destination, + leaf, + AtFlags::empty(), + ) { + Ok(()) => { + let _ = unlinkat(self.parent, self.name.as_str(), AtFlags::empty()); + self.commit(); + Ok(()) + } + Err(Errno::EXIST) => Err(Rejection::Unsafe.into()), + Err(error) => Err(error.into()), + } + } + + fn commit(mut self) { + self.committed = true; + } } impl Drop for Staging<'_> { diff --git a/packages/codex-executor/src/write_file_tests.rs b/packages/codex-executor/src/write_file_tests.rs index c14fab048..f0fd1ad21 100644 --- a/packages/codex-executor/src/write_file_tests.rs +++ b/packages/codex-executor/src/write_file_tests.rs @@ -16,8 +16,17 @@ fn frame(data: &[u8]) -> impl Read + '_ { fn install(root: &Path, relative: &str, size: u64, input: impl Read) -> Result<(), Failure> { let staging = fixture(); - let result = super::install(root, relative, size, input, staging.path()); + let result = super::install( + root, + relative, + size, + input, + staging.path(), + InstallMode::Replace, + ); assert_eq!(fs::read_dir(staging.path()).unwrap().count(), 0); + // Replace mode keeps the legacy error codes of trusted initialization. + assert!(result.as_ref().err().is_none_or(|f| f.rejection.is_none())); result } @@ -45,9 +54,16 @@ fn requires_existing_disjoint_nofollow_staging_before_consuming_input() { staging.join("../private"), Path::new("relative").to_path_buf(), ] { - let failure = super::install(&root, "file", 3, Unread, &invalid).unwrap_err(); + for mode in [InstallMode::Replace, InstallMode::Create] { + let failure = super::install(&root, "new/file", 3, Unread, &invalid, mode).unwrap_err(); + assert!(!failure.committed); + assert!(failure.rejection.is_none()); + } + let failure = + super::install(&root, "file", 3, Unread, &invalid, InstallMode::Replace).unwrap_err(); assert!(!failure.committed); assert_eq!(fs::read(root.join("file")).unwrap(), b"old"); + assert!(!root.join("new").exists()); } assert_eq!(fs::read_dir(&staging).unwrap().count(), 0); } @@ -220,3 +236,259 @@ fn requires_matching_commit_trailer_and_does_not_wait_for_eof() { install(f.path(), "file", 3, frame(b"new").chain(NoMore)).unwrap(); assert_eq!(fs::read(f.path().join("file")).unwrap(), b"new"); } + +fn create(root: &Path, relative: &str, size: u64, input: impl Read) -> Result<(), Failure> { + let staging = fixture(); + let result = super::install( + root, + relative, + size, + input, + staging.path(), + InstallMode::Create, + ); + assert_eq!(fs::read_dir(staging.path()).unwrap().count(), 0); + result +} + +fn rejection(result: Result<(), Failure>) -> Option { + let failure = result.unwrap_err(); + assert!(!failure.committed); + failure.rejection +} + +fn mode(path: &Path) -> u32 { + fs::symlink_metadata(path).unwrap().permissions().mode() & 0o777 +} + +struct Unread; + +impl Read for Unread { + fn read(&mut self, _: &mut [u8]) -> io::Result { + panic!("a known conflict must be rejected before reading input") + } +} + +#[test] +fn create_mode_makes_missing_parents_after_verified_input() { + let f = fixture(); + let root = f.path().join("workspace"); + fs::create_dir(&root).unwrap(); + create(&root, "top.txt", 3, frame(b"top")).unwrap(); + create(&root, "n1/n2/nested.txt", 6, frame(b"nested")).unwrap(); + assert_eq!(fs::read(root.join("n1/n2/nested.txt")).unwrap(), b"nested"); + assert_eq!(fs::read(root.join("top.txt")).unwrap(), b"top"); + for created in ["n1", "n1/n2"] { + assert!(fs::symlink_metadata(root.join(created)).unwrap().is_dir()); + assert_eq!(mode(&root.join(created)), 0o700); + } + assert_eq!(mode(&root.join("n1/n2/nested.txt")), 0o600); + // An existing prefix is reused; only the missing tail is created. + create(&root, "n1/n3/deeper/empty", 0, frame(b"")).unwrap(); + assert!( + fs::read(root.join("n1/n3/deeper/empty")) + .unwrap() + .is_empty() + ); + // Incomplete or mismatched input creates no parents. + assert!(rejection(create(&root, "p1/p2/file", 3, Cursor::new(b"ab"))).is_none()); + let mismatched = [b"abc".as_slice(), &[0u8; 32]].concat(); + assert!(rejection(create(&root, "p1/p2/file", 3, Cursor::new(mismatched))).is_none()); + assert!(!root.join("p1").exists()); +} + +#[test] +fn create_mode_never_replaces_existing_destinations() { + let f = fixture(); + let root = f.path().join("workspace"); + fs::create_dir_all(root.join("n1/n2")).unwrap(); + let outside = f.path().join("outside"); + fs::write(&outside, b"outside").unwrap(); + fs::hard_link(&outside, root.join("alias")).unwrap(); + fs::write(root.join("n1/tracked.txt"), b"old").unwrap(); + fs::write(root.join("empty"), b"").unwrap(); + rustix::fs::mknodat( + rustix::fs::CWD, + root.join("fifo").as_path(), + FileType::Fifo, + rustix::fs::Mode::from_raw_mode(0o600), + 0, + ) + .unwrap(); + for name in ["n1/tracked.txt", "alias", "empty", "fifo"] { + assert_eq!( + rejection(create(&root, name, 3, Unread)), + Some(Rejection::Unsafe), + "{name}" + ); + } + for name in ["n1", "n1/n2"] { + assert_eq!( + rejection(create(&root, name, 3, Unread)), + Some(Rejection::Directory), + "{name}" + ); + assert!(fs::symlink_metadata(root.join(name)).unwrap().is_dir()); + } + assert_eq!(fs::read(root.join("n1/tracked.txt")).unwrap(), b"old"); + assert_eq!(fs::read(&outside).unwrap(), b"outside"); + assert_eq!( + fs::metadata(&outside).unwrap().ino(), + fs::metadata(root.join("alias")).unwrap().ino() + ); + assert!(fs::read(root.join("empty")).unwrap().is_empty()); + // A second create of the same path is refused after the first commits. + create(&root, "n1/once.txt", 3, frame(b"one")).unwrap(); + assert_eq!( + rejection(create(&root, "n1/once.txt", 3, frame(b"two"))), + Some(Rejection::Unsafe) + ); + assert_eq!(fs::read(root.join("n1/once.txt")).unwrap(), b"one"); +} + +#[test] +fn create_mode_rejects_symlink_and_nondirectory_components() { + let f = fixture(); + let root = f.path().join("workspace"); + fs::create_dir_all(root.join("real")).unwrap(); + let outside = f.path().join("outside"); + fs::create_dir(&outside).unwrap(); + fs::write(outside.join("file"), b"canary").unwrap(); + symlink(outside.join("file"), root.join("leaf-link")).unwrap(); + symlink(&outside, root.join("dir-link")).unwrap(); + symlink(&outside, root.join("real/nested-link")).unwrap(); + symlink(root.join("real"), root.join("inside-link")).unwrap(); + symlink(f.path().join("missing"), root.join("dangling")).unwrap(); + fs::write(root.join("plain"), b"plain").unwrap(); + for name in [ + "leaf-link", + "dir-link", + "dangling", + "inside-link", + "dir-link/file", + "dir-link/new/file", + "real/nested-link/file", + "inside-link/file", + "dangling/file", + "plain/child", + "plain/child/deeper", + ] { + assert_eq!( + rejection(create(&root, name, 3, Unread)), + Some(Rejection::Unsafe), + "{name}" + ); + } + assert_eq!(fs::read(outside.join("file")).unwrap(), b"canary"); + assert_eq!(fs::read_dir(&outside).unwrap().count(), 1); + assert_eq!(fs::read_dir(root.join("real")).unwrap().count(), 1); + assert!(!f.path().join("missing").exists()); + for invalid in [ + "../outside/file", + "/file", + "a/../b", + "a//b", + "a\\b", + "x\n", + ".", + ] { + assert!( + rejection(create(&root, invalid, 3, Unread)).is_none(), + "{invalid:?}" + ); + } +} + +/// Changes the workspace once, after the pre-install checks and before commit. +struct Race<'a, R> { + inner: R, + change: Option>, +} + +impl Read for Race<'_, R> { + fn read(&mut self, buf: &mut [u8]) -> io::Result { + if let Some(change) = self.change.take() { + change(); + } + self.inner.read(buf) + } +} + +fn racing<'a>(data: &'a [u8], change: impl FnOnce() + 'a) -> impl Read + 'a { + Race { + inner: frame(data), + change: Some(Box::new(change)), + } +} + +#[test] +fn create_mode_race_never_replaces_or_follows() { + let f = fixture(); + let root = f.path().join("workspace"); + fs::create_dir(&root).unwrap(); + let outside = f.path().join("outside"); + fs::create_dir(&outside).unwrap(); + // The destination appears after the leaf check. + let target = root.join("raced.txt"); + let input = racing(b"api", || fs::write(&target, b"racer").unwrap()); + assert_eq!( + rejection(create(&root, "raced.txt", 3, input)), + Some(Rejection::Unsafe) + ); + assert_eq!(fs::read(&target).unwrap(), b"racer"); + // A directory appears at the destination. + let input = racing(b"api", || fs::create_dir(root.join("raced-dir")).unwrap()); + assert_eq!( + rejection(create(&root, "raced-dir", 3, input)), + Some(Rejection::Unsafe) + ); + assert_eq!(fs::read_dir(root.join("raced-dir")).unwrap().count(), 0); + // A missing parent becomes a symlink before it is created. + let link = root.join("parent"); + let input = racing(b"api", || symlink(&outside, &link).unwrap()); + assert_eq!( + rejection(create(&root, "parent/child/file", 3, input)), + Some(Rejection::Unsafe) + ); + assert_eq!(fs::read_dir(&outside).unwrap().count(), 0); + // A missing parent is created concurrently as a real directory. + let concurrent = root.join("shared"); + let input = racing(b"api", || fs::create_dir(&concurrent).unwrap()); + create(&root, "shared/file", 3, input).unwrap(); + assert_eq!(fs::read(root.join("shared/file")).unwrap(), b"api"); + // A held existing ancestor that is replaced by a link keeps the commit inside. + fs::create_dir(root.join("a")).unwrap(); + let input = racing(b"api", || { + fs::rename(root.join("a"), root.join("held")).unwrap(); + symlink(&outside, root.join("a")).unwrap(); + }); + create(&root, "a/b/file", 3, input).unwrap(); + assert_eq!(fs::read(root.join("held/b/file")).unwrap(), b"api"); + assert_eq!(fs::read_dir(&outside).unwrap().count(), 0); +} + +#[test] +fn link_fallback_never_replaces_and_removes_staging() { + let f = fixture(); + let staging_root = f.path().join("staging"); + let root = f.path().join("workspace"); + fs::create_dir(&staging_root).unwrap(); + fs::create_dir(&root).unwrap(); + fs::write(root.join("existing"), b"old").unwrap(); + let staging_parent = directory(&anchor(&staging_root).unwrap(), "").unwrap(); + let parent = directory(&anchor(&root).unwrap(), "").unwrap(); + let staging = Staging::new(&staging_parent).unwrap(); + (&staging.file).write_all(b"new").unwrap(); + assert_eq!( + staging.link_new(&parent, "existing").unwrap_err().rejection, + Some(Rejection::Unsafe) + ); + assert_eq!(fs::read(root.join("existing")).unwrap(), b"old"); + assert_eq!(fs::read_dir(&staging_root).unwrap().count(), 0); + let staging = Staging::new(&staging_parent).unwrap(); + (&staging.file).write_all(b"new").unwrap(); + staging.link_new(&parent, "fresh").unwrap(); + assert_eq!(fs::read(root.join("fresh")).unwrap(), b"new"); + assert_eq!(fs::metadata(root.join("fresh")).unwrap().nlink(), 1); + assert_eq!(fs::read_dir(&staging_root).unwrap().count(), 0); +} From d40a6277a912998b4fb9aefc76fe6eee872f758c Mon Sep 17 00:00:00 2001 From: saladday <1203511142@qq.com> Date: Wed, 23 Sep 2026 18:17:25 +0000 Subject: [PATCH 2/6] Report Files.create destination conflicts through the daemon The local workspace writer always selects the helper's create mode. Its new directory and unsafe-destination codes become known rejections that the daemon sends as write_rejected with an optional reason; the gateway accepts a reason only on that rejection, and older peers ignore it. --- .../internal/agent/workspace_write.go | 8 +++ .../internal/dispatch/workspace_write.go | 11 ++++ .../internal/dispatch/workspace_write_test.go | 26 ++++++++ .../internal/localworkspace/write.go | 15 ++++- .../internal/localworkspace/write_test.go | 66 ++++++++++++------- .../agentdaemon/gateway/workspace_write.go | 3 + .../gateway/workspace_write_test.go | 13 ++++ internal/agentdaemon/proto/workspace_write.go | 9 +++ 8 files changed, 123 insertions(+), 28 deletions(-) diff --git a/apps/parsar-daemon/internal/agent/workspace_write.go b/apps/parsar-daemon/internal/agent/workspace_write.go index 77d69c939..237b73997 100644 --- a/apps/parsar-daemon/internal/agent/workspace_write.go +++ b/apps/parsar-daemon/internal/agent/workspace_write.go @@ -3,6 +3,7 @@ package agent import ( "context" "errors" + "fmt" ) type WorkspaceWriteResult struct { @@ -22,3 +23,10 @@ var ( ErrWorkspaceWriteRejected = errors.New("workspace write rejected") ErrWorkspaceWriteUncertain = errors.New("workspace write outcome uncertain") ) + +// Known Files.create destination refusals. Both wrap ErrWorkspaceWriteRejected: +// nothing was installed. +var ( + ErrWorkspaceWriteDirectory = fmt.Errorf("%w: destination is a directory", ErrWorkspaceWriteRejected) + ErrWorkspaceWriteUnsafe = fmt.Errorf("%w: destination exists or its path is not a plain directory chain", ErrWorkspaceWriteRejected) +) diff --git a/apps/parsar-daemon/internal/dispatch/workspace_write.go b/apps/parsar-daemon/internal/dispatch/workspace_write.go index 338e3bdd8..751cdb807 100644 --- a/apps/parsar-daemon/internal/dispatch/workspace_write.go +++ b/apps/parsar-daemon/internal/dispatch/workspace_write.go @@ -137,6 +137,17 @@ func workspaceWriteResult(write agent.WorkspaceWriteResult, err error, size int) if err == nil && write.SizeBytes == int64(size) { return proto.WorkspaceWriteResultPayload{Outcome: "completed", SizeBytes: size} } + for _, conflict := range []struct { + err error + reason string + }{ + {agent.ErrWorkspaceWriteDirectory, proto.WorkspaceWriteReasonDirectory}, + {agent.ErrWorkspaceWriteUnsafe, proto.WorkspaceWriteReasonUnsafe}, + } { + if errors.Is(err, conflict.err) { + return proto.WorkspaceWriteResultPayload{Outcome: "rejected", ErrorCode: "write_rejected", Reason: conflict.reason} + } + } for _, failure := range []struct { err error code string diff --git a/apps/parsar-daemon/internal/dispatch/workspace_write_test.go b/apps/parsar-daemon/internal/dispatch/workspace_write_test.go index d33f46688..d3d71f805 100644 --- a/apps/parsar-daemon/internal/dispatch/workspace_write_test.go +++ b/apps/parsar-daemon/internal/dispatch/workspace_write_test.go @@ -165,3 +165,29 @@ func TestLocalUploadUnknownRetainsOwner(t *testing.T) { t.Fatal("shutdown declared uncertain mutation settled") } } + +func TestLocalUploadReportsDestinationConflictsAndReleasesOwner(t *testing.T) { + for helperError, reason := range map[string]string{ + "destination_directory": proto.WorkspaceWriteReasonDirectory, + "unsafe_destination": proto.WorkspaceWriteReasonUnsafe, + "write_failed": "", + } { + t.Run(helperError, func(t *testing.T) { + r, sender, request, _ := localWriterRouter(t, `{"version":1,"outcome":"failed","error":"`+helperError+`"}`) + id := uuid.NewString() + for _, p := range []proto.WorkspaceWritePayload{request, {Step: "chunk", Data: []byte("abc")}, {Step: "commit"}} { + if err := r.Handle(t.Context(), mustEnv(t, proto.TypeWorkspaceWrite, id, p)); err != nil { + t.Fatal(err) + } + } + if got := waitWorkspaceWrite(t, sender, id, "rejected"); got.ErrorCode != "write_rejected" || got.Reason != reason { + t.Fatal(got) + } + next := uuid.NewString() + if err := r.Handle(t.Context(), mustEnv(t, proto.TypeWorkspaceWrite, next, request)); err != nil { + t.Fatal(err) + } + waitWorkspaceWrite(t, sender, next, "ready") + }) + } +} diff --git a/apps/parsar-daemon/internal/localworkspace/write.go b/apps/parsar-daemon/internal/localworkspace/write.go index fc391c51a..15aaf726d 100644 --- a/apps/parsar-daemon/internal/localworkspace/write.go +++ b/apps/parsar-daemon/internal/localworkspace/write.go @@ -52,7 +52,9 @@ func (b *Binding) WriteWorkspaceFile(ctx context.Context, path string, data []by operation, cancel := context.WithTimeout(context.WithoutCancel(ctx), 65*time.Second) defer cancel() digest := sha256.Sum256(data) - cmd := exec.CommandContext(operation, w.helper, b.workspace, path, strconv.Itoa(len(data)), w.staging) + // This writer serves only public Files.create, so it always selects the + // helper's create mode: create missing parents and never replace. + cmd := exec.CommandContext(operation, w.helper, b.workspace, path, strconv.Itoa(len(data)), w.staging, "create") cmd.Dir = "/" cmd.Env = []string{"PATH=/usr/bin:/bin", "LANG=C.UTF-8"} cmd.Stdin = io.MultiReader(bytes.NewReader(data), bytes.NewReader(digest[:])) @@ -96,8 +98,15 @@ func decodeWrite(data []byte, expected int) (agent.WorkspaceWriteResult, error) if wire.Outcome == "completed" && wire.Error == nil && wire.SizeBytes != nil && *wire.SizeBytes == int64(expected) { return agent.WorkspaceWriteResult{SizeBytes: *wire.SizeBytes}, nil } - if wire.Outcome == "failed" && wire.SizeBytes == nil && wire.Error != nil && (*wire.Error == "invalid_input" || *wire.Error == "write_failed") { - return agent.WorkspaceWriteResult{}, agent.ErrWorkspaceWriteRejected + if wire.Outcome == "failed" && wire.SizeBytes == nil && wire.Error != nil { + switch *wire.Error { + case "invalid_input", "write_failed": + return agent.WorkspaceWriteResult{}, agent.ErrWorkspaceWriteRejected + case "destination_directory": + return agent.WorkspaceWriteResult{}, agent.ErrWorkspaceWriteDirectory + case "unsafe_destination": + return agent.WorkspaceWriteResult{}, agent.ErrWorkspaceWriteUnsafe + } } return agent.WorkspaceWriteResult{}, invalid } diff --git a/apps/parsar-daemon/internal/localworkspace/write_test.go b/apps/parsar-daemon/internal/localworkspace/write_test.go index 70f83121b..774208332 100644 --- a/apps/parsar-daemon/internal/localworkspace/write_test.go +++ b/apps/parsar-daemon/internal/localworkspace/write_test.go @@ -60,7 +60,8 @@ func TestWriteRejectsUnsafeBindings(t *testing.T) { func TestWriteReceiptAndCredentialBoundary(t *testing.T) { t.Setenv("PARSAR_PRIVATE_CREDENTIAL", "synthetic-secret") - b := writableBinding(t, "[ -z \"$PARSAR_PRIVATE_CREDENTIAL\" ] || exit 13\ncat >/dev/null\nprintf '%s' '{\"version\":1,\"outcome\":\"completed\",\"size_bytes\":3}'\n") + // Files.create always selects the helper's explicit create mode. + b := writableBinding(t, "[ -z \"$PARSAR_PRIVATE_CREDENTIAL\" ] || exit 13\n[ \"$#\" = 5 ] && [ \"$5\" = create ] || exit 14\ncat >/dev/null\nprintf '%s' '{\"version\":1,\"outcome\":\"completed\",\"size_bytes\":3}'\n") result, err := b.WriteWorkspaceFile(t.Context(), "file", []byte{0, 1, 2}) if err != nil || result.SizeBytes != 3 { t.Fatalf("write: %+v %v", result, err) @@ -144,14 +145,22 @@ func TestWriteReceiptValidation(t *testing.T) { `{"version":1,"outcome":"failed","error":"other"}`, `{"version":1,"outcome":"completed","size_bytes":0} {}`, `{"version":1,"outcome":"completed","size_bytes":0,"extra":true}`, + `{"version":1,"outcome":"unknown","error":"unsafe_destination"}`, + `{"version":1,"outcome":"failed","size_bytes":0,"error":"destination_directory"}`, } { if _, err := decodeWrite([]byte(response), 0); !errors.Is(err, agent.ErrWorkspaceWriteUncertain) { t.Fatalf("unsafe receipt %s: %v", response, err) } } - for _, code := range []string{"invalid_input", "write_failed"} { - if _, err := decodeWrite([]byte(fmt.Sprintf(`{"version":1,"outcome":"failed","error":%q}`, code)), 0); !errors.Is(err, agent.ErrWorkspaceWriteRejected) { - t.Fatal(err) + for code, want := range map[string]error{ + "invalid_input": agent.ErrWorkspaceWriteRejected, + "write_failed": agent.ErrWorkspaceWriteRejected, + "destination_directory": agent.ErrWorkspaceWriteDirectory, + "unsafe_destination": agent.ErrWorkspaceWriteUnsafe, + } { + _, err := decodeWrite([]byte(fmt.Sprintf(`{"version":1,"outcome":"failed","error":%q}`, code)), 0) + if err != want || !errors.Is(err, agent.ErrWorkspaceWriteRejected) { + t.Fatal(code, err) } } } @@ -165,40 +174,47 @@ func TestLocalWriteNativeInstaller(t *testing.T) { if err := b.bindWriter(helper, b.writer.staging); err != nil { t.Fatal(err) } + escape := filepath.Join(b.workspace, "escape") + if err := os.Symlink(b.writer.staging, escape); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(b.workspace, "setup.txt"), []byte("setup"), 0600); err != nil { + t.Fatal(err) + } + // Missing parents are created; an existing destination is never replaced. for _, size := range []int{0, 3, (1 << 20) + 17, WriteMaxBytes} { + path := fmt.Sprintf("n1/n2/file-%d", size) data := bytes.Repeat([]byte{0, 255, 17}, (size+2)/3)[:size] - got, err := b.WriteWorkspaceFile(t.Context(), "file", data) + got, err := b.WriteWorkspaceFile(t.Context(), path, data) if err != nil || got.SizeBytes != int64(size) { t.Fatalf("size %d: %+v %v", size, got, err) } - actual, err := os.ReadFile(filepath.Join(b.workspace, "file")) + actual, err := os.ReadFile(filepath.Join(b.workspace, path)) if err != nil || !bytes.Equal(actual, data) { t.Fatalf("size %d bytes differ: %v", size, err) } } - old := filepath.Join(b.workspace, "file") - if err := os.WriteFile(old, []byte("original"), 0600); err != nil { - t.Fatal(err) - } - alias := filepath.Join(b.workspace, "alias") - if err := os.Link(old, alias); err != nil { - t.Fatal(err) + if info, err := os.Lstat(filepath.Join(b.workspace, "n1")); err != nil || !info.IsDir() || info.Mode().Perm() != 0700 { + t.Fatal("parent directory", info, err) } - if _, err := b.WriteWorkspaceFile(t.Context(), "file", []byte("replacement")); err != nil { - t.Fatal(err) - } - if data, err := os.ReadFile(alias); err != nil || string(data) != "original" { - t.Fatal("hard-link contents changed", err) - } - escape := filepath.Join(b.workspace, "escape") - if err := os.Symlink(b.writer.staging, escape); err != nil { - t.Fatal(err) - } - for _, path := range []string{"escape/new", "escape", "missing/file"} { - if _, err := b.WriteWorkspaceFile(t.Context(), path, []byte("denied")); !errors.Is(err, agent.ErrWorkspaceWriteRejected) { + for path, want := range map[string]error{ + "n1/n2/file-3": agent.ErrWorkspaceWriteUnsafe, + "setup.txt": agent.ErrWorkspaceWriteUnsafe, + "escape": agent.ErrWorkspaceWriteUnsafe, + "escape/new": agent.ErrWorkspaceWriteUnsafe, + "n1/n2": agent.ErrWorkspaceWriteDirectory, + "n1": agent.ErrWorkspaceWriteDirectory, + } { + if _, err := b.WriteWorkspaceFile(t.Context(), path, []byte("denied")); err != want { t.Fatalf("%s: %v", path, err) } } + if data, err := os.ReadFile(filepath.Join(b.workspace, "n1/n2/file-3")); err != nil || !bytes.Equal(data, []byte{0, 255, 17}) { + t.Fatal("existing file changed", err) + } + if data, err := os.ReadFile(filepath.Join(b.workspace, "setup.txt")); err != nil || string(data) != "setup" { + t.Fatal("untracked file changed", err) + } if _, err := b.WriteWorkspaceFile(t.Context(), "after-rejection", nil); err != nil { t.Fatal("known rejection blocked next write", err) } diff --git a/internal/agentdaemon/gateway/workspace_write.go b/internal/agentdaemon/gateway/workspace_write.go index 42631b5ea..d614acfb3 100644 --- a/internal/agentdaemon/gateway/workspace_write.go +++ b/internal/agentdaemon/gateway/workspace_write.go @@ -79,6 +79,9 @@ func (s *Session) WriteWorkspaceFile(ctx context.Context, id string, request pro } func validWorkspaceWriteResult(r proto.WorkspaceWriteResultPayload, expected string, offset, size int) bool { + if r.Reason != "" && (r.Outcome != "rejected" || r.ErrorCode != "write_rejected") { + return false + } if r.Outcome == "rejected" { if r.Offset != 0 || r.SizeBytes != 0 { return false diff --git a/internal/agentdaemon/gateway/workspace_write_test.go b/internal/agentdaemon/gateway/workspace_write_test.go index 4dc98b5ee..3f7c90af3 100644 --- a/internal/agentdaemon/gateway/workspace_write_test.go +++ b/internal/agentdaemon/gateway/workspace_write_test.go @@ -94,11 +94,24 @@ func TestWorkspaceWriteRejectsPrematureOrContradictoryReceipts(t *testing.T) { {Outcome: "ready", Offset: 1}, {Outcome: "rejected", ErrorCode: "private-detail"}, {Outcome: "unknown", ErrorCode: "write_unconfirmed", SizeBytes: 4}, + // A destination reason refines only a known installer rejection. + {Outcome: "ready", Reason: proto.WorkspaceWriteReasonUnsafe}, + {Outcome: "rejected", ErrorCode: "invalid_request", Reason: proto.WorkspaceWriteReasonUnsafe}, + {Outcome: "unknown", ErrorCode: "write_unconfirmed", Reason: proto.WorkspaceWriteReasonDirectory}, } { if validWorkspaceWriteResult(result, "ready", 0, 4) { t.Fatal("unsafe result", result) } } + if validWorkspaceWriteResult(proto.WorkspaceWriteResultPayload{Outcome: "completed", SizeBytes: 4, Reason: proto.WorkspaceWriteReasonUnsafe}, "completed", 0, 4) { + t.Fatal("reason accepted on a commit") + } + // Unrecognized reasons from newer daemons remain generic known rejections. + for _, reason := range []string{"", proto.WorkspaceWriteReasonDirectory, proto.WorkspaceWriteReasonUnsafe, "future_reason"} { + if !validWorkspaceWriteResult(proto.WorkspaceWriteResultPayload{Outcome: "rejected", ErrorCode: "write_rejected", Reason: reason}, "completed", 0, 4) { + t.Fatal("known rejection refused", reason) + } + } if validWorkspaceWriteResult(proto.WorkspaceWriteResultPayload{Outcome: "received", Offset: 1}, "received", 2, 4) { t.Fatal("wrong offset accepted") } diff --git a/internal/agentdaemon/proto/workspace_write.go b/internal/agentdaemon/proto/workspace_write.go index 19afdbf45..13a594e30 100644 --- a/internal/agentdaemon/proto/workspace_write.go +++ b/internal/agentdaemon/proto/workspace_write.go @@ -34,8 +34,17 @@ type WorkspaceWriteResultPayload struct { Offset int `json:"offset,omitempty"` SizeBytes int `json:"size_bytes,omitempty"` ErrorCode string `json:"error_code,omitempty"` + // Reason optionally refines a write_rejected result. Older peers omit or + // ignore it and keep the generic rejection. + Reason string `json:"reason,omitempty"` } +// Files.create destination refusals carried in Reason. +const ( + WorkspaceWriteReasonDirectory = "destination_directory" + WorkspaceWriteReasonUnsafe = "unsafe_destination" +) + func ValidWorkspaceWriteRequest(p WorkspaceWritePayload) bool { if p.Step == "begin" { for _, id := range []string{p.EnvironmentID, p.SessionID} { From 4b5b6fa8a43491765034307fb6db980bfd377ed6 Mon Sep 17 00:00:00 2001 From: saladday <1203511142@qq.com> Date: Wed, 23 Sep 2026 18:17:32 +0000 Subject: [PATCH 3/6] Align Files.create inline bound and destination errors Inline data above 5 MiB decoded returns the official 400 before any Runtime work; file_id copies keep the 50 MiB destination bound. The Worker maps the installer's destination reasons to the official conflict and symlink or overwrite messages. Core keeps no path ledger, so an existing file written by an earlier Files.create uses the untracked message. Rejections settle their durable intent as rejected and consume no Source File. --- contracts/agents-api/openapi.yaml | 12 +- .../internal/api/environment_files_create.go | 35 +++- .../api/environment_files_write_test.go | 79 ++++++++ .../execution/environment_file_write.go | 15 +- ...onment_file_write_semantics_public_test.go | 177 ++++++++++++++++++ .../tests/official_environment_files.py | 19 ++ .../official_environment_files_create.py | 14 +- 7 files changed, 336 insertions(+), 15 deletions(-) create mode 100644 services/agents-api/internal/api/environment_files_write_test.go create mode 100644 services/agents-api/internal/store/environment_file_write_semantics_public_test.go diff --git a/contracts/agents-api/openapi.yaml b/contracts/agents-api/openapi.yaml index aa2602e9d..f5119a583 100644 --- a/contracts/agents-api/openapi.yaml +++ b/contracts/agents-api/openapi.yaml @@ -3311,11 +3311,13 @@ paths: source file_id through the same write path. Unknown body fields are rejected with their name as param. Basic public hosted creation requires explicit managed Runtime configuration; an openai_hosted Environment that has not connected - yet returns 400. A private 50 MiB decoded-content limit applies. The parent - directory must exist. Replacement installs a new mode-0600 inode; upstream - overwrite metadata semantics remain unverified. Idle writes exclude execution. - Missing receipts return unavailable and retain a durable mutation gate without - automatic replay. Error/timing parity with upstream remains unverified. + yet returns 400. Inline data is limited to 5 MiB decoded and a file_id copy + to 50 MiB. Missing parent directories are created with mode 0700 and the file + with mode 0600. An existing destination is never replaced; a directory, an + existing file or a path through a symlink or non-directory returns 400. Idle + writes exclude execution. Missing receipts return unavailable and retain a + durable mutation gate without automatic replay. Error/timing parity with upstream + remains unverified. parameters: - description: agents=v1 in: header diff --git a/services/agents-api/internal/api/environment_files_create.go b/services/agents-api/internal/api/environment_files_create.go index 8b57122b9..9184b211a 100644 --- a/services/agents-api/internal/api/environment_files_create.go +++ b/services/agents-api/internal/api/environment_files_create.go @@ -5,6 +5,7 @@ import ( "context" "encoding/base64" "encoding/json" + "errors" "io" "net/http" "slices" @@ -29,7 +30,7 @@ func WithEnvironmentFileWriter(writer EnvironmentFileWriter) Option { } // @Summary Create an Environment file from inline bytes or a source file -// @Description Uploads standard Base64 bytes to a file beneath /workspace in a qualified local Environment and returns 201. Accepts inline bytes or a project-owned source file_id through the same write path. Unknown body fields are rejected with their name as param. Basic public hosted creation requires explicit managed Runtime configuration; an openai_hosted Environment that has not connected yet returns 400. A private 50 MiB decoded-content limit applies. The parent directory must exist. Replacement installs a new mode-0600 inode; upstream overwrite metadata semantics remain unverified. Idle writes exclude execution. Missing receipts return unavailable and retain a durable mutation gate without automatic replay. Error/timing parity with upstream remains unverified. +// @Description Uploads standard Base64 bytes to a file beneath /workspace in a qualified local Environment and returns 201. Accepts inline bytes or a project-owned source file_id through the same write path. Unknown body fields are rejected with their name as param. Basic public hosted creation requires explicit managed Runtime configuration; an openai_hosted Environment that has not connected yet returns 400. Inline data is limited to 5 MiB decoded and a file_id copy to 50 MiB. Missing parent directories are created with mode 0700 and the file with mode 0600. An existing destination is never replaced; a directory, an existing file or a path through a symlink or non-directory returns 400. Idle writes exclude execution. Missing receipts return unavailable and retain a durable mutation gate without automatic replay. Error/timing parity with upstream remains unverified. // @Tags Environments // @Accept json // @Produce json @@ -97,8 +98,8 @@ func (h *Handler) createEnvironmentFile(w http.ResponseWriter, r *http.Request) writeStoreError(w, r, store.ErrInvalidInput) return } - if len(data) > proto.WorkspaceWriteMaxBytes { - writeStoreError(w, r, store.ErrSourceFileTooLarge) + if len(data) > maxInlineEnvironmentFileBytes { + writeFieldError(w, errEnvironmentFileInlineTooLarge) return } } @@ -136,7 +137,9 @@ func (h *Handler) createEnvironmentFile(w http.ResponseWriter, r *http.Request) } size, err := h.fileWriter.WriteEnvironmentFile(r.Context(), environment, strings.TrimPrefix(*request.Path, "/workspace/"), data) if err != nil { - writeStoreError(w, r, err) + if !writeFieldError(w, environmentFileWriteError(err)) { + writeStoreError(w, r, err) + } return } if size != int64(len(data)) { @@ -152,6 +155,30 @@ var ( errEnvironmentFileCreateComponents = &fieldError{message: "environment.files[0].path cannot contain empty, . or .. path components"} ) +// maxInlineEnvironmentFileBytes is the official decoded inline bound (HE-15), +// measured before any Runtime work. file_id copies keep the 50 MiB destination bound. +const maxInlineEnvironmentFileBytes = 5 << 20 + +// Official Files.create size and destination errors (HE-12, HE-13, HE-15). +var ( + errEnvironmentFileInlineTooLarge = &fieldError{message: "environment.files[0].data exceeds the 5 MiB decoded limit"} + errEnvironmentFileConflict = &fieldError{message: "file path conflicts with an existing environment file"} + errEnvironmentFileUnsafe = &fieldError{message: "environment.files paths must not traverse symlinks or overwrite existing files"} +) + +// environmentFileWriteError maps the installer's known destination refusals. +// Core keeps no path ledger of earlier writes, so an existing regular file uses +// the untracked-file message even when an earlier Files.create wrote it. +func environmentFileWriteError(err error) error { + switch { + case errors.Is(err, execution.ErrEnvironmentFileDirectory): + return errEnvironmentFileConflict + case errors.Is(err, execution.ErrEnvironmentFileUnsafe): + return errEnvironmentFileUnsafe + } + return nil +} + // environmentFileCreatePathError accepts exactly the canonical absolute paths // below /workspace; the accepted set is unchanged, only the errors are specific. func environmentFileCreatePathError(value string) error { diff --git a/services/agents-api/internal/api/environment_files_write_test.go b/services/agents-api/internal/api/environment_files_write_test.go new file mode 100644 index 000000000..db9437177 --- /dev/null +++ b/services/agents-api/internal/api/environment_files_write_test.go @@ -0,0 +1,79 @@ +package api + +import ( + "bytes" + "encoding/base64" + "encoding/json" + "testing" + "time" + + "github.com/MiniMax-AI-Dev/parsar/services/agents-api/internal/execution" + "github.com/MiniMax-AI-Dev/parsar/services/agents-api/internal/store" + "github.com/google/uuid" +) + +// Files.create write semantics rows FW2–FW6 of the workspace-file-writes batch. + +func inlineCreateBody(size int) string { + body, _ := json.Marshal(map[string]any{"type": "inline", "path": "/workspace/n1/n2/big.bin", "data": base64.StdEncoding.EncodeToString(bytes.Repeat([]byte{7}, size))}) + return string(body) +} + +func TestEnvironmentFileCreateInlineDecodedLimit(t *testing.T) { + const message = "environment.files[0].data exceeds the 5 MiB decoded limit" + h, f := environmentFileCreateHandler(t) + if w := requestCreateEnvironmentFile(h, f.environment.ID, inlineCreateBody(5<<20), "files-key"); w.Code != 201 || f.writes != 1 || len(f.data) != 5<<20 || f.path != "n1/n2/big.bin" { + t.Fatal("exact 5 MiB inline rejected", w.Code, w.Body) + } + for _, size := range []int{5<<20 + 1, 50 << 20} { + assertListQueryError(t, requestCreateEnvironmentFile(h, f.environment.ID, inlineCreateBody(size), "files-key"), "invalid_request_error", nil, message) + } + // The bound is checked before provisioning or any Runtime work. + f.environment.Configuration, f.environment.Status = json.RawMessage(`{"type":"openai_hosted","network":{"access":"disabled"}}`), "pending" + assertListQueryError(t, requestCreateEnvironmentFile(h, f.environment.ID, inlineCreateBody(5<<20+1), "files-key"), "invalid_request_error", nil, message) + if f.writes != 1 { + t.Fatal("oversized inline data reached the writer", f.writes) + } + // Tenant isolation precedes the body. + foreign := requestCreateEnvironmentFile(h, f.environment.ID, inlineCreateBody(5<<20+1), "other-key") + missing := requestCreateEnvironmentFile(h, uuid.NewString(), inlineCreateBody(5<<20+1), "files-key") + if foreign.Code != 404 || foreign.Body.String() != missing.Body.String() { + t.Fatal("foreign oversized create inspected", foreign.Code, foreign.Body) + } +} + +func TestEnvironmentFileCreateSourceCopyKeepsDestinationBound(t *testing.T) { + sources := &sourceFilesFixture{} + h, f := environmentFileCreateHandler(t, WithSourceFiles(sources)) + data := bytes.Repeat([]byte{9}, 6<<20) + sources.tenant, sources.data = f.environment.TenantID, data + sources.file = store.SourceFile{ID: "file-" + uuid.NewString(), SizeBytes: int64(len(data)), CreatedAt: time.Unix(1, 0)} + body := `{"type":"file_id","file_id":"` + sources.file.ID + `","path":"/workspace/copy.bin"}` + if w := requestCreateEnvironmentFile(h, f.environment.ID, body, "files-key"); w.Code != 201 || f.writes != 1 || !bytes.Equal(f.data, data) { + t.Fatal("6 MiB source copy rejected", w.Code, w.Body) + } +} + +func TestEnvironmentFileCreateDestinationConflicts(t *testing.T) { + const conflict = "file path conflicts with an existing environment file" + const unsafe = "environment.files paths must not traverse symlinks or overwrite existing files" + h, f := environmentFileCreateHandler(t) + body := `{"type":"inline","data":"YWJj","path":"/workspace/n1"}` + for _, tc := range []struct { + err error + code any + message string + }{ + {execution.ErrEnvironmentFileDirectory, "invalid_request_error", conflict}, + {execution.ErrEnvironmentFileUnsafe, "invalid_request_error", unsafe}, + // A generic installer rejection keeps the local code. + {store.ErrInvalidInput, "invalid_request", "Invalid resource identifier or request limits."}, + } { + f.err = tc.err + assertListQueryError(t, requestCreateEnvironmentFile(h, f.environment.ID, body, "files-key"), tc.code, nil, tc.message) + } + f.err = execution.ErrExecutionUnavailable + if w := requestCreateEnvironmentFile(h, f.environment.ID, body, "files-key"); w.Code != 503 { + t.Fatal("unknown outcome changed", w.Code, w.Body) + } +} diff --git a/services/agents-api/internal/execution/environment_file_write.go b/services/agents-api/internal/execution/environment_file_write.go index c28e58ce7..78df59e15 100644 --- a/services/agents-api/internal/execution/environment_file_write.go +++ b/services/agents-api/internal/execution/environment_file_write.go @@ -5,6 +5,7 @@ import ( "crypto/sha256" "encoding/hex" "encoding/json" + "fmt" "time" "github.com/MiniMax-AI-Dev/parsar/internal/agentdaemon/proto" @@ -12,6 +13,13 @@ import ( "github.com/google/uuid" ) +// Known Files.create destination refusals reported by the Runtime installer. +// They wrap store.ErrInvalidInput; the rejected write installed nothing. +var ( + ErrEnvironmentFileDirectory = fmt.Errorf("%w: environment file destination is a directory", store.ErrInvalidInput) + ErrEnvironmentFileUnsafe = fmt.Errorf("%w: environment file destination exists or traverses a link", store.ErrInvalidInput) +) + type fileWriteResult struct { size int64 err error @@ -117,7 +125,12 @@ func (w *Worker) runFileWrite(owner context.Context, request fileWriteRequest) f return unavailable } if state == "rejected" { - if result.ErrorCode == "invalid_request" || result.ErrorCode == "write_rejected" { + switch { + case result.ErrorCode == "write_rejected" && result.Reason == proto.WorkspaceWriteReasonDirectory: + return fileWriteResult{err: ErrEnvironmentFileDirectory} + case result.ErrorCode == "write_rejected" && result.Reason == proto.WorkspaceWriteReasonUnsafe: + return fileWriteResult{err: ErrEnvironmentFileUnsafe} + case result.ErrorCode == "invalid_request" || result.ErrorCode == "write_rejected": return fileWriteResult{err: store.ErrInvalidInput} } return unavailable diff --git a/services/agents-api/internal/store/environment_file_write_semantics_public_test.go b/services/agents-api/internal/store/environment_file_write_semantics_public_test.go new file mode 100644 index 000000000..4c51697dd --- /dev/null +++ b/services/agents-api/internal/store/environment_file_write_semantics_public_test.go @@ -0,0 +1,177 @@ +package store_test + +import ( + "bytes" + "encoding/base64" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "reflect" + "strings" + "testing" + "time" + + "github.com/MiniMax-AI-Dev/parsar/internal/agentdaemon/device" + "github.com/MiniMax-AI-Dev/parsar/internal/agentdaemon/proto" + "github.com/MiniMax-AI-Dev/parsar/services/agents-api/internal/api" + "github.com/MiniMax-AI-Dev/parsar/services/agents-api/internal/store" + "github.com/google/uuid" +) + +type fileCreateResponse struct { + status int + body string +} + +// serveFileWrite answers one complete daemon transfer with the final result. +func serveFileWrite(h *dispatchHarness, final proto.WorkspaceWriteResultPayload) string { + h.t.Helper() + begin := h.read(proto.TypeWorkspaceWrite) + var request proto.WorkspaceWritePayload + if begin.DecodePayload(&request) != nil || request.Step != "begin" { + h.t.Fatal("write did not begin") + } + h.write(begin.ID, proto.TypeWorkspaceWriteResult, proto.WorkspaceWriteResultPayload{Outcome: "ready"}) + for offset := 0; offset < request.SizeBytes; { + chunk := h.read(proto.TypeWorkspaceWrite) + var body proto.WorkspaceWritePayload + if chunk.ID != begin.ID || chunk.DecodePayload(&body) != nil || body.Step != "chunk" { + h.t.Fatal("chunk changed") + } + offset += len(body.Data) + h.write(begin.ID, proto.TypeWorkspaceWriteResult, proto.WorkspaceWriteResultPayload{Outcome: "received", Offset: offset}) + } + if commit := h.read(proto.TypeWorkspaceWrite); commit.ID != begin.ID { + h.t.Fatal("commit changed") + } + h.write(begin.ID, proto.TypeWorkspaceWriteResult, final) + return begin.ID +} + +func TestEnvironmentFileCreateRejectionsLeaveNoReceiptOrConsumption(t *testing.T) { + h, w, environment := localWorker(t, true, false) + _, pool := store.NewTestStore(t) + if _, err := pool.Exec(t.Context(), `UPDATE environments SET status='connected' WHERE id=$1`, environment.ID); err != nil { + t.Fatal(err) + } + token, other := uuid.NewString(), uuid.NewString() + auth, err := api.NewAuthenticator([]api.APIKey{ + {OrganizationID: "test-org", ProjectID: uuid.NewString(), SubjectKind: "service_account", SubjectID: "test-runner", TokenSHA256: device.HashCredential(token), TenantID: h.tenant}, + {OrganizationID: "test-org", ProjectID: uuid.NewString(), SubjectKind: "service_account", SubjectID: "tenant-b", TokenSHA256: device.HashCredential(other), TenantID: uuid.NewString()}, + }) + if err != nil { + t.Fatal(err) + } + handler, err := api.NewHandler(h.s, auth, "codex", api.WithEnvironmentFileWriter(w), api.WithSourceFiles(h.s)) + if err != nil { + t.Fatal(err) + } + server := httptest.NewServer(handler) + defer server.Close() + source, err := h.s.CreateSourceFile(t.Context(), h.tenant, func(out io.Writer) (store.SourceFileUpload, error) { + _, err := out.Write([]byte("src")) + return store.SourceFileUpload{Filename: "source.txt", Purpose: "user_data"}, err + }) + if err != nil { + t.Fatal(err) + } + post := func(key, id, body string) <-chan fileCreateResponse { + done := make(chan fileCreateResponse, 1) + go func() { + r, _ := http.NewRequest(http.MethodPost, server.URL+"/v1/agents/environments/"+id+"/files", strings.NewReader(body)) + r.Header.Set("Authorization", "Bearer "+key) + r.Header.Set("OpenAI-Beta", "agents=v1") + resp, err := server.Client().Do(r) + if err != nil { + done <- fileCreateResponse{} + return + } + defer resp.Body.Close() + data, _ := io.ReadAll(resp.Body) + done <- fileCreateResponse{resp.StatusCode, string(data)} + }() + return done + } + await := func(done <-chan fileCreateResponse) fileCreateResponse { + select { + case got := <-done: + return got + case <-time.After(10 * time.Second): + t.Fatal("create did not return") + return fileCreateResponse{} + } + } + states := func() map[string]int { + rows, err := pool.Query(t.Context(), `SELECT state, count(*) FROM environment_file_writes WHERE environment_id=$1 GROUP BY state`, environment.ID) + if err != nil { + t.Fatal(err) + } + defer rows.Close() + result := map[string]int{} + for rows.Next() { + var state string + var count int + if err := rows.Scan(&state, &count); err != nil { + t.Fatal(err) + } + result[state] = count + } + return result + } + assertError := func(got fileCreateResponse, message string) { + t.Helper() + var body map[string]any + want := map[string]any{"error": map[string]any{"type": "invalid_request_error", "code": "invalid_request_error", "param": nil, "message": message}} + if got.status != 400 || json.Unmarshal([]byte(got.body), &body) != nil || !reflect.DeepEqual(body, want) { + t.Fatalf("status=%d body=%s want=%v", got.status, got.body, want) + } + } + inline := `{"type":"inline","data":"YWJj","path":"/workspace/n1/existing.txt"}` + copyBody := `{"type":"file_id","file_id":"` + source.ID + `","path":"/workspace/link/copy.txt"}` + rejected := func(reason string) proto.WorkspaceWriteResultPayload { + return proto.WorkspaceWriteResultPayload{Outcome: "rejected", ErrorCode: "write_rejected", Reason: reason} + } + for _, tc := range []struct { + body, reason, message string + }{ + {`{"type":"inline","data":"YWJj","path":"/workspace/n1"}`, proto.WorkspaceWriteReasonDirectory, "file path conflicts with an existing environment file"}, + {inline, proto.WorkspaceWriteReasonUnsafe, "environment.files paths must not traverse symlinks or overwrite existing files"}, + {copyBody, proto.WorkspaceWriteReasonUnsafe, "environment.files paths must not traverse symlinks or overwrite existing files"}, + } { + done := post(token, environment.ID, tc.body) + id := serveFileWrite(h, rejected(tc.reason)) + assertError(await(done), tc.message) + if intent, err := h.s.GetEnvironmentFileWrite(t.Context(), h.tenant, environment.ID, id); err != nil || intent.State != "rejected" { + t.Fatal("rejection did not settle", intent, err) + } + } + // The rejected copy did not consume its Source File. + if got, err := h.s.GetSourceFile(t.Context(), h.tenant, source.ID); err != nil || got.SizeBytes != 3 { + t.Fatal("source file consumed", got, err) + } + // The inline bound is checked before any mutation intent or dispatch. + oversized, _ := json.Marshal(map[string]string{"type": "inline", "path": "/workspace/big.bin", "data": base64.StdEncoding.EncodeToString(bytes.Repeat([]byte{1}, 5<<20+1))}) + assertError(await(post(token, environment.ID, string(oversized))), "environment.files[0].data exceeds the 5 MiB decoded limit") + // Tenant B sees the Environment as missing, before body inspection. + foreign, missing := await(post(other, environment.ID, inline)), await(post(token, uuid.NewString(), inline)) + if foreign.status != 404 || foreign != missing { + t.Fatal("tenant B reached the Environment", foreign, missing) + } + if got := states(); !reflect.DeepEqual(got, map[string]int{"rejected": 3}) { + t.Fatal("rejections left a receipt or blocking intent", got) + } + // Known rejections release the mutation owner for a successor. + done := post(token, environment.ID, copyBody) + serveFileWrite(h, proto.WorkspaceWriteResultPayload{Outcome: "completed", SizeBytes: 3}) + if got := await(done); got.status != 201 { + t.Fatal("successor rejected", got) + } + if got := states(); !reflect.DeepEqual(got, map[string]int{"rejected": 3, "committed": 1}) { + t.Fatal("successor receipt", got) + } + session, err := h.s.GetSession(t.Context(), h.tenant, h.session.ID) + if err != nil || session.LastTurn != nil { + t.Fatal("file writes created model execution", err) + } +} diff --git a/services/agents-api/tests/official_environment_files.py b/services/agents-api/tests/official_environment_files.py index ad52a6f88..af51892f3 100644 --- a/services/agents-api/tests/official_environment_files.py +++ b/services/agents-api/tests/official_environment_files.py @@ -1,5 +1,6 @@ """Pinned Files.list checks for a known, unchanged directory of regular files.""" +import base64 from pathlib import PurePosixPath from urllib.parse import urlencode @@ -202,6 +203,24 @@ def verify_file_create_rows(client, http, environment_id, directory): return path +def verify_file_write_rows(client, http, environment_id, directory): + """Files.create write rows FW1-FW6: parent creation, no replacement and the inline bound.""" + endpoint = str(client.base_url).rstrip("/") + "/agents/environments/" + environment_id + "/files" + headers = {"Authorization": "Bearer " + client.api_key, "OpenAI-Beta": "agents=v1"} + path = directory + "/n1/n2/nested.txt" + response = http.post(endpoint, headers=headers, json={"type": "inline", "data": "bmVzdGVk", "path": path}) + assert response.status_code == 201, "Files.create did not create missing parents" + assert response.json() == {"environment_id": environment_id, "object": "agent.environment.file", "path": path, "size_bytes": 6} + response = http.post(endpoint, headers=headers, json={"type": "inline", "data": "b3RoZXI=", "path": path}) + assert_invalid_request(response, "environment.files paths must not traverse symlinks or overwrite existing files") + response = http.post(endpoint, headers=headers, json={"type": "inline", "data": "cg==", "path": directory + "/n1"}) + assert_invalid_request(response, "file path conflicts with an existing environment file") + oversized = base64.b64encode(b"x" * ((5 << 20) + 1)).decode() + response = http.post(endpoint, headers=headers, json={"type": "inline", "data": oversized, "path": directory + "/oversized.bin"}) + assert_invalid_request(response, "environment.files[0].data exceeds the 5 MiB decoded limit") + return path + + def verify_files_provisioning(client, http, environment_id): """Files.list/create on a pending hosted Environment (F9). Returns None when it connected first.""" endpoint = str(client.base_url).rstrip("/") + "/agents/environments/" + environment_id + "/files" diff --git a/services/agents-api/tests/official_environment_files_create.py b/services/agents-api/tests/official_environment_files_create.py index 93b20f6b8..d551c7e25 100644 --- a/services/agents-api/tests/official_environment_files_create.py +++ b/services/agents-api/tests/official_environment_files_create.py @@ -16,7 +16,7 @@ import httpx2 from openai import OpenAI from official_environment_files import (verify_environment_files, verify_file_create_rows, verify_file_list_rows, - verify_file_tenant_isolation) + verify_file_tenant_isolation, verify_file_write_rows) from official_environment_files_native import caller_token from official_source_files import verify_source_files @@ -37,9 +37,11 @@ def main(): "empty.bin": b"", "binary.bin": bytes(range(256)), "chunked.bin": bytes(range(256)) * 8193, - "large.bin": b"x" * (50 << 20), + # Inline data is bounded at 5 MiB decoded; source copies keep 50 MiB. + "large.bin": b"x" * (5 << 20), "model-input.txt": settings["model_input"].encode(), } + source_cases = {**cases, "large.bin": b"x" * (50 << 20)} expected, receipts = {}, [] with httpx2.Client(trust_env=False, timeout=215) as http: client = OpenAI(api_key=token, base_url=base, max_retries=0, _strict_response_validation=True, http_client=http) @@ -57,7 +59,7 @@ def main(): assert receipt == {"environment_id": environment, "object": "agent.environment.file", "path": path, "size_bytes": len(content)}, "Wrong upload metadata" expected[path] = len(content) receipts.append({"path": path, "size": len(content), "sha256": hashlib.sha256(content).hexdigest()}) - source_expected, source_receipts, source_proof = verify_source_files(client, foreign, http, environment, directory, cases) + source_expected, source_receipts, source_proof = verify_source_files(client, foreign, http, environment, directory, source_cases) expected.update(source_expected) receipts.extend(source_receipts) pages, continuation = verify_environment_files(client, http, environment, directory, expected) @@ -69,6 +71,8 @@ def main(): "symlink": "/workspace/stage-link"}) created = verify_file_create_rows(client, http, environment, directory) receipts.append({"path": created, "size": 3, "sha256": hashlib.sha256(b"201").hexdigest()}) + nested = verify_file_write_rows(client, http, environment, directory) + receipts.append({"path": nested, "size": 6, "sha256": hashlib.sha256(b"nested").hexdigest()}) for body in ( {"type": "inline", "data": "?", "path": target}, {"type": "inline", "data": "", "path": "/workspace/../escape"}, @@ -84,8 +88,8 @@ def main(): assert response.status_code == 404, "Foreign tenant upload accepted" assert token not in response.text and foreign_token not in response.text, "Credential leaked in error" print(json.dumps({"sdk": pin["sdk_version"], "commit": pin["commit"], "uploads": receipts, "listing": pages, "sources": source_proof, - "wire_rows": wire_rows + ["create_201_and_field_errors"], - "limits": ["Private Environment setup", "Other source purposes/expiration/listing unimplemented", "Overwrite metadata/error parity unverified", "Real-model consumption verified by invoking fixture"]})) + "wire_rows": wire_rows + ["create_201_and_field_errors", "create_parents_no_replace_inline_bound"], + "limits": ["Private Environment setup", "Other source purposes/expiration/listing unimplemented", "Tracked and untracked existing files share the untracked message", "Real-model consumption verified by invoking fixture"]})) if __name__ == "__main__": From 15d1a330b6f0a48d18bba5583d4c0601e149a0ae Mon Sep 17 00:00:00 2001 From: saladday <1203511142@qq.com> Date: Wed, 23 Sep 2026 18:17:32 +0000 Subject: [PATCH 4/6] Apply the 5 MiB inline Environment file bound in the client and Web The client's strict Base64 check now scans with a flat character class, since the grouped pattern overflowed the regular-expression stack near 4 MiB and blocked an exact 5 MiB upload. --- .../EnvironmentFileCreatePanel.test.tsx | 3 ++- .../environment/EnvironmentFileCreatePanel.tsx | 4 ++-- .../environment/environment-file-create.ts | 9 +++++---- packages/agents-client/src/client.test.ts | 15 +++++++++++++++ packages/agents-client/src/client.ts | 16 +++++++++++----- 5 files changed, 35 insertions(+), 12 deletions(-) diff --git a/apps/web/src/features/sessions/environment/EnvironmentFileCreatePanel.test.tsx b/apps/web/src/features/sessions/environment/EnvironmentFileCreatePanel.test.tsx index 52ffafded..834abdc4c 100644 --- a/apps/web/src/features/sessions/environment/EnvironmentFileCreatePanel.test.tsx +++ b/apps/web/src/features/sessions/environment/EnvironmentFileCreatePanel.test.tsx @@ -44,7 +44,8 @@ describe("EnvironmentFileCreatePanel", () => { expect(validInlineEnvironmentFilePath(`/workspace/${"界".repeat(1_363)}`)).toBe(false); }); - it("enforces the decoded 50 MiB boundary and encodes multibyte bytes as standard Base64", () => { + it("enforces the decoded 5 MiB boundary and encodes multibyte bytes as standard Base64", () => { + expect(maxInlineEnvironmentFileBytes).toBe(5 * 1024 * 1024); expect(validInlineEnvironmentFileByteLength(maxInlineEnvironmentFileBytes)).toBe(true); expect(validInlineEnvironmentFileByteLength(maxInlineEnvironmentFileBytes + 1)).toBe(false); expect(validInlineEnvironmentFileByteLength(-1)).toBe(false); diff --git a/apps/web/src/features/sessions/environment/EnvironmentFileCreatePanel.tsx b/apps/web/src/features/sessions/environment/EnvironmentFileCreatePanel.tsx index cd3d7feb1..035c1096d 100644 --- a/apps/web/src/features/sessions/environment/EnvironmentFileCreatePanel.tsx +++ b/apps/web/src/features/sessions/environment/EnvironmentFileCreatePanel.tsx @@ -101,7 +101,7 @@ export function EnvironmentFileCreatePanel({