-
Notifications
You must be signed in to change notification settings - Fork 1
fix: resolve vulnerabilities #50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
I have read the CLA Document and I hereby sign the CLA 1 out of 2 committers have signed the CLA. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request addresses security vulnerabilities by updating dependencies and improving thread safety in the database layer.
Changes:
- Upgraded urllib3 (2.0.6 → 2.6.3) and Werkzeug (3.0.6 → 3.1.5) to address security vulnerabilities
- Updated PIP (22.2.0 → 25.3.0) and SETUPTOOLS (70.0.0 → 80.9.0) in the Alpine Dockerfile
- Refactored DB class to store connections instead of cursors, added threading locks, and implemented cursor context managers for better resource management
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| requirements.txt | Updated urllib3 and Werkzeug to newer versions to resolve security vulnerabilities |
| alpine/Dockerfile | Updated PIP and SETUPTOOLS versions to their latest releases |
| src/db.py | Enhanced thread safety with locks, changed to connection-based architecture, and added cursor context managers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
||
| class DB: | ||
| _lock = threading.Lock() |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _lock is a class-level attribute shared across all DB instances. This means all database operations across different database files will compete for the same lock, which could cause unnecessary serialization and performance degradation. Consider using an instance-level lock (self._lock = threading.Lock() in init) if each DB instance operates on a separate database file.
| with DB._lock: | ||
| log.debug("SQL command: " + sql.replace('?', '%s') % args) | ||
| cursor.execute(sql, args) |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The threading lock is being used only around the logging and execute operations in __log_and_execute, but this may not provide adequate thread safety. The lock should protect the entire database operation sequence, not just the execute call. Consider that cursor operations might need protection at a higher level, or use connection-level locks instead of a class-level lock since each DB instance has its own connection.
| if login: | ||
| cursor = DB.__create_connection(self.__dbfile).cursor() | ||
| else: | ||
| cursor = self.__cursor | ||
|
|
||
| DB.__log_and_execute(cursor, query, params) | ||
| cursor = self.__conn.cursor() | ||
| with cursor: | ||
| DB.__log_and_execute(cursor, query, params) |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When login=True, a new connection is created but never explicitly closed, which could lead to resource leaks. The cursor context manager only closes the cursor, not the underlying connection. Consider storing the connection and ensuring it's properly closed, or restructure to reuse the existing connection.
| if login: | ||
| cursor = DB.__create_connection(self.__dbfile).cursor() | ||
| else: | ||
| cursor = self.__cursor | ||
|
|
||
| DB.__log_and_execute(cursor, query, params) | ||
| return cursor.fetchall() | ||
| cursor = self.__conn.cursor() | ||
| with cursor: | ||
| DB.__log_and_execute(cursor, query, params) | ||
| return cursor.fetchall() |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When login=True, a new connection is created but never explicitly closed, which could lead to resource leaks. The cursor context manager only closes the cursor, not the underlying connection. Consider storing the connection and ensuring it's properly closed, or restructure to reuse the existing connection.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
I have read the CLA Document and I hereby sign the CLA |
Co-authored-by: borislavr <[email protected]> Co-authored-by: Philipp Bouzid <[email protected]>
Co-authored-by: NetcrackerCLPLCI <[email protected]>
Co-authored-by: borislavr <[email protected]> Co-authored-by: Philipp Bouzid <[email protected]>
…ependencies (#48) Co-authored-by: borislavr <[email protected]>
…racker/qubership-backup-daemon into fix/resolve_vulnerabilities
What type of PR is this? (check all applicable)
Description
TDB
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Please replace this line with instructions on how to test your changes, a note
on the devices and browsers this has been tested on, as well as any relevant
images for UI changes.
Breaking Change checklist
If your PR includes any deployment or processing changes, please utilize this checklist:
Added/updated tests?
have not been included
[optional] Are there any things to highlight or double check?
[optional] What gif best describes this PR or how it makes you feel?