fix(fsmonitor): policy-check symlink leaf ops on the link, fall through to target eval

[es-fabricemarie]

Two related issues with how the FUSE policy layer handles symlinks in the workspace, both triggered by ordinary Python venv usage (python -m venv venv -- the most common thing an agent does with Python):

1. Leaf-symlink subject mismatch.

   check() always called resolveRealPathUnderRoot with mustExist=true, which dereferenced the leaf symlink before policy lookup. For ops whose subject is the path itself (stat, readlink, delete, rmdir), this re-routed the check to the symlink's target. So ls venv/bin (lstat on each entry) and rm -rf venv (unlink each entry) denied on the venv/bin/python -> /usr/bin/python3 link with EACCES, because the policy has no rule for /usr/bin/**.

   Fix: extend the mustExist=false set in check() to include stat, readlink, delete, and rmdir. The resolver then only walks the parent directory and leaves the leaf symlink alone. unlink/rmdir remove the symlink; the target is untouched, so checking the target was the wrong subject.

2. Workspace-escape blanket deny on system-symlink targets.

   When the resolved target of a symlink falls outside the workspace root, checkWithExist returned a hardcoded "workspace-escape" deny. That made it impossible to express "allow read via /usr/bin/python3" in policy -- every venv read/exec was an automatic deny regardless of file_rules.

   Fix: on resolveRealPathUnderRoot failure, call a new helper evalEscapedSymlink(realRoot, virtPath, virtualRoot) that fully resolves through symlinks even when the result lies outside the workspace. If it returns a resolvable real path, evaluate the policy on that path. Operators who want to block system symlinks can still do so via a regular file_rule deny on /usr/bin/** etc. "..":-style escapes and unresolvable links remain a hard deny since there is no useful real path to evaluate.

Tests:

• internal/fsmonitor/path_test.go: unit tests for evalEscapedSymlink (resolves outside target, returns "" on broken link, returns "" when path is not under virtual root).
• internal/fsmonitor/check_symlink_test.go: behavioral tests on a minimal node{} that exercise check() directly:
  • stat/readlink/delete/rmdir on a workspace symlink-to-outside no longer dereferences the leaf;
  • read on a workspace symlink-to-outside falls through to the file_rule governing the target (allow or deny), not the hardcoded workspace-escape rule;
  • "..":-style escapes still deny as workspace-escape.

Note: part 2 is a policy-semantics change. If maintainers prefer the fallthrough gated behind a config flag (e.g. policy.symlink_escape: "evaluate" | "deny"), happy to add that -- but the current default (evaluate, with operators expressing deny via regular file_rules) seems closer to the principle of least surprise: agentsh shouldn't have hardcoded path policy that operators can't override.

[erans]

Thanks, this looks like a real issue to me, especially for ordinary Python venv usage.

I agree with fixing the leaf-symlink behavior for operations like stat, readlink, delete, and rmdir: those should be checked against the symlink path itself, not the symlink target.

For the second part, I think we should avoid keeping the escaped-symlink target behavior as an unconditional hard-deny. My preference would be to make this controlled by a config.yaml setting, defaulting to the new behavior where escaped symlink targets are evaluated through normal file_rules. That keeps venvs usable by default, while still letting operators who want the stricter posture close this back down explicitly.

Something along these lines would work:

policy:
  symlink_escape: evaluate # evaluate | deny

So: enable target policy evaluation by default, but give deployments a clear flag to restore the blanket deny if they want it.

[es-fabricemarie]

@erans this should do it.

[erans]

Checked the follow-up fix. The config knob is mostly wired correctly: policies.symlink_escape accepts evaluate|deny, defaults to evaluate, and deny restores the old workspace-escape path in the covered symlink-target case.

Two things still need fixing before merge:

1. .. escapes can fall through to normal file_rules when the escaped sibling path exists. evalEscapedSymlink only verifies the raw virtual prefix, so /workspace/../outside/secret can resolve to a real sibling path and then be allowed by a broad rule like /**. The existing test uses /workspace/../etc/hostname, which is nonexistent under the temp parent, so it misses this. Please add a regression where the sibling path exists and keep this as workspace-escape.

2. Please run gofmt; current gofmt -l reports:

• internal/api/core.go
• internal/fsmonitor/fuse.go

Verification on cb558f10: go test ./... passes and GOOS=windows go build ./... passes.

[erans]

@es-fabricemarie gentle ping — just wanted to make sure my May 15 review didn't get lost. Two items still pending before merge:

1. .. escape regression test. evalEscapedSymlink uses pathutil.IsUnderRoot, which is a raw string-prefix check, so /workspace/../outside/secret passes it; then filepath.EvalSymlinks resolves the .. and returns a real sibling path. The existing TestCheck_DotDotEscapeStillDeniesAsWorkspaceEscape only passes because the sibling doesn't exist on disk (EvalSymlinks errors → "" → workspace-escape). Could you add a regression where the sibling target does exist and a broad /** allow rule is in place, and make sure it still denies as workspace-escape? An early-bail in evalEscapedSymlink when rel contains a .. segment (or comparing the pre-EvalSymlinks clean candidate against rootClean) should be enough.

2. gofmt. Current gofmt -l on HEAD cb558f10 still reports:

   • internal/api/core.go
   • internal/fsmonitor/fuse.go

Everything else looks good — the leaf-symlink fix and the policies.symlink_escape knob are wired correctly, CI is green across all targets, and the branch still merges cleanly. Happy to merge once these two are addressed.