Potential fix for code scanning alert no. 142: Uncontrolled data used in path expression#630
Potential fix for code scanning alert no. 142: Uncontrolled data used in path expression#630schneidergithub wants to merge 3 commits intomainfrom
Conversation
… in path expression If running locally as root or super-user (your own computer), this is not a big deal. However, on restricted computers, this is a security risk and considered best practice. Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
📝 WalkthroughWalkthroughReplaces export path handling with a safe-base constrained strategy: input paths are sanitized, resolved under "./exports", required to start with that base, and directories are created. Invalid paths raise HTTP 400. HTTPException is re-raised; other exceptions return HTTP 500. Existing export (remove/copy) logic preserved. Changes
Sequence Diagram(s)(omitted — changes are localized and do not introduce multi-component sequential flows) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
acestep/api/train_api_service.py (1)
183-183: Ensure the base exports directory exists.The
safe_basedirectory (./exports) may not exist. Whileos.makedirson line 183 creates parent directories ofexport_path, ifexportsitself doesn't exist andexport_pathhas no subdirectory component, the operation could fail or behave unexpectedly.🛠️ Proposed fix to ensure base directory exists
safe_base = os.path.abspath("./exports") + os.makedirs(safe_base, exist_ok=True) export_path = export_path.lstrip("/").lstrip("\\") export_path = os.path.abspath(os.path.join(safe_base, export_path))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/api/train_api_service.py` at line 183, The code currently calls os.makedirs(os.path.dirname(export_path) if os.path.dirname(export_path) else ".", exist_ok=True) which doesn't guarantee the base exports directory (safe_base, e.g. "./exports") exists; update the logic to explicitly ensure safe_base exists before creating parent dirs for export_path by calling os.makedirs(safe_base, exist_ok=True) (or equivalent) prior to the existing os.makedirs line, referencing the export_path and safe_base variables so the exports base folder is always created first.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@acestep/api/train_api_service.py`:
- Around line 177-181: The export_path containment check is bypassable because
os.path.join(safe_base, export_path) will treat an absolute export_path as
replacing safe_base; before joining, sanitize the user-supplied export_path (the
variable export_path used in this snippet) by stripping any leading path
separators or drive letters (e.g., remove leading "/" or "\" and on Windows
strip "C:"), then compute export_path = os.path.abspath(os.path.join(safe_base,
sanitized_export_path)) and keep the existing containment check that raises
HTTPException(status_code=400, detail="Invalid export path") if the resolved
path is outside safe_base.
---
Nitpick comments:
In `@acestep/api/train_api_service.py`:
- Line 183: The code currently calls os.makedirs(os.path.dirname(export_path) if
os.path.dirname(export_path) else ".", exist_ok=True) which doesn't guarantee
the base exports directory (safe_base, e.g. "./exports") exists; update the
logic to explicitly ensure safe_base exists before creating parent dirs for
export_path by calling os.makedirs(safe_base, exist_ok=True) (or equivalent)
prior to the existing os.makedirs line, referencing the export_path and
safe_base variables so the exports base folder is always created first.
There was a problem hiding this comment.
Pull request overview
This PR addresses a code scanning alert (#142) for an uncontrolled path expression vulnerability in the export_lora API endpoint by introducing path validation logic to prevent path traversal attacks. The fix adds a safe base directory constraint and validates that user-supplied paths cannot escape this base using absolute paths or .. traversals.
Changes:
- Added path validation to constrain
export_pathto a./exportsbase directory - Added exception handling to re-raise HTTP exceptions while catching other errors
First commit was too focused, this will fix another alert too. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
good catch Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if not export_path.startswith(safe_base + os.sep) and export_path != safe_base: | ||
| raise HTTPException(status_code=400, detail="Invalid export path") | ||
|
|
||
| os.makedirs(os.path.dirname(export_path), exist_ok=True) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
In general, to fix uncontrolled-path issues, normalize the path relative to a trusted base directory and then verify that the normalized path is still within that base. Use os.path.abspath/os.path.normpath or os.path.realpath, and compare using robust directory containment checks (e.g., os.path.commonpath) instead of plain string-prefix checks, which can be error‑prone (e.g., /exports2 vs /exports).
In this specific function (export_lora in acestep/api/train_api_service.py), we should: (1) keep treating ./exports as the only allowed root; (2) normalize the user-provided request.export_path relative to safe_base; (3) verify containment using os.path.commonpath([safe_base, export_path]) == safe_base; and (4) optionally handle the case where export_path is empty or only whitespace. After we have a validated export_path, we can safely call os.path.dirname(export_path) and the write operations (os.makedirs, shutil.copytree) without risking writes outside safe_base. We can implement this entirely within the shown snippet: no new imports are needed, as os is already imported, and we only adjust the logic around lines 175–183 to strengthen validation while leaving the external behavior (export under ./exports/...) the same.
Concretely:
- In the
tryblock ofexport_lora, replace the current sanitization code (lines 176–183) with a more robust version:- Strip and check that
request.export_pathis non-empty. - Strip leading slashes/backslashes.
- Build
export_pathwithos.path.join(safe_base, user_path)and normalize withos.path.abspath. - Use
os.path.commonpath([safe_base, export_path])to ensureexport_pathstays undersafe_base.
- Strip and check that
- Keep the rest of the logic (creating parent directory, removing existing tree, copying) unchanged.
This hardens the path validation and should eliminate the CodeQL alert because all sinks now consume a value that has passed a strict containment check.
| @@ -173,20 +173,27 @@ | ||
| raise HTTPException(status_code=404, detail=f"No trained model found in {request.lora_output_dir}") | ||
|
|
||
| try: | ||
| export_path = request.export_path.strip() | ||
| raw_export_path = (request.export_path or "").strip() | ||
| if not raw_export_path: | ||
| raise HTTPException(status_code=400, detail="Export path must not be empty") | ||
|
|
||
| # Constrain export_path to a safe base directory and prevent path traversal | ||
| safe_base = os.path.abspath("./exports") | ||
| # Strip leading separators to prevent os.path.join from treating input as absolute | ||
| export_path = export_path.lstrip("/").lstrip("\\") | ||
| export_path = os.path.abspath(os.path.join(safe_base, export_path)) | ||
| if not export_path.startswith(safe_base + os.sep) and export_path != safe_base: | ||
| sanitized_relative = raw_export_path.lstrip("/").lstrip("\\") | ||
| export_path = os.path.abspath(os.path.join(safe_base, sanitized_relative)) | ||
|
|
||
| # Ensure the final path is within the safe base directory | ||
| if os.path.commonpath([safe_base, export_path]) != safe_base: | ||
| raise HTTPException(status_code=400, detail="Invalid export path") | ||
|
|
||
| os.makedirs(os.path.dirname(export_path), exist_ok=True) | ||
| if os.path.exists(export_path): | ||
| shutil.rmtree(export_path) | ||
| shutil.copytree(source_path, export_path) | ||
| return wrap_response({"message": "LoRA exported successfully", "export_path": export_path, "source": source_path}) | ||
| return wrap_response( | ||
| {"message": "LoRA exported successfully", "export_path": export_path, "source": source_path} | ||
| ) | ||
| except HTTPException: | ||
| # Re-raise HTTP exceptions to preserve intended status codes | ||
| raise |
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@acestep/api/train_api_service.py`:
- Around line 176-181: The current check still allows absolute paths because
os.path.join(safe_base, export_path) is bypassed when request.export_path is
absolute; fix by normalizing and forcing export_path to be relative before
joining: take request.export_path, strip any leading path separators (e.g.,
export_path = request.export_path.lstrip(os.sep) and on Windows also
lstrip("\\/") or use a cross-platform lstrip for both slashes), then
os.path.normpath that result, join with safe_base, and validate as before (using
safe_base and export_path variables and raising HTTPException on failure).
Ensure you update the code paths that set export_path and preserve the existing
safe_base and HTTPException usage.
|
@ChuxiJ - I am converting this to a draft. The auto-suggested fix from the security scanner is not understanding the full architecture. I haven't tested this fix out, but suspect it could break based on whether it's a windows or mac/linux machine. Regardless, these security warnings are typically meant in stricter work environments and less concerning on personal computers. Up to you if this security issue is a high priority. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
acestep/api/train_api_service.py (1)
158-160:⚠️ Potential issue | 🟡 MinorAdd return type + expand the docstring to include inputs/outputs/exceptions.
The function body was modified, so please align the docstring and type hints with the repo’s Python guidelines.
🔧 Suggested update
- async def export_lora(request: ExportLoRARequest, _: None = Depends(verify_api_key)): - """Export trained LoRA/LoKr weights.""" + async def export_lora( + request: ExportLoRARequest, _: None = Depends(verify_api_key) + ) -> Dict[str, Any]: + """Export trained LoRA/LoKr weights. + + Args: + request: Export parameters including `export_path` and `lora_output_dir`. + Returns: + Response payload with export metadata. + Raises: + HTTPException: If no model is found or the export path is invalid. + """As per coding guidelines: "Docstrings must be concise and include purpose plus key inputs/outputs (and raised exceptions when relevant)" and "Add type hints for new/modified functions when practical in Python."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acestep/api/train_api_service.py` around lines 158 - 160, The export_lora endpoint's signature and docstring are missing a return type and detailed docstring; update the async def export_lora(request: ExportLoRARequest, _: None = Depends(verify_api_key)) to include an explicit return type (e.g., -> Response or the specific pydantic/fastapi response model used) and expand its docstring to briefly describe purpose, parameters (request: ExportLoRARequest, dependency: verify_api_key), the returned value (type and shape), and any exceptions/errors that can be raised (e.g., HTTPException for auth or export failures); ensure the docstring follows module style (concise purpose + inputs/outputs/exceptions) and keep the function name export_lora and request type ExportLoRARequest as references.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@acestep/api/train_api_service.py`:
- Around line 158-160: The export_lora endpoint's signature and docstring are
missing a return type and detailed docstring; update the async def
export_lora(request: ExportLoRARequest, _: None = Depends(verify_api_key)) to
include an explicit return type (e.g., -> Response or the specific
pydantic/fastapi response model used) and expand its docstring to briefly
describe purpose, parameters (request: ExportLoRARequest, dependency:
verify_api_key), the returned value (type and shape), and any exceptions/errors
that can be raised (e.g., HTTPException for auth or export failures); ensure the
docstring follows module style (concise purpose + inputs/outputs/exceptions) and
keep the function name export_lora and request type ExportLoRARequest as
references.
Thanks for pointing this out. |
|
Closing PR and dismissing security alerts. Everyone is aware of the issue and planning for a fix in the future. |
2 participants
Potential fix for https://github.com/ace-step/ACE-Step-1.5/security/code-scanning/142
In general, paths derived from untrusted input should either be (a) normalized and checked to ensure they reside under a trusted root directory, or (b) reduced to safe filenames (e.g., using
secure_filename) and combined with a trusted root. For an export endpoint like this, the safest design is to constrain exports to a configured base directory, then ensure the user-supplied portion cannot escape that base (no absolute paths, no..up-traversals).For this specific function, the least invasive fix while preserving behavior is:
./exportsor whatever is appropriate for this service).export_pathand join it to that base directory.export_pathfor all filesystem operations (os.makedirs,shutil.rmtree,shutil.copytree).Concretely, within
acestep/api/train_api_service.py:export_path = request.export_path.strip(), compute a safe base likesafe_base = os.path.abspath("./exports")(or another sensible root), then computeexport_path = os.path.abspath(os.path.join(safe_base, export_path)).if not export_path.startswith(safe_base + os.sep):(or equivalent robust prefix check) and raiseHTTPException(status_code=400, detail="Invalid export path")if the check fails.source_path) unchanged, but now using the validatedexport_path.This adds only local logic in the existing function, uses already-imported
os, and does not change the API surface beyond rejecting malicious paths.Suggested fixes powered by Copilot Autofix. Review carefully before merging.
Summary by CodeRabbit