Make Plain SQL restore configurable, disabled by default in server mode. #9368 #9398
Conversation
WalkthroughAdds a configurable flag Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Restore UI
participant BE as Backend
participant Val as SQL Validator
participant Notif as Confirmation Dialog
User->>UI: Choose Plain restore & Save
UI->>BE: POST /restore (confirmed=false)
BE->>Val: is_safe_sql_file(filepath)
alt File rejected (server mode & feature disabled)
BE-->>UI: block_msg (error)
UI-->>User: Show error
else Unsafe patterns found
BE-->>UI: confirmation_msg
UI->>Notif: Show confirmation dialog
alt User confirms
UI->>BE: POST /restore (confirmed=true)
BE-->>UI: Job started response
UI-->>User: Restore started
else User cancels
UI-->>User: Operation cancelled
end
else File safe
BE-->>UI: Job started response
UI-->>User: Restore started
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/pgadmin/static/js/UtilityView.jsx (1)
91-94: Fix self-referentialitemNodeDatainitialization (runtime ReferenceError)
itemNodeDatais used in its own initializer:let itemNodeData = extraData?.itemNodeData ? itemNodeData: undefined;This will throw
ReferenceError: Cannot access 'itemNodeData' before initializationwheneverextraData.itemNodeDatais truthy.Consider:
- let itemNodeData = extraData?.itemNodeData ? itemNodeData: undefined; + let itemNodeData = extraData?.itemNodeData ? extraData.itemNodeData : undefined;
🧹 Nitpick comments (2)
web/pgadmin/static/js/UtilityView.jsx (1)
95-120: Confirm dialog flow and cancel semantics inonSaveClickThe new
confirmation_msghandling generally looks sound (a second POST/PUT withconfirmed: trueand promise chaining viaresolve(onSaveClick(...))), but two details are worth double-checking:
- Passing
rejectdirectly as the cancel callback means a user cancel will surface as a rejected promise to the caller. If the higher-level code treats any rejection as an error, that may display error UX for a normal cancel.- If the backend ever returns
confirmation_msgagain even whenconfirmed: trueis sent, this will recurse indefinitely. Assumption seems to be "confirm at most once"; please ensure the server side enforces that.If the intent is to treat cancel as a silent no-op, wrap
rejectto return a benign value or a specific cancellation error the caller can distinguish.docs/en_US/restore_dialog.rst (1)
28-34: Doc note aligns with new config behavior; optional clarificationThe note that Plain restore is disabled by default in server mode and enabled by setting
ENABLE_PLAIN_SQL_RESTOREtoTruematches the new config and evaluation logic.Optionally, you might add that the Plain format is only available when restoring from a database node, to mirror the UI behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
docs/en_US/restore_dialog.rst(1 hunks)web/.editorconfig(1 hunks)web/config.py(1 hunks)web/pgadmin/browser/__init__.py(1 hunks)web/pgadmin/browser/templates/browser/js/utils.js(1 hunks)web/pgadmin/evaluate_config.py(1 hunks)web/pgadmin/static/js/UtilityView.jsx(3 hunks)web/pgadmin/tools/backup/static/js/backup.js(0 hunks)web/pgadmin/tools/maintenance/static/js/maintenance.js(0 hunks)web/pgadmin/tools/restore/__init__.py(5 hunks)web/pgadmin/tools/restore/static/js/restore.js(0 hunks)web/pgadmin/tools/restore/static/js/restore.ui.js(2 hunks)
💤 Files with no reviewable changes (3)
- web/pgadmin/tools/backup/static/js/backup.js
- web/pgadmin/tools/maintenance/static/js/maintenance.js
- web/pgadmin/tools/restore/static/js/restore.js
🧰 Additional context used
🧬 Code graph analysis (3)
web/pgadmin/tools/restore/static/js/restore.ui.js (1)
web/pgadmin/static/js/UtilityView.jsx (2)
pgAdmin(24-24)pgAdmin(80-80)
web/pgadmin/static/js/UtilityView.jsx (2)
web/pgadmin/static/js/SchemaView/SchemaDialogView.jsx (2)
pgAdmin(74-74)onSaveClick(122-147)web/pgadmin/misc/properties/ObjectNodeProperties.jsx (2)
pgAdmin(27-27)onSaveClick(115-135)
web/pgadmin/tools/restore/__init__.py (1)
web/pgadmin/utils/ajax.py (1)
make_json_response(66-84)
🪛 Ruff (0.14.5)
web/pgadmin/tools/restore/__init__.py
440-440: Consider moving this statement to an else block
(TRY300)
442-442: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
444-444: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: run-feature-tests-pg (14)
- GitHub Check: run-feature-tests-pg (18)
- GitHub Check: run-feature-tests-pg (16)
- GitHub Check: run-feature-tests-pg (15)
- GitHub Check: run-feature-tests-pg (17)
- GitHub Check: run-feature-tests-pg (13)
- GitHub Check: run-python-tests-pg (windows-latest, 15)
- GitHub Check: run-python-tests-pg (windows-latest, 17)
- GitHub Check: run-python-tests-pg (windows-latest, 16)
- GitHub Check: run-python-tests-pg (windows-latest, 18)
🔇 Additional comments (10)
web/.editorconfig (1)
22-24: JSX indentation aligned with JS configAdding
indent_size = 2for*.jsxmatches the existing JS style and keeps formatting consistent across React files.web/pgadmin/static/js/UtilityView.jsx (1)
117-119: Consistent rejection withErrorinstancesStandardizing the
catchblocks to always reject with anErrorinstance (err instanceof Error ? err : new Error(gettext('Something went wrong'))) inonSaveClick,getSQLValue, andinitDataimproves downstream error handling (stack traces,instanceof Errorchecks, etc.).No further changes needed here.
Also applies to: 133-135, 177-185
web/pgadmin/browser/__init__.py (1)
512-542: Config flag correctly exposed to JS utils templatePassing
enable_plain_sql_restore=config.ENABLE_PLAIN_SQL_RESTOREintobrowser/js/utils.jsis consistent with howenable_psqland other feature flags are wired and will correctly reflect the evaluated config (desktop vs server) on the frontend.web/pgadmin/evaluate_config.py (1)
129-136: Desktop-mode patch correctly forces plain SQL restore onSetting
config['ENABLE_PLAIN_SQL_RESTORE'] = TruewhenSERVER_MODEis false cleanly enforces “always enabled in Desktop mode”, matching the config comments and documentation, and avoids accidental disabling via local overrides.web/pgadmin/tools/restore/static/js/restore.ui.js (1)
10-14: Plain format correctly gated by frontend feature flagUsing
pgAdmin['enable_plain_sql_restore'] && this.fieldOptions.nodeType == 'database'to decide whether to insert the Plain option ensures:
- Desktop mode (flag true) retains existing behavior.
- Server mode keeps Plain hidden unless explicitly enabled via config.
- Non-database nodes never see Plain.
The
sources/pgadminimport is consistent with other modules that consume global pgAdmin config.Also applies to: 359-375
web/config.py (1)
972-981: New ENABLE_PLAIN_SQL_RESTORE flag is well-documented and safely defaultedIntroducing
ENABLE_PLAIN_SQL_RESTORE = Falsewith clear comments about server vs desktop behavior and potential meta-command risks provides a clean, centralized control point. Combined withevaluate_and_patch_config’s desktop override, this gives a sensible secure-by-default stance in server mode without regressing desktop UX.web/pgadmin/browser/templates/browser/js/utils.js (1)
75-77: Frontend flag wiring for plain SQL restore matches existing patternsDefining
pgAdmin['enable_plain_sql_restore'] = '{{enable_plain_sql_restore}}' == 'True';follows the same pattern as other boolean flags (e.g.enable_psql) and reliably converts the template value into a JS boolean, with a safe default offalseif unset.web/pgadmin/tools/restore/__init__.py (3)
13-13: LGTM!The
configimport is necessary to accessSERVER_MODEandENABLE_PLAIN_SQL_RESTOREflags used in the security validation flow.
449-484: All callers correctly handle the new 4-value return signature.Verification confirms:
- Only one call site exists (line 510 in
create_restore_job)- The call correctly unpacks all 4 return values:
error_msg, utility, args, confirmation_msg- The variable is pre-initialized at line 501 (
confirmation_msg = None), ensuring safe handling in both branches (plain format sets it fromuse_sql_utility(), other formats leave it asNone)- The confirmation message is properly checked and handled at line 519
The function signature change has been fully integrated without issues.
522-526: This code correctly follows pgAdmin's convention for confirmation dialogs—no changes needed.The use of
success=0with adatafield containingconfirmation_msgis pgAdmin's standard pattern for confirmation prompts. The frontend is explicitly designed to recognize and handle this response structure, displaying it as a confirmation dialog rather than an error. The implementation is correct.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
web/pgadmin/tools/restore/__init__.py (1)
441-446: Critical: Fail-open security vulnerability persists in exception handling.The function returns
True(treating the file as safe) whenFileNotFoundErrororPermissionErroroccurs. This is a fail-open security posture that could allow unsafe files to be processed if there's a filesystem issue. The function should fail closed by returningFalse.Additionally, static analysis suggests using
logging.exceptioninstead oflogging.errorto include stack traces.Apply this diff to fix the security vulnerability:
return True except FileNotFoundError: - current_app.logger.error("File not found.") + current_app.logger.exception(f"Security Alert: File {path} not found.") + return False except PermissionError: - current_app.logger.error("Insufficient permissions to access.") - - return True + current_app.logger.exception(f"Security Alert: Insufficient permissions to access {path}.") + return False
🧹 Nitpick comments (2)
web/pgadmin/static/js/UtilityView.jsx (1)
103-116: Potential unhandled rejection on user cancel.When the user cancels the confirmation dialog (line 110),
rejectis called without an argument. This may leave the caller without a meaningful error object, and the promise rejection might not be properly caught downstream, potentially causing unhandled promise rejection warnings.Consider providing an explicit error or handling cancellation gracefully:
pgAdmin.Browser.notifier.confirm( gettext('Warning'), res.data.data.confirmation_msg, function() { resolve(onSaveClick(isNew, {...data, confirmed: true})); }, - reject + function() { + reject(new Error(gettext('Operation cancelled by user.'))); + } );web/pgadmin/tools/restore/__init__.py (1)
388-440: Consider restructuring the try-except for clarity.The static analysis tool suggests moving the
return Trueat line 440 to anelseblock for better code clarity. While the current logic works, the suggested structure makes the success path more explicit.for i, line in enumerate(normalized_text.split("\n"), 1): stripped = line.strip() # ... checks ... if "\\g" in line or "\\c" in line or "\\!" in line: # ... return False - return True except FileNotFoundError: # ... except PermissionError: # ... + else: + return TrueNote: This refactor only makes sense after fixing the exception handling to return
Falseon errors.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
docs/en_US/restore_dialog.rst(1 hunks)web/.editorconfig(1 hunks)web/config.py(1 hunks)web/pgadmin/browser/__init__.py(1 hunks)web/pgadmin/browser/templates/browser/js/utils.js(1 hunks)web/pgadmin/evaluate_config.py(1 hunks)web/pgadmin/static/js/UtilityView.jsx(3 hunks)web/pgadmin/tools/backup/static/js/backup.js(0 hunks)web/pgadmin/tools/maintenance/static/js/maintenance.js(0 hunks)web/pgadmin/tools/restore/__init__.py(5 hunks)web/pgadmin/tools/restore/static/js/restore.js(0 hunks)web/pgadmin/tools/restore/static/js/restore.ui.js(2 hunks)
💤 Files with no reviewable changes (3)
- web/pgadmin/tools/restore/static/js/restore.js
- web/pgadmin/tools/maintenance/static/js/maintenance.js
- web/pgadmin/tools/backup/static/js/backup.js
✅ Files skipped from review due to trivial changes (1)
- web/.editorconfig
🚧 Files skipped from review as they are similar to previous changes (5)
- web/pgadmin/tools/restore/static/js/restore.ui.js
- web/pgadmin/browser/init.py
- web/config.py
- web/pgadmin/browser/templates/browser/js/utils.js
- web/pgadmin/evaluate_config.py
🧰 Additional context used
🧬 Code graph analysis (1)
web/pgadmin/static/js/UtilityView.jsx (2)
web/pgadmin/static/js/SchemaView/SchemaDialogView.jsx (2)
pgAdmin(74-74)onSaveClick(122-147)web/pgadmin/misc/properties/ObjectNodeProperties.jsx (2)
pgAdmin(27-27)onSaveClick(115-135)
🪛 Ruff (0.14.5)
web/pgadmin/tools/restore/__init__.py
440-440: Consider moving this statement to an else block
(TRY300)
442-442: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
444-444: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: run-feature-tests-pg (13)
- GitHub Check: run-feature-tests-pg (15)
- GitHub Check: run-feature-tests-pg (17)
- GitHub Check: run-feature-tests-pg (18)
- GitHub Check: run-feature-tests-pg (14)
- GitHub Check: run-feature-tests-pg (16)
🔇 Additional comments (4)
web/pgadmin/static/js/UtilityView.jsx (1)
117-119: LGTM!The error handling standardization ensures consistent Error object wrapping across all catch blocks, matching the pattern used elsewhere in the codebase.
web/pgadmin/tools/restore/__init__.py (2)
462-475: LGTM on the gating logic.The server mode gating and confirmation flow is well-structured:
- Server mode with disabled flag returns immediately with appropriate message
- Unsafe files in server mode are blocked; in desktop mode, users get a confirmation prompt
- The
confirmedflag correctly skips re-checking once user has acknowledged
522-526: The review comment references code that does not exist in the current codebase.After thorough verification:
Lines 522-526 don't exist:
web/pgadmin/tools/restore/__init__.pycontains only 521 lines. The current code at lines 515-535 shows error handling witherrormsg, notconfirmation_msg.No 4-tuple unpacking: The current
use_sql_utility()function (line 407) returns exactly 3 values:(error_msg, utility, args), not 4 as claimed in line 509-511.No frontend confirmation_msg handling: A comprehensive search of the codebase finds zero references to
confirmation_msganywhere—neither inUtilityView.jsxnor any other frontend file. The claimed check atUtilityView.jsx:103does not exist.The review comment appears to be based on proposed code changes that are not yet present in the repository, or it references an incorrect branch/version of the code.
Likely an incorrect or invalid review comment.
docs/en_US/restore_dialog.rst (1)
30-34: LGTM on documentation update.The documentation clearly explains that Plain SQL restore is disabled by default in server mode and provides the configuration setting name to enable it. This aligns with the code changes.
Consider adding a reference to where the configuration file is located (e.g.,
config_local.py) for users unfamiliar with pgAdmin configuration.
1 participant
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Behavior Changes
✏️ Tip: You can customize this high-level summary in your review settings.