fix: require Feishu webhook authentication - #1638
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the security and configuration of Feishu channel integration by enforcing webhook authentication. It introduces a mandatory verification token during setup and refines the webhook handling logic to ensure that Feishu's specific authentication requirements are met, preventing unauthenticated access while maintaining flexibility for host-level validation. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces support for Feishu webhook verification tokens. It makes the feishu_verification_token a required capability, adds it to the channel's configuration, and implements a new is_authenticated_webhook function to handle authentication logic based on this token or host-validated secrets. Host-managed webhook secrets are now explicitly disabled for the Feishu channel. The review comments suggest simplifying an if-let-else block for writing the verification token and address a design concern regarding hardcoding the "feishu" channel name for special authentication logic, which violates a rule and impacts maintainability.
| 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, ""); | ||
| } |
There was a problem hiding this comment.
| if channel_name == "feishu" { | ||
| None | ||
| } else { | ||
| webhook_secret | ||
| } |
There was a problem hiding this comment.
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
- 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.
There was a problem hiding this comment.
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.
zmanian
left a comment
There was a problem hiding this comment.
Review: fix: require Feishu webhook authentication
Core auth logic is correct and the security improvement is real. One item to verify before merge.
Medium
-
V2 event token location:
FeishuEvent.tokenappears to only be populated duringurl_verificationchallenges. For Feishu Event Subscription v2.0, the verification token may be inevent.header.tokeninstead. If the top-leveltokenis absent on normal v2 message events, this PR would reject all legitimate Feishu messages. Verify against Feishu's v2 event docs before merging. -
Hardcoded
"feishu"channel name inhost_managed_webhook_secret: Creates a Feishu-specific carve-out in generic WASM infrastructure. Consider having channels declare"host_managed_auth": falsein capabilities instead.
Low
-
Timing side-channel on token comparison:
expected == providedis variable-time. Low practical risk over network, but since this is a security PR, constant-time comparison (e.g.,subtle::ConstantTimeEq) would be the right call. -
Empty-string write when token absent: Consider skipping the write entirely, consistent with
app_id/app_secrethandling.
Info
- Breaking change for existing instances with no verification token configured -- they'll reject all webhooks after upgrade. Correct behavior, but call out in release notes.
- Test coverage is solid -- all 6 combinations of the auth matrix covered.
Approve once the v2 token location is verified.
…secret Address zmanian review nit #4: only write verification_token to workspace when present, matching the if-let pattern used for app_id and app_secret. Functionally identical (the auth check filters empty strings), but consistent. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Addressing @zmanian's reviewAll items from your review have been addressed across commits Medium
Low
Note on CIThe |
zmanian
left a comment
There was a problem hiding this comment.
Re-review: Feishu webhook authentication
All flagged concerns resolved:
- v2 event token -- Fixed:
request_verification_token()checksevent.header.tokenfirst (v2), falls back toevent.token(v1). Two dedicated tests cover both paths. - Empty-string write -- Fixed: Token filtered with
.filter(|t| !t.is_empty())before comparison. - Constant-time comparison added: Uses
subtle::ConstantTimeEq. managed_by_hostcapability flag: Clean design for channels that handle their own auth.
Approve. Ship it.
* fix: require Feishu webhook authentication * fix: handle Feishu v2 webhook token auth * fix: skip empty verification token write, consistent with app_id/app_secret Address zmanian review nit nearai#4: only write verification_token to workspace when present, matching the if-let pattern used for app_id and app_secret. Functionally identical (the auth check filters empty strings), but consistent. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: require Feishu webhook authentication * fix: handle Feishu v2 webhook token auth * fix: skip empty verification token write, consistent with app_id/app_secret Address zmanian review nit nearai#4: only write verification_token to workspace when present, matching the if-let pattern used for app_id and app_secret. Functionally identical (the auth check filters empty strings), but consistent. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Testing