Repository navigation
Conversation
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR adds an optional ingress relay feature for Lark that forwards events to external durable consumers. The implementation includes HMAC signature verification, request validation, and comprehensive test coverage.
Critical Issues Found
Three blocking issues were identified that must be addressed before merge:
- Security: Replay Attack Vulnerability - The HMAC timestamp has no expiration check, allowing captured requests to be replayed indefinitely (CWE-294)
- Logic: Complex Validation Expression - Line 51 contains an excessively complex 285-character validation that is error-prone and difficult to verify
- Error Handling: Marshaling Error Masked - JSON marshaling errors are not properly separated from size limit checks, potentially masking serialization failures
Architecture & Testing
The overall design is sound:
- Clean separation between ingress relay and private agent authority
- Proper fail-closed behavior for receiver outages
- Comprehensive test coverage including duplicate handling, HMAC validation, and routing logic
- HTTP client properly configured with 2s timeout and redirect blocking
Once the critical issues are resolved, this implementation will provide a secure foundation for durable event forwarding.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| timestamp := strconv.FormatInt(time.Now().Unix(), 10) | ||
| mac := hmac.New(sha256.New, []byte(r.config.Secret)) | ||
| mac.Write([]byte(timestamp + ".")) | ||
| mac.Write(body) |
There was a problem hiding this comment.
🛑 Security Vulnerability: The timestamp in HMAC signature verification has no expiration check, allowing replay attacks. An attacker who intercepts a valid signed request can replay it indefinitely since time.Now().Unix() generates the timestamp but never validates its freshness.1
Add timestamp validation to reject requests outside an acceptable time window (e.g., ±5 minutes). The receiver should verify the timestamp freshness before processing the event.
Footnotes
-
CWE-294: Authentication Bypass by Capture-replay - https://cwe.mitre.org/data/definitions/294.html ↩
| body, err := json.Marshal(ingressEnvelope{Version: 1, InstallationID: uuidString(inst.ID), WorkspaceID: uuidString(inst.WorkspaceID), AgentID: uuidString(inst.AgentID), AppID: m.AppID, EventID: m.EventID, EventType: m.EventType, ChatID: string(m.ChatID), ChatType: string(m.ChatType), MessageID: m.MessageID, SenderID: string(m.SenderOpenID), SenderType: m.SenderType, MessageType: m.MessageType, Content: m.Content, CreateTime: m.CreateTime, ParentID: m.ParentID, RootID: m.RootID, ThreadID: m.ThreadID, AddressedToBot: m.AddressedToBot, CommandBody: m.CommandBody}) | ||
| if err != nil || len(body) > 512<<10 { | ||
| return false, errors.New("ingress source exceeds limit") | ||
| } |
There was a problem hiding this comment.
Missing error handling: If json.Marshal returns an error on line 102, the function returns the error but leaves body uninitialized. The subsequent check len(body) > 512<<10 on line 103 will evaluate against a nil/empty slice instead of detecting the marshaling failure. This could mask serialization errors and allow processing to continue with invalid data.
| body, err := json.Marshal(ingressEnvelope{Version: 1, InstallationID: uuidString(inst.ID), WorkspaceID: uuidString(inst.WorkspaceID), AgentID: uuidString(inst.AgentID), AppID: m.AppID, EventID: m.EventID, EventType: m.EventType, ChatID: string(m.ChatID), ChatType: string(m.ChatType), MessageID: m.MessageID, SenderID: string(m.SenderOpenID), SenderType: m.SenderType, MessageType: m.MessageType, Content: m.Content, CreateTime: m.CreateTime, ParentID: m.ParentID, RootID: m.RootID, ThreadID: m.ThreadID, AddressedToBot: m.AddressedToBot, CommandBody: m.CommandBody}) | |
| if err != nil || len(body) > 512<<10 { | |
| return false, errors.New("ingress source exceeds limit") | |
| } | |
| body, err := json.Marshal(ingressEnvelope{Version: 1, InstallationID: uuidString(inst.ID), WorkspaceID: uuidString(inst.WorkspaceID), AgentID: uuidString(inst.AgentID), AppID: m.AppID, EventID: m.EventID, EventType: m.EventType, ChatID: string(m.ChatID), ChatType: string(m.ChatType), MessageID: m.MessageID, SenderID: string(m.SenderOpenID), SenderType: m.SenderType, MessageType: m.MessageType, Content: m.Content, CreateTime: m.CreateTime, ParentID: m.ParentID, RootID: m.RootID, ThreadID: m.ThreadID, AddressedToBot: m.AddressedToBot, CommandBody: m.CommandBody}) | |
| if err != nil { | |
| return false, errors.New("ingress source exceeds limit") | |
| } | |
| if len(body) > 512<<10 { | |
| return false, errors.New("ingress source exceeds limit") | |
| } |
| } | ||
| u, err := url.Parse(c.Endpoint) | ||
| _, idErr := util.ParseUUID(c.InstallationID) | ||
| if err != nil || u.Host == "" || u.User != nil || u.Fragment != "" || u.RawQuery != "" || (u.Scheme != "https" && !(u.Scheme == "http" && (u.Hostname() == "localhost" || u.Hostname() == "127.0.0.1" || u.Hostname() == "::1"))) || idErr != nil || !strings.HasPrefix(c.AppID, "cli_") || !strings.HasPrefix(c.ChatID, "oc_") || len(c.Secret) < 32 || (c.Disposition != "consume" && c.Disposition != "observe") { |
There was a problem hiding this comment.
🛑 Logic Error: Line 51 validation condition is excessively complex (285+ characters on one line) and difficult to verify for correctness. This creates high risk of logic bugs in security-critical configuration validation. Complex boolean expressions like this are prone to operator precedence errors and missing edge cases that could allow invalid configurations to pass validation.
| if err != nil || u.Host == "" || u.User != nil || u.Fragment != "" || u.RawQuery != "" || (u.Scheme != "https" && !(u.Scheme == "http" && (u.Hostname() == "localhost" || u.Hostname() == "127.0.0.1" || u.Hostname() == "::1"))) || idErr != nil || !strings.HasPrefix(c.AppID, "cli_") || !strings.HasPrefix(c.ChatID, "oc_") || len(c.Secret) < 32 || (c.Disposition != "consume" && c.Disposition != "observe") { | |
| if err != nil { | |
| return nil, errors.New("invalid ingress relay target, secret or disposition") | |
| } | |
| if u.Host == "" || u.User != nil || u.Fragment != "" || u.RawQuery != "" { | |
| return nil, errors.New("invalid ingress relay target, secret or disposition") | |
| } | |
| isLocalHTTP := u.Scheme == "http" && (u.Hostname() == "localhost" || u.Hostname() == "127.0.0.1" || u.Hostname() == "::1") | |
| if u.Scheme != "https" && !isLocalHTTP { | |
| return nil, errors.New("invalid ingress relay target, secret or disposition") | |
| } | |
| if idErr != nil || !strings.HasPrefix(c.AppID, "cli_") || !strings.HasPrefix(c.ChatID, "oc_") { | |
| return nil, errors.New("invalid ingress relay target, secret or disposition") | |
| } | |
| if len(c.Secret) < 32 || (c.Disposition != "consume" && c.Disposition != "observe") { | |
| return nil, errors.New("invalid ingress relay target, secret or disposition") | |
| } |
1 participant
What does this PR do?
Provide an optional, disabled-by-default relay from the existing native Lark connection to a durable external consumer. This replaces the product-heavy approach in #13 and keeps installation workflows outside Multica.
The relay runs after active installation resolution and before native mention filtering. Configuration pins one installation, app and group. A signed, versioned literal event must receive a matching durable receipt before the connector acknowledges it.
Related Issue
Supersedes the product implementation in #13.
Type of Change
Changes Made
Risks
A configured receiver outage NACKs events; provider retries are finite, so the external consumer must reconcile missed history. Consume mode deliberately owns all events in the selected group. Switching observe/consume requires a coordinated receiver configuration change. This PR does not enable or deploy the relay.
How to Test
Passed from server/:
go test ./internal/integrations/lark ./internal/integrations/channel/engine ./cmd/server -count=1Tests cover matching and nonmatching routing, duplicate durable receipts, HMAC, literal payload isolation, receipt errors, rejected status and redirects.
git diff --checkpassed. No UI changes, live Lark calls, real-agent tests or production activation; frontend suites were not run.Checklist
AI Disclosure
AI tool used: Codex
Prompt / approach: Keep the product close to upstream; move installation automation outside the product and retain only the smallest reusable ingress contract. Consequential code review covered both sides of the boundary. No merge or deployment is requested.