Skip to content

fix(deps): bump tokio-postgres to 0.7.18 - #465

Open
tobyhede wants to merge 4 commits into
mainfrom
fix/cip-4104-tokio-postgres-patch
Open

tobyhede wants to merge 4 commits into
mainfrom
fix/cip-4104-tokio-postgres-patch

Conversation

@tobyhede

Copy link
Copy Markdown
Contributor

Summary

Bumps tokio-postgres in Cargo.lock from 0.7.13 → 0.7.18 to address GHSA-3gjw-f78c-vvpw (Medium; affected >= 0.4.0, < 0.7.18). Resolves Dependabot alert #55.

Lockfile-only change; no Cargo.toml edits (all manifests already use tokio-postgres = "0.7").

Lockfile changes

  • tokio-postgres 0.7.13 → 0.7.18
  • Companion/transitive bumps pulled in by the resolver: postgres-types 0.2.9 → 0.2.14, postgres-derive 0.4.6 → 0.4.9, whoami 1.6.0 → 2.1.3, wasite 0.1.0 → 1.0.2, phf/phf_shared 0.11.3 → 0.13.1, serde_json 1.0.140 → 1.0.151; added objc2-core-foundation, objc2-system-configuration, zmij
  • libredox 0.1.3 → 0.1.24 (required: whoami 2.x needs a newer libredox than was locked)
  • postgres-protocol (0.6.12) and tokio-postgres-rustls (0.13.0) unchanged

Verification

  • cargo tree -i tokio-postgres --workspace: single version, tokio-postgres v0.7.18
  • cargo check --workspace --all-targets: passes

Addresses GHSA-3gjw-f78c-vvpw (affected >= 0.4.0, < 0.7.18).
Lockfile-only change.
tokio-postgres 0.7.18 no longer appends the cause to Error's Display, so
integration assertions on err.to_string() saw a bare "db error" and failed
on all four PostgreSQL versions.

Assert on the structured error instead: assert_db_error reads severity and
message off as_db_error(), assert_client_error reads the kind off Display
and the detail off source(). The exact customer-visible message text,
including the docs/errors.md links, stays pinned.

ConfigError::Database is no longer transparent: it renders the cause so
Proxy's own logs keep the server's message rather than logging "db error".
@tobyhede
tobyhede requested a review from freshtonic September 21, 2026 01:49
tokio-postgres 0.7.18 follows the convention that an error's Display
describes only that error, with the cause reached through source(). The
previous fix worked against that by re-embedding the cause in
ConfigError::Database's Display, and it only covered errors converted into
that variant: sites logging a raw tokio_postgres::Error still dropped the
server's message.

Add ErrorChain, a Display wrapper that walks source(), and use it at every
log site that can carry a database error. ConfigError::Database is
transparent again. Recording the error as a dyn Error field is not enough:
the Structured (JSON) format, the default off a terminal, renders only
Display.

ErrorChain skips a cause the message already ends with, so variants that
embed their cause in Display are not printed twice.
Replace the hand-rolled source() match with the shared helper. Same
severity and message are pinned; failures now report what arrived instead
of hitting a bare unreachable!().
Comment thread CHANGELOG.md

### Fixed

- **Database error detail in Proxy's logs**: logged database errors again include the message PostgreSQL returned, not just the error kind. The upgraded PostgreSQL client library stopped appending the underlying cause when an error is rendered as text, which left entries such as `Database connection error` reading only `db error`. Proxy now renders the cause itself, so the server's message is back in the log line.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this entry describes a regression that no user saw. The tokio-postgres bump and the ErrorChain fix are both in this PR, so a released Proxy never logged a bare db error. 3.0.1 logs db error: ERROR: … and so does this branch. A reader of the release notes would think that database errors in the logs were broken in a released version.

I suggest that you remove this entry, or merge one sentence into the ### Security entry below. For example: "Logged database errors still include the message from PostgreSQL." Not blocking.

@freshtonic freshtonic left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving. The dependency bump is correct and the follow-up work is sound.

What I checked

  • ErrorChain gives the same log text as before the bump. Under 0.7.13, Display rendered db error: ERROR: <msg>. Now ErrorChain renders the kind (db error), then : , then the DbError source (ERROR: <msg>). ConfigError::Database and Error::Config are #[error(transparent)], so both Display and source() forward to the inner error and the chain stays correct through the crate wrappers.
  • No client-facing path sends a tokio_postgres::Error to the client. The only tokio-postgres clients are the schema loader, the EQL version probe and the migrate CLI. Their errors are logged, not converted in error_to_response, so no client can get a bare db error in an ErrorResponse.
  • The remaining err.to_string() log sites (ConnectionTimeout, prometheus, TLS, config reload) cannot contain a tokio_postgres::Error, so it is correct to leave them unchanged.
  • assert_db_error compares severity() and message() on the DbError. This is more precise than the old string compare, and the passthrough test no longer needs its own nested match.
  • All required checks pass on 9eaa4bc.

Non-blocking

  1. The PR description is out of date. It says "Lockfile-only change; no Cargo.toml edits", but the branch now also adds ErrorChain, changes nine log sites, adds two test helpers and adds CHANGELOG entries. Please update the summary so the squash commit and the release notes match the change.
  2. See the inline comment on the ### Fixed CHANGELOG entry.
  3. Small point: ErrorChain skips a cause when the text rendered so far ends with that cause. A short cause (for example timeout) can match the end of an unrelated message and get dropped. This is acceptable for log output. If you want it exact, compare only against the parent's own Display, not the accumulated string. No change is necessary.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants