[Refactor/Chore] Validate shellctl job ids at API and service boundaries
- Dominant language
- TypeScript
- Stars
- 156k
- Forks
- 24.6k
- Avg merge
- 22h 9m
- Merged PRs (30d)
- 610
Description
**AI disclosure**: This issue was drafted and analyzed with Codex using GPT-5.4. I have reviewed the analysis, and I am responsible for the content.
## Description
The new agent shell feature is currently intended for trusted users. Under the normal product flow, shellctl job ids are generated by the shellctl server and then tracked by the Dify shell layer. The model/user should not be able to invent arbitrary job ids for `shell_wait`, `shell_input`, or `shell_interrupt` because the layer checks the current session's tracked job list first.
CodeQL currently reports `py/path-injection` alerts around shellctl job-id usage in `dify-agent/src/shellctl/server/tmux.py`, including temporary file creation and cleanup in `send_input()`.
These alerts are likely false positives or low-risk under the current design because:
- `run_job()` allocates job ids server-side with `generate_job_id()`.
- Existing product calls remember job ids only after shellctl returns them.
- The Dify shell layer rejects untracked job ids before calling shellctl commands.
- Shellctl service methods look up the job row before acting on a job id.
Still, shellctl should validate job id shape at its own API/service boundary as defense in depth.
## Motivation
Even trusted-user features benefit from local invariant checks. A strict job id validator would:
- encode the intended job id contract close to the shellctl boundary;
- make future direct shellctl API usage safer;
- reduce noise from CodeQL path-injection alerts;
- avoid relying on every caller to preserve the Dify shell layer's tracked-job invariant.
Recommended scope:
- Add a shared job id pattern matching the generated format, for example the timestamp prefix plus the configured alphabet suffix.
- Validate `job_id` path parameters in shellctl API routes or immediately inside `ShellctlService` / `TmuxController` entrypoints.
- Return a structured 400 error for malformed job ids before touching tmux, temporary files, or artifact paths.
- Add local tests for malformed job ids such as path separators, `..`, absolute-path-looking values, whitespace, and shell metacharacters.
## Original CodeQL Annotations
- Alert 346: `dify-agent/src/shellctl/server/tmux.py:235`
- Code: https://github.com/langgenius/dify/blob/7fabc39e97e4d54787384afdc35f7c021ff8e929/dify-agent/src/shellctl/server/tmux.py#L235
- CodeQL alert: https://github.com/langgenius/dify/security/code-scanning/346
- Title: `Uncontrolled data used in path expression`
- Message: `This path depends on a user-provided value.`
- Alert 344: `dify-agent/src/shellctl/server/tmux.py:283`
- Code: https://github.com/langgenius/dify/blob/7fabc39e97e4d54787384afdc35f7c021ff8e929/dify-agent/src/shellctl/server/tmux.py#L283
- CodeQL alert: https://github.com/langgenius/dify/security/code-scanning/344
- Title: `Uncontrolled data used in path expression`
- Message: `This path depends on a user-provided value.`
## Additional Context
Relevant files in the current PR branch:
- `dify-agent/src/shellctl/shared/runtime.py`
- `dify-agent/src/shellctl/server/api.py`
- `dify-agent/src/shellctl/server/service.py`
- `dify-agent/src/shellctl/server/tmux.py`
- `dify-agent/tests/local/shellctl/test_shellctl_service.py`
This is a hardening issue, not a statement that the current trusted-user new agent flow is exploitable through arbitrary job id path traversal.
Contributor guide
Assessment
This issue has not been assessed yet.