Merge commit from fork

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Alexander Petric
2026-10-23 18:49:17 -04:00
committed by GitHub
co-authored by Claude Opus 5
parent 8af484292f
commit 812ff6ea5e
13 changed files with 105 additions and 299 deletions
+1 -1
View File
@@ -1 +1 @@
b6e23d229966cf0019c0a1926a254fda658d8def 5dfdf811c862c689e42f03f485e02f9f1e7bc7ca
+37 -5
View File
@@ -134,6 +134,17 @@ async fn list_search_flows(
Ok(Json(rows)) Ok(Json(rows))
} }
/// `WHERE` condition matching one label, directly on the flow or inherited from its folder.
///
/// The template has two `?`, so both are filled in a single `binds` pass. Chaining `bind`
/// would not work: `bind` replaces the first `?` of the string *as it is at that moment*,
/// so the second call would find the `?` that the first call inserted as part of the bound
/// value and substitute there, leaving the value partly outside its quotes.
fn label_condition(label: &str) -> String {
"(o.labels @> ARRAY[?] OR folder_labels(o.workspace_id, o.path) @> ARRAY[?])"
.binds(&[&label.trim()])
}
async fn list_flows( async fn list_flows(
authed: ApiAuthed, authed: ApiAuthed,
Extension(user_db): Extension<UserDB>, Extension(user_db): Extension<UserDB>,
@@ -217,11 +228,7 @@ async fn list_flows(
} }
if let Some(label) = &lq.label { if let Some(label) = &lq.label {
for l in label.split(',') { for l in label.split(',') {
sqlb.and_where( sqlb.and_where(label_condition(l));
"(o.labels @> ARRAY[?] OR folder_labels(o.workspace_id, o.path) @> ARRAY[?])"
.bind(&l.trim())
.bind(&l.trim()),
);
} }
} }
@@ -2284,6 +2291,31 @@ mod tests {
const SECOND: Duration = Duration::from_secs(1); const SECOND: Duration = Duration::from_secs(1);
#[test]
fn label_condition_fills_both_placeholders_in_one_pass() {
// Ordinary label: unchanged behaviour, the value appears quoted in both positions.
assert_eq!(
super::label_condition(" finance "),
"(o.labels @> ARRAY['finance'] OR folder_labels(o.workspace_id, o.path) @> ARRAY['finance'])"
);
// A `?` in the label must stay inside the quoted literal in both positions and must
// not leave a placeholder behind. With chained `bind`s this produced
// `ARRAY['a'a?b'b'] ... ARRAY[?]`, i.e. label text outside the quotes.
let cond = super::label_condition("a?b");
assert_eq!(
cond,
"(o.labels @> ARRAY['a?b'] OR folder_labels(o.workspace_id, o.path) @> ARRAY['a?b'])"
);
assert!(!cond.contains("ARRAY[?]"));
// Single quotes keep being escaped by doubling.
assert_eq!(
super::label_condition("a'?b"),
"(o.labels @> ARRAY['a''?b'] OR folder_labels(o.workspace_id, o.path) @> ARRAY['a''?b'])"
);
}
#[test] #[test]
fn flowmodule_serde() { fn flowmodule_serde() {
let fv = FlowValue { let fv = FlowValue {
@@ -406,62 +406,3 @@ async fn test_operators_draft_resources_and_variables(db: Pool<Postgres>) -> any
Ok(()) Ok(())
} }
/// The two endpoints that make the API server run `pg_dump`/`psql` against a database a request
/// names are closed to a role that cannot run code.
///
/// Both polarities on each route: a 403 alone would equally be a route that does not exist or one
/// refusing the source for some other reason, and the admin's 400 shows the gate is about the role
/// rather than the request. The source is deliberately unresolvable, so the gate is the only thing
/// that can answer 403 and no database is ever reached.
#[sqlx::test(migrations = "../migrations", fixtures("base", "permissions_test"))]
#[serial]
async fn test_dumping_a_database_needs_the_code_right(db: Pool<Postgres>) -> anyhow::Result<()> {
initialize_tracing().await;
windmill_common::roles::invalidate_all_roles_cache();
let server = ApiServer::start(db.clone()).await?;
let port = server.addr.port();
let api = format!("http://localhost:{port}/api/w/{WS}");
let cases = [
(
"workspaces/export_pg_schema",
json!({"source": "u/operator/not-a-source"}),
"export a database schema",
),
(
"workspaces/import_pg_database",
json!({"source": "u/operator/not-a-source", "target": "u/operator/not-a-source",
"fork_behavior": "schema_only"}),
"import a database",
),
];
for (route, body, action) in cases {
let resp = operator_client()
.post(format!("{api}/{route}"))
.json(&body)
.send()
.await?;
let status = resp.status();
let text = resp.text().await?;
assert_eq!(status, 403, "{route}: {text}");
assert!(
text.contains(&format!("Your role cannot {action} in this workspace")),
"{route} refused for another reason: {text}"
);
let resp = reqwest::Client::new()
.post(format!("{api}/{route}"))
.header("Authorization", "Bearer SECRET_TOKEN")
.json(&body)
.send()
.await?;
let status = resp.status();
let text = resp.text().await?;
assert_eq!(status, 400, "{route}: {text}");
assert!(text.contains("Invalid source format"), "{route}: {text}");
}
Ok(())
}
+34 -3
View File
@@ -268,9 +268,10 @@ pub fn filter_list_queue_query(
if let Some(bf) = &lq.broad_filter { if let Some(bf) = &lq.broad_filter {
let pat = format!("%{}%", escape_ilike_pattern(bf)); let pat = format!("%{}%", escape_ilike_pattern(bf));
// `binds` fills every `?` in one pass; chained `bind`s would re-scan a `?` in the bound value
sqlb.and_where( sqlb.and_where(
"(runnable_path ILIKE ? OR v2_job.tag ILIKE ? OR trigger ILIKE ? OR trigger_kind::text ILIKE ?)" "(runnable_path ILIKE ? OR v2_job.tag ILIKE ? OR trigger ILIKE ? OR trigger_kind::text ILIKE ?)"
.bind(&pat).bind(&pat).bind(&pat).bind(&pat) .binds(&[&pat]),
); );
} }
@@ -490,7 +491,7 @@ pub fn filter_list_completed_query(
} }
if let Some(dt) = &lq.created_or_started_after { if let Some(dt) = &lq.created_or_started_after {
let ts = dt.to_rfc3339(); let ts = dt.to_rfc3339();
sqlb.and_where("(created_at >= ? OR started_at >= ?)".bind(&ts).bind(&ts)); sqlb.and_where("(created_at >= ? OR started_at >= ?)".binds(&[&ts]));
} }
if let Some(dt) = &lq.created_before { if let Some(dt) = &lq.created_before {
@@ -591,10 +592,11 @@ pub fn filter_list_completed_query(
if let Some(bf) = &lq.broad_filter { if let Some(bf) = &lq.broad_filter {
let pat = format!("%{}%", escape_ilike_pattern(bf)); let pat = format!("%{}%", escape_ilike_pattern(bf));
// `binds` fills every `?` in one pass; chained `bind`s would re-scan a `?` in the bound value
sqlb.and_where( sqlb.and_where(
"(runnable_path ILIKE ? OR v2_job.tag ILIKE ? OR trigger ILIKE ? OR trigger_kind::text ILIKE ? \ "(runnable_path ILIKE ? OR v2_job.tag ILIKE ? OR trigger ILIKE ? OR trigger_kind::text ILIKE ? \
OR EXISTS (SELECT 1 FROM unnest(v2_job.labels) lbl WHERE lbl ILIKE ?))" OR EXISTS (SELECT 1 FROM unnest(v2_job.labels) lbl WHERE lbl ILIKE ?))"
.bind(&pat).bind(&pat).bind(&pat).bind(&pat).bind(&pat) .binds(&[&pat]),
); );
} }
@@ -945,6 +947,20 @@ mod tests {
assert!(sql.contains("'schedule'")); assert!(sql.contains("'schedule'"));
} }
#[test]
fn test_queue_broad_filter_with_question_mark() {
let lq = ListQueueQuery { broad_filter: Some("a?b".to_string()), ..empty_queue_query() };
let sqlb = filter_list_queue_query(
SqlBuilder::select_from("v2_job_queue").clone(),
&lq,
"ws",
false,
);
let sql = build_sql(sqlb);
assert_eq!(sql.matches("ILIKE '%a?b%'").count(), 4);
assert!(!sql.contains("ILIKE ?"));
}
// --- Completed query tests --- // --- Completed query tests ---
#[test] #[test]
@@ -1124,4 +1140,19 @@ mod tests {
let sql = build_sql(sqlb); let sql = build_sql(sqlb);
assert!(sql.contains("result @>")); assert!(sql.contains("result @>"));
} }
#[test]
fn test_completed_broad_filter_with_question_mark() {
let lq =
ListCompletedQuery { broad_filter: Some("a?b".to_string()), ..empty_completed_query() };
let sqlb = filter_list_completed_query(
SqlBuilder::select_from("v2_job_completed").clone(),
&lq,
"ws",
false,
);
let sql = build_sql(sqlb);
assert_eq!(sql.matches("ILIKE '%a?b%'").count(), 5);
assert!(!sql.contains("ILIKE ?"));
}
} }
+4 -3
View File
@@ -894,17 +894,18 @@ async fn list_schedule(
} }
if let Some(broad_filter) = &lsq.broad_filter { if let Some(broad_filter) = &lsq.broad_filter {
let pat = format!("%{}%", escape_ilike_pattern(broad_filter)); let pat = format!("%{}%", escape_ilike_pattern(broad_filter));
// `binds` fills every `?` in one pass; chained `bind`s would re-scan a `?` in the bound value
sqlb.and_where( sqlb.and_where(
"(path ILIKE ? OR script_path ILIKE ? OR description ILIKE ? OR summary ILIKE ? OR schedule ILIKE ?)" "(path ILIKE ? OR script_path ILIKE ? OR description ILIKE ? OR summary ILIKE ? OR schedule ILIKE ?)"
.bind(&pat).bind(&pat).bind(&pat).bind(&pat).bind(&pat) .binds(&[&pat]),
); );
} }
if let Some(label) = &lsq.label { if let Some(label) = &lsq.label {
for l in label.split(',') { for l in label.split(',') {
// `binds` fills every `?` in one pass; chained `bind`s would re-scan a `?` in the bound value
sqlb.and_where( sqlb.and_where(
"(labels @> ARRAY[?] OR folder_labels(workspace_id, path) @> ARRAY[?])" "(labels @> ARRAY[?] OR folder_labels(workspace_id, path) @> ARRAY[?])"
.bind(&l.trim()) .binds(&[&l.trim()]),
.bind(&l.trim()),
); );
} }
} }
+2 -2
View File
@@ -309,10 +309,10 @@ async fn list_scripts(
} }
if let Some(label) = &lq.label { if let Some(label) = &lq.label {
for l in label.split(',') { for l in label.split(',') {
// `binds` fills every `?` in one pass; chained `bind`s would re-scan a `?` in the bound value
sqlb.and_where( sqlb.and_where(
"(o.labels @> ARRAY[?] OR folder_labels(o.workspace_id, o.path) @> ARRAY[?])" "(o.labels @> ARRAY[?] OR folder_labels(o.workspace_id, o.path) @> ARRAY[?])"
.bind(&l.trim()) .binds(&[&l.trim()]),
.bind(&l.trim()),
); );
} }
} }
+13 -150
View File
@@ -3366,10 +3366,6 @@ pub(crate) async fn resolve_pg_source_checked_with_kind(
let pg: PgDatabase = serde_json::from_value(db_resource) let pg: PgDatabase = serde_json::from_value(db_resource)
.map_err(|e| Error::internal_err(format!("Failed to parse database credentials: {}", e)))?; .map_err(|e| Error::internal_err(format!("Failed to parse database credentials: {}", e)))?;
// Checked here rather than at each caller: every source and target of a dump, restore or copy
// comes through this function, and `validate_dbname` only ever reaches a target. The check is
// deliberately the narrow one — see `validate_dbname_not_option_like`.
windmill_common::validate_dbname_not_option_like(&pg.dbname)?;
Ok((pg, managed_kind)) Ok((pg, managed_kind))
} }
@@ -3459,20 +3455,21 @@ pub(crate) struct PgDumpOptions<'a> {
pub(crate) no_acl: bool, pub(crate) no_acl: bool,
} }
/// A pg_dump invocation against `pg_db` writing to `dump_path`, carrying the connection settings /// Run pg_dump against a PgDatabase, writing output to a temp file on disk.
/// the CLI reads from the environment. The returned file, when there is one, holds the root /// Returns a DumpFile handle; the file is deleted when the handle is dropped.
/// certificate the command points at and must outlive it. pub(crate) async fn pg_dump_database(
fn pg_dump_command(
pg_db: &PgDatabase, pg_db: &PgDatabase,
opts: PgDumpOptions<'_>, opts: PgDumpOptions<'_>,
dump_path: &std::path::Path, ) -> Result<DumpFile> {
) -> Result<(tokio::process::Command, Option<DumpFile>)> { let dump_file = DumpFile::new()?;
let host = &pg_db.host; let host = &pg_db.host;
let port = pg_db.port.unwrap_or(5432).to_string(); let port = pg_db.port.unwrap_or(5432).to_string();
let user = pg_db.login_name(); let user = pg_db.login_name();
let dbname = &pg_db.dbname;
let mut cmd = tokio::process::Command::new("pg_dump"); let mut cmd = tokio::process::Command::new("pg_dump");
cmd.arg("--format=plain").arg("--file").arg(dump_path); cmd.arg("--format=plain").arg("--file").arg(&dump_file.path);
if opts.schema_only { if opts.schema_only {
cmd.arg("--schema-only"); cmd.arg("--schema-only");
} }
@@ -3490,31 +3487,13 @@ fn pg_dump_command(
.arg("--port") .arg("--port")
.arg(&port) .arg(&port)
.arg("--username") .arg("--username")
.arg(user); .arg(user)
.arg(dbname);
// The name comes from a resource, so it never goes on the command line: as the positional
// argument a name starting with `-` is read as an option of pg_dump (`--file`,
// `--restrict-key`), and as `--dbname` a name holding `=` is expanded as a libpq connection
// string overriding the host, port and user set above. libpq takes the environment variable
// as a plain database name, neither parsed as an option nor expanded.
cmd.env("PGDATABASE", &pg_db.dbname);
if let Some(ref password) = pg_db.password { if let Some(ref password) = pg_db.password {
cmd.env("PGPASSWORD", password); cmd.env("PGPASSWORD", password);
} }
let root_cert = apply_pg_tls_env(&mut cmd, pg_db)?; let _root_cert = apply_pg_tls_env(&mut cmd, pg_db)?;
Ok((cmd, root_cert))
}
/// Run pg_dump against a PgDatabase, writing output to a temp file on disk.
/// Returns a DumpFile handle; the file is deleted when the handle is dropped.
pub(crate) async fn pg_dump_database(
pg_db: &PgDatabase,
opts: PgDumpOptions<'_>,
) -> Result<DumpFile> {
let dump_file = DumpFile::new()?;
let (mut cmd, _root_cert) = pg_dump_command(pg_db, opts, &dump_file.path)?;
let output = cmd let output = cmd
.output() .output()
@@ -3640,14 +3619,12 @@ fn psql_command(pg_db: &PgDatabase) -> Result<(tokio::process::Command, Option<D
.arg(pg_db.port.unwrap_or(5432).to_string()) .arg(pg_db.port.unwrap_or(5432).to_string())
.arg("--username") .arg("--username")
.arg(pg_db.login_name()) .arg(pg_db.login_name())
.arg("--dbname")
.arg(&pg_db.dbname)
.arg("--no-psqlrc") .arg("--no-psqlrc")
.stdout(std::process::Stdio::piped()) .stdout(std::process::Stdio::piped())
.stderr(std::process::Stdio::piped()); .stderr(std::process::Stdio::piped());
// Not `--dbname`, which expands a name holding `=` as a libpq connection string overriding
// the host, port and user set above. See `pg_dump_command`.
cmd.env("PGDATABASE", &pg_db.dbname);
if let Some(ref password) = pg_db.password { if let Some(ref password) = pg_db.password {
cmd.env("PGPASSWORD", password); cmd.env("PGPASSWORD", password);
} }
@@ -3738,97 +3715,6 @@ mod pg_tls_env_tests {
} }
} }
/// The database name of a resource reaches `pg_dump` and `psql` through `PGDATABASE`, never in
/// their argument vector, so neither an option (`--file=`, `--restrict-key=`) nor a libpq
/// connection string (`host=... port=...`) in that field is parsed as one.
#[cfg(test)]
mod pg_dbname_env_tests {
use super::{pg_dump_command, pg_dump_database, psql_command, DumpFile, PgDumpOptions};
use windmill_common::PgDatabase;
/// Nothing listens on that port.
fn pg_db(dbname: &str) -> PgDatabase {
PgDatabase {
host: "127.0.0.1".to_string(),
user: None,
password: None,
port: Some(1),
sslmode: None,
dbname: dbname.to_string(),
root_certificate_pem: None,
accept_invalid_certs: None,
use_iam_auth: None,
region: None,
options: None,
}
}
fn pgdatabase_of(cmd: &std::process::Command) -> Option<std::ffi::OsString> {
cmd.get_envs()
.find(|(k, _)| *k == "PGDATABASE")
.and_then(|(_, v)| v.map(|v| v.to_os_string()))
}
#[test]
fn pg_dump_takes_the_dbname_from_the_environment() {
let dump_file = DumpFile::new().unwrap();
let name = "--file=/tmp/written_by_the_caller";
let (cmd, _root_cert) =
pg_dump_command(&pg_db(name), PgDumpOptions::default(), &dump_file.path).unwrap();
let cmd = cmd.as_std();
assert!(
cmd.get_args().all(|arg| arg != name),
"the dbname is still in pg_dump's argument vector"
);
assert_eq!(pgdatabase_of(cmd).as_deref(), Some(name.as_ref()));
// The only `--file` pg_dump is given is still Windmill's own.
assert_eq!(
cmd.get_args()
.filter(|arg| arg.to_string_lossy().starts_with("--file"))
.count(),
1
);
}
#[test]
fn psql_takes_the_dbname_from_the_environment() {
let name = "host=elsewhere";
let (cmd, _root_cert) = psql_command(&pg_db(name)).unwrap();
let cmd = cmd.as_std();
assert!(
cmd.get_args().all(|arg| arg != name),
"the dbname is still in psql's argument vector"
);
assert_eq!(pgdatabase_of(cmd).as_deref(), Some(name.as_ref()));
}
/// The same, observed on the real binary: an option-shaped name must reach the connection
/// attempt rather than pg_dump's option parser. Skipped where pg_dump is not installed —
/// which is why the assertion also requires that pg_dump ran at all (our `pg_dump failed:`
/// prefix) instead of only that the option message is absent.
#[tokio::test]
async fn pg_dump_does_not_read_an_option_shaped_dbname_as_an_option() {
if std::process::Command::new("pg_dump")
.arg("--version")
.output()
.is_err()
{
eprintln!("skipped: pg_dump is not installed");
return;
}
let Err(err) = pg_dump_database(&pg_db("--no-such-option"), PgDumpOptions::default()).await
else {
panic!("pg_dump reached a server on 127.0.0.1:1");
};
let err = err.to_string();
assert!(
err.contains("pg_dump failed:"),
"pg_dump did not run: {err}"
);
assert!(!err.contains("unrecognized option"), "{err}");
}
}
/// GUC names the server backing `pg_db` knows about. /// GUC names the server backing `pg_db` knows about.
/// ///
/// Asked through psql rather than a tokio-postgres connection so the lookup reaches /// Asked through psql rather than a tokio-postgres connection so the lookup reaches
@@ -4175,25 +4061,6 @@ fn ensure_copied_without_roles(governing: &GoverningDatatable) -> Result<()> {
Ok(()) Ok(())
} }
/// Gate for the endpoints that make the API server run `pg_dump`/`psql` against a database a
/// request names.
///
/// Those two processes are children of the API server, outside any job sandbox and holding its
/// environment, and the dump they exchange is a `psql` script assembled from strings the source
/// server returns — so reaching them is close enough to running code on the server that the role
/// which may not run code at all must not reach them either. `code` is the capability the data
/// table migration endpoints next door already ask for (`datatable_migrations::ensure_not_operator`),
/// and not admin: the database manager offers an export and an import of a user's own Postgres
/// resource to every author, who can run the same `pg_dump` in a job anyway.
async fn ensure_can_dump_databases(
db: &DB,
w_id: &str,
authed: &ApiAuthed,
action: &str,
) -> Result<()> {
windmill_common::workspaces::check_role_can_code(db, w_id, authed, action).await
}
/// Import (pg_dump/pg_import) from source to target /// Import (pg_dump/pg_import) from source to target
async fn import_pg_database( async fn import_pg_database(
authed: ApiAuthed, authed: ApiAuthed,
@@ -4202,8 +4069,6 @@ async fn import_pg_database(
Path(w_id): Path<String>, Path(w_id): Path<String>,
Json(req): Json<ImportPgDatabaseRequest>, Json(req): Json<ImportPgDatabaseRequest>,
) -> Result<String> { ) -> Result<String> {
ensure_can_dump_databases(&db, &w_id, &authed, "import a database").await?;
if req.fork_behavior == DataTableForkBehavior::KeepOriginal { if req.fork_behavior == DataTableForkBehavior::KeepOriginal {
return Ok("No action needed for KeepOriginal behavior".to_string()); return Ok("No action needed for KeepOriginal behavior".to_string());
} }
@@ -4320,8 +4185,6 @@ async fn export_pg_schema(
Path(w_id): Path<String>, Path(w_id): Path<String>,
Json(req): Json<ExportPgSchemaRequest>, Json(req): Json<ExportPgSchemaRequest>,
) -> Result<String> { ) -> Result<String> {
ensure_can_dump_databases(&db, &w_id, &authed, "export a database schema").await?;
let pg = resolve_pg_source_checked(&db, &user_db, &authed, &w_id, &req.source).await?; let pg = resolve_pg_source_checked(&db, &user_db, &authed, &w_id, &req.source).await?;
let dump_file = pg_dump_database( let dump_file = pg_dump_database(
&pg, &pg,
+2 -2
View File
@@ -740,10 +740,10 @@ async fn list_apps(
if let Some(label) = &lq.label { if let Some(label) = &lq.label {
for l in label.split(',') { for l in label.split(',') {
// `binds` fills every `?` in one pass; chained `bind`s would re-scan a `?` in the bound value
sqlb.and_where( sqlb.and_where(
"(app.labels @> ARRAY[?] OR folder_labels(app.workspace_id, app.path) @> ARRAY[?])" "(app.labels @> ARRAY[?] OR folder_labels(app.workspace_id, app.path) @> ARRAY[?])"
.bind(&l.trim()) .binds(&[&l.trim()]),
.bind(&l.trim()),
); );
} }
} }
+2 -2
View File
@@ -183,8 +183,8 @@ fn items_query(
let mut sqlb = SqlBuilder::select_from(&format!("{} as o", item_type)); let mut sqlb = SqlBuilder::select_from(&format!("{} as o", item_type));
if scope_type == "favorites" { if scope_type == "favorites" {
sqlb.join("favorite") sqlb.join("favorite")
.on("favorite.favorite_kind = ? AND favorite.workspace_id = o.workspace_id AND favorite.path = o.path AND favorite.usr = ?".bind(&item_type) .on("favorite.favorite_kind = ? AND favorite.workspace_id = o.workspace_id AND favorite.path = o.path AND favorite.usr = ?"
.bind(&authed.username)); .binds(&[&item_type, &authed.username]));
} }
sqlb.and_where("o.workspace_id = ?".bind(&workspace_id)) sqlb.and_where("o.workspace_id = ?".bind(&workspace_id))
.and_where("o.archived = false"); .and_where("o.archived = false");
+2 -2
View File
@@ -98,10 +98,10 @@ async fn list_apps(
if let Some(label) = &lq.label { if let Some(label) = &lq.label {
for l in label.split(',') { for l in label.split(',') {
// `binds` fills every `?` in one pass; chained `bind`s would re-scan a `?` in the bound value
sqlb.and_where( sqlb.and_where(
"(app.labels @> ARRAY[?] OR folder_labels(app.workspace_id, app.path) @> ARRAY[?])" "(app.labels @> ARRAY[?] OR folder_labels(app.workspace_id, app.path) @> ARRAY[?])"
.bind(&l.trim()) .binds(&[&l.trim()]),
.bind(&l.trim()),
); );
} }
} }
-64
View File
@@ -843,50 +843,6 @@ mod validate_dbname_tests {
} }
} }
#[cfg(test)]
mod validate_dbname_not_option_like_tests {
use super::validate_dbname_not_option_like;
/// A source is any database a user already has, so everything `validate_dbname` refuses but
/// Postgres accepts has to keep working.
#[test]
fn accepts_every_name_that_is_not_an_option() {
for name in [
"mydb",
"My-Db_1",
"2024_archive",
"my.db",
"db with spaces",
"Grüße",
"host=elsewhere port=5433",
"",
"a-dash-inside--it",
] {
assert!(
validate_dbname_not_option_like(name).is_ok(),
"refused the legitimate name {name:?}"
);
}
}
#[test]
fn rejects_a_name_that_an_argument_vector_would_read_as_an_option() {
for name in [
"--file=/tmp/x",
"--restrict-key=k",
"-f",
"-",
" --file=/tmp/x",
"\t-f",
] {
let err = validate_dbname_not_option_like(name)
.expect_err(&format!("accepted the option-like name {name:?}"))
.to_string();
assert!(err.contains("cannot start with '-'"), "{err}");
}
}
}
#[cfg(test)] #[cfg(test)]
mod pg_tls_tests { mod pg_tls_tests {
use super::PgDatabase; use super::PgDatabase;
@@ -1639,26 +1595,6 @@ pub fn validate_dbname(dbname: &str) -> error::Result<()> {
Ok(()) Ok(())
} }
/// The one restriction on the name of a database Windmill only *reads from*: it may not look like
/// a command-line option.
///
/// [`validate_dbname`] is the rule for a name Windmill creates or interpolates into SQL, and is
/// far too narrow here: a source is any Postgres resource a user already has, and an existing
/// database may legitimately be named `2024_archive`, `my.db`, `Grüße` or anything else Postgres
/// accepts as a quoted identifier. Refusing those would break working resources. A name opening
/// with `-` is the one shape that is never a database and always a mistake or an attempt to smuggle
/// an option into a `pg_dump`/`psql` argument vector, so it is refused wherever a source is
/// resolved — the commands themselves pass the name through `PGDATABASE` and never on the command
/// line, and this keeps a future caller that forgets from re-opening that door.
pub fn validate_dbname_not_option_like(dbname: &str) -> error::Result<()> {
if dbname.trim_start().starts_with('-') {
return Err(error::Error::BadRequest(
"Database name cannot start with '-'".to_string(),
));
}
Ok(())
}
/// Lock the instance databases among `names` until `tx` ends, in a stable order. Taken by every /// Lock the instance databases among `names` until `tx` ends, in a stable order. Taken by every
/// settings save naming an instance database, and by [`drop_unused_instance_database`]: a save /// settings save naming an instance database, and by [`drop_unused_instance_database`]: a save
/// cannot start using a database between that drop's check that nothing does and the drop. /// cannot start using a database between that drop's check that nothing does and the drop.
+4 -3
View File
@@ -439,9 +439,10 @@ async fn list_resources(
if let Some(broad_filter) = &lq.broad_filter { if let Some(broad_filter) = &lq.broad_filter {
let pat = format!("%{}%", escape_ilike_pattern(broad_filter)); let pat = format!("%{}%", escape_ilike_pattern(broad_filter));
// `binds` fills every `?` in one pass; chained `bind`s would re-scan a `?` in the bound value
sqlb.and_where( sqlb.and_where(
"(resource.path ILIKE ? OR resource.description ILIKE ? OR resource_type ILIKE ? OR resource.value::text ILIKE ?)" "(resource.path ILIKE ? OR resource.description ILIKE ? OR resource_type ILIKE ? OR resource.value::text ILIKE ?)"
.bind(&pat).bind(&pat).bind(&pat).bind(&pat) .binds(&[&pat]),
); );
} }
@@ -455,10 +456,10 @@ async fn list_resources(
if let Some(label) = &lq.label { if let Some(label) = &lq.label {
for l in label.split(',') { for l in label.split(',') {
// `binds` fills every `?` in one pass; chained `bind`s would re-scan a `?` in the bound value
sqlb.and_where( sqlb.and_where(
"(resource.labels @> ARRAY[?] OR folder_labels(resource.workspace_id, resource.path) @> ARRAY[?])" "(resource.labels @> ARRAY[?] OR folder_labels(resource.workspace_id, resource.path) @> ARRAY[?])"
.bind(&l.trim()) .binds(&[&l.trim()]),
.bind(&l.trim()),
); );
} }
} }
+4 -3
View File
@@ -200,18 +200,19 @@ async fn list_variables(
if let Some(broad_filter) = &lq.broad_filter { if let Some(broad_filter) = &lq.broad_filter {
let pat = format!("%{}%", escape_ilike_pattern(broad_filter)); let pat = format!("%{}%", escape_ilike_pattern(broad_filter));
// `binds` fills every `?` in one pass; chained `bind`s would re-scan a `?` in the bound value
sqlb.and_where( sqlb.and_where(
"(variable.path ILIKE ? OR variable.description ILIKE ? OR (is_secret = FALSE AND variable.value ILIKE ?))" "(variable.path ILIKE ? OR variable.description ILIKE ? OR (is_secret = FALSE AND variable.value ILIKE ?))"
.bind(&pat).bind(&pat).bind(&pat) .binds(&[&pat]),
); );
} }
if let Some(label) = &lq.label { if let Some(label) = &lq.label {
for l in label.split(',') { for l in label.split(',') {
// `binds` fills every `?` in one pass; chained `bind`s would re-scan a `?` in the bound value
sqlb.and_where( sqlb.and_where(
"(variable.labels @> ARRAY[?] OR folder_labels(variable.workspace_id, variable.path) @> ARRAY[?])" "(variable.labels @> ARRAY[?] OR folder_labels(variable.workspace_id, variable.path) @> ARRAY[?])"
.bind(&l.trim()) .binds(&[&l.trim()]),
.bind(&l.trim()),
); );
} }
} }