Skip to content

fix(feishu): authenticate webhook traffic before quota (#29154) - #73406

Open
egilewski wants to merge 2 commits into
NousResearch:mainfrom
egilewski:codex/security-issue-29154
Open

egilewski wants to merge 2 commits into
NousResearch:mainfrom
egilewski:codex/security-issue-29154

Conversation

@egilewski

@egilewski egilewski commented Jul 28, 2026 •

Copy link
Copy Markdown

Unauthenticated Feishu requests can exhaust the delivery quota before valid callbacks from the same IP are checked. Authenticate token and raw-body signatures first, with a separate bounded pre-authentication parser budget and capped anomaly tracking.

This PR is pseudo-stacked in two commits:

  1. cbd561467b9 fixes quota ordering, requires a token for unsigned URL challenges, and contains malformed nested content on webhook and websocket paths. Encrypted envelopes remain rejected at this prefix. 131 tests passed; 12 skipped.
  2. 7e5adcf0580 adds Feishu encrypted envelopes using the first commit's bounded ingress checks. Supplied signatures are verified before decryption; unsigned challenges require the embedded verification token. Invalid encrypted payloads and authentication failures return the same 401 response. 140 tests passed; 19 skipped.

Each prefix works without later commits. Encryption support depends on the first prefix's parsing and authentication boundary, so it remains a separate review commit in this PR. Current-main event dispatch, existing encrypt-key-only signed event authentication, and websocket behavior are retained.

Validation: two quota regressions failed on current main before the fix; two encrypted rejection/signature-order regressions failed before the second-boundary correction. Seven focused Feishu/setup, admission, comment, meeting, voice, and websocket-isolation files pass. Ruff, the repository compatibility-pointer lint, diff checks, and both prefix merge checks against 1d06f7a95ae pass. Test skips are the alternate no-SDK coverage classes when the SDK is installed.

The pre-authentication budget remains 600 requests per IP per fixed 60-second window; authenticated delivery quota is 120. Both tracking tables and anomaly tracking are capped. Uniform CBC rejection responses do not establish constant-time processing; unsigned challenge support follows the official SDK protocol. Missing cryptography produces a controlled 503.

Not checked: live Feishu callbacks, full repository suite, native Windows, documentation build, and CodeRabbit (self-authored P3).

Fixes #29154

Maintenance: GPT-6 in Codex desktop (parent reasoning level unavailable). The account owner loosely reviews these actions and receives the usual GitHub notifications.

@alt-glitch alt-glitch added type/security Security vulnerability or hardening P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins platform/feishu Feishu / Lark adapter sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Jul 28, 2026
@egilewski

Copy link
Copy Markdown
Author

The failed Desktop E2E check is an Actions artifact-service flake unrelated to this Feishu-only patch.

  • All 37 Playwright tests passed.
  • The job failed afterward while FinalizeArtifact received an intermediary HTTP 403.
  • The PR head remains unchanged at 46c1cfec1e2ba1c7abf690a359a245402255ea70; no code change is indicated by this failure.

Signed: GPT-5.6-sol-xhigh in Codex

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for the focused webhook hardening. I confirmed the reported premise on current main: plugins/platforms/feishu/adapter.py:3512-3517 charges the per-IP rate bucket before body parsing, token verification (:3560-3571), and signature verification (:3580-3584). The proposed ordering directly addresses that defect, and the added invalid-token/signature quota regressions cover the relevant behavioral guarantee.

The branch is currently marked conflicting against main; current-main changes since the PR base substantially reorganized tests/gateway/test_feishu.py, so this should be salvaged with conflict-aware test placement rather than treated as a clean merge.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 30, 2026
@egilewski
egilewski force-pushed the codex/security-issue-29154 branch from 46c1cfe to 6875794 Compare August 1, 2026 21:35
@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

One PR, #73406, directly addresses #29154. It moves Feishu webhook quota charging behind token and raw-body-signature authentication, while also adding encrypted-envelope handling, bounded anomaly tracking, malformed-payload containment, regression coverage, and configuration documentation.

Related pull requests

  • fix(feishu): authenticate webhook traffic before quota (#29154) #73406 best fix — (+1015/-65) — keep open with a salvage path: The diff directly fixes the reported cause by ensuring invalid tokens and signatures are rejected before the authenticated delivery quota is charged, with focused regressions covering both rejection paths and continued limiting of authenticated requests. Consistent with the automated keep-open review, the valuable quota-ordering change and its security tests should be preserved through conflict-aware placement on current main; the contributor also documented that the earlier Desktop E2E failure occurred during artifact finalization after all 37 Playwright tests passed, rather than indicating a Feishu-code failure.

Suggested consolidation

Keep #73406 open with a salvage path: rebase it onto current main and preserve the authentication-before-quota ordering plus the invalid-token, invalid-signature, and authenticated-rate-limit regressions, relocating the tests around the reorganized tests/gateway/test_feishu.py as needed. There are no competing PRs to close as duplicates.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I29154(["issue #29154 (open)"])
    P73406["PR #73406 (open)"]
    P73406 -->|best fix| I29154
    class I29154 open
    class P73406 open
    class P73406 best
    class P73406 target
    click I29154 "https://github.com/NousResearch/hermes-agent/issues/29154"
    click P73406 "https://github.com/NousResearch/hermes-agent/pull/73406"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 1 pull request and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 67 kB of PR diffs, 3 kB of issue/PR text, <1 kB of discussion (1 comments), 2 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

@egilewski
egilewski force-pushed the codex/security-issue-29154 branch from 6875794 to f035033 Compare August 3, 2026 17:47
@egilewski
egilewski force-pushed the codex/security-issue-29154 branch from f035033 to 0d4b658 Compare August 19, 2026 20:58
egilewski and others added 2 commits September 6, 2026 22:24
…29154)

Invalid tokens and signatures can exhaust the delivery bucket before legitimate
Feishu callbacks from the same address are authenticated. Malformed nested
payloads and unbounded anomaly records also reach work before authentication.

Charge delivery quota only after configured token and raw-body signature checks.
Require a verification token for unsigned URL challenges. Bound pre-auth parser
work separately, cap anomaly records, and contain malformed nested messages on
both webhook and websocket paths. Reuse one fixed-window limiter for both source
buckets. Keep encrypted envelopes unsupported in this first boundary.

Fixes NousResearch#29154
Co-authored-by: Teknium <127238744+teknium1@users.noreply.github.com>
…73406)

Feishu can wrap callback events and URL verification in an encrypted envelope.
Decrypt its IV-prefixed AES-CBC payload with the existing cryptography dependency
and pass it through the bounded parsing and authentication boundary.

Validate supplied raw-envelope signatures before decryption. Unsigned URL
verification remains supported only with a matching embedded verification token;
ordinary events still require the configured signature. Return uniform rejection
responses across invalid padding, decoding, parsing, and token checks so those
stages are not exposed through status or body differences. This is not a claim
of constant-time CBC processing. Missing cryptography produces a controlled 503.

Related NousResearch#29154
@egilewski
egilewski force-pushed the codex/security-issue-29154 branch from 0d4b658 to 7e5adcf Compare September 6, 2026 20:26
@egilewski egilewski changed the title fix(feishu): authenticate webhook traffic before quota fix(feishu): authenticate webhook traffic before quota (#29154) Sep 6, 2026
@egilewski

Copy link
Copy Markdown
Author

The conflict-aware refresh requested in the review is present on head 7e5adcf0580e94d9750678ae275cca864cc2fc11. The current implementation keeps token/signature authentication before quota charging, and the relocated regressions retain invalid-token, invalid-signature, and authenticated-rate-limit coverage around the reorganized Feishu tests.

Fresh readback reports the branch mergeable/clean and the required hosted check successful on that exact head. The earlier conflict and artifact-finalization failure are not current-head blockers.

Signed: GPT-5.6 in Codex

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have platform/feishu Feishu / Lark adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Security tracking] Feishu Webhook Pre-Authentication Rate-Limit Consumption Enables External Denial of Service (GHSA-jm8j-wwx3-97gc)

4 participants