Potential fix for code scanning alert no. 5: Uncontrolled data used in path expression #99
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…n path expression Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| # Check that new_path is within MODIFIED_DIR to prevent path traversal or escapes | ||
| resolved_dir = MODIFIED_DIR.resolve() | ||
| resolved_path = new_path.resolve() |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To robustly address the risk, we should:
- Update
safe_filenameinstreamlit_app/helper.pyso it rejects/filters any input that could result in absolute paths or traversal (".." or separators), not just forbidden characters. - In
_csv_path_for, after constructing the path, normalize (resolve) and check that the resulting path is insideMODIFIED_DIR, returningNoneor raising an exception if not. - In the Streamlit UI (in
main.py), before proceeding to rename, ensure the result of_csv_path_foris notNoneor is valid, and display an error if the filename is unsafe.
Concrete implementation:
- Edit
safe_filename: Reject any names containing path separators or "..", return a default safe name. - Edit
_csv_path_for: After constructing the path, normalize with.resolve()and check if path is withinMODIFIED_DIR, otherwise signal unsafe. - Edit UI logic in
main.py: Ensure we abort with a clear message if the new filename/path is deemed unsafe by_csv_path_for.
Suggested changeset
2
streamlit_app/main.py
| @@ -668,24 +668,10 @@ | ||
| if new_name == current_name: | ||
| st.toast("Table name unchanged.") | ||
| else: | ||
| new_path = _csv_path_for(new_name,MODIFIED_DIR) | ||
|
|
||
| # Check that new_path is within MODIFIED_DIR to prevent path traversal or escapes | ||
| resolved_dir = MODIFIED_DIR.resolve() | ||
| resolved_path = new_path.resolve() | ||
| try: | ||
| # Python 3.9+ has is_relative_to | ||
| if hasattr(resolved_path, "is_relative_to"): | ||
| if not resolved_path.is_relative_to(resolved_dir): | ||
| st.error("Unsafe file name: resulting path is outside the allowed directory.") | ||
| st.stop() | ||
| else: | ||
| # Fallback for Python <3.9 | ||
| if str(resolved_path).find(str(resolved_dir)) != 0: | ||
| st.error("Unsafe file name: resulting path is outside the allowed directory.") | ||
| st.stop() | ||
| except Exception as check_exc: | ||
| st.error(f"Error verifying target path: {check_exc}") | ||
| new_path = _csv_path_for(new_name,MODIFIED_DIR) | ||
| except ValueError as bad_path_exc: | ||
| st.error(str(bad_path_exc)) | ||
| st.stop() | ||
|
|
||
| # Move/rename the CSV on disk (if it exists). If it doesn't, create from in-memory df. |
streamlit_app/helper.py
Outside changed files
| @@ -173,6 +173,7 @@ | ||
| - Replaces sequences of forbidden characters with underscores. | ||
| - Ensures the base name is non-empty (falls back to 'table'). | ||
| - Ensures `ext` starts with a '.'. | ||
| - Rejects path traversal and separator, always returns a safe basename. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| @@ -186,9 +187,13 @@ | ||
| str | ||
| A sanitized filename like 'my_table.csv'. | ||
| """ | ||
| base = re.sub(r"[^A-Za-z0-9_.-]+", "_", name).strip("._") | ||
| if not base: | ||
| # Reject path traversal or absolute path attempts | ||
| if os.path.isabs(name) or ".." in name or "/" in name or "\\" in name: | ||
| base = "table" | ||
| else: | ||
| base = re.sub(r"[^A-Za-z0-9_.-]+", "_", name).strip("._") | ||
| if not base: | ||
| base = "table" | ||
|
|
||
| # Normalize extension: ensure a single leading dot if ext was provided | ||
| ext = ext.strip() | ||
| @@ -720,8 +724,20 @@ | ||
| """ | ||
| Return the output CSV path for a given table name within MODIFIED_DIR. | ||
| Requires `MODIFIED_DIR` and `safe_filename` to be defined globally. | ||
| Always guarantees containment within MODIFIED_DIR or raises ValueError. | ||
| """ | ||
| return MODIFIED_DIR / safe_filename(name, ".csv") # type: ignore[name-defined] | ||
| filename = safe_filename(name, ".csv") | ||
| path = MODIFIED_DIR / filename | ||
| resolved_dir = MODIFIED_DIR.resolve() | ||
| resolved_path = path.resolve() | ||
| # Python 3.9+ supports is_relative_to | ||
| if hasattr(resolved_path, "is_relative_to"): | ||
| if not resolved_path.is_relative_to(resolved_dir): | ||
| raise ValueError("Unsafe file name: resulting path is outside the allowed directory.") | ||
| else: | ||
| if str(resolved_path).find(str(resolved_dir)) != 0: | ||
| raise ValueError("Unsafe file name: resulting path is outside the allowed directory.") | ||
| return path | ||
|
|
||
|
|
||
| # ------------------------------ UI callbacks ------------------------------ |
Copilot is powered by AI and may make mistakes. Always verify output.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Potential fix for https://github.com/Intugle/data-tools/security/code-scanning/5
To robustly defend against any path traversal or escaping, after constructing the new CSV path, normalize and check that it still resides within the intended
MODIFIED_DIR. This means:_csv_path_for(new_name, MODIFIED_DIR)producesnew_path, resolve it to its absolute form (using.resolve()from pathlib).MODIFIED_DIRto its absolute path.replace()or writing), confirm thatnew_pathis a child ofMODIFIED_DIRviais_relative_to(Python 3.9+) or using a manual fallback.new_path, before any filesystem modification.No new methods or imports are required as
Path.resolve()and (if available)Path.is_relative_to()are standard, but for broad compatibility (<3.9) we can use a try/except or manual check.Edit region:
streamlit_app/main.pyJust after
new_path = _csv_path_for(new_name, MODIFIED_DIR)(line 671, in the RENAME TABLE button block).Suggested fixes powered by Copilot Autofix. Review carefully before merging.