Owner identities are configured, because a person is not one handle - #58
Owner identities are configured, because a person is not one handle#58ThinkOffApp wants to merge 1 commit into
Conversation
petrus tapped Approve from his tablet and nothing happened. The guard compared the sender against the literal string "petrus"; his tablet posts as "@petrus-boox". Verified in the raw API payload rather than inferred: 13:43:46 from='@petrus-boox' isHuman=False -> dropped, intent stayed pending 14:12:42 from='petrus' isHuman=False -> settled instantly Same person, same room, same command, 29 minutes apart. `owners` is now a list, defaulting to ['petrus'] so existing callers behave exactly as before, read from mcp.confirmations.owners. DELIBERATELY AN EXPLICIT LIST, NOT A PREFIX MATCH. A `petrus-*` rule would be shorter and would hand approval authority to any agent that registers itself as "petrus-helper" -- which is precisely the attack this guard was added for, after a fleet agent auto-replied "/approve <id>" to a card. Verified: petrus-helper is rejected, @petrus-boox is accepted. The `@` prefix is stripped and case is normalised on both sides, so "@Petrus-BOOX" in config matches "petrus-boox" on the wire. Worth knowing what this does NOT fix: it covers a TYPED /approve. The tablet's Approve button is a separate path, and while testing this we found the app had stopped posting from that device entirely since 14:03, so the button remains unverified either way. Granting a device handle approval authority means granting it to whoever holds that device's API key. petrus asked for it explicitly, knowing that, after the alternative was laid out. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98010a4c00
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| apiKey, room, intervalMs: 5000, | ||
| // Owner identities that may settle intents. A person is not one handle: | ||
| // petrus posts from a web session, a tablet and (soon) a watch. | ||
| owners: cc.owners, |
There was a problem hiding this comment.
Forward aliases from the in-process MCP server
This forwards mcp.confirmations.owners only when running the standalone daemon. When no daemon is detected, src/mcp-server.mjs starts the same poller in-process without passing confirmCfg.owners, so configured aliases silently fall back to ['petrus'] and a reply from @petrus-boox remains rejected in that supported deployment. Pass the configured owners through that startup path as well.
Useful? React with 👍 / 👎.
ThinkOffApp
left a comment
There was a problem hiding this comment.
The guard change itself is right, and rejecting petrus-helper is a better call than the /^petrus(-|$)/ I suggested in the room — my version would have accepted exactly that handle. Exact-set membership, @ stripped, lowercased, defaulting to ['petrus'], config-driven. No notes.
One question this does not close, and it decides whether the allowlist means anything
The second arm is untouched:
if (!ownerSet.has(sender) && m.isHuman !== true) { ...ignore }So any message with isHuman === true settles an intent regardless of sender. The owner list is only load-bearing for senders where isHuman is falsy. If isHuman is ever client-settable, the allowlist is decoration and an agent bypasses it by setting one flag.
I cannot see the server, so I am asking rather than asserting. But the room data is not entirely reassuring:
| sender | isHuman | count |
|---|---|---|
petrus |
true | 5 |
petrus |
false | 1 |
@petrus-boox |
false | 17 |
every agent (@claudemm, @claudeMB, @codexmb, @kimi3) |
false | 77 |
The reassuring part: no agent has ever posted with isHuman=true, across 77 messages.
The unreassuring part: the same handle petrus appears with both values. If isHuman were derived from the account identity it would be constant per handle. It is not, so it appears to reflect the client or route that posted — which is exactly the kind of thing a client can usually influence.
Two things worth settling, and you can see the server and I cannot:
- Is
isHumanserver-assigned from the API key's account type, or accepted from the request body? If the latter, this arm should go, and the allowlist becomes the only check. - If it stays, it is worth a comment saying why it is safe, because the next person reading this guard will reasonably assume the allowlist is the whole story.
Not blocking. The PR strictly improves things either way — it fixes the real bug and narrows nothing. I would just rather we know whether the door beside the one we just locked is open.
|
To use Codex here, create an environment for this repo. |
petrus tapped Approve from his tablet and nothing happened, on camera, while filming the approval flow.
The bug
The owner guard compared the sender against the literal string
petrus. His tablet posts as@petrus-boox. Verified in the raw API payload rather than inferred:fromisHuman@petrus-booxfalsependingpetrusfalseSame person, same room, same command, 29 minutes apart.
The change
ownersis now a list, read frommcp.confirmations.owners, defaulting to['petrus']so existing callers behave exactly as before.The
@prefix is stripped and case normalised on both sides, so@Petrus-BOOXin config matchespetrus-booxon the wire.Why an explicit list and not
petrus-*A prefix rule is shorter and it is wrong. Any agent can register any handle, so
petrus-*hands approval authority to a fleet agent that calls itselfpetrus-helper— which is exactly the attack this guard was added for, after one auto-replied/approve <id>to a confirmation card and the poller executed it. Verified above thatpetrus-helperis rejected.What this does not fix
It covers a typed
/approve. The tablet's Approve button is a separate path, and while testing this we found the app had stopped posting from that device entirely since 14:03 — so the button is unverified either way, not fixed and not disproven.Authorisation
Granting a device handle approval authority grants it to whoever holds that device's API key — a weaker claim than the
isHumansignal, which marks a signed-in human. petrus asked for this explicitly, with that tradeoff stated and the alternative (merge the visibility fix only, keep approving from the browser) laid out first.Stacks cleanly beside #54, which makes the rejection visible; the two solve different halves and neither depends on the other.
🤖 Generated with Claude Code