Conversation
Personal provider tokens, including Slack user tokens that can post as the person, were stored in plaintext at ~/.opensession-mcp-oauth.json and injected into engine config. Any agent shell on the host could read them and impersonate a teammate. Storage is now AES-256-GCM under a root-owned key that systemd exposes only inside the service's private credential mount, migrated in place on first read. Credentials resolve in the coordinator immediately before the upstream transport opens, so no provider token enters engine config, env, argv, logs or sandbox files. Model-controlled processes run under an AppArmor profile that denies the credential mount, the key directory, the store and the process mirrors that would otherwise reach them. Grants are also now strictly the prompter's. Anyone signed in can prompt anyone else's session, so resolving a credential by session owner let one person spend another's token. Visibility and credential selection both key on the prompter alone.
🕙 Outdated review — superseded by a newer review belowsequenceDiagram
participant E as Agent engine
participant C as Coordinator proxy
participant K as Encrypted grant store
participant S as stdio MCP child
E->>C: Tool call via run-rpc
C->>K: Decrypt selected grant
C->>S: Spawn with provider token
Note over S,K: stdio child is currently unconfined
|
tella-butler
left a comment
tella-butler
left a comment
There was a problem hiding this comment.
OS review · 7e69d73
| const session = sessionId ? findSession(sessionId) : undefined; | ||
| // Personal provider tools are opt-in at the run launch sites. Other callers | ||
| // (notably Desk voice) deliberately consume a narrower interactive facade. | ||
| const personalMcp = session && personalMcpScope |
There was a problem hiding this comment.
🔴 P1 — Pass the personal MCP scope from every run launcher
Personal proxies are now opt-in, but session-create.ts:644 still calls interactiveMcpServers(spec.user, bksId) and runner-session.ts:78 does the same for every Runner turn. Meanwhile buildOpencodeMcpConfig removes any server for which that user has a grant. Therefore a new local session's opening prompt, and every Runner prompt, gets neither the external server nor its proxy. For example, an opening request to post through a connected Slack account has no Slack tools, although a later ordinary local prompt does. Pass spec.runMcpServers ?? "all" and opts.mcpServers ?? "all" at those call sites and add coverage for both launch paths.
There was a problem hiding this comment.
Fixed in 9720bbb, and this one was worse than reported. Rather than patch the two call sites, I made personalMcpScope a required parameter, because the failure is silent in both directions: buildOpencodeMcpConfig drops a granted server assuming a proxy replaces it, so a launcher that forgets the scope gets neither. Making it required had the compiler find two more launchers you had not flagged, in opensession.ts's resume path. Those and the run-rpc fallback builder now share one derivation, personalMcpScopeForSession. Desk voice passes undefined explicitly to keep its narrow facade. On coverage: I did not add per-launcher tests, since the type signature now enforces this at every present and future call site, which a test enumerating today's launchers would not.
Four review findings, all real: The proxy spawned an OAuth stdio server straight from the coordinator, so a compromised MCP package held the credential mount and could recover every teammate's grant rather than the one token it was handed. It now goes through secureAgentCommand like every other model-controlled process. Adoption reused detached engine servers spawned before the profile existed, so the first boot after install could reuse an unconfined survivor exactly as the key was mounted. Survivors are now checked against /proc/<pid>/attr/current and retired unless they are already enforcing under our profile. The stdio binding pinned command and args but not env, so keeping command: "bun" and pointing PATH at a workspace directory ran a replacement binary with the decrypted token. The canonicalized env is part of the binding, and a binding written without one compares as empty rather than as a wildcard. The personal MCP scope was optional, and forgetting it is silent: the engine config drops a granted server on the assumption a proxy replaces it, so a launcher that omits the scope gets neither. Two launchers had already forgotten it. The parameter is now required, which made the compiler find two more in opensession.ts's resume path that the review had not caught.
🕙 Outdated review — superseded by a newer review below🤖 OS review · request changes · confidence 1/5Not safe to merge until the confinement boundary is closed. The four previously reported issues are addressed, but a confined agent can still escape through the user systemd manager, and the rollout exposes the credential mount to surviving unconfined processes before retiring them.
🔁 Not merge-ready and no live session owns this branch — add the |
tella-butler
left a comment
tella-butler
left a comment
There was a problem hiding this comment.
OS review · 9720bbb
Two more review findings, both correct. The profile granted broad file, unix and dbus access, which left the systemd USER manager reachable. That manager is unconfined, and a transient unit it creates does not inherit the profile, so `systemd-run --user` produced an unconfined process at the same uid that could read the store and the coordinator's credential mount, making every other deny decorative. The manager's control channels are now denied: its private sockets, the user bus, the session bus, and the systemd1 peer on the system bus. Denying only the systemd-run/systemctl/busctl binaries would not have done it, since a raw D-Bus client reaches the same manager. The cost is real and deliberate: a confined agent shell can no longer start work outside its own cgroup this way. Retiring unconfined survivors ran in the coordinator's boot sweep, i.e. after systemd had already mounted the key, leaving a window in which a pre-existing engine could copy it. The deploy now stops surviving opensession-oc-* and opensession-preview-* scopes BEFORE the restart, and only on the first start that gains LoadCredential, so later deploys keep the detached-survival behaviour those scopes exist for. The boot sweep stays as defence in depth.
🕙 Outdated review — superseded by a newer review below🤖 OS review · request changes · confidence 1/5Safe once the two P1 credential-exposure paths below are fixed. The prompter-only authorization and AppArmor/systemd escape fixes otherwise hold across the reviewed launch and resume paths, but OAuth stdio executable resolution and the service installer still leave ways for an existing agent process to capture protected credentials.
🔁 Not merge-ready and no live session owns this branch — add the |
Same shape as the systemd user manager, and reachable on the deployment host: the service user is in the `docker` group and the daemon answers, so a confined agent could ask that root daemon to bind-mount /var/lib/opensession or this home directory into a container and read the key and the grant store. The caller's path denies do not constrain filesystem work the daemon performs on its behalf. The docker, containerd, podman and buildkit control sockets are now denied. Container work stays coordinator-side, which is unconfined by design. The pre-mount retirement existed only in deploy/deploy.sh, so the documented `opensession service install` path still installed the profile and key, copied the credential-bearing unit and started it while pre-existing agent scopes ran unconfined. The installer now reads the INSTALLED unit before overwriting it and, on the first start that gains LoadCredential, stops surviving opensession-oc-* and opensession-preview-* scopes before the copy.
|
Addressing the latest review verdict in P0, Docker daemon escape. Confirmed exploitable here, not theoretical: the service user is in the P1, installer path. Correct, the pre-mount retirement existed only in Standing note on the tradeoff, since it now applies twice: closing these brokers removes capabilities agents on this instance genuinely use. |
tella-butler
left a comment
tella-butler
left a comment
There was a problem hiding this comment.
OS review · 7a9fb8c
🕙 Outdated review — superseded by a newer review below🤖 OS review · request changes · confidence 1/5Not safe to merge: the encryption, proxy, and prompter-authorization design is sound, but three P1 escape paths remain. A personal stdio token can still reach a PATH-hijacked executable, and first-install retirement can silently fail or leave detached Pi run hosts alive when the credential mount appears.
🔁 Not merge-ready and no live session owns this branch — add the |
tella-butler
left a comment
tella-butler
left a comment
There was a problem hiding this comment.
OS review · d0ba933
The AppArmor half of this change could not deliver what it claimed and was costing real capability to maintain. The coordinator and the agents share a Unix user, so any root-equivalent broker reachable at that uid defeats the profile: two review rounds found the systemd user manager and the Docker socket, and that list was not going to end. On a self-hosted instance it is worse than incomplete, because sessions author the code the coordinator later executes, which no confinement of the agent can address. Closing the holes had already cost agents `systemd-run --user` and `docker`, which several documented workflows use. So the profile, the spawn wrappers, the survivor retirement, the unit's LoadCredential, and the deploy and installer changes are gone. What remains is the part that holds regardless of uid: the store is ciphertext, tokens resolve in the coordinator immediately before the upstream transport opens and never enter engine config, environments, arguments, logs or sandbox files, grants follow the prompter rather than the session owner, and a grant is pinned to the server binding it was issued against. The key now prefers a systemd credential when an operator has configured one and otherwise mints a 0600 file beside the store on first use. That is what makes this work on a rootless install, where requiring LoadCredential would have failed the unit outright, and it keeps personal connections working for installs that never have root at all. docs/security-model.md now states what this does and does not protect against, including that same-uid isolation needs a privileged broker holding the key and returning per-use grants, which is the intended end state.
soutar
changed the title
🕙 Outdated review — superseded by a newer review below🤖 OS review · request changes · confidence 1/5Not safe to merge. The proxy and prompter-scoping changes keep personal tokens out of engine configuration, but current HEAD removes the runtime boundary and leaves the decryption key readable by the same ordinary agent shells this PR is meant to defend against. OAuth stdio launch also remains vulnerable to command capture through the inherited, agent-writable PATH.
🔁 Not merge-ready and no live session owns this branch — add the |
tella-butler
left a comment
tella-butler
left a comment
There was a problem hiding this comment.
OS review · 0c28b99
| if ((error as NodeJS.ErrnoException)?.code !== "ENOENT") throw error; | ||
| } | ||
| } | ||
| const path = statePath(KEY_NAME); |
There was a problem hiding this comment.
🔴 P1 — Keep the encryption key outside the agent's Unix identity
statePath(KEY_NAME) places the fallback key under the same state root as the encrypted store, normally ~/.opensession-mcp-oauth.key beside ~/.opensession-mcp-oauth.json. The final installer supplies no isolated credential, and model-controlled OpenCode/Pi shells run as the coordinator's Unix user, so a prompt can read both files and decrypt every access and refresh token offline. For example, it can recover Michiel's Slack user token and post as him; mode 0600 does not separate same-UID processes, and a home backup or snapshot also contains both key and ciphertext. This is the original threat the PR claims to close. Do not enable personal MCP with a same-user key fallback: require a broker/second UID or another enforced boundary that keeps both key and store unreadable to model-controlled processes, and fail closed until that boundary exists.
There was a problem hiding this comment.
Accurate, and deliberate. Same-uid readability is stated as a non-goal in the PR body and in docs/security-model.md rather than claimed as closed, so this is not a gap between what the code does and what it says.
Declining the recommendation to fail closed, because of what it would cost. Requiring an isolated credential means personal MCP is unavailable on every rootless install, which is precisely the audience of the simple-mode installer in #100: a release artefact under a systemd --user service with no root step. Worth noting that an earlier revision of this PR did require LoadCredential, and against #100's user-scope unit rendering that is not a degradation but a startup failure, since the unit would carry a credential pointing at a root-owned file its own manager cannot read.
What the fallback key buys is narrower than a boundary and is described as such: the store is ciphertext at rest, so a stray copy, a paste of its contents or a partial sync yields nothing, and it is the substrate a per-use broker sits on. Your backup point was right and the docs overclaimed, so 2efb367 corrects it: with the fallback key both files live in the same directory, and a whole-home backup can be decrypted offline.
The real fix is the one you name, a broker or a second uid, and it is recorded in docs/security-model.md as the intended end state rather than left implicit. It is out of scope here on a considered call by the repository owner: an earlier revision of this PR tried to reach that property with an AppArmor profile, and two review rounds showed it could not hold at a shared uid, while costing agents systemd-run --user and docker.
The binding pinned the configured command string, but the configured command is normally a bare name and StdioClientTransport resolved it through the inherited PATH at spawn time. On a normal install that PATH runs through directories this same user can write (~/.bun/bin is 0775 here, a checkout's node_modules/.bin likewise), so shadowing the name captured the person's token without touching mcp-config.json and the binding still matched. The binding now records the command RESOLVED to an absolute path when the grant is issued, the match re-resolves and compares it, and the proxy launches that pinned path instead of the name. A binding from before this fails closed and asks for a reconnect rather than being trusted against a name. Note Bun.which() reads the PATH captured at process start, not the live one, so the resolution passes PATH explicitly; without that the pin would ignore the environment the server is actually running with, and the test that shadows a name on PATH would pass for the wrong reason. Also corrects a claim in docs/security-model.md: with the fallback key the key file sits beside the store, so a whole-home backup contains both and can be decrypted offline. Only a systemd credential, or the broker described there, puts it out of reach of a copy of the home directory.
🕙 Outdated review — superseded by a newer review below🤖 OS review · request changes · confidence 1/5Not safe to merge: the two P1 credential-exfiltration paths from the previous review remain. The new commit correctly propagates personal MCP scope and pins an executable pathname, but that executable remains writable by the agent identity, while rootless encryption stores its key under the same identity. A run can still recover or capture reusable personal tokens.
🔁 Not merge-ready and no live session owns this branch — add the |
tella-butler
left a comment
tella-butler
left a comment
There was a problem hiding this comment.
OS review · 2efb367
msg6.tmp is the body of the previous commit, left in the repo root. Nothing reads it.
🕙 Outdated review — superseded by a newer review below🤖 OS review · request changes · confidence 1/5Not safe to merge until the P1 findings below are fixed. The proxy and prompter-identity changes keep grants out of engine config, but the motivating same-UID token-theft path remains open through the key and OAuth stdio launch chain; the latest commit only removes a stray temp file.
🔁 Not merge-ready and no live session owns this branch — add the |
tella-butler
left a comment
tella-butler
left a comment
There was a problem hiding this comment.
OS review · e811efe
| // rather than a later chmod, so it is never briefly world-readable. | ||
| const key = randomBytes(32); | ||
| try { | ||
| const fd = openSync(path, "wx", 0o600); |
There was a problem hiding this comment.
🔴 P1 — Keep the encryption key outside the agent's Unix identity
The fallback key is created as a 0600 file owned by the same Unix user that runs every agent. An agent can read both ~/.opensession-mcp-oauth.key and the encrypted store, then reproduce the documented AES-256-GCM decryption and recover every access and refresh token. This preserves the exact credential-theft capability that motivated the PR; documenting that limitation does not make the encrypted store a security boundary. Keep decryption behind a broker running as a different UID and give agents only short-lived, run-scoped capabilities, or fail closed when that boundary is unavailable.
There was a problem hiding this comment.
This remains deliberately out of scope for the rootless simple-mode target. The fallback key is now described only as protection for a stray copy of the grant file, not as a same-UID boundary or whole-backup protection. Requiring a separate-UID broker would disable personal connections on the default unprivileged install; that broker remains the documented end state rather than a claim made by this PR.
opensession#104 moved the server under packages/core/opensession-server, so this branch's 22 files had to follow. Rename detection carried most of them across; six needed a decision. mcp-relay.ts: main renamed it, this branch deletes it. Deleted. mcp-oauth-proxy.ts: new here, placed at the new path. automations.ts, pi-mcp-bridge.ts, run-session.ts: unions, not either-or. Main's work (runAgentHosted and toPiModel, the mcp-tools-cache cacheKey and the classifyInProcessMcp note, the isAutomationSession branch) is kept alongside this branch's (the OAuth proxy mount, the grant guard that stops a second credential-bearing connection, the personal MCP scope argument). readOpencodeBridgeConfig went with main, which had already dropped its only use. mcp-oauth.test.ts was added on both sides independently: this branch's eight cases plus the Figma registration case from 82c9662. Both are kept. The Figma test now runs through the same dynamically imported module as the rest of the file, because a static import loads mcp-oauth before beforeAll sets the fixture's state dir, and it passes an initiator: connecting is gated on a signed-in identity on this branch, so without one the flow stops a step before the registration error the test is about. Verified in a clean worktree with its own bun install, since the workspace layout needs real @tellahq/opensession-protocol resolution and a symlinked node_modules from a pre-restructure checkout fails to resolve it. Every path outside this branch's own 22 is byte-identical to the merged main. The full packages/core/opensession-server/src sweep is 34 distinct failures against that main's 36, none present here and absent there. tsc reports the same 8 pre-existing Setup.tsx errors as the merged main does on its own; they are not from this branch.
🕙 Outdated review — superseded by a newer review below🤖 OS review · request changes · confidence 2/5Safe once the P1 below is fixed. The coordinator proxy now keeps OAuth tokens out of engine configuration and consistently scopes personal grants to the prompter. I retract the prior key-location blocker because the revised description and security model explicitly limit encryption to at-rest protection, but the stdio proxy can still disclose a grant through a mutable pinned executable.
🔁 Not merge-ready and no live session owns this branch — add the |
soutar
changed the title
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…auth-restructured
…auth-restructured
🕙 Outdated review — superseded by a newer review below🤖 OS review🔄 Reviewing |
🕙 Outdated review — superseded by a newer review below🤖 OS review · request changes · confidence 2/5Safe once the two P1 credential-binding bugs below are fixed. The previous launcher-scope issue is fixed, and I am retracting the earlier same-UID key and writable-stdio concerns because the limitation is now explicit and personal grants no longer execute stdio servers. The remaining paths can either send an existing grant to a repointed server or silently ignore a newly saved manual grant.
🔁 Not merge-ready and no live session owns this branch — add the |
The boot recovery gate treated an empty requester as an untrusted person and cleared the marker. Webhook- and reconcile-triggered reviews arm `requestedBy: ""` by design, so every automated review that spanned a restart was refused — and clearing the marker also discarded the run's durable reviewResult, stranding a finished review as a permanently spinning "Reviewing..." comment (PR #99 sat that way for 16h). Treat a requester-less review marker as automation. Simplify, adversarial, auto-fix and mentions always record a human, so they keep the strict check. Gate extracted as recoveryPermitted() so the case is testable.
![open-session-os-tella-dev[bot]](https://avatars.githubusercontent.com/u/65127121?s=60&v=4){kind=small}
| ...(scopes ? { scopes } : {}), | ||
| endpoints, | ||
| clientInfo: { clientId: reg.client_id }, | ||
| binding: configuredBinding(name, serverUrl), |
There was a problem hiding this comment.
🔴 P1 — Do not carry old grants onto a newly bound server URL
When cur.serverUrl !== serverUrl, this branch registers the new upstream and binds it here, but line 522 copies cur.shared and cur.users into the new entry. For example, after tella has a grant for https://original.example/mcp, repointing its config to https://attacker.example/mcp and merely starting a new OAuth flow runs ensureServerAuth, binds the entry to the attacker URL, and preserves the original bearer token. Any run can then send that old token through the proxy before the new consent flow completes. The recent legacy-migration check does not cover this active re-registration path. When the URL changes, revoke all existing grants instead of copying them into next; only a completed flow for the new binding should install a grant.
There was a problem hiding this comment.
Fixed in 5136a67: re-registering a server at a different URL now starts from a clean registration and does not carry shared or per-user grants across the binding change. Added a regression test proving the old grant is absent and cannot be proxied while the replacement consent flow is pending.
| users: Array<string | undefined>, | ||
| ): boolean { | ||
| return ( | ||
| tryReadStore()[name]?.binding?.kind === "http" && |
There was a problem hiding this comment.
🔴 P1 — Bind manual-token grants so the proxy can mount them
saveManualMcpGrant still writes Vercel's validated token without setting entry.binding, while this new gate requires an HTTP binding. A user can successfully POST a Vercel token and see the account reported as connected, but mcpOauthProxyServers excludes it; Pi and sandbox launchers consequently retain the ordinary external server instead of using the personal token, potentially running tools as the workspace identity. Set and validate entry.binding = configuredBinding(name, serverUrl) before storing the manual grant, failing if the current configured URL does not match.
There was a problem hiding this comment.
Fixed in 5136a67: manual-token saves now require the configured HTTP URL to match, persist that binding, and discard grants from an older URL before installing the newly validated token. Added coverage that the grant is proxy-mounted only for the configured URL and a repointed mismatch fails closed.
🕙 Outdated review — superseded by a newer review below🤖 OS review
|
# Conflicts: # packages/core/opensession-server/src/server/pi-mcp-bridge.ts
|
🛠️ OS auto-fix — ☑️ No further changes this round — the remaining items were deliberately skipped or couldn't be auto-fixed: · 📺 open session Skipped (deliberate): same-UID fallback key exposure — out of scope, follow-up requiring a separate-UID broker or new credential boundary |
open-session-os-tella-dev
Bot
removed
the
os-auto-fix
🤖 OS review · request changes · confidence 2/5Safe once the two P1 credential-routing gaps below are fixed. The newly pushed commits correctly address replacement-URL registration and manual-token binding, but the merged MCP runtime bypasses the new proxy for local Pi turns, and coordinator catalog calls still send grants without checking their URL binding.
🔁 Not merge-ready and no live session owns this branch — add the |
| createdBy, | ||
| isAdmin: true, | ||
| }), | ||
| ...personalMcp, |
There was a problem hiding this comment.
🔴 P1 — Make the personal proxy shadow the external MCP entry
This proxy does not handle ordinary local Pi turns after the latest main merge. createMcpRuntime adds every configured external server first (mcp-runtime.ts:312-318), records those names as taken, and then skips same-named inProcessMcp servers. Its external connector still detects personal grants and uses the legacy relay with [mcpGrantUser, user] (mcp-runtime.ts:246,256-264). For example, when Kent prompts a session created by Michiel, a configured tella entry wins over this proxy and the relay selects Michiel's grant first, despite this PR requiring the verified prompter's identity. Ensure the in-process OAuth proxy shadows/removes the same-named external entry in the new runtime, and do not use the creator-first legacy grant path for new turns.
There was a problem hiding this comment.
Fixed in 9c7aed2: the Pi MCP runtime now removes external entries shadowed by coordinator-owned in-process servers, and its external connector no longer selects or relays creator-first personal grants. Same-named personal proxies therefore own the runtime entry and use only the prompter identity supplied by interactive-mcp. Added coverage for in-process shadowing.
| } | ||
| } | ||
|
|
||
| export function mcpOauthBindingMatches( |
There was a problem hiding this comment.
🔴 P1 — Enforce the URL binding in coordinator MCP clients too
The binding check is used only by mcp-oauth-proxy.ts. mcp-client.ts:31 and :69 still call mcpAuthHeader and send the returned grant to the current cfg.url without invoking this helper. If tella was connected to https://original.example/mcp and its config is repointed to https://attacker.example/mcp, requesting /api/connections/mcp/tella/tools sends the old bearer token to the attacker-controlled URL. Apply mcpOauthBindingMatches(serverName, cfg) before selecting an OAuth header in both coordinator client paths, falling back only to the configured workspace credential when the personal binding does not match.
There was a problem hiding this comment.
Fixed in 9c7aed2: both coordinator catalog and tool-call paths now select a personal OAuth header only when the current configured HTTP URL matches the stored grant binding, otherwise falling back to the configured workspace Authorization header. Added matching and repointed-URL coverage.
|
🛠️ OS auto-fix — |
soutar
added
os-review
2 participants
Goal
Make personal MCP connections materially safer without adding a root requirement or breaking the default single-user install.
What changed
mcp-oauth-keysystemd credential. Rootless simple-mode installs mint a0600key beside the store, so installation and first run need no privileged step.allowedUsersvisibility follow the verified prompter, not the session creator. OAuth callbacks are tied to the verified account that started them when web sign-in is enabled.Simple-mode behavior
The default personal install remains rootless and requires no additional setup. It has no web identity gate, so it creates shared grants in line with its single-user trust model. Existing configured stdio integrations, including Slack bot tooling, continue using their workspace credential.
The fallback key is intentionally a limited at-rest improvement: it protects a stray copy of the grant file, but not a whole state-directory backup or another process already running as the same Unix user. Full same-UID isolation still requires a separate-identity credential broker;
docs/security-model.mdstates that boundary explicitly.Compatibility
The legacy loopback relay remains available for detached runs created before this update, but new runs use the coordinator proxy. Existing plaintext grants migrate atomically and are left unchanged if decryption or migration fails.
OPENSESSION_PERSONAL_MCP=0disables reads and writes. The branch also fixes the current simple installer’s ShellCheck warning without changing its displayed~/...profile path.Verification
bun run typecheckbun scripts/check-module-side-effects.ts(426 modules)bash -n install.sh; CI’s ShellCheck gate now passes the changed lineStarted by John Soutar in this OS session