Fail closed on unsafe config and sed parsing (#39700)

## Why

Unsupported untrusted approval policies must remain startup errors even when
app-server is allowed to fall back from other invalid configuration. Likewise,
compound command summaries must not discard a `sed` stage that can edit files
in place.

## What changed

- Propagate `UnsupportedUntrustedApprovalPolicyError` from both app-server
  configuration loads instead of replacing it with default configuration.
- Parse `sed` options through `--`, option arguments, combined short flags, and
  backup suffixes so `-i`/`--in-place` commands remain unknown actions.
- Keep non-mutating `sed` operands after `--` from being mistaken for flags.

## Testing

Added parser coverage for in-place `sed` variants in compound commands and for
dash-prefixed operands after `--`.

GitOrigin-RevId: 112ead912e10fcb6c7dd0ede4bf84e390af82da8
This commit is contained in:
jif
2026-08-20 11:47:16 +00:00
committed by copyberry
parent 478dbe9df0
commit f277e313f1
2 changed files with 81 additions and 11 deletions

View File

@@ -82,6 +82,14 @@ use tracing_subscriber::util::SubscriberInitExt;
const SQLITE_RECOVERY_CONFIG_WARNING_SUMMARY: &str = "Codex rebuilt its local database.";
fn is_unsupported_untrusted_approval_policy_error(err: &std::io::Error) -> bool {
err.get_ref().is_some_and(
<dyn std::error::Error + Send + Sync + 'static>::is::<
UnsupportedUntrustedApprovalPolicyError,
>,
)
}
mod analytics_utils;
mod app_info;
mod app_server_tracing;
@@ -505,13 +513,7 @@ pub async fn run_main_with_transport_options(
config.http_client_factory(),
);
}
Err(err)
if err.get_ref().is_some_and(
<dyn std::error::Error + Send + Sync + 'static>::is::<
UnsupportedUntrustedApprovalPolicyError,
>,
) =>
{
Err(err) if is_unsupported_untrusted_approval_policy_error(&err) => {
return Err(err);
}
Err(err) => {
@@ -526,6 +528,9 @@ pub async fn run_main_with_transport_options(
.await
{
Ok(config) => config,
Err(err) if is_unsupported_untrusted_approval_policy_error(&err) => {
return Err(err);
}
Err(err) => {
if strict_config {
return Err(err);

View File

@@ -856,6 +856,34 @@ mod tests {
)));
}
#[test]
fn keeps_mutating_sed_in_compound_command() {
for sed_command in [
"sed -n -i.bak 1p secret.txt",
"sed -ni.bak 1p secret.txt",
"sed -Eni.bak 1p secret.txt",
] {
let inner = format!("cat README.md && {sed_command}");
assert_parsed(
&vec_str(&["bash", "-lc", &inner]),
vec![ParsedCommand::Unknown { cmd: inner }],
);
}
}
#[test]
fn ignores_sed_operands_after_double_dash_when_checking_mutation() {
let inner = "cat README.md && sed 's/a/x/' -- -input.txt";
assert_parsed(
&vec_str(&["bash", "-lc", inner]),
vec![ParsedCommand::Read {
cmd: "cat README.md".to_string(),
name: "README.md".to_string(),
path: PathBuf::from("README.md"),
}],
);
}
#[test]
fn empty_tokens_is_not_small() {
let empty: Vec<String> = Vec::new();
@@ -1560,7 +1588,8 @@ fn is_valid_sed_n_arg(arg: Option<&str>) -> bool {
fn sed_read_path(args: &[String]) -> Option<String> {
let args_no_connector = trim_at_connector(args);
if has_in_place_flag(&args_no_connector) || !args_no_connector.iter().any(|arg| arg == "-n") {
if sed_has_in_place_flag(&args_no_connector) || !args_no_connector.iter().any(|arg| arg == "-n")
{
return None;
}
let mut has_range_script = false;
@@ -2158,8 +2187,10 @@ fn is_small_formatting_command(tokens: &[String]) -> bool {
}
"sed" => {
// Keep `sed -n <range> file` (treated as a file read elsewhere);
// otherwise consider it a formatting helper in a pipeline.
sed_read_path(&tokens[1..]).is_none()
// keep in-place mutations as unknown actions; otherwise consider it
// a formatting helper in a pipeline.
let args = &tokens[1..];
!sed_has_in_place_flag(args) && sed_read_path(args).is_none()
}
_ => false,
}
@@ -2200,7 +2231,8 @@ fn xargs_is_mutating_subcommand(tokens: &[String]) -> bool {
return false;
};
match head.as_str() {
"perl" | "ruby" | "sed" => has_in_place_flag(tail),
"perl" | "ruby" => has_in_place_flag(tail),
"sed" => sed_has_in_place_flag(tail),
"rg" => tail.iter().any(|token| token == "--replace"),
_ => false,
}
@@ -2217,6 +2249,39 @@ fn has_in_place_flag(tokens: &[String]) -> bool {
})
}
fn sed_has_in_place_flag(tokens: &[String]) -> bool {
let mut tokens = tokens.iter();
while let Some(token) = tokens.next() {
match token.as_str() {
"--" => break,
"-e" | "-f" | "--expression" | "--file" => {
let _ = tokens.next();
}
"--in-place" => return true,
token if token.starts_with("--in-place=") => return true,
token if token.starts_with("--") => {}
token => {
let Some(short_options) = token.strip_prefix('-') else {
continue;
};
for (index, option) in short_options.char_indices() {
match option {
'i' => return true,
'e' | 'f' => {
if index + option.len_utf8() == short_options.len() {
let _ = tokens.next();
}
break;
}
_ => {}
}
}
}
}
}
false
}
fn drop_small_formatting_commands(mut commands: Vec<Vec<String>>) -> Vec<Vec<String>> {
commands.retain(|tokens| !is_small_formatting_command(tokens));
commands