Skip to content

Owner identities are configured, because a person is not one handle - #58

Open
ThinkOffApp wants to merge 1 commit into
mainfrom
feat/configurable-owner-identities
Open

Owner identities are configured, because a person is not one handle#58
ThinkOffApp wants to merge 1 commit into
mainfrom
feat/configurable-owner-identities

Conversation

@ThinkOffApp

Copy link
Copy Markdown
Owner

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:

time from isHuman result
13:43:46 @petrus-boox false dropped, intent stayed pending
14:12:42 petrus false settled instantly

Same person, same room, same command, 29 minutes apart.

The change

owners is now a list, read from mcp.confirmations.owners, defaulting to ['petrus'] so existing callers behave exactly as before.

owners unset                      → petrus ✓  @petrus-boox ✗  hermes ✗  petrus-helper ✗
owners ['petrus']                 → petrus ✓  @petrus-boox ✗  hermes ✗  petrus-helper ✗
owners ['petrus','petrus-boox']   → petrus ✓  @petrus-boox ✓  hermes ✗  petrus-helper ✗

The @ prefix is stripped and case normalised on both sides, so @Petrus-BOOX in config matches petrus-boox on 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 itself petrus-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 that petrus-helper is 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 isHuman signal, 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

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>
@cursor

cursor Bot commented Aug 3, 2026

Copy link
Copy Markdown

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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 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".

Comment thread bin/iak-mcp-daemon.mjs
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,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge 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 ThinkOffApp left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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:

  1. Is isHuman server-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.
  2. 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.

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create an environment for this repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants