From 28708acfce578017552ed124cd5a186302b0607b Mon Sep 17 00:00:00 2001 From: harehare Date: Mon, 13 Jul 2026 21:10:31 +0900 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20feat(mq-lang,mq-run):=20add=20--all?= =?UTF-8?q?ow-read=20runtime=20flag=20for=20filesystem=20reads?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit read_file/read_file_bytes were only gated by the compile-time file-io Cargo feature, unlike http()/write_file which also require an explicit runtime opt-in (--allow-net/--allow-write). This let a third-party module fetched via HTTP import silently read local files. Add a process-wide READ_ALLOWED flag (capability::set_allow_read/is_read_allowed) and the --allow-read CLI flag to close that gap, matching the existing pattern. --- crates/mq-lang/src/engine.rs | 8 +++ crates/mq-lang/src/eval/builtin.rs | 69 +++++++++++++++---- crates/mq-lang/src/eval/builtin/capability.rs | 28 ++++++-- crates/mq-run/src/cli.rs | 54 +++++++++++++++ crates/mq-run/tests/integration_tests.rs | 24 +++++++ 5 files changed, 164 insertions(+), 19 deletions(-) diff --git a/crates/mq-lang/src/engine.rs b/crates/mq-lang/src/engine.rs index 10e85236c..ffe354092 100644 --- a/crates/mq-lang/src/engine.rs +++ b/crates/mq-lang/src/engine.rs @@ -127,6 +127,14 @@ impl Engine { capability::set_allow_net(allow); } + /// Enables or disables the `read_file`/`read_file_bytes` builtins for the current process. + /// + /// Disabled by default. This is a process-wide setting (see + /// [`capability`](crate::eval::builtin::capability)), not per-`Engine`. + pub fn set_allow_read(&self, allow: bool) { + capability::set_allow_read(allow); + } + /// Enables or disables the `write_file` builtin for the current process. /// /// Disabled by default. This is a process-wide setting (see diff --git a/crates/mq-lang/src/eval/builtin.rs b/crates/mq-lang/src/eval/builtin.rs index 4363dfff0..2d444da16 100644 --- a/crates/mq-lang/src/eval/builtin.rs +++ b/crates/mq-lang/src/eval/builtin.rs @@ -3868,9 +3868,17 @@ fn path_join_impl(ident: &Ident, _: &RuntimeValue, mut args: Args, _: &SharedEnv } } +/// Reads the contents of `path` as a string. Requires the `--allow-read` CLI flag (see +/// [`capability`]). #[cfg(feature = "file-io")] #[mq_macros::mq_fn(name = "read_file", params = Fixed(1))] fn read_file_impl(ident: &Ident, _: &RuntimeValue, mut args: Args, _: &SharedEnv) -> Result { + if !capability::is_read_allowed() { + return Err(Error::Runtime( + "read_file: filesystem reads are disabled; re-run mq with --allow-read to enable read_file".into(), + )); + } + match args.as_mut_slice() { [RuntimeValue::String(path)] => match std::fs::read_to_string(&path) { Ok(content) => Ok(RuntimeValue::String(content)), @@ -3891,9 +3899,18 @@ fn file_exists_impl(ident: &Ident, _: &RuntimeValue, mut args: Args, _: &SharedE } } +/// Reads the contents of `path` as raw bytes. Requires the `--allow-read` CLI flag (see +/// [`capability`]). #[cfg(feature = "file-io")] #[mq_macros::mq_fn(name = "read_file_bytes", params = Fixed(1))] fn read_file_bytes_impl(ident: &Ident, _: &RuntimeValue, mut args: Args, _: &SharedEnv) -> Result { + if !capability::is_read_allowed() { + return Err(Error::Runtime( + "read_file_bytes: filesystem reads are disabled; re-run mq with --allow-read to enable read_file_bytes" + .into(), + )); + } + match args.as_mut_slice() { [RuntimeValue::String(path)] => match std::fs::read(&path) { Ok(content) => Ok(RuntimeValue::Bytes(content)), @@ -5820,7 +5837,7 @@ pub static BUILTIN_FUNCTION_DOC: LazyLock map.insert( SmolStr::new("read_file"), BuiltinFunctionDoc { - description: "Reads the contents of a file at the given path and returns it as a string.", + description: "Reads the contents of a file at the given path and returns it as a string. Requires the --allow-read CLI flag; otherwise returns a runtime error.", params: &["path"], }, ); @@ -5836,7 +5853,7 @@ pub static BUILTIN_FUNCTION_DOC: LazyLock map.insert( SmolStr::new("read_file_bytes"), BuiltinFunctionDoc { - description: "Reads the contents of a file at the given path and returns it as raw bytes.", + description: "Reads the contents of a file at the given path and returns it as raw bytes. Requires the --allow-read CLI flag; otherwise returns a runtime error.", params: &["path"], }, ); @@ -9540,27 +9557,55 @@ mod tests { assert!(result.is_err()); } + // READ_ALLOWED is a single process-wide flag, so every case that toggles it must run in + // one #[test] function — cargo test runs tests in parallel by default, and two tests + // flipping the same global independently would race and flake. #[cfg(feature = "file-io")] #[test] - fn test_read_file_bytes() { + fn test_read_file_capability_gate_and_success() { use std::io::Write; - let mut tmp = tempfile::NamedTempFile::new().expect("failed to create temp file"); - tmp.write_all(&[0x89, 0x50, 0x4e, 0x47]).expect("failed to write"); - let path = tmp.path().to_string_lossy().to_string(); + + let mut text_tmp = tempfile::NamedTempFile::new().expect("failed to create temp file"); + text_tmp.write_all(b"hello").expect("failed to write"); + let text_path = text_tmp.path().to_string_lossy().to_string(); + + let mut bytes_tmp = tempfile::NamedTempFile::new().expect("failed to create temp file"); + bytes_tmp.write_all(&[0x89, 0x50, 0x4e, 0x47]).expect("failed to write"); + let bytes_path = bytes_tmp.path().to_string_lossy().to_string(); + + capability::set_allow_read(false); + assert!( + call("read_file", vec![RuntimeValue::String(text_path.clone())]).is_err(), + "read_file should be blocked when --allow-read is not set" + ); + assert!( + call("read_file_bytes", vec![RuntimeValue::String(bytes_path.clone())]).is_err(), + "read_file_bytes should be blocked when --allow-read is not set" + ); + + capability::set_allow_read(true); + assert_eq!( + call("read_file", vec![RuntimeValue::String(text_path.clone())]), + Ok(RuntimeValue::String("hello".to_string())) + ); assert_eq!( - call("read_file_bytes", vec![RuntimeValue::String(path)]), + call("read_file_bytes", vec![RuntimeValue::String(bytes_path.clone())]), Ok(RuntimeValue::Bytes(vec![0x89, 0x50, 0x4e, 0x47])) ); - } - #[cfg(feature = "file-io")] - #[test] - fn test_read_file_bytes_with_nonexistent_file() { let result = call( "read_file_bytes", vec![RuntimeValue::String("/nonexistent/path/no_such_file.png".into())], ); - assert!(result.is_err()); + assert!(result.is_err(), "read_file_bytes should error for a nonexistent file"); + + let result = call( + "read_file", + vec![RuntimeValue::String("/nonexistent/path/no_such_file.md".into())], + ); + assert!(result.is_err(), "read_file should error for a nonexistent file"); + + capability::set_allow_read(false); } // WRITE_ALLOWED is a single process-wide flag, so every case that toggles it must run in diff --git a/crates/mq-lang/src/eval/builtin/capability.rs b/crates/mq-lang/src/eval/builtin/capability.rs index a03d190af..2d4bf8d1a 100644 --- a/crates/mq-lang/src/eval/builtin/capability.rs +++ b/crates/mq-lang/src/eval/builtin/capability.rs @@ -1,18 +1,22 @@ //! Process-wide opt-in flags gating capabilities with real-world side effects: -//! `http` (network) and `write_file` (filesystem writes). +//! `http` (network), `read_file`/`read_file_bytes` (filesystem reads), and `write_file` +//! (filesystem writes). //! -//! Both default to `false`. A host must explicitly enable them via -//! [`set_allow_net`]/[`set_allow_write`] (wired to the `--allow-net`/`--allow-write` CLI flags -//! in `mq-run`) before the corresponding builtins will run; otherwise they return a runtime -//! error explaining how to opt in. +//! All default to `false`. A host must explicitly enable them via +//! [`set_allow_net`]/[`set_allow_read`]/[`set_allow_write`] (wired to the +//! `--allow-net`/`--allow-read`/`--allow-write` CLI flags in `mq-run`) before the corresponding +//! builtins will run; otherwise they return a runtime error explaining how to opt in. This +//! keeps the model symmetric: a third-party module fetched via HTTP import can't silently read +//! or write local files, or reach the network, without the host opting in to each capability. //! //! This is process-wide rather than per-[`Engine`](crate::Engine): like the `file-io` Cargo -//! feature that gates `read_file` at compile time, network/write access is a deployment-level -//! decision, not something a single process needs to vary per query. +//! feature that gates these functions at compile time, filesystem/network access is a +//! deployment-level decision, not something a single process needs to vary per query. use std::sync::atomic::{AtomicBool, Ordering}; static NET_ALLOWED: AtomicBool = AtomicBool::new(false); +static READ_ALLOWED: AtomicBool = AtomicBool::new(false); static WRITE_ALLOWED: AtomicBool = AtomicBool::new(false); /// Enables or disables `http` for the current process. @@ -20,6 +24,11 @@ pub fn set_allow_net(allow: bool) { NET_ALLOWED.store(allow, Ordering::Relaxed); } +/// Enables or disables `read_file`/`read_file_bytes` for the current process. +pub fn set_allow_read(allow: bool) { + READ_ALLOWED.store(allow, Ordering::Relaxed); +} + /// Enables or disables `write_file` for the current process. pub fn set_allow_write(allow: bool) { WRITE_ALLOWED.store(allow, Ordering::Relaxed); @@ -30,6 +39,11 @@ pub(crate) fn is_net_allowed() -> bool { NET_ALLOWED.load(Ordering::Relaxed) } +#[cfg(feature = "file-io")] +pub(crate) fn is_read_allowed() -> bool { + READ_ALLOWED.load(Ordering::Relaxed) +} + #[cfg(feature = "file-io")] pub(crate) fn is_write_allowed() -> bool { WRITE_ALLOWED.load(Ordering::Relaxed) diff --git a/crates/mq-run/src/cli.rs b/crates/mq-run/src/cli.rs index 048372a2d..4674c6b7c 100644 --- a/crates/mq-run/src/cli.rs +++ b/crates/mq-run/src/cli.rs @@ -350,6 +350,11 @@ struct InputArgs { #[arg(long = "allow-net", default_value_t = false)] allow_net: bool, + /// Allow the `read_file`/`read_file_bytes` functions to read from the filesystem. Disabled + /// by default. + #[arg(long = "allow-read", default_value_t = false)] + allow_read: bool, + /// Allow the `write_file` function to write to the filesystem. Disabled by default. #[arg(long = "allow-write", default_value_t = false)] allow_write: bool, @@ -861,6 +866,7 @@ impl Cli { engine.set_allow_net(self.input.allow_net); } + engine.set_allow_read(self.input.allow_read); engine.set_allow_write(self.input.allow_write); #[cfg(feature = "debugger")] @@ -1546,6 +1552,54 @@ mod tests { assert!(cli.run().is_ok()); } + // READ_ALLOWED is a single process-wide flag (see mq_lang::eval::builtin::capability), so + // every case that toggles it must run in one #[test] function — cargo test runs tests in + // parallel by default, and two tests flipping the same global independently would race and + // flake. No other test in this file exercises read_file/read_file_bytes. + #[test] + fn test_allow_read_flag_gates_read_file() { + let (_, temp_file_path) = create_file("test_allow_read.md", "hello"); + let temp_file_path_clone = temp_file_path.clone(); + + defer! { + if temp_file_path_clone.exists() { + std::fs::remove_file(&temp_file_path_clone).expect("Failed to delete temp file"); + } + } + + let query = format!("read_file(\"{}\")", temp_file_path.to_string_lossy()); + + let blocked_cli = Cli { + input: InputArgs { + input_format: Some(InputFormat::Null), + ..Default::default() + }, + output: OutputArgs::default(), + commands: None, + query: Some(query.clone()), + files: None, + ..Cli::default() + }; + assert!( + blocked_cli.run().is_err(), + "read_file should be blocked without --allow-read" + ); + + let allowed_cli = Cli { + input: InputArgs { + input_format: Some(InputFormat::Null), + allow_read: true, + ..Default::default() + }, + output: OutputArgs::default(), + commands: None, + query: Some(query), + files: None, + ..Cli::default() + }; + assert!(allowed_cli.run().is_ok(), "read_file should succeed with --allow-read"); + } + #[test] fn test_cli_raw_input() { let (_, temp_file_path) = create_file("test1.md", "# test"); diff --git a/crates/mq-run/tests/integration_tests.rs b/crates/mq-run/tests/integration_tests.rs index 43fcec68b..e10d17f1f 100644 --- a/crates/mq-run/tests/integration_tests.rs +++ b/crates/mq-run/tests/integration_tests.rs @@ -621,6 +621,7 @@ fn test_read_file() -> Result<(), Box> { #[cfg(unix)] let assert = cmd .arg("--unbuffered") + .arg("--allow-read") .arg(format!(r#"read_file("{}")"#, temp_file_path.to_string_lossy())) .arg(temp_file_path.to_string_lossy().to_string()) .assert(); @@ -628,6 +629,7 @@ fn test_read_file() -> Result<(), Box> { #[cfg(windows)] let assert = cmd .arg("--unbuffered") + .arg("--allow-read") .arg(format!( r#"read_file("{}")"#, temp_file_path.to_string_lossy().replace("\\", "/") @@ -639,6 +641,28 @@ fn test_read_file() -> Result<(), Box> { Ok(()) } +#[test] +fn test_read_file_without_allow_read_is_blocked() -> Result<(), Box> { + let (_, temp_file_path) = create_file("test_read_file_blocked.md", "test"); + let temp_file_path_clone = temp_file_path.clone(); + + defer! { + if temp_file_path_clone.exists() { + std::fs::remove_file(&temp_file_path_clone).expect("Failed to delete temp file"); + } + } + + let mut cmd = cargo::cargo_bin_cmd!("mq"); + let assert = cmd + .arg("--unbuffered") + .arg(format!(r#"read_file("{}")"#, temp_file_path.to_string_lossy())) + .arg(temp_file_path.to_string_lossy().to_string()) + .assert(); + + assert.failure(); + Ok(()) +} + #[test] fn test_def_argument_scope_with_let_and_do() -> Result<(), Box> { let mut cmd = cargo::cargo_bin_cmd!("mq");