Skip to content

fix(channels): apply sender settings and preserve masked credentials - #6074

Merged
Astro-Han merged 4 commits into
mainfrom
fix/channels-settings-roundtrip
Oct 12, 2026
Merged

Astro-Han merged 4 commits into
mainfrom
fix/channels-settings-roundtrip

Conversation

@Astro-Han

@Astro-Han Astro-Han commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Apply configured sender restrictions at the shared bot adapter before message routing, replacing duplicated bridge checks. Empty sender lists retain their existing unrestricted behavior.

Preserve saved credentials when settings updates contain a current or legacy mask, keep save responses masked, and retain explicit clearing and onboarding rollback. Tavily and Jev use the same mask check. Update the private reporting address to ASF Security.

Verification

  • GitHub CI test passed for commit 85b4ee4de1d55fef3ef2dfac2f55918261603375.
  • Core: 36 related tests passed.
  • Runtime: all 109 bot tests passed.
  • Desktop: 81 related tests passed, including adapter-to-routing, settings persistence/reload, Host settings IPC and onboarding cancellation.
  • Desktop and workspace dependency builds, Desktop typecheck, format, lint, desktop/ui knip and diff check passed.
  • Source archive creation, verification, reproduction against the exact commit and extracted-source header audit passed.
  • Shipped dependency audit: 342 packages, zero moderate-or-higher advisories reaching the product closure.
  • Regression ablations fail when the relevant behavior is removed. No live platform credentials, full-repository tests or local E2E were used.

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Codex implemented the changes, regression tests and verification, with a separate Codex review.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@Astro-Han Astro-Han added the effort/L Under 1000 readable lines label Oct 12, 2026
@Astro-Han
Astro-Han marked this pull request as ready for review October 12, 2026 01:26

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Two independent review lineages found no P0–P2 issue in 85b4ee4de1d55fef3ef2dfac2f55918261603375.

The sender check is now applied in BaseBotAdapter.emitIncomingMessage before the message reaches the registry and Desktop routing. I checked the seven modified bridges, plus the existing Slack bridge that also uses this adapter. Direct/group messages, QQ's four dispatch types, DingTalk callbacks, Telegram commands and captionless media, both WeChat receive transports, and Slack thread messages all passed controlled tests through their actual receive handlers. Unlisted senders did not reach routing or update inbound activity; live allowlist changes did not require a restart, and empty/absent lists remained unrestricted. Unsupported edit/slash/callback event types are not new input paths. DingTalk and Slack still send their transport acknowledgments for rejected deliveries; those acknowledgments are not user-facing replies.

Credential preservation uses only the two exact mask strings, •••••••• and legacy ••••••, not a broad pattern. I exercised the actual settings store and client IPC across all eight providers: masked/omitted values preserved existing credentials, replacements survived reload, explicit empty strings cleared them, save responses stayed masked, and a mask could not create a credential in an empty field. Similar-looking values were not classified as masks. Controlled Host settings IPC tests confirmed the same preservation and clearing behavior for Tavily and Jev without copying their vault keys into Desktop settings. Onboarding cancellation still rolls back an initially absent secret, while its existing conditional rollback preserves a concurrent manual credential edit.

The changed private reporting address matches the ASF's published private security contact.

Validation passed: a fresh dependency install and normal complete test build, 36 related Core tests, all 109 Runtime bot tests, 63 Desktop tests, four Desktop typechecks, checks for all 25 changed TypeScript files, and the strict renderer architecture check. Separate probes passed 21 receive-path scenarios with 210 admission assertions and 146 credential cases. Removing the admission check let an unlisted sender through; removing credential preservation overwrote a stored token with a mask; removing onboarding's explicit rollback clear failed two cancellation tests. Restoring each behavior passed again. The controls did not alter tracked source.

CI run 38190737519 passed on this exact head. A trial merge with current main 1e1fde82 is conflict-free; the intervening main change is confined to CLI retraction handling. There is no database schema or Host protocol change, and the merged-result epoch check remains at 217.

These results use controlled SDK/transport and Host-vault fixtures, not live platform credentials, native Electron, or end-to-end platform sessions. I did not run the full repository test suite or independently repeat the source-archive/dependency-advisory audits described in the PR. Within those limits, I found no technical blocker in the changes.

Two P3 notes from a second, independent review lineage (inline below); neither blocks this change:

  • P3, pre-existing: bot credential fields are controlled inputs that save on every keystroke and re-render from the save result. With the reveal path removed, the save response is masked immediately, so typing into a field that already shows the mask can persist ••••••••x. On main the follow-up settings:get reload already masks the field, so this is not a regression. A follow-up could clear the field on focus when it shows the sentinel, or commit on blur.
  • P3, scope: the sender list is now enforced for every channel using the shared adapter, including Slack, which the description does not mention. The settings UI only exposes the list for Telegram, so lists already stored for other channels will start dropping senders. This matches the stated intent; a release note would help.

)
? channel.appSecret
: (maskSensitive(channel.appSecret) ?? ""),
token: maskSensitive(channel.token) ?? "",

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

P3 (pre-existing): save responses are now masked immediately. Bot credential PasswordInputs save on every keystroke and re-render from the response, so typing into a masked field persists ••••••••x as the credential. On main the follow-up settings:get reload already masked the field, so this is not a regression. A follow-up could clear the field on focus when it shows the sentinel, or commit on blur.

protected emitIncomingMessage(message: BotIncomingMessage): void {
protected emitIncomingMessage(message: BotIncomingMessage): boolean {
const allowedUserIds = this.settings.allowedUserIds;
if (allowedUserIds?.length && !allowedUserIds.includes(message.userId)) return false;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

P3 (scope): enforcement now also applies to Discord, DingTalk, QQ, WeChat and Slack. Slack is not in the PR description and has no test. The settings UI only exposes this field for Telegram, so any stored lists for other channels start dropping senders silently. This matches the stated intent, but it is worth a release note.

@M4n5ter M4n5ter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

English

The shared adapter is a sensible place to enforce sender restrictions before routing. Preserving credentials in the merge layer while consistently masking IPC responses also keeps the responsibilities clear.

I checked the adapter-to-routing path, credential round-trips, explicit clearing, and onboarding cancellation rollback. The related source-snapshot tests passed locally, and I haven’t found any blocking issues in this diff.

简体中文

在共享适配器中、消息进入路由前统一检查发送者限制是合理的。由设置合并层保留凭据、IPC 响应统一隐藏明文,也保持了清楚的职责划分。

我检查了适配器到路由的链路、凭据保存往返、明确清除和 onboarding 取消回滚。相关源码快照测试在本地通过,目前没有发现这份 diff 中需要阻断合并的问题。

@Astro-Han

Copy link
Copy Markdown
Contributor Author

Thanks @M4n5ter for the review and approval. Merging at the approved head 85b4ee4d.

@Astro-Han
Astro-Han merged commit fa15e64 into main Oct 12, 2026
3 checks passed
@Astro-Han
Astro-Han deleted the fix/channels-settings-roundtrip branch October 12, 2026 03:47
@Astro-Han Astro-Han mentioned this pull request Oct 12, 2026
4 of 6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/L Under 1000 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants