Skip to content
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 66 additions & 24 deletions web/pgadmin/tools/restore/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
"""Implements Restore Utility"""

import json
import re

from flask import render_template, request, current_app, Response
from flask_babel import gettext as _
Expand Down Expand Up @@ -375,42 +374,85 @@ def use_restore_utility(data, manager, server, driver, conn, filepath):
return None, utility, args


def has_meta_commands(path, chunk_size=8 * 1024 * 1024):
def is_safe_sql_file(path):
"""
Quickly detect lines starting with '\' in large SQL files.
Works even when lines cross chunk boundaries.
"""
# Look for start-of-line pattern: beginning or after newline,
# optional spaces, then backslash
pattern = re.compile(br'(^|\n)[ \t]*\\')
Security-first checker for psql meta-commands.

Security Strategy:
1. Strict Encoding: Rejects anything that isn't valid UTF-8.
2. Normalization: Converts all line endings to \n before checking.
3. Null Byte Prevention: Rejects files with binary nulls.
4. Paranoid Regex: Flags any backslash at the start of a line.
"""
try:
with open(path, "rb") as f:
prev_tail = b""
while chunk := f.read(chunk_size):
data = prev_tail + chunk

# Search for pattern
if pattern.search(data):
return True
raw_data = f.read()

# --- SECURITY CHECK 1: Strict Encoding ---
# We force UTF-8. If the file is UTF-16/32, this throws an error,
# and we reject the file. This prevents encoding bypass attacks.
try:
# utf-8-sig handles the BOM automatically (and removes it)
text_data = raw_data.decode("utf-8-sig")
except UnicodeDecodeError:
current_app.logger.warning(f"Security Alert: File {path} is not "
f"valid UTF-8.")
return False

# --- SECURITY CHECK 2: Null Bytes ---
# C-based tools (like psql) can behave unpredictably with null bytes.
if "\0" in text_data:
current_app.logger.warning(f"Security Alert: File {path} contains "
f"null bytes.")
return False

# --- SECURITY CHECK 3: Normalized Line Endings ---
# We normalize all weird line endings (\r, \r\n, Form Feed) to \n
# so we don't have to write a complex regex.
# Note: \x0b (Vertical Tab) and \x0c (Form Feed) are treated as breaks.
normalized_text = text_data.replace("\r\n", "\n").replace(
"\r","\n").replace(
"\f", "\n").replace("\v", "\n")

# --- SECURITY CHECK 4: The Scan ---
# We iterate lines. This is safer than a multiline regex which can
# sometimes encounter buffer limits or backtracking issues.
for i, line in enumerate(normalized_text.split("\n"), 1):
stripped = line.strip()

# Check 1: Meta command at start of line
if stripped.startswith("\\"):
current_app.logger.warning(f"Security Alert: Meta-command "
f"detected on line {i}:{stripped}")
return False

# Check 2 (Optional but Recommended): Dangerous trailing commands
# psql allows `SELECT ... \gexec`. The \ is not at the start.
# If you want to be 100% secure, block ALL backslashes.
# If that is too aggressive, look for specific tokens:
if "\\g" in line or "\\c" in line or "\\!" in line:
# Refine this list based on your tolerance.
# psql parses `\g` even if not at start of line.
# Simplest security rule: No backslashes allowed anywhere.
pass

# Keep a small tail to preserve line boundary context
prev_tail = data[-10:] # keep last few bytes
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
Comment on lines +439 to +445
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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 False

This 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.

Suggested change
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.



def use_sql_utility(data, manager, server, filepath):
# Check the meta commands in file.
if has_meta_commands(filepath):
return _("Restore blocked: the selected PLAIN SQL file contains psql "
"meta-commands (for example \\! or \\i). For safety, "
"pgAdmin does not execute meta-commands from PLAIN restores. "
"Please remove meta-commands."), None, None
# Check the SQL file is safe to process.
if not is_safe_sql_file(filepath):
return _("Restore blocked: The selected PLAIN SQL file either contains"
" meta-commands (such as \\! or \\i) or is considered unsafe "
"to execute on the database server. For security reasons, "
"pgAdmin will not restore this PLAIN SQL file. Please check "
"the logs for more details."), None, None

utility = manager.utility('sql')
ret_val = does_utility_exist(utility)
Expand Down
Loading