Three safety findings in the Elixir reference implementation (with a fix for one) #107
SEPURI-SAI-KRISHNA
started this conversation in
Show and tell
Replies: 0 comments
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Hi all — I've been reading through and running the Elixir reference implementation, and while doing so I ran into three issues that seem worth surfacing. Two are workspace/path-safety related, one is about the observability API. I've written a fix + regression test for the first and pushed it to my fork; the other two I'm describing here in case they're useful (Issues are disabled, so Discussions seemed like the right place).
All line references are against
main(elixir/…).1.
PathSafety.canonicalize/1recurses unbounded on a symlink cyclelib/symphony_elixir/path_safety.exresolves symlinks recursively with no hop limit. A cyclic symlink makes it recurse forever and hang any caller (workspace path validation, Codex app-server cwd resolution, config validation all go through it).Repro:
Fix: cap symlink traversal at a fixed hop budget (mirroring the OS
MAXSYMLINKSapproach) and return{:error, :symlink_loop}when exhausted, so cyclic links fail fast instead of hanging. Includes a regression test that builds ana <-> bcycle.Fork branch / diff: https://github.com/SEPURI-SAI-KRISHNA/symphony/pull/new/fix/path-safety-symlink-loop
2. Remote workspace paths skip the root-containment check
lib/symphony_elixir/workspace.exvalidates workspace paths differently depending on the target:validate_workspace_path(workspace, nil), lines 403–429) canonicalizes and enforces that the path stays under the configured workspace root.validate_workspace_path(workspace, worker_host), lines 431–443) only rejects empty / newline / NUL — no containment check.Since
remove/2runsrm -rf "$workspace"on the remote host (line 114), a workspace path that escapes the configured root (e.g. via..in the derived path) would delete outside the intended root on the remote worker. The local path is guarded; the remote path isn't symmetric with it.This may be intentional given the trust model ("trusted environments" per the README), but the asymmetry between the local and remote validators looked accidental rather than deliberate.
3.
/api/v1/*observability routes have no auth and skip the browser pipelinelib/symphony_elixir_web/router.ex(lines 31–39) serves the observability API in a scope that does notpipe_through(:browser), so there's no CSRF protection, and there's no auth on the routes.post("/api/v1/refresh", ...)(line 36) is state-changing.This is mitigated in the default config by
server.hostdefaulting to127.0.0.1, so it isn't exposed off-host out of the box. It becomes a concern if an operator binds to a non-loopback interface. Flagging in case a note in the docs or a lightweight guard is wanted for that case.Happy to open fork PRs for #2 and #3 as well if that's useful, or to adjust any of the above if I've misread the intended trust boundaries. Thanks for publishing this — it's been a great codebase to read.
cc @frantic-openai in case this is helpful.
All reactions