fix: don't hard-fail on an undecryptable saved password - #10174
fix: don't hard-fail on an undecryptable saved password#10174kundansable wants to merge 2 commits into
Conversation
Connection._decode_password() let UnicodeDecodeError from decrypt() propagate as a hard error. For OIDC/OAuth2 users whose crypt key can differ between sessions, a password saved under a previous key can never be decoded, and every connection attempt surfaced "Failed to decrypt the saved password" — even after deleting and recreating the pgAdmin user, since the corrupted ciphertext is what's stored, not anything tied to the user record. Catch the decode failure, log a warning, and treat it as "no saved password" instead of propagating the error. connect() then falls through to the normal password prompt, so the account is recoverable instead of permanently stuck. Fixes pgadmin-org#10140
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe psycopg3 connection driver now treats saved-password decryption failures as non-fatal, clears cached password values, logs a warning, and allows the connection flow to request a password again. ChangesSaved password recovery
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 (2)
web/pgadmin/utils/driver/psycopg3/connection.py (2)
261-274: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the recovery contract.
Test invalid ciphertext, valid encrypted passwords, passexec fallback, and an SSH-tunnel server with
save_password=True; assert that the invalid credential is not reused and the response prompts for a password.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/utils/driver/psycopg3/connection.py` around lines 261 - 274, Extend the connection tests covering the saved-password handling around the decryption failure path to cover invalid ciphertext, valid encrypted passwords, passexec fallback, and an SSH-tunnel server configured with save_password=True. Assert that invalid credentials are discarded rather than reused and that the resulting response prompts the user for a password, while preserving successful reuse for valid encrypted passwords and existing passexec behavior.
255-274: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNarrow the exception scope.
except Exceptionconverts unrelated crypto-helper or programming failures into “bad saved password,” logs them as a warning, and continues withNone. Catch the concrete invalid-ciphertext/decryption exceptions plusUnicodeDecodeError, after verifying the repository’sdecrypt()contract, and let unexpected failures surface.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/utils/driver/psycopg3/connection.py` around lines 255 - 274, In the password-decryption block of the connection logic, replace the broad `except Exception` with the concrete invalid-ciphertext/decryption exceptions raised by `decrypt()` plus `UnicodeDecodeError`, verifying the helper’s contract first. Preserve the existing warning and `(False, '', None)` fallback only for those expected bad-password cases, while allowing unrelated failures to propagate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/pgadmin/utils/driver/psycopg3/connection.py`:
- Around line 261-274: Update the invalid saved-password handling in the
connection credential flow to clear the cached ciphertext held by self.password
and return an explicit invalid-password state from the decryption helper. Ensure
connect() and the downstream get_response_for_password/SSH prompt flow consume
that state so passexec fallback is not skipped and the UI reliably prompts for a
new password.
---
Nitpick comments:
In `@web/pgadmin/utils/driver/psycopg3/connection.py`:
- Around line 261-274: Extend the connection tests covering the saved-password
handling around the decryption failure path to cover invalid ciphertext, valid
encrypted passwords, passexec fallback, and an SSH-tunnel server configured with
save_password=True. Assert that invalid credentials are discarded rather than
reused and that the resulting response prompts the user for a password, while
preserving successful reuse for valid encrypted passwords and existing passexec
behavior.
- Around line 255-274: In the password-decryption block of the connection logic,
replace the broad `except Exception` with the concrete
invalid-ciphertext/decryption exceptions raised by `decrypt()` plus
`UnicodeDecodeError`, verifying the helper’s contract first. Preserve the
existing warning and `(False, '', None)` fallback only for those expected
bad-password cases, while allowing unrelated failures to propagate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b6638f40-221e-49e2-a5e5-2c2ff1ab7df6
📒 Files selected for processing (1)
web/pgadmin/utils/driver/psycopg3/connection.py
| # The saved password could not be decrypted. This happens | ||
| # when the stored ciphertext was encrypted with a different | ||
| # key (e.g. OIDC/OAuth2 logins where the derived encryption | ||
| # key changed between sessions), leaving un-decodable bytes | ||
| # (typically a "'utf-8' codec can't decode byte 0x.." error). | ||
| # Instead of failing every connection attempt permanently, | ||
| # discard the bad saved password and continue so the user is | ||
| # prompted for the password again. | ||
| current_app.logger.warning( | ||
| 'Ignoring the saved password as it could not be ' | ||
| 'decrypted. The user will be prompted for the password. ' | ||
| 'Error: {0}'.format(str(e)) | ||
| ) | ||
| return False, '', None |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Propagate the invalid-saved-password state to the prompt flow.
Returning (False, '', None) at Line 274 does not clear the ciphertext or identify that the saved credential was unusable. connect() has already stored it in self.password, and the truthy encpass skips the passexec fallback at Lines 323-334. The supplied downstream handler also passes not server.save_password to get_response_for_password; for this stale record that is false, so the SSH path can return prompt_password=False. Clear the cached ciphertext and propagate an explicit invalid-password state so the UI reliably re-prompts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/pgadmin/utils/driver/psycopg3/connection.py` around lines 261 - 274,
Update the invalid saved-password handling in the connection credential flow to
clear the cached ciphertext held by self.password and return an explicit
invalid-password state from the decryption helper. Ensure connect() and the
downstream get_response_for_password/SSH prompt flow consume that state so
passexec fallback is not skipped and the UI reliably prompts for a new password.
Per CodeRabbit review on PR pgadmin-org#10174: _decode_password() discarded the undecryptable password for the current decode call, but the stale ciphertext was already cached on self.password (set earlier in connect(), before decoding) and never cleared. That meant: - Every subsequent connect() attempt re-decoded and re-warned on the same bad ciphertext instead of the "no saved password" state sticking. - If a server also had passexec_cmd configured, the passexec fallback in connect() (which only runs when neither password nor encpass is set) was skipped, since the stale encpass was still truthy. Clear self.password and manager.password alongside discarding the password in _decode_password's except branch, so the bad ciphertext doesn't linger. The SSH-tunnel prompt path in get_response_for_password() trusting `not server.save_password` (which can be true even for an undecryptable saved password) is a separate, pre-existing issue and is intentionally left out of this narrower fix.
dpage
left a comment
dpage
left a comment
There was a problem hiding this comment.
Thanks for picking this one up. The direction is right: turning an undecryptable saved password into a warning plus the normal prompt path is exactly what the issue asks for, and the change is pep8-clean and nicely contained. I do have a few things I would like addressed before this goes in, plus one observation about the root cause that I think matters for how we close #10140.
Should fix
1. The blanket except Exception is now silent, so real bugs disappear. (connection.py:260) The catch was already broad, but it used logger.exception and surfaced an error, so a TypeError from a bad crypt key, a binascii.Error from a truncated column, or any future breakage in key management stayed visible. After this change all of those become a one-line warning with no traceback, and the user just sees an unexplained password prompt on every connect. Could we narrow the catch to the failures actually being targeted (UnicodeDecodeError, binascii.Error, ValueError) and let anything else keep the old behaviour, or at the very least log with exc_info=True so the traceback survives?
2. Clearing self.password/manager.password does not clear the local encpass in connect(), and that leaks in two ways. (connection.py:274 versus connection.py:310-313 and connection.py:425)
- At the end of
connect(),if status and is_update_password: manager._update_password(encpass)re-caches the bad ciphertext into the manager and into every open connection (server_manager.py:512-517), whenever the connection nonetheless succeeds without a password: trust or peer auth, a.pgpass/passfile, a service definition, or Kerberos. That silently undoes the clearing and re-arms the remaining hard-fail sites in point 4. - The
passexecfallback atconnection.py:319(if not password and not encpass and manager.passexec) is still skipped on this attempt, becauseencpassis unchanged. That is the half of CodeRabbit's open thread which still stands.
Both close cleanly if _decode_password() signals "discard the ciphertext" back to the caller (a fourth return value, or returning encpass alongside password) and connect() then sets encpass = None.
3. The comment overstates what the clearing achieves. (connection.py:270-273) The claim that the ciphertext "isn't silently reused on the next connect() attempt" holds only for the no-argument reconnect paths (_restore_connections()). The main connect route calls manager.update(server) when the server is not connected, and update() unconditionally re-populates self.password = server.password from the config DB (server_manager.py:78), after which the route passes conn_passwd or server.password straight back in (browser/server_groups/servers/__init__.py:1673-1682). The behaviour is still fine, since the user gets prompted either way, but the comment should describe what the clearing really buys.
4. Sibling decrypt sites with the same root cause still hard-fail, so an affected user is only partly unstuck. Once a user's stored ciphertext is undecryptable under the current key they will still hit an uncaught UnicodeDecodeError in ServerManager.export_password_env() (server_manager.py:548, used by Backup, Restore and Import/Export), and Connection._decrypt_password() (connection.py:1464, used by reset()) has the same uncaught .decode(), masked only for as long as the in-memory clearing survives, which per point 3 is not long. The SSH tunnel password path (server_manager.py:565-579) still returns the analogous "Failed to decrypt the SSH tunnel password" hard error. The tunnel path at least deserves the same treatment here, since a key change breaks both passwords identically.
5. For SSH-tunnel servers the prompt this fix relies on may never appear. (credit to CodeRabbit, this half of the thread is valid) On failure the route calls get_response_for_password(server, 401, not server.save_password, ...) (servers/__init__.py:1731-1733), and for a server with save_password = 1 that third argument is False. The non-tunnel branch hardcodes "prompt_password": True (servers/__init__.py:2219) so it is fine, but the use_ssh_tunnel branch passes the argument through (servers/__init__.py:2198), so an SSH-tunnel server with a saved-but-undecryptable password gets a dialog with no server-password field and stays stuck. It is pre-existing, but it defeats the recovery path this PR is built on, so I think it belongs in scope.
On the root cause
The description says the OIDC crypt key "can differ between sessions" without saying why, and I think it is worth recording, because it changes how we should close the issue. Two facts from the code: authenticate/oauth2.py:690 sets session['pass_enc_key'] = session['oauth2_token']['access_token'], so the key protecting every saved server password is the OAuth2 access token, and utils/crypto.py:pad() truncates any key to its first 32 bytes. The rest is inference rather than something I can prove about the reporter's deployment: for a JWT access token those first 32 characters are the base64url-encoded header prefix, constant as long as the header content and field ordering are, which is why it works for most people; any deployment or moment where those bytes vary (a kid appearing early in the header and rotating, a different alg, or an IdP issuing opaque non-JWT tokens) produces a different AES key and permanently undecryptable ciphertext. That fits the reported pattern of a subset of users, no deterministic repro, and survival across user recreation.
So I am happy to take this as the mitigation, but affected users will keep being re-prompted whenever the token prefix shifts, and the real fix is a stable KEK for OAuth2 sessions (MASTER_PASSWORD_HOOK, the OS keyring, or a per-user key wrapped at first login). I would rather we opened a separate issue for the key derivation than closed #10140 as fully solved on the back of this.
Minor
- After the password is discarded the user sees libpq's
fe_sendauth: no password suppliedin the prompt dialog, which is fairly opaque. The route already relayserrmsgto the dialog, so a short explanatory message ("the saved password could not be decrypted, please re-enter it") would make the recovery obvious. - No test accompanies the change.
web/pgadmin/utils/driver/psycopg3/tests/currently holds only__init__.py, so it would be new ground, but a unit test patchingdecryptto raise and asserting_decode_password()returns(False, '', None)whilst clearing both caches would be cheap and would pin down the contract that points 2 and 3 are about. Not a blocker. - Dropping
manager.stop_ssh_tunnel()from this path is correct now that we continue and still need the tunnel; noting it only so it is on the record as deliberate.
2 participants
Summary
Fixes #10140 — some OIDC/OAuth2 users intermittently get a hard
Failed to decrypt the saved passworderror on every connection attempt, and the account stays broken even after being deleted and recreated via OIDC.Root Cause
Connection._decode_password()callsdecrypt()(AES-CFB8, unauthenticated) on the stored password ciphertext and lets anyUnicodeDecodeErrorfrom a failed decrypt propagate as a hard error. For OIDC/OAuth2 logins, the crypt key used to encrypt a saved password can differ between sessions; if it does, the ciphertext saved under a previous key can never be decoded correctly under the current key, and every connection attempt hits this same decode failure — surfacing the scary, unrecoverable-looking "Failed to decrypt the saved password" error. Deleting and recreating the pgAdmin user doesn't help because the problem is in the stored ciphertext for the server record, not the user record.Fix
Catch the decode failure in
_decode_password(), log a warning, and return an empty/no-password result instead of propagating the exception — i.e. treat an undecryptable saved password the same as "no saved password was ever set."connect()then falls through to its normal password-prompt path, so the user is prompted for and can re-enter the correct password, making the account recoverable instead of permanently stuck. OIDC/OAuth2 crypt-key derivation itself is untouched by this change.Test Steps
(Exact repro requires an OIDC/OAuth2 setup where the crypt key can rotate between sessions; a close approximation can be done by directly corrupting a stored password's ciphertext in the config DB for a test server.)
passwordciphertext for that server record so it no longer decrypts under the current key.Summary by CodeRabbit