mirror of
https://github.com/bahdotsh/wrkflw.git
synced 2026-05-18 05:05:35 +02:00
fix(executor): guard against empty container image and volume paths
It turns out that if someone writes `container:` with an empty image
string, we'd happily pass "" to Docker and let it figure out what
that means. Spoiler: it doesn't.
Similarly, volume specs like "/host:" or ":/container" would produce
a PathBuf::from("") mount, which is the kind of thing that makes
container runtimes *very* unhappy. Let's just skip those with a
warning instead of pretending they're valid.
While at it, replace the derived Serialize on ContainerCredentials
with a custom impl that redacts the password field. The Debug impl
was already doing this, but serde_json::to_string was still happily
dumping passwords in plaintext. Please don't do that.
This commit is contained in:
@@ -2215,7 +2215,12 @@ fn get_runner_image_from_opt(runs_on: &Option<Vec<String>>) -> String {
|
||||
|
||||
fn get_effective_runner_image(job: &Job) -> String {
|
||||
if let Some(ref container) = job.container {
|
||||
container.image.clone()
|
||||
if container.image.is_empty() {
|
||||
wrkflw_logging::warning("container image is empty, falling back to runs-on");
|
||||
get_runner_image_from_opt(&job.runs_on)
|
||||
} else {
|
||||
container.image.clone()
|
||||
}
|
||||
} else {
|
||||
get_runner_image_from_opt(&job.runs_on)
|
||||
}
|
||||
@@ -2287,6 +2292,13 @@ fn prepare_container_mounts(
|
||||
let parts: Vec<&str> = vol_spec.splitn(3, ':').collect();
|
||||
match parts.len() {
|
||||
3 => {
|
||||
if parts[0].is_empty() || parts[1].is_empty() {
|
||||
wrkflw_logging::warning(&format!(
|
||||
"skipping volume spec with empty host or container path: '{}'",
|
||||
vol_spec
|
||||
));
|
||||
continue;
|
||||
}
|
||||
wrkflw_logging::warning(&format!(
|
||||
"volume mount option '{}' in '{}' is not yet supported and will be ignored",
|
||||
parts[2], vol_spec
|
||||
@@ -2294,6 +2306,13 @@ fn prepare_container_mounts(
|
||||
owned_volume_paths.push((PathBuf::from(parts[0]), PathBuf::from(parts[1])));
|
||||
}
|
||||
2 => {
|
||||
if parts[0].is_empty() || parts[1].is_empty() {
|
||||
wrkflw_logging::warning(&format!(
|
||||
"skipping volume spec with empty host or container path: '{}'",
|
||||
vol_spec
|
||||
));
|
||||
continue;
|
||||
}
|
||||
owned_volume_paths.push((PathBuf::from(parts[0]), PathBuf::from(parts[1])));
|
||||
}
|
||||
_ => {
|
||||
@@ -3176,6 +3195,44 @@ mod tests {
|
||||
assert!(!step_env.contains_key("GITHUB_STEP_SUMMARY"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn effective_runner_image_empty_image_falls_back() {
|
||||
let job = make_job(
|
||||
Some(JobContainer {
|
||||
image: "".into(),
|
||||
credentials: None,
|
||||
env: HashMap::new(),
|
||||
ports: None,
|
||||
volumes: None,
|
||||
options: None,
|
||||
}),
|
||||
Some(vec!["ubuntu-latest".into()]),
|
||||
);
|
||||
let image = get_effective_runner_image(&job);
|
||||
// Should fall back to runs-on, not return empty string
|
||||
assert!(!image.is_empty());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn container_mounts_skips_empty_container_path() {
|
||||
let mut step_env = HashMap::new();
|
||||
let job_env = HashMap::new();
|
||||
|
||||
let container = JobContainer {
|
||||
image: "node:18".into(),
|
||||
credentials: None,
|
||||
env: HashMap::new(),
|
||||
ports: None,
|
||||
volumes: Some(vec!["/host:".into(), ":/container".into()]),
|
||||
options: None,
|
||||
};
|
||||
|
||||
let (volumes, _) = prepare_container_mounts(&mut step_env, &job_env, Some(&container));
|
||||
|
||||
// Both specs have an empty path component and should be skipped
|
||||
assert!(volumes.is_empty());
|
||||
}
|
||||
|
||||
// --- container env precedence tests ---
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -73,7 +73,7 @@ where
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Deserialize, Serialize, Clone)]
|
||||
#[derive(Deserialize, Clone)]
|
||||
pub struct ContainerCredentials {
|
||||
#[serde(default)]
|
||||
pub username: Option<String>,
|
||||
@@ -81,6 +81,23 @@ pub struct ContainerCredentials {
|
||||
pub password: Option<String>,
|
||||
}
|
||||
|
||||
impl serde::Serialize for ContainerCredentials {
|
||||
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
|
||||
where
|
||||
S: serde::Serializer,
|
||||
{
|
||||
use serde::ser::SerializeStruct;
|
||||
let mut state = serializer.serialize_struct("ContainerCredentials", 2)?;
|
||||
state.serialize_field("username", &self.username)?;
|
||||
if self.password.is_some() {
|
||||
state.serialize_field("password", &"[REDACTED]")?;
|
||||
} else {
|
||||
state.serialize_field("password", &None::<String>)?;
|
||||
}
|
||||
state.end()
|
||||
}
|
||||
}
|
||||
|
||||
impl std::fmt::Debug for ContainerCredentials {
|
||||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
|
||||
f.debug_struct("ContainerCredentials")
|
||||
@@ -575,4 +592,27 @@ jobs:
|
||||
let container = job.container.as_ref().unwrap();
|
||||
assert_eq!(container.image, "ghcr.io/owner/image:latest");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn container_credentials_serialize_redacts_password() {
|
||||
let creds = ContainerCredentials {
|
||||
username: Some("user".into()),
|
||||
password: Some("super-secret".into()),
|
||||
};
|
||||
let json = serde_json::to_string(&creds).unwrap();
|
||||
assert!(json.contains("user"));
|
||||
assert!(json.contains("[REDACTED]"));
|
||||
assert!(!json.contains("super-secret"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn container_credentials_serialize_null_password() {
|
||||
let creds = ContainerCredentials {
|
||||
username: Some("user".into()),
|
||||
password: None,
|
||||
};
|
||||
let json = serde_json::to_string(&creds).unwrap();
|
||||
assert!(json.contains("user"));
|
||||
assert!(!json.contains("[REDACTED]"));
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user