From e8b47f832fe272f9a51ea568446e43b77a86ef62 Mon Sep 17 00:00:00 2001 From: Wolfgang Schoenberger <221313372+wolfiesch@users.noreply.github.com> Date: Wed, 7 Oct 2026 23:00:59 -0700 Subject: [PATCH 1/2] Keep upload file names and readable bytes for file inputs Stage each upload as / so pages see the real file name and MIME type, and keep delivered uploads on disk for 10 minutes because Chrome reads file input bytes only when the page uses them. A starting host sweeps per-upload directories older than 15 minutes. --- CHANGELOG.md | 1 + docs/security.md | 2 +- .../crates/agenttab-host/src/guardrails.rs | 174 +++++++++++++++++- host-rs/crates/agenttab-host/src/runtime.rs | 25 ++- 4 files changed, 184 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b3b0704..1a6261c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ # Changelog ## Unreleased +- Fixed `upload_file` failing on sites that check the uploaded file. The host staged every upload as `.upload`, so pages saw a file with no extension or MIME type and rejected it, for example GitHub's avatar upload ("Only images, please"). It also deleted the staged copy as soon as the action returned, but Chrome reads a file input's bytes only when the page reads or submits the file, so uploads that passed the type check then failed to send. Staged copies now keep the source file name inside a private per-upload directory and stay readable for 10 minutes after they reach the page; a starting host removes per-upload directories older than 15 minutes. - Fixed OMP requests to OpenAI-compatible providers failing with `Invalid schema for function 'browser_handoff'`. Codex and DeepSeek reject a tool whose root parameters schema is a union, so OMP runs that forward extension schemas unnormalized failed on every request. `browser_handoff` and `browser_credentials` now advertise one strict object, as `browser_open` and `browser_snapshot` already did; Core still enforces the fields each operation or action requires. Pi schemas are unchanged. - Fixed `browser_open` returning a page revision that was already stale. Chrome starts loading a created tab after the tab exists, and that load advanced the revision right after `browser_open` returned, so the first `browser_act` with the returned revision failed with `stale_page_revision`. `browser_open` now returns after the first load completes, up to 10 seconds, and reports the loaded page's revision and URL. - Made failed `browser_act` batches report their progress. The error `details` now carry `failed_action_index` and the `completed_actions` results, and in multi-action batches the message names the failing step and how many steps completed before it. A failure after a completed step now reports `outcome: "unknown"` instead of `not_started`, because the earlier steps already changed the page. diff --git a/docs/security.md b/docs/security.md index efc8428..12e1dc2 100644 --- a/docs/security.md +++ b/docs/security.md @@ -76,7 +76,7 @@ This keeps human-only input out of agent requests. It cannot protect secrets fro ## Upload guardrails -`upload_file` is available only for regular files below a configured `dlp_allowed_roots` path and under the configured size limit. The host canonicalizes the path, rejects symlink races and Unix hard-linked files, verifies the opened file, copies it into a private staging directory, and uses the staged copy for the action. On Unix, staging files are mode `0600`; staged files are removed after the terminal Commit path when cleanup succeeds. +`upload_file` is available only for regular files below a configured `dlp_allowed_roots` path and under the configured size limit. The host canonicalizes the path, rejects symlink races and Unix hard-linked files, verifies the opened file, copies it into a private staging directory, and uses the staged copy for the action. Each copy keeps the source file name inside its own per-upload directory, because pages derive the uploaded file's name and type from it. On Unix, staging directories are mode `0700` and staging files are mode `0600`. Chrome reads a file input's bytes only when the page reads or submits the file, so an upload that reached the page stays on disk for 10 minutes before the host removes it. Uploads that never reach the page are removed immediately, and a starting host removes per-upload directories older than 15 minutes that an exited host left behind. These checks limit accidental path selection. They do not establish that a permitted file is safe to disclose or that the destination is trustworthy. Upload is a recognizable Commit effect and should be reviewed by the human. diff --git a/host-rs/crates/agenttab-host/src/guardrails.rs b/host-rs/crates/agenttab-host/src/guardrails.rs index 59d7068..553cd11 100644 --- a/host-rs/crates/agenttab-host/src/guardrails.rs +++ b/host-rs/crates/agenttab-host/src/guardrails.rs @@ -7,12 +7,19 @@ use serde_json::Value; use std::fs::{self, File, OpenOptions}; use std::io::{self, Read}; use std::path::{Path, PathBuf}; +use std::time::Duration; use thiserror::Error; use url::Url; use uuid::Uuid; const MAX_POLICY_BYTES: u64 = 1024 * 1024; const DEFAULT_DLP_MAX_FILE_BYTES: u64 = 10 * 1024 * 1024; +/// How long an upload stays readable after it reached the page. Chrome reads a +/// file input's bytes lazily, when the page reads or submits the File. +const DELIVERED_UPLOAD_RETENTION: Duration = Duration::from_secs(10 * 60); +/// Staging directories older than this are leftovers from an exited host. It +/// exceeds both the delivered-upload retention and the staged-commit lifetime. +pub(crate) const STALE_UPLOAD_AGE: Duration = Duration::from_secs(15 * 60); #[derive(Debug, Error)] pub enum GuardrailLoadError { @@ -393,12 +400,23 @@ impl Guardrails { pub(crate) fn cleanup_staged_uploads(paths: &[PathBuf]) -> Result<(), RpcError> { let mut first_error = None; + let mut record = |result: io::Result<()>| match result { + Ok(()) => {} + Err(error) if error.kind() == io::ErrorKind::NotFound => {} + Err(error) if first_error.is_none() => first_error = Some(error), + Err(_) => {} + }; for path in paths { - match fs::remove_file(path) { - Ok(()) => {} - Err(error) if error.kind() == io::ErrorKind::NotFound => {} - Err(error) if first_error.is_none() => first_error = Some(error), - Err(_) => {} + record(fs::remove_file(path)); + // Each staged file lives alone in a per-upload UUID directory so the + // page sees the source file name; remove only that directory. + if let Some(directory) = path.parent().filter(|directory| { + directory + .file_name() + .and_then(|name| name.to_str()) + .is_some_and(|name| Uuid::parse_str(name).is_ok()) + }) { + record(fs::remove_dir(directory)); } } match first_error { @@ -413,6 +431,42 @@ impl Guardrails { } } + /// Removes uploads that reached the page once the page has had time to + /// read them. A host that exits first leaves them to `sweep_stale_uploads`. + pub(crate) fn retire_delivered_uploads(paths: Vec) { + if paths.is_empty() { + return; + } + std::thread::spawn(move || { + std::thread::sleep(DELIVERED_UPLOAD_RETENTION); + let _ = Self::cleanup_staged_uploads(&paths); + }); + } + + /// Removes per-upload staging directories last modified before `older_than`. + pub(crate) fn sweep_stale_uploads( + staging_directory: &Path, + older_than: Duration, + ) -> io::Result<()> { + for entry in fs::read_dir(staging_directory)? { + let entry = entry?; + let is_upload_directory = entry.file_type()?.is_dir() + && entry + .file_name() + .to_str() + .is_some_and(|name| Uuid::parse_str(name).is_ok()); + let is_stale = entry + .metadata()? + .modified()? + .elapsed() + .is_ok_and(|age| age >= older_than); + if is_upload_directory && is_stale { + fs::remove_dir_all(entry.path())?; + } + } + Ok(()) + } + fn authorize_file(&self, path: &Path) -> Result<(), RpcError> { self.open_authorized_file(path).map(|_| ()) } @@ -425,7 +479,29 @@ impl Guardrails { format!("Cannot resolve private upload staging directory: {error}"), ) })?; - let staged_path = staging_directory.join(format!("{}.upload", Uuid::new_v4())); + // Keep the source file name: pages derive the File name and MIME type + // from it, and type-checking upload widgets reject an unknown extension. + let file_name = path + .canonicalize() + .ok() + .and_then(|canonical| canonical.file_name().map(std::ffi::OsStr::to_os_string)); + let file_name = file_name.ok_or_else(|| { + RpcError::new("upload_file_unavailable", "Upload file has no file name") + })?; + let staged_directory = staging_directory.join(Uuid::new_v4().to_string()); + let mut builder = fs::DirBuilder::new(); + #[cfg(unix)] + { + use std::os::unix::fs::DirBuilderExt; + builder.mode(0o700); + } + builder.create(&staged_directory).map_err(|error| { + RpcError::new( + "upload_file_unavailable", + format!("Cannot create private upload staging directory: {error}"), + ) + })?; + let staged_path = staged_directory.join(file_name); let mut options = OpenOptions::new(); options.write(true).create_new(true); #[cfg(unix)] @@ -434,6 +510,7 @@ impl Guardrails { options.mode(0o600); } let mut staged = options.open(&staged_path).map_err(|error| { + let _ = fs::remove_dir(&staged_directory); RpcError::new( "upload_file_unavailable", format!("Cannot create private upload staging file: {error}"), @@ -446,14 +523,14 @@ impl Guardrails { &mut staged, ) .map_err(|error| { - let _ = fs::remove_file(&staged_path); + let _ = Self::cleanup_staged_uploads(std::slice::from_ref(&staged_path)); RpcError::new( "upload_file_unavailable", format!("Cannot stage upload file: {error}"), ) })?; if copied > self.policy.dlp_max_file_bytes { - let _ = fs::remove_file(&staged_path); + let _ = Self::cleanup_staged_uploads(std::slice::from_ref(&staged_path)); return Err(RpcError::new( "upload_file_too_large", format!( @@ -463,7 +540,7 @@ impl Guardrails { )); } staged.sync_all().map_err(|error| { - let _ = fs::remove_file(&staged_path); + let _ = Self::cleanup_staged_uploads(std::slice::from_ref(&staged_path)); RpcError::new( "upload_file_unavailable", format!("Cannot finalize staged upload file: {error}"), @@ -796,4 +873,83 @@ mod tests { .unwrap_err(); assert_eq!(error.code, "upload_file_hardlinked"); } + + #[test] + fn upload_staging_keeps_source_file_name_and_cleans_its_directory() { + let temp = tempfile::tempdir().unwrap(); + let allowed_root = temp.path().join("allowed"); + let staging_directory = temp.path().join("staging"); + fs::create_dir(&allowed_root).unwrap(); + fs::create_dir(&staging_directory).unwrap(); + let source = allowed_root.join("avatar.png"); + fs::write(&source, b"png bytes").unwrap(); + let oversized = allowed_root.join("large.png"); + fs::write(&oversized, vec![0_u8; 32]).unwrap(); + let guardrails = Guardrails::from_policy(Policy { + dlp_allowed_roots: vec![allowed_root.clone()], + dlp_max_file_bytes: 16, + ..Policy::default() + }) + .unwrap(); + let upload = |file: &std::path::Path| { + MethodParams::Act(agenttab_protocol::BrowserActParams { + tab_id: 7, + expected_page_revision: 1, + actions: vec![BrowserAction::UploadFile { + r#ref: Some("e1".into()), + selector: None, + frame_id: None, + files: vec![file.display().to_string()], + }], + }) + }; + + let params = upload(&source); + let mut serialized = params.value(); + let staged = guardrails + .stage_uploads(¶ms, &mut serialized, &staging_directory) + .unwrap(); + let staged_path = PathBuf::from(serialized["actions"][0]["files"][0].as_str().unwrap()); + assert_eq!(staged_path.file_name().unwrap(), "avatar.png"); + let staged_directory = staged_path.parent().unwrap(); + assert_eq!( + staged_directory.parent().unwrap(), + staging_directory.canonicalize().unwrap() + ); + assert_eq!(fs::read(&staged_path).unwrap(), b"png bytes"); + Guardrails::cleanup_staged_uploads(&staged).unwrap(); + assert!(!staged_directory.exists()); + assert!(staging_directory.is_dir()); + + let params = upload(&oversized); + let mut serialized = params.value(); + let error = guardrails + .stage_uploads(¶ms, &mut serialized, &staging_directory) + .unwrap_err(); + assert_eq!(error.code, "upload_file_too_large"); + assert_eq!(fs::read_dir(&staging_directory).unwrap().count(), 0); + } + + #[test] + fn stale_upload_sweep_removes_only_old_upload_directories() { + let temp = tempfile::tempdir().unwrap(); + let staging = temp.path(); + let old_upload = staging.join(Uuid::new_v4().to_string()); + let fresh_upload = staging.join(Uuid::new_v4().to_string()); + let unrelated = staging.join("not-an-upload"); + for directory in [&old_upload, &fresh_upload, &unrelated] { + fs::create_dir(directory).unwrap(); + fs::write(directory.join("file.png"), b"bytes").unwrap(); + } + let past = std::time::SystemTime::now() - Duration::from_secs(3600); + for directory in [&old_upload, &unrelated] { + File::open(directory).unwrap().set_modified(past).unwrap(); + } + + Guardrails::sweep_stale_uploads(staging, Duration::from_secs(900)).unwrap(); + + assert!(!old_upload.exists()); + assert!(fresh_upload.join("file.png").exists()); + assert!(unrelated.join("file.png").exists()); + } } diff --git a/host-rs/crates/agenttab-host/src/runtime.rs b/host-rs/crates/agenttab-host/src/runtime.rs index 67490aa..25d0baa 100644 --- a/host-rs/crates/agenttab-host/src/runtime.rs +++ b/host-rs/crates/agenttab-host/src/runtime.rs @@ -2,7 +2,7 @@ use crate::audit::{canonicalize, now_ms, AuditEntry, AuditLog}; use crate::credentials::{ BrokerError, CredentialBroker, NeedsUserReason, PrepareResult, SelectResult, }; -use crate::guardrails::{GuardrailLoadError, Guardrails}; +use crate::guardrails::{GuardrailLoadError, Guardrails, STALE_UPLOAD_AGE}; use crate::journal::{ BeginDecision, InventoryReconciliation, Journal, JournalError, StagedCommitApproval, StagedCommitConsumption, StagedReplayResolution, @@ -178,6 +178,9 @@ impl Runtime { native: Arc, ) -> Result, RuntimeBuildError> { paths.prepare()?; + // Delivered uploads outlive their deferred cleanup thread when the host + // exits; remove any left behind by a previous host process. + let _ = Guardrails::sweep_stale_uploads(&paths.upload_staging_dir, STALE_UPLOAD_AGE); let journal = Arc::new(Journal::open(&paths.state_db)?); let tab_urls = Arc::new(RwLock::new(HashMap::new())); let sink = Arc::new(JournalNativeEventSink { @@ -1204,7 +1207,11 @@ impl Runtime { origin_policy, timeout, ); - if let Err(error) = Guardrails::cleanup_staged_uploads(&committed_uploads) { + // Chrome hands the page a path-backed File and reads it only when the + // page does, so uploads that reached the page must stay on disk. + if native_result.is_ok() { + Guardrails::retire_delivered_uploads(committed_uploads); + } else if let Err(error) = Guardrails::cleanup_staged_uploads(&committed_uploads) { let _ = Guardrails::cleanup_staged_uploads(&staged_uploads); return RpcResponse::failure(request_id, Outcome::Unknown, error); } @@ -1322,9 +1329,7 @@ impl Runtime { } }; } - if let Err(error) = Guardrails::cleanup_staged_uploads(&staged_uploads) { - return RpcResponse::failure(request_id, Outcome::Unknown, error); - } + Guardrails::retire_delivered_uploads(staged_uploads); if native.staged.is_some() { return RpcResponse::failure( request_id, @@ -3506,7 +3511,7 @@ mod tests { } #[test] - fn staged_upload_survives_review_and_is_removed_after_commit() { + fn staged_upload_survives_review_and_stays_readable_after_commit() { let native = FakeNative::staging(); let (_temp, runtime, connection, upload_root) = connected_runtime_with_upload_root(native.clone()); @@ -3566,7 +3571,11 @@ mod tests { ); assert_eq!(committed["outcome"], "completed"); - assert!(!std::path::Path::new(&staged_path).exists()); + // The page reads a file input lazily, so the committed bytes must remain. + assert_eq!( + std::fs::read(&staged_path).unwrap(), + b"approved upload bytes" + ); } #[test] @@ -3643,7 +3652,7 @@ mod tests { }), ); assert_eq!(committed["outcome"], "completed"); - assert!(!std::path::Path::new(&success_path).exists()); + assert!(std::path::Path::new(&success_path).exists()); assert!(runtime .journal .abandon_popup_staged_commit(task_id, 3, &success_handle) From b04d54470a7a30ba06d9a1108e13ea18fdbc3729 Mon Sep 17 00:00:00 2001 From: Wolfgang Schoenberger <221313372+wolfiesch@users.noreply.github.com> Date: Thu, 8 Oct 2026 00:42:49 -0700 Subject: [PATCH 2/2] Set test directory times portably on Windows Windows opens a directory handle only with backup semantics and needs the write-attributes right to change its timestamps. --- .../crates/agenttab-host/src/guardrails.rs | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/host-rs/crates/agenttab-host/src/guardrails.rs b/host-rs/crates/agenttab-host/src/guardrails.rs index 553cd11..f571a50 100644 --- a/host-rs/crates/agenttab-host/src/guardrails.rs +++ b/host-rs/crates/agenttab-host/src/guardrails.rs @@ -943,7 +943,7 @@ mod tests { } let past = std::time::SystemTime::now() - Duration::from_secs(3600); for directory in [&old_upload, &unrelated] { - File::open(directory).unwrap().set_modified(past).unwrap(); + set_directory_modified(directory, past); } Guardrails::sweep_stale_uploads(staging, Duration::from_secs(900)).unwrap(); @@ -952,4 +952,23 @@ mod tests { assert!(fresh_upload.join("file.png").exists()); assert!(unrelated.join("file.png").exists()); } + + fn set_directory_modified(directory: &Path, time: std::time::SystemTime) { + let mut options = fs::OpenOptions::new(); + #[cfg(unix)] + options.read(true); + // Windows opens a directory handle only with backup semantics, and + // changing its timestamps needs the write-attributes right. + #[cfg(windows)] + { + use std::os::windows::fs::OpenOptionsExt; + use windows_sys::Win32::Storage::FileSystem::{ + FILE_FLAG_BACKUP_SEMANTICS, FILE_WRITE_ATTRIBUTES, + }; + options + .access_mode(FILE_WRITE_ATTRIBUTES) + .custom_flags(FILE_FLAG_BACKUP_SEMANTICS); + } + options.open(directory).unwrap().set_modified(time).unwrap(); + } }