Skip to content

Commit 2a36205

Browse files
starr-openaicodex
starr-openai
and
codex
committed
codex: simplify sandboxed filesystem trait calls
Remove the unused sandbox cwd parameter from the exec-server filesystem trait and callsites. Keep the explicit cwd override private to the local access-check helper used by focused unit coverage. Co-authored-by: Codex <noreply@openai.com>
1 parent d41a048 commit 2a36205

5 files changed

Lines changed: 82 additions & 97 deletions

File tree

‎codex-rs/exec-server/src/file_system.rs‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,6 @@ pub trait ExecutorFileSystem: Send + Sync {
5050
&self,
5151
path: &AbsolutePathBuf,
5252
sandbox_policy: Option<&SandboxPolicy>,
53-
sandbox_cwd: Option<&AbsolutePathBuf>,
5453
) -> FileSystemResult<Vec<u8>>;
5554

5655
async fn write_file(&self, path: &AbsolutePathBuf, contents: Vec<u8>) -> FileSystemResult<()>;
@@ -60,7 +59,6 @@ pub trait ExecutorFileSystem: Send + Sync {
6059
path: &AbsolutePathBuf,
6160
contents: Vec<u8>,
6261
sandbox_policy: Option<&SandboxPolicy>,
63-
sandbox_cwd: Option<&AbsolutePathBuf>,
6462
) -> FileSystemResult<()>;
6563

6664
async fn create_directory(
@@ -74,7 +72,6 @@ pub trait ExecutorFileSystem: Send + Sync {
7472
path: &AbsolutePathBuf,
7573
create_directory_options: CreateDirectoryOptions,
7674
sandbox_policy: Option<&SandboxPolicy>,
77-
sandbox_cwd: Option<&AbsolutePathBuf>,
7875
) -> FileSystemResult<()>;
7976

8077
async fn get_metadata(&self, path: &AbsolutePathBuf) -> FileSystemResult<FileMetadata>;
@@ -83,7 +80,6 @@ pub trait ExecutorFileSystem: Send + Sync {
8380
&self,
8481
path: &AbsolutePathBuf,
8582
sandbox_policy: Option<&SandboxPolicy>,
86-
sandbox_cwd: Option<&AbsolutePathBuf>,
8783
) -> FileSystemResult<FileMetadata>;
8884

8985
async fn read_directory(
@@ -95,7 +91,6 @@ pub trait ExecutorFileSystem: Send + Sync {
9591
&self,
9692
path: &AbsolutePathBuf,
9793
sandbox_policy: Option<&SandboxPolicy>,
98-
sandbox_cwd: Option<&AbsolutePathBuf>,
9994
) -> FileSystemResult<Vec<ReadDirectoryEntry>>;
10095

10196
async fn remove(&self, path: &AbsolutePathBuf, options: RemoveOptions) -> FileSystemResult<()>;
@@ -105,7 +100,6 @@ pub trait ExecutorFileSystem: Send + Sync {
105100
path: &AbsolutePathBuf,
106101
remove_options: RemoveOptions,
107102
sandbox_policy: Option<&SandboxPolicy>,
108-
sandbox_cwd: Option<&AbsolutePathBuf>,
109103
) -> FileSystemResult<()>;
110104

111105
async fn copy(
@@ -121,6 +115,5 @@ pub trait ExecutorFileSystem: Send + Sync {
121115
destination_path: &AbsolutePathBuf,
122116
copy_options: CopyOptions,
123117
sandbox_policy: Option<&SandboxPolicy>,
124-
sandbox_cwd: Option<&AbsolutePathBuf>,
125118
) -> FileSystemResult<()>;
126119
}

‎codex-rs/exec-server/src/local_file_system.rs‎

Lines changed: 76 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -44,9 +44,8 @@ impl ExecutorFileSystem for LocalFileSystem {
4444
&self,
4545
path: &AbsolutePathBuf,
4646
sandbox_policy: Option<&SandboxPolicy>,
47-
sandbox_cwd: Option<&AbsolutePathBuf>,
4847
) -> FileSystemResult<Vec<u8>> {
49-
enforce_read_access(path, sandbox_policy, sandbox_cwd)?;
48+
enforce_read_access(path, sandbox_policy)?;
5049
self.read_file(path).await
5150
}
5251

@@ -59,9 +58,8 @@ impl ExecutorFileSystem for LocalFileSystem {
5958
path: &AbsolutePathBuf,
6059
contents: Vec<u8>,
6160
sandbox_policy: Option<&SandboxPolicy>,
62-
sandbox_cwd: Option<&AbsolutePathBuf>,
6361
) -> FileSystemResult<()> {
64-
enforce_write_access(path, sandbox_policy, sandbox_cwd)?;
62+
enforce_write_access(path, sandbox_policy)?;
6563
self.write_file(path, contents).await
6664
}
6765

@@ -83,9 +81,8 @@ impl ExecutorFileSystem for LocalFileSystem {
8381
path: &AbsolutePathBuf,
8482
create_directory_options: CreateDirectoryOptions,
8583
sandbox_policy: Option<&SandboxPolicy>,
86-
sandbox_cwd: Option<&AbsolutePathBuf>,
8784
) -> FileSystemResult<()> {
88-
enforce_write_access(path, sandbox_policy, sandbox_cwd)?;
85+
enforce_write_access(path, sandbox_policy)?;
8986
self.create_directory(path, create_directory_options).await
9087
}
9188

@@ -103,9 +100,8 @@ impl ExecutorFileSystem for LocalFileSystem {
103100
&self,
104101
path: &AbsolutePathBuf,
105102
sandbox_policy: Option<&SandboxPolicy>,
106-
sandbox_cwd: Option<&AbsolutePathBuf>,
107103
) -> FileSystemResult<FileMetadata> {
108-
enforce_read_access(path, sandbox_policy, sandbox_cwd)?;
104+
enforce_read_access(path, sandbox_policy)?;
109105
self.get_metadata(path).await
110106
}
111107

@@ -130,9 +126,8 @@ impl ExecutorFileSystem for LocalFileSystem {
130126
&self,
131127
path: &AbsolutePathBuf,
132128
sandbox_policy: Option<&SandboxPolicy>,
133-
sandbox_cwd: Option<&AbsolutePathBuf>,
134129
) -> FileSystemResult<Vec<ReadDirectoryEntry>> {
135-
enforce_read_access(path, sandbox_policy, sandbox_cwd)?;
130+
enforce_read_access(path, sandbox_policy)?;
136131
self.read_directory(path).await
137132
}
138133

@@ -161,9 +156,8 @@ impl ExecutorFileSystem for LocalFileSystem {
161156
path: &AbsolutePathBuf,
162157
remove_options: RemoveOptions,
163158
sandbox_policy: Option<&SandboxPolicy>,
164-
sandbox_cwd: Option<&AbsolutePathBuf>,
165159
) -> FileSystemResult<()> {
166-
enforce_write_access_preserving_leaf(path, sandbox_policy, sandbox_cwd)?;
160+
enforce_write_access_preserving_leaf(path, sandbox_policy)?;
167161
self.remove(path, remove_options).await
168162
}
169163

@@ -224,23 +218,20 @@ impl ExecutorFileSystem for LocalFileSystem {
224218
destination_path: &AbsolutePathBuf,
225219
copy_options: CopyOptions,
226220
sandbox_policy: Option<&SandboxPolicy>,
227-
sandbox_cwd: Option<&AbsolutePathBuf>,
228221
) -> FileSystemResult<()> {
229-
enforce_copy_source_read_access(source_path, sandbox_policy, sandbox_cwd)?;
230-
enforce_write_access(destination_path, sandbox_policy, sandbox_cwd)?;
222+
enforce_copy_source_read_access(source_path, sandbox_policy)?;
223+
enforce_write_access(destination_path, sandbox_policy)?;
231224
self.copy(source_path, destination_path, copy_options).await
232225
}
233226
}
234227

235228
fn enforce_read_access(
236229
path: &AbsolutePathBuf,
237230
sandbox_policy: Option<&SandboxPolicy>,
238-
sandbox_cwd: Option<&AbsolutePathBuf>,
239231
) -> FileSystemResult<()> {
240-
enforce_access(
232+
enforce_access_for_current_dir(
241233
path,
242234
sandbox_policy,
243-
sandbox_cwd,
244235
FileSystemSandboxPolicy::can_read_path_with_cwd,
245236
"read",
246237
AccessPathMode::ResolveAll,
@@ -250,12 +241,10 @@ fn enforce_read_access(
250241
fn enforce_write_access(
251242
path: &AbsolutePathBuf,
252243
sandbox_policy: Option<&SandboxPolicy>,
253-
sandbox_cwd: Option<&AbsolutePathBuf>,
254244
) -> FileSystemResult<()> {
255-
enforce_access(
245+
enforce_access_for_current_dir(
256246
path,
257247
sandbox_policy,
258-
sandbox_cwd,
259248
FileSystemSandboxPolicy::can_write_path_with_cwd,
260249
"write",
261250
AccessPathMode::ResolveAll,
@@ -265,12 +254,10 @@ fn enforce_write_access(
265254
fn enforce_write_access_preserving_leaf(
266255
path: &AbsolutePathBuf,
267256
sandbox_policy: Option<&SandboxPolicy>,
268-
sandbox_cwd: Option<&AbsolutePathBuf>,
269257
) -> FileSystemResult<()> {
270-
enforce_access(
258+
enforce_access_for_current_dir(
271259
path,
272260
sandbox_policy,
273-
sandbox_cwd,
274261
FileSystemSandboxPolicy::can_write_path_with_cwd,
275262
"write",
276263
AccessPathMode::PreserveLeaf,
@@ -280,38 +267,92 @@ fn enforce_write_access_preserving_leaf(
280267
fn enforce_copy_source_read_access(
281268
path: &AbsolutePathBuf,
282269
sandbox_policy: Option<&SandboxPolicy>,
283-
sandbox_cwd: Option<&AbsolutePathBuf>,
284270
) -> FileSystemResult<()> {
285271
let path_mode = match std::fs::symlink_metadata(path.as_path()) {
286272
Ok(metadata) if metadata.file_type().is_symlink() => AccessPathMode::PreserveLeaf,
287273
_ => AccessPathMode::ResolveAll,
288274
};
289-
enforce_access(
275+
enforce_access_for_current_dir(
276+
path,
277+
sandbox_policy,
278+
FileSystemSandboxPolicy::can_read_path_with_cwd,
279+
"read",
280+
path_mode,
281+
)
282+
}
283+
284+
#[cfg(test)]
285+
fn enforce_read_access_for_cwd(
286+
path: &AbsolutePathBuf,
287+
sandbox_policy: Option<&SandboxPolicy>,
288+
sandbox_cwd: &AbsolutePathBuf,
289+
) -> FileSystemResult<()> {
290+
enforce_access_for_cwd(
290291
path,
291292
sandbox_policy,
292293
sandbox_cwd,
293294
FileSystemSandboxPolicy::can_read_path_with_cwd,
294295
"read",
296+
AccessPathMode::ResolveAll,
297+
)
298+
}
299+
300+
fn enforce_access_for_current_dir(
301+
path: &AbsolutePathBuf,
302+
sandbox_policy: Option<&SandboxPolicy>,
303+
is_allowed: fn(&FileSystemSandboxPolicy, &Path, &Path) -> bool,
304+
access_kind: &str,
305+
path_mode: AccessPathMode,
306+
) -> FileSystemResult<()> {
307+
let Some(sandbox_policy) = sandbox_policy else {
308+
return Ok(());
309+
};
310+
let cwd = current_sandbox_cwd()?;
311+
enforce_access(
312+
path,
313+
sandbox_policy,
314+
cwd.as_path(),
315+
is_allowed,
316+
access_kind,
295317
path_mode,
296318
)
297319
}
298320

299-
fn enforce_access(
321+
#[cfg(test)]
322+
fn enforce_access_for_cwd(
300323
path: &AbsolutePathBuf,
301324
sandbox_policy: Option<&SandboxPolicy>,
302-
sandbox_cwd: Option<&AbsolutePathBuf>,
325+
sandbox_cwd: &AbsolutePathBuf,
303326
is_allowed: fn(&FileSystemSandboxPolicy, &Path, &Path) -> bool,
304327
access_kind: &str,
305328
path_mode: AccessPathMode,
306329
) -> FileSystemResult<()> {
307330
let Some(sandbox_policy) = sandbox_policy else {
308331
return Ok(());
309332
};
310-
let cwd = resolve_sandbox_cwd(sandbox_cwd)?;
333+
let cwd = resolve_existing_path(sandbox_cwd.as_path())?;
334+
enforce_access(
335+
path,
336+
sandbox_policy,
337+
cwd.as_path(),
338+
is_allowed,
339+
access_kind,
340+
path_mode,
341+
)
342+
}
343+
344+
fn enforce_access(
345+
path: &AbsolutePathBuf,
346+
sandbox_policy: &SandboxPolicy,
347+
sandbox_cwd: &Path,
348+
is_allowed: fn(&FileSystemSandboxPolicy, &Path, &Path) -> bool,
349+
access_kind: &str,
350+
path_mode: AccessPathMode,
351+
) -> FileSystemResult<()> {
311352
let resolved_path = resolve_path_for_access_check(path.as_path(), path_mode)?;
312353
let file_system_policy =
313354
canonicalize_file_system_policy_paths(FileSystemSandboxPolicy::from(sandbox_policy))?;
314-
if is_allowed(&file_system_policy, resolved_path.as_path(), cwd.as_path()) {
355+
if is_allowed(&file_system_policy, resolved_path.as_path(), sandbox_cwd) {
315356
Ok(())
316357
} else {
317358
Err(io::Error::new(
@@ -396,12 +437,9 @@ fn resolve_existing_path(path: &Path) -> io::Result<PathBuf> {
396437
Ok(resolved)
397438
}
398439

399-
fn resolve_sandbox_cwd(sandbox_cwd: Option<&AbsolutePathBuf>) -> io::Result<PathBuf> {
400-
let cwd = match sandbox_cwd {
401-
Some(cwd) => cwd.to_path_buf(),
402-
None => std::env::current_dir()
403-
.map_err(|err| io::Error::other(format!("failed to read current dir: {err}")))?,
404-
};
440+
fn current_sandbox_cwd() -> io::Result<PathBuf> {
441+
let cwd = std::env::current_dir()
442+
.map_err(|err| io::Error::other(format!("failed to read current dir: {err}")))?;
405443
resolve_existing_path(cwd.as_path())
406444
}
407445

@@ -531,9 +569,9 @@ mod tests {
531569
let other_cwd = absolute_path(other_dir);
532570
let note_path = absolute_path(note_path);
533571

534-
enforce_read_access(&note_path, Some(&sandbox_policy), Some(&sandbox_cwd))?;
572+
enforce_read_access_for_cwd(&note_path, Some(&sandbox_policy), &sandbox_cwd)?;
535573

536-
let error = enforce_read_access(&note_path, Some(&sandbox_policy), Some(&other_cwd))
574+
let error = enforce_read_access_for_cwd(&note_path, Some(&sandbox_policy), &other_cwd)
537575
.expect_err("read should be rejected outside provided cwd");
538576
assert_eq!(error.kind(), io::ErrorKind::InvalidInput);
539577
Ok(())

‎codex-rs/exec-server/src/remote_file_system.rs‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,6 @@ impl ExecutorFileSystem for RemoteFileSystem {
6262
&self,
6363
path: &AbsolutePathBuf,
6464
sandbox_policy: Option<&SandboxPolicy>,
65-
_sandbox_cwd: Option<&AbsolutePathBuf>,
6665
) -> FileSystemResult<Vec<u8>> {
6766
trace!("remote fs read_file_with_sandbox_policy");
6867
let response = self
@@ -99,7 +98,6 @@ impl ExecutorFileSystem for RemoteFileSystem {
9998
path: &AbsolutePathBuf,
10099
contents: Vec<u8>,
101100
sandbox_policy: Option<&SandboxPolicy>,
102-
_sandbox_cwd: Option<&AbsolutePathBuf>,
103101
) -> FileSystemResult<()> {
104102
trace!("remote fs write_file_with_sandbox_policy");
105103
self.client
@@ -135,7 +133,6 @@ impl ExecutorFileSystem for RemoteFileSystem {
135133
path: &AbsolutePathBuf,
136134
create_directory_options: CreateDirectoryOptions,
137135
sandbox_policy: Option<&SandboxPolicy>,
138-
_sandbox_cwd: Option<&AbsolutePathBuf>,
139136
) -> FileSystemResult<()> {
140137
trace!("remote fs create_directory_with_sandbox_policy");
141138
self.client
@@ -171,7 +168,6 @@ impl ExecutorFileSystem for RemoteFileSystem {
171168
&self,
172169
path: &AbsolutePathBuf,
173170
sandbox_policy: Option<&SandboxPolicy>,
174-
_sandbox_cwd: Option<&AbsolutePathBuf>,
175171
) -> FileSystemResult<FileMetadata> {
176172
trace!("remote fs get_metadata_with_sandbox_policy");
177173
let response = self
@@ -218,7 +214,6 @@ impl ExecutorFileSystem for RemoteFileSystem {
218214
&self,
219215
path: &AbsolutePathBuf,
220216
sandbox_policy: Option<&SandboxPolicy>,
221-
_sandbox_cwd: Option<&AbsolutePathBuf>,
222217
) -> FileSystemResult<Vec<ReadDirectoryEntry>> {
223218
trace!("remote fs read_directory_with_sandbox_policy");
224219
let response = self
@@ -259,7 +254,6 @@ impl ExecutorFileSystem for RemoteFileSystem {
259254
path: &AbsolutePathBuf,
260255
remove_options: RemoveOptions,
261256
sandbox_policy: Option<&SandboxPolicy>,
262-
_sandbox_cwd: Option<&AbsolutePathBuf>,
263257
) -> FileSystemResult<()> {
264258
trace!("remote fs remove_with_sandbox_policy");
265259
self.client
@@ -299,7 +293,6 @@ impl ExecutorFileSystem for RemoteFileSystem {
299293
destination_path: &AbsolutePathBuf,
300294
copy_options: CopyOptions,
301295
sandbox_policy: Option<&SandboxPolicy>,
302-
_sandbox_cwd: Option<&AbsolutePathBuf>,
303296
) -> FileSystemResult<()> {
304297
trace!("remote fs copy_with_sandbox_policy");
305298
self.client

0 commit comments

Comments
 (0)