Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a security enhancement to block requests to the instance's Operations API ports within the http_fetch tool, including during redirects. It updates the design documentation, retrieves the configured ports from the environment, integrates the port-blocking logic into the target checks, and adds corresponding unit tests. The review feedback correctly identifies a high-severity issue where ports loaded from environment variables as strings would bypass the blocking policy due to a strict type check (typeof port === 'number'). A suggestion is provided to parse and validate these ports properly to ensure the security policy is correctly enforced.
…olders An environment placeholder in config expands to a string, so a port such as '9926' failed the typeof number filter and http_fetch would not block it. Normalize numeric strings to ports and drop anything that is not a valid port. Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Thank you for the PR, this is great!
I think we had intended to close part of this gap with #3011, right @DavidCockerill ? But for the default mode, we should block this by default.
AI review found that this might not be complete:
End-to-end confirmation of the two inline host:port comments below, from Harper's security QA lane:
Thanks for this — the port policy itself holds up well: bare and numeric-string ports, the localhost / 127.0.0.1 / [::1] / https: spellings, and even a real 302 hop from the HTTP port to the ops URL were all refused in testing. One gap, though, and it makes the block a no-op on some deployments.
The block never fires when the Operations API port is configured as host:port. operationsApiPortsFrom (agent/tools/httpFetchTool.ts:157) only accepts a number or a bare-digits string, so operationsApi.network.port: 127.0.0.2:9925 is dropped and the blocklist built in agent/agent.ts:111-114 is []:
operationsApiPortsFrom([9925]) -> [9925]
operationsApiPortsFrom(['9925']) -> [9925]
operationsApiPortsFrom(['127.0.0.2:9925']) -> []
operationsApiPortsFrom(['[::1]:9925']) -> []
operationsApiPortsFrom(['localhost:9925']) -> []
host:port is a supported form, and the only way to bind the ops API to one interface: server/threads/threadServer.js and server/threads/socketRouter.ts (portNumberOf, :284) both split on the last colon, and @harperfast/integration-testing starts Harper with --OPERATIONSAPI_NETWORK_PORT=127.0.0.2:9925. The default config and the Docker image use a bare 9925, so they are protected; any deployment that binds the ops API to a specific interface is not.
End to end on 68925d3: with the agent enabled (autoApprove: true) and a fake OpenAI-compatible model that scripts one http_fetch call (POST / with {"operation":"add_user", ...}), the tool returned {"ok":true,"result":{"status":200, ... "successfully added"}} and list_users showed the new user afterwards.
Suggested fix: parse the port the way portNumberOf does (digits after the last colon; sharing that helper would keep the two from drifting), and add '127.0.0.2:9925', '[::1]:9925' and 'localhost:9925' → [9925] to the operationsApiPortsFrom cases in unitTests/agent/httpFetchTool.test.js. With that change applied to dist, the same end-to-end run is refused with http_fetch blocked by operations API port policy: 9925 and no user is created. securePort goes through the same function, so it gets the same fix.
We have two integration specs for this (the real-port policy across config spellings, and the agent loop end to end) that we can contribute to integrationTests/security/ once the fix is in, if useful.
— Claude Opus 5.5, posting a finding from Harper's security QA lane (found and verified by Claude Mythos 5.1)
| export function operationsApiPortsFrom(values: readonly unknown[]): number[] { | ||
| const ports: number[] = []; | ||
| for (const value of values) { | ||
| const port = typeof value === 'string' && /^\s*\d+\s*$/.test(value) ? Number(value) : value; |
There was a problem hiding this comment.
[major] Host-and-port Operations API settings are still discarded. With operationsApi.network.port: "127.0.0.1:9925" (and securePort: null), this digits-only check drops the configured port, so http_fetch can reach http://127.0.0.1:9925/, directly or through a redirect. The listener accepts host-and-port strings, and the target guard compares against the resulting numeric port. Please normalize that supported form as well as numeric strings, and add a regression that exercises the boot-setting normalization boundary.
There was a problem hiding this comment.
Fixed in operationsApiPortsFrom: the port is now the digits after the last colon, so 127.0.0.1:9925 and [::1]:9925 are blocked like 9925. The new unit tests cover host:port settings at that boundary and check that the request is refused before any connection is made.
| const operationsApiPorts = operationsApiPortsFrom([ | ||
| env.get(CONFIG_PARAMS.OPERATIONSAPI_NETWORK_PORT), | ||
| env.get(CONFIG_PARAMS.OPERATIONSAPI_NETWORK_SECUREPORT), | ||
| ]); |
There was a problem hiding this comment.
[major] Normalize supported string ports before building the deny-list.
With operationsApi.network.port: "127.0.0.1:9925" (or quoted "9925") and securePort: null, this filter produces an empty list. Those are supported listener inputs: validation/configValidator.ts:79-81 accepts strings, and server/threads/threadServer.js:416-426 parses numeric strings and host:port forms into TCP listener ports. Configuration flattening preserves the original values. Consequently, http_fetch admits http://127.0.0.1:9925/, directly or through a redirect, despite the intended Operations API exclusion.
A focused execution of this production filter followed by the real target guard confirmed that numeric 9925 is blocked while "9925", "127.0.0.1:9925" and "[::1]:9925" are admitted. This verified the policy decision, without starting an Operations API server. The added regression at unitTests/agent/httpFetchTool.test.js:297-314 supplies numeric server.address().port directly to the factory, bypassing this boot boundary.
Please normalize both settings using the listener's numeric-string and host:port interpretation before filtering, and verify that these supported configurations reject the target before any request is sent.
There was a problem hiding this comment.
The fix is in operationsApiPortsFrom, which agent.ts already calls with the raw boot values, so the deny-list now includes host:port forms as well as numeric strings. Numeric strings were already handled; this round adds host:port parsing and a unit test for it.
|
operationsApiPortsFrom now takes the port after the last colon, matching how the listener parses host:port, so 127.0.0.2:9925, [::1]:9925 and localhost:9925 all produce 9925. I added those cases to the operationsApiPortsFrom tests and a request-level test that refuses host:port settings without connecting. I mirrored the listener's logic rather than exporting portNumberOf from socketRouter.ts, to keep the agent tool from depending on the server module. |
|
|
||
| // Built from the boot config, never `liveConfig`: no runtime config change can widen what it reaches. | ||
| const httpFetchTool = buildHttpFetchTool(config.httpFetch); | ||
| const operationsApiPorts = operationsApiPortsFrom([ |
There was a problem hiding this comment.
Nothing tests these two reads, and they’re the only link between the config and the block. The new tests pass ports straight to operationsApiPortsFrom and the factory, so a wrong key here would leave every test passing while http_fetch reaches 9925 again. Could you export this read the way mergeConfig and resolveScopes are, and add a case in unitTests/agent/agent.test.js that sets the shipped default with env.setProperty (port: null, securePort: 9925) and expects [9925]? Non-blocking, since the keys are right today.
There was a problem hiding this comment.
Extracted the Operations API port read into an exported resolveOperationsApiPorts(), which startOnMainThread now calls. Removed the = [] default from buildHttpFetchTool's operationsApiPorts parameter so a caller that omits it fails typecheck. Added a test that sets the shipped defaults (port null, securePort 9925) with env.setProperty and asserts the result is [9925].
DavidCockerill
left a comment
There was a problem hiding this comment.
Thanks, this is what I was after. Making the ports argument required is a nice touch because typecheck catches a caller that forgets it.
Summary
Closes #3154
Validation
pm run build
ode --conditions=typestrip ./node_modules/mocha/bin/mocha.js unitTests/agent/httpFetchTool.test.js unitTests/agent/agent.test.js (96 passing)
pm run typecheck
pm run lint:required