From a847eec52a33879546e67c3c628d3a26174d219c Mon Sep 17 00:00:00 2001 From: Mike Garde Date: Thu, 6 Aug 2026 13:53:13 -0400 Subject: [PATCH] Support export-prefixed env vars and parsing - Add EnvValue.exported flag (default false) and propagate to scalar/list values; initialize in all constructors. - Implement robust multi-line list parsing with bracket-depth, export prefix support, key validation, and error handling; introduce helpers and propagate exported flag through values. - Centralize and preserve export-prefix handling in set_value to keep rewritten files sourceable. - Replace legacy end-line detection with the new parsing approach. - Extend release notes with a Docker installation snippet and arch-aware dotenv-cli download logic. - Add regression tests for complex lists, trailing comments, export semantics, and export-preservation behavior across set/delete flows. --- Taskfile.yaml | 1 + devops/render-release-notes.sh | 13 +++ src/env_object.rs | 3 + src/env_parser.rs | 167 +++++++++++++++++++++++------- src/handlers/set_value.rs | 15 ++- tests/env_parser.rs | 183 +++++++++++++++++++++++++++++++++ tests/set_and_delete.rs | 83 +++++++++++++++ 7 files changed, 424 insertions(+), 41 deletions(-) diff --git a/Taskfile.yaml b/Taskfile.yaml index 46705f3..45dc983 100644 --- a/Taskfile.yaml +++ b/Taskfile.yaml @@ -114,6 +114,7 @@ tasks: cmds: - task: test - bash devops/release.sh {{.VERSION}} + - echo "Don't forget \033[0;32mcargo publish" publish:npm: desc: "Manually publish the current version to npmjs (normally handled by GitHub Actions)" diff --git a/devops/render-release-notes.sh b/devops/render-release-notes.sh index 8a8005c..279fcd3 100755 --- a/devops/render-release-notes.sh +++ b/devops/render-release-notes.sh @@ -26,6 +26,19 @@ brew install mikegarde/tap/dotenv-cli npm install -g @mikegarde/dotenv-cli \`\`\` +### Docker + +\`\`\`bash +ARG TARGETARCH +ARG DOTENV_CLI_VERSION=${VERSION} + +RUN ARCH=$([ "$TARGETARCH" = "arm64" ] && echo "aarch64" || echo "x86_64") \ + && curl -fsSL \ + "https://github.com/MikeGarde/dotenv-cli/releases/download/${DOTENV_CLI_VERSION}/dotenv-cli-${DOTENV_CLI_VERSION}-unknown-linux-gnu-${ARCH}.gz" \ + | gzip -d > /usr/local/bin/dotenv \ + && chmod +x /usr/local/bin/dotenv +\`\`\` + ### Manual RHEL x86 diff --git a/src/env_object.rs b/src/env_object.rs index 4eca750..be2ad7b 100644 --- a/src/env_object.rs +++ b/src/env_object.rs @@ -9,6 +9,7 @@ pub struct EnvValue { pub line_start: i64, pub line_end: i64, pub no_expand: bool, + pub exported: bool, } impl EnvValue { @@ -19,6 +20,7 @@ impl EnvValue { line_start: -1, line_end: -1, no_expand: false, + exported: false, } } @@ -28,6 +30,7 @@ impl EnvValue { line_start, line_end, no_expand: false, + exported: false, } } } diff --git a/src/env_parser.rs b/src/env_parser.rs index e4102ae..570cc31 100644 --- a/src/env_parser.rs +++ b/src/env_parser.rs @@ -3,17 +3,84 @@ use std::fs; use crate::env_object::{EnvObject, EnvValue}; use crate::errors::EnvParseError; -/// Walk forward from `start` until a line whose trimmed form ends with `end_str`. -/// Returns the index of that line, capped at the last line index. -fn get_end_line(lines: &[&str], start: usize, end_str: &str) -> usize { - let mut end = start; - while end < lines.len() && !lines[end].trim().ends_with(end_str) { - if end + 1 >= lines.len() { - break; +/// Find the line and byte offset of the `]` that closes a JSON array opened by +/// the `[` at `open_pos` on `lines[start]`. +/// +/// Counts bracket depth rather than looking for a `]` by position, so nested +/// arrays close on the right bracket and a `]` inside a JSON string (`["a]b"]`) +/// is skipped. String state carries across lines so a multi-line array scans as +/// one blob. Locating the close structurally is what lets a trailing comment be +/// dropped safely — including one that itself contains a bracket, as in +/// `LIST=["a"] # see note [1]`, which a scan for the *last* `]` on the line got +/// wrong. +/// +/// Errors instead of scanning to EOF if the array never closes, so an +/// unterminated list fails loudly rather than absorbing the rest of the file. +fn find_list_close( + lines: &[&str], + start: usize, + open_pos: usize, +) -> Result<(usize, usize), EnvParseError> { + let mut depth: usize = 0; + let mut in_string = false; + let mut escaped = false; + + for (line_idx, line) in lines.iter().enumerate().skip(start) { + let from = if line_idx == start { open_pos } else { 0 }; + for (rel, c) in line[from..].char_indices() { + if escaped { + escaped = false; + continue; + } + match c { + '\\' if in_string => escaped = true, + '"' => in_string = !in_string, + '[' if !in_string => depth += 1, + // `depth <= 1` rather than a decrement-then-test so a stray + // leading `]` cannot underflow the counter. + ']' if !in_string && depth <= 1 => return Ok((line_idx, from + rel)), + ']' if !in_string => depth -= 1, + _ => {} + } } - end += 1; + // A line break cannot continue a backslash escape. + escaped = false; } - end + + Err(EnvParseError { + line: start + 1, + message: format!( + "Unterminated list starting on line {}: {}", + start + 1, + lines[start] + ), + }) +} + +/// Extract the value of a list KEY spanning lines[start..=end_line], given the +/// byte offsets of the opening `[` on lines[start] and the closing `]` on +/// lines[end_line] (as returned by `find_list_close`). Both brackets are part +/// of the value; everything after the closing one — including a trailing +/// `# comment` — is discarded. +fn extract_list_value( + lines: &[&str], + start: usize, + end_line: usize, + open_pos: usize, + close_pos: usize, +) -> Result { + let blob = if start == end_line { + lines[start][open_pos..=close_pos].to_string() + } else { + let mut parts: Vec<&str> = vec![&lines[start][open_pos..]]; + for line in lines.iter().take(end_line).skip(start + 1) { + parts.push(line); + } + parts.push(&lines[end_line][..=close_pos]); + parts.join("\n") + }; + + reformat_json_array_if_present(blob, start) } /// If `blob` looks like a JSON array, re-format it as `["a", "b", ...]`. @@ -45,15 +112,12 @@ fn reformat_json_array_if_present(blob: String, start: usize) -> Result= 2 { - // Strip outer quote character (first and last byte – safe for ASCII " and ') - blob = blob[1..blob.len() - 1].to_string(); - } + let blob = parts.join("\n").trim().to_string(); reformat_json_array_if_present(blob, start) } @@ -156,7 +215,12 @@ fn extract_quoted_value( /// - `KEY="double quoted"` (single or multiline) /// - `KEY='single quoted'` (single or multiline) /// - `KEY=["json", "array"]` (single or multiline) -/// - Comments (`# …`) and blank lines are ignored. +/// - `export KEY=value` — the prefix is dropped from the key name. It is the +/// only word allowed before a key; any other whitespace inside a key name is +/// reported as a violation. +/// - Comments (`# …`) and blank lines are ignored. A comment may also trail a +/// quoted value or a list's closing `]`; on an unquoted value, everything +/// after `=` is taken literally. /// - Lines without `=` are ignored (e.g. `// invalid comment`). pub fn parse_env_file(file_path: &str) -> Result { let content = fs::read_to_string(file_path).map_err(|e| EnvParseError { @@ -193,21 +257,45 @@ pub fn parse_env_file(file_path: &str) -> Result { let key_segment = &trimmed[..eq_pos]; let value_segment = &trimmed[eq_pos + 1..]; - let key = key_segment.trim().to_string(); let value_part = value_segment.trim(); + // `export KEY=value` is the common form for .env files that are also + // meant to be `source`d. The prefix is not part of the key name; only + // strip it when whitespace follows, so a key literally named `export` + // still works. + let key_text = key_segment.trim(); + let (key, exported) = match key_text.strip_prefix("export") { + Some(rest) if rest.starts_with(char::is_whitespace) => (rest.trim().to_string(), true), + _ => (key_text.to_string(), false), + }; + if key.is_empty() { i += 1; continue; } + // `export` is the only word allowed to precede a key, and it has already + // been stripped above. Anything else separated by whitespace is a typo + // or an unsupported dialect — either way the key is unaddressable, so + // say so rather than inventing a key name that no shell could use. + if key.contains(char::is_whitespace) { + violations.push(( + line_start + 1, + format!("invalid key '{}': keys cannot contain whitespace", key), + )); + } + // Whitespace hugging the '=' (`KEY = value` or `KEY= value`) is not part // of the key or value and is almost always a mistake. - if key_segment.ends_with(char::is_whitespace) || value_segment.starts_with(char::is_whitespace) + if key_segment.ends_with(char::is_whitespace) + || value_segment.starts_with(char::is_whitespace) { violations.push(( line_start + 1, - format!("whitespace around '=' is not allowed: {}", raw_lines[line_start]), + format!( + "whitespace around '=' is not allowed: {}", + raw_lines[line_start] + ), )); } @@ -230,15 +318,21 @@ pub fn parse_env_file(file_path: &str) -> Result { let mut env_value = EnvValue::with_lines(value, line_start as i64, end as i64); // Single quotes disable ${VAR} expansion (literal, POSIX-style). env_value.no_expand = quote == '\''; + env_value.exported = exported; env_object.set(key, env_value); i = end + 1; } else if value_part.starts_with('[') { - let end = get_end_line(&raw_lines, line_start, "]"); - let value = extract_value(&raw_lines, line_start, end, false)?; - env_object.set( - key, - EnvValue::with_lines(value, line_start as i64, end as i64), - ); + let first_line = raw_lines[line_start]; + let raw_eq_pos = first_line.find('=').unwrap_or(0); + let open_pos = first_line[raw_eq_pos..] + .find('[') + .map(|p| raw_eq_pos + p) + .unwrap_or(raw_eq_pos); + let (end, close_pos) = find_list_close(&raw_lines, line_start, open_pos)?; + let value = extract_list_value(&raw_lines, line_start, end, open_pos, close_pos)?; + let mut env_value = EnvValue::with_lines(value, line_start as i64, end as i64); + env_value.exported = exported; + env_object.set(key, env_value); i = end + 1; } else { // Unquoted value — must not contain bare quotes (would indicate a parse issue) @@ -248,11 +342,10 @@ pub fn parse_env_file(file_path: &str) -> Result { message: format!("Invalid value: {}", raw_lines[i]), }); } - let value = extract_value(&raw_lines, line_start, line_start, false)?; - env_object.set( - key, - EnvValue::with_lines(value, line_start as i64, line_start as i64), - ); + let value = extract_value(&raw_lines, line_start, line_start)?; + let mut env_value = EnvValue::with_lines(value, line_start as i64, line_start as i64); + env_value.exported = exported; + env_object.set(key, env_value); i += 1; } } diff --git a/src/handlers/set_value.rs b/src/handlers/set_value.rs index d689d80..9118429 100644 --- a/src/handlers/set_value.rs +++ b/src/handlers/set_value.rs @@ -11,14 +11,21 @@ use crate::qualifying_rules::Options; pub fn set_value(options: &Options) { let key = &options.target_keys[0]; let set_val = options.set_value.as_deref().unwrap_or(""); + + let env_object = options.env_object.as_ref().unwrap(); + + // A key that was written as `export KEY=value` keeps its prefix on rewrite, + // so a file meant to be `source`d stays sourceable. + let prefix = match env_object.get(key) { + Some(env_val) if env_val.exported => "export ", + _ => "", + }; let new_line = if set_val.contains('\n') { - format!("{}=\"{}\"", key, set_val) + format!("{}{}=\"{}\"", prefix, key, set_val) } else { - format!("{}={}", key, set_val) + format!("{}{}={}", prefix, key, set_val) }; - let env_object = options.env_object.as_ref().unwrap(); - if let Some(env_val) = env_object.get(key) { // Read file fresh (in case concurrent changes occurred) let content = fs::read_to_string(&options.full_env_path).expect("Failed to read .env file"); diff --git a/tests/env_parser.rs b/tests/env_parser.rs index 23f7eb2..5f30c49 100644 --- a/tests/env_parser.rs +++ b/tests/env_parser.rs @@ -94,6 +94,189 @@ fn unquoted_value_keeps_trailing_hash_literally() { assert_eq!(json["NEXT"], "after"); } +// Regression tests for the list equivalent of the quoted-value bug above: the +// closing-bracket search required the line to *end* with ']', so an inline +// comment after the bracket sent it scanning to EOF, swallowing every +// following key into the list value. + +#[test] +fn single_line_list_with_trailing_comment_does_not_swallow_next_key() { + let tmp = write_env("LIST=[\"a\", \"b\"] # inline comment\nNEXT=after\n"); + let json = env_json(&tmp); + assert_eq!(json["LIST"], "[\"a\", \"b\"]"); + assert_eq!(json["NEXT"], "after"); + assert_eq!(json.as_object().unwrap().len(), 2); +} + +#[test] +fn multiline_list_with_comment_on_closing_line_does_not_swallow_next_key() { + let tmp = write_env("LIST=[\n\"a\",\n\"b\"\n] # inline comment\nNEXT=after\n"); + let json = env_json(&tmp); + assert_eq!(json["LIST"], "[\"a\", \"b\"]"); + assert_eq!(json["NEXT"], "after"); + assert_eq!(json.as_object().unwrap().len(), 2); +} + +#[test] +fn list_item_containing_hash_is_not_mistaken_for_a_comment() { + let tmp = write_env("LIST=[\"a # b\", \"c\"]\nNEXT=after\n"); + let json = env_json(&tmp); + assert_eq!(json["LIST"], "[\"a # b\", \"c\"]"); + assert_eq!(json["NEXT"], "after"); +} + +#[test] +fn multiline_list_keeps_hash_on_interior_lines() { + let tmp = write_env("LIST=[\n\"a # b\",\n\"c\"\n] # trailing\nNEXT=after\n"); + let json = env_json(&tmp); + assert_eq!(json["LIST"], "[\"a # b\", \"c\"]"); + assert_eq!(json["NEXT"], "after"); +} + +// The closing `]` is found by counting bracket depth, not by position on the +// line. Locating it any other way (the last `]`, or one followed only by a +// comment) misreads a bracket that appears inside the trailing comment or +// inside a string. + +#[test] +fn a_bracket_inside_the_trailing_comment_does_not_end_the_list_early() { + let tmp = write_env("LIST=[\"a\"] # see note [1]\nNEXT=after\n"); + let json = env_json(&tmp); + assert_eq!(json["LIST"], "[\"a\"]"); + assert_eq!(json["NEXT"], "after"); + assert_eq!(json.as_object().unwrap().len(), 2); +} + +#[test] +fn a_bracket_inside_a_list_item_does_not_close_the_list() { + let tmp = write_env("LIST=[\"a]b\", \"c\"]\nNEXT=after\n"); + let json = env_json(&tmp); + assert_eq!(json["LIST"], "[\"a]b\", \"c\"]"); + assert_eq!(json["NEXT"], "after"); +} + +#[test] +fn nested_lists_close_on_the_outermost_bracket() { + let tmp = write_env("LIST=[[\"a\",\"b\"],[\"c\"]] # tail [2]\nNEXT=after\n"); + let json = env_json(&tmp); + assert_eq!(json["LIST"], "[[\"a\",\"b\"], [\"c\"]]"); + assert_eq!(json["NEXT"], "after"); +} + +#[test] +fn multiline_nested_list_with_bracket_in_comment_closes_correctly() { + let tmp = write_env("LIST=[\n\"x]y\",\n[\"n\"]\n] # tail [3]\nNEXT=after\n"); + let json = env_json(&tmp); + assert_eq!(json["LIST"], "[\"x]y\", [\"n\"]]"); + assert_eq!(json["NEXT"], "after"); +} + +#[test] +fn unterminated_list_fails_loudly_instead_of_swallowing_rest_of_file() { + let tmp = write_env("LIST=[\"a\",\nNEXT=after\nMORE=x\n"); + bin() + .arg("--file") + .arg(tmp.path()) + .assert() + .failure() + .stderr(predicate::str::contains( + "Unterminated list starting on line 1", + )); +} + +// `export KEY=value` is the standard form for .env files that double as shell +// sourceable scripts. The prefix used to become part of the key name, so the +// key was unreachable by its real name and `--` injected a variable literally +// named `export FOO`. + +#[test] +fn export_prefix_is_not_part_of_the_key() { + let tmp = write_env("export FOO=bar\nPLAIN=baz\n"); + let json = env_json(&tmp); + assert_eq!(json["FOO"], "bar"); + assert_eq!(json["PLAIN"], "baz"); + assert!(json.get("export FOO").is_none()); +} + +#[test] +fn export_prefix_works_for_quoted_and_list_values() { + let tmp = write_env("export Q=\"quoted\"\nexport L=[\"a\"]\nexport M=\"one\ntwo\"\n"); + let json = env_json(&tmp); + assert_eq!(json["Q"], "quoted"); + assert_eq!(json["L"], "[\"a\"]"); + assert_eq!(json["M"], "one\ntwo"); +} + +#[test] +fn export_prefix_requires_whitespace_so_a_key_named_export_still_works() { + let tmp = write_env("export=value\n"); + let json = env_json(&tmp); + assert_eq!(json["export"], "value"); +} + +#[test] +fn exported_key_defined_twice_is_still_a_duplicate() { + let tmp = write_env("export FOO=bar\nFOO=baz\n"); + bin() + .arg("--file") + .arg(tmp.path()) + .assert() + .failure() + .stderr(predicate::str::contains("duplicate key 'FOO'")); +} + +// A key never contains a space, and `export` is the only word allowed to sit in +// front of one. Anything else is rejected rather than silently parsed into a key +// name that no shell could address. + +#[test] +fn a_key_containing_a_space_is_rejected() { + let tmp = write_env("FOO BAR=baz\n"); + bin() + .arg("--file") + .arg(tmp.path()) + .assert() + .failure() + .stderr(predicate::str::contains( + "invalid key 'FOO BAR': keys cannot contain whitespace", + )); +} + +#[test] +fn export_is_the_only_word_allowed_before_a_key() { + let tmp = write_env("exprot FOO=baz\n"); + bin() + .arg("--file") + .arg(tmp.path()) + .assert() + .failure() + .stderr(predicate::str::contains( + "invalid key 'exprot FOO': keys cannot contain whitespace", + )); +} + +#[test] +fn a_second_word_after_export_is_still_rejected() { + let tmp = write_env("export FOO BAR=baz\n"); + bin() + .arg("--file") + .arg(tmp.path()) + .assert() + .failure() + .stderr(predicate::str::contains( + "invalid key 'FOO BAR': keys cannot contain whitespace", + )); +} + +#[test] +fn a_key_adjacent_to_export_is_left_alone() { + let tmp = write_env("exportFOO=a\nexport_ISH=b\nexport SPACED=c\n"); + let json = env_json(&tmp); + assert_eq!(json["exportFOO"], "a"); + assert_eq!(json["export_ISH"], "b"); + assert_eq!(json["SPACED"], "c"); +} + #[test] fn unterminated_quoted_value_fails_loudly_instead_of_swallowing_rest_of_file() { let tmp = write_env("BROKEN=\"never closed\nNEXT=after\n"); diff --git a/tests/set_and_delete.rs b/tests/set_and_delete.rs index 5deb1d1..8f3baa5 100644 --- a/tests/set_and_delete.rs +++ b/tests/set_and_delete.rs @@ -500,3 +500,86 @@ fn add_key_to_file_without_trailing_newline() { let content = fs::read_to_string(tmp.path()).unwrap(); assert_eq!(content, "FOO=bar\nBAZ=qux\n"); } + +#[test] +fn set_on_a_list_with_a_bracket_in_its_comment_does_not_truncate_the_file() { + use std::io::Write; + let mut tmp = NamedTempFile::new().unwrap(); + tmp.write_all(b"LIST=[\"a\"] # see note [1]\nAFTER=iamhere\nTHIRD=alsohere\n") + .unwrap(); + tmp.flush().unwrap(); + + bin() + .arg("LIST") + .arg("--set") + .arg("[\"z\"]") + .arg("--file") + .arg(tmp.path()) + .assert() + .success(); + + let content = fs::read_to_string(tmp.path()).unwrap(); + assert_eq!(content, "LIST=[\"z\"]\nAFTER=iamhere\nTHIRD=alsohere\n"); +} + +// `export KEY=value` lines are addressable by their real key name, and a +// rewrite keeps the prefix so a file meant to be `source`d stays sourceable. + +#[test] +fn set_preserves_the_export_prefix() { + use std::io::Write; + let mut tmp = NamedTempFile::new().unwrap(); + tmp.write_all(b"export FOO=bar\nPLAIN=keep\n").unwrap(); + tmp.flush().unwrap(); + + bin() + .arg("FOO") + .arg("--set") + .arg("baz") + .arg("--file") + .arg(tmp.path()) + .assert() + .success(); + + let content = fs::read_to_string(tmp.path()).unwrap(); + assert_eq!(content, "export FOO=baz\nPLAIN=keep\n"); +} + +#[test] +fn delete_removes_an_exported_key() { + use std::io::Write; + let mut tmp = NamedTempFile::new().unwrap(); + tmp.write_all(b"export FOO=bar\nPLAIN=keep\n").unwrap(); + tmp.flush().unwrap(); + + bin() + .arg("FOO") + .arg("--delete") + .arg("--file") + .arg(tmp.path()) + .assert() + .success(); + + let content = fs::read_to_string(tmp.path()).unwrap(); + assert_eq!(content, "PLAIN=keep\n"); +} + +#[test] +fn set_on_a_non_exported_key_does_not_add_a_prefix() { + use std::io::Write; + let mut tmp = NamedTempFile::new().unwrap(); + tmp.write_all(b"export FOO=bar\nPLAIN=old\n").unwrap(); + tmp.flush().unwrap(); + + bin() + .arg("PLAIN") + .arg("--set") + .arg("new") + .arg("--file") + .arg(tmp.path()) + .assert() + .success(); + + let content = fs::read_to_string(tmp.path()).unwrap(); + assert_eq!(content, "export FOO=bar\nPLAIN=new\n"); +}