-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix/agent init type hints #1284
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
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 | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -514,18 +514,41 @@ def snapshot( | |||||||||
| return str(output_path) | ||||||||||
|
|
||||||||||
| def _get_output_path(self, agent_name: str = "") -> Path: | ||||||||||
| """Get output path, optionally per-agent.""" | ||||||||||
| """Get output path, optionally per-agent. | ||||||||||
|
|
||||||||||
| Security: Agent names are sanitized to prevent path traversal. | ||||||||||
| """ | ||||||||||
| if not self.multi_agent_files or not agent_name: | ||||||||||
| return self.path | ||||||||||
|
|
||||||||||
| # Create per-agent path | ||||||||||
| stem = self.path.stem | ||||||||||
| suffix = self.path.suffix or (".json" if self.format == "json" else ".txt") | ||||||||||
|
|
||||||||||
| # Sanitize agent name | ||||||||||
| safe_name = re.sub(r'[^\w\-]', '_', agent_name.lower()) | ||||||||||
|
|
||||||||||
| return self.path.parent / f"{stem}_{safe_name}{suffix}" | ||||||||||
| # Sanitize agent name: | ||||||||||
| # 1. Strip any path separators first (defence in depth) | ||||||||||
| safe_name = agent_name.replace('/', '_').replace('\\', '_') | ||||||||||
| # 2. Collapse to safe chars only | ||||||||||
| safe_name = re.sub(r'[^\w\-]', '_', safe_name.lower()) | ||||||||||
| # 3. Enforce length limit to prevent filesystem issues | ||||||||||
| safe_name = safe_name[:64] if safe_name else 'unknown' | ||||||||||
| # 4. Strip leading/trailing underscores / dots | ||||||||||
| safe_name = safe_name.strip('_.') | ||||||||||
| if not safe_name: | ||||||||||
| safe_name = 'unknown' | ||||||||||
|
|
||||||||||
| result = self.path.parent / f"{stem}_{safe_name}{suffix}" | ||||||||||
|
|
||||||||||
| # Verify the resolved path stays within the expected parent dir | ||||||||||
| resolved = result.resolve() | ||||||||||
| parent_resolved = self.path.parent.resolve() | ||||||||||
| if not str(resolved).startswith(str(parent_resolved) + '/') and resolved.parent != parent_resolved: | ||||||||||
|
||||||||||
| if not str(resolved).startswith(str(parent_resolved) + '/') and resolved.parent != parent_resolved: | |
| try: | |
| resolved.relative_to(parent_resolved) | |
| except ValueError: |
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -143,7 +143,15 @@ def safe_execute(): | |||||||||||||
| blocked_attrs = {{ | ||||||||||||||
| '__subclasses__', '__bases__', '__mro__', '__globals__', | ||||||||||||||
| '__code__', '__class__', '__dict__', '__builtins__', | ||||||||||||||
| '__import__', '__loader__', '__spec__' | ||||||||||||||
| '__import__', '__loader__', '__spec__', '__init_subclass__', | ||||||||||||||
| '__set_name__', '__reduce__', '__reduce_ex__', | ||||||||||||||
| '__traceback__', '__qualname__', '__module__', | ||||||||||||||
| '__wrapped__', '__closure__', '__annotations__', | ||||||||||||||
| # Frame/code object introspection | ||||||||||||||
| 'gi_frame', 'gi_code', 'cr_frame', 'cr_code', | ||||||||||||||
| 'ag_frame', 'ag_code', 'tb_frame', 'tb_next', | ||||||||||||||
| 'f_globals', 'f_locals', 'f_builtins', 'f_code', | ||||||||||||||
| 'co_consts', 'co_names', | ||||||||||||||
| }} | ||||||||||||||
|
|
||||||||||||||
| for node in ast.walk(tree): | ||||||||||||||
|
|
@@ -162,7 +170,9 @@ def safe_execute(): | |||||||||||||
| "success": False | ||||||||||||||
| }} | ||||||||||||||
| if isinstance(node, ast.Call) and isinstance(node.func, ast.Name): | ||||||||||||||
| if node.func.id in ('exec', 'eval', 'compile', '__import__', 'open'): | ||||||||||||||
| if node.func.id in ('exec', 'eval', 'compile', '__import__', | ||||||||||||||
| 'open', 'input', 'breakpoint', | ||||||||||||||
| 'setattr', 'delattr', 'dir'): | ||||||||||||||
|
Comment on lines
+173
to
+175
Contributor
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. The
Suggested change
|
||||||||||||||
| return {{ | ||||||||||||||
| "result": None, | ||||||||||||||
| "stdout": "", | ||||||||||||||
|
|
||||||||||||||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { AgentFlow, Task, TaskConfig } from './index'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import * as path from 'path'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export interface YAMLWorkflowDefinition { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| name: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -39,8 +40,9 @@ export interface ParsedWorkflow { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Parse YAML string into workflow definition | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export function parseYAMLWorkflow(yamlContent: string): YAMLWorkflowDefinition { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Simple YAML parser for workflow definitions | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // For production, use js-yaml package | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // SECURITY: If migrating to js-yaml, you MUST use: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // yaml.load(content, { schema: yaml.JSON_SCHEMA }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Never use yaml.load() with DEFAULT_SCHEMA — it enables arbitrary JS execution. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const lines = yamlContent.split('\n'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const result: YAMLWorkflowDefinition = { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| name: '', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -82,7 +84,15 @@ export function parseYAMLWorkflow(yamlContent: string): YAMLWorkflowDefinition { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type: 'agent' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } else if (currentStep) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Step properties | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Step properties — whitelist allowed keys to prevent injection | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const ALLOWED_STEP_KEYS = new Set([ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'type', 'agent', 'tool', 'input', 'output', 'condition', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'onError', 'maxRetries', 'timeout', 'loopCondition', 'maxIterations', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ]); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+88
to
+91
Contributor
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.
Comment on lines
+87
to
+91
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!ALLOWED_STEP_KEYS.has(key)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Ignore unknown keys — do not allow arbitrary property injection | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (key === 'type') currentStep.type = value as any; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else if (key === 'agent') currentStep.agent = value; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else if (key === 'tool') currentStep.tool = value; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -265,9 +275,33 @@ function parseValue(value: string): any { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export async function loadWorkflowFromFile( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| filePath: string, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| agents: Record<string, any> = {}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tools: Record<string, any> = {} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tools: Record<string, any> = {}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| options: { basePath?: string; maxFileSizeBytes?: number } = {} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): Promise<ParsedWorkflow> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const fs = await import('fs/promises'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // SECURITY: Prevent path traversal | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const normalizedPath = path.normalize(filePath); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (normalizedPath.includes('..')) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new Error('Path traversal detected: ".." is not allowed in file paths'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+283
to
+287
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // If basePath is specified, ensure resolvedPath stays within it | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (options.basePath) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const resolvedBase = path.resolve(options.basePath); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const resolvedFile = path.resolve(options.basePath, normalizedPath); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!resolvedFile.startsWith(resolvedBase + path.sep) && resolvedFile !== resolvedBase) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new Error(`File path must be within base directory: ${options.basePath}`); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // SECURITY: Enforce file size limit (default 1 MB) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const maxSize = options.maxFileSizeBytes ?? 1_048_576; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const stat = await fs.stat(filePath); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (stat.size > maxSize) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new Error(`File too large: ${stat.size} bytes exceeds limit of ${maxSize} bytes`); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const content = await fs.readFile(filePath, 'utf-8'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+292
to
305
Contributor
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. There are two issues here:
Comment on lines
+289
to
305
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // If basePath is specified, ensure resolvedPath stays within it | |
| if (options.basePath) { | |
| const resolvedBase = path.resolve(options.basePath); | |
| const resolvedFile = path.resolve(options.basePath, normalizedPath); | |
| if (!resolvedFile.startsWith(resolvedBase + path.sep) && resolvedFile !== resolvedBase) { | |
| throw new Error(`File path must be within base directory: ${options.basePath}`); | |
| } | |
| } | |
| // SECURITY: Enforce file size limit (default 1 MB) | |
| const maxSize = options.maxFileSizeBytes ?? 1_048_576; | |
| const stat = await fs.stat(filePath); | |
| if (stat.size > maxSize) { | |
| throw new Error(`File too large: ${stat.size} bytes exceeds limit of ${maxSize} bytes`); | |
| } | |
| const content = await fs.readFile(filePath, 'utf-8'); | |
| let resolvedPath: string; | |
| // If basePath is specified, ensure resolvedPath stays within it | |
| if (options.basePath) { | |
| const resolvedBase = path.resolve(options.basePath); | |
| resolvedPath = path.resolve(resolvedBase, normalizedPath); | |
| if (!resolvedPath.startsWith(resolvedBase + path.sep) && resolvedPath !== resolvedBase) { | |
| throw new Error(`File path must be within base directory: ${options.basePath}`); | |
| } | |
| } else { | |
| resolvedPath = path.resolve(normalizedPath); | |
| } | |
| // SECURITY: Enforce file size limit (default 1 MB) | |
| const maxSize = options.maxFileSizeBytes ?? 1_048_576; | |
| const stat = await fs.stat(resolvedPath); | |
| if (stat.size > maxSize) { | |
| throw new Error(`File too large: ${stat.size} bytes exceeds limit of ${maxSize} bytes`); | |
| } | |
| const content = await fs.readFile(resolvedPath, 'utf-8'); |
Copilot
AI
Apr 7, 2026
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.
When options.basePath is provided, you validate resolvedFile but then call fs.stat(filePath) and fs.readFile(filePath). This means the code can read a different file than the one you validated (depending on CWD / relative paths). Use the same resolved/validated path for stat + read (and consider resolving even when basePath is not provided for consistency).
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.
1. Basepath check not enforced 🐞 Bug ⛨ Security
In loadWorkflowFromFile(), the basePath guard validates a resolved path (resolvedFile) but then size-checks and reads using the original filePath, so the code may read a different file than the one validated (depending on process CWD). This undermines the intended basePath restriction and can load unintended workflow files.
Agent Prompt
### Issue description
`loadWorkflowFromFile()` validates `resolvedFile` (based on `options.basePath` + `normalizedPath`) but then uses `filePath` for `fs.stat()` and `fs.readFile()`. This means the security check may not apply to the actual file being accessed.
### Issue Context
When `options.basePath` is provided, the function should consistently treat the file to load as `resolvedFile` (and ideally also return/operate on that canonical path).
### Fix Focus Areas
- src/praisonai-ts/src/workflows/yaml-parser.ts[283-307]
### Implementation notes
- Compute a single `const effectivePath = options.basePath ? resolvedFile : normalizedOrAbsolutePath`.
- Use `effectivePath` for `fs.stat()` and `fs.readFile()`.
- Consider also returning/using `effectivePath` for any subsequent logic so logs/errors refer to the canonical path.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -15,9 +15,70 @@ | |||||||||
| from .code_intelligence import CodeIntelligenceRouter | ||||||||||
| from .action_orchestrator import ActionOrchestrator | ||||||||||
|
|
||||||||||
| import os | ||||||||||
| import re | ||||||||||
|
||||||||||
| import re |
Copilot
AI
Apr 7, 2026
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.
_sanitize_filepath annotates workspace as str but defaults it to None. This is an incorrect type hint; update it to Optional[str] (and import Optional) so static type checkers don’t report a mismatch.
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.
The DANGEROUS_PATTERNS blacklist is incomplete and easily bypassed. For example, it checks for '| ' (with a space) but not '|' (without a space), allowing command piping like ls|grep. It also misses critical shell metacharacters like ;, &, and newlines which are checked in _sanitize_filepath but omitted here.
DANGEROUS_PATTERNS = [
'$(', '`', # Command substitution
'&&', '||', # Command chaining
'>>', '>', # Output redirection
'|', ';', '&', # Pipe and separators
'\n', '\r' # Line breaks
]
Copilot
AI
Apr 7, 2026
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.
The workspace escape check uses str(resolved).startswith(str(ws_resolved) + os.sep), which is error-prone across platforms (case-insensitive FS, separator differences) and can be brittle. Prefer resolved.relative_to(ws_resolved) / Path.is_relative_to() (or os.path.commonpath) to enforce containment robustly.
| if not str(resolved).startswith(str(ws_resolved) + os.sep) and resolved != ws_resolved: | |
| try: | |
| resolved.relative_to(ws_resolved) | |
| except ValueError: |
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.
The path validation logic uses string concatenation with a forward slash (
/), which is not cross-platform compatible and will fail on Windows where the path separator is\. Since the project requires Python 3.10+, you should use the idiomaticis_relative_tomethod for a robust, platform-independent check.