Potential fix for code scanning alert no. 1: Uncontrolled command line#82
Conversation
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses a code scanning security alert by adding input validation to prevent uncontrolled command line injection in the log streaming endpoint. The fix introduces a new validation function for log filenames and applies it before constructing file paths and passing them to subprocess calls.
- Adds
validate_log_filename()function following the same pattern as existingvalidate_container_name()validation - Applies validation to the
stream_logendpoint's non-container branch before file path construction - Uses character whitelisting and path separator rejection to prevent directory traversal and command injection
| if not re.fullmatch(r"[a-zA-Z0-9._-]+", name): | ||
| raise HTTPException(status_code=400, detail="Invalid log filename format") |
There was a problem hiding this comment.
The validation regex allows dots in filenames, which could potentially be used for path traversal via sequences like .. (parent directory). While the os.path.normpath and os.path.commonpath checks on lines 296-298 provide defense in depth, it would be safer to either disallow consecutive dots or add explicit validation to reject names containing .. sequences before the path construction step. This would provide earlier detection of malicious input.
Potential fix for https://github.com/karam-ajaj/atlas/security/code-scanning/1
In general, to fix uncontrolled command line usage, any user-provided value that influences a command should be either (a) selected from a hard-coded allowlist or (b) passed through strict validation that limits it to safe characters and reasonable length, and commands should be invoked with
shell=Falseand argument lists.In this file, the Docker container branch in
stream_logalready usesvalidate_container_namebefore buildingcmd. The remaining untrusted influence is thefilenamepath segment for log files. We should introduce avalidate_log_filenamehelper that mirrors the container name validation but is tailored to filenames, and apply it before constructing thefilepathandcmd. The helper should reject empty names, overly long names, names with path separators (/or\), and characters outside a conservative set such as letters, digits, dots, underscores, and hyphens. This keeps existing behavior (reading logs from withinLOGS_DIR) while ensuring that only simple file names are accepted; deeper directory traversal remains blocked by the existingos.path.commonpathcheck.Concretely:
validate_log_filename(name: str) -> strnearvalidate_container_namethat raisesHTTPExceptionwith 400 for invalid input.stream_log, inside theelsebranch (non-container:case), callvalidate_log_filename(filename)and use the returned safe value when building the path.tailinvocation) unchanged.All required imports (
HTTPException,re,os,subprocess) are already present; no new imports are necessary.Suggested fixes powered by Copilot Autofix. Review carefully before merging.