mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-12 00:06:14 +00:00
fix: gracefully handle malformed OAuth entries in instance config (#8205)
When an OAuth provider entry in instance settings has unexpected types (e.g. `"true"` instead of `true` for req_body_auth), the entire /api/settings/instance_config endpoint would fail with a deserialization error, preventing access to any instance settings. Introduce OAuthClientEntry enum that tries typed OAuthClient deserialization first and falls back to raw JSON, logging the deserialization error. This allows the settings page to load even when individual OAuth entries are malformed. Also show a user-visible error toast in SaveButton on save failure instead of only logging to console. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.6
parent
4a14e9436e
commit
cac4bdd54f
@@ -285,7 +285,7 @@ pub struct GlobalSettings {
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub indexer_settings: Option<IndexerSettings>,
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub oauths: Option<BTreeMap<String, OAuthClient>>,
|
||||
pub oauths: Option<BTreeMap<String, OAuthClientEntry>>,
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub otel: Option<OtelSettings>,
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
@@ -406,6 +406,49 @@ pub struct IndexerSettings {
|
||||
// OAuth
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
/// Wrapper that tries to deserialize as a typed [`OAuthClient`] and falls
|
||||
/// back to a raw JSON value when the stored config contains unexpected types
|
||||
/// (e.g. `"true"` instead of `true` for a boolean field). This prevents the
|
||||
/// entire `/api/settings/instance_config` endpoint from failing because of
|
||||
/// one malformed provider entry.
|
||||
#[derive(Serialize, Clone, Debug)]
|
||||
#[cfg_attr(feature = "instance_config_schema", derive(schemars::JsonSchema))]
|
||||
#[serde(untagged)]
|
||||
pub enum OAuthClientEntry {
|
||||
Typed(OAuthClient),
|
||||
/// Fallback: the raw JSON value plus the error that prevented typed parsing.
|
||||
Raw {
|
||||
value: serde_json::Value,
|
||||
/// Human-readable deserialization error (not serialized).
|
||||
#[serde(skip)]
|
||||
error: String,
|
||||
},
|
||||
}
|
||||
|
||||
impl<'de> serde::Deserialize<'de> for OAuthClientEntry {
|
||||
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
|
||||
where
|
||||
D: serde::Deserializer<'de>,
|
||||
{
|
||||
let raw = serde_json::Value::deserialize(deserializer)?;
|
||||
match serde_json::from_value::<OAuthClient>(raw.clone()) {
|
||||
Ok(client) => Ok(OAuthClientEntry::Typed(client)),
|
||||
Err(e) => Ok(OAuthClientEntry::Raw { value: raw, error: e.to_string() }),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl OAuthClientEntry {
|
||||
/// Returns a mutable reference to the inner [`OAuthClient`] if this entry
|
||||
/// was successfully parsed, or `None` for raw/fallback entries.
|
||||
pub fn as_typed_mut(&mut self) -> Option<&mut OAuthClient> {
|
||||
match self {
|
||||
OAuthClientEntry::Typed(c) => Some(c),
|
||||
OAuthClientEntry::Raw { .. } => None,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// OAuth client configuration for a single provider.
|
||||
#[derive(Deserialize, Serialize, Clone, Debug)]
|
||||
#[cfg_attr(feature = "instance_config_schema", derive(schemars::JsonSchema))]
|
||||
@@ -1170,8 +1213,18 @@ pub fn resolve_env_refs(settings: &mut GlobalSettings) -> Result<(), String> {
|
||||
}
|
||||
|
||||
if let Some(oauths) = &mut settings.oauths {
|
||||
for oauth in oauths.values_mut() {
|
||||
resolve_env_field(&mut oauth.secret)?;
|
||||
for (name, entry) in oauths.iter_mut() {
|
||||
match entry {
|
||||
OAuthClientEntry::Typed(oauth) => {
|
||||
resolve_env_field(&mut oauth.secret)?;
|
||||
}
|
||||
OAuthClientEntry::Raw { error, .. } => {
|
||||
tracing::error!(
|
||||
"OAuth entry '{name}' could not be deserialized as OAuthClient \
|
||||
and was kept as raw JSON — it likely has an unexpected shape: {error}"
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1938,6 +1991,88 @@ mod tests {
|
||||
assert_eq!(parsed.id, client.id);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn oauth_entry_valid_config_deserializes_as_typed() {
|
||||
let json = r#"{
|
||||
"id": "slack_id",
|
||||
"secret": "slack_secret",
|
||||
"connect_config": {
|
||||
"auth_url": "https://slack.com/oauth/v2/authorize",
|
||||
"token_url": "https://slack.com/api/oauth.v2.access",
|
||||
"scopes": ["channels:read", "chat:write"],
|
||||
"req_body_auth": true
|
||||
}
|
||||
}"#;
|
||||
let entry: OAuthClientEntry = serde_json::from_str(json).unwrap();
|
||||
match &entry {
|
||||
OAuthClientEntry::Typed(c) => {
|
||||
assert_eq!(c.id, "slack_id");
|
||||
let cc = c.connect_config.as_ref().unwrap();
|
||||
assert_eq!(cc.req_body_auth, Some(true));
|
||||
}
|
||||
OAuthClientEntry::Raw { .. } => panic!("expected Typed variant"),
|
||||
}
|
||||
// round-trip preserves the value
|
||||
let serialized = serde_json::to_string(&entry).unwrap();
|
||||
assert!(serialized.contains("slack_id"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn oauth_entry_string_bool_falls_back_to_raw_with_error() {
|
||||
// This is the real-world scenario: req_body_auth stored as "true" (string)
|
||||
let json = r#"{
|
||||
"id": "custom_provider",
|
||||
"secret": "secret",
|
||||
"connect_config": {
|
||||
"auth_url": "https://example.com/auth",
|
||||
"token_url": "https://example.com/token",
|
||||
"req_body_auth": "true"
|
||||
}
|
||||
}"#;
|
||||
let entry: OAuthClientEntry = serde_json::from_str(json).unwrap();
|
||||
match &entry {
|
||||
OAuthClientEntry::Raw { error, .. } => {
|
||||
assert!(
|
||||
error.contains("invalid type"),
|
||||
"error should mention type mismatch, got: {error}"
|
||||
);
|
||||
}
|
||||
OAuthClientEntry::Typed(_) => panic!("expected Raw fallback for string bool"),
|
||||
}
|
||||
// Serialization still round-trips the raw JSON faithfully
|
||||
let serialized = serde_json::to_string(&entry).unwrap();
|
||||
assert!(serialized.contains("custom_provider"));
|
||||
assert!(serialized.contains(r#""req_body_auth":"true""#));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn oauth_entry_map_with_mixed_valid_and_invalid() {
|
||||
// Simulates what InstanceConfig.oauths looks like when one provider is
|
||||
// well-formed and another has a string-typed boolean.
|
||||
let json = r#"{
|
||||
"google": {
|
||||
"id": "google_id",
|
||||
"secret": "google_secret",
|
||||
"connect_config": {
|
||||
"auth_url": "https://accounts.google.com/o/oauth2/v2/auth",
|
||||
"token_url": "https://oauth2.googleapis.com/token"
|
||||
}
|
||||
},
|
||||
"broken": {
|
||||
"id": "broken_id",
|
||||
"secret": "broken_secret",
|
||||
"connect_config": {
|
||||
"auth_url": "https://example.com/auth",
|
||||
"token_url": "https://example.com/token",
|
||||
"req_body_auth": "false"
|
||||
}
|
||||
}
|
||||
}"#;
|
||||
let map: BTreeMap<String, OAuthClientEntry> = serde_json::from_str(json).unwrap();
|
||||
assert!(matches!(map["google"], OAuthClientEntry::Typed(_)));
|
||||
assert!(matches!(map["broken"], OAuthClientEntry::Raw { .. }));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn custom_instance_pg_databases_roundtrips() {
|
||||
let json = r#"{
|
||||
@@ -2238,7 +2373,7 @@ mod tests {
|
||||
let mut m = BTreeMap::new();
|
||||
m.insert(
|
||||
"google".to_string(),
|
||||
OAuthClient {
|
||||
OAuthClientEntry::Typed(OAuthClient {
|
||||
id: "id".to_string(),
|
||||
secret: StringOrSecretRef::EnvRef(EnvRefWrapper {
|
||||
env_ref: "__WM_TEST_OAUTH_SECRET".to_string(),
|
||||
@@ -2251,7 +2386,7 @@ mod tests {
|
||||
share_with_workspaces: None,
|
||||
grant_types: vec![],
|
||||
extra: BTreeMap::new(),
|
||||
},
|
||||
}),
|
||||
);
|
||||
m
|
||||
}),
|
||||
@@ -2269,10 +2404,12 @@ mod tests {
|
||||
.and_then(|v| v.as_literal()),
|
||||
Some("smtp-secret")
|
||||
);
|
||||
assert_eq!(
|
||||
gs.oauths.as_ref().unwrap()["google"].secret.as_literal(),
|
||||
Some("oauth-secret")
|
||||
);
|
||||
match &gs.oauths.as_ref().unwrap()["google"] {
|
||||
OAuthClientEntry::Typed(c) => {
|
||||
assert_eq!(c.secret.as_literal(), Some("oauth-secret"));
|
||||
}
|
||||
OAuthClientEntry::Raw { .. } => panic!("expected Typed variant"),
|
||||
}
|
||||
|
||||
unsafe {
|
||||
std::env::remove_var("__WM_TEST_SMTP_PWD");
|
||||
|
||||
@@ -3,6 +3,7 @@
|
||||
import { type ButtonType } from './common/button/model'
|
||||
import { Save, CheckCircle2, AlertCircle, Loader2 } from 'lucide-svelte'
|
||||
import { fly } from 'svelte/transition'
|
||||
import { sendUserToast } from '$lib/toast'
|
||||
|
||||
let {
|
||||
onSave,
|
||||
@@ -48,6 +49,7 @@
|
||||
})
|
||||
.catch((error) => {
|
||||
console.error('Save failed:', error)
|
||||
sendUserToast(error?.message ?? 'Save failed', true)
|
||||
saveStatus = 'error'
|
||||
statusTimeout = setTimeout(() => {
|
||||
saveStatus = null
|
||||
|
||||
Reference in New Issue
Block a user