fix: validate app and trigger paths, refuse traversal in workspace export (#11311)

* fix: enforce proper_id paths on apps and triggers, refuse traversal in export

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

* fix: skip proper_id on tables already holding non-conforming paths

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

* test: pin archive entry path traversal guard

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

* fix: refuse windows-normalized traversal in export, check raw app path early

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

* fix: only treat a colon in the first archive segment as a drive prefix

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

* test: accept a colon past the first archive segment

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

* revert: drop proper_id migration, keep path validation in the API

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

* fix: validate paths in bulk http trigger creation

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Ruben Fiszel
2026-09-23 15:50:32 +02:00
committed by GitHub
co-authored by Claude Opus 5.5
parent 1de54eeea4
commit 7593597617
5 changed files with 75 additions and 3 deletions
+6 -1
View File
@@ -69,7 +69,7 @@ use windmill_common::{
user_drafts::{overlay_or_draft_only, DraftUserRef, UserDraftItemKind, WithDraftOverlay},
users::username_to_permissioned_as,
utils::{
http_get_from_hub, not_found_if_none, paginate, paginate_optional,
check_proper_path, http_get_from_hub, not_found_if_none, paginate, paginate_optional,
query_elems_from_hub, require_admin, strip_json_nul, Pagination, RunnableKind, StripPath,
},
variables::{build_crypt, build_crypt_with_key_suffix, encrypt},
@@ -2506,6 +2506,7 @@ async fn create_app_internal<'a>(
// inside process_app_multipart!, so checking after this call would leave a
// denied app committed in the DB.
check_scopes(&authed, || format!("apps:write:{}", &app.path))?;
check_proper_path(&app.path)?;
validate_frontend_sdk_scopes(&app.policy)?;
if raw_app {
validate_raw_app_path_keys(&app.value.0)?;
@@ -3252,6 +3253,7 @@ async fn create_app_raw_source(
// Before the compile, which costs a job on a worker: it must not run for a
// path already taken, nor for one the caller can't write. `create_app_internal`
// rejects both, but only after the sources have been built.
check_proper_path(&path)?;
if app_exists(&db, &w_id, &path).await? {
return Err(Error::BadRequest(format!("App {path} already exists")));
}
@@ -3462,6 +3464,9 @@ async fn update_app_internal<'a>(
// the token's write scope, not just the source path.
if let Some(npath) = ns.path.as_deref() {
check_scopes(&authed, || format!("apps:write:{}", npath))?;
if npath != path {
check_proper_path(npath)?;
}
}
if raw_app {
+59 -1
View File
@@ -281,8 +281,29 @@ enum ArchiveImpl {
Tar(tokio_tar::Builder<File>),
}
/// Entry paths come from item paths stored in the workspace; an absolute or
/// drive-prefixed path, or a parent segment, would make extraction write outside
/// the target directory. Win32 strips trailing dots and spaces from a segment,
/// so `.. ` resolves to `..` there: any segment made only of those is refused.
/// A colon matters only in the first segment (a drive prefix); later segments,
/// such as a data table name, may carry one.
fn check_archive_entry_path(path: &str) -> Result<()> {
let is_dot_segment = |seg: &str| !seg.is_empty() && seg.chars().all(|c| c == '.' || c == ' ');
let first = path.split(['/', '\\']).next().unwrap_or_default();
if path.starts_with(['/', '\\'])
|| first.contains(':')
|| path.split(['/', '\\']).any(is_dot_segment)
{
return Err(Error::internal_err(format!(
"refusing to write archive entry with path traversal: {path}"
)));
}
Ok(())
}
impl ArchiveImpl {
async fn write_to_archive(&mut self, content: &str, path: &str) -> Result<()> {
check_archive_entry_path(path)?;
match self {
ArchiveImpl::Tar(t) => {
let bytes = content.as_bytes();
@@ -1647,7 +1668,9 @@ pub(crate) async fn tarball_workspace(
mute_critical_alerts: row.mute_critical_alerts,
color: row.color.clone(),
operator_settings: row.operator_settings.clone(),
datatable: windmill_common::workspaces::strip_datatable_permissions(row.datatable.clone()),
datatable: windmill_common::workspaces::strip_datatable_permissions(
row.datatable.clone(),
),
slack_team_id: row.slack_team_id.clone(),
slack_name: row.slack_name.clone(),
slack_command_script: row.slack_command_script.clone(),
@@ -1792,6 +1815,41 @@ pub(crate) async fn tarball_workspace(
Ok((headers, body))
}
#[cfg(test)]
mod archive_entry_path_tests {
use super::check_archive_entry_path;
#[test]
fn rejects_traversal_and_absolute_paths() {
for ok in [
"u/admin/app.app.json",
"f/x/a..b.script.json",
"settings.yaml",
"migrations/datatable/foo:bar/20260617120000_name.up.sql",
] {
assert!(
check_archive_entry_path(ok).is_ok(),
"{ok} should be accepted"
);
}
for bad in [
"f/x/../../evil.app.json",
"../evil",
"/etc/evil",
"f\\..\\evil",
"f/x/.. /.. /evil.app.json",
"f/x/.../evil",
"C:/evil",
"\\evil",
] {
assert!(
check_archive_entry_path(bad).is_err(),
"{bad} should be rejected"
);
}
}
}
#[cfg(test)]
mod fork_export_tests {
use super::*;
+3
View File
@@ -245,6 +245,9 @@ lazy_static::lazy_static! {
/// Reject a path the `proper_id` constraint would reject anyway, so the caller
/// gets a plain 400 instead of the raw Postgres constraint-violation string,
/// which names the table and constraint and echoes the input back.
///
/// `app` and the trigger tables have no such constraint: for them this check is
/// the only authority, so loosening it lets malformed paths be stored.
pub fn check_proper_path(path: &str) -> Result<()> {
// The column is varchar(255); without this an over-long but well-formed path
// still reaches Postgres and leaks the same kind of message back.
@@ -13,6 +13,7 @@ use windmill_common::global_settings::{validate_allowed_origins, HTTP_ROUTE_WORK
use windmill_common::{
db::UserDB,
error::{Error, Result},
utils::check_proper_path,
worker::CLOUD_HOSTED,
DB,
};
@@ -269,6 +270,7 @@ pub async fn create_many_http_triggers(
check_scopes(&authed, || {
format!("http_triggers:write:{}", &new_http_trigger.base.path)
})?;
check_proper_path(&new_http_trigger.base.path)?;
// This route inserts directly, bypassing the shared create handler.
// `error_wrapper` would turn the rejection into a 500.
+5 -1
View File
@@ -22,7 +22,7 @@ use windmill_common::{
fetch_draft_only_list_rows, overlay_or_draft_only, UserDraftItemKind, WithDraftOverlay,
WithDraftQuery,
},
utils::{paginate, Pagination, StripPath},
utils::{check_proper_path, paginate, Pagination, StripPath},
worker::CLOUD_HOSTED,
DB,
};
@@ -547,6 +547,7 @@ async fn create_trigger<T: TriggerCrud>(
&new_trigger.base.path
)
})?;
check_proper_path(&new_trigger.base.path)?;
if *CLOUD_HOSTED && !T::IS_ALLOWED_ON_CLOUD {
return Err(Error::BadRequest(format!(
@@ -817,6 +818,9 @@ async fn update_trigger<T: TriggerCrud>(
&edit_trigger.base.path
)
})?;
if edit_trigger.base.path != path {
check_proper_path(&edit_trigger.base.path)?;
}
edit_trigger.error_handling.validate()?;