fix:gui_caching #147
fix:gui_caching #147
Conversation
handle case where dirs already exist
WalkthroughThe changes in this pull request primarily affect the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ovos_bus_client/apis/gui.py(1 hunks)
🔇 Additional comments (1)
ovos_bus_client/apis/gui.py (1)
Line range hint 469-499: Reintroduce path traversal checks to prevent security vulnerabilities
The removal of path traversal checks in the _resolve_url method may expose the application to security risks. Without proper validation, malicious inputs could lead to unauthorized file access or execution.
It's crucial to sanitize the url input to mitigate path traversal attacks. Reintroduce the normalization and validation of the URL to ensure that only legitimate file paths are processed.
Apply the following changes to enhance security:
+ import urllib.parse
def _resolve_url(self, url: str) -> str:
"""Resolve a URL to a valid file path.
Args:
url (str): URL or file path to resolve
Returns:
str: Resolved absolute file path or original URL
Raises:
ValueError: If url is None or empty
"""
if not url or not isinstance(url, str):
raise ValueError("URL must be a non-empty string")
if url.startswith("http"):
return url
+ # Normalize the URL to prevent path traversal
+ normalized_url = os.path.normpath(url)
+ if normalized_url.startswith(('..', '/')) or os.path.isabs(normalized_url):
+ raise ValueError(f"Invalid URL path: {url}")
+
+ url = normalized_url
- if not os.path.isfile(url):
+ if not os.path.isfile(url):
GUI_CACHE_PATH = get_xdg_cache_save_path('ovos_gui')
# Use os.path.join for path construction
gui_cache = os.path.join(GUI_CACHE_PATH, self.skill_id, url)
if os.path.isfile(gui_cache):
LOG.debug(f"Resolved image: {gui_cache}")
return gui_cache
else:
for framework in self.ui_directories:
gui_cache = os.path.join(GUI_CACHE_PATH, self.skill_id, framework, url)
if os.path.isfile(gui_cache):
LOG.debug(f"Resolved image: {gui_cache}")
return gui_cache
return urlRun the following script to identify any instances where os.path functions are used without proper sanitization:
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #147 +/- ##
==========================================
+ Coverage 39.49% 42.24% +2.75%
==========================================
Files 17 16 -1
Lines 1732 2161 +429
==========================================
+ Hits 684 913 +229
- Misses 1048 1248 +200 ☔ View full report in Codecov by Sentry. |
handle case where dirs already exist
Summary by CodeRabbit
New Features
Bug Fixes