Borrow upstream security and correctness hardening - #24
Merged
Conversation
Adapt OpenBot PRs #48, #51, #52, #54, and #58 for Kayco's managed agents, per-Bot computers, credential store, prompts, and Knowledge configuration.
`DELETE /api/sandboxed/:name` handed the raw name to `store.remove`, which deleted from the shared `components` table with nothing checking which kind of component the name belonged to. `save` refuses a name that is not a slug and `publish` refuses a name with no draft behind it; `remove` did neither, and that asymmetry was the bug rather than the delete itself. Naming a compiled component removed its governance row, and the foreign keys took its per-Bot withholdings and its function grants with it. The withholdings are the half that fails open: a published component is available to every Bot unless a `component_exclusions` row says otherwise, so losing that row releases the component rather than hiding it, and the next catalogue announcement writes it back with `published: true`. A deliberate "not this Bot" returns as "every Bot", under an audit row saying `kind: "sandboxed"`. Gated on the governance row's `kind` rather than on the presence of a `sandboxed_components` row, because ownership is the actual question and the two differ in one case worth keeping. The two deletes are not in a transaction, so a failure between them leaves a governance row with no source; that orphan is this surface's to clear, and requiring the source row would have made it undeletable here. The route answers 404 for a name it does not own, the way publish does. Admin-only either way, so this is a footgun rather than an escalation. Co-authored-by: David McKay <davidmckayv@users.noreply.github.com>
A click citing a ref the server cannot resolve was carried out, and every deny rule keyed on the element was silently unable to match it. `resolve` answering undefined binds an all-empty element into the policy context. Empty is the right neutral value for an action that names no element, which is what stops a rule about one action surface throwing on another, and it is the wrong answer for an action that named a ref and did not get one. `contains(element.name, "submit")` against empty strings is false, the shipped default allows, and the click lands on whatever that ref points at now. The rule did not decline to match. It was never shown the element. The computer's own staleness check does not make this safe. It compares the citation against its own counter, so it catches the cases where the two disagree. The case that matters is the one where the computer is content and only this server is out of step, which is what a computer restarting its generation counter under a stored row leaves behind: the store keeps the higher generation and refuses every snapshot until the counter climbs back past it, while the model is handed those refs anyway. Driven against a real computer and a real browser: the same click on the same button, refused by the rule before the restart and performed after it. Narrow on purpose, in three ways, each of which turns a test red if it goes. Only a cited ref, so a scroll, a page-level keypress, a shell call and a file read are untouched. Only against a snapshot this server holds, because with no stored page there is nothing here to judge the citation against, the computer is the only party that can answer, and refusing locally would take its answer away, including the one that says a person has the wheel. And raised after the decision row is written, because an action whose ref resolves to nothing is still an action somebody tried to take. A comment in the tests claimed the empty element was "the one a deny rule treats as a match". That was the misconception underneath this, and it is corrected where it sat.
The computer router's bot-access guard steps aside for the paths that are about the deployment rather than about a Bot. It has to: Hono matches `/*` against zero segments, so `/policy` and `/fleet` arrive at `/:botId/*` as Bot ids, and nothing in the request tells them apart from a Bot that really is called that. A Bot with one of those ids is therefore served to anybody who can sign in, and the guard is not bypassed so much as never asked. Closed at the three places it is reachable. A package id is the only way a Bot gets a chosen id, since everything created through the API is `agent_<uuid>`, so the YAML is refused first. That is also the answer an operator can act on, and it sits with this file's other cross-file checks rather than adding a lookup to a per-request guard. Refusing the file does not remove a Bot that already has the name. Nothing here deletes a canonical agent when a package stops declaring it, so a deployment that once shipped `policy` keeps the row after the operator corrects the YAML. So the table is checked too, inside the synchronise transaction, and a deployment holding one refuses to start rather than serving it. And the guard itself now steps aside only for the deployment path, not for everything beneath it. Both routes are a single segment, so `/policy/status` is `/:botId/status` and nothing else; skipping the subtree was the actual hole, and it stays closed whether or not such a Bot can be created. The list moves into its own module because both ends now have to agree on it: adding a third deployment route should reserve the name at the same moment it starts being answered.
AGENT_COMPUTER_ALLOW_PRIVATE_HOSTS was read as a plain equality with no
environment gate, and .env.example shipped it live, so a deployment inherited it
by copying the file. That is the ordinary path rather than an unusual one.
It lifts more than browsing: app.ts hands the same value to createAgentRoutes,
and target.ts:257 is an early return { allowed: true } rather than a relaxation
of one rule, so with it on a signed-in person can register a Bot at a link-local
address.
Now mirrors the KEY_ENCRYPTION_KEY placeholder at config.ts:228: throws under
NODE_ENV=production naming the variable and what to do, warns everywhere else
and still allows it, so the local workflow is unchanged. The example line is
commented out, since copying the file is how a deployment ends up with it.
Dockerfile set the switch and NODE_ENV=production, so the all-in-one image would
have refused to start. The comment justified it as the server reaching its own
browser, but that path is checkComputerAddress, which never consults this switch
and decides on the protocol and the metadata floor alone. The switch only
governed where a Bot may browse, which on a default bridge network is the host's
LAN. Removed, and the comment now says what it actually controls.
The cloud metadata floor is unchanged and still refuses 169.254.169.254 and its
spellings with the switch on. The flag-absent default is still false, now with a
production test guarding it.
cel-js throws on an unbound identifier, and a thrown rule fails closed, so a policy expression that names a field the current action does not carry does not evaluate to false — it refuses the action. Each acting surface already builds its context with the other surfaces' fields at a neutral, matches-nothing value for exactly this reason: the MCP context carries an empty `element`, `key` and `file` so a browser rule does not catch a tool call, and the browser context carries them so a tool rule does not catch a click. Two fields were missed, one on each side: - the browser context (gateway) had no `mcp`, so `deny: mcp.effect == "write"` threw on every click, keypress, navigation and file action and refused all of them; - the MCP context had no `command`, so `deny: contains(command, "rm -rf")` threw on every MCP call and refused all of them. Either way an operator forbidding one surface silently disabled another — the one thing this policy exists to let a deployment reason about. Measured before the change: a `computer_click` under `deny: mcp.effect == "write"` is refused, and an MCP read under `deny: contains(command, "rm -rf")` is refused. Both are permitted after it, while a real MCP write and a real shell command are still refused. Fill the two neutral fields in, matching the ones already there. `mcp.effect` gains `""` as its neutral so that neither `== "read"` nor `== "write"` matches a non-MCP action, the same way the empty strings match no browser rule. The engine still fails closed on a genuinely unbound field, the deliberate floor the unit tests carry; this only completes what each surface hands it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The supervisor's port was published with no interface in front of it, so it bound every address the machine had. That is the wrong default anywhere and worst here: this is the service that mounts /var/run/docker.sock, so reaching it is root on the host by way of four verbs, and SUPERVISOR_TOKEN is a shared secret rather than a network boundary. The computer already says exactly this about its own port, a few lines up, and binds 127.0.0.1 for it. So do the Bots. The supervisor and PostgreSQL were the two that did not, and the supervisor is the one holding the socket. Only the published mapping changes. The process still listens on every interface inside its own container, which is what a deployment running the server inside the network depends on: it sets COMPUTER_NETWORK and reaches this as supervisor:4300, never through the host mapping. Binding the process itself to loopback would have broken both paths, because Docker forwards to the container's interface address rather than its loopback. Verified against docker 29.1.3 before and after: published without a prefix answers on loopback and on the host's routable address; with the prefix it answers on loopback and refuses the routable one; a container on the same network still reaches it by service name. The test pins every service that holds a secret, not just this one, and names them rather than applying a blanket rule, so adding a service stays a decision about where it should answer. It fails on the unprefixed port for each of the four independently.
… the computer The port change on its own leaves four places describing the old behaviour, and one of them was already describing the behaviour we wanted rather than the one the compose file had. docs/architecture.md said "do not expose it outside the deployment network" and stopped there, which is the instruction the published port was breaking. It now says where Compose binds it, the way the paragraph above it already does for the computer, and names the in-network path so a deployment that needs no published port knows it has one. The README's loopback bullet named computers only. The supervisor belongs in it for a stronger reason than the computer does. supervisor/src/index.ts argued that the vocabulary is the boundary and the token is not, which is still true and was the whole argument. It now also says what sits in front of both, and that the listener stays on every interface inside its own container, because that is the part someone will otherwise "fix" and break the in-network deployment. The changelog entry is written for the person upgrading: what stops working is reaching the supervisor from another machine, which was the thing worth stopping, and it says what to do instead.
…r limits use #96 found that an unset variable declared in a compose file arrives as an empty string, so `Number.parseInt(process.env.X ?? "default")` never falls back: `??` sees "" rather than undefined, and the parse is NaN. It moved the browser limits onto `numberFromEnv`, which treats empty, absent, non-numeric and non-positive alike as "not set". The three values alongside them in `agent-computer/src/index.ts` kept the raw parse: `PORT`, `NAVIGATION_TIMEOUT_MS` and `ACTION_TIMEOUT_MS`. `ACTION_TIMEOUT_MS` is a documented variable (`.env.example:173`), so a deployment that sets it the way the compose file already sets the browser limits gets a NaN timeout, Playwright waiting on NaN instead of the 10s the default promises, a computer that looks broken rather than misconfigured. Export `numberFromEnv` and read the three through it. Adds a test for the helper, including the empty-string case that is the whole point. This is in agent-computer, which the root typecheck does not reach (that gap is #112). Verified in the package: `bunx tsc --noEmit` clean, and `bun test tests/number-from-env.test.ts` is 5/5. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Closes #72. A thread id is minted before the thread exists — the platform creates it on the first run — so reading history on a new conversation asks about a thread nothing has heard of. The platform answers 404 with `THREAD_NOT_FOUND`, `handleGetThreadMessages` catches everything and returns `500 Failed to fetch thread messages`, and every new chat left one in the log with a stack trace. The browser only reads a history it got a 200 for, so nothing looked broken; what was missing was any way to tell a thread that does not exist yet from a history store that is down. `PlatformRequestError` carries `.status` for exactly this, and its own documentation gives `error.status === 404` as the example. The runtime calls `runtime.intelligence.getThreadMessages` and returns `Response.json` of whatever comes back, and that object is the one this file constructs — so the answer is corrected where it is produced, in one call, rather than guessed at afterwards. Matched on the shape, not with `instanceof`: the class is not re-exported from `@copilotkit/runtime/v2` and the package's `exports` map offers no subpath that reaches it. Both the name and the status are checked, so an unrelated error carrying a 404 does not qualify. A subclass rather than a wrapper, because the base class keeps its state in `#private` fields and a `Proxy` cannot forward them — a method invoked with the proxy as `this` cannot reach them at all. 404 only. A 500 stays a 500: an outage answered with an empty history would tell the browser the conversation is gone and invite somebody to start it over, which is the failure worth avoiding while removing the noisy one. The decision is a pair of exported functions so the tests exercise the shipped code rather than a copy of it. Ten tests: 404 recognised, 500 and 403 not, an unrelated error carrying a 404 not, a plain object not, and the read path returning history, reading empty on a 404, and rethrowing on a 500 and on a transport failure.
Closes #106. A grant names `serverId/toolName`, and `listForAgent` reads it against the tool list, so a grant on a tool the vendor has stopped advertising reaches no model. That is a property of what the vendor advertises today rather than of the grant. `google-drive` is one line from proving it: the entry's own comment says the REST transport can be swapped back to MCP and that "tool names match Google's MCP server exactly, so grants survive the swap in either direction", so a name that resolves to nothing today resolves again when it does. Nothing said it was happening. `listServers` asked for grants on the refs of the tools it had just listed, which can only ever return a subset of those — so a grant on a withdrawn tool appeared nowhere, on the one screen an administrator reads to answer what a Bot may do. Reported, not pruned, which is the decision the issue asked to be made deliberately. `refreshTools` is the only place a prune could go and it is the wrong place: the tool list is replaced by a `delete` and then an `insert`, so a failure between them already empties a server, and a vendor answering with an empty list is not a failure at all — every tool row goes, `lastError` is set to null and the refresh is stamped as healthy. Pruning there means one bad answer from a vendor silently revokes every grant on that connector and the trail says the refresh went fine. That turns a visible, inert discrepancy into an invisible, destructive one. So: `listServers` reports them, the connector's page draws them under "Held but not offered", and a refresh that leaves any behind writes a `configuration.changed` row naming the refs and the Bots. The row is the part that answers the transport swap rather than only displaying it — the discrepancy enters the trail when it arises, instead of the only record of the gap being its absence. An explicit "drop these" action for an administrator is now cheap and is left undone, because revoking should stay something somebody decided. Grants are untouched, so the run-time behaviour is unchanged. There is a test for that specifically: reporting a grant must not become honouring one.
* Close the statement that emptied the audit trail in silence The trail is append-only, enforced in the database because the application is not the only thing that can reach the table. That enforcement was a row-level trigger, and a row-level trigger cannot fire on TRUNCATE. So UPDATE and DELETE were refused while `TRUNCATE audit_events` removed every row and returned success. The guarantee that "we deleted the rows about the incident" stays impossible had a one-statement way around it. Retention is untouched, because retention deletes: it names a window and removes what is older, and the rows about last night survive by being newer than it. TRUNCATE cannot name a window. It takes the whole table, recent rows included, which is the thing the trail exists to prevent, so it is refused in every session whatever any setting says. The operation is answered before anything else is read. A statement-level trigger has no OLD record, so a guard that reaches `OLD.created_at` is comparing against NULL and falls through to the return that lets the statement proceed. That path is open exactly while a retention sweep has set the window, which is the state a happy-path check does not look at. Both states are covered by tests against a real database. Each one runs its truncate in a transaction that always throws, so a regression here cannot destroy the trail of whatever database the suite is pointed at: the run that finds the bug must not also be the run that demonstrates it. Reported in #138, including the failure mode of the obvious fix. * Format the migration metadata the way the repo formats it
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9dc07a3597
ℹ️ 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".
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
6 participants
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.
What this borrows
This selectively ports upstream security, correctness, and operator-facing fixes without merging upstream wholesale or changing Kayco's documented product boundaries.
/policydeployment route from Bot IDsTRUNCATEwith migration0011The upstream “New chat” race patch was inspected but does not apply to Kayco's channel hook/API. The larger credential transaction redesign, deletion work, and computer video work remain separate. Computer/video files were not touched.
Kayco adaptations
/policyis reserved because Kayco does not ship upstream's/fleetrouteValidation
bun run typecheck— passedbunx biome lint .— passed (existing warnings only)The local formatting command reports the repository's existing CRLF blobs as formatter differences; CI runs the canonical Linux check.