Repository navigation
fix(relay): keep database error text off WebSocket replies - #9234
Draft
ThePumpingLemma wants to merge 1 commit into
Draft
ThePumpingLemma wants to merge 1 commit into
ThePumpingLemma wants to merge 1 commit into
Conversation
Problem: two WebSocket paths sent the database driver's error text to
the client. HTTP already avoids this (`api::db_read_error` logs only
`DbError::kind()`, because DB error text can repeat a bound value).
- COUNT: `req::db_read_closed_reason` returned `format!("error: {e}")`
for any non-timeout failure, so a failed COUNT read closed with the
raw sqlx message, e.g. `error: database error: encountered unexpected
or invalid data: <text>`. The sibling `history_read_closed_reason`
already returned the fixed `DATABASE_ERROR_CLOSED`.
- Channel write gate: `ingest::check_channel_membership` returned
`Result<(), String>` and built `"error: database error: {e}"` for a
failed lookup. Callers mapped every `Err` to `IngestError::Rejected`,
whose text is forwarded verbatim and counted as `invalid`. A channel
ephemeral event whose membership lookup failed answered
`OK false "error: database error: database error: pool timed out
while waiting for an open connection"`. Two callers told the cases
apart by `starts_with("error:")`.
Change:
- One `req::db_read_closed_reason(&DbError) -> &'static str` for REQ
history and COUNT: a statement cancel keeps `error: query timed out`,
anything else is `error: database error`. The four COUNT failure
sites go through `count::close_failed_count`, which logs only the
error kind.
- `check_channel_membership` returns `ChannelWriteRefusal`
(`Denied(&'static str)` | `Db(DbError)`). `From<ChannelWriteRefusal>
for IngestError` maps a denial to `Rejected` with the same text and a
failed lookup to `Internal`. The string sniffing in
`validate_edit_ownership` and `report::reporter_can_read` is deleted;
both now match on the variant.
- `IngestError::db(&DbError)` builds the existing
`Internal("error: database error: <kind>")` text. It replaces the one
existing copy (the event insert) and is used by the gate, edit
ownership and the report handler. `validate_edit_ownership` and
`handle_report_event` return `IngestError`, so their own lookup
failures (which also became `Rejected("…: <driver text>")`) are
internal too.
Why it is safe: every client-facing refusal text stays byte-identical
(`restricted: not a channel member`, `restricted: only members can post
in this channel`, the edit/report `invalid:` reasons). Only DB failures
change: persistent writes now answer `error: internal server error`
(HTTP 500) instead of `invalid`/400 with driver text, ephemeral writes
answer `error: database error: <kind>`, and COUNT closes with the same
fixed reason as REQ. A connection-unavailable COUNT read gets
`error: database error`, like a REQ history read; a separate WS
"unavailable" reason would have to change REQ too. A non-member whose
channel lookup fails is still refused as a non-member, and a
token-scoped target denial still hides the target on any refusal.
Tests:
- `handlers::req::tests::db_read_closed_reason_never_carries_driver_text`
failed before (`left: "error: database error: encountered unexpected
or invalid data: bound value 'secret'"`), passes after.
- `handlers::event::tests::channel_ephemeral_membership_lookup_failure_is_internal`
drives `publish_channel_ephemeral` against an unreachable database.
Failed before (`Err(Rejected("error: database error: database error:
pool timed out while waiting for an open connection"))`), passes
after with `Internal("error: database error: pool_timed_out")`.
- The Postgres-only bridge COUNT test now pins the exact ordinary-error
CLOSED frame instead of accepting any `error: ` prefix.
Signed-off-by: David Grochowski <dgrochowski@squareup.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Two WebSocket paths sent the database driver's error text to clients. HTTP already avoids this:
api::db_read_errorlogs onlyDbError::kind(), because DB error text can repeat a bound value.req::db_read_closed_reasonreturnedformat!("error: {e}")for any failure that wasn't a timeout. A failed COUNT read therefore closed with the raw sqlx message, e.g.["CLOSED","c","error: database error: encountered unexpected or invalid data: <text>"]. Its siblinghistory_read_closed_reason, used for REQ, already returned the fixederror: database error.ingest::check_channel_membershipreturnedResult<(), String>and built"error: database error: {e}"when a lookup failed. Callers turned everyErrintoIngestError::Rejected, whose text is forwarded verbatim and counted asinvalid. A channel ephemeral event whose membership lookup failed gotOK false "error: database error: database error: pool timed out while waiting for an open connection". Two callers (edit ownership, report readability) told denials from DB errors apart withstarts_with("error:").Change
req::db_read_closed_reason(&DbError) -> &'static str. A statement cancel keepserror: query timed out; anything else getserror: database error. All four COUNT failure sites go throughcount::close_failed_count, which logs only the error kind.check_channel_membershipreturnsChannelWriteRefusal(Denied(&'static str)|Db(DbError)).From<ChannelWriteRefusal> for IngestErrorturns a denial intoRejectedwith the same text and a failed lookup intoInternal. The string sniffing invalidate_edit_ownershipandreport::reporter_can_readis gone; both match on the variant.IngestError::db(&DbError)builds the existingInternal("error: database error: <kind>")text. It replaces the one existing copy (the event insert) and is used by the gate, edit ownership and the report handler.validate_edit_ownershipandhandle_report_eventnow returnIngestError, so their own lookup failures are internal too. Before, those also becameRejected("…: <driver text>").Every client-facing refusal text is byte-identical:
restricted: not a channel member,restricted: only members can post in this channel, and the edit/reportinvalid:reasons. Only DB failures change:error: internal server error(HTTP 500) instead ofinvalid/400 with driver text.error: database error: <kind>.Two cases keep their behaviour: a non-member whose channel lookup fails is still refused as a non-member, and a denial for a token-scoped target still hides the target. A COUNT read that can't get a connection closes with
error: database error, exactly like a REQ history read today (the bridge shed-read test pins that for REQ). A separate WebSocket "unavailable" reason would have to change REQ too, so it is not part of this PR.Testing
Regression tests, run before the fix (only the tests were added; production code unchanged):
After the fix:
cargo test -p buzz-relay --lib -- handlers::req:: handlers::count:: handlers::event:: handlers::report:: handlers::ingest::→297 passed; 0 failed; 40 ignored, including both regression tests. The ephemeral test now getsInternal("error: database error: pool_timed_out").cargo clippy -p buzz-relay --all-targets -- -D warnings→ clean (this also compiles the Postgres-only tests).cargo fmt --all→ no changes.rust-testsandbranch-skew.The Postgres-only
routed_read_cancel_maps_to_timeout_contract_on_count_and_searchused to accept anyerror:prefix for the ordinary-error WS COUNT frame. It now pins the exact["CLOSED","c","error: database error"]. It compiles but was not run here (no local Postgres).Not covered
These handlers still put
format!("database error: {e}")(driver text) into aStringthat becomesIngestError::Rejected. Each needs a typed error orIngestError::db; that is a separate sweep:handlers/relay_admin.rslines 258, 277, 352, 402, 409, 488, 496 (viamap_relay_admin_error)handlers/identity_archive.rslines 82, 88, 243, 281 (mapped toRejected("invalid: …")iningest.rs)handlers/moderation_commands.rslines 114, 202, 285, 331, 392, 460handlers/product_feedback.rsline 55handlers/ingest.rsthread-parent lookups (around lines 1186, 1198, 1377, 1386) and the forum vote target lookup (around line 1568)These are already
Internal, so clients get sanitised text, but they still put raw driver text into the HTTP error log:load_channel_for_write(ingest.rs around line 1104), the relay-member removal (around line 3061), andmap_serving_fence_state.Review
No blockers, and no code changes from review. Checked:
invalid:reasons, and the reportinvalid:reasons.target_deniedstill hides the target for every gate refusal, including a failed lookup.IngestError::dbcarries onlyDbError::kind()labels.Internalmaps to the existingerrormetric label, HTTP 500, and the conformanceserver_error.invalid:to retryableerror:.Open optional items:
validate_edit_ownershipandreport::reporter_can_readhave no test. An unreachable-DB unit test, like the ephemeral one, would pin them.access.db_failed(&e)does the same thing. This predates the PR.Exercised on a local relay
Commit tested:
a0987ab65. Dev profile, isolated harness relay on :3030, authenticated channel member. I broke the database two ways, each with a WS connection already open:eventsandchannel_memberstables, so queries fail with SQLSTATE 42P01.docker pauseon the harness Postgres, so pool acquire times out.COUNTCOUNT {count:1}CLOSED "error: database error"CLOSED "error: database error"(≈3 s)REQCLOSED "error: database error"CLOSED "error: database error"EVENTkind 9 (channel write)OK trueOK false "error: internal server error"OK false "error: internal server error"EVENTkind 20001 (channel ephemeral)OK trueOK false "error: database error: database:42P01"OK false "error: internal error checking restriction state"/countinternal server errorrelay: no community is configured for this host/eventskind 9internal server errorNo WebSocket reply contained driver text: no relation names, no sqlx messages, no "pool timed out". The ephemeral reply carries only the error kind (
database:42P01). After the tables were restored and Postgres unpaused, the same requests succeeded again without restarting the relay.Observation (outside this PR): while Postgres is unreachable, HTTP requests get 404
no community is configured for this host. That is because tenant resolution treats a failed lookup as an unknown host. A 503 would describe the outage better.Postgres-gated tests: I pointed
DATABASE_URL,BUZZ_TEST_DATABASE_URLandBUZZ_TEST_REDIS_URLat the harness.--ignored routed_read_cancel_maps_to_timeout_contract_on_count_and_search handlers::count:: handlers::report::: 2 passed, including the modifiedrouted_read_cancel…test.--ignored --test-threads=1 routed_read_cancel handlers::req:: handlers::count:: handlers::event:: handlers::report:: handlers::ingest::: 40 passed, 1 failed. The failure isingest_write_fence_follows_community_deletion_lifecycle. It runs sqlx migrations, which fail against the harness schema that pgschema already applied (type "channel_type" already exists). That is a test-environment problem.Not exercised:
error: query timed out).