fix(feishu): normalize webhook rate-limit client keys [AI]

[pgondhi987]

Summary

• Problem: Feishu webhook rate-limit keys used raw socket address strings, so equivalent loopback client forms could land in separate buckets.
• Why it matters: Webhook burst throttling should apply consistently to the same local client identity before deeper request processing.
• What changed: The Feishu webhook transport now builds rate-limit keys through the webhook ingress IP resolver and folds loopback address-family variants into one client bucket.
• What did NOT change (scope boundary): No changes to webhook authentication, request body limits, Feishu event dispatch, or shared limiter semantics.

Change Type (select all)

• Bug fix
• Feature
• Refactor required for the fix
• Docs
• Security hardening
• Chore/infra

Scope (select all touched areas)

• Gateway / orchestration
• Skills / tool execution
• Auth / tokens
• Memory / storage
• Integrations
• API / contracts
• UI / DX
• CI/CD / infra

Linked Issue/PR

• Related: private maintainer tracking
• This PR addresses a bug or regression

Real behavior proof (required for external PRs)

• Behavior or issue addressed: Feishu webhook rate-limit key construction now treats loopback IPv4, IPv4-mapped IPv6, and IPv6 loopback forms as the same local client bucket.
• Real environment tested: Not run in this metadata drafting step.
• Exact steps or command run after this patch: Not run; supervisor owns deterministic verification commands.
• Evidence after change: Regression coverage added in extensions/feishu/src/monitor.webhook-security.test.ts.
• Observed result after change: Not executed in this step.
• What was not tested: Live dual-stack webhook harness and package-level command verification.
• Before evidence (optional but encouraged): N/A

Root Cause (if applicable)

• Root cause: Feishu webhook rate-limit keys included req.socket.remoteAddress directly instead of a normalized client identity.
• Missing detection / guardrail: Existing coverage checked rate limiting for a stable client key but did not lock equivalent loopback address-family forms to the same key.
• Contributing context (if known): The shared fixed-window limiter stores counters by exact key string, so caller-provided key normalization is required.

Regression Test Plan (if applicable)

• Coverage level that should have caught this:
  • Unit test
  • Seam / integration test
  • End-to-end test
  • Existing coverage already sufficient
• Target test or file: extensions/feishu/src/monitor.webhook-security.test.ts
• Scenario the test should lock in: Feishu webhook rate-limit key construction maps loopback IPv4, IPv4-mapped IPv6, and IPv6 loopback forms to one bucket.
• Why this is the smallest reliable guardrail: It verifies the key builder used by the request path without requiring a live dual-stack listener.
• Existing test that already covers this (if any): N/A
• If no new test is added, why not: N/A

User-visible / Behavior Changes

Feishu webhook burst throttling now applies consistently across equivalent loopback address-family forms for the same account and path.

Diagram (if applicable)

N/A

Security Impact (required)

• New permissions/capabilities? (Yes/No): No
• Secrets/tokens handling changed? (Yes/No): No
• New/changed network calls? (Yes/No): No
• Command/tool execution surface changed? (Yes/No): No
• Data access scope changed? (Yes/No): No
• If any Yes, explain risk + mitigation: N/A

Repro + Verification

Environment

• OS: Not run in this metadata drafting step
• Runtime/container: Not run in this metadata drafting step
• Model/provider: N/A
• Integration/channel (if any): Feishu webhook
• Relevant config (redacted): Webhook mode with default in-memory rate limiter

Steps

1. Start a Feishu webhook listener with dual-stack loopback reachability.
2. Send enough signed webhook requests over IPv4 loopback to reach the configured burst limit.
3. Send a follow-up signed webhook request over IPv6 loopback from the same local host.

Expected

• The IPv6 loopback follow-up uses the same local client bucket and receives the same throttling result once the bucket is exhausted.

Actual

• Not executed in this metadata drafting step.

Evidence

• Failing test/log before + passing after
• Trace/log snippets
• Screenshot/recording
• Perf numbers (if relevant)

Added regression coverage in extensions/feishu/src/monitor.webhook-security.test.ts; command execution is left to the supervisor workflow.

Human Verification (required)

What you personally verified (not just CI), and how:

• Verified scenarios: Reviewed the Feishu webhook transport key construction and confirmed the request path now uses the normalized key helper.
• Edge cases checked: Missing remote address, IPv4 loopback, IPv4-mapped IPv6 loopback, and IPv6 loopback key construction.
• What you did not verify: Live dual-stack listener behavior or package command output in this metadata drafting step.

Review Conversations

• I replied to or resolved every bot review conversation I addressed in this PR.
• I left unresolved only the conversations that still need reviewer or maintainer judgment.

Compatibility / Migration

• Backward compatible? (Yes/No): Yes
• Config/env changes? (Yes/No): No
• Migration needed? (Yes/No): No
• If yes, exact upgrade steps: N/A

Risks and Mitigations

• Risk: Loopback IPv4 and IPv6 forms now share one Feishu webhook burst bucket for a given account and path.
  • Mitigation: This is intentional normalization for equivalent local client identities; non-loopback addresses continue to use normalized IP identity.

[clawsweeper]

Codex review: needs real behavior proof before merge.

Summary
The PR re-exports resolveRequestClientIp, builds Feishu webhook rate-limit keys through a loopback-normalizing helper, and adds regression coverage for loopback, non-loopback, and unknown client key suffixes.

Reproducibility: yes. from source inspection: current main keys the Feishu webhook limiter with raw req.socket.remoteAddress, and the shared limiter stores exact caller keys. I did not run a live dual-stack listener in this read-only review.

Real behavior proof
Needs real behavior proof before merge: The PR body explicitly says no real environment or after-patch command was run; contributor-supplied redacted terminal output, logs, recording, or linked artifact is needed before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, ask a maintainer to comment @clawsweeper re-review.

Next step before merge
Needs contributor-supplied real behavior proof and explicit maintainer handling for the protected label; there is no narrow ClawSweeper repair to queue.

Security
Cleared: The diff narrows Feishu webhook rate-limit identity handling without adding permissions, secret handling, dependencies, CI, or code-execution surface.

Review details

Best possible solution:

Keep the narrow Feishu plugin fix if maintainer review agrees, with redacted real runtime proof and focused verification before merge.

Do we have a high-confidence way to reproduce the issue?

Yes from source inspection: current main keys the Feishu webhook limiter with raw req.socket.remoteAddress, and the shared limiter stores exact caller keys. I did not run a live dual-stack listener in this read-only review.

Is this the best way to solve the issue?

Yes: using the existing SDK client-IP resolver plus a plugin-local loopback bucket is narrower than changing shared limiter semantics. The path still needs real behavior proof and maintainer review before merge.

What I checked:

• Protected label and proof gap: Live PR metadata shows the PR is open, authored by a contributor, has the protected maintainer label, and the body says no real environment or after-patch command was run. (7de2ccd6a58a)
• PR diff: The patch adds normalizeFeishuWebhookRateLimitClient, buildFeishuWebhookRateLimitKey, switches the request path to resolveRequestClientIp(req), and adds focused key-normalization tests. (extensions/feishu/src/monitor.transport.ts:91, 7de2ccd6a58a)
• Current main raw key behavior: Current main still builds the Feishu webhook limiter key from accountId, path, and raw req.socket.remoteAddress, so equivalent address-family strings can occupy separate buckets. (extensions/feishu/src/monitor.transport.ts:303, 485e04266c56)
• Limiter contract: applyBasicWebhookRequestGuards passes the caller-provided rateLimitKey directly to the fixed-window limiter, so caller-side key normalization is the right boundary. (src/plugin-sdk/webhook-request-guards.ts:169, 485e04266c56)
• Resolver contract: The SDK webhook ingress surface already exports resolveRequestClientIp, which resolves socket addresses through normalized client-IP handling. (src/plugin-sdk/webhook-ingress.ts:43, 485e04266c56)
• IP normalization support: The shared IP helper normalizes IPv4-mapped IPv6 addresses to IPv4 form before returning the normalized address string. (src/shared/net/ip.ts:145, 485e04266c56)

Likely related people:

• @steipete: Local blame attributes the sampled current Feishu webhook and shared webhook guard lines to the current grafted commit, and GitHub path history shows repeated recent work on Feishu transport and webhook SDK seams. (role: recent area contributor; confidence: high; commits: a765668e6344, f46bd2e0cc17, 3a68c56264bb; files: extensions/feishu/src/monitor.transport.ts, src/plugin-sdk/webhook-request-guards.ts, src/plugin-sdk/webhook-ingress.ts)
• @vincentkoc: GitHub path history shows recent substantive Feishu monitor transport and monitor state work adjacent to this webhook transport path. (role: recent Feishu contributor; confidence: medium; commits: f9b47ad2a169, d93e6f61580a, 2fff6be4c393; files: extensions/feishu/src/monitor.transport.ts, extensions/feishu/src/monitor.state.ts)
• @gumadeiras: GitHub path history shows Feishu plugin SDK import migration work, which is relevant to this PR's local runtime barrel and SDK export boundary. (role: adjacent plugin SDK boundary contributor; confidence: medium; commits: 3e1ca111afcd, 1ebd1fdb2d40; files: extensions/feishu/src/monitor.transport.ts, extensions/feishu/src/monitor-transport-runtime-api.ts)

Remaining risk / open question:

• External-contributor real behavior proof is absent; the PR body says no real environment or after-patch command was run.
• The protected maintainer label means this PR should not be closed or landed by cleanup automation without explicit maintainer handling.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 485e04266c56.

[pgondhi987]

Not applicable to this automation stage; changelog/release-note and external real behavior proof requirements are handled outside auto-pr stages.

Quoted comment from @clawsweeper:

Codex review: needs real behavior proof before merge.

Summary
The PR re-exports resolveRequestClientIp, builds Feishu webhook rate-limit keys through a loopback-normalizing helper, and adds regression coverage for loopback, non-loopback, and unknown client key suffixes.

Reproducibility: yes. from source inspection: current main keys the Feishu webhook limiter with raw req.socket.remoteAddress, and the shared limiter stores exact caller keys. I did not run a live dual-stack listener in this read-only review.

Real behavior proof
Needs real behavior proof before merge: The PR body explicitly says no real environment or after-patch command was run; contributor-supplied redacted terminal output, logs, recording, or linked artifact is needed before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, ask a maintainer to comment @clawsweeper re-review.

Next step before merge
Needs contributor-supplied real behavior proof and explicit maintainer handling for the protected label; there is no narrow ClawSweeper repair to queue.

Security
Cleared: The diff narrows Feishu webhook rate-limit identity handling without adding permissions, secret handling, dependencies, CI, or code-execution surface.

Review details

Best possible solution:

Keep the narrow Feishu plugin fix if maintainer review agrees, with redacted real runtime proof and focused verification before merge.

Do we have a high-confidence way to reproduce the issue?

Yes from source inspection: current main keys the Feishu webhook limiter with raw req.socket.remoteAddress, and the shared limiter stores exact caller keys. I did not run a live dual-stack listener in this read-only review.

Is this the best way to solve the issue?

Yes: using the existing SDK client-IP resolver plus a plugin-local loopback bucket is narrower than changing shared limiter semantics. The path still needs real behavior proof and maintainer review before merge.

What I checked:

• Protected label and proof gap: Live PR metadata shows the PR is open, authored by a contributor, has the protected maintainer label, and the body says no real environment or after-patch command was run. (7de2ccd6a58a)
• PR diff: The patch adds normalizeFeishuWebhookRateLimitClient, buildFeishuWebhookRateLimitKey, switches the request path to resolveRequestClientIp(req), and adds focused key-normalization tests. (extensions/feishu/src/monitor.transport.ts:91, 7de2ccd6a58a)
• Current main raw key behavior: Current main still builds the Feishu webhook limiter key from accountId, path, and raw req.socket.remoteAddress, so equivalent address-family strings can occupy separate buckets. (extensions/feishu/src/monitor.transport.ts:303, 485e04266c56)
• Limiter contract: applyBasicWebhookRequestGuards passes the caller-provided rateLimitKey directly to the fixed-window limiter, so caller-side key normalization is the right boundary. (src/plugin-sdk/webhook-request-guards.ts:169, 485e04266c56)
• Resolver contract: The SDK webhook ingress surface already exports resolveRequestClientIp, which resolves socket addresses through normalized client-IP handling. (src/plugin-sdk/webhook-ingress.ts:43, 485e04266c56)
• IP normalization support: The shared IP helper normalizes IPv4-mapped IPv6 addresses to IPv4 form before returning the normalized address string. (src/shared/net/ip.ts:145, 485e04266c56)

Likely related people:

• @steipete: Local blame attributes the sampled current Feishu webhook and shared webhook guard lines to the current grafted commit, and GitHub path history shows repeated recent work on Feishu transport and webhook SDK seams. (role: recent area contributor; confidence: high; commits: a765668e6344, f46bd2e0cc17, 3a68c56264bb; files: extensions/feishu/src/monitor.transport.ts, src/plugin-sdk/webhook-request-guards.ts, src/plugin-sdk/webhook-ingress.ts)
• @vincentkoc: GitHub path history shows recent substantive Feishu monitor transport and monitor state work adjacent to this webhook transport path. (role: recent Feishu contributor; confidence: medium; commits: f9b47ad2a169, d93e6f61580a, 2fff6be4c393; files: extensions/feishu/src/monitor.transport.ts, extensions/feishu/src/monitor.state.ts)
• @gumadeiras: GitHub path history shows Feishu plugin SDK import migration work, which is relevant to this PR's local runtime barrel and SDK export boundary. (role: adjacent plugin SDK boundary contributor; confidence: medium; commits: 3e1ca111afcd, 1ebd1fdb2d40; files: extensions/feishu/src/monitor.transport.ts, extensions/feishu/src/monitor-transport-runtime-api.ts)

Remaining risk / open question:

• External-contributor real behavior proof is absent; the PR body says no real environment or after-patch command was run.
• The protected maintainer label means this PR should not be closed or landed by cleanup automation without explicit maintainer handling.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 485e04266c56.