fix(awssm,scaleway): a JSON null field is no value, not the string "null"

[dngr2]

The bug

extract_json_key reads one key out of a JSON secret value for a ref's field. A string comes back as itself, a missing key as None, and everything else falls through a catch-all rendering it with to_string().

serde_json renders Value::Null as "null". So:

DB_PASSWORD = { ref = { item = "prod/db-credentials", field = "password" } }

against {"username": "admin", "password": null} produces a secret whose value is the four characters null.

Verified against the current code before changing anything:

{"password": null}  ->  Some("null")

That satisfies a required secret and reaches the program as a password spelled n-u-l-l. Nothing warns; as far as every later check is concerned the secret is present.

Why this is a bug and not a choice

The project already treats a JSON null as no value — in two other providers, deliberately:

Provider	Behaviour
bw.rs:538	None | Some(serde_json::Value::Null) => Ok(None)
dashlane.rs:236	Value::Null => return None, under a doc comment reading "treating an absent or empty value as no value"
awssm.rs	rendered "null"
scaleway.rs	rendered "null"

scaleway's extract_json_key carries the comment "mirroring the AWS provider's field semantics", so it inherited this rather than deciding it.

(keeper.rs:364 also mentions Value::Null => "null", but that is value_type, returning a type name for error messages — unrelated, and correct as it stands.)

The change

One match arm in each provider. A null field now behaves exactly like an absent one. Numbers and bools keep rendering verbatim — only null moves.

Tests

• awssm: null is None; and a null field and a missing field agree with each other
• scaleway: the null case

Removing the new arm fails all three, while the five pre-existing extract_json_key tests still pass — so they pin the behaviour rather than the implementation.

Full suite: 1279 passed, 0 failed (baseline before the change was 1276). Getting there needed jq, sops ≥ 3.13 and age on PATH — without jq 66 bw tests fail on a shim, and an older sops fails 14 more on flag provided but not defined: -value-stdin. Might be worth a line in the contributing notes; happy to send that separately.

One caveat in the interest of accuracy: the suite has a flaky test under parallel execution — provider::passbolt::tests::uuid_writes_bypass_the_folder_listing failed once in about six runs, on a clean tree as well as with this change, and passes 3/3 in isolation. Unrelated to this patch, but you may want to know.

[domenkozar]

we should share both codepaths with extract from file provider which supports JSON pointers

[dngr2]

Agreed, and there's a reason to do it beyond deduplication: extract_stored_value has the same bug.

secrets.rs:1757:

let selected = match selected {
    serde_json::Value::String(value) => value.clone(),
    value => serde_json::to_string(value).expect("serializing JSON value cannot fail"),
};

Same shape as the two providers — string clones, everything else stringifies — so a pointer landing on a null yields "null". Checked rather than assumed:

pointer /password on {"password": null}  ->  "null"

So there are three copies of this, and folding the providers into the shared one without fixing it there would spread the defect rather than remove it.

Two things I'd want your call on before I write it, because they aren't mechanical:

1. What a null means at each boundary. The providers return Result<Option<SecretString>>, where None means "not set" and lets a provider chain fall through to the next one. extract_stored_value returns Result<SecretString>, where a pointer that matches nothing is a DecodeFailed error. A null is "present but has no value", which maps to None on the provider side and has no obvious equivalent on the extract side — error, or plumb an Option through? I lean toward None/absent in both, so a null behaves exactly like a missing key, but that changes extract_stored_value's signature.

2. field is not a pointer today. awssm and scaleway do a flat json.get(json_key), so field = "a/b" means the key literally named a/b. Under JSON Pointer that becomes a → b, and ~ gains meaning too. Unifying either changes what those refs select, or keeps a flat-key mode alongside pointers. The second is duller but doesn't move anyone's secrets.

Happy to take it either way — say which and I'll redo this PR as the shared implementation with the null case covered in one place. If you'd rather land the narrow fix first and unify separately, this one is self-contained and does not touch extract_stored_value.

[dngr2]

Done — pushed as the shared implementation.

Rendering now lives in crate::json_field and all three call sites use it. Selection stays with each caller, because a flat field key and a JSON Pointer are not interchangeable: a field literally named a/b selects that key today, and would become a → b under pointer syntax. Folding those together would move people's secrets, so I left it alone.

One thing the unification turned up

The two paths should differ on null, and one of them already said so:

tests.rs:5309, which predates this PR:

assert_eq!(values["NULL_VALUE"].expose_secret(), "null");

So rendering a null as "null" on an extract pointer is deliberate and pinned end to end. My first attempt at sharing changed it and broke that test — correctly. An extract names one location and reports what the document holds there; a provider field is a lookup that can come up empty, where a null means "not set" and the chain should continue.

So the module exposes the two policies by name rather than letting them coincide:

render(&Value) -> SecretString                 // extract pointer: a null renders
render_field(&Value) -> Option<SecretString>   // provider field: a null is absent

render_field is render plus the null arm, and a test asserts they agree on every non-null value, so the two cannot drift.

Result

• One rendering implementation instead of three
• extract_stored_value behaviour unchanged, still pinned by the existing test
• awssm and scaleway now match bw and dashlane on nulls
• 1284 passed, 0 failed (1276 before this PR). Dropping the null arm from render_field fails five tests.

Needs jq, sops >= 3.13 and age on PATH for a green run, incidentally — without jq 66 bw tests fail on a shim, and an older sops fails 14 more on flag provided but not defined: -value-stdin. Happy to send a line for the contributing notes separately.