Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion channels-src/feishu/feishu.capabilities.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
{
"name": "feishu_verification_token",
"prompt": "Enter your Feishu/Lark Verification Token (from Event Subscription webhook settings)",
"optional": true
"optional": false
}
],
"setup_url": "https://open.feishu.cn/app"
Expand Down Expand Up @@ -70,6 +70,7 @@
"config": {
"app_id": null,
"app_secret": null,
"verification_token": null,
"api_base": "https://open.feishu.cn",
"owner_id": null,
"dm_policy": "pairing",
Expand Down
77 changes: 75 additions & 2 deletions channels-src/feishu/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@
//! - App credentials (app_id, app_secret) are injected by the host into
//! the config JSON during startup for token exchange
//! - Bearer token for API calls is obtained via token exchange and cached
//! - Verification token validated by host for webhook requests
//! - Webhook requests must be authenticated by the host or by a matching
//! Feishu verification token in the request body

// Generate bindings from the WIT file
wit_bindgen::generate!({
Expand All @@ -50,6 +51,7 @@ const ALLOW_FROM_PATH: &str = "allow_from";
const API_BASE_PATH: &str = "api_base";
const APP_ID_PATH: &str = "app_id";
const APP_SECRET_PATH: &str = "app_secret";
const VERIFICATION_TOKEN_PATH: &str = "verification_token";
const TOKEN_PATH: &str = "tenant_access_token";
const TOKEN_EXPIRY_PATH: &str = "token_expiry";

Expand Down Expand Up @@ -251,6 +253,9 @@ struct FeishuConfig {
/// Feishu App Secret (for token exchange).
app_secret: Option<String>,

/// Feishu Event Subscription verification token.
verification_token: Option<String>,

/// API base URL. Defaults to "https://open.feishu.cn" (use
/// "https://open.larksuite.com" for Lark international).
#[serde(default = "default_api_base")]
Expand Down Expand Up @@ -300,6 +305,11 @@ impl Guest for FeishuChannel {
if let Some(ref app_secret) = config.app_secret {
let _ = channel_host::workspace_write(APP_SECRET_PATH, app_secret);
}
if let Some(ref verification_token) = config.verification_token {
let _ = channel_host::workspace_write(VERIFICATION_TOKEN_PATH, verification_token);
} else {
let _ = channel_host::workspace_write(VERIFICATION_TOKEN_PATH, "");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This if-let-else block can be simplified by using as_deref() and unwrap_or() to make the code more concise and readable.

        let token_to_write = config.verification_token.as_deref().unwrap_or("");
        let _ = channel_host::workspace_write(VERIFICATION_TOKEN_PATH, token_to_write);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 810cb18 — now uses config.verification_token.as_deref().unwrap_or(""), and further simplified in c6704b8 to use the same if let Some pattern as app_id/app_secret.


if let Some(owner_id) = &config.owner_id {
let _ = channel_host::workspace_write(OWNER_ID_PATH, owner_id);
Expand Down Expand Up @@ -376,6 +386,23 @@ impl Guest for FeishuChannel {
}
};

let configured_token =
channel_host::workspace_read(VERIFICATION_TOKEN_PATH).filter(|token| !token.is_empty());
if !is_authenticated_webhook(
req.secret_validated,
configured_token.as_deref(),
event.token.as_deref(),
) {
channel_host::log(
channel_host::LogLevel::Warn,
"Rejecting unauthenticated Feishu webhook request",
);
return json_response(
401,
serde_json::json!({"error": "Webhook authentication failed"}),
);
}

// Handle URL verification challenge (initial webhook setup).
if event.event_type.as_deref() == Some("url_verification") {
if let Some(challenge) = &event.challenge {
Expand Down Expand Up @@ -839,6 +866,21 @@ fn json_response(status: u16, body: serde_json::Value) -> OutgoingHttpResponse {
}
}

fn is_authenticated_webhook(
secret_validated: bool,
configured_token: Option<&str>,
request_token: Option<&str>,
) -> bool {
if secret_validated {
return true;
}

match (configured_token, request_token) {
(Some(expected), Some(provided)) => expected == provided,
_ => false,
}
}

#[cfg(test)]
mod tests {
use super::*;
Expand All @@ -862,7 +904,10 @@ mod tests {
fn parse_token_response_rejects_missing_token() {
let json = r#"{"code": 0, "msg": "ok", "expire": 7200}"#;
let result: Result<TenantAccessTokenResponse, _> = serde_json::from_str(json);
assert!(result.is_err(), "should fail when tenant_access_token is missing");
assert!(
result.is_err(),
"should fail when tenant_access_token is missing"
);
}

#[test]
Expand Down Expand Up @@ -894,4 +939,32 @@ mod tests {
assert_eq!(resp.code, 10003);
assert!(resp.tenant_access_token.is_empty());
}

#[test]
fn webhook_auth_requires_host_auth_or_matching_verification_token() {
assert!(
!is_authenticated_webhook(false, None, Some("token")),
"requests without any configured verification mechanism must be rejected"
);
assert!(
!is_authenticated_webhook(false, Some("expected"), None),
"requests missing the Feishu token must be rejected when host auth did not pass"
);
assert!(
!is_authenticated_webhook(false, Some("expected"), Some("wrong")),
"requests with the wrong Feishu token must be rejected"
);
assert!(
is_authenticated_webhook(false, Some("expected"), Some("expected")),
"matching Feishu verification token should authenticate the request"
);
assert!(
is_authenticated_webhook(true, None, None),
"host-authenticated requests should still be accepted"
);
assert!(
is_authenticated_webhook(true, Some("expected"), Some("wrong")),
"host authentication should take precedence over body token checks"
);
}
}
24 changes: 19 additions & 5 deletions src/channels/wasm/setup.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,13 +139,14 @@ async fn register_channel(
};

let secret_header = loaded.webhook_secret_header().map(|s| s.to_string());
let host_webhook_secret = host_managed_webhook_secret(&channel_name, webhook_secret.clone());

let webhook_path = format!("/webhook/{}", channel_name);
let endpoints = vec![RegisteredEndpoint {
channel_name: channel_name.clone(),
path: webhook_path,
methods: vec!["POST".to_string()],
require_secret: webhook_secret.is_some(),
require_secret: host_webhook_secret.is_some(),
}];

let channel_arc = Arc::new(loaded.channel.with_owner_actor_id(owner_actor_id.clone()));
Expand Down Expand Up @@ -205,7 +206,7 @@ async fn register_channel(

tracing::info!(
channel = %channel_name,
has_webhook_secret = webhook_secret.is_some(),
has_webhook_secret = host_webhook_secret.is_some(),
secret_header = ?secret_header,
"Registering channel with router"
);
Expand All @@ -214,7 +215,7 @@ async fn register_channel(
.register(
Arc::clone(&channel_arc),
endpoints,
webhook_secret.clone(),
host_webhook_secret.clone(),
secret_header,
)
.await;
Expand Down Expand Up @@ -384,6 +385,17 @@ pub async fn inject_channel_credentials(
Ok(count)
}

fn host_managed_webhook_secret(
channel_name: &str,
webhook_secret: Option<String>,
) -> Option<String> {
if channel_name == "feishu" {
None
} else {
webhook_secret
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Hardcoding the channel name "feishu" to special-case its webhook authentication logic makes the code harder to maintain and violates Rule 9. Instead of hardcoding specific provider IDs, prefer checking for a common property (like a specific environment variable or a feature flag) that defines the group of providers needing this special handling. This approach improves maintainability and scalability.

References
  1. When implementing backward compatibility logic, scope it precisely to the intended cases. Instead of hardcoding specific provider IDs, prefer checking for a common property (like a specific environment variable) that defines the group of providers needing the compatibility logic.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 810cb18 — replaced the hardcoded "feishu" check with a generic managed_by_host field in the webhook capabilities schema (WebhookSchema.managed_by_host). Channels now opt out of host-managed secret validation declaratively via feishu.capabilities.json.

}

/// Inject channel-specific secrets into the config JSON.
///
/// Some channels (e.g., Feishu) need raw credential values in their config
Expand All @@ -392,8 +404,9 @@ pub async fn inject_channel_credentials(
/// placeholders in URLs and headers, so this function fills config fields
/// that map to secret names.
///
/// Mapping: for a channel named "feishu", secrets `feishu_app_id` and
/// `feishu_app_secret` are injected as config keys `app_id` and `app_secret`.
/// Mapping: for a channel named "feishu", secrets `feishu_app_id`,
/// `feishu_app_secret`, and `feishu_verification_token` are injected as config
/// keys `app_id`, `app_secret`, and `verification_token`.
async fn inject_channel_secrets_into_config(
channel_name: &str,
secrets_store: &Option<Arc<dyn SecretsStore + Send + Sync>>,
Expand All @@ -404,6 +417,7 @@ async fn inject_channel_secrets_into_config(
"feishu" => &[
("app_id", "feishu_app_id"),
("app_secret", "feishu_app_secret"),
("verification_token", "feishu_verification_token"),
],
_ => return,
};
Expand Down
Loading