From fc44df1e458a3ce1a0f0ff3fb3c131801e65c7ec Mon Sep 17 00:00:00 2001 From: Jonathan Turner Date: Sun, 3 Jan 2021 19:44:21 +1300 Subject: [PATCH] Don't leak set/set-env/source scopes via actions (#2849) --- crates/nu-cli/src/commands/benchmark.rs | 7 +- .../nu-cli/src/commands/classified/block.rs | 15 --- .../src/commands/classified/internal.rs | 33 ------ crates/nu-cli/src/commands/do_.rs | 3 +- crates/nu-cli/src/commands/set.rs | 14 +-- crates/nu-cli/src/commands/set_env.rs | 14 +-- crates/nu-cli/src/commands/source.rs | 29 ++++- crates/nu-cli/src/evaluate/scope.rs | 4 + crates/nu-cli/tests/commands/every.rs | 112 ++++++------------ crates/nu-protocol/src/return_value.rs | 11 +- tests/shell/pipeline/commands/internal.rs | 24 ++++ 11 files changed, 114 insertions(+), 152 deletions(-) diff --git a/crates/nu-cli/src/commands/benchmark.rs b/crates/nu-cli/src/commands/benchmark.rs index 0595b753b1..8348278057 100644 --- a/crates/nu-cli/src/commands/benchmark.rs +++ b/crates/nu-cli/src/commands/benchmark.rs @@ -84,7 +84,9 @@ async fn benchmark(raw_args: CommandArgs) -> Result { #[cfg(feature = "rich-benchmark")] let start = time().await; + context.scope.enter_scope(); let result = run_block(&block.block, &context, input).await; + context.scope.exit_scope(); let output = result?.into_vec().await; #[cfg(feature = "rich-benchmark")] @@ -152,7 +154,10 @@ where // add autoview for an empty block let time_block = add_implicit_autoview(time_block.block); - let _ = run_block(&time_block, context, benchmark_output).await?; + context.scope.enter_scope(); + let result = run_block(&time_block, context, benchmark_output).await; + context.scope.exit_scope(); + result?; context.clear_errors(); Ok(block_output.into()) diff --git a/crates/nu-cli/src/commands/classified/block.rs b/crates/nu-cli/src/commands/classified/block.rs index 6959ffe301..5c73b86d21 100644 --- a/crates/nu-cli/src/commands/classified/block.rs +++ b/crates/nu-cli/src/commands/classified/block.rs @@ -19,7 +19,6 @@ pub async fn run_block( mut input: InputStream, ) -> Result { let mut output: Result = Ok(InputStream::empty()); - ctx.scope.enter_scope(); for (_, definition) in block.definitions.iter() { ctx.scope.add_definition(definition.clone()); } @@ -48,7 +47,6 @@ pub async fn run_block( { Ok(x) => x, Err(e) => { - ctx.scope.exit_scope(); return Err(e); } }; @@ -57,36 +55,30 @@ pub async fn run_block( value: UntaggedValue::Error(e), .. }))) => { - ctx.scope.exit_scope(); return Err(e); } Ok(Some(_item)) => { if let Some(err) = ctx.get_errors().get(0) { ctx.clear_errors(); - ctx.scope.exit_scope(); return Err(err.clone()); } if ctx.ctrl_c.load(Ordering::SeqCst) { - ctx.scope.exit_scope(); return Ok(InputStream::empty()); } } Ok(None) => { if let Some(err) = ctx.get_errors().get(0) { ctx.clear_errors(); - ctx.scope.exit_scope(); return Err(err.clone()); } } Err(e) => { - ctx.scope.exit_scope(); return Err(e); } } } } Err(e) => { - ctx.scope.exit_scope(); return Err(e); } } @@ -102,13 +94,11 @@ pub async fn run_block( value: UntaggedValue::Error(e), .. }))) => { - ctx.scope.exit_scope(); return Err(e); } Ok(Some(_item)) => { if let Some(err) = ctx.get_errors().get(0) { ctx.clear_errors(); - ctx.scope.exit_scope(); return Err(err.clone()); } if ctx.ctrl_c.load(Ordering::SeqCst) { @@ -117,25 +107,21 @@ pub async fn run_block( // causes lifetime issues. A future contribution // could attempt to return the current output. // https://github.com/nushell/nushell/pull/2830#discussion_r550319687 - ctx.scope.exit_scope(); return Ok(InputStream::empty()); } } Ok(None) => { if let Some(err) = ctx.get_errors().get(0) { ctx.clear_errors(); - ctx.scope.exit_scope(); return Err(err.clone()); } } Err(e) => { - ctx.scope.exit_scope(); return Err(e); } } } Err(e) => { - ctx.scope.exit_scope(); return Err(e); } } @@ -144,7 +130,6 @@ pub async fn run_block( input = InputStream::empty(); } } - ctx.scope.exit_scope(); output } diff --git a/crates/nu-cli/src/commands/classified/internal.rs b/crates/nu-cli/src/commands/classified/internal.rs index d1a8b19cbf..413ac9eeff 100644 --- a/crates/nu-cli/src/commands/classified/internal.rs +++ b/crates/nu-cli/src/commands/classified/internal.rs @@ -176,39 +176,6 @@ pub(crate) async fn run_internal_command( )); InputStream::from_stream(futures::stream::iter(vec![])) } - CommandAction::AddVariable(name, value) => { - context.scope.add_var(name, value); - InputStream::from_stream(futures::stream::iter(vec![])) - } - CommandAction::AddEnvVariable(name, value) => { - context.scope.add_env_var(name, value); - InputStream::from_stream(futures::stream::iter(vec![])) - } - CommandAction::SourceScript(filename) => { - let contents = std::fs::read_to_string(&filename.item); - match contents { - Ok(contents) => { - let result = crate::script::run_script_standalone( - contents, true, &context, false, - ) - .await; - - if let Err(err) = result { - context.error(err.into()); - } - InputStream::empty() - } - Err(_) => { - context.error(ShellError::labeled_error( - "Can't load file to source", - "can't load file", - filename.span(), - )); - - InputStream::empty() - } - } - } CommandAction::AddPlugins(path) => { match crate::plugin::scan(vec![std::path::PathBuf::from(path)]) { Ok(plugins) => { diff --git a/crates/nu-cli/src/commands/do_.rs b/crates/nu-cli/src/commands/do_.rs index 9fd4264f0d..9d94f621b4 100644 --- a/crates/nu-cli/src/commands/do_.rs +++ b/crates/nu-cli/src/commands/do_.rs @@ -83,8 +83,9 @@ async fn do_(raw_args: CommandArgs) -> Result { }; block.block.set_redirect(block_redirection); - + context.scope.enter_scope(); let result = run_block(&block.block, &context, input).await; + context.scope.exit_scope(); if ignore_errors { // To properly ignore errors we need to redirect stderr, consume it, and remove diff --git a/crates/nu-cli/src/commands/set.rs b/crates/nu-cli/src/commands/set.rs index 4e80289ead..534b5fc944 100644 --- a/crates/nu-cli/src/commands/set.rs +++ b/crates/nu-cli/src/commands/set.rs @@ -2,10 +2,7 @@ use crate::prelude::*; use crate::{commands::WholeStreamCommand, evaluate::evaluate_baseline_expr}; use nu_errors::ShellError; -use nu_protocol::{ - hir::CapturedBlock, hir::ClassifiedCommand, CommandAction, ReturnSuccess, Signature, - SyntaxShape, -}; +use nu_protocol::{hir::CapturedBlock, hir::ClassifiedCommand, Signature, SyntaxShape}; use nu_source::Tagged; pub struct Set; @@ -97,7 +94,10 @@ pub async fn set(args: CommandArgs) -> Result { format!("${}", name.item) }; - Ok(OutputStream::one(ReturnSuccess::action( - CommandAction::AddVariable(name, value), - ))) + // Note: this is a special case for setting the context from a command + // In this case, if we don't set it now, we'll lose the scope that this + // variable should be set into. + ctx.scope.add_var(name, value); + + Ok(OutputStream::empty()) } diff --git a/crates/nu-cli/src/commands/set_env.rs b/crates/nu-cli/src/commands/set_env.rs index 2d20e1468b..56c7bd15b7 100644 --- a/crates/nu-cli/src/commands/set_env.rs +++ b/crates/nu-cli/src/commands/set_env.rs @@ -2,10 +2,7 @@ use crate::prelude::*; use crate::{commands::WholeStreamCommand, evaluate::evaluate_baseline_expr}; use nu_errors::ShellError; -use nu_protocol::{ - hir::CapturedBlock, hir::ClassifiedCommand, CommandAction, ReturnSuccess, Signature, - SyntaxShape, -}; +use nu_protocol::{hir::CapturedBlock, hir::ClassifiedCommand, Signature, SyntaxShape}; use nu_source::Tagged; pub struct SetEnv; @@ -98,7 +95,10 @@ pub async fn set_env(args: CommandArgs) -> Result { let name = name.item.clone(); - Ok(OutputStream::one(ReturnSuccess::action( - CommandAction::AddEnvVariable(name, value), - ))) + // Note: this is a special case for setting the context from a command + // In this case, if we don't set it now, we'll lose the scope that this + // variable should be set into. + ctx.scope.add_env_var(name, value); + + Ok(OutputStream::empty()) } diff --git a/crates/nu-cli/src/commands/source.rs b/crates/nu-cli/src/commands/source.rs index 776382d30b..72ae45a6bf 100644 --- a/crates/nu-cli/src/commands/source.rs +++ b/crates/nu-cli/src/commands/source.rs @@ -2,7 +2,7 @@ use crate::commands::WholeStreamCommand; use crate::prelude::*; use nu_errors::ShellError; -use nu_protocol::{CommandAction, ReturnSuccess, Signature, SyntaxShape}; +use nu_protocol::{Signature, SyntaxShape}; use nu_source::Tagged; pub struct Source; @@ -40,9 +40,30 @@ impl WholeStreamCommand for Source { } pub async fn source(args: CommandArgs) -> Result { + let ctx = EvaluationContext::from_args(&args); let (SourceArgs { filename }, _) = args.process().await?; - Ok(OutputStream::one(ReturnSuccess::action( - CommandAction::SourceScript(filename), - ))) + // Note: this is a special case for setting the context from a command + // In this case, if we don't set it now, we'll lose the scope that this + // variable should be set into. + let contents = std::fs::read_to_string(&filename.item); + match contents { + Ok(contents) => { + let result = crate::script::run_script_standalone(contents, true, &ctx, false).await; + + if let Err(err) = result { + ctx.error(err.into()); + } + Ok(OutputStream::empty()) + } + Err(_) => { + ctx.error(ShellError::labeled_error( + "Can't load file to source", + "can't load file", + filename.span(), + )); + + Ok(OutputStream::empty()) + } + } } diff --git a/crates/nu-cli/src/evaluate/scope.rs b/crates/nu-cli/src/evaluate/scope.rs index a975d47b22..1d8a91788e 100644 --- a/crates/nu-cli/src/evaluate/scope.rs +++ b/crates/nu-cli/src/evaluate/scope.rs @@ -52,6 +52,10 @@ impl Scope { names } + pub fn len(&self) -> usize { + self.frames.lock().len() + } + fn has_cmd_helper(&self, name: &str, f: fn(&ScopeFrame, &str) -> bool) -> bool { self.frames.lock().iter().any(|frame| f(frame, name)) } diff --git a/crates/nu-cli/tests/commands/every.rs b/crates/nu-cli/tests/commands/every.rs index 9992713460..f48b2bf2ab 100644 --- a/crates/nu-cli/tests/commands/every.rs +++ b/crates/nu-cli/tests/commands/every.rs @@ -6,10 +6,10 @@ use nu_test_support::{nu, pipeline}; fn gets_all_rows_by_every_zero() { Playground::setup("every_test_1", |dirs, sandbox| { sandbox.with_files(vec![ - EmptyFile("los.txt"), - EmptyFile("tres.txt"), EmptyFile("amigos.txt"), EmptyFile("arepas.clu"), + EmptyFile("los.txt"), + EmptyFile("tres.txt"), ]); let actual = nu!( @@ -18,17 +18,14 @@ fn gets_all_rows_by_every_zero() { ls | get name | every 0 + | to json "# )); - let expected = nu!( - cwd: dirs.test(), pipeline( - r#" - echo [ amigos.txt arepas.clu los.txt tres.txt ] - "# - )); - - assert_eq!(actual.out, expected.out); + assert_eq!( + actual.out, + r#"["amigos.txt","arepas.clu","los.txt","tres.txt"]"# + ); }) } @@ -36,10 +33,10 @@ fn gets_all_rows_by_every_zero() { fn gets_no_rows_by_every_skip_zero() { Playground::setup("every_test_2", |dirs, sandbox| { sandbox.with_files(vec![ - EmptyFile("los.txt"), - EmptyFile("tres.txt"), EmptyFile("amigos.txt"), EmptyFile("arepas.clu"), + EmptyFile("los.txt"), + EmptyFile("tres.txt"), ]); let actual = nu!( @@ -48,17 +45,11 @@ fn gets_no_rows_by_every_skip_zero() { ls | get name | every 0 --skip + | to json "# )); - let expected = nu!( - cwd: dirs.test(), pipeline( - r#" - echo [ ] - "# - )); - - assert_eq!(actual.out, expected.out); + assert_eq!(actual.out, ""); }) } @@ -66,10 +57,10 @@ fn gets_no_rows_by_every_skip_zero() { fn gets_all_rows_by_every_one() { Playground::setup("every_test_3", |dirs, sandbox| { sandbox.with_files(vec![ - EmptyFile("los.txt"), - EmptyFile("tres.txt"), EmptyFile("amigos.txt"), EmptyFile("arepas.clu"), + EmptyFile("los.txt"), + EmptyFile("tres.txt"), ]); let actual = nu!( @@ -78,17 +69,14 @@ fn gets_all_rows_by_every_one() { ls | get name | every 1 + | to json "# )); - let expected = nu!( - cwd: dirs.test(), pipeline( - r#" - echo [ amigos.txt arepas.clu los.txt tres.txt ] - "# - )); - - assert_eq!(actual.out, expected.out); + assert_eq!( + actual.out, + r#"["amigos.txt","arepas.clu","los.txt","tres.txt"]"# + ); }) } @@ -96,10 +84,10 @@ fn gets_all_rows_by_every_one() { fn gets_no_rows_by_every_skip_one() { Playground::setup("every_test_4", |dirs, sandbox| { sandbox.with_files(vec![ - EmptyFile("los.txt"), - EmptyFile("tres.txt"), EmptyFile("amigos.txt"), EmptyFile("arepas.clu"), + EmptyFile("los.txt"), + EmptyFile("tres.txt"), ]); let actual = nu!( @@ -108,17 +96,11 @@ fn gets_no_rows_by_every_skip_one() { ls | get name | every 1 --skip + | to json "# )); - let expected = nu!( - cwd: dirs.test(), pipeline( - r#" - echo [ ] - "# - )); - - assert_eq!(actual.out, expected.out); + assert_eq!(actual.out, ""); }) } @@ -126,10 +108,10 @@ fn gets_no_rows_by_every_skip_one() { fn gets_first_row_by_every_too_much() { Playground::setup("every_test_5", |dirs, sandbox| { sandbox.with_files(vec![ - EmptyFile("los.txt"), - EmptyFile("tres.txt"), EmptyFile("amigos.txt"), EmptyFile("arepas.clu"), + EmptyFile("los.txt"), + EmptyFile("tres.txt"), ]); let actual = nu!( @@ -156,10 +138,10 @@ fn gets_first_row_by_every_too_much() { fn gets_all_rows_except_first_by_every_skip_too_much() { Playground::setup("every_test_6", |dirs, sandbox| { sandbox.with_files(vec![ - EmptyFile("los.txt"), - EmptyFile("tres.txt"), EmptyFile("amigos.txt"), EmptyFile("arepas.clu"), + EmptyFile("los.txt"), + EmptyFile("tres.txt"), ]); let actual = nu!( @@ -168,17 +150,11 @@ fn gets_all_rows_except_first_by_every_skip_too_much() { ls | get name | every 999 --skip + | to json "# )); - let expected = nu!( - cwd: dirs.test(), pipeline( - r#" - echo [ arepas.clu los.txt tres.txt ] - "# - )); - - assert_eq!(actual.out, expected.out); + assert_eq!(actual.out, r#"["arepas.clu","los.txt","tres.txt"]"#); }) } @@ -186,11 +162,11 @@ fn gets_all_rows_except_first_by_every_skip_too_much() { fn gets_every_third_row() { Playground::setup("every_test_7", |dirs, sandbox| { sandbox.with_files(vec![ - EmptyFile("los.txt"), - EmptyFile("tres.txt"), - EmptyFile("quatro.txt"), EmptyFile("amigos.txt"), EmptyFile("arepas.clu"), + EmptyFile("los.txt"), + EmptyFile("quatro.txt"), + EmptyFile("tres.txt"), ]); let actual = nu!( @@ -199,17 +175,11 @@ fn gets_every_third_row() { ls | get name | every 3 + | to json "# )); - let expected = nu!( - cwd: dirs.test(), pipeline( - r#" - echo [ amigos.txt quatro.txt ] - "# - )); - - assert_eq!(actual.out, expected.out); + assert_eq!(actual.out, r#"["amigos.txt","quatro.txt"]"#); }) } @@ -217,11 +187,11 @@ fn gets_every_third_row() { fn skips_every_third_row() { Playground::setup("every_test_8", |dirs, sandbox| { sandbox.with_files(vec![ - EmptyFile("los.txt"), - EmptyFile("tres.txt"), - EmptyFile("quatro.txt"), EmptyFile("amigos.txt"), EmptyFile("arepas.clu"), + EmptyFile("los.txt"), + EmptyFile("quatro.txt"), + EmptyFile("tres.txt"), ]); let actual = nu!( @@ -230,16 +200,10 @@ fn skips_every_third_row() { ls | get name | every 3 --skip + | to json "# )); - let expected = nu!( - cwd: dirs.test(), pipeline( - r#" - echo [ arepas.clu los.txt tres.txt ] - "# - )); - - assert_eq!(actual.out, expected.out); + assert_eq!(actual.out, r#"["arepas.clu","los.txt","tres.txt"]"#); }) } diff --git a/crates/nu-protocol/src/return_value.rs b/crates/nu-protocol/src/return_value.rs index a57d23ce0c..745860b940 100644 --- a/crates/nu-protocol/src/return_value.rs +++ b/crates/nu-protocol/src/return_value.rs @@ -1,6 +1,6 @@ use crate::value::Value; use nu_errors::ShellError; -use nu_source::{b, DebugDocBuilder, PrettyDebug, Tagged}; +use nu_source::{b, DebugDocBuilder, PrettyDebug}; use serde::{Deserialize, Serialize}; /// The inner set of actions for the command processor. Each denotes a way to change state in the processor without changing it directly from the command itself. @@ -20,14 +20,8 @@ pub enum CommandAction { EnterValueShell(Value), /// Enter the help shell, which allows exploring the help system EnterHelpShell(Value), - /// Add a variable into scope - AddVariable(String, Value), - /// Add an environment variable into scope - AddEnvVariable(String, String), /// Add plugins from path given AddPlugins(String), - /// Run the given script in the current context (given filename) - SourceScript(Tagged), /// Go to the previous shell in the shell ring buffer PreviousShell, /// Go to the next shell in the shell ring buffer @@ -49,9 +43,6 @@ impl PrettyDebug for CommandAction { CommandAction::EnterShell(s) => b::typed("enter shell", b::description(s)), CommandAction::EnterValueShell(v) => b::typed("enter value shell", v.pretty()), CommandAction::EnterHelpShell(v) => b::typed("enter help shell", v.pretty()), - CommandAction::AddVariable(..) => b::description("add variable"), - CommandAction::AddEnvVariable(..) => b::description("add environment variable"), - CommandAction::SourceScript(..) => b::description("source script"), CommandAction::AddPlugins(..) => b::description("add plugins"), CommandAction::PreviousShell => b::description("previous shell"), CommandAction::NextShell => b::description("next shell"), diff --git a/tests/shell/pipeline/commands/internal.rs b/tests/shell/pipeline/commands/internal.rs index f0e23931e7..c65a0f8909 100644 --- a/tests/shell/pipeline/commands/internal.rs +++ b/tests/shell/pipeline/commands/internal.rs @@ -418,6 +418,18 @@ fn set_variable() { assert_eq!(actual.out, "17"); } +#[test] +fn set_doesnt_leak() { + let actual = nu!( + cwd: ".", + r#" + do { set x = 5 }; echo $x + "# + ); + + assert!(actual.err.contains("unknown variable")); +} + #[test] fn set_env_variable() { let actual = nu!( @@ -431,6 +443,18 @@ fn set_env_variable() { assert_eq!(actual.out, "hello world"); } +#[test] +fn set_env_doesnt_leak() { + let actual = nu!( + cwd: ".", + r#" + do { set-env xyz = "my message" }; echo $nu.env.xyz + "# + ); + + assert!(actual.err.contains("did you mean")); +} + #[cfg(feature = "which")] #[test] fn argument_invocation_reports_errors() {