fix(gateway): constantize Telegram API URL + reject empty LINE source IDs#678
Conversation
… IDs openabdev#10: Extract TELEGRAM_API_BASE constant (matching LINE's LINE_API_BASE). Replaces 3 hardcoded URLs. Enables future mock testing. openabdev#11: Replace unwrap_or_default() with explicit empty-check + reject for LINE source IDs (groupId, roomId, userId). Per LINE API contract, these IDs are guaranteed present for their respective source types. Empty ID = anomalous payload, reject with warning log (fail-closed). Existing LINE dispatch tests unaffected (test reply/push routing, not source parsing). Partial fix for openabdev#676
masami-agent
left a comment
There was a problem hiding this comment.
PR Review: #678
Summary
- Problem: Telegram API URLs hardcoded (no mock testing), LINE empty source IDs silently accepted
- Approach: Extract constant + fail-closed validation
- Risk level: #10 zero, #11 low (LINE API guarantees IDs present)
Core Assessment
- Problem clearly stated: ✅ — partial fix for #676
- Approach appropriate: ✅
- Best approach for now: ✅
#10 — Telegram URL constant ✅
TELEGRAM_API_BASEdefined aspub const— matches LINE'sLINE_API_BASEpattern- All 3 URLs replaced:
createForumTopic,setMessageReaction,sendMessage - URL values unchanged — pure refactor, zero behavior change
pubvisibility enables future test files to reference it
Verdict: No issues.
#11 — LINE empty source ID rejection ✅
- All 3
unwrap_or_default()replaced withmatch+ empty check +continue - Warning log includes source type and missing field name — good for debugging
continueskips only the affected event, not the entire webhook batch — correct
Security review (fail-closed check):
groupIdmissing → reject ✅roomIdmissing → reject ✅userIdmissing → reject ✅source: None→continue(already existed) ✅- No
if let/ silent pass patterns — all paths either produce a valid ID or reject ✅
LINE API contract verification:
Per LINE docs, source object schema guarantees:
type: group→groupIdpresenttype: room→roomIdpresenttype: user→userIdpresent
Empty ID would indicate anomalous payload. Rejecting is correct.
Existing tests: 6 LINE dispatch tests test reply/push routing with pre-constructed GatewayReply objects — they don't go through source parsing. Unaffected. ✅
Verdict
APPROVE — both changes are clean and correct.
Note: Cannot submit binding approval on own PR.
obrutjack
left a comment
There was a problem hiding this comment.
Clean small fixes — Telegram API URL constant + LINE empty source ID rejection. LGTM.
Consistency fix — all reject paths now have warning logs: - group missing groupId ✅ - room missing roomId ✅ - user missing userId ✅ - source missing (None) ✅ (this commit)
四法師 Triage Review — PR #678 (2026-05-01)LGTM ✅ — Clean hardening fixes. Ready to merge. Review Details🟢 INFO
🟡 NIT
🔴 SUGGESTED CHANGES
Reviewed by 超渡法師 on behalf of the 四法師 triage team. <@1493128125402320996> <@1496097857940361326> <@1496553369442189472> — 小 PR,hardening fixes,LGTM。有異議請補充。 |
|
All review feedback addressed — added warning log for LINE None source arm (commit d349b52). Ready for code owner review. |
Discord Discussion URL: https://discord.com/channels/1488041051187974246/1497258664090931280
Description
Two hardening fixes from the deferred review items (#676).
Changes
#10 — Telegram API URL constant
Extract
TELEGRAM_API_BASEconstant, replacing 3 hardcodedhttps://api.telegram.orgURLs. Matches LINE'sLINE_API_BASEpattern. Enables future mock testing for Telegram adapter.format!("https://api.telegram.org/bot{bot_token}/sendMessage")format!("{TELEGRAM_API_BASE}/bot{bot_token}/sendMessage")#11 — LINE empty source ID rejection
Replace
unwrap_or_default()with explicit empty-check + reject for LINE source IDs (groupId,roomId,userId).Per LINE API contract, these IDs are guaranteed present for their respective source types:
source.type == "group"→groupIdalways presentsource.type == "room"→roomIdalways presentsource.type == "user"→userIdalways presentEmpty ID = anomalous payload → reject with warning log (fail-closed).
s.group_id.clone().unwrap_or_default()(empty string enters session pool)match s.group_id { Some(id) if !id.is_empty() => ..., _ => { warn!(...); continue; } }Risk Assessment
Tests
Existing 6 LINE dispatch tests unaffected (test reply/push routing, not source parsing).
Partial fix for #676