From 5d5e03cebae4aaa8ac00a8d52ba4b48b259ebc6e Mon Sep 17 00:00:00 2001 From: nicomiguelino Date: Mon, 31 Aug 2026 09:54:49 -0700 Subject: [PATCH 1/3] Guard create/create-in-place against EDGE_APP_ID, warn on stale id after delete edge-app create and edge-app create --in-place now refuse to run while EDGE_APP_ID is set, since both always create a brand new Edge App and would otherwise leave the env var pointing at an app the user didn't mean to create. edge-app delete still clears id from screenly.yml via clear_app_id, but now also prints a warning if EDGE_APP_ID is set afterward, since it can't be cleared from the environment and would otherwise silently keep resolving to the deleted app. --- docs/EdgeApps.md | 6 ++- src/cli.rs | 6 +++ src/commands/edge_app/app.rs | 89 +++++++++++++++++++++++++++++++++--- 3 files changed, 94 insertions(+), 7 deletions(-) diff --git a/docs/EdgeApps.md b/docs/EdgeApps.md index 96e20b39..2be4c909 100644 --- a/docs/EdgeApps.md +++ b/docs/EdgeApps.md @@ -294,7 +294,11 @@ The `syntax` field specifies the version of the manifest file. The current versi The `id` field is a unique identifier for the Edge App. This ID is generated by the system and is used to identify the Edge App across the platform. -> **Deprecated:** Reading the `id` from `screenly.yml` is deprecated. Set the `EDGE_APP_ID` environment variable instead — for example, via a repo secret in CI or a local `.env` file loaded into your shell. When `EDGE_APP_ID` is set, it takes priority over the manifest's `id` field. The CLI still falls back to reading `id` from `screenly.yml` for now, but prints a deprecation warning each time it does. +> **Deprecated:** Reading the `id` from `screenly.yml` is deprecated. Set the `EDGE_APP_ID` environment variable instead, for example via a repo secret in CI or a local `.env` file loaded into your shell. When `EDGE_APP_ID` is set, it takes priority over the manifest's `id` field. The CLI still falls back to reading `id` from `screenly.yml` for now, but prints a deprecation warning each time it does. +> +> `screenly edge-app create` and `screenly edge-app create --in-place` refuse to run while `EDGE_APP_ID` is set, since both commands always create a brand new Edge App and would otherwise leave the configured id pointing at an app you didn't mean to create. Unset `EDGE_APP_ID` first if you want to create a new app, or use `screenly edge-app deploy` if you meant to deploy to the existing one. +> +> `screenly edge-app delete` clears the `id` field from `screenly.yml`, but it can't clear `EDGE_APP_ID` from your environment. If it's still set, subsequent commands keep resolving to the deleted app's id until you unset or update it yourself; the CLI prints a warning after deletion as a reminder. #### Entrypoint diff --git a/src/cli.rs b/src/cli.rs index 1509aa2d..ee4c1ec6 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -11,6 +11,7 @@ use thiserror::Error; use crate::authentication::{verify_and_store_token, Authentication, AuthenticationError, Config}; use crate::commands; +use crate::commands::edge_app::app::{edge_app_id_from_env, EDGE_APP_ID_ENV}; use crate::commands::edge_app::instance_manifest::InstanceManifest; use crate::commands::edge_app::manifest::EdgeAppManifest; use crate::commands::edge_app::server::MOCK_DATA_FILENAME; @@ -996,6 +997,11 @@ pub fn handle_cli_edge_app_command(command: &EdgeAppCommands, output: OutputForm std::process::exit(1); } } + + if let Some(id) = edge_app_id_from_env() { + eprintln!("Warning: the {EDGE_APP_ID_ENV} environment variable is still set to \"{id}\". Commands will keep using that id until you unset or update it."); + } + std::process::exit(0); } Err(e) => { diff --git a/src/commands/edge_app/app.rs b/src/commands/edge_app/app.rs index c5000dd9..917dceb1 100644 --- a/src/commands/edge_app/app.rs +++ b/src/commands/edge_app/app.rs @@ -29,10 +29,21 @@ use crate::commands::edge_app::utils::{ use crate::commands::edge_app::EdgeAppCommand; use crate::commands::{CommandError, EdgeApps}; -const EDGE_APP_ID_ENV: &str = "EDGE_APP_ID"; +pub const EDGE_APP_ID_ENV: &str = "EDGE_APP_ID"; pub const INJECT_JS_FILE_NAME: &str = "screenly_inject.js"; +pub fn edge_app_id_from_env() -> Option { + std::env::var(EDGE_APP_ID_ENV).ok().and_then(|id| { + let trimmed = id.trim(); + if trimmed.is_empty() { + None + } else { + Some(trimmed.to_string()) + } + }) +} + // Edge apps commands impl EdgeAppCommand { pub fn create( @@ -46,6 +57,12 @@ impl EdgeAppCommand { ))?; let index_html_path = parent_dir_path.join("index.html"); + if let Some(id) = edge_app_id_from_env() { + return Err(CommandError::InitializationError(format!( + "An Edge App id is already configured via the {EDGE_APP_ID_ENV} environment variable ({id}). 'edge-app create' always creates a new Edge App; unset {EDGE_APP_ID_ENV} first if you want to create a new one, or use 'edge-app deploy' if you meant to deploy to the existing app." + ))); + } + if Path::new(&path).exists() || Path::new(&index_html_path).exists() { return Err(CommandError::FileSystemError(format!( "The directory {} already contains a screenly.yml or index.html file. Use --in-place if you want to create an Edge App in this directory", @@ -187,6 +204,12 @@ impl EdgeAppCommand { let data = fs::read_to_string(path)?; let mut manifest: EdgeAppManifest = serde_yaml::from_str(&data)?; + if let Some(id) = edge_app_id_from_env() { + return Err(CommandError::InitializationError(format!( + "An Edge App id is already configured via the {EDGE_APP_ID_ENV} environment variable ({id}). The operation can only proceed when no Edge App id is already configured; unset {EDGE_APP_ID_ENV} first if you want to create a new Edge App, or use 'edge-app deploy' if you meant to deploy to the existing app." + ))); + } + if manifest.id.is_some() { return Err(CommandError::InitializationError("The operation can only proceed when 'id' is not set in the 'screenly.yml' configuration file".to_string())); } @@ -701,11 +724,8 @@ impl EdgeAppCommand { } pub fn get_app_id(&self, path: Option) -> Result { - if let Ok(id) = std::env::var(EDGE_APP_ID_ENV) { - let id = id.trim(); - if !id.is_empty() { - return Ok(id.to_string()); - } + if let Some(id) = edge_app_id_from_env() { + return Ok(id); } let edge_app_manifest = EdgeAppManifest::new(&transform_edge_app_path_to_manifest(&path)?)?; @@ -1747,6 +1767,63 @@ mod tests { assert_eq!(app_id.unwrap(), manifest.unwrap().id.unwrap()); } + #[test] + fn test_edge_app_create_when_env_var_is_set_should_return_error() { + let (tmp_dir, command, _mock_server, _manifest, _instance_manifest) = + prepare_edge_apps_test(false, false); + + let _lock = lock_test(); + let _env = set_env( + OsString::from(EDGE_APP_ID_ENV), + "01ENVOVERRIDEXXXXXXXXXXXXX", + ); + let result = command.create( + "Best app ever", + tmp_dir.path().join("screenly.yml").as_path(), + None, + ); + + assert!(result.is_err()); + assert!(result + .unwrap_err() + .to_string() + .contains("EDGE_APP_ID environment variable (01ENVOVERRIDEXXXXXXXXXXXXX)")); + assert!(!tmp_dir.path().join("screenly.yml").exists()); + } + + #[test] + fn test_create_in_place_edge_app_when_env_var_is_set_should_return_error() { + let (tmp_dir, command, _mock_server, _manifest, _instance_manifest) = + prepare_edge_apps_test(false, false); + + File::create(tmp_dir.path().join("index.html")).unwrap(); + + let manifest = EdgeAppManifest { + id: None, + syntax: MANIFEST_VERSION.to_owned(), + ..Default::default() + }; + + EdgeAppManifest::save_to_file(&manifest, tmp_dir.path().join("screenly.yml").as_path()) + .unwrap(); + + let _lock = lock_test(); + let _env = set_env( + OsString::from(EDGE_APP_ID_ENV), + "01ENVOVERRIDEXXXXXXXXXXXXX", + ); + let result = command.create_in_place( + "Best app ever", + tmp_dir.path().join("screenly.yml").as_path(), + ); + + assert!(result.is_err()); + assert!(result + .unwrap_err() + .to_string() + .contains("EDGE_APP_ID environment variable (01ENVOVERRIDEXXXXXXXXXXXXX)")); + } + #[test] fn test_clear_app_id_should_remove_app_id_from_manifest() { let (temp_dir, command, _mock_server, _manifest, _instance_manifest) = From d23ae275acc6e6946eb2d1923a4daa0a058e2741 Mon Sep 17 00:00:00 2001 From: nicomiguelino Date: Mon, 31 Aug 2026 10:18:16 -0700 Subject: [PATCH 2/3] Narrow EDGE_APP_ID helpers to pub(crate), check env var before manifest I/O EDGE_APP_ID_ENV and edge_app_id_from_env() are only used within this crate, so pub unnecessarily expanded the public API surface. In create_in_place, the EDGE_APP_ID guard now runs before reading and parsing screenly.yml, so a malformed manifest doesn't surface a YAML error instead of the intended "refuse to run while EDGE_APP_ID is set" behavior. Added a regression test with deliberately invalid YAML. --- src/commands/edge_app/app.rs | 40 +++++++++++++++++++++++++++++++----- 1 file changed, 35 insertions(+), 5 deletions(-) diff --git a/src/commands/edge_app/app.rs b/src/commands/edge_app/app.rs index 917dceb1..4919afb0 100644 --- a/src/commands/edge_app/app.rs +++ b/src/commands/edge_app/app.rs @@ -29,11 +29,11 @@ use crate::commands::edge_app::utils::{ use crate::commands::edge_app::EdgeAppCommand; use crate::commands::{CommandError, EdgeApps}; -pub const EDGE_APP_ID_ENV: &str = "EDGE_APP_ID"; +pub(crate) const EDGE_APP_ID_ENV: &str = "EDGE_APP_ID"; pub const INJECT_JS_FILE_NAME: &str = "screenly_inject.js"; -pub fn edge_app_id_from_env() -> Option { +pub(crate) fn edge_app_id_from_env() -> Option { std::env::var(EDGE_APP_ID_ENV).ok().and_then(|id| { let trimmed = id.trim(); if trimmed.is_empty() { @@ -201,15 +201,15 @@ impl EdgeAppCommand { ))); } - let data = fs::read_to_string(path)?; - let mut manifest: EdgeAppManifest = serde_yaml::from_str(&data)?; - if let Some(id) = edge_app_id_from_env() { return Err(CommandError::InitializationError(format!( "An Edge App id is already configured via the {EDGE_APP_ID_ENV} environment variable ({id}). The operation can only proceed when no Edge App id is already configured; unset {EDGE_APP_ID_ENV} first if you want to create a new Edge App, or use 'edge-app deploy' if you meant to deploy to the existing app." ))); } + let data = fs::read_to_string(path)?; + let mut manifest: EdgeAppManifest = serde_yaml::from_str(&data)?; + if manifest.id.is_some() { return Err(CommandError::InitializationError("The operation can only proceed when 'id' is not set in the 'screenly.yml' configuration file".to_string())); } @@ -1824,6 +1824,36 @@ mod tests { .contains("EDGE_APP_ID environment variable (01ENVOVERRIDEXXXXXXXXXXXXX)")); } + #[test] + fn test_create_in_place_edge_app_when_env_var_is_set_and_manifest_is_malformed_should_still_return_env_var_error( + ) { + let (tmp_dir, command, _mock_server, _manifest, _instance_manifest) = + prepare_edge_apps_test(false, false); + + File::create(tmp_dir.path().join("index.html")).unwrap(); + fs::write( + tmp_dir.path().join("screenly.yml"), + "not: [valid, yaml: manifest", + ) + .unwrap(); + + let _lock = lock_test(); + let _env = set_env( + OsString::from(EDGE_APP_ID_ENV), + "01ENVOVERRIDEXXXXXXXXXXXXX", + ); + let result = command.create_in_place( + "Best app ever", + tmp_dir.path().join("screenly.yml").as_path(), + ); + + assert!(result.is_err()); + assert!(result + .unwrap_err() + .to_string() + .contains("EDGE_APP_ID environment variable (01ENVOVERRIDEXXXXXXXXXXXXX)")); + } + #[test] fn test_clear_app_id_should_remove_app_id_from_manifest() { let (temp_dir, command, _mock_server, _manifest, _instance_manifest) = From f2b1cbde3199b7f48168900b402f033f4669936b Mon Sep 17 00:00:00 2001 From: nicomiguelino Date: Mon, 31 Aug 2026 10:55:33 -0700 Subject: [PATCH 3/3] Fix double period and use full command names in EDGE_APP_ID guard messages The trailing period in both messages doubled up with the one cli.rs's error handler already appends, producing "...existing app..". Dropped it to match the existing convention of CommandError messages not including their own terminal punctuation. Also swapped the shorthand 'edge-app create'/'edge-app deploy' for the actual, copy-pasteable 'screenly edge-app create'/'screenly edge-app deploy' invocations. --- src/commands/edge_app/app.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/commands/edge_app/app.rs b/src/commands/edge_app/app.rs index 55eb550a..306189b3 100644 --- a/src/commands/edge_app/app.rs +++ b/src/commands/edge_app/app.rs @@ -59,7 +59,7 @@ impl EdgeAppCommand { if let Some(id) = edge_app_id_from_env() { return Err(CommandError::InitializationError(format!( - "An Edge App id is already configured via the {EDGE_APP_ID_ENV} environment variable ({id}). 'edge-app create' always creates a new Edge App; unset {EDGE_APP_ID_ENV} first if you want to create a new one, or use 'edge-app deploy' if you meant to deploy to the existing app." + "An Edge App id is already configured via the {EDGE_APP_ID_ENV} environment variable ({id}). 'screenly edge-app create' always creates a new Edge App; unset {EDGE_APP_ID_ENV} first if you want to create a new one, or use 'screenly edge-app deploy' if you meant to deploy to the existing app" ))); } @@ -203,7 +203,7 @@ impl EdgeAppCommand { if let Some(id) = edge_app_id_from_env() { return Err(CommandError::InitializationError(format!( - "An Edge App id is already configured via the {EDGE_APP_ID_ENV} environment variable ({id}). The operation can only proceed when no Edge App id is already configured; unset {EDGE_APP_ID_ENV} first if you want to create a new Edge App, or use 'edge-app deploy' if you meant to deploy to the existing app." + "An Edge App id is already configured via the {EDGE_APP_ID_ENV} environment variable ({id}). The operation can only proceed when no Edge App id is already configured; unset {EDGE_APP_ID_ENV} first if you want to create a new Edge App, or use 'screenly edge-app deploy' if you meant to deploy to the existing app" ))); }