fix(feishu): carry forward DM fallback and topic labels#73399
fix(feishu): carry forward DM fallback and topic labels#73399vincentkoc wants to merge 2 commits intomainfrom
Conversation
🔒 Aisle Security AnalysisWe found 3 potential security issue(s) in this PR:
1. 🟡 Potential sender name misattribution in Feishu DM member lookup fallback
Description
Vulnerable code: const matchedMember =
members.find((member) => member.member_id?.trim() === senderId) ??
(members.length === 1 ? members[0] : undefined);RecommendationRemove the non-matching single-item fallback, or make it safe by ensuring the selected member is actually the sender. Safer approach (only accept exact match; otherwise fall back to contact lookup): const matched = members.find(m => m.member_id?.trim() === senderId);
const name = normalizeName(matched?.name);
if (!name) return {};
return { name };If you need to handle pagination/incomplete results, explicitly page through results until either:
Additionally consider logging when the sender cannot be found in the member list for a p2p/private chat to catch API/permission anomalies. 2. 🟡 Unbounded in-memory caches for Feishu sender/direct-member names can cause memory growth (DoS)
Description
Vulnerable code: const senderNameCache = new Map<string, { name: string; expireAt: number }>();
const directMemberNameCache = new Map<string, { name: string; expireAt: number }>();
...
directMemberNameCache.set(cacheKey, { name, expireAt: now + SENDER_NAME_TTL_MS });
...
senderNameCache.set(cacheKey, { name, expireAt: now + SENDER_NAME_TTL_MS });RecommendationBound and/or actively prune the caches. Options:
function getWithTtl<T>(m: Map<string, {value: T; expireAt: number}>, k: string): T | undefined {
const v = m.get(k);
if (!v) return;
if (v.expireAt <= Date.now()) {
m.delete(k);
return;
}
return v.value;
}
function pruneExpired(m: Map<string, {expireAt: number}>, maxToScan = 1000) {
const now = Date.now();
let scanned = 0;
for (const [k, v] of m) {
if (scanned++ > maxToScan) break;
if (v.expireAt <= now) m.delete(k);
}
}
import LRUCache from "lru-cache";
const senderNameCache = new LRUCache<string, string>({ max: 5000, ttl: SENDER_NAME_TTL_MS });Also consider limiting caching for DM chat member lookups (cache only by senderId, or cap per-account/per-chat) to reduce key cardinality. 3. 🟡 Untrusted Feishu error URL injected into agent System prompt (phishing/prompt injection)
Description
Impact:
Vulnerable code (extraction + propagation):
RecommendationTreat
function isAllowedFeishuGrantUrl(raw: string): boolean {
try {
const u = new URL(raw);
return (
u.protocol === "https:" &&
["open.feishu.cn", "open.larksuite.com"].includes(u.hostname) &&
u.pathname.startsWith("/app/")
);
} catch {
return false;
}
}
Example usage: const raw = permissionErrorForAgent.grantUrl;
const safeUrl = raw && isAllowedFeishuGrantUrl(raw) ? raw : undefined;
messageBody += `\n\n[System: ... Permission grant URL: ${safeUrl ?? "(unavailable)"}]`;
Analyzed PR: #73399 at commit Last updated on: 2026-04-28T07:56:58Z |
Greptile SummaryThis PR carries forward the remaining Feishu DM display-name behavior from #38958 on top of the already-merged group-name baseline from #51032. It adds a Confidence Score: 4/5Safe to merge; no runtime-breaking issues found — only minor style/observability improvements suggested. All findings are P2. The logic for DM fallback, cache scoping, permission-error propagation, and topic-label gating is correct. Tests cover the primary scenarios including per-account cache isolation and stale-scope suppression. extensions/feishu/src/bot-sender-name.ts — review the extractPermissionError dual-path fallback and the absence of negative-hit caching. Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/feishu/src/bot-sender-name.ts
Line: 52
Comment:
**`extractPermissionError` now treats the raw error object as data**
`const data = axiosErr.response?.data ?? err` means any object with `code: 99991672` will now be matched as a Feishu permission error even without an axios `response` wrapper. This is intentional for the non-throw path where `extractPermissionError(res)` is called directly on an API response object (line 132), and it correctly handles the test cases for thrown plain objects. The risk is that a non-Feishu error coincidentally carrying `code: 99991672` would be misidentified — very unlikely given how specific that code is, but worth a brief comment explaining the dual-path design.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/feishu/src/bot-sender-name.ts
Line: 127-130
Comment:
**No negative-hit caching for direct member lookup**
When the API call succeeds but no matching member is found (e.g., `matchedMember` is undefined or the name trims to empty), the function returns `{}` without caching anything. This means every subsequent message in the same DM will trigger another `chatMembers.get` call until a name is resolved. The existing contact (`senderNameCache`) path has the same omission. For DMs this is limited to 2 members per call and the TTL would cap the problem, but consider storing a sentinel or using the same TTL on negative hits to bound API usage when a user genuinely has no display name configured.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(clownfish): address review for ghcra..." | Re-trigger Greptile |
| } | ||
| const axiosErr = err as { response?: { data?: unknown } }; | ||
| const data = axiosErr.response?.data; | ||
| const data = axiosErr.response?.data ?? err; |
There was a problem hiding this comment.
extractPermissionError now treats the raw error object as data
const data = axiosErr.response?.data ?? err means any object with code: 99991672 will now be matched as a Feishu permission error even without an axios response wrapper. This is intentional for the non-throw path where extractPermissionError(res) is called directly on an API response object (line 132), and it correctly handles the test cases for thrown plain objects. The risk is that a non-Feishu error coincidentally carrying code: 99991672 would be misidentified — very unlikely given how specific that code is, but worth a brief comment explaining the dual-path design.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/feishu/src/bot-sender-name.ts
Line: 52
Comment:
**`extractPermissionError` now treats the raw error object as data**
`const data = axiosErr.response?.data ?? err` means any object with `code: 99991672` will now be matched as a Feishu permission error even without an axios `response` wrapper. This is intentional for the non-throw path where `extractPermissionError(res)` is called directly on an API response object (line 132), and it correctly handles the test cases for thrown plain objects. The risk is that a non-Feishu error coincidentally carrying `code: 99991672` would be misidentified — very unlikely given how specific that code is, but worth a brief comment explaining the dual-path design.
How can I resolve this? If you propose a fix, please make it concise.| const res: FeishuChatMembersGetResponse = await getChatMembers({ | ||
| path: { chat_id: normalizedChatId }, | ||
| params: { page_size: 10, member_id_type: memberIdType }, | ||
| }); |
There was a problem hiding this comment.
When the API call succeeds but no matching member is found (e.g., matchedMember is undefined or the name trims to empty), the function returns {} without caching anything. This means every subsequent message in the same DM will trigger another chatMembers.get call until a name is resolved. The existing contact (senderNameCache) path has the same omission. For DMs this is limited to 2 members per call and the TTL would cap the problem, but consider storing a sentinel or using the same TTL on negative hits to bound API usage when a user genuinely has no display name configured.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/feishu/src/bot-sender-name.ts
Line: 127-130
Comment:
**No negative-hit caching for direct member lookup**
When the API call succeeds but no matching member is found (e.g., `matchedMember` is undefined or the name trims to empty), the function returns `{}` without caching anything. This means every subsequent message in the same DM will trigger another `chatMembers.get` call until a name is resolved. The existing contact (`senderNameCache`) path has the same omission. For DMs this is limited to 2 members per call and the TTL would cap the problem, but consider storing a sentinel or using the same TTL on negative hits to bound API usage when a user genuinely has no display name configured.
How can I resolve this? If you propose a fix, please make it concise.|
Codex review: found issues before merge. Summary Reproducibility: yes. A focused unit test can mock Next step before merge Security Review findings
Review detailsBest possible solution: Keep the Feishu-plugin change set, but repair the PR head by requiring exact sender matches, bounding/pruning the new caches, validating Feishu/Lark grant URL hosts, rebasing cleanly, and rerunning focused Feishu tests plus the changed gate. Do we have a high-confidence way to reproduce the issue? Yes. A focused unit test can mock Is this the best way to solve the issue? No, not as submitted. The product direction is narrow and in the right plugin, but the maintainable implementation needs exact attribution, bounded cache behavior, URL validation, and a clean rebase before merge. Full review comments:
Overall correctness: patch is incorrect Security concerns:
Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 4a7e60d05bcd. |
|
/clawsweeper automerge |
|
🦞🦞
Draft PRs stay fix-only until GitHub marks them ready for review. Pause with Automerge progress:
|
|
🦞🦞 Source: I added |
Summary
Carry forward the remaining Feishu display-name behavior from #38958 after #51032 already landed the group-name session label fix.
This replacement should stay narrow:
Credit: this work is based on the implementation direction and review iterations from @futuremind2026 in #38958. The already-merged group-name baseline came from @jnuyao in #51032.
Validation
ProjectClownfish replacement details: