From e4752efa8e549a8ddcd4bf24eb387b9df85e22b1 Mon Sep 17 00:00:00 2001 From: Gokul Date: Sat, 18 Apr 2026 22:20:55 +0530 Subject: [PATCH] feat(evaluator): make toJSON(secrets) return secrets object (#100) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It turns out that toJSON(secrets) has been returning "null" this whole time — same disease as env, steps, needs, and github before it. The evaluator had no bare-`secrets` branch in ExpressionContext::resolve, so resolving the identifier fell through to the catch-all `_ => Null` and toJSON dutifully serialised that. Fourth in a series after #96/#97/#98/#99. The fix is the simplest of the bunch. secrets_context is already a flat &HashMap holding exactly the shape real GHA exposes as the `secrets` context — no prefix strip, no exclusion list (unlike #99's github arm), no nested outputs sub-object (unlike steps/needs). Clone the entries into ExprValue::String and wrap in ExprValue::Object. Done. Values are returned in plaintext by design. That matches real GHA — `toJSON(secrets)` there is not auto-redacted either, and the common `fromJSON(toJSON(secrets))` pipe-through-an-action pattern depends on the exact original values surviving the round-trip. Masking stays where it belongs: the log boundary, via wrkflw_secrets::SecretMasker, already wired up in engine.rs. Pulling the masker into the evaluator would (a) break the fromJSON round-trip, (b) diverge from GHA semantics, and (c) duplicate a concern that already has one correct home. Please don't do that. Tests mirror the #96–#99 suites: populated secrets, empty context, sorted keys, special-character values (quotes, backslashes, PEM-style newlines), fromJSON(toJSON(secrets)) round-trip, bare-secrets truthiness, a regression guard that the bare arm doesn't shadow the existing dotted-access arm, and a plaintext-values test that pins the no-masking-here decision so any future switch is deliberate rather than silent. While at it, drop `secrets` from the lingering `TODO: support other bare contexts` comment. Only `matrix` remains. --- crates/executor/src/expression.rs | 156 +++++++++++++++++++++++++++++- 1 file changed, 155 insertions(+), 1 deletion(-) diff --git a/crates/executor/src/expression.rs b/crates/executor/src/expression.rs index 6511ac1..b133a8f 100644 --- a/crates/executor/src/expression.rs +++ b/crates/executor/src/expression.rs @@ -453,7 +453,7 @@ impl<'a> ExpressionContext<'a> { .unwrap_or(ExprValue::Null), // Bare context names — return the whole context as an Object so that // `toJSON(env)` (and similar) can serialise it. - // TODO: support other bare contexts: secrets, matrix + // TODO: support other bare contexts: matrix "steps" if parts.len() == 1 => { // Collect all step IDs from both outputs and statuses maps. let mut all_ids: HashSet<&String> = self.step_outputs.keys().collect(); @@ -551,6 +551,23 @@ impl<'a> ExpressionContext<'a> { } ExprValue::Object(map) } + "secrets" if parts.len() == 1 => { + // Wrap `secrets_context` as an Object so `toJSON(secrets)` can + // serialise it. Mirrors real GHA's `secrets` context shape + // (flat `{ name: value }` map). + // + // Values are returned in plaintext by design — same policy as + // `toJSON(github)` for `GITHUB_TOKEN`. Masking is a log-boundary + // concern handled by `wrkflw_secrets::SecretMasker` when wired in + // via `engine.rs`. Do not dump this object to untrusted sinks + // without routing through the masker. + let map = self + .secrets_context + .iter() + .map(|(k, v)| (k.clone(), ExprValue::String(v.clone()))) + .collect(); + ExprValue::Object(map) + } _ => ExprValue::Null, } } @@ -2294,6 +2311,143 @@ mod tests { assert_eq!(deploy_outputs.get("emoji").unwrap(), "\u{1F680}"); } + // -- toJSON(secrets) tests -- + + /// Helper to build an ExpressionContext with secrets data. + fn make_secrets_ctx(secrets: &HashMap) -> ExpressionContext<'_> { + ExpressionContext { + env_context: &EMPTY_ENV, + step_outputs: &EMPTY_STEPS, + matrix_combination: &EMPTY_MATRIX, + step_statuses: &EMPTY_STATUSES, + job_status: "success", + secrets_context: secrets, + needs_context: &EMPTY_NEEDS, + needs_results: &EMPTY_NEEDS_RESULTS, + } + } + + #[test] + fn tojson_secrets_returns_object() { + let mut secrets = HashMap::new(); + secrets.insert("NPM_TOKEN".to_string(), "abc".to_string()); + secrets.insert("DEPLOY_KEY".to_string(), "xyz".to_string()); + let ctx = make_secrets_ctx(&secrets); + let result = evaluate("toJSON(secrets)", &ctx).unwrap(); + let s = result.to_output_string(); + let parsed: serde_json::Value = serde_json::from_str(&s).expect("should be valid JSON"); + let obj = parsed.as_object().expect("should be a JSON object"); + assert_eq!(obj.get("NPM_TOKEN").unwrap(), "abc"); + assert_eq!(obj.get("DEPLOY_KEY").unwrap(), "xyz"); + assert_eq!(obj.len(), 2); + } + + #[test] + fn tojson_secrets_empty_context() { + let secrets = HashMap::new(); + let ctx = make_secrets_ctx(&secrets); + let result = evaluate("toJSON(secrets)", &ctx).unwrap(); + let s = result.to_output_string(); + let parsed: serde_json::Value = serde_json::from_str(&s).expect("should be valid JSON"); + let obj = parsed.as_object().expect("should be a JSON object"); + assert!(obj.is_empty(), "should be empty with no secrets: {}", s); + } + + #[test] + fn tojson_secrets_sorted_keys() { + let mut secrets = HashMap::new(); + secrets.insert("ZEBRA".to_string(), "z".to_string()); + secrets.insert("APPLE".to_string(), "a".to_string()); + secrets.insert("MANGO".to_string(), "m".to_string()); + let ctx = make_secrets_ctx(&secrets); + let result = evaluate("toJSON(secrets)", &ctx).unwrap(); + let s = result.to_output_string(); + let apple_pos = s.find("APPLE").unwrap(); + let mango_pos = s.find("MANGO").unwrap(); + let zebra_pos = s.find("ZEBRA").unwrap(); + assert!(apple_pos < mango_pos, "APPLE should come before MANGO"); + assert!(mango_pos < zebra_pos, "MANGO should come before ZEBRA"); + } + + #[test] + fn tojson_secrets_preserves_special_characters() { + let mut secrets = HashMap::new(); + secrets.insert("QUOTE".to_string(), "he said \"hi\"".to_string()); + secrets.insert("BACKSLASH".to_string(), "path\\to\\key".to_string()); + secrets.insert( + "NEWLINE".to_string(), + "-----BEGIN-----\nBODY\n-----END-----".to_string(), + ); + let ctx = make_secrets_ctx(&secrets); + let result = evaluate("toJSON(secrets)", &ctx).unwrap(); + let s = result.to_output_string(); + let parsed: serde_json::Value = + serde_json::from_str(&s).expect("should be valid JSON despite special chars"); + let obj = parsed.as_object().unwrap(); + assert_eq!(obj.get("QUOTE").unwrap(), "he said \"hi\""); + assert_eq!(obj.get("BACKSLASH").unwrap(), "path\\to\\key"); + assert_eq!( + obj.get("NEWLINE").unwrap(), + "-----BEGIN-----\nBODY\n-----END-----" + ); + } + + #[test] + fn fromjson_tojson_secrets_produces_parseable_json() { + // `fromJSON` currently returns the raw JSON text as `ExprValue::String` + // (same pattern as `fromjson_tojson_env_produces_parseable_json`). The + // round-trip must preserve exact values so pipe-through-an-action use + // cases work and so any future switch to value-masking here is a + // deliberate decision. + let mut secrets = HashMap::new(); + secrets.insert("NPM_TOKEN".to_string(), "npm_ABC123".to_string()); + secrets.insert("DEPLOY_KEY".to_string(), "deploy_XYZ".to_string()); + let ctx = make_secrets_ctx(&secrets); + let result = evaluate("fromJSON(toJSON(secrets))", &ctx).unwrap(); + let s = result.to_output_string(); + let parsed: serde_json::Value = serde_json::from_str(&s).expect("should be valid JSON"); + let obj = parsed.as_object().expect("should be a JSON object"); + assert_eq!(obj.get("NPM_TOKEN").unwrap(), "npm_ABC123"); + assert_eq!(obj.get("DEPLOY_KEY").unwrap(), "deploy_XYZ"); + } + + #[test] + fn bare_secrets_is_truthy() { + let mut secrets = HashMap::new(); + secrets.insert("NPM_TOKEN".to_string(), "abc".to_string()); + let ctx = make_secrets_ctx(&secrets); + let result = evaluate("secrets", &ctx).unwrap(); + assert!(result.is_truthy()); + } + + #[test] + fn bare_secrets_does_not_shadow_dotted_access() { + // Regression guard: the bare-`secrets` arm must not shadow the existing + // `secrets.NAME` dotted-access arm. + let mut secrets = HashMap::new(); + secrets.insert("NPM_TOKEN".to_string(), "abc".to_string()); + let ctx = make_secrets_ctx(&secrets); + let result = evaluate("secrets.NPM_TOKEN", &ctx).unwrap(); + assert_eq!(result, ExprValue::String("abc".to_string())); + } + + #[test] + fn tojson_secrets_returns_values_in_plaintext() { + // Documents current behavior: secret values surface in plaintext inside + // `toJSON(secrets)`. This matches real GHA; masking lives at the log + // boundary via `wrkflw_secrets::SecretMasker`, not in the evaluator. + // Pin the behavior so any future change (exclude, redact, route through + // a masker at this layer) is a deliberate decision. + let mut secrets = HashMap::new(); + secrets.insert("GITHUB_TOKEN".to_string(), "ghs_supersecret".to_string()); + let ctx = make_secrets_ctx(&secrets); + let result = evaluate("toJSON(secrets)", &ctx).unwrap(); + let s = result.to_output_string(); + let parsed: serde_json::Value = serde_json::from_str(&s).expect("should be valid JSON"); + let obj = parsed.as_object().unwrap(); + assert_eq!(obj.get("GITHUB_TOKEN").unwrap(), "ghs_supersecret"); + } + #[test] fn object_cmp_returns_none() { // Object comparisons via <, >, <=, >= should all evaluate to false