Skip to content

Commit a46b5f9

Browse files
beardthelionguidovizosorenovate[bot]zopeVaibhavdavidmckayv
authored
Carry the per-Bot egress proxy as far as the process that reads it (#250)
* Carry the per-Bot egress proxy as far as the process that reads it `EGRESS_PROXY_DEFAULT` and `EGRESS_PROXY_<BOT>` are documented in .env.example and docs/configuration.md, and neither reached any process. docker-compose.yml named no EGRESS variable and had no `env_file`, and Compose hands a container only what those two blocks name. So the shared computer resolved every Bot to null and went out directly, and in the supervisor arrangement the supervisor's own environment held none either, leaving its EGRESS_PROXY passthrough with nothing to forward into the computers it creates. Nothing said so. The operator sets a proxy, the stack starts, the browser leaves by the host, and the Computers screen reports "Leaves directly" because it is reading the same empty environment. For a setting whose stated purpose is to give a security team a per-Bot address for network rules, silently doing nothing is the worst of the available failures. A file rather than more `environment:` entries because `EGRESS_PROXY_<BOT>` is derived from a Bot's id, so there is no fixed set of names to write out here. A file of its own rather than .env because that one holds the deployment's secrets, and the container driving a browser and running a Bot's shell is deliberately given what it needs and not the rest. It is optional, since going out directly is the ordinary case and must still start, and gitignored, because a proxy URL can carry a password. The all-in-one image was never affected: its s6 service runs under `with-contenv` and inherits the container's environment, which is the mechanism this restores for Compose. * Show a dot on a channel a Bot has spoken in unseen (#259) * Give a membership a memory of when its channel was last read * Stamp the caller's membership read, and say so in the roster * Let a member say they have read a channel * Carry the read marker to the app and let it be stamped * Draw a dot on a channel a Bot has spoken in unseen * Mark a channel read the moment it is the one on screen * Narrow the admin and settings rails to the width their labels earn * Keep a fast clock elsewhere from turning mark-read into a storm * Say in the changelog what the unread dot is and is not * Stamp a read against the message clock, not just this one * Assert the clamped stamp without reaching through an optional chain * Regenerate the read-marker migration behind the queue and the frames * Refuse to mark a deleted channel read, matching the pin * Update azure/setup-helm action to v5 (#260) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Say what a strict content-security-policy has to allow (#225) * Point the test at the database the project actually has (#234) * Let the API reach Intelligence and sign-in when a NetworkPolicy is on (#257) * Refuse a credential written into the rest of the address (#230) Refusing a credential in the userinfo closed one spelling and left the two beside it open. A token in the query string or the fragment was accepted, and addCustomServer writes the address it was given into mcp_servers.url and into the configuration.changed audit payload verbatim. Redaction keys on the field name and url is not a sensitive one, so the secret landed in an append-only row in clear text, which is the disclosure the userinfo rule exists to prevent. The name is read rather than matched against a list. An exact-name version of this rule refused token and accepted auth_token, api_token, x-api-key and X-Amz-Signature, and an operator has no way to know which spellings the check happens to hold. Reading the name over-refuses in one direction on purpose: a misread parameter costs a rename, a missed one cannot be deleted afterwards. The fragment is split at the first question mark first, because a hash route or an OAuth-style callback puts a path in front of the parameters and reading the whole fragment as one query string turns all of it into a single name that matches nothing. metadata.goog is refused too, by asking the list browsing already uses rather than keeping a second copy here. It is Google's own short alias for the metadata server and it carries a dot and none of the suffixes this check lists, so it read as an ordinary vendor name, while the long spelling was refused only incidentally by the .internal test. * Spend an MCP token only for its own server, and only at its own address (#238) * Point a curated MCP server only at a credential of its own kind Adding a server by URL checks which credential it is being pointed at. Adding one from the catalogue took the same field from the same request and stored it unread, so a credential of any kind could be attached to a curated server and spent by the refresh that runs before the add returns. The reach is narrower than the path beside it and worth saying so. The column is a foreign key, so an id naming nothing was already refused by the database, and the one entry in the catalogue is reached with each person's own account, whose OAuth client is registered through its own call and sent to a pinned address. What was reachable is a credential of the wrong kind being accepted and spent on behalf of somebody who never agreed to it, a malformed id arriving as a database error where a refusal belongs, and the whole shape returning with the first deployment-bearer entry a fork re-adds, which the catalogue invites. Which kind an entry takes is decided beside the entry, because it is a property of the vendor's auth rather than of the request. Both add paths then ask one function the same question, so a credential that does not exist and one of the wrong kind are still refused in the same words and the endpoint cannot be asked which ids are real. The curated route maps that refusal to a 400 rather than letting it surface as a 500. Re-adding a curated server no longer clears the credential it points at. That column holds the OAuth client registering one put there, and a re-add to change an instance host said nothing about it while clearing it anyway, leaving the row orphaned and everybody who had connected told there is no client registered. * Spend an MCP token only for its own server, and only at its own address Attaching a credential to a server is the one place this deployment accepts a reference to a stored secret rather than the secret itself. Everywhere else the value arrives in the request that stores it, and the id it gets is nobody's to choose: storeAgentAuth mints its own row from the key an administrator typed. So this is the field where which secret and which address can be made to disagree, and the add is what settles it, because refreshTools runs before the call returns and sends what it decrypts to the URL from the same request. Both ways they could disagree are now refused. A credential has to belong to the server it is attached to, which the vault already records: storeMcpToken sets the provider to the server it mints for and is the only way the plugins screen makes one, so nothing a deployment can reach through the UI is refused by this. And a server that already holds a credential cannot be re-added at a different address, which is the case a check on ownership cannot see: the token does belong to that server, and only the address moved. The second is why the first is not enough alone. Both delivered a stored token to a host the caller named, before any Bot, grant or policy check existed, and a stored credential is otherwise unreadable by design. Refused rather than repaired, because both harmless readings are served by something else. Correcting a title or retrying an interrupted add sends the same URL and is untouched, a server holding no credential can still be re-addressed, and moving one that does means removing it and adding it again with the token the new address is meant to have. Curated servers are unaffected: their URL comes from the catalogue rather than the request, and an instance hostname is matched against the vendor's anchored pattern before anything is stored. The upsert test from #214 now mints its own token. It had reused one credential across two server ids, which is a shape storeMcpToken cannot produce. * Show a dot on a channel a Bot has spoken in unseen (#259) * Give a membership a memory of when its channel was last read * Stamp the caller's membership read, and say so in the roster * Let a member say they have read a channel * Carry the read marker to the app and let it be stamped * Draw a dot on a channel a Bot has spoken in unseen * Mark a channel read the moment it is the one on screen * Narrow the admin and settings rails to the width their labels earn * Keep a fast clock elsewhere from turning mark-read into a storm * Say in the changelog what the unread dot is and is not * Stamp a read against the message clock, not just this one * Assert the clamped stamp without reaching through an optional chain * Regenerate the read-marker migration behind the queue and the frames * Refuse to mark a deleted channel read, matching the pin * Update azure/setup-helm action to v5 (#260) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Say what a strict content-security-policy has to allow (#225) * Point the test at the database the project actually has (#234) * Let the API reach Intelligence and sign-in when a NetworkPolicy is on (#257) * Refuse a credential written into the rest of the address (#230) Refusing a credential in the userinfo closed one spelling and left the two beside it open. A token in the query string or the fragment was accepted, and addCustomServer writes the address it was given into mcp_servers.url and into the configuration.changed audit payload verbatim. Redaction keys on the field name and url is not a sensitive one, so the secret landed in an append-only row in clear text, which is the disclosure the userinfo rule exists to prevent. The name is read rather than matched against a list. An exact-name version of this rule refused token and accepted auth_token, api_token, x-api-key and X-Amz-Signature, and an operator has no way to know which spellings the check happens to hold. Reading the name over-refuses in one direction on purpose: a misread parameter costs a rename, a missed one cannot be deleted afterwards. The fragment is split at the first question mark first, because a hash route or an OAuth-style callback puts a path in front of the parameters and reading the whole fragment as one query string turns all of it into a single name that matches nothing. metadata.goog is refused too, by asking the list browsing already uses rather than keeping a second copy here. It is Google's own short alias for the metadata server and it carries a dot and none of the suffixes this check lists, so it read as an ordinary vendor name, while the long spelling was refused only incidentally by the .internal test. --------- Co-authored-by: Guido Vizoso <guido.vizoso9@gmail.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Vaibhav Zope <121456155+zopeVaibhav@users.noreply.github.com> Co-authored-by: David McKay <davidmckayv@users.noreply.github.com> --------- Co-authored-by: Guido Vizoso <guido.vizoso9@gmail.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Vaibhav Zope <121456155+zopeVaibhav@users.noreply.github.com> Co-authored-by: David McKay <davidmckayv@users.noreply.github.com>
1 parent 8f68eaa commit a46b5f9

6 files changed

Lines changed: 96 additions & 4 deletions

File tree

.env.example

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -230,8 +230,13 @@ COMPUTER_TOKEN=
230230
#
231231
# This is attribution, not anonymity, and it is not a boundary by itself: it gives a security team a
232232
# per-Bot address for network rules alongside AGENT_COMPUTER_POLICY.
233-
# EGRESS_PROXY_DEFAULT=http://user:password@proxy.internal:8080
234-
# EGRESS_PROXY_SALES_BOT=http://sales.proxy.internal:8080
233+
#
234+
# These go in `egress.env` beside this file, NOT here. The names are per-Bot, so Compose cannot
235+
# list them the way it lists every variable below, and it hands a container only what it is told to.
236+
# In `.env` they reach no process and the browser goes out directly with nothing saying so.
237+
#
238+
# EGRESS_PROXY_DEFAULT=http://user:password@proxy.internal:8080
239+
# EGRESS_PROXY_SALES_BOT=http://sales.proxy.internal:8080
235240

236241

237242
# The managed coworker AG-UI endpoint. Optional: use an HTTP(S) URL, and set MANAGED_AGENT_TOKEN

.gitignore

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@ docs/plans/
1111
.env
1212
.env.*
1313
!.env.example
14+
# Per-Bot egress proxies. Carries credentials in the URL, like .env does.
15+
egress.env
1416
node_modules/
1517
**/dist/
1618
app/src/lib/generated/application-config.ts

CHANGELOG.md

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -231,6 +231,27 @@ credential is refused rather than quietly attached to fail on its next call.
231231
Curated servers keep working as they did. Their URL comes from the catalogue rather than the
232232
request, and a per-instance hostname is matched against the vendor's own anchored pattern before
233233
anything is stored, so re-adding one cannot point it at an address of the caller's choosing.
234+
### A configured egress proxy reaches the browser that uses it
235+
236+
`EGRESS_PROXY_DEFAULT` and `EGRESS_PROXY_<BOT>` were documented as the way to give a Bot a stable
237+
outbound address, and Compose passed neither to anything. `docker-compose.yml` named no egress
238+
variable and had no `env_file`, so the shared computer resolved every Bot to no proxy and went out
239+
directly, and under the supervisor the same emptiness meant there was nothing to forward into the
240+
computers it creates.
241+
242+
The failure was silent, which for a setting whose purpose is to give a security team a per-Bot
243+
address for network rules is the worst of the available failures. The stack started, the browser
244+
left by the host, and the Computers screen reported "Leaves directly" because it was reading the
245+
same empty environment.
246+
247+
They now live in `egress.env`, which both the computer and the supervisor are given. A file rather
248+
than more `environment:` entries because `EGRESS_PROXY_<BOT>` is derived from a Bot's id and there
249+
is no fixed set of names to list; a file of its own rather than `.env` because that one holds the
250+
deployment's secrets and the container running a browser and a Bot's shell is deliberately not
251+
given them. It is optional, so a deployment with no proxy is unchanged, and gitignored, because a
252+
proxy URL can carry a password.
253+
254+
**Move these two out of `.env` and into `egress.env`.** In `.env` they reach no process.
234255

235256
### Knowledge searches instead of guessing
236257

docker-compose.yml

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,15 @@ services:
6161
# Loopback only. This process drives a browser holding real logins; COMPUTER_TOKEN is the
6262
# request control, and loopback keeps the surface off routed networks.
6363
- "127.0.0.1:${COMPUTER_PORT:-4100}:4100"
64+
# Per-Bot egress, in a file of its own because the names are not knowable here.
65+
#
66+
# `EGRESS_PROXY_<BOT>` is derived from the Bot's id, so there is no fixed list to write out the
67+
# way COMPUTER_TOKEN is. Not `.env`: that holds the deployment's secrets, and this container
68+
# drives a browser and runs a Bot's shell, so it is given what it needs and not the rest.
69+
# Optional, because going out directly is the ordinary case and must still start.
70+
env_file:
71+
- path: ./egress.env
72+
required: false
6473
environment:
6574
# The secret every caller must present. The container refuses to start without it.
6675
COMPUTER_TOKEN: ${COMPUTER_TOKEN:-}
@@ -143,6 +152,11 @@ services:
143152
build:
144153
context: .
145154
dockerfile: supervisor/Dockerfile
155+
# The same file, because this process does not read these itself: it forwards every EGRESS_PROXY
156+
# key out of its own environment into each computer it creates, so it has to be given them first.
157+
env_file:
158+
- path: ./egress.env
159+
required: false
146160
environment:
147161
PORT: "4300"
148162
# Shared with the API server. The Bot-level verb set is the boundary; this token keeps other

docs/configuration.md

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -191,15 +191,33 @@ where `<provider>` is `google`, `microsoft` or `okta`.
191191
- `WORKSPACE_DIR`
192192
- `PROFILES_DIR`
193193
- `COMPUTER_BOT_ID`
194-
- `EGRESS_PROXY_DEFAULT`
195-
- `EGRESS_PROXY_<BOT_ID>`
194+
- `EGRESS_PROXY_DEFAULT` (in `egress.env`, see below)
195+
- `EGRESS_PROXY_<BOT_ID>` (in `egress.env`, see below)
196196
- `COMPUTER_SHELL_ENV`
197197

198198
A command on the computer inherits PATH, locale and terminal names, and the proxy variables, not
199199
the rest of the process environment. Userinfo is stripped from a proxy URL, so a password in
200200
`HTTP_PROXY` is not in `env`. `COMPUTER_SHELL_ENV` is a comma-separated list of extra names to
201201
pass. Naming a secret or a credentialed proxy there is an operator's decision; the default does not.
202202

203+
### Per-Bot egress
204+
205+
The two egress variables live in `egress.env` at the repository root, not in `.env`. `EGRESS_PROXY_<BOT_ID>`
206+
is derived from a Bot's id, so there is no fixed set of names for Compose to list the way it lists
207+
every other variable, and Compose passes a container only the names it is given. A file of its own
208+
rather than `.env` because that one holds the deployment's secrets and neither the browser container
209+
nor the supervisor is given those.
210+
211+
```sh
212+
# egress.env
213+
EGRESS_PROXY_DEFAULT=http://user:password@proxy.internal:8080
214+
EGRESS_PROXY_SALES_BOT=http://sales.proxy.internal:8080
215+
```
216+
217+
The file is optional and gitignored. Without it every Bot's browser goes out directly, which is the
218+
default. Both the shared computer and the supervisor are given it: the computer resolves its own
219+
proxy from these names, and the supervisor forwards them into each computer it creates.
220+
203221
The supervisor also reads:
204222

205223
- `COMPUTER_IMAGE`

tests/compose.test.ts

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,3 +121,35 @@ test("runs migrations after PostgreSQL becomes healthy", () => {
121121
expect(compose).toContain("condition: service_healthy");
122122
expect(compose).toContain('"drizzle-kit", "migrate"');
123123
});
124+
125+
/**
126+
* Per-Bot egress reaches the processes that read it.
127+
*
128+
* `EGRESS_PROXY_<BOT>` and `EGRESS_PROXY_DEFAULT` are resolved from `process.env` by the computer
129+
* itself (`agent-computer/src/egress.ts`), and the supervisor forwards every `EGRESS_PROXY` key out
130+
* of its own environment into each computer it creates (`supervisor/src/index.ts`). Compose gives a
131+
* container only what its `environment:` and `env_file:` blocks name, and for a long time neither
132+
* named these, so an operator who configured a proxy per the documentation got a browser that went
133+
* out directly and no error saying so.
134+
*
135+
* A file rather than `environment:` entries because the names are per-Bot and therefore not knowable
136+
* here, and a file of its own rather than `.env` because that one holds the deployment's secrets and
137+
* the browser container is deliberately not given them.
138+
*/
139+
test("carries per-Bot egress into the computer and the supervisor", () => {
140+
const compose = readFileSync(
141+
join(import.meta.dir, "..", "docker-compose.yml"),
142+
"utf8",
143+
);
144+
145+
// Both halves: the shared computer reads them itself, and the supervisor passes them on.
146+
const services = compose.split(/^ {2}(?=\S)/m);
147+
for (const name of ["agent-computer:", "supervisor:"]) {
148+
const service = services.find((block) => block.startsWith(name));
149+
expect(service).toBeDefined();
150+
expect(service).toContain("egress.env");
151+
}
152+
153+
// Optional, because a deployment with no proxy is the ordinary case and must still start.
154+
expect(compose).toContain("required: false");
155+
});

0 commit comments

Comments
 (0)