Ensure that the plain SQL file contains no unsafe or destructive operations before restoring it. #9366
Conversation
…ations before restoring it.
WalkthroughReplace chunked, regex-based meta-command detection with a comprehensive, security-focused file validation function that enforces UTF-8 encoding with BOM handling, rejects null bytes, normalizes line endings, and performs line-by-line inspection for dangerous backslash sequences. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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)
web/pgadmin/tools/restore/__init__.py (1)
381-437: Tighten scan behaviour and clarify what is actually enforcedA couple of points in the scan logic are worth adjusting:
- The block under “Check 2” (
if "\\g" in line or "\\c" in line or "\\!" in line:) currently just executespass, so it has no effect despite the comment about blocking dangerous trailing commands. Either implement a concrete action (e.g., log +return False) or remove the branch/comment to avoid future confusion about what’s enforced.- Loading the entire file into memory (
raw_data = f.read(), Line 389) is simple but may be problematic for very large plain SQL dumps. If users routinely restore multi-GB plain files, consider reintroducing a chunked/streaming scan to keep memory usage bounded.- Enforcing UTF‑8 only via
utf-8-sig(Lines 392–397) is a stricter policy than before and will block valid dumps in other encodings. Please confirm this behaviour is acceptable for your user base and documented, since it can be a visible compatibility change.These are not correctness bugs, but tightening them would make the safety check clearer and more robust.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/pgadmin/tools/restore/__init__.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.4)
web/pgadmin/tools/restore/__init__.py
439-439: Consider moving this statement to an else block
(TRY300)
441-441: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
443-443: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (1)
web/pgadmin/tools/restore/__init__.py (1)
449-455: Safety gate inuse_sql_utilitylooks correctThe new
is_safe_sql_fileguard inuse_sql_utilitycorrectly short-circuits before constructing arguments for thesqlutility and returns a clear, user-facing error message that covers both meta-commands and other unsafe conditions. No changes needed here.
| return True | ||
| except FileNotFoundError: | ||
| current_app.logger.error("File not found.") | ||
| except PermissionError: | ||
| current_app.logger.error("Insufficient permissions to access.") | ||
|
|
||
| return False | ||
| return True |
There was a problem hiding this comment.
Treat unreadable SQL files as unsafe and improve exception logging
Right now, if opening/reading the file raises FileNotFoundError or PermissionError, is_safe_sql_file logs the error but then returns True via the final return True (Line 445), so use_sql_utility will proceed as if the file were safe. For a “security-first” gate, any inability to fully inspect the file should be treated as unsafe and block restore, and the exceptions are good candidates for logger.exception as Ruff suggests.
Consider something along these lines:
- return True
- except FileNotFoundError:
- current_app.logger.error("File not found.")
- except PermissionError:
- current_app.logger.error("Insufficient permissions to access.")
-
- return True
+ return True
+ except FileNotFoundError:
+ current_app.logger.exception(
+ "Security Alert: File not found while scanning %s", path
+ )
+ except PermissionError:
+ current_app.logger.exception(
+ "Security Alert: Insufficient permissions to access %s", path
+ )
+
+ # If we couldn't fully inspect the file, treat it as unsafe.
+ return FalseThis both aligns with the “restore blocked” behaviour and addresses the TRY300/TRY400 hints.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return True | |
| except FileNotFoundError: | |
| current_app.logger.error("File not found.") | |
| except PermissionError: | |
| current_app.logger.error("Insufficient permissions to access.") | |
| return False | |
| return True | |
| return True | |
| except FileNotFoundError: | |
| current_app.logger.exception( | |
| "Security Alert: File not found while scanning %s", path | |
| ) | |
| except PermissionError: | |
| current_app.logger.exception( | |
| "Security Alert: Insufficient permissions to access %s", path | |
| ) | |
| # If we couldn't fully inspect the file, treat it as unsafe. | |
| return False |
🧰 Tools
🪛 Ruff (0.14.4)
439-439: Consider moving this statement to an else block
(TRY300)
441-441: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
443-443: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🤖 Prompt for AI Agents
In web/pgadmin/tools/restore/__init__.py around lines 439 to 445, the function
currently logs FileNotFoundError and PermissionError but then falls through to
return True; change this so that any FileNotFoundError or PermissionError is
treated as unsafe by logging the full exception (use
current_app.logger.exception or logger.exception) and returning False
immediately for those exceptions, ensuring the function blocks restore on
unreadable files and removes the final unconditional return True that allows
unsafe files through.
1 participant
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor