Skip to content

fix(feishu): sanitize unsupported markdown and route tables through post/md - #62354

Open
linhuchong wants to merge 1 commit into
NousResearch:mainfrom
linhuchong:fix/feishu-markdown-sanitize
Open

fix(feishu): sanitize unsupported markdown and route tables through post/md#62354
linhuchong wants to merge 1 commit into
NousResearch:mainfrom
linhuchong:fix/feishu-markdown-sanitize

Conversation

@linhuchong

Copy link
Copy Markdown

Problem

The Feishu adapter forced markdown tables into plain-text mode (msg_type="text"), rendering them as raw pipe characters on the client. Other unsupported syntax (HTML <details>/<summary>, <kbd>, <mark>, <span>, <sup>/<sub>, ==highlight==, Mermaid diagrams, inline images) passed through unmodified, causing blank or malformed output.

Root Cause

_build_outbound_payload detected markdown tables via _MARKDOWN_TABLE_RE and routed them to plain text (commit 8e18d1031). No sanitization layer existed for other Feishu-unsupported syntax.

Changes

  • Route tables through post/md instead of forcing plain-text mode. Feishu post md elements support GFM tables.
  • Add _sanitize_feishu_markdown() — rewrites unsupported syntax into Feishu-compatible equivalents:
    • ==highlight==**bold**
    • <details>/<summary>**title**\nbody
    • <kbd>, <mark>, <span>, <div> → stripped
    • <sup>/<sub> → stripped
    • Mermaid blocks → fenced text code block with descriptive label
    • Inline markdown/HTML images → readable text links
    • Code blocks are skipped (preserved as-is)
  • Override extract_images() to skip auto-downloading third-party image URLs that would block text replies on the Feishu send path. Explicit MEDIA: / send_image / send_image_file still use the native upload flow.
  • Apply sanitization in all text-fallback paths so even degraded messages are clean.

Test Plan

  • Send a message containing a GFM table — verify it renders as a table, not raw pipes
  • Send a message with ==highlight== — verify it renders as bold
  • Send a message with <details><summary> — verify it renders as bold title + body
  • Send a message with a Mermaid diagram — verify it renders as a text code block
  • Send a message with inline image markdown — verify it renders as a text link, not blank
  • Send a plain text message — verify no regression
  • Send a message with code blocks — verify code blocks are preserved unchanged

Notes

  • The _MARKDOWN_TABLE_RE workaround (commit 8e18d1031, 2026-04-22) is superseded by this change.
  • No changes to interactive card or approval button code paths.
  • Fallback chain remains: post/mdtext (with sanitization).

…ost/md

The Feishu adapter forced markdown tables into plain-text mode, rendering
them as raw pipe characters on the client. Meanwhile, other unsupported
syntax (HTML details/summary, kbd, mark, span, sup/sub, ==highlight==,
Mermaid, inline images) passed through unmodified, causing blank or
malformed output.

Changes:
- Route tables through post/md instead of forcing text mode
- Add _sanitize_feishu_markdown() to rewrite unsupported syntax into
  Feishu-compatible equivalents (e.g. ==highlight== -> **bold**,
  <details>/<summary> -> **title**\nbody, Mermaid -> fenced text block)
- Override extract_images() to skip auto-downloading third-party image
  URLs that would block text replies
- Apply sanitization in all text-fallback paths

The table rendering workaround in _build_outbound_payload (commit 8e18d10)
is superseded by this change.
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have platform/feishu Feishu / Lark adapter comp/plugins Plugin system and bundled plugins sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Jul 10, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: this joins the long-standing Feishu markdown-table cluster (#53355, #57566, #33800, #61647, canonical native-card #12114). The table-rendering half here (remove the force-text branch, route tables through post/md) overlaps those competing PRs; this PR additionally adds a broader _sanitize_feishu_markdown() layer for other unsupported syntax (<details>, <kbd>, <mark>, Mermaid, inline images). Flagging for a maintainer to pick one approach for the cluster — not marking any as a duplicate since the mechanisms and scopes differ.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing Feishu output compatibility. The current implementation needs revision before its table-routing and sanitizer changes are safe.

Problems

  • plugins/platforms/feishu/adapter.py:4673-4674 reverses the current workaround, but main still documents that post/md table rendering produced blank client messages (plugins/platforms/feishu/adapter.py:4525-4530; commit 8e18d1031). Please provide an integration-backed reason to change that route.
  • _sanitize_feishu_markdown() sanitizes the whole input at adapter.py:646 before it tracks fences at :655; thus HTML-like content inside fenced code is modified despite the stated code-preservation guarantee.
  • Standard fenced Mermaid enters the generic fence branch at :655 and never reaches the Mermaid conversion at :661.
  • The one-file diff adds no tests for these new delivery paths; current tests/gateway/test_feishu.py only covers generic md-post construction around :2577.

Suggested changes

  • Preserve/protect fenced content before sanitization, explicitly handle ```mermaid fences, and add focused adapter tests. Keep the existing table fallback unless a real Feishu rendering test demonstrates that post/md now renders tables.

Automated hermes-sweeper review.

if not content:
return content

normalized = _unwrap_feishu_unsupported_html(content.replace("\r\n", "\n"))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This normalizes HTML before the fence-state loop starts, so <kbd>, <details>, and similar source inside a fenced code block are changed before lines 655-676 can preserve the block. Please protect/split fenced regions first, then sanitize only prose.

while index < len(lines):
raw_line = lines[index]
stripped = raw_line.strip()
if _MARKDOWN_FENCE_OPEN_RE.match(stripped) or _MARKDOWN_FENCE_CLOSE_RE.match(stripped):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A standard fenced Mermaid block begins with ```mermaid, so this generic fence branch preserves it and its graph/`flowchart` body never reaches the conversion at line 661. Detect the Mermaid fence language before this branch and cover it with a test.

text_payload = {"text": content}
safe_content = _sanitize_feishu_markdown(content)
if _MARKDOWN_TABLE_RE.search(safe_content):
return "post", _build_markdown_post_payload(safe_content)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reverses the current workaround whose documented purpose is preventing blank table messages in Feishu post/md (8e18d1031; current main adapter.py:4525-4530). Please retain text routing until a Feishu integration test or authoritative API evidence proves post/md now renders these tables.

@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 11, 2026
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 type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants