fix: drop deprecated import #16
Conversation
https://github.com/OpenVoiceOS/ovos_skill_installer has been archived for a LONG time
|
Caution Review failedThe pull request is closed. WalkthroughThe changes involve significant updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Plugin
participant URL
participant FileSystem
User->>Plugin: Request to download file
Plugin->>URL: Fetch file from URL
URL-->>Plugin: Return file data
Plugin->>FileSystem: Save file to specified location
Plugin->>FileSystem: If tar file, extract contents
Plugin->>FileSystem: If zip file, extract contents
FileSystem-->>Plugin: Confirm extraction
Plugin-->>User: Download complete
Possibly related PRs
Warning Rate limit exceeded@github-actions[bot] has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 39 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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: 3
🧹 Outside diff range and nitpick comments (2)
ovos_stt_plugin_vosk/__init__.py (2)
266-269: Simplify conditional assignment with a ternary operatorThe
if-elseblock can be simplified using a ternary operator for readability and conciseness.Apply this diff to simplify the code:
-if session: - content = session.get(url).content -else: - content = requests.get(url).content +content = session.get(url).content if session else requests.get(url).content🧰 Tools
🪛 Ruff
266-269: Use ternary operator
content = session.get(url).content if session else requests.get(url).contentinstead ofif-else-blockReplace
if-else-block withcontent = session.get(url).content if session else requests.get(url).content(SIM108)
279-347: Refactor to eliminate code duplicationThe
download_extract_taranddownload_extract_zipfunctions share similar logic. Consider refactoring to reduce redundancy and improve maintainability.You could create a generic extraction function:
def download_and_extract(url, folder, filename='', archive_type='zip', skill_folder_name=None, session=None): # Common code for downloading ... if archive_type == 'zip': with zipfile.ZipFile(filename, 'r') as archive: # Extraction logic with security checks ... elif archive_type == 'tar': with tarfile.open(filename) as archive: # Extraction logic with security checks ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ovos_stt_plugin_vosk/__init__.py(2 hunks)
🧰 Additional context used
🪛 Ruff
ovos_stt_plugin_vosk/__init__.py
264-264: Use a context manager for opening files
(SIM115)
266-269: Use ternary operator content = session.get(url).content if session else requests.get(url).content instead of if-else-block
Replace if-else-block with content = session.get(url).content if session else requests.get(url).content
(SIM108)
| with tarfile.open(tar_filename) as tar: | ||
| tar.extractall(path=folder) | ||
|
|
There was a problem hiding this comment.
Critical security issue: Prevent path traversal in tar extraction
Using tar.extractall without validation can introduce path traversal vulnerabilities. Malicious tar files may overwrite arbitrary files on the filesystem.
Modify the extraction to safely validate file paths:
with tarfile.open(tar_filename) as tar:
def is_within_directory(directory, target):
abs_directory = os.path.abspath(directory)
abs_target = os.path.abspath(os.path.join(directory, target))
return abs_target.startswith(abs_directory)
def safe_extract(tar, path=".", members=None):
for member in tar.getmembers():
member_path = os.path.join(path, member.name)
if not is_within_directory(path, member_path):
raise Exception("Attempted Path Traversal in Tar File")
tar.extractall(path, members)
safe_extract(tar, path=folder)| with zipfile.ZipFile(zip_filename, 'r') as zip_ref: | ||
| zip_ref.extractall(folder) | ||
|
|
There was a problem hiding this comment.
Critical security issue: Prevent path traversal in zip extraction
Similar to tar extraction, using zip_ref.extractall without validation can lead to path traversal attacks.
Update the extraction logic to validate file paths:
with zipfile.ZipFile(zip_filename, 'r') as zip_ref:
for member in zip_ref.namelist():
member_path = os.path.join(folder, member)
if not os.path.abspath(member_path).startswith(os.path.abspath(folder)):
raise Exception("Attempted Path Traversal in Zip File")
zip_ref.extractall(folder)| """ | ||
|
|
||
| if isinstance(file, str): | ||
| file = open(file, 'wb') |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a context manager when opening files
To ensure files are properly closed even if an error occurs, it's recommended to use a context manager when opening files.
Apply this diff to use a context manager:
-if isinstance(file, str):
- file = open(file, 'wb')
+if isinstance(file, str):
+ with open(file, 'wb') as f:
+ if session:
+ content = session.get(url).content
+ else:
+ content = requests.get(url).content
+ f.write(content)
+ returnAdjust the rest of the code to handle when file is a file-like object or None.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff
264-264: Use a context manager for opening files
(SIM115)
https://github.com/OpenVoiceOS/ovos_skill_installer has been archived for a LONG time
Summary by CodeRabbit
New Features
Chores
ovos_skill_installerdependency from requirements, streamlining the project.