fix(lark): handle link-share card content#5427
Conversation
|
Someone is attempting to deploy a commit to the IndexLabs Team on Vercel. A member of the Team first needs to authorize it. |
Bohan-J
left a comment
There was a problem hiding this comment.
Thanks for the fix, and especially for the test coverage — the interactive-card flattening and its graceful degradation read cleanly. Before merge, though, there are two gaps in the local-image path redaction that still let the absolute path reach Lark.
Must-fix
1. The sanitizer is bypassed for empty-alt images (leaks on macOS/Linux)
Outbound chat replies are routed in sendChatReply (server/internal/integrations/lark/outbound.go):
if containsMarkdown(content) {
// SendMarkdownCard ← sanitizeMarkdownForLarkCard only runs here
} else {
// SendTextMessage ← no sanitization
}containsMarkdown detects links/images with \[[^\]\n]+\]\([^)\n]+\), which requires at least one character inside the [...]. So an empty-alt image  does not match. If the reply contains no other markdown token, the whole message is routed to SendTextMessage, sanitizeMarkdownForLarkCard never runs, and the absolute local path is sent verbatim as plain text.
This reproduces with the exact shape used in the new test:
Scan this:

containsMarkdown(...) returns false here → text path → path leaked. It only returns true if the reply happens to also contain other markdown, or if the image has non-empty alt text. The new unit test stays green only because it calls SendMarkdownCard directly, bypassing the containsMarkdown router.
Suggestion: sanitize content once in sendChatReply before the containsMarkdown branch (or in both send paths), and add a regression test at the routing level (not directly against SendMarkdownCard) covering "plain text + empty-alt local image".
2. Redaction regex only covers /-prefixed (Unix) paths
var localMarkdownImageRE = regexp.MustCompile(`!\[([^\]]*)\]\((?:file://)?(/[^)]+)\)`)The path group requires a leading /, so Windows paths (C:\Users\..., C:/Users/...) and file://host/... forms are not redacted. On a Windows daemon / Desktop the local path still leaks, even on the card path. Please make the local-path detection platform-independent and add regression cases for Unix, Windows (incl. drive letter), and file: forms.
Nits
- The inbound interactive-card handling only extracts
title+card_link.url; generic template cards degrade to the[interactive card]placeholder. That fallback is fine as a first step, but the PR title "handle interactive card content" promises more than is delivered — consider scoping it to link-share cards for now. - The
open_id/app_namealignment change in theGetBotInfotest is an unrelated gofmt reflow; harmless but out of scope for this PR.
Direction is right — this mainly needs the redaction to be unconditional and cross-platform before it can be relied on for privacy.
a503d3f to
ea6864c
Compare
|
Implemented the requested local-image redaction fixes in
Validation:
Could you please take another look? |
Bohan-J
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround — the two items from the last review are properly resolved: redaction now runs in sendChatReply ahead of the routing decision, the regex covers Unix / drive-letter / file: forms, and the router-level regression test is in place.
One blocker remains before this can be relied on as a privacy boundary. The single [^)\s]+ destination matcher only recognizes the simplest space-free, title-free path, so several valid, common Markdown image forms still slip through. I reproduced each against the current head (ea6864ce):
| input | result |
|---|---|
 |
unchanged — path leaked |
 |
unchanged — path leaked (angle-bracket destination) |
 |
unchanged — path leaked (Windows UNC) |
 |
unchanged — path leaked (optional title) |
.png) |
becomes [scan omitted].png) — truncated at the first ), leaking the tail and corrupting the message |
None of these are exotic: spaces in C:\Program Files\ and C:\Users\First Last\, <...>-wrapped destinations, UNC shares, and optional titles are all standard CommonMark (spec §images). TestSanitizeMarkdownForLarkCard_LocalPathFormats only exercises simple no-space destinations, which is why these aren't caught.
The root cause is that a single "stop at the first space or paren" regex can't represent the image-destination grammar. Suggested direction rather than another regex tweak:
- Extract the image destination reliably first — handle
<...>wrapping, the optional title, and balanced/escaped parens — instead of approximating it inline. - Then classify the extracted destination as local (
file:, Unix absolute, Windows drive, UNC) vs remote, and redact when local. - Extend the table test with the five forms above, and keep the router-level assertion covering both the text and card send paths.
The product direction and the core fix are right — this last step is what makes "local paths never leave the box" actually hold. Requesting changes for it.
Co-authored-by: multica-agent <github@multica.ai>
|
Addressed the remaining local-image redaction blocker in
Validation:
Could you please take another look? |
Co-Authored-By: Claude <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai>
|
Updated the PR to handle CommonMark image labels with balanced brackets before parsing the destination. Added sanitizer coverage for nested and escaped label brackets, and routing-level coverage for both text and markdown-card paths with balanced-label local images. |
Co-Authored-By: Claude <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai>
|
Updated the PR to redact reference-style markdown images as well as inline images. The sanitizer now resolves full, collapsed, and shortcut image references against link definitions, redacts referenced local destinations, removes the local definition, and leaves remote definitions unchanged. Added sanitizer and routing-level coverage for the text and markdown-card paths. |
Co-Authored-By: Claude <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai>
|
Updated the PR to handle multiline link reference definitions. The sanitizer now parses definitions where the destination is on the next line with up to three spaces of indentation, reuses that parser when removing local definitions, and covers multiline Unix, Windows, UNC, file, remote, and optional-title cases in tests, plus text/card routing paths. |
Co-Authored-By: Claude <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai>
|
Updated the PR for the R4 review cases. The sanitizer now classifies destinations after CommonMark-style backslash/entity decoding, keeps raw remote output unchanged, avoids panics on truncated definition escapes, and recognizes reference definitions inside blockquote/list containers. Added sanitizer regression coverage plus text/card routing assertions for encoded local paths and container reference definitions. |
Co-Authored-By: Claude <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai>
|
Updated the PR for the latest review blockers. Reference labels now use Unicode case folding via golang.org/x/text/cases, and the definition scanner/removal path ignores fenced and indented code block lines so pseudo definitions inside code cannot shadow real local definitions. Added sanitizer coverage for Unicode label folding and fenced/indented code-block shadowing, plus text/card routing assertions for both paths. |
What does this PR do?
Fixes Lark interactive card content handling so rich inbound message bodies are flattened into readable markdown and local image paths are not exposed when markdown is sent back through Lark cards.
Related Issue
N/A
Type of Change
Changes Made
How to Test
cd servergo test ./internal/integrations/larkChecklist
apps/web/features/landing/i18n/) and relevant docs (apps/docs/content/docs/)apps/docs/content/docs/developers/conventions.zh.mdx(terminology, mixed-rule fortask/issue/skill)AI Disclosure
AI tool used: Multica Agent
Prompt / approach:
Reviewed the Lark interactive card content flow, followed existing backend test patterns, and verified the Lark integration package.
Screenshots (optional)
N/A