-
Notifications
You must be signed in to change notification settings - Fork 241
feat: add cloudtrail tracking to execute_bash #2535
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ use regex::Regex; | |
use serde::Deserialize; | ||
use tracing::error; | ||
|
||
use super::env_vars_with_user_agent; | ||
use crate::cli::agent::{ | ||
Agent, | ||
PermissionEvalResult, | ||
|
@@ -128,8 +129,8 @@ impl ExecuteCommand { | |
false | ||
} | ||
|
||
pub async fn invoke(&self, output: &mut impl Write) -> Result<InvokeOutput> { | ||
let output = run_command(&self.command, MAX_TOOL_RESPONSE_SIZE / 3, Some(output)).await?; | ||
pub async fn invoke(&self, os: &Os, output: &mut impl Write) -> Result<InvokeOutput> { | ||
let output = run_command(os, &self.command, MAX_TOOL_RESPONSE_SIZE / 3, Some(output)).await?; | ||
let clean_stdout = sanitize_unicode_tags(&output.stdout); | ||
let clean_stderr = sanitize_unicode_tags(&output.stderr); | ||
|
||
|
@@ -437,4 +438,53 @@ mod tests { | |
let res = tool.eval_perm(&agent); | ||
assert!(matches!(res, PermissionEvalResult::Allow)); | ||
} | ||
|
||
#[tokio::test] | ||
async fn test_cloudtrail_tracking() { | ||
use crate::cli::chat::consts::{ | ||
USER_AGENT_APP_NAME, | ||
USER_AGENT_ENV_VAR, | ||
USER_AGENT_VERSION_KEY, | ||
USER_AGENT_VERSION_VALUE, | ||
}; | ||
|
||
let os = Os::new().await.unwrap(); | ||
|
||
// Test that env_vars_with_user_agent sets the AWS_EXECUTION_ENV variable correctly | ||
let env_vars = env_vars_with_user_agent(&os); | ||
|
||
// Check that AWS_EXECUTION_ENV is set | ||
assert!(env_vars.contains_key(USER_AGENT_ENV_VAR)); | ||
|
||
let user_agent_value = env_vars.get(USER_AGENT_ENV_VAR).unwrap(); | ||
|
||
// Check the format is correct | ||
let expected_metadata = format!( | ||
"{} {}/{}", | ||
USER_AGENT_APP_NAME, USER_AGENT_VERSION_KEY, USER_AGENT_VERSION_VALUE | ||
); | ||
assert!(user_agent_value.contains(&expected_metadata)); | ||
} | ||
|
||
#[tokio::test] | ||
async fn test_cloudtrail_tracking_with_existing_env() { | ||
use crate::cli::chat::consts::{ | ||
USER_AGENT_APP_NAME, | ||
USER_AGENT_ENV_VAR, | ||
}; | ||
|
||
let os = Os::new().await.unwrap(); | ||
|
||
// Set an existing AWS_EXECUTION_ENV value (safe because Os uses in-memory hashmap in tests) | ||
unsafe { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's not use unsafe - tests run in multiple threads within the same process. Instead, pass the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we still going to keep the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is more context here. According to him unsafe here is not unsafe. I had added a non unsafe version but that was very complicated. So he referred to this method. |
||
os.env.set_var(USER_AGENT_ENV_VAR, "ExistingValue"); | ||
} | ||
|
||
let env_vars = env_vars_with_user_agent(&os); | ||
let user_agent_value = env_vars.get(USER_AGENT_ENV_VAR).unwrap(); | ||
|
||
// Should contain both the existing value and our metadata | ||
assert!(user_agent_value.contains("ExistingValue")); | ||
assert!(user_agent_value.contains(USER_AGENT_APP_NAME)); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't construct
Os
like this, you could just doOs::new
then follow with anunsafe set_var
(note that this is safe because the test instance uses an in-memory hashmap)