mirror of
https://github.com/Tencent/BrowserSkill.git
synced 2026-09-10 13:01:23 +08:00
feat(bsk): fix upload function that cannot bypass layout
This commit is contained in:
@@ -55,7 +55,7 @@ pub fn dispatch(args: DownloadArgs, format: Format) -> Result<(), CliError> {
|
||||
selector,
|
||||
tab_id: args.tab_id,
|
||||
timeout_ms: Some(args.timeout),
|
||||
staging_path: None,
|
||||
browser_relative_dir: None,
|
||||
};
|
||||
let reply: DownloadResult = crate::cli::business_rpc::call(
|
||||
info.sock_path.clone(),
|
||||
@@ -82,7 +82,7 @@ pub fn dispatch(args: DownloadArgs, format: Format) -> Result<(), CliError> {
|
||||
value["path"] = serde_json::json!(args.out.to_string_lossy());
|
||||
if let Some(obj) = value.as_object_mut() {
|
||||
obj.remove("transfer_id");
|
||||
obj.remove("staged_path");
|
||||
obj.remove("browser_path");
|
||||
}
|
||||
println!("{}", serde_json::to_string_pretty(&value).unwrap());
|
||||
}
|
||||
|
||||
@@ -49,9 +49,10 @@ pub mod reason {
|
||||
pub const TAB_NOT_ACTIVE: &str = "tab_not_active";
|
||||
pub const BORROW_CONFLICT: &str = "borrow_conflict";
|
||||
pub const SCREENSHOT_CAPTURE_FAILED: &str = "screenshot_capture_failed";
|
||||
pub const FILE_CHOOSER_CONTROL_FAILED: &str = "file_chooser_control_failed";
|
||||
pub const FILE_CHOOSER_NOT_OPENED: &str = "file_chooser_not_opened";
|
||||
pub const UNSUPPORTED_FILE_CHOOSER: &str = "unsupported_file_chooser";
|
||||
pub const FILE_INPUT_PROBE_FAILED: &str = "file_input_probe_failed";
|
||||
pub const FILE_INPUT_NOT_ACTIVATED: &str = "file_input_not_activated";
|
||||
pub const SET_FILE_INPUT_FAILED: &str = "set_file_input_failed";
|
||||
pub const DOWNLOAD_CAPTURE_FAILED: &str = "download_capture_failed";
|
||||
pub const SESSION_BUSY: &str = crate::rpc_reason::SESSION_BUSY;
|
||||
pub const RECORD_START_PAGE_UNREACHABLE: &str = "record_start_page_unreachable";
|
||||
}
|
||||
@@ -283,26 +284,33 @@ pub fn info_for_error(code: ErrorCode, data: Option<&serde_json::Value>) -> Rend
|
||||
),
|
||||
exit_code: base.exit_code,
|
||||
},
|
||||
(ErrorCode::Unsupported, reason::UNSUPPORTED_FILE_CHOOSER) => RenderInfo {
|
||||
summary: "the page opened a non-input file picker",
|
||||
(ErrorCode::Unsupported, reason::FILE_INPUT_NOT_ACTIVATED) => RenderInfo {
|
||||
summary: "the upload trigger did not activate a file input",
|
||||
hint: Some(
|
||||
"do not retry upload; use `bsk request-help` and tell the user the exact original local path to select, or stop if human help is disabled",
|
||||
"the page may use a non-input picker such as showOpenFilePicker(); do not retry blindly — use `bsk request-help` with the exact original local path, or stop if human help is disabled",
|
||||
),
|
||||
exit_code: base.exit_code,
|
||||
},
|
||||
(ErrorCode::Timeout | ErrorCode::CdpFailed, reason::FILE_CHOOSER_CONTROL_FAILED) => {
|
||||
(ErrorCode::Timeout | ErrorCode::CdpFailed, reason::FILE_INPUT_PROBE_FAILED) => {
|
||||
RenderInfo {
|
||||
summary: "the browser file-chooser transaction could not complete",
|
||||
summary: "the browser upload transaction could not be established",
|
||||
hint: Some(
|
||||
"do not retry the same upload blindly; use `bsk request-help` when human help is available",
|
||||
),
|
||||
exit_code: base.exit_code,
|
||||
}
|
||||
}
|
||||
(ErrorCode::Timeout, reason::FILE_CHOOSER_NOT_OPENED) => RenderInfo {
|
||||
summary: "the upload action did not open a file chooser",
|
||||
(ErrorCode::Timeout | ErrorCode::CdpFailed, reason::SET_FILE_INPUT_FAILED) => RenderInfo {
|
||||
summary: "the browser could not attach the staged file to the input",
|
||||
hint: Some(
|
||||
"observe once after expanding any upload menu, then retry with the actual upload action or use `bsk request-help`",
|
||||
"check that BrowserSkill has Chrome's 'Allow access to file URLs' permission; otherwise use `bsk request-help`",
|
||||
),
|
||||
exit_code: base.exit_code,
|
||||
},
|
||||
(ErrorCode::CdpFailed, reason::DOWNLOAD_CAPTURE_FAILED) => RenderInfo {
|
||||
summary: "the browser download could not be attributed or completed",
|
||||
hint: Some(
|
||||
"do not retry blindly; the target browser may not expose a reliable download intent signal",
|
||||
),
|
||||
exit_code: base.exit_code,
|
||||
},
|
||||
@@ -446,24 +454,26 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn file_chooser_reasons_render_actionable_fallbacks() {
|
||||
let unsupported = serde_json::json!({ "reason": reason::UNSUPPORTED_FILE_CHOOSER });
|
||||
fn file_transfer_reasons_render_actionable_fallbacks() {
|
||||
let unsupported = serde_json::json!({ "reason": reason::FILE_INPUT_NOT_ACTIVATED });
|
||||
let info = info_for_error(ErrorCode::Unsupported, Some(&unsupported));
|
||||
assert_eq!(info.summary, "the page opened a non-input file picker");
|
||||
assert!(info.hint.unwrap().contains("exact original local path"));
|
||||
|
||||
let control = serde_json::json!({ "reason": reason::FILE_CHOOSER_CONTROL_FAILED });
|
||||
let info = info_for_error(ErrorCode::Timeout, Some(&control));
|
||||
assert!(info.summary.contains("transaction could not complete"));
|
||||
assert!(info.hint.unwrap().contains("do not retry"));
|
||||
|
||||
let not_opened = serde_json::json!({ "reason": reason::FILE_CHOOSER_NOT_OPENED });
|
||||
let info = info_for_error(ErrorCode::Timeout, Some(¬_opened));
|
||||
assert_eq!(
|
||||
info.summary,
|
||||
"the upload action did not open a file chooser"
|
||||
"the upload trigger did not activate a file input"
|
||||
);
|
||||
assert!(info.hint.unwrap().contains("expanding any upload menu"));
|
||||
assert!(info.hint.unwrap().contains("exact original local path"));
|
||||
|
||||
let control = serde_json::json!({ "reason": reason::FILE_INPUT_PROBE_FAILED });
|
||||
let info = info_for_error(ErrorCode::Timeout, Some(&control));
|
||||
assert!(
|
||||
info.summary
|
||||
.contains("transaction could not be established")
|
||||
);
|
||||
assert!(info.hint.unwrap().contains("do not retry"));
|
||||
|
||||
let download = serde_json::json!({ "reason": reason::DOWNLOAD_CAPTURE_FAILED });
|
||||
let info = info_for_error(ErrorCode::CdpFailed, Some(&download));
|
||||
assert!(info.summary.contains("download could not be attributed"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -43,7 +43,7 @@ struct Entry {
|
||||
#[derive(Debug)]
|
||||
pub struct DownloadStaging {
|
||||
pub transfer_id: String,
|
||||
pub directory: PathBuf,
|
||||
pub browser_relative_dir: String,
|
||||
}
|
||||
|
||||
#[derive(Debug)]
|
||||
@@ -247,32 +247,70 @@ impl TransferRegistry {
|
||||
},
|
||||
);
|
||||
Ok(DownloadStaging {
|
||||
browser_relative_dir: format!("BrowserSkill/{id}"),
|
||||
transfer_id: id,
|
||||
directory: dir,
|
||||
})
|
||||
}
|
||||
|
||||
pub fn finish_download(&self, id: &str, reported_path: &Path) -> Result<u64, RpcError> {
|
||||
let canonical = reported_path.canonicalize().map_err(io_error)?;
|
||||
pub fn import_download(&self, id: &str, reported_path: &Path) -> Result<u64, RpcError> {
|
||||
let mut entries = self.entries.lock().unwrap();
|
||||
let entry = entries
|
||||
.get_mut(id)
|
||||
.ok_or_else(|| not_found("download transfer not found"))?;
|
||||
let dir = entry.path.canonicalize().map_err(io_error)?;
|
||||
if entry.direction != Direction::Download || !canonical.starts_with(&dir) {
|
||||
return Err(permission("download escaped its staging directory"));
|
||||
if entry.direction != Direction::Download || !reported_path.is_absolute() {
|
||||
return Err(permission(
|
||||
"download path is outside its browser capability",
|
||||
));
|
||||
}
|
||||
let expected_parent = Path::new("BrowserSkill").join(id);
|
||||
let reported_parent = reported_path
|
||||
.parent()
|
||||
.ok_or_else(|| permission("download path has no parent directory"))?;
|
||||
if !reported_parent.ends_with(&expected_parent) {
|
||||
return Err(permission(
|
||||
"download escaped its browser capability directory",
|
||||
));
|
||||
}
|
||||
for path in [reported_path, reported_parent] {
|
||||
if fs::symlink_metadata(path)
|
||||
.map_err(io_error)?
|
||||
.file_type()
|
||||
.is_symlink()
|
||||
{
|
||||
return Err(permission("download capability path contains a symlink"));
|
||||
}
|
||||
}
|
||||
let canonical = reported_path.canonicalize().map_err(io_error)?;
|
||||
let meta = fs::metadata(&canonical).map_err(io_error)?;
|
||||
if !meta.is_file() || meta.len() > MAX_TRANSFER_BYTES {
|
||||
return Err(invalid(
|
||||
"download is not a regular file or exceeds the transfer limit",
|
||||
));
|
||||
}
|
||||
entry.path = canonical;
|
||||
entry.written = meta.len();
|
||||
entry.expected_size = Some(meta.len());
|
||||
|
||||
let destination = entry.path.join("payload");
|
||||
let source = File::open(&canonical).map_err(io_error)?;
|
||||
let mut destination_file = OpenOptions::new()
|
||||
.write(true)
|
||||
.create_new(true)
|
||||
.open(&destination)
|
||||
.map_err(io_error)?;
|
||||
let copied = std::io::copy(
|
||||
&mut source.take(MAX_TRANSFER_BYTES.saturating_add(1)),
|
||||
&mut destination_file,
|
||||
)
|
||||
.map_err(io_error)?;
|
||||
if copied > MAX_TRANSFER_BYTES {
|
||||
let _ = fs::remove_file(&destination);
|
||||
return Err(invalid("download exceeds the transfer limit"));
|
||||
}
|
||||
entry.path = destination;
|
||||
entry.written = copied;
|
||||
entry.expected_size = Some(copied);
|
||||
entry.ready = true;
|
||||
Ok(meta.len())
|
||||
let _ = fs::remove_file(&canonical);
|
||||
let _ = fs::remove_dir(reported_parent);
|
||||
Ok(copied)
|
||||
}
|
||||
|
||||
pub fn read_chunk(&self, p: TransferChunkParams) -> Result<TransferChunkResult, RpcError> {
|
||||
@@ -430,25 +468,45 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn download_must_finish_inside_its_minted_directory() {
|
||||
fn download_must_import_from_its_browser_capability_directory() {
|
||||
let (temp, registry) = registry();
|
||||
let staging = registry.begin_download("s1").unwrap();
|
||||
let outside = temp.path().join("outside.bin");
|
||||
fs::write(&outside, b"secret").unwrap();
|
||||
assert!(
|
||||
registry
|
||||
.finish_download(&staging.transfer_id, &outside)
|
||||
.import_download(&staging.transfer_id, &outside)
|
||||
.is_err()
|
||||
);
|
||||
|
||||
let inside = staging.directory.join("result.bin");
|
||||
let browser_dir = temp
|
||||
.path()
|
||||
.join("Downloads")
|
||||
.join("BrowserSkill")
|
||||
.join(&staging.transfer_id);
|
||||
fs::create_dir_all(&browser_dir).unwrap();
|
||||
let inside = browser_dir.join("result.bin");
|
||||
fs::write(&inside, b"result").unwrap();
|
||||
assert_eq!(
|
||||
registry
|
||||
.finish_download(&staging.transfer_id, &inside)
|
||||
.import_download(&staging.transfer_id, &inside)
|
||||
.unwrap(),
|
||||
6
|
||||
);
|
||||
assert!(!inside.exists());
|
||||
let chunk = registry
|
||||
.read_chunk(TransferChunkParams {
|
||||
transfer_id: staging.transfer_id,
|
||||
offset: 0,
|
||||
data_base64: String::new(),
|
||||
})
|
||||
.unwrap();
|
||||
assert_eq!(
|
||||
base64::engine::general_purpose::STANDARD
|
||||
.decode(chunk.data_base64.unwrap())
|
||||
.unwrap(),
|
||||
b"result"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -456,9 +514,11 @@ mod tests {
|
||||
let (_temp, registry) = registry();
|
||||
let first = registry.begin_download("s1").unwrap();
|
||||
let second = registry.begin_download("s2").unwrap();
|
||||
let first_directory = registry.root.join(&first.transfer_id);
|
||||
let second_directory = registry.root.join(&second.transfer_id);
|
||||
registry.release_session("s1");
|
||||
assert!(!first.directory.exists());
|
||||
assert!(second.directory.exists());
|
||||
assert!(!first_directory.exists());
|
||||
assert!(second_directory.exists());
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -381,7 +381,7 @@ async fn handle_tool_dispatch(
|
||||
Ok(v) => v,
|
||||
Err(err) => return ResponseBody::Err(err),
|
||||
};
|
||||
download.staging_path = Some(staging.directory.to_string_lossy().into_owned());
|
||||
download.browser_relative_dir = Some(staging.browser_relative_dir);
|
||||
download_transfer_id = Some(staging.transfer_id);
|
||||
params = serde_json::to_value(download).unwrap_or(Value::Null);
|
||||
}
|
||||
@@ -416,19 +416,19 @@ async fn handle_tool_dispatch(
|
||||
});
|
||||
}
|
||||
};
|
||||
let Some(path) = result.staged_path.take() else {
|
||||
let Some(path) = result.browser_path.take() else {
|
||||
state
|
||||
.transfers
|
||||
.release(TransferIdParams { transfer_id: id });
|
||||
return ResponseBody::Err(RpcError {
|
||||
code: ErrorCode::ProtocolError,
|
||||
message: "tool.download returned no staged_path".into(),
|
||||
message: "tool.download returned no browser_path".into(),
|
||||
data: None,
|
||||
});
|
||||
};
|
||||
match state
|
||||
.transfers
|
||||
.finish_download(&id, std::path::Path::new(&path))
|
||||
.import_download(&id, std::path::Path::new(&path))
|
||||
{
|
||||
Ok(size) => {
|
||||
result.byte_size = size;
|
||||
|
||||
Reference in New Issue
Block a user