mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-27 00:02:35 +00:00
Merge commit from fork
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
8af484292f
commit
812ff6ea5e
@@ -1 +1 @@
|
||||
b6e23d229966cf0019c0a1926a254fda658d8def
|
||||
5dfdf811c862c689e42f03f485e02f9f1e7bc7ca
|
||||
|
||||
@@ -134,6 +134,17 @@ async fn list_search_flows(
|
||||
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(
|
||||
authed: ApiAuthed,
|
||||
Extension(user_db): Extension<UserDB>,
|
||||
@@ -217,11 +228,7 @@ async fn list_flows(
|
||||
}
|
||||
if let Some(label) = &lq.label {
|
||||
for l in label.split(',') {
|
||||
sqlb.and_where(
|
||||
"(o.labels @> ARRAY[?] OR folder_labels(o.workspace_id, o.path) @> ARRAY[?])"
|
||||
.bind(&l.trim())
|
||||
.bind(&l.trim()),
|
||||
);
|
||||
sqlb.and_where(label_condition(l));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2284,6 +2291,31 @@ mod tests {
|
||||
|
||||
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]
|
||||
fn flowmodule_serde() {
|
||||
let fv = FlowValue {
|
||||
|
||||
@@ -406,62 +406,3 @@ async fn test_operators_draft_resources_and_variables(db: Pool<Postgres>) -> any
|
||||
|
||||
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(())
|
||||
}
|
||||
|
||||
@@ -268,9 +268,10 @@ pub fn filter_list_queue_query(
|
||||
|
||||
if let Some(bf) = &lq.broad_filter {
|
||||
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(
|
||||
"(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 {
|
||||
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 {
|
||||
@@ -591,10 +592,11 @@ pub fn filter_list_completed_query(
|
||||
|
||||
if let Some(bf) = &lq.broad_filter {
|
||||
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(
|
||||
"(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 ?))"
|
||||
.bind(&pat).bind(&pat).bind(&pat).bind(&pat).bind(&pat)
|
||||
.binds(&[&pat]),
|
||||
);
|
||||
}
|
||||
|
||||
@@ -945,6 +947,20 @@ mod tests {
|
||||
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 ---
|
||||
|
||||
#[test]
|
||||
@@ -1124,4 +1140,19 @@ mod tests {
|
||||
let sql = build_sql(sqlb);
|
||||
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 ?"));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -894,17 +894,18 @@ async fn list_schedule(
|
||||
}
|
||||
if let Some(broad_filter) = &lsq.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(
|
||||
"(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 {
|
||||
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(
|
||||
"(labels @> ARRAY[?] OR folder_labels(workspace_id, path) @> ARRAY[?])"
|
||||
.bind(&l.trim())
|
||||
.bind(&l.trim()),
|
||||
.binds(&[&l.trim()]),
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -309,10 +309,10 @@ async fn list_scripts(
|
||||
}
|
||||
if let Some(label) = &lq.label {
|
||||
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(
|
||||
"(o.labels @> ARRAY[?] OR folder_labels(o.workspace_id, o.path) @> ARRAY[?])"
|
||||
.bind(&l.trim())
|
||||
.bind(&l.trim()),
|
||||
.binds(&[&l.trim()]),
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -3366,10 +3366,6 @@ pub(crate) async fn resolve_pg_source_checked_with_kind(
|
||||
|
||||
let pg: PgDatabase = serde_json::from_value(db_resource)
|
||||
.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))
|
||||
}
|
||||
|
||||
@@ -3459,20 +3455,21 @@ pub(crate) struct PgDumpOptions<'a> {
|
||||
pub(crate) no_acl: bool,
|
||||
}
|
||||
|
||||
/// A pg_dump invocation against `pg_db` writing to `dump_path`, carrying the connection settings
|
||||
/// the CLI reads from the environment. The returned file, when there is one, holds the root
|
||||
/// certificate the command points at and must outlive it.
|
||||
fn pg_dump_command(
|
||||
/// 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<'_>,
|
||||
dump_path: &std::path::Path,
|
||||
) -> Result<(tokio::process::Command, Option<DumpFile>)> {
|
||||
) -> Result<DumpFile> {
|
||||
let dump_file = DumpFile::new()?;
|
||||
|
||||
let host = &pg_db.host;
|
||||
let port = pg_db.port.unwrap_or(5432).to_string();
|
||||
let user = pg_db.login_name();
|
||||
let dbname = &pg_db.dbname;
|
||||
|
||||
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 {
|
||||
cmd.arg("--schema-only");
|
||||
}
|
||||
@@ -3490,31 +3487,13 @@ fn pg_dump_command(
|
||||
.arg("--port")
|
||||
.arg(&port)
|
||||
.arg("--username")
|
||||
.arg(user);
|
||||
|
||||
// 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);
|
||||
.arg(user)
|
||||
.arg(dbname);
|
||||
|
||||
if let Some(ref password) = pg_db.password {
|
||||
cmd.env("PGPASSWORD", password);
|
||||
}
|
||||
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 _root_cert = apply_pg_tls_env(&mut cmd, pg_db)?;
|
||||
|
||||
let output = cmd
|
||||
.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("--username")
|
||||
.arg(pg_db.login_name())
|
||||
.arg("--dbname")
|
||||
.arg(&pg_db.dbname)
|
||||
.arg("--no-psqlrc")
|
||||
.stdout(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 {
|
||||
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.
|
||||
///
|
||||
/// 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(())
|
||||
}
|
||||
|
||||
/// 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
|
||||
async fn import_pg_database(
|
||||
authed: ApiAuthed,
|
||||
@@ -4202,8 +4069,6 @@ async fn import_pg_database(
|
||||
Path(w_id): Path<String>,
|
||||
Json(req): Json<ImportPgDatabaseRequest>,
|
||||
) -> Result<String> {
|
||||
ensure_can_dump_databases(&db, &w_id, &authed, "import a database").await?;
|
||||
|
||||
if req.fork_behavior == DataTableForkBehavior::KeepOriginal {
|
||||
return Ok("No action needed for KeepOriginal behavior".to_string());
|
||||
}
|
||||
@@ -4320,8 +4185,6 @@ async fn export_pg_schema(
|
||||
Path(w_id): Path<String>,
|
||||
Json(req): Json<ExportPgSchemaRequest>,
|
||||
) -> 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 dump_file = pg_dump_database(
|
||||
&pg,
|
||||
|
||||
@@ -740,10 +740,10 @@ async fn list_apps(
|
||||
|
||||
if let Some(label) = &lq.label {
|
||||
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(
|
||||
"(app.labels @> ARRAY[?] OR folder_labels(app.workspace_id, app.path) @> ARRAY[?])"
|
||||
.bind(&l.trim())
|
||||
.bind(&l.trim()),
|
||||
.binds(&[&l.trim()]),
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -183,8 +183,8 @@ fn items_query(
|
||||
let mut sqlb = SqlBuilder::select_from(&format!("{} as o", item_type));
|
||||
if scope_type == "favorites" {
|
||||
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)
|
||||
.bind(&authed.username));
|
||||
.on("favorite.favorite_kind = ? AND favorite.workspace_id = o.workspace_id AND favorite.path = o.path AND favorite.usr = ?"
|
||||
.binds(&[&item_type, &authed.username]));
|
||||
}
|
||||
sqlb.and_where("o.workspace_id = ?".bind(&workspace_id))
|
||||
.and_where("o.archived = false");
|
||||
|
||||
@@ -98,10 +98,10 @@ async fn list_apps(
|
||||
|
||||
if let Some(label) = &lq.label {
|
||||
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(
|
||||
"(app.labels @> ARRAY[?] OR folder_labels(app.workspace_id, app.path) @> ARRAY[?])"
|
||||
.bind(&l.trim())
|
||||
.bind(&l.trim()),
|
||||
.binds(&[&l.trim()]),
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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)]
|
||||
mod pg_tls_tests {
|
||||
use super::PgDatabase;
|
||||
@@ -1639,26 +1595,6 @@ pub fn validate_dbname(dbname: &str) -> error::Result<()> {
|
||||
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
|
||||
/// 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.
|
||||
|
||||
@@ -439,9 +439,10 @@ async fn list_resources(
|
||||
|
||||
if let Some(broad_filter) = &lq.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(
|
||||
"(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 {
|
||||
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(
|
||||
"(resource.labels @> ARRAY[?] OR folder_labels(resource.workspace_id, resource.path) @> ARRAY[?])"
|
||||
.bind(&l.trim())
|
||||
.bind(&l.trim()),
|
||||
.binds(&[&l.trim()]),
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -200,18 +200,19 @@ async fn list_variables(
|
||||
|
||||
if let Some(broad_filter) = &lq.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(
|
||||
"(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 {
|
||||
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(
|
||||
"(variable.labels @> ARRAY[?] OR folder_labels(variable.workspace_id, variable.path) @> ARRAY[?])"
|
||||
.bind(&l.trim())
|
||||
.bind(&l.trim()),
|
||||
.binds(&[&l.trim()]),
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user