feat: let the host channel use a custom notifier command - #3
Open
fengfanfan-max wants to merge 5 commits into
Open
fengfanfan-max wants to merge 5 commits into
fengfanfan-max wants to merge 5 commits into
Conversation
The host system channel hard-codes the operating system's own notification
command. That is not always usable: on macOS the identity behind `osascript
display notification` is inherited from the host app up the launch chain, so
when that app never asked for notification permission (a terminal whose
`app-notifications` is `never`, a launchd agent with no GUI session) macOS
drops every notification — while `osascript` still exits 0, so `runExecFile`
resolves and the plugin reports success it never delivered.
Add `system.notifier` as an escape hatch:
system:
enabled: true
notifier:
command: /opt/homebrew/bin/terminal-notifier
args: ["-title", "{{title}}", "-message", "{{body}}"]
An empty `command` keeps the OS default and is the default value, so nothing
changes for anyone who does not opt in. `{{title}}` / `{{body}}` interpolate
with the same shape `renderTemplate` uses; unknown tokens are left intact.
Besides unblocking those environments, a real notifier also reports real exit
codes (terminal-notifier: 3 = not authorized), so a failed delivery now reaches
`runJobs`' existing warning path instead of a silent success.
The settings page has no fields for this yet: it is an advanced escape hatch,
kept config-file-only (the README documents cordis.patch.yml), and the client
round-trips unknown config keys untouched, so a save from the UI preserves it.
Addresses review findings on this branch. **Fence the plugin's own routes.** `webServer.register` routes get none of the protection `dsh-client-connection` applies to the `/api` bridge, so they are reachable by the two confused-deputy paths a browser opens against a local HTTP API: DNS rebinding (Host names the attacker's domain while the socket lands here) and cross-site requests from a malicious page. This API is a write surface, and `system.notifier.command` is executed, so either path is arbitrary local command execution. Verified against a running instance before this commit: a `text/plain` POST carrying `origin: https://evil.example` and `sec-fetch-site: cross-site` was accepted with 200 and applied. The fence binds browser and non-browser clients alike — over plain HTTP a browser may send neither Origin nor Fetch metadata, so `Host` is the only always-reliable signal. Loopback is trusted by default; a deployment served on anything else lists it in `security.trustedHosts`, mirroring the naming DSH already uses for the same fence. **Drop the committed lockfile.** `pnpm-lock.yaml` was a by-product of running `pnpm install` in the working copy. Upstream has none, `.gitignore` did not list one, and nothing here depends on it — but committing it pins every consumer's dependency resolution. Removed, and ignored so it cannot return. **Honour an explicit `args: []`.** A notifier that takes no arguments is a legitimate configuration, but an empty filtered list is produced both when `args` is absent and when it is explicitly empty, so the fallback silently replaced it with the terminal-notifier template. Only a malformed value falls back now. Test stubs now carry a Host header, as real requests do.
The escape hatch shipped with `args` defaulting to
`["-title", "{{title}}", "-message", "{{body}}"]`, shaped after
terminal-notifier. That is a tool-specific convention inside a feature whose
whole point is to be tool-agnostic: a user pointing `command` at anything else
(`alerter` takes `--title`/`--message`) silently got flags their tool does not
understand. The default was also the only reason `args` needed a documented
"absent versus explicitly empty" rule.
Remove the template. `args` now defaults to `[]` and means exactly "run it with
no arguments", so `command` plus `args` is simply the argv and the plugin has no
opinion about which tool is on the other end. That also deletes a code path
rather than adding one.
The READMEs stop leading with a single tool and show three, with the tradeoffs
that actually decide between them: terminal-notifier removed `-sender` in 3.0.0
(it moved to `UserNotifications`, which reads the real signed identity and needs
its own authorisation), alerter still runs the older `NSUserNotification` so
`--sender` still impersonates an already-authorised bundle id, and a
self-compiled app bundle is the clean option if one authorisation is fine.
fengfanfan-max
force-pushed
the
feat/system-notifier-command
branch
from
September 15, 2026 02:54
15b4b15 to
bc892db
Compare
The three YAML snippets were bare `notifier:` fragments with nothing saying where the block belongs; a reader had to infer two levels of nesting. The first example now carries the full composition entry including the file it goes in, and the other two are labelled as replacements for the same block. Also drops the duplicated assertion in the fence test that the previous review pass flagged: it passed `[]` explicitly, which is the parameter default, so it repeated the assertion above it and exercised no new branch.
The fence notes said a LAN or tunnel deployment must list its authority in
`security.trustedHosts`. That is true for a proxy that passes the external Host
through, but it overstated the case for dsh-mobile: its gateway authenticates
the LAN caller and then forwards *as the upstream*, rewriting the trust context
on the way in.
headers.host = upstream.host // 127.0.0.1:3080
headers.origin = upstream.origin // http://127.0.0.1:3080
headers["sec-fetch-site"] = "same-origin"
So the plugin sees an ordinary loopback same-origin request and the fence allows
it with no configuration. Only a pass-through proxy (nginx / Caddy defaults, a
bare tunnel) needs the entry. Stated in both READMEs rather than leaving users
to discover which kind their setup is.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
动机
宿主系统通知渠道把操作系统自带的命令写死了。但这套默认在某些环境里会静默失效,插件完全无从察觉。
最典型的是 macOS。
osascript display notification的通知身份不来自osascript自己——/usr/bin/osascript是个裸二进制,没有 bundle id,macOS 取的是启动链上的宿主 App:如果那个宿主 App 从未申请过通知权限(Ghostty 的
app-notifications默认就是never),macOS 会把通知直接丢掉——而osascript仍然以0退出。于是runExecFile永远 resolve,runJobs里的告警分支永远走不到,插件回报{"ok":true,"note":"测试消息已发送"}——一次都没发出去。同类环境还有:
dsh web由 launchd 启动(没有 GUI 会话)、跑在某个未注册的终端里。这个 PR 加什么
一个逃生舱:
system.notifier。插件不对通知器做任何假设,也没有默认参数模板:
command指向什么就执行什么,args就是它的完整 argv,{{title}}/{{body}}与 Webhook 模板同一套写法(未识别的 token 原样保留)。command留空即维持原有系统默认行为(也是默认值),因此不配置时行为完全不变。文档给了三种工具的写法与取舍(terminal-notifier / alerter / 自编译 app bundle)。这一条不是可选项。
system.notifier.command会被execFile执行,而写入它的POST /dsh-plugin-notify/config在此之前没有任何保护:Origin/Host/sec-fetch-site校验readBody不要求 JSON content-type,所以跨站text/plain是一个 CORS "simple request",不触发预检Host也不校验 → DNS rebinding 同样可达已在真实实例上验证:带
origin: https://evil.example+sec-fetch-site: cross-site的text/plainPOST 被接受并应用。也就是说,任何用户访问的网页都能设置一个可执行文件路径,并在下一个轮次边界执行它。
在本 PR 之前,同样的写入只能把 webhook 指向攻击者服务器(数据外泄)。加上"可执行文件路径"就把它变成了代码执行——所以尽管这条无守卫的路由是既有的,是本 PR 让它升级为 RCE,因此本 PR 自带修复。
修复方式是在路由包装层加一道
isTrustedRequest栅栏,三条路由共用:127.0.0.1:3080http://127.0.0.1:3080127.0.0.1:3080127.0.0.1:3080https://evil.exampleevil.example:3080http://evil.example:3080127.0.0.1:3080先卡
Host(纯 HTTP 下浏览器唯一无法省略的信号),再拒sec-fetch-site: cross-site与不匹配的Origin,畸形输入一律 fail-closed。这会改变对外部署的行为:非回环主机需要加进
security.trustedHosts(命名沿用 DSH 自己对/api桥的同一套栅栏)。补充一个实测结论:转发时重写 Host / Origin 的网关不需要配置。
dsh-mobile就是这种——它在 LAN 侧认证调用者后,以上游身份转发(headers.host = upstream.host、headers.origin = upstream.origin、sec-fetch-site: same-origin),插件收到的是干净的环回同源请求,栅栏直接放行。只有原样透传外部 Host 的反向代理(nginx / Caddy 默认行为、裸隧道)才需要加trustedHosts。两个 README 都写明了这一点,但分类仍值得进 release notes。设计取舍
command: ""时插件行为与现在逐字节一致,不配置的人感知不到。-title,alerter 用--title),一个通用逃生舱不该内置其中任何一个的约定。command缺失/为空/类型不对 → 视为用系统默认;args里的非字符串项被丢弃;args: []表示"不带参数执行",是合法配置。usesCustomNotifier()抽成一个谓词,避免systemNotify与 dispatch 处对"什么算配置了"判断不一致。3表示未授权、4表示拿不到通知服务),失败会真正 reject,runJobs现有的告警日志第一次能生效。没有做设置页 UI
有意的:这是高级逃生舱,保持只走配置文件,与 README 里已有的
cordis.patch.yml路径一致。已验证设置页保存不会擦掉它:客户端保存时是
setDraft(deepClone(snap.config))+ 原地update(mutator),未知字段原样往返。测试
node --test43 → 56,全绿。原有用例除两个请求桩补上Host(真实请求本来就有)外未做改动。新增覆盖:通知器命令与参数插值、无参数运行、OS 默认回退、
normalizeConfig的默认/保留/清洗、isLoopbackHostname、isTrustedRequest的放行与拒绝矩阵、trustedHosts清洗,以及一个打真实注册路由的集成用例(跨站写入 → 403 且不落盘)。兼容性
systemNotify(title, body, execImpl, platformImpl)现有签名不变,新参数在第 5 位且默认null;DEFAULT_CONFIG、schema、normalizeConfig都是新增字段,老配置读入后自动补齐。提交
e44feb7feat: let the host channel use a custom notifier command6a67c5ffix: fence the config API, drop the stray lockfile, honour args: []bc892dbrefactor: drop the default notifier arg template, document several tools330f1b9docs: give the notifier examples their placement context