-
Notifications
You must be signed in to change notification settings - Fork 7.7k
fix: re-validate interpolated output_file against untrusted inputs #6447
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,7 @@ | |
| import inspect | ||
| import json | ||
| import logging | ||
| from pathlib import Path | ||
| from pathlib import Path, PurePosixPath, PureWindowsPath | ||
| import threading | ||
| from typing import ( | ||
| Annotated, | ||
|
|
@@ -504,6 +504,29 @@ def output_file_validation(cls, value: str | None) -> str | None: | |
| if value is None: | ||
| return None | ||
|
|
||
| if "{" in value or "}" in value: | ||
| template_vars = [part.split("}")[0] for part in value.split("{")[1:]] | ||
| for var in template_vars: | ||
| if not var.isidentifier(): | ||
| raise ValueError(f"Invalid template variable name: {var}") | ||
| # Literal portions are still checked here; the fully interpolated | ||
| # path is re-validated at runtime (see | ||
| # interpolate_inputs_and_add_conversation_history) because template | ||
| # variables may be filled from untrusted kickoff inputs. | ||
| cls._sanitize_output_file_path(value) | ||
| return value | ||
|
|
||
| return cls._sanitize_output_file_path(value) | ||
|
|
||
| @staticmethod | ||
| def _sanitize_output_file_path(value: str) -> str: | ||
| """Enforce path-safety on an ``output_file`` value. | ||
|
|
||
| Shared by the field validator's literal and template branches. Rejects | ||
| traversal sequences, shell expansion, and shell metacharacters, and | ||
| strips a leading ``/`` so a literal path stays relative to the working | ||
| directory. | ||
| """ | ||
| if ".." in value: | ||
| raise ValueError( | ||
| "Path traversal attempts are not allowed in output_file paths" | ||
|
|
@@ -519,17 +542,56 @@ def output_file_validation(cls, value: str | None) -> str | None: | |
| "Shell special characters are not allowed in output_file paths" | ||
| ) | ||
|
|
||
| if "{" in value or "}" in value: | ||
| template_vars = [part.split("}")[0] for part in value.split("{")[1:]] | ||
| for var in template_vars: | ||
| if not var.isidentifier(): | ||
| raise ValueError(f"Invalid template variable name: {var}") | ||
| return value | ||
|
|
||
| if value.startswith("/"): | ||
| return value[1:] | ||
| return value | ||
|
|
||
| def _validate_output_file_input_values( | ||
| self, inputs: dict[str, str | int | float | dict[str, Any] | list[Any]] | ||
| ) -> None: | ||
| """Reject untrusted input values that would escape the output path. | ||
|
|
||
| Only the variables that actually appear in the ``output_file`` template | ||
| are checked. The developer-authored template is trusted (it may contain | ||
| an absolute base directory), but a value substituted into it must not | ||
| introduce path traversal (``..``), an absolute path, a home/variable | ||
| expansion (``~``/``$``), or shell metacharacters that would redirect the | ||
| write outside the intended location. | ||
| """ | ||
| if not self._original_output_file: | ||
| return | ||
|
|
||
| template_vars = [ | ||
| part.split("}")[0] for part in self._original_output_file.split("{")[1:] | ||
| ] | ||
| for var in template_vars: | ||
| if var not in inputs: | ||
| continue | ||
| value = str(inputs[var]) | ||
| if ".." in value: | ||
| raise ValueError( | ||
| f"Invalid value for output_file variable '{var}': path " | ||
| "traversal sequences ('..') are not allowed" | ||
| ) | ||
| if value.startswith(("~", "$")): | ||
| raise ValueError( | ||
| f"Invalid value for output_file variable '{var}': shell " | ||
| "expansion characters are not allowed" | ||
| ) | ||
| if any(char in value for char in ["|", ">", "<", "&", ";"]): | ||
| raise ValueError( | ||
| f"Invalid value for output_file variable '{var}': shell " | ||
| "special characters are not allowed" | ||
| ) | ||
| if ( | ||
| PurePosixPath(value).is_absolute() | ||
| or PureWindowsPath(value).is_absolute() | ||
| ): | ||
|
Comment on lines
+586
to
+589
|
||
| raise ValueError( | ||
| f"Invalid value for output_file variable '{var}': absolute " | ||
| "paths are not allowed" | ||
| ) | ||
|
|
||
|
Comment on lines
+549
to
+594
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Locate the target file and inspect the relevant range with line numbers.
git ls-files 'lib/crewai/src/crewai/task.py'
wc -l lib/crewai/src/crewai/task.py
sed -n '480,620p' lib/crewai/src/crewai/task.py | cat -n
# Also inspect any nearby references to output_file validation/commentary.
rg -n "output_file|validate_output_file|fully interpolated|re-validated" lib/crewai/src/crewai/task.pyRepository: crewAIInc/crewAI Length of output: 9365 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '1084,1110p' lib/crewai/src/crewai/task.py | cat -n
printf '\n---\n'
sed -n '1258,1285p' lib/crewai/src/crewai/task.py | cat -nRepository: crewAIInc/crewAI Length of output: 2947 Validate the interpolated 🤖 Prompt for AI Agents |
||
| @model_validator(mode="after") | ||
| def set_attributes_based_on_config(self) -> Task: | ||
| """Set attributes based on the agent configuration.""" | ||
|
|
@@ -1026,6 +1088,12 @@ def interpolate_inputs_and_add_conversation_history( | |
| raise ValueError(f"Error interpolating expected_output: {e!s}") from e | ||
|
|
||
| if self.output_file is not None: | ||
| # Values interpolated into the output path may come from untrusted | ||
| # kickoff inputs. The developer-authored template (including any | ||
| # absolute base directory) is trusted, but an injected value must | ||
| # not introduce path traversal, an absolute path, or shell | ||
| # expansion that would escape the intended location. | ||
| self._validate_output_file_input_values(inputs) | ||
| try: | ||
| self.output_file = interpolate_only( | ||
| input_string=self._original_output_file, inputs=inputs | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Sync
.pre-commit-config.yamlignore list.The pre-commit hook's own comment states "Keep this ignore list in sync with .github/workflows/vulnerability-scan.yml." but its
--ignore-vulnlist does not yet includePYSEC-2026-597. Without updating it there too, localpre-commitruns will still fail/block on this advisory even though CI now ignores it.🔧 Suggested fix for .pre-commit-config.yaml
--ignore-vuln PYSEC-2026-89 --ignore-vuln PYSEC-2026-97 + --ignore-vuln PYSEC-2026-597 --ignore-vuln PYSEC-2025-148Also applies to: 82-82
🤖 Prompt for AI Agents
Source: Linked repositories