From 8bd5b3f697ff00b1a32bc90a156eb8ace286450b Mon Sep 17 00:00:00 2001 From: John Coffey Date: Sun, 18 Oct 2026 14:49:52 -0700 Subject: [PATCH] Never save "****" as a secret A stored secret reads back as "****", and a registry write that sent it back stored "****" as the secret: a webhook's password or signing key, a store's or directory's password, a DNS provider's key, an OAuth client's secret. Written straight at a secret it now keeps the stored one; anywhere else, inside a whole object, a create or a variant switch, the write is refused with invalidProperties, since there's no stored secret it could stand for. Accounts keep their own credential checks, which already did this. --- crates/jmap/src/inbuxa/masked_secrets.rs | 85 +++++++++++ crates/jmap/src/inbuxa/mod.rs | 1 + crates/jmap/src/registry/set.rs | 28 ++++ tests/src/system/masked_secrets.rs | 177 +++++++++++++++++++++++ tests/src/system/mod.rs | 1 + 5 files changed, 292 insertions(+) create mode 100644 crates/jmap/src/inbuxa/masked_secrets.rs create mode 100644 tests/src/system/masked_secrets.rs diff --git a/crates/jmap/src/inbuxa/masked_secrets.rs b/crates/jmap/src/inbuxa/masked_secrets.rs new file mode 100644 index 0000000..2a5985f --- /dev/null +++ b/crates/jmap/src/inbuxa/masked_secrets.rs @@ -0,0 +1,85 @@ +/* + * SPDX-FileCopyrightText: 2026 Coffey Labs LLC + * + * SPDX-License-Identifier: AGPL-3.0-only + */ + +//! A stored secret reads back as `****` (`MASKED_PASSWORD`), so a client +//! that writes back what it read sends `****` where the secret was. That is +//! never saved as the secret. Written straight at a secret, it means "keep +//! the stored one"; anywhere else (a create, a variant switch, a whole +//! object replaced) the write is refused, since there is no stored secret +//! it could stand for. +//! +//! Accounts are left to their own credential checks, which already keep a +//! credential written back as `****`. + +use jmap_proto::error::set::SetError; +use jmap_tools::{JsonPointer, JsonPointerItem, Key}; +use registry::{ + jmap::JmapValue, + schema::prelude::{MASKED_PASSWORD, Property}, +}; + +fn is_secret_key(key: &Key<'_, Property>) -> bool { + match key { + Key::Property(property) => *property == Property::Secret, + Key::Borrowed(name) => *name == "secret", + Key::Owned(name) => name == "secret", + } +} + +fn is_masked(value: &JmapValue<'_>) -> bool { + value.as_str().is_some_and(|value| value == MASKED_PASSWORD) +} + +/// Whether `value` holds `****` as a secret anywhere inside it. +pub fn holds_masked_secret(value: &JmapValue<'_>) -> bool { + match value { + JmapValue::Object(map) => map.iter().any(|(key, value)| { + (is_secret_key(key) && is_masked(value)) || holds_masked_secret(value) + }), + JmapValue::Array(items) => items.iter().any(holds_masked_secret), + _ => false, + } +} + +/// Whether a patch at `pointer` writes `****` straight at a secret: the +/// client sent back what it read, so the stored secret stays. +pub fn keeps_stored_secret(pointer: &JsonPointer, value: &JmapValue<'_>) -> bool { + is_masked(value) + && matches!(pointer.last(), Some(JsonPointerItem::Key(key)) if is_secret_key(key)) +} + +/// The top-level property a write at `pointer` goes to, for the error. +pub fn top_level_property(pointer: &JsonPointer) -> Option { + pointer.iter().find_map(|item| match item { + JsonPointerItem::Key(Key::Property(property)) => Some(property.clone()), + _ => None, + }) +} + +/// For a whole-object write (a create or a variant switch): the first +/// top-level property holding `****` as a secret, if any. +pub fn masked_secret_in_object(value: &JmapValue<'_>) -> Option> { + let map = value.as_object()?; + map.iter().find_map(|(key, value)| { + ((is_secret_key(key) && is_masked(value)) || holds_masked_secret(value)).then( + || match key { + Key::Property(property) => Some(property.clone()), + _ => None, + }, + ) + }) +} + +pub fn masked_secret_error(property: Option) -> SetError { + let error = SetError::invalid_properties().with_description(concat!( + "\"****\" stands for a stored secret and can't be saved as one. ", + "Send the secret itself, or leave the field out to keep the stored one." + )); + match property { + Some(property) => error.with_property(property), + None => error, + } +} diff --git a/crates/jmap/src/inbuxa/mod.rs b/crates/jmap/src/inbuxa/mod.rs index bd975f4..c940d66 100644 --- a/crates/jmap/src/inbuxa/mod.rs +++ b/crates/jmap/src/inbuxa/mod.rs @@ -28,6 +28,7 @@ pub mod data_inventory; pub mod directory_test; pub mod webhook_test; pub mod explanation; +pub mod masked_secrets; // inbuxa: "****" is never saved as a secret pub mod protocol_policy; pub mod tenant_protocol_policy; pub mod sharing_policy; diff --git a/crates/jmap/src/registry/set.rs b/crates/jmap/src/registry/set.rs index 02f18a6..9a62a94 100644 --- a/crates/jmap/src/registry/set.rs +++ b/crates/jmap/src/registry/set.rs @@ -6,6 +6,7 @@ * Modified by Coffey Labs in 2026 for INBUXA. */ +use crate::inbuxa::masked_secrets; use crate::registry::{ EnterpriseRegistry, mapping::{ @@ -348,6 +349,10 @@ impl RegistrySet for Server { continue 'outer; }; + // inbuxa: "****" is never saved as a secret (accounts keep + // their own credential checks) + let guard_masked_secrets = object_type != ObjectType::Account; + if is_create || value .as_object() @@ -356,6 +361,13 @@ impl RegistrySet for Server { .and_then(|v| v.as_str()) .is_some_and(|t| new_object.object_variant().is_some_and(|v| v != t)) { + if guard_masked_secrets + && let Some(property) = masked_secrets::masked_secret_in_object(&value) + { + set.failed(modification, masked_secrets::masked_secret_error(property)); + continue 'outer; + } + // Patch object match new_object.patch( JsonPointerPatch::new(&JsonPointer::new(vec![])) @@ -411,6 +423,22 @@ impl RegistrySet for Server { Key::Owned(other) => JsonPointer::parse(&other), }; + // inbuxa: "****" written back at a secret keeps it; + // anywhere deeper there is nothing it could stand for + if guard_masked_secrets { + if masked_secrets::keeps_stored_secret(&ptr, &value) { + continue; + } else if masked_secrets::holds_masked_secret(&value) { + set.failed( + modification, + masked_secrets::masked_secret_error( + masked_secrets::top_level_property(&ptr), + ), + ); + continue 'outer; + } + } + // Patch object match new_object.patch( JsonPointerPatch::new(&ptr) diff --git a/tests/src/system/masked_secrets.rs b/tests/src/system/masked_secrets.rs new file mode 100644 index 0000000..0b23808 --- /dev/null +++ b/tests/src/system/masked_secrets.rs @@ -0,0 +1,177 @@ +/* + * SPDX-FileCopyrightText: 2026 Coffey Labs LLC + * + * SPDX-License-Identifier: AGPL-3.0-only + */ + +//! A stored secret reads back as "****", and "****" is never saved as one: +//! written straight at a secret it keeps the stored one, anywhere else the +//! write is refused. + +use crate::utils::{account::Account, server::TestServerBuilder}; +use common::Server; +use registry::{ + schema::{ + prelude::{ObjectInner, ObjectType}, + structs::{HttpAuth, SecretKey, SecretKeyOptional, WebHook}, + }, + types::id::ObjectId, +}; +use serde_json::{Value, json}; +use std::str::FromStr; +use types::id::Id; + +const USING: &[&str] = &["urn:ietf:params:jmap:core", "urn:inbuxa:jmap:registry"]; + +async fn webhook_set(admin: &Account, arguments: Value) -> Value { + let mut arguments = arguments; + arguments["accountId"] = admin.id_string().into(); + let response = admin + .jmap_request(USING, json!([["x:WebHook/set", arguments, "0"]])) + .await; + let call = response.0.pointer("/methodResponses/0").cloned().unwrap(); + assert_eq!(call[0], "x:WebHook/set", "{call}"); + call[1].clone() +} + +/// The webhook's Basic password and signature key as stored. +async fn stored_secrets(server: &Server, id: &str) -> (String, String) { + let object = server + .registry() + .get(ObjectId::new( + ObjectType::WebHook, + Id::from_str(id).unwrap(), + )) + .await + .unwrap() + .expect("webhook not found"); + let ObjectInner::WebHook(WebHook { + http_auth: HttpAuth::Basic(basic), + signature_key, + .. + }) = object.inner + else { + panic!("not a webhook with Basic auth"); + }; + let SecretKey::Value(password) = basic.secret else { + panic!("password not stored as a value"); + }; + let SecretKeyOptional::Value(signature) = signature_key else { + panic!("signature key not stored as a value"); + }; + (password.secret, signature.secret) +} + +fn assert_refused(response: &Value, key: &str, id: &str, property: &str) { + let error = &response[key][id]; + assert_eq!(error["type"], "invalidProperties", "{response}"); + assert_eq!(error["properties"], json!([property]), "{response}"); +} + +#[tokio::test(flavor = "multi_thread")] +pub async fn masked_secrets_tests() { + let test = TestServerBuilder::new("masked_secrets_tests") + .await + .with_default_listeners() + .await + .build() + .await; + let admin = test.create_admin_account("admin@example.org").await; + println!("Running masked secret tests..."); + + let response = webhook_set( + &admin, + json!({"create": {"hook": { + "url": "https://hooks.example.org/inbuxa", + "enable": false, + "httpAuth": {"@type": "Basic", "username": "inbuxa", + "secret": {"@type": "Value", "secret": "basic-password"}}, + "signatureKey": {"@type": "Value", "secret": "signing-key"} + }}}), + ) + .await; + let id = response["created"]["hook"]["id"] + .as_str() + .unwrap_or_else(|| panic!("{response}")) + .to_string(); + let original = ("basic-password".to_string(), "signing-key".to_string()); + assert_eq!(stored_secrets(&test.server, &id).await, original); + + // Read back, the secrets are masked + let response = admin + .jmap_request( + USING, + json!([["x:WebHook/get", {"accountId": admin.id_string(), "ids": [&id]}, "0"]]), + ) + .await; + let hook = response + .0 + .pointer("/methodResponses/0/1/list/0") + .cloned() + .unwrap(); + assert_eq!(hook["httpAuth"]["secret"]["secret"], "****", "{hook}"); + assert_eq!(hook["signatureKey"]["secret"], "****", "{hook}"); + + // "****" straight at a secret keeps it, and the rest of the update applies + let response = webhook_set( + &admin, + json!({"update": {&id: { + "url": "https://hooks.example.org/changed", + "httpAuth/secret/secret": "****", + "signatureKey/secret": "****" + }}}), + ) + .await; + assert!(response["updated"].get(&id).is_some(), "{response}"); + assert_eq!(stored_secrets(&test.server, &id).await, original); + + // Inside a whole object, there is no stored secret it could stand for + for (patch, property) in [ + ( + json!({"httpAuth/secret": {"@type": "Value", "secret": "****"}}), + "httpAuth", + ), + ( + json!({"signatureKey": {"@type": "Value", "secret": "****"}}), + "signatureKey", + ), + ( + json!({"httpAuth": {"@type": "Basic", "username": "inbuxa", + "secret": {"@type": "Value", "secret": "****"}}}), + "httpAuth", + ), + ] { + let response = webhook_set(&admin, json!({"update": {&id: patch}})).await; + assert_refused(&response, "notUpdated", &id, property); + assert_eq!(stored_secrets(&test.server, &id).await, original); + } + + // Nor in a new object + let response = webhook_set( + &admin, + json!({"create": {"copy": { + "url": "https://hooks.example.org/copy", + "enable": false, + "httpAuth": {"@type": "Basic", "username": "inbuxa", + "secret": {"@type": "Value", "secret": "****"}}, + }}}), + ) + .await; + assert_refused(&response, "notCreated", "copy", "httpAuth"); + + // A real secret is still saved + let response = webhook_set( + &admin, + json!({"update": {&id: {"httpAuth/secret/secret": "new-password"}}}), + ) + .await; + assert!(response["updated"].get(&id).is_some(), "{response}"); + assert_eq!( + stored_secrets(&test.server, &id).await, + ("new-password".to_string(), "signing-key".to_string()) + ); + + if test.is_reset() { + test.temp_dir.delete(); + } +} diff --git a/tests/src/system/mod.rs b/tests/src/system/mod.rs index 06013bc..cb88811 100644 --- a/tests/src/system/mod.rs +++ b/tests/src/system/mod.rs @@ -29,6 +29,7 @@ pub mod crypto; pub mod delivery; pub mod directory; pub mod masked_email; +pub mod masked_secrets; // inbuxa: "****" is never saved as a secret pub mod monitoring; pub mod oidc; pub mod purge; -- 2.54.0