D3 finite source guard — bounded replacement - #70
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 |
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: 434204ecf5
ℹ️ 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 (arg === undefined) return { kind: 'unresolved' }; | ||
| if (ts.isStringLiteral(arg)) return { kind: 'string', value: arg.text }; | ||
| if (ts.isNumericLiteral(arg)) return { kind: 'number', value: Number(arg.text) }; | ||
| if (ts.isIdentifier(arg)) return resolveConstLiteral(sourceFile, arg.text); |
There was a problem hiding this comment.
If main introduces a valid local shadow such as const HOST = '0.0.0.0' and passes it to server.listen, this lookup ignores that binding and resolves the identifier to the top-level HOST = '127.0.0.1'. The loopback test therefore passes while the server actually binds all IPv4 interfaces; resolve the identifier's lexical declaration, or fail closed when it is not the expected top-level constant.
Useful? React with 👍 / 👎.
| const isMemberSite = | ||
| ts.isPropertyAccessExpression(callee) && | ||
| callee.name.text === 'createServer' && | ||
| ts.isIdentifier(callee.expression) && | ||
| defaultOrNamespace.has(callee.expression.text); |
There was a problem hiding this comment.
If another closure file calls (http).createServer(...) or http['createServer'](...), the call still directly targets the imported node:http binding but this predicate ignores it because the receiver is not a bare identifier/property-access pair. Adding such a second authored server leaves the reported site count at one, so the new exact-one invariant passes; unwrap transparent expression wrappers and recognize static element access before matching.
Useful? React with 👍 / 👎.
…er wrappers Close the two Codex findings on the finite D3 source guard, keeping both rules strictly structural (no alias/value-flow, fixpoint, Proxy/Reflect, RC/HA, taint, or runtime logic): - Rule D listen host/port now resolves the identifier to the declaration actually visible at the use site (innermost lexical scope), so a nearer const literal shadows the top-level one and a let/var, parameter, destructured, for-loop/catch, or non-literal binding fails closed. - Rule D createServer recognition normalizes transparent syntax only — parenthesized receivers and static element access such as http['createServer'] — so a second wrapped authored site is counted. Adds regressions for both, including sibling-scope isolation and the adjacent shadow/wrapper forms. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016A9gwyNtdnZzRHduGzih11
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df72c2bfc4
ℹ️ 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 target = resolution.target; | ||
| edges.push({ from: fromRel, specifier, to: toRepoRelative(target), kind }); | ||
| if (!closure.has(target)) { | ||
| closure.add(target); | ||
| queue.push(target); |
There was a problem hiding this comment.
When a file under src/cockpit-host/ directly imports an already-transitive dependency such as ../domain/review.js, this code merely records the edge and finds the target already present in EXPECTED_CLOSURE, so every new closure assertion remains green. This removes the documented boundary that host code may reach domain truth only through src/cockpit/, allowing future rendering code to bypass the D1/D2 validation and projection layer without CI noticing; validate host-originated edges against the host/Cockpit roots in addition to checking closure membership.
Useful? React with 👍 / 👎.
| const file = queue.shift(); | ||
| if (file === undefined) break; | ||
| const fromRel = toRepoRelative(file); | ||
| const references = collectRuntimeReferences(file, readFileSync(file, 'utf8')); |
There was a problem hiding this comment.
If authored host code reads process.env—for example, interpolating a token into a response—the new guard reports no reference or violation because it inventories only module edges and calls to require/import. The replacement deleted the previous environment-access check while the D3 architecture still promises no environment or secret access, so such a type-correct change now passes this purity suite and can leak secrets; retain a finite source check for direct environment reads or explicitly remove that security guarantee.
Useful? React with 👍 / 👎.
| if (element.propertyName?.text === 'createServer' || element.name.text === 'createServer') { | ||
| named.add(element.name.text); |
There was a problem hiding this comment.
For an aliased import such as import { request as createServer } from 'node:http', element.name.text is createServer, so a subsequent request call is counted as the required HTTP server creation even though the imported export is request. The exact-one assertion can therefore pass after the real http.createServer site is replaced while still claiming that a direct server-creation site exists; when propertyName is present, only that imported name should determine whether the binding represents createServer.
Useful? React with 👍 / 👎.
1 participant
D3 finite source guard — bounded replacement
Built fresh from the exact PR #55 HEAD
5ae2b786ad6dc4653286d4c2b50e1fd705daa974. PR #64 and PR #67 are retired / closed / unmerged; none of their analyzer implementations were resumed, copied, cherry-picked, or recreated.What this is
A finite reviewed-source policy guard only. It asserts bounded facts about the written source of the read-only Cockpit D3 dashboard host — nothing about runtime behavior. Concretely it proves:
src/cockpit-host/, resolved through the TypeScript AST and normal relative-module resolution, equals a pinned set of files. Only runtime edges count; type-only imports/exports are erased.node:http,node:urlonly (the builtins the current host actually imports); every other builtin is rejected.require(...)is forbidden (andimport x = require(...)); no CommonJS.cts/.cjsexecution.import(...)fails closed; a static-string dynamic import is a supported, resolvable edge.http.createServer(...)site exists in the authored closure (the real host), and its authoredlisten(...)uses the loopback bind127.0.0.1, with the legitimate port constant preserved where statically resolvable.Scope of the network claim (honest bound)
node:httpserver), not runtime behavior. Node'snode:httpclient API and globalfetch/WebSocketare outside what an import-closure source guard can prove.Changed files (exactly)
tests/cockpit-host/purity.test.tstests/cockpit-host/executable-closure.tsdocs/architecture/D3-cockpit-dashboard-host.mdExplicitly NOT present in this replacement
This guard deliberately implements none of the following (and adds no such machinery):
The single source of truth for "what is an import" is the official
typescriptparser AST — there is no custom JavaScript lexer/parser, no value/fact propagation, no receiver or global tracking, and no runtime code-generation route scanning.🤖 Generated with Claude Code
https://claude.ai/code/session_016A9gwyNtdnZzRHduGzih11