Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions console/web/src/components/chat/DirectoryPicker.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { describe, expect, it } from 'vitest'
import {
WORKSPACE_LIST_FUNCTION_ID,
WORKSPACE_ROOTS_FUNCTION_ID,
WORKSPACE_VALIDATE_FUNCTION_ID,
} from './DirectoryPicker'

describe('DirectoryPicker workspace API wiring', () => {
it('uses shell workspace control-plane functions', () => {
expect(WORKSPACE_ROOTS_FUNCTION_ID).toBe('shell::workspace::roots')
expect(WORKSPACE_LIST_FUNCTION_ID).toBe('shell::workspace::list')
expect(WORKSPACE_VALIDATE_FUNCTION_ID).toBe('shell::workspace::validate')
})
})
71 changes: 45 additions & 26 deletions console/web/src/components/chat/DirectoryPicker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@ import { cn } from '@/lib/utils'
* Per-session working-directory picker, project-switcher style.
*
* Opens to your remembered projects (most-recent first) — pick one in a click,
* or "browse to add" a new directory. Browsing lists the operator's coder roots
* (`coder::info`) one level at a time (`coder::list-folder`, lazy). The search
* box filters the current level live; typing/pasting an absolute path jumps
* straight there (browse) or selects it (projects). A pasted/remembered dir is
* validated against the live roots before it's accepted. The chosen dir is what
* the harness scopes the chat to (`base_dir`); it is re-scopable mid-conversation
* (a change drops a visible transcript marker).
* or "browse to add" a new directory. Browsing uses shell's operator workspace
* control plane one level at a time. The search box filters the current level
* live; typing/pasting an absolute path jumps straight there (browse) or
* selects it (projects). A pasted/remembered dir is validated against the live
* shell worker before it's accepted. The chosen dir is what the harness scopes
* the chat to (`base_dir`); it is re-scopable mid-conversation (a change drops
* a visible transcript marker).
*/

interface DirectoryPickerProps {
Expand All @@ -36,21 +36,25 @@ interface DirectoryPickerProps {
className?: string
}

interface CoderInfo {
base_paths?: string[]
interface WorkspaceRootsResult {
roots?: string[]
}

interface DirEntry {
name: string
kind: string
non_accessible?: boolean
path: string
}

interface ListFolderResult {
interface WorkspaceListResult {
path: string
entries?: DirEntry[]
}

interface WorkspaceValidateResult {
path: string
}

function basename(p: string): string {
const parts = p.split('/').filter(Boolean)
return parts.length ? parts[parts.length - 1] : p
Expand All @@ -69,6 +73,10 @@ function parentOf(p: string): string {

const isAbsPath = (s: string) => s.trim().startsWith('/')

export const WORKSPACE_ROOTS_FUNCTION_ID = 'shell::workspace::roots'
export const WORKSPACE_LIST_FUNCTION_ID = 'shell::workspace::list'
export const WORKSPACE_VALIDATE_FUNCTION_ID = 'shell::workspace::validate'

/**
* iii triggers reject with a plain object `{ code, message }`, not an Error, and
* the message is often a nested `handler error: {"code":"C211","message":"…"}`.
Expand Down Expand Up @@ -140,8 +148,11 @@ export function DirectoryPicker({
setError(null)
try {
const client = await getIiiClient()
const info = await client.trigger<CoderInfo>('coder::info', {})
const r = info?.base_paths ?? []
const info = await client.trigger<WorkspaceRootsResult>(
WORKSPACE_ROOTS_FUNCTION_ID,
{},
)
const r = info?.roots ?? []
setRoots(r)
return r
} catch (err) {
Expand All @@ -158,13 +169,16 @@ export function DirectoryPicker({
setError(null)
try {
const client = await getIiiClient()
const res = await client.trigger<ListFolderResult>('coder::list-folder', {
path: target,
page_size: 200,
})
const res = await client.trigger<WorkspaceListResult>(
WORKSPACE_LIST_FUNCTION_ID,
{
path: target,
page_size: 200,
},
)
const names = (res?.entries ?? [])
.filter((e) => e.kind === 'dir' && !e.non_accessible)
.map((e) => `${target.replace(/\/+$/, '')}/${e.name}`)
.filter((e) => e.kind === 'dir')
.map((e) => e.path)
.sort((a, b) => a.localeCompare(b))
setDirs(names)
} catch (err) {
Expand Down Expand Up @@ -235,7 +249,13 @@ export function DirectoryPicker({
setView('browse')
setQuery('')
const r = await ensureRoots()
setRoot(r.find((x) => p === x || p.startsWith(`${x}/`)) ?? r[0] ?? null)
const matchedRoot =
[...r]
.sort((a, b) => b.length - a.length)
.find((x) => p === x || p.startsWith(`${x}/`)) ??
r[0] ??
null
setRoot(matchedRoot)
setPath(p)
await loadFolder(p)
},
Expand All @@ -260,13 +280,12 @@ export function DirectoryPicker({
setValidating(dir)
try {
const client = await getIiiClient()
const res = await client.trigger<ListFolderResult>(
'coder::list-folder',
{ path: dir, page_size: 1 },
const res = await client.trigger<WorkspaceValidateResult>(
WORKSPACE_VALIDATE_FUNCTION_ID,
{ path: dir },
)
// Select the CANONICAL resolved dir the worker echoes back — not the
// raw input — so what's stored is exactly what coder will resolve to
// (a file path errors C210; a non-existent path errors C211).
// Select the canonical resolved dir the worker echoes back, not raw
// input, so stored recent projects are stable across symlinks.
select(res?.path ?? dir)
} catch (err) {
setError(`can't use ${dir} — ${errMsg(err)}`)
Expand Down
6 changes: 3 additions & 3 deletions console/web/src/hooks/use-shell-status.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
import {
type WorkerPresence,
isWorkerPresent,
useWorkerPresence,
type WorkerPresence,
} from './use-worker-presence'

/**
* Presence probe for the `shell` worker. Shell owns the chat's working-directory
* surface: the `coder::*` directory-discovery functions the `DirectoryPicker`
* browses, and the `shell::*` exec/file calls the chosen dir scopes. It is
* surface: the `shell::workspace::*` picker control plane and the `shell::*` /
* `coder::*` calls the chosen dir scopes. It is
* OPTIONAL, so the console gates the working-directory picker + banner on its
* presence rather than rendering controls that would call functions that don't
* exist. Thin wrapper over the generic worker-presence probe.
Expand Down
8 changes: 7 additions & 1 deletion console/web/src/lib/backend/real-metadata.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { describe, expect, it } from 'vitest'
import { buildTurnMetadata } from './real'
import { buildTurnMetadata, FALLBACK_FUNCTION_POLICY } from './real'

/**
* C1 regression: working_dir must ride options.metadata so the harness sees it
Expand All @@ -24,3 +24,9 @@ describe('buildTurnMetadata — working_dir forwarding', () => {
}
})
})

describe('fallback function policy', () => {
it('does not expose workspace picker functions to the agent', () => {
expect(FALLBACK_FUNCTION_POLICY.deny).toContain('shell::workspace::*')
})
})
4 changes: 2 additions & 2 deletions console/web/src/lib/backend/real.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,9 @@ interface RunParams {
* via `agent_trigger`. The approval-gate rules supply the structural floor;
* the gate hook remains the human decision surface.
*/
const FALLBACK_FUNCTION_POLICY: HarnessFunctionPolicy = {
export const FALLBACK_FUNCTION_POLICY: HarnessFunctionPolicy = {
allow: ['*'],
deny: ['approval::*', 'configuration::*'],
deny: ['approval::*', 'configuration::*', 'shell::workspace::*'],
expose: 'agent_trigger',
}

Expand Down
45 changes: 29 additions & 16 deletions harness/src/workspace_inject.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,33 +18,36 @@ const BASE_DIR_FIELD: &str = "base_dir";
/// True when `function_id` names a workspace-scoped worker call (`shell::*` or
/// `coder::*`) whose paths must be anchored at the session working directory.
fn is_scoped_function(function_id: &str) -> bool {
function_id.starts_with("shell::") || function_id.starts_with("coder::")
(function_id.starts_with("shell::") && !function_id.starts_with("shell::workspace::"))
|| function_id.starts_with("coder::")
}

/// Stamp the session `working_dir` onto a scoped call's arguments as `base_dir`.
///
/// Returns a NEW `Value` (the codebase favours immutability — no in-place
/// mutation of the caller's args). The original is returned UNCHANGED when any
/// of the following holds:
/// * `working_dir` is `None` (no per-session scope is active — the
/// byte-for-byte back-compat path);
/// * `function_id` is not a `shell::*` / `coder::*` call;
/// * `args` is not a JSON object (nowhere to place a top-level field).
/// mutation of the caller's args). The original is returned unchanged when
/// `function_id` is not a `shell::*` / `coder::*` call, or when `args` is not a
/// JSON object (nowhere to place a top-level field).
///
/// When it does apply, the top-level `base_dir` is SET to `working_dir`,
/// OVERWRITING any caller-supplied value — the harness controls scoping and the
/// OVERWRITING any caller-supplied value. If no `working_dir` is active, a
/// caller-supplied `base_dir` is removed. The harness controls scoping and the
/// model must not be able to widen it.
pub fn inject(function_id: &str, args: Value, working_dir: Option<&str>) -> Value {
let Some(dir) = working_dir else {
return args;
};
if !is_scoped_function(function_id) {
return args;
}
let Value::Object(mut map) = args else {
return args;
};
map.insert(BASE_DIR_FIELD.to_string(), Value::String(dir.to_string()));
match working_dir {
Some(dir) => {
map.insert(BASE_DIR_FIELD.to_string(), Value::String(dir.to_string()));
}
None => {
map.remove(BASE_DIR_FIELD);
}
}
Value::Object(map)
}

Expand Down Expand Up @@ -95,11 +98,21 @@ mod tests {
}

#[test]
fn leaves_args_unchanged_when_working_dir_absent() {
// Back-compat: with no session scope the args pass through verbatim,
// including any caller-supplied base_dir.
fn strips_caller_supplied_base_dir_when_working_dir_absent() {
// With no session scope, the model must not be able to invent one.
let args = json!({ "command": "ls", "base_dir": "/etc" });
let out = inject("shell::exec", args.clone(), None);
let out = inject("shell::exec", args, None);
assert_eq!(out, json!({ "command": "ls" }));
}

#[test]
fn passthrough_for_workspace_control_plane_functions() {
let args = json!({ "path": "/Users/example/project" });
let out = inject(
"shell::workspace::validate",
args.clone(),
Some("/work/session-7"),
);
assert_eq!(out, args);
}

Expand Down
7 changes: 2 additions & 5 deletions shell/src/code/functions/create_file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,9 @@ use crate::code::path::PathResolver;
#[schemars(example = "example_create_file_input")]
pub struct CreateFileInput {
pub files: Vec<CreateFileSpec>,
/// Optional per-call session working directory. When set, relative
/// `path`s anchor here instead of the primary allowed root, and every
/// resolved path must stay inside it. `base_dir` itself must canonicalize
/// inside an allowed root (`coder::info` lists them). Omit to resolve
/// against the primary allowed root exactly as before.
/// Internal harness-scoped working directory; omitted from published schema.
#[serde(default)]
#[schemars(skip)]
pub base_dir: Option<String>,
}

Expand Down
7 changes: 2 additions & 5 deletions shell/src/code/functions/delete_file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,9 @@ pub struct DeleteFileInput {
/// Required for non-empty directories. Files and empty dirs ignore it.
#[serde(default)]
pub recursive: bool,
/// Optional per-call session working directory. When set, relative
/// `paths` anchor here instead of the primary allowed root, and every
/// resolved path must stay inside it. `base_dir` itself must canonicalize
/// inside an allowed root (`coder::info` lists them). Omit to resolve
/// against the primary allowed root exactly as before.
/// Internal harness-scoped working directory; omitted from published schema.
#[serde(default)]
#[schemars(skip)]
pub base_dir: Option<String>,
}

Expand Down
7 changes: 2 additions & 5 deletions shell/src/code/functions/list_folder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,9 @@ pub struct ListFolderInput {
/// `config.list_default_page_size` when omitted.
#[serde(default)]
pub page_size: Option<u32>,
/// Optional per-call session working directory. When set, a relative
/// `path` anchors here instead of the primary allowed root, and the
/// resolved folder must stay inside it. `base_dir` itself must canonicalize
/// inside an allowed root (`coder::info` lists them). Omit to resolve
/// against the primary allowed root exactly as before.
/// Internal harness-scoped working directory; omitted from published schema.
#[serde(default)]
#[schemars(skip)]
pub base_dir: Option<String>,
}

Expand Down
12 changes: 11 additions & 1 deletion shell/src/code/functions/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,7 @@ fn register_read_file(iii: &IIIClient, resolver: Arc<PathResolver>, cfg: ConfigC
let resolver = resolver.clone();
let cfg = cfg.clone();
async move {
let resolver = resolver.session_scoped(req.base_dir.as_deref());
let cfg = cfg.read().await.clone();
Comment on lines +294 to 295

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift

Don't treat wire base_dir as trusted input.

All of these handlers now widen the resolver with req.base_dir, but the corresponding request types still deserialize that field from raw JSON; #[schemars(skip)] only hides it from published schema. A direct coder::* call can therefore smuggle base_dir in the payload and escape the configured coder roots to any absolute directory that session_scoped() accepts. Strip or overwrite base_dir at the registration boundary, or split public request types from the internal harness-injected ones. Based on review stack context, harness-side stripping only protects the harness path; it does not harden direct function invocations.

Also applies to: 312-313, 330-331, 348-349, 365-366, 382-383, 400-401, 414-416

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@shell/src/code/functions/mod.rs` around lines 294 - 295, The handlers that
call resolver.session_scoped(req.base_dir.as_deref()) are trusting req.base_dir
from deserialized request payloads, which lets direct coder::* calls widen
access outside configured roots. Fix this by preventing public request structs
from accepting base_dir at all, or by stripping/overwriting it at the
registration boundary before any handler like the resolver/session_scoped paths
use it. Keep the harness-injected base_dir separate from the public request
types so only internal harness code can supply it, and direct invocations always
use a safe, server-controlled value.

read_file::handle(resolver, cfg, req)
.await
Expand All @@ -308,6 +309,7 @@ fn register_search(iii: &IIIClient, resolver: Arc<PathResolver>, cfg: ConfigCell
let resolver = resolver.clone();
let cfg = cfg.clone();
async move {
let resolver = resolver.session_scoped(req.base_dir.as_deref());
let cfg = cfg.read().await.clone();
search::handle(resolver, cfg, req)
.await
Expand All @@ -325,6 +327,7 @@ fn register_update_file(iii: &IIIClient, resolver: Arc<PathResolver>, cfg: Confi
let resolver = resolver.clone();
let cfg = cfg.clone();
async move {
let resolver = resolver.session_scoped(req.base_dir.as_deref());
let cfg = cfg.read().await.clone();
update_file::handle(resolver, cfg, req)
.await
Expand All @@ -342,6 +345,7 @@ fn register_create_file(iii: &IIIClient, resolver: Arc<PathResolver>, cfg: Confi
let resolver = resolver.clone();
let cfg = cfg.clone();
async move {
let resolver = resolver.session_scoped(req.base_dir.as_deref());
let cfg = cfg.read().await.clone();
create_file::handle(resolver, cfg, req)
.await
Expand All @@ -358,6 +362,7 @@ fn register_delete_file(iii: &IIIClient, resolver: Arc<PathResolver>) {
RegisterFunction::new_async(move |req: delete_file::DeleteFileInput| {
let resolver = resolver.clone();
async move {
let resolver = resolver.session_scoped(req.base_dir.as_deref());
delete_file::handle(resolver, req)
.await
.map_err(Error::from)
Expand All @@ -374,6 +379,7 @@ fn register_list_folder(iii: &IIIClient, resolver: Arc<PathResolver>, cfg: Confi
let resolver = resolver.clone();
let cfg = cfg.clone();
async move {
let resolver = resolver.session_scoped(req.base_dir.as_deref());
let cfg = cfg.read().await.clone();
list_folder::handle(resolver, cfg, req)
.await
Expand All @@ -391,6 +397,7 @@ fn register_tree(iii: &IIIClient, resolver: Arc<PathResolver>, cfg: ConfigCell)
let resolver = resolver.clone();
let cfg = cfg.clone();
async move {
let resolver = resolver.session_scoped(req.base_dir.as_deref());
let cfg = cfg.read().await.clone();
tree::handle(resolver, cfg, req).await.map_err(Error::from)
}
Expand All @@ -404,7 +411,10 @@ fn register_move_file(iii: &IIIClient, resolver: Arc<PathResolver>) {
MOVE_FILE_ID,
RegisterFunction::new_async(move |req: move_file::MoveFileInput| {
let resolver = resolver.clone();
async move { move_file::handle(resolver, req).await.map_err(Error::from) }
async move {
let resolver = resolver.session_scoped(req.base_dir.as_deref());
move_file::handle(resolver, req).await.map_err(Error::from)
}
})
.description(MOVE_FILE_DESC),
);
Expand Down
7 changes: 2 additions & 5 deletions shell/src/code/functions/move_file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,9 @@ pub struct MoveFileInput {
/// Entries to move. Each entry is processed independently so a single
/// failure never aborts the rest.
pub files: Vec<MoveFileSpec>,
/// Optional per-call session working directory. When set, relative
/// `from`/`to` paths anchor here instead of the primary allowed root,
/// and BOTH resolved endpoints must stay inside it. `base_dir` itself
/// must canonicalize inside an allowed root (`coder::info` lists them).
/// Omit to resolve against the primary allowed root exactly as before.
/// Internal harness-scoped working directory; omitted from published schema.
#[serde(default)]
#[schemars(skip)]
pub base_dir: Option<String>,
}

Expand Down
Loading
Loading