mirror of
https://github.com/block/buzz.git
synced 2026-08-18 06:50:31 +02:00
fix(db): range-check operator timeout values against Postgres
pg_timeout_or_default only checked shape, so a well-formed but
unstorable value ("999...9d") reached every pool's after_connect and
Postgres rejected it there — failing all database access, the exact
outcome the fallback exists to avoid. Parse the magnitude, convert it to
milliseconds by unit, and fall back on overflow or anything above the
int GUC ceiling.
The ceiling lives in buzz-db as PG_TIMEOUT_MAX_MILLIS next to the
timeouts it bounds, pinned to the live server by a Postgres-backed test
so it can drift in neither direction.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Eli Foster <efoster@squareup.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
640b28ef6b
commit
332ef5ff56
+3
-1
@@ -40,7 +40,9 @@ REDIS_URL=redis://localhost:6379
|
||||
|
||||
# Postgres statement_timeout and lock_timeout applied to every runtime
|
||||
# connection. Accepts an integer (milliseconds) with an optional us/ms/s/min/h/d
|
||||
# unit; `0` disables the limit. Schema migrations always run with both lifted.
|
||||
# unit; `0` disables the limit. Postgres stores both as int milliseconds, so the
|
||||
# ceiling is 2147483647ms (~24 days); anything above it, or otherwise malformed,
|
||||
# falls back to the default. Schema migrations always run with both lifted.
|
||||
# BUZZ_DB_STATEMENT_TIMEOUT=30s
|
||||
# BUZZ_DB_LOCK_TIMEOUT=5s
|
||||
|
||||
|
||||
@@ -71,6 +71,13 @@ pub const RUNTIME_STATEMENT_TIMEOUT: &str = "30s";
|
||||
pub const RUNTIME_LOCK_TIMEOUT: &str = "5s";
|
||||
/// Postgres spelling of "no limit", used for schema migrations.
|
||||
pub const TIMEOUT_DISABLED: &str = "0";
|
||||
/// `statement_timeout` and `lock_timeout` are `int` GUCs measured in
|
||||
/// milliseconds, so Postgres refuses anything larger regardless of the unit it
|
||||
/// is spelled with. Callers building a [`DbConfig`] from operator input must
|
||||
/// range-check against this: [`apply_runtime_connection_timeouts`] runs on every
|
||||
/// pooled connection, so an unusable value fails all database access.
|
||||
/// `pg_timeout_max_millis_matches_postgres` pins it to the live server.
|
||||
pub const PG_TIMEOUT_MAX_MILLIS: u128 = i32::MAX as u128;
|
||||
|
||||
/// Apply the runtime safety limits shared by writer, reader, audit, and search
|
||||
/// pools. Values are Postgres interval strings (`"30s"`, `"500ms"`), with
|
||||
@@ -8522,6 +8529,67 @@ mod tests {
|
||||
db.pool.close().await;
|
||||
}
|
||||
|
||||
/// [`PG_TIMEOUT_MAX_MILLIS`] is the bound callers range-check operator input
|
||||
/// against, so it must be the server's real bound: too high and an accepted
|
||||
/// value still fails every `after_connect`; too low and we reject settings
|
||||
/// Postgres would have taken.
|
||||
#[tokio::test]
|
||||
#[ignore = "requires Postgres"]
|
||||
async fn pg_timeout_max_millis_matches_postgres() {
|
||||
let mut conn = PgConnection::connect(&admin_url().await)
|
||||
.await
|
||||
.expect("connect");
|
||||
|
||||
let boundary = PG_TIMEOUT_MAX_MILLIS.to_string();
|
||||
apply_runtime_connection_timeouts(&mut conn, &boundary, &boundary)
|
||||
.await
|
||||
.expect("PG_TIMEOUT_MAX_MILLIS must be settable");
|
||||
|
||||
// Same instant spelled in a coarser unit — the conversion the caller's
|
||||
// range check performs must land inside the range too.
|
||||
let in_seconds = (PG_TIMEOUT_MAX_MILLIS / 1_000).to_string();
|
||||
apply_runtime_connection_timeouts(
|
||||
&mut conn,
|
||||
&format!("{in_seconds}s"),
|
||||
&format!("{in_seconds}s"),
|
||||
)
|
||||
.await
|
||||
.expect("the boundary in seconds must be settable");
|
||||
|
||||
for over in [
|
||||
(PG_TIMEOUT_MAX_MILLIS + 1).to_string(),
|
||||
format!("{}ms", PG_TIMEOUT_MAX_MILLIS + 1),
|
||||
format!("{}s", PG_TIMEOUT_MAX_MILLIS / 1_000 + 1),
|
||||
"999999999999999999999999999999999999999999d".to_string(),
|
||||
] {
|
||||
let err = apply_runtime_connection_timeouts(&mut conn, &over, RUNTIME_LOCK_TIMEOUT)
|
||||
.await
|
||||
.expect_err(&format!("{over} must be rejected by Postgres"));
|
||||
let code = match &err {
|
||||
sqlx::Error::Database(db) => db.code().map(|c| c.to_string()),
|
||||
other => panic!("expected a database error, got {other:?}"),
|
||||
};
|
||||
assert_eq!(
|
||||
code.as_deref(),
|
||||
Some("22023"),
|
||||
"{over}: expected invalid_parameter_value"
|
||||
);
|
||||
}
|
||||
|
||||
// A rejected `set_config` leaves the session usable, so the failure mode
|
||||
// is a poisoned pool of unbounded sessions only if the caller ignores it.
|
||||
let statement_timeout: String = sqlx::query_scalar("SHOW statement_timeout")
|
||||
.fetch_one(&mut conn)
|
||||
.await
|
||||
.expect("SHOW statement_timeout");
|
||||
assert_ne!(
|
||||
statement_timeout, "0",
|
||||
"a rejected value must not silently disable the limit"
|
||||
);
|
||||
|
||||
conn.close().await.expect("close");
|
||||
}
|
||||
|
||||
/// `spawn_fence_probe` must verify the floor guard before letting the
|
||||
/// probe run — catalog shape AND observed behavior — and refuse on
|
||||
/// sabotage. This is the production gate for a relay running with
|
||||
|
||||
+102
-20
@@ -306,14 +306,30 @@ fn parse_bind_addr(raw: &str) -> Result<SocketAddr, ConfigError> {
|
||||
.map_err(|e| ConfigError::InvalidBindAddr(e.to_string()))
|
||||
}
|
||||
|
||||
/// Postgres accepts a timeout as an integer (milliseconds) with an optional
|
||||
/// unit. Anything else is refused in favor of the default rather than failing
|
||||
/// the config: a malformed value would break every `after_connect`, taking all
|
||||
/// Postgres access with it, and a relay that keeps its documented default is a
|
||||
/// better outcome than one that will not start.
|
||||
fn pg_timeout_or_default(raw: Option<&str>, default: &str) -> String {
|
||||
const UNITS: [&str; 6] = ["us", "ms", "s", "min", "h", "d"];
|
||||
/// Convert a Postgres timeout magnitude and unit to milliseconds, mirroring the
|
||||
/// rounding Postgres applies to sub-millisecond `us` values. `None` means the
|
||||
/// spelling is not something Postgres would accept.
|
||||
fn pg_timeout_millis(magnitude: &str, unit: &str) -> Option<u128> {
|
||||
let value = magnitude.parse::<u128>().ok()?;
|
||||
match unit {
|
||||
"us" => Some((value + 500) / 1_000),
|
||||
"" | "ms" => Some(value),
|
||||
"s" => value.checked_mul(1_000),
|
||||
"min" => value.checked_mul(60_000),
|
||||
"h" => value.checked_mul(3_600_000),
|
||||
"d" => value.checked_mul(86_400_000),
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
|
||||
/// Postgres accepts a timeout as an integer (milliseconds) with an optional
|
||||
/// unit, bounded by `i32::MAX` ms once converted. Anything else is refused in
|
||||
/// favor of the default rather than failing the config: an unusable value would
|
||||
/// break every `after_connect`, taking all Postgres access with it, and a relay
|
||||
/// that keeps its documented default is a better outcome than one that will not
|
||||
/// start. Magnitude is range-checked, not just shape-checked — `999...9d` is a
|
||||
/// well-formed spelling that Postgres still rejects.
|
||||
fn pg_timeout_or_default(raw: Option<&str>, default: &str) -> String {
|
||||
let candidate = raw.map(str::trim).filter(|value| !value.is_empty());
|
||||
let Some(candidate) = candidate else {
|
||||
return default.to_string();
|
||||
@@ -321,19 +337,29 @@ fn pg_timeout_or_default(raw: Option<&str>, default: &str) -> String {
|
||||
|
||||
let digits = candidate.chars().take_while(char::is_ascii_digit).count();
|
||||
let (magnitude, unit) = candidate.split_at(digits);
|
||||
let unit = unit.trim();
|
||||
let valid = !magnitude.is_empty()
|
||||
&& (unit.is_empty() || UNITS.iter().any(|known| unit.eq_ignore_ascii_case(known)));
|
||||
if valid {
|
||||
candidate.to_string()
|
||||
} else {
|
||||
tracing::warn!(
|
||||
value = candidate,
|
||||
default,
|
||||
"ignoring malformed Postgres timeout — expected an integer with an optional \
|
||||
us/ms/s/min/h/d unit"
|
||||
);
|
||||
default.to_string()
|
||||
let unit = unit.trim().to_ascii_lowercase();
|
||||
|
||||
match pg_timeout_millis(magnitude, &unit) {
|
||||
Some(millis) if millis <= buzz_db::PG_TIMEOUT_MAX_MILLIS => candidate.to_string(),
|
||||
Some(millis) => {
|
||||
tracing::warn!(
|
||||
value = candidate,
|
||||
millis = %millis,
|
||||
max_millis = %buzz_db::PG_TIMEOUT_MAX_MILLIS,
|
||||
default,
|
||||
"ignoring out-of-range Postgres timeout — Postgres stores it as int milliseconds"
|
||||
);
|
||||
default.to_string()
|
||||
}
|
||||
None => {
|
||||
tracing::warn!(
|
||||
value = candidate,
|
||||
default,
|
||||
"ignoring malformed Postgres timeout — expected an integer with an optional \
|
||||
us/ms/s/min/h/d unit"
|
||||
);
|
||||
default.to_string()
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1337,6 +1363,47 @@ mod tests {
|
||||
assert_eq!(pg_timeout_or_default(None, "5s"), "5s");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn pg_timeout_refuses_magnitudes_postgres_cannot_store() {
|
||||
// Well-formed spellings whose millisecond value exceeds the int GUC
|
||||
// range. Postgres rejects these in `set_config`, which would fail every
|
||||
// pool's `after_connect`.
|
||||
for rejected in [
|
||||
"2147483648",
|
||||
"2147483648ms",
|
||||
"2147484s",
|
||||
"35792min",
|
||||
"597h",
|
||||
"25d",
|
||||
// Wider than any integer type — must fall back, not overflow.
|
||||
"999999999999999999999999999999999999999999d",
|
||||
"99999999999999999999999999999999999999999999999999",
|
||||
] {
|
||||
assert_eq!(
|
||||
pg_timeout_or_default(Some(rejected), "30s"),
|
||||
"30s",
|
||||
"{rejected:?} is out of range for Postgres and must fall back"
|
||||
);
|
||||
}
|
||||
|
||||
// The boundary itself, and the same instant in every unit, stay valid.
|
||||
for accepted in [
|
||||
"2147483647",
|
||||
"2147483647ms",
|
||||
"2147483647000us",
|
||||
"2147483s",
|
||||
"35791min",
|
||||
"596h",
|
||||
"24d",
|
||||
] {
|
||||
assert_eq!(
|
||||
pg_timeout_or_default(Some(accepted), "30s"),
|
||||
accepted,
|
||||
"{accepted} is inside the Postgres range"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn db_timeout_env_overrides_and_invalid_fallback() {
|
||||
let _guard = ENV_MUTEX.lock().unwrap();
|
||||
@@ -1366,6 +1433,21 @@ mod tests {
|
||||
"a malformed value must not be handed to Postgres"
|
||||
);
|
||||
|
||||
// Shape-valid but far outside the int millisecond GUC range: Postgres
|
||||
// would reject it in every pool's `after_connect`.
|
||||
std::env::set_var(
|
||||
"BUZZ_DB_STATEMENT_TIMEOUT",
|
||||
"999999999999999999999999999999999999999999d",
|
||||
);
|
||||
std::env::set_var("BUZZ_DB_LOCK_TIMEOUT", "25d");
|
||||
let out_of_range = Config::from_env().expect("config");
|
||||
assert_eq!(
|
||||
out_of_range.db_statement_timeout,
|
||||
buzz_db::RUNTIME_STATEMENT_TIMEOUT,
|
||||
"an out-of-range value must not be handed to Postgres"
|
||||
);
|
||||
assert_eq!(out_of_range.db_lock_timeout, buzz_db::RUNTIME_LOCK_TIMEOUT);
|
||||
|
||||
match previous_statement {
|
||||
Some(value) => std::env::set_var("BUZZ_DB_STATEMENT_TIMEOUT", value),
|
||||
None => std::env::remove_var("BUZZ_DB_STATEMENT_TIMEOUT"),
|
||||
|
||||
Reference in New Issue
Block a user