-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(codex): close pool eligibility inside the caller-owned preview read fence #4883
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
1b6e1ac
05bf0f6
3dacc5b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| # L1 — preview read fence and dependency audit | ||
|
|
||
| Two independent safety units that deliberately do not stack on any feature branch. | ||
| Each ends as its own pull request against `dev` with exact-head CI evidence. | ||
|
|
||
| | Unit | Subject | Artifact | | ||
| |---|---|---| | ||
| | A | Issue #4850 — caller-owned `thread_spawn` preview still reads physical main `auth.json` | `010_issue_4850_preview_pool_eligibility_fence.md` | | ||
| | B | PR #4873 — dependency audit overrides for `hono` and the docs-site toolchain | `020_pr_4873_dependency_audit_review.md` | | ||
|
|
||
| ## Why they are separate | ||
|
|
||
| Unit A changes runtime credential-boundary behaviour in `src/codex/` and | ||
| `src/server/responses/`. Unit B changes only `package.json` and lockfiles and is | ||
| authored by an outside contributor. Putting them on one branch would make the | ||
| contributor's commit un-landable on its own and would drag a credential-boundary | ||
| review into a dependency bump. | ||
|
|
||
| ## Verification posture | ||
|
|
||
| No local suite, typecheck, build, or install runs in this lane. Correctness is | ||
| argued statically from the source and the call graph, and confirmed by hosted CI | ||
| at the exact head of each pull request. That constraint is why unit A's completion | ||
| criteria are written as observable read counts rather than as "the right token was | ||
| eventually sent": a behavioural assertion that hosted CI can run is the only proof | ||
| available here, and it is the stronger one anyway. | ||
|
|
||
| ## Boundaries | ||
|
|
||
| This lane does not merge, does not push to `dev`, and does not rebase without an | ||
| instruction. It does not widen timeouts, add retries, skip platforms, or mask a | ||
| failure to make CI green. Windows jobs are dispatch-only, so any change with | ||
| Windows impact is reported rather than dispatched here. | ||
|
|
||
| Unreleased security analysis belongs in `.tmp/`, never in this directory. | ||
| Both units here concern already-public material: #4850 is a filed public issue | ||
| with the call path in its body, and #4873's advisories are published GHSA records. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| # Unit A — issue #4850: pool eligibility is outside the preview read fence | ||
|
|
||
| ## The gap, stated precisely | ||
|
|
||
| `src/server/responses/request-prepare.ts` already computes the ownership fence. | ||
| `previewRequestScopedMainCredential` is the route ownership predicate ANDed with | ||
| `hasCallerCodexBearer`, exactly as final authentication validates it, and | ||
| `nativeMainReadsForbidden` ORs it with retained recovery and a draining selector. | ||
| Quota priming, entitlement discovery, and the denied-model cache all honour it. | ||
|
|
||
| `previewSelectionOptions` does not. It carries `nativeMainSelectionOnly` and the | ||
| uploaded-file retention bit and stops there. When that object reaches | ||
| `previewCodexAccountForRequest -> pickPriorityPreemption -> getEligiblePoolAccounts`, | ||
| `codexAccountUnusableReason` in `src/codex/account-usability.ts` finds no | ||
| `isMainAccountTokenLive` override and falls through to its default, | ||
| `isMainAccountCredentialUsable()`, which opens and parses the physical `auth.json`. | ||
|
|
||
| It happens twice because the same options object is used twice: once for the direct | ||
| preview in `prepareResponsesRequest`, and once inside the callback | ||
| `applySubagentModelFallback` invokes per candidate model. The post-decryption | ||
| recovery re-preview builds `recoverySelectionOptions` the same way and has the same | ||
| omission. | ||
|
|
||
| ## What is and is not at stake | ||
|
|
||
| Not a token leak. Final authentication never selects the physical main credential | ||
| for a caller-owned request: it passes `isMainAccountTokenLive: () => preserveRequestOwnedMainPin` | ||
| into its own selection options, so main is either served as the caller's own | ||
| credential or scored `main_credential_unavailable` and dropped. ADR-0086 already | ||
| rejected reading the physical main token for identity. | ||
|
|
||
| What is at stake is that operator-main liveness, cached quota, and plan state can | ||
| enter the score that decides whether a subagent's model is rewritten, for a request | ||
| that owns its credential. A preview that scores main differently from the resolution | ||
| it exists to predict is a correctness defect on top of the boundary defect. | ||
|
|
||
| ## Chosen direction | ||
|
|
||
| Use the existing `CodexAccountUsabilityOptions.isMainAccountTokenLive` seam, and | ||
| give it the same value final authentication gives it rather than a preview-only | ||
| constant. | ||
|
|
||
| That answers the open question in the issue review directly. `preserveRequestOwnedMainPin` | ||
| is not an arbitrary choice: it is the only value that makes the preview agree with | ||
| the resolution in both branches. When the operator has an effective manual main pin | ||
| with quota headroom, final authentication returns the caller-owned main context, so | ||
| the request really is served by main and the preview should score main eligible. | ||
| When there is no such pin, final authentication drops main from pool eligibility, | ||
| and the preview must drop it too. A hardcoded `true` would be wrong in the second | ||
| case, and a hardcoded `false` would be wrong in the first. | ||
|
|
||
| Every input to that predicate is config, policy, or in-memory runtime state — | ||
| `activeCodexAccountPinned`, `isEffectiveCodexAccountPinned`, `pausedCodexAccountIds`, | ||
| the in-memory quota score, and `matchesMainQuotaCredential`, which compares HMACs | ||
| against an observed-credential record held in `main-account-cache.ts`. Nothing in | ||
| it opens a file, which is what makes it usable on the fenced side. | ||
|
|
||
| To keep preview and final authentication from drifting apart again, the predicate | ||
| moves into one exported function in `src/codex/auth-context.ts` that both callers | ||
| use. Two copies of a fence is how this gap appeared in the first place. | ||
|
|
||
| ## Edit set | ||
|
|
||
| | File | Change | | ||
| |---|---| | ||
| | `src/codex/auth-context.ts` | Extract `requestOwnedMainPinState` and call it from `resolveCodexAuthContext` | | ||
| | `src/server/responses/request-prepare.ts` | Pass the synthetic `isMainAccountTokenLive` in `previewSelectionOptions` and `recoverySelectionOptions`, scoped to the ownership flag | | ||
| | `tests/responses/responses-preview-main-read-fence.test.ts` | Assertions (a) and (b) below | | ||
|
|
||
| No new test file, so `layout.json` and `test-layout-expected.json` are untouched. | ||
| No file here is on the size-ratchet baseline. No `src/` area is created or removed | ||
| and no invariant test disappears, so `structure:check` has nothing to consume. | ||
|
|
||
| ## Completion criteria | ||
|
|
||
| Deliberately stricter than "the right token was eventually sent", because that was | ||
| already true before the fix and the defect survived it anyway. | ||
|
|
||
| (a) A caller-owned `thread_spawn` performs **zero** `auth.json` reads across the | ||
| whole request, asserted on the unfiltered read counter rather than through the | ||
| denial-cache stack filter that currently hides these two reads. | ||
|
|
||
| (b) Ordinary main selection is unchanged. A request with no caller bearer still | ||
| reads the physical credential and still selects main when it is healthy, so the | ||
| fix cannot be satisfied by making main globally ineligible. | ||
|
|
||
| (c) The #3166 healthy main-pin behaviour survives: a caller-owned request under an | ||
| effective main pin is still previewed as main. | ||
|
|
||
| ## Risk | ||
|
|
||
| The behaviour change is confined to requests where `previewRequestScopedMainCredential` | ||
| is true. For every other request the option is absent and `account-usability.ts` | ||
| takes the identical default branch it takes today. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| # Unit B — PR #4873: dependency audit overrides | ||
|
|
||
| Contributor PR from `agentHits`, head `7c9479b5722e3f0af56a73410ca6a74fd18905b8`, | ||
| base `dev`, fork `agentHits/opencodex`, branch `fix/security-audit-overrides-hono-astro`. | ||
| Four files: `package.json`, `bun.lock`, `docs-site/package.json`, `docs-site/bun.lock`. | ||
| No application source changes. | ||
|
|
||
| This unit reviews and clears the existing pull request. It does not open a | ||
| replacement, and it does not merge. | ||
|
|
||
| ## The follow-up commit landed | ||
|
|
||
| The review asked for an exact pin instead of a caret. Head `7c9479b57` has | ||
| `"hono": "4.13.8"` in root `overrides`, the caret removed, matching the | ||
| neighbouring exact pin on `@hono/node-server`. Root `bun.lock` carries the same | ||
| `4.13.8` in its overrides block and resolves `hono@4.13.8`. Manifest and lock agree. | ||
|
|
||
| ## Actual impact, not advisory severity | ||
|
|
||
| The PR description groups the `hono` advisories under "Root proxy runtime". That is | ||
| accurate about which manifest changed and misleading about what is exposed. | ||
|
|
||
| `hono` is not a direct dependency. It arrives only through | ||
| `@modelcontextprotocol/sdk@1.30.0`, which declares `hono: ^4.11.4`. This repository | ||
| imports that SDK in exactly one file, `src/adapters/cursor/mcp-manager.ts`, and only | ||
| its **client** entrypoints: `client/index.js`, `client/stdio.js`, and | ||
| `client/streamableHttp.js`. Nothing under `src/`, `gui/src/`, or `scripts/` imports | ||
| `@modelcontextprotocol/sdk/server/*` or `@hono/node-server`. | ||
|
|
||
| All three `hono` advisories need the application to be running hono as a server: | ||
| `toSSG()` is the static-site generation helper, `parseBody()` parses an inbound | ||
| request body, and the query-parser differential is about inbound request URLs. The | ||
| proxy serves its own HTTP through `Bun.serve`. So no proxy request path reaches the | ||
| vulnerable code, and this half of the PR is dependency-graph hygiene that gets | ||
| `bun audit` to zero rather than a fix for a reachable proxy vulnerability. | ||
|
|
||
| The Critical is in the other half. `GHSA-26w7-cxv4-gfx2` is remote code execution | ||
| through Astro's AVIF image optimization, which runs during `astro build` and | ||
| `astro dev`. The exposed parties are contributor machines and the docs deploy | ||
| runner, and the input is images in the repository, so an attack needs a malicious | ||
| image committed first. Bounded, real, and worth fixing. | ||
|
|
||
| ## Lockfile review | ||
|
|
||
| Reviewed statically; no install runs in this lane. | ||
|
|
||
| Every added `docs-site/bun.lock` entry is a registry package with a `sha512` | ||
| integrity hash. No `git+`, `http(s):`, `file:`, `workspace:`, or `link:` source | ||
| appears in any added line. The additions are exactly what an Astro 7.2.2 to 7.3.3 | ||
| minor bump plus the `sharp`, `svgo`, `smol-toml` and `js-yaml` overrides produce: | ||
| refreshed `@astrojs/compiler-binding-*` and `@img/sharp-*` platform binaries, and | ||
| the transitive dependencies those versions declare. | ||
|
Comment on lines
+49
to
+52
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🛡️ Analyzed with Security Review | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: sed -n '1,90p' devlog/_plan/260917_l1_preview_read_fence_and_dep_audit/020_pr_4873_dependency_audit_review.md
git show-ref | rg '4873|pull' | head -50
git log --all --oneline --decorate -- docs-site/package.json docs-site/bun.lock | head -40Repository: lidge-jun/opencodex Length of output: 5643 🌐 Web query:
💡 Result: <search_synthesis> <source_evidence> Citations:
Security Misconfiguration CWE: CWE-16 Record PR
🧰 Tools🪛 LanguageTool[style] ~49-~49: Consider an alternative for the overused word “exactly”. (EXACTLY_PRECISELY) 🤖 Prompt for AI Agents |
||
|
|
||
| Two things that look like new supply chain but are not. `@astrojs/markdown-satteri` | ||
| and the `@bruits/satteri-*` binaries are already in `dev`'s lockfile and only change | ||
| version. `find-proc` replaces `find-process`, dropping `ansi-styles`, `chalk`, | ||
| `color-convert`, `color-name`, and `loglevel`; that substitution is declared by | ||
| `astro@7.3.3` itself, not introduced by this pull request. | ||
|
|
||
| ## The docs build is not covered by CI | ||
|
|
||
| This is the part worth separating out, and it does not resolve in this PR's favour. | ||
|
|
||
| `.github/workflows/ci.yml` contains no `docs-site` reference and builds no docs. | ||
| `deploy-docs.yml` triggers only on `push` to `main` under `docs-site/**`. So the | ||
| Astro minor bump has no pull-request build gate anywhere: a fully green exact-head | ||
| run on this PR is not evidence that the docs site still builds. The author's local | ||
| "449 pages" result is the only build evidence and is an unverifiable attestation. | ||
|
|
||
| The residual exposure is a broken docs build discovered at promotion to `main` | ||
| rather than at review. That fails the deploy instead of shipping a broken site, so | ||
| it is a delay rather than an outage, but it should be a conscious acceptance. | ||
| Adding a docs-build job is out of this lane's scope. | ||
|
|
||
| What CI *does* cover: `package.json` and `bun.lock` are both in the `changes` job's | ||
| `ci` allowlist, so the cross-platform suite is in scope for this head once it runs. | ||
|
|
||
| ## What blocks exact-head evidence | ||
|
|
||
| Two independent gates, both maintainer actions, neither of which the contributor can | ||
| clear: | ||
|
|
||
| 1. **`unsponsored_surface`.** `hygiene` and `enforce-target` both fail on it, and | ||
| the PR carries `intake: hygiene-blocked`. `MAINTAINERS.md` requires explicit | ||
| security review for dependency-installation surfaces; the gate wants a | ||
| `maintainer-sponsored` label recording that the review happened. | ||
| 2. **Fork workflow approval.** `Cross-platform CI`, `React Doctor`, and | ||
| `Service lifecycle` are all sitting at `action_required` for this head. The | ||
| repository uses `all_external_contributors` approval, and `ci.yml` documents that | ||
| this approval — not the workflow's own routing — is the real boundary keeping | ||
| untrusted code off runners. For a `pull_request` event `select-windows-runner` | ||
| marks the run untrusted and pins GitHub-hosted runners, so approving does not | ||
| expose a self-hosted runner. It does run the resolved packages' install hooks, | ||
| which is why the lockfile review above had to come first. | ||
|
|
||
| The merge decision, and the decision to spend either of those gates, belongs to the | ||
| host session. This unit's output is the review and the evidence, not the merge. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔒 Security & Privacy | 🛡️ Analyzed with Security Review | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: lidge-jun/opencodex
Length of output: 157
🏁 Script executed:
Repository: lidge-jun/opencodex
Length of output: 50375
Security Misconfiguration
Reachability: Unreachable
Exploitability: Theoretical
CWE: CWE-693
Record separate Hono reachability results.
The statement that all three advisories require a running Hono server is too broad.
toSSG()is a static-site-generation API, whileparseBody()and query parsing have different usage conditions. The repository currently uses only MCP client entry points and has no affected Hono API call sites, so record each advisory as separately unreachable instead of using one server-only rationale.🤖 Prompt for AI Agents
Source: MCP tools