security: close audit findings 1/2/4/7/9/11 and partially 3/16 - #697
Conversation
📝 WalkthroughWalkthroughThis PR separates management and data-plane authentication, adds GUI session and CSRF handling, introduces ownership-aware config cleanup, centralizes provider diagnostic outbound safety, hardens npm updates, and updates platform integrations, tests, and documentation. ChangesManagement authentication and GUI sessions
Owned config cleanup
Provider diagnostics and transport hardening
Update and platform hardening
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: 7d995ef70f
ℹ️ 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".
| } | ||
| try { | ||
| const path = adminApiTokenFilePath(); | ||
| assertSafeDirectory(dirname(path)); |
There was a problem hiding this comment.
On a default first start with no OPENCODEX_ADMIN_AUTH_TOKEN, this creates the config directory and later admin-api-token without calling recordOwnedConfigPath. Because createOwnership() only accepts an empty directory, the subsequent first config write cannot establish ownership; if ownership already exists, the token is still absent from the manifest. Consequently ocx uninstall refuses or reports a partial uninstall and leaves the config and credential behind. Register the token path before directory/file creation and include it among the initially owned paths.
Useful? React with 👍 / 👎.
| const session: GuiSessionRecord = { | ||
| csrfToken: randomBytes(32).toString("base64url"), | ||
| origin, | ||
| expiresAt: now + GUI_SESSION_TTL_MS, |
There was a problem hiding this comment.
For every loopback dashboard opened longer than five minutes, this fixed expiration is reached even when the dashboard remains active, and requireManagementAuth() deletes the session without extending it. The GUI then handles the next 401 by prompting for the admin token, which a normal local user was never given; reloading happens to mint another session but is not performed automatically. Use sliding expiration on valid requests or add a safe automatic session-refresh flow so the advertised seamless local dashboard does not stop working after five minutes.
Useful? React with 👍 / 👎.
| warnProxyBoundaryOnce(); | ||
| return globalThis.fetch(url, { ...init, method: "GET", redirect: "manual" }); | ||
| } | ||
| if (proxyConfigured && resolved.privateNetwork && !noProxyMatches(parsed)) { |
There was a problem hiding this comment.
When HTTP_PROXY, HTTPS_PROXY, or ALL_PROXY is supplied directly by the environment and NO_PROXY is absent, a localhost/private provider with allowPrivateNetwork: true reaches this branch and is rejected. applyProxyEnv() only appends the documented automatic loopback exclusions when config.proxy is set, because it returns immediately otherwise, so common environment-proxy setups cannot run connection tests or model discovery against Ollama and other local providers. Append the loopback entries whenever any outbound proxy is active, or special-case loopback here.
Useful? React with 👍 / 👎.
| const actual = req.headers.get("x-opencodex-api-key")?.trim() | ||
| || req.headers.get("authorization")?.replace(/^Bearer\s+/i, "").trim(); | ||
| if (actual && equalSecret(actual, state.token)) return null; |
There was a problem hiding this comment.
This now accepts only the independent admin token for ordinary management requests, but docs-site/src/content/docs/reference/configuration.md:235-256 and its translations still instruct remote users that OPENCODEX_API_AUTH_TOKEN or dashboard-generated apiKeys authenticate both /api/* and the data plane. Users following the published LAN setup will therefore receive 401s from the dashboard and operational scripts. Update the directly affected remote-access and architecture documentation to explain OPENCODEX_ADMIN_AUTH_TOKEN, the generated admin-token file, and which credentials belong to each plane.
AGENTS.md reference: AGENTS.md:L122-L123
Useful? React with 👍 / 👎.
| export function configuredAdminToken(configDir = getConfigDir(), env: NodeJS.ProcessEnv = process.env): string | null { | ||
| return env.OPENCODEX_ADMIN_AUTH_TOKEN?.trim() || loadAdminTokenFromFile(configDir); |
There was a problem hiding this comment.
When a user installs a service with OPENCODEX_ADMIN_AUTH_TOKEN set, none of the launchd, systemd, or Windows service definitions propagate that variable; they persist only OPENCODEX_API_AUTH_TOKEN (src/service.ts:274-279, 329-331, and 908-917). The daemon therefore generates and accepts a different file-backed admin token, while CLI operations switched to this helper continue preferring the shell's configured token, causing management calls such as doctor, OAuth live updates, and graceful control to return 401. Persist the admin credential for services or otherwise ensure the daemon and clients select the same token.
Useful? React with 👍 / 👎.
| export function assertServerAuthConfig(config: OcxConfig): void { | ||
| if (isApiAuthRequired(config) && !configuredApiAuthToken(config)) { | ||
| throw new Error("OPENCODEX_API_AUTH_TOKEN is required when binding opencodex to a non-loopback hostname"); | ||
| const hasConfiguredDataCredential = !!configuredApiAuthToken(config) | ||
| || (config.apiKeys ?? []).some(entry => !!entry.key.trim()); | ||
| if (isApiAuthRequired(config) && !hasConfiguredDataCredential) { |
There was a problem hiding this comment.
For a non-loopback configuration that relies solely on config.apiKeys, the new server startup predicate accepts the credential, but assertServiceAuthEnvironment() in src/service.ts:244-250 still checks only OPENCODEX_API_AUTH_TOKEN and aborts ocx service install. This makes a configuration explicitly accepted by startServer() unusable as a background service unless the user adds an unnecessary second credential. Reuse the same data-plane credential predicate in the service installer.
Useful? React with 👍 / 👎.
| // Absolute cross-origin URLs must never get the local API token or 401 prompt. | ||
| if (url.origin !== window.location.origin) return false; | ||
| return url.pathname.startsWith("/api/") || url.pathname.startsWith("/v1/"); | ||
| return url.pathname.startsWith("/api/"); |
There was a problem hiding this comment.
On a non-loopback dashboard, this deliberately stops attaching the management token to /v1/*, but gui/src/pages/ApiKeys.tsx:86-95 still fetches /v1/models. That endpoint requires a separate data-plane credential remotely, and the dashboard neither stores nor sends one, so the API Access page always clears its model list and reports a load failure even after successful admin authentication. Fetch the catalog through the existing authenticated /api/models surface or add an equivalent management endpoint.
AGENTS.md reference: gui/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
| if (url.pathname.startsWith("/api/")) { | ||
| const apiAuthError = requireApiAuth(req, config, "management"); | ||
| if (apiAuthError) return withCors(apiAuthError, req, config); | ||
| const apiAuthError = requireManagementAuth(req, managementAuth, config); | ||
| if (apiAuthError) return withManagementCors(apiAuthError, req, config); |
There was a problem hiding this comment.
Every ocx opencode launch calls fetchOpencodeProxyModels() for /api/models using opencodeApiKey(), which can return only OPENCODEX_API_AUTH_TOKEN, the service data-token file, a configured data key, or the loopback placeholder. This new management gate rejects all of those credentials, so the catalog request returns 401 and cmdOpencode() exits before spawning OpenCode. Use the admin credential for the management catalog fetch while retaining the data credential for the injected OpenCode provider.
Useful? React with 👍 / 👎.
| const actual = req.headers.get("x-opencodex-api-key")?.trim() | ||
| || req.headers.get("authorization")?.replace(/^Bearer\s+/i, "").trim(); | ||
| if (actual && equalSecret(actual, state.token)) return null; |
There was a problem hiding this comment.
fetchCodexHealthFromLiveProxy() in src/oauth/health.ts:323-346 still authenticates /api/codex-auth/accounts with OPENCODEX_API_AUTH_TOKEN or service-api-token. Those are now data-plane-only credentials, so even a healthy running proxy returns 401 and ocx status/ocx doctor report Codex health as unavailable. Switch this remaining management client to configuredAdminToken().
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/storage-policy-job-responsive.test.ts (1)
239-259: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAllowlist tracked environment names before unsetting them.
At Line 252, this test cannot pass with
src/server/system-env.ts:371-380:revertSystemEnv()iterates everytracking.injectedKeysentry, includingUNRELATED_USER_SETTING, and callsunsetLaunchctlEnv()on it. A tampered tracking file can therefore remove unrelated launchctl environment variables.Filter/reject tracking entries against the canonical opencodex-owned environment-name allowlist before the unset loop; retain this regression test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/storage-policy-job-responsive.test.ts` around lines 239 - 259, Update revertSystemEnv() to filter tracking.injectedKeys against the canonical opencodex-owned environment-name allowlist before calling unsetLaunchctlEnv(). Ensure untrusted or unrelated tracked names are ignored while valid owned variables are still reverted, and retain the regression test in tests/storage-policy-job-responsive.test.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/help.ts`:
- Around line 33-44: Update the cleanup descriptions in the help entries for
remove and its related command to state that cleanup requires valid ownership
metadata, removes only manifest-listed paths, and leaves unowned files in place;
avoid claiming the metadata comes only from a fresh install.
In `@src/lib/config-ownership.ts`:
- Line 36: Update the append logic in the ownership-recording function around
the manifest path list to enforce MANIFEST_MAX_PATHS before adding a new path.
When the list has reached the ceiling, do not append and return false; otherwise
preserve the existing append behavior and success result.
- Around line 169-194: Update src/lib/config-ownership.ts:169-194 in
createOwnership to adopt non-empty legacy directories only when every entry is
in INITIAL_OWNED_PATHS or the two metadata filenames, while still refusing
unexpected entries; write both metadata files and avoid caching null results so
later adoption can succeed. The src/cli/index.ts:611-617 refusal branch requires
no direct change once adoption works; add a regression test covering uninstall
of a non-empty legacy directory and make residual-path guidance actionable
output if supported by the existing CLI flow.
- Around line 229-243: Make recordOwnedConfigPath non-throwing for all
filesystem and ownership operations, returning false when bookkeeping cannot be
completed. Extract the existing manifest-update tail into appendOwnedPath, and
retry ownership loading/creation once when a concurrent owner-file creation
fails with EEXIST so the losing process reuses the winner’s metadata instead of
caching null. Preserve the existing true result for already-recorded paths and
ensure failures do not propagate to callers.
- Around line 244-249: Update the manifest write flow around ownershipCache.set
to re-read the current on-disk manifest immediately before merging and writing,
rather than merging from the potentially stale ownershipCache snapshot. Preserve
existing paths recorded by other processes while adding rel, then update the
cache with the merged manifest; use the repository’s established locking
mechanism if needed to serialize concurrent read-modify-write operations.
In `@src/lib/destination-policy.ts`:
- Around line 272-288: Update the address validation loop around
validatedAddresses so a resolver result is accepted only when isIP(address)
returns 4 or 6; reject non-IP strings immediately instead of falling back to the
resolver-supplied family. Preserve the existing IPv4/IPv6 classification and
validated family assignment for parseable literals.
In `@src/lib/process-control.ts`:
- Line 70: Verify how resolveConfigDir/getConfigDir maps OPENCODEX_HOME before
constructing the token in the process-control flow. Update the
configuredAdminToken call to pass the resolved config directory rather than
env.OPENCODEX_HOME directly whenever resolution adds a suffix or normalization;
preserve the existing environment-token behavior.
In `@src/lib/provider-outbound.ts`:
- Around line 40-70: Update noProxyMatches to recognize CIDR entries in NO_PROXY
and match them against IP-literal hostnames, including IPv4 ranges such as
10.0.0.0/8 while preserving existing host, wildcard, and port matching. Ensure
providerOutboundGet no longer reports a missing NO_PROXY entry when the request
address is covered by a valid CIDR.
- Around line 153-158: Replace the plain Error thrown in the private-network
rejection branch of the outbound provider validation function with
ProviderOutboundPolicyError, preserving the existing actionable NO_PROXY message
and hostname. Ensure this rejection follows the same typed policy-error path as
the other policy checks so provider discovery classifies and surfaces it
correctly.
In `@src/lib/winsw.ts`:
- Line 137: Check the boolean result of recordOwnedConfigPath(getConfigDir(),
winswDir()) before proceeding with WinSW setup. If ownership registration fails,
stop the installation and surface the error; do not create winswDir() or
download the executable unless registration succeeds.
In `@src/server/gui-static.ts`:
- Around line 59-77: Update htmlResponse to HTML-escape session.token,
session.csrfToken, and session.origin before interpolating them into the meta
tag content attributes. Apply the escaping at this sink while preserving the
existing bootstrap structure and no-store response headers.
In `@src/server/index.ts`:
- Around line 740-743: Restrict the guiSessionCandidate condition in the request
handler to genuine document navigations that serve the SPA shell, rather than
every extensionless GET. Reuse the existing request headers/navigation checks
and the same GUI-route criteria used by serveGuiFile, ensuring issueGuiSession
is not called for arbitrary or missing-resource paths while preserving session
creation for the root and valid GUI document requests.
In `@src/server/management-auth.ts`:
- Around line 62-73: Update readExistingToken and initializeManagementAuthState
to classify invalid token content/shape (oversized, non-regular, symlink, or
regex mismatch) as a recoverable token error, regenerate the proxy-owned token
for that error and ENOENT, while continuing to fail closed on ACL-hardening and
other unsafe-path failures. Preserve the initialization failure reason in the
management auth state, and update requireManagementAuth to include that reason
in the 503 response so operators receive actionable guidance.
- Around line 27-28: Update the GUI session lifecycle around issueGuiSession and
requireManagementAuth so long-lived dashboards do not expire after the fixed
five-minute TTL. Add sliding renewal on each successfully authenticated
management request by extending the session expiry using GUI_SESSION_TTL_MS
before allowing the request, while preserving removeExpiredSessions cleanup and
existing authentication behavior.
In `@src/server/system-env.ts`:
- Around line 134-145: Update MANAGED_SYSTEM_ENV_NAMES to include
ANTHROPIC_MODEL, matching the key written and recorded by the system environment
injection flow so readTracking preserves it for cleanup. Keep the existing
allowlist filtering and other managed names unchanged.
In `@src/tray/windows.ts`:
- Line 480: Move the recordOwnedConfigPath call in the tray installation flow
until after the foreign-resource preflight checks succeed, and reject an
existing trayStatePath unless it represents the existing tray installation.
Ensure failed installations never add the path to the ownership manifest,
preventing removeOwnedConfigState from deleting unowned state.
In `@src/update/npm-invocation.mjs`:
- Around line 38-63: Update resolveNpmCommand to resolve npm through PATH on
POSIX as well as Windows, considering only absolute, non-empty PATH entries and
returning the resolved executable path; preserve the existing Windows extension
handling. Ensure relative or empty entries such as PATH=.:... cannot be
selected, and add regression coverage in the existing npm invocation tests for
both cases.
In `@tests/forward-admission-separation.test.ts`:
- Around line 91-97: Strengthen the “Images” case in the credential-rejection
table by using a supported image model that reaches the authentication gate, or
by asserting the response body/error code identifies an authentication error.
Update the case around the “Images” entry while preserving the existing request
structure and ensuring unrelated model-validation failures cannot satisfy the
test.
In `@tests/provider-live-models.test.ts`:
- Around line 174-199: Add model-cache cleanup to the redirect test’s finally
block by calling clearModelCache(PROVIDER) alongside warning.mockRestore().
Preserve the existing assertions and console warning cleanup, ensuring the
failure/discovery state recorded by gatherRoutedModels is removed after the
test.
In `@tests/provider-outbound.test.ts`:
- Around line 145-158: Make the spawned child environment in the “proxy mode
reaches one real proxy across outbound, connection-test, and model-discovery
paths” test deterministic by clearing HTTP_PROXY, HTTPS_PROXY, ALL_PROXY, and
NO_PROXY after spreading process.env. Preserve the fixture’s own proxy
configuration while preventing ambient proxy settings from affecting its routing
assertions.
In `@tests/server-auth.test.ts`:
- Around line 59-65: Remove the local managementHeaders helper from
server-auth.test.ts and import the shared helper from
tests/helpers/management-auth.ts. Preserve fail-fast behavior by adding an
explicit configuredAdminToken assertion at the specific test setup point that
requires initialization, while allowing managementHeaders to retain its shared
lenient and non-overwriting semantics.
- Line 613: Update the management request in the host-rebinding rejection test
to obtain the admin token through the existing fail-fast path used around lines
59-61, or reuse managementHeaders with the overridden host as shown near lines
671-674. Remove the configuredAdminToken() nullish sentinel so the test cannot
pass due to credential rejection instead of the host-rebinding guard.
In `@tests/server-management-auth.test.ts`:
- Around line 42-62: Shorten the fallback timer in websocketHandshakeOpens so it
expires before Bun’s default per-test timeout, then configure the affected
authentication test containing the five handshakes with an explicit timeout long
enough to cover all attempts. Preserve the helper’s false result on stalled
handshakes and the existing assertions for rejected and accepted credentials.
---
Outside diff comments:
In `@tests/storage-policy-job-responsive.test.ts`:
- Around line 239-259: Update revertSystemEnv() to filter tracking.injectedKeys
against the canonical opencodex-owned environment-name allowlist before calling
unsetLaunchctlEnv(). Ensure untrusted or unrelated tracked names are ignored
while valid owned variables are still reverted, and retain the regression test
in tests/storage-policy-job-responsive.test.ts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 90dcc582-5b56-498a-99c7-8c8f7c991f33
📒 Files selected for processing (117)
bin/ocx.mjsdocs-site/src/content/docs/reference/configuration.mdgui/src/api.tsgui/tests/api-auth-memory.test.tsgui/tests/models-empty-provider.test.tsxsrc/adapters/mimo-free.tssrc/cli/claude.tssrc/cli/doctor.tssrc/cli/help.tssrc/cli/index.tssrc/cli/star-prompt.tssrc/codex/catalog/provider-fetch.tssrc/codex/shim.tssrc/config.tssrc/images/artifacts.tssrc/lib/admin-secrets.tssrc/lib/config-ownership.tssrc/lib/crash-guard.tssrc/lib/destination-policy.tssrc/lib/pinned-http.tssrc/lib/process-control.tssrc/lib/provider-outbound.tssrc/lib/provider-url.tssrc/lib/proxy-env.tssrc/lib/winsw.tssrc/oauth/kimi.tssrc/oauth/login-cli.tssrc/oauth/store.tssrc/server/auth-cors.tssrc/server/gui-static.tssrc/server/index.tssrc/server/management-api.tssrc/server/management-auth.tssrc/server/management/oauth-account-routes.tssrc/server/management/provider-routes.tssrc/server/management/system-routes.tssrc/server/system-env.tssrc/service.tssrc/tray/windows.tssrc/update/index.tssrc/update/job.tssrc/update/npm-invocation.d.mtssrc/update/npm-invocation.mjssrc/usage/debug.tssrc/usage/log.tsstructure/02_config-and-codex-home.mdstructure/04_transports-and-sidecars.mdstructure/05_gui-and-management-api.mdtests/account-pool-management-api.test.tstests/api-debug.test.tstests/api-storage-cleanup.test.tstests/api-storage-policy.test.tstests/api-storage.test.tstests/api-usage.test.tstests/autostart-health.test.tstests/claude-desktop-config-path.test.tstests/claude-management-api.test.tstests/claude-messages-endpoint.test.tstests/claude-native-passthrough.test.tstests/claude-system-env-auto.test.tstests/cli-management-auth.test.tstests/codex-catalog.test.tstests/codex-sync-api.test.tstests/codex-v2-gate.test.tstests/combo-management-api.test.tstests/config-ownership-uninstall.test.tstests/config.test.tstests/cursor-hardening.test.tstests/destination-policy-resolved.test.tstests/effort-policy.test.tstests/fixtures/provider-outbound-e2e.tstests/forward-admission-separation.test.tstests/grok-management-api.test.tstests/gui-management-session.test.tstests/helpers/management-auth.tstests/injection-model-api.test.tstests/management-api-logs-metrics.test.tstests/management-provider-validation.test.tstests/memory-watchdog.test.tstests/model-visibility-management-api.test.tstests/native-model-toggle.test.tstests/oauth-accounts-api.test.tstests/oauth-login-cli-live-update.test.tstests/oauth-manual-code.test.tstests/oauth-public-surface.test.tstests/oauth-reauth-bind.test.tstests/ocx-launcher-source.test.tstests/openai-api-virtual-models.test.tstests/openai-provider-option-e2e.test.tstests/opencode-cli.test.tstests/process-control-graceful.test.tstests/project-config-warnings.test.tstests/provider-api-keys.test.tstests/provider-connection-test.test.tstests/provider-live-models.test.tstests/provider-outbound.test.tstests/server-403-permission-e2e.test.tstests/server-auth.test.tstests/server-clickjacking-headers.test.tstests/server-combo-failover-e2e.test.tstests/server-management-auth.test.tstests/settings-stream-mode.test.tstests/startup-action-control.test.tstests/storage-mutation-race.test.tstests/storage-policy-job-responsive.test.tstests/storage-restore-job-responsive.test.tstests/subagent-fallback-handle-responses.test.tstests/subagent-model-fallback-api.test.tstests/system-env.test.tstests/system-restart.test.tstests/uninstall.test.tstests/update-job.test.tstests/update-npm-invocation.test.tstests/update-stop-first.test.tstests/vision-anthropic.test.tstests/windows-deploy-close-regressions.test.tstests/windows-tray.test.ts
| details: [ | ||
| "Alias: ocx remove", | ||
| "Config cleanup requires ownership metadata created by a fresh install; legacy or shared directories are left in place.", | ||
| ], | ||
| }, | ||
| remove: { | ||
| usage: "ocx remove", | ||
| summary: "Remove service/shim/config and restore native Codex.", | ||
| details: ["Alias of: ocx uninstall"], | ||
| details: [ | ||
| "Alias of: ocx uninstall", | ||
| "Config cleanup requires ownership metadata created by a fresh install; legacy or shared directories are left in place.", | ||
| ], |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Describe the cleanup contract precisely.
The cleanup implementation requires valid ownership metadata and removes only manifest-listed paths; recordOwnedConfigPath can create that metadata lazily during managed writes, not only during a fresh install. The current wording may mislead users of upgraded or legacy configuration directories.
Use wording such as: “Config cleanup requires valid ownership metadata; only manifest-listed paths are removed, and unowned files remain.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli/help.ts` around lines 33 - 44, Update the cleanup descriptions in the
help entries for remove and its related command to state that cleanup requires
valid ownership metadata, removes only manifest-listed paths, and leaves unowned
files in place; avoid claiming the metadata comes only from a fresh install.
| }; | ||
|
|
||
| const METADATA_MAX_BYTES = 64 * 1024; | ||
| const MANIFEST_MAX_PATHS = 1024; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
MANIFEST_MAX_PATHS is enforced on read but not on append — the manifest can invalidate itself.
isManifest (Line 113) rejects manifests with more than 1024 paths, yet Lines 244-248 append without any bound. If the list ever crosses 1024 (e.g. a future call site recording per-file artifact paths), loadOwnership starts returning null and uninstall permanently refuses with "metadata is missing or invalid" — the same dead end as an unowned directory, but self-inflicted. Bound the append and return false at the ceiling.
♻️ Proposed bound on append
if (ownership.manifest.paths.includes(rel)) return true;
+ if (ownership.manifest.paths.length >= MANIFEST_MAX_PATHS) return false;
const manifest = {Also applies to: 244-248
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/config-ownership.ts` at line 36, Update the append logic in the
ownership-recording function around the manifest path list to enforce
MANIFEST_MAX_PATHS before adding a new path. When the list has reached the
ceiling, do not append and return false; otherwise preserve the existing append
behavior and success result.
| function createOwnership(configDir: string): { owner: ConfigOwner; manifest: ConfigUninstallManifest } | null { | ||
| const rootStat = lstatSync(configDir); | ||
| if (!rootStat.isDirectory() || rootStat.isSymbolicLink() || readdirSync(configDir).length !== 0) return null; | ||
| const owner: ConfigOwner = { | ||
| version: 1, | ||
| ownerId: randomUUID(), | ||
| root: canonicalRoot(configDir), | ||
| }; | ||
| const manifest: ConfigUninstallManifest = { ...owner, paths: [...INITIAL_OWNED_PATHS] }; | ||
| writeFileSync(join(configDir, CONFIG_OWNER_FILE), `${JSON.stringify(owner, null, 2)}\n`, { | ||
| encoding: "utf8", | ||
| flag: "wx", | ||
| mode: 0o600, | ||
| }); | ||
| try { | ||
| writeFileSync(join(configDir, CONFIG_UNINSTALL_MANIFEST), `${JSON.stringify(manifest, null, 2)}\n`, { | ||
| encoding: "utf8", | ||
| flag: "wx", | ||
| mode: 0o600, | ||
| }); | ||
| } catch (error) { | ||
| try { unlinkSync(join(configDir, CONFIG_OWNER_FILE)); } catch { /* incomplete metadata fails closed */ } | ||
| throw error; | ||
| } | ||
| return { owner, manifest }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
Legacy config directories are never adopted, so ocx uninstall hard-fails for every pre-existing installation. The single root cause is createOwnership's requirement that the config dir be empty: an upgraded ~/.opencodex has files but no ownership metadata, so ownership can never be established, and the new uninstall path can only report refused.
src/lib/config-ownership.ts#L169-L194: drop the strictreaddirSync(configDir).length !== 0bail-out and adopt a directory whose entries are all inINITIAL_OWNED_PATHS(plus the two metadata files), writing owner + manifest at that point; keep refusing when unexpected entries are present. Also avoid caching thenullresult so a later successful adoption is not blocked for the process lifetime.src/cli/index.ts#L611-L617: this branch is the user-visible symptom — with no adoption path it always takes thethrowat Line 617 (refused uninstall: config ownership metadata is missing or invalid.) and exits 1 where the previousrmSyncsucceeded. Once adoption lands, add a regression test alongsidetests/config-ownership-uninstall.test.tscovering uninstall of a non-empty legacy config dir, and consider printing the residual-path guidance as actionable output rather than only inside the thrown message.
📍 Affects 2 files
src/lib/config-ownership.ts#L169-L194(this comment)src/cli/index.ts#L611-L617
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/config-ownership.ts` around lines 169 - 194, Update
src/lib/config-ownership.ts:169-194 in createOwnership to adopt non-empty legacy
directories only when every entry is in INITIAL_OWNED_PATHS or the two metadata
filenames, while still refusing unexpected entries; write both metadata files
and avoid caching null results so later adoption can succeed. The
src/cli/index.ts:611-617 refusal branch requires no direct change once adoption
works; add a regression test covering uninstall of a non-empty legacy directory
and make residual-path guidance actionable output if supported by the existing
CLI flow.
Source: Path instructions
| export function recordOwnedConfigPath(configDir: string, candidatePath: string): boolean { | ||
| const rel = manifestRelativePath(configDir, candidatePath); | ||
| if (!rel) return false; | ||
| const cacheKey = ownershipCacheKey(configDir); | ||
| if (!existsSync(configDir)) { | ||
| ownershipCache.delete(cacheKey); | ||
| mkdirSync(configDir, { recursive: true, mode: 0o700 }); | ||
| } | ||
| let ownership = ownershipCache.get(cacheKey); | ||
| if (ownership === undefined) { | ||
| ownership = loadOwnership(configDir) ?? createOwnership(configDir); | ||
| ownershipCache.set(cacheKey, ownership); | ||
| } | ||
| if (!ownership) return false; | ||
| if (ownership.manifest.paths.includes(rel)) return true; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
recordOwnedConfigPath can throw, but several call sites treat it as best-effort bookkeeping on hot/critical paths.
The function performs mkdirSync (Line 235), lstatSync/realpathSync.native (via createOwnership/canonicalRoot), writeFileSync(..., flag: "wx") (Line 178) and writeManifest (Line 248) — all of which can throw: EEXIST when two processes create the owner file concurrently, EACCES/EROFS on a read-only or foreign-owned config dir, ENOENT if the dir disappears between Line 233 and lstatSync. Ownership bookkeeping is metadata, not the caller's job:
src/usage/log.tsLine 321 →ensureUsageLogDir()is called fromappendUsageEntrywith no try/catch, so a manifest write failure now breaks usage recording on the request path.src/images/artifacts.tsLines 245 and 367 → an unguarded throw turns a successful image download into a hard failure.src/service.tsLines 150/259/1118/1155/1191/1330 → aborts a service install midway.
Make the recorder non-throwing (and retry once on the concurrent-create race so a losing racer picks up the winner's metadata instead of caching null):
🛡️ Proposed fail-soft recorder
export function recordOwnedConfigPath(configDir: string, candidatePath: string): boolean {
- const rel = manifestRelativePath(configDir, candidatePath);
- if (!rel) return false;
- const cacheKey = ownershipCacheKey(configDir);
- if (!existsSync(configDir)) {
- ownershipCache.delete(cacheKey);
- mkdirSync(configDir, { recursive: true, mode: 0o700 });
- }
- let ownership = ownershipCache.get(cacheKey);
- if (ownership === undefined) {
- ownership = loadOwnership(configDir) ?? createOwnership(configDir);
- ownershipCache.set(cacheKey, ownership);
- }
+ try {
+ const rel = manifestRelativePath(configDir, candidatePath);
+ if (!rel) return false;
+ const cacheKey = ownershipCacheKey(configDir);
+ if (!existsSync(configDir)) {
+ ownershipCache.delete(cacheKey);
+ mkdirSync(configDir, { recursive: true, mode: 0o700 });
+ }
+ let ownership = ownershipCache.get(cacheKey);
+ if (ownership === undefined) {
+ try {
+ ownership = loadOwnership(configDir) ?? createOwnership(configDir);
+ } catch {
+ // lost a concurrent create race — re-read the winner's metadata
+ ownership = loadOwnership(configDir);
+ }
+ ownershipCache.set(cacheKey, ownership);
+ }
+ return appendOwnedPath(configDir, cacheKey, ownership, rel);
+ } catch {
+ return false;
+ }
+}…with the existing tail (Lines 242-250) moved into appendOwnedPath.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/config-ownership.ts` around lines 229 - 243, Make
recordOwnedConfigPath non-throwing for all filesystem and ownership operations,
returning false when bookkeeping cannot be completed. Extract the existing
manifest-update tail into appendOwnedPath, and retry ownership loading/creation
once when a concurrent owner-file creation fails with EEXIST so the losing
process reuses the winner’s metadata instead of caching null. Preserve the
existing true result for already-recorded paths and ensure failures do not
propagate to callers.
| const manifest = { | ||
| ...ownership.manifest, | ||
| paths: [...ownership.manifest.paths, rel].sort(), | ||
| }; | ||
| writeManifest(configDir, manifest); | ||
| ownershipCache.set(cacheKey, { owner: ownership.owner, manifest }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Stale-cache manifest writes silently drop paths recorded by other processes.
ownershipCache holds a per-process snapshot of the manifest, and Lines 244-248 rewrite the whole file from that snapshot via renameSync (last writer wins). opencodex routinely has several processes touching the same config dir (server, tray, CLI, update job). Sequence:
- Server caches
manifest = {paths: [...INITIAL]}. - Tray process records
tray-heartbeat.json→ file now contains it. - Server records a new path from its stale snapshot → the rename replaces the file and
tray-heartbeat.jsonis gone from the manifest.
The lost entry is never re-added (the tray's own cache says it is already present, Line 243), so removeOwnedConfigState leaves it behind and returns status: "partial" → uninstall fails with residual paths. Re-read the on-disk manifest immediately before merging instead of trusting the cache:
🔀 Proposed merge-before-write
- if (ownership.manifest.paths.includes(rel)) return true;
- const manifest = {
- ...ownership.manifest,
- paths: [...ownership.manifest.paths, rel].sort(),
- };
+ if (ownership.manifest.paths.includes(rel)) return true;
+ // Re-read so concurrently recorded paths from other processes are preserved.
+ const current = loadOwnership(configDir)?.manifest ?? ownership.manifest;
+ if (current.paths.includes(rel)) {
+ ownershipCache.set(cacheKey, { owner: ownership.owner, manifest: current });
+ return true;
+ }
+ const manifest = {
+ ...current,
+ paths: [...current.paths, rel].sort(),
+ };A file lock (the repo already uses auth.store.lock-style locks) would be the fully robust version if you expect real contention.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const manifest = { | |
| ...ownership.manifest, | |
| paths: [...ownership.manifest.paths, rel].sort(), | |
| }; | |
| writeManifest(configDir, manifest); | |
| ownershipCache.set(cacheKey, { owner: ownership.owner, manifest }); | |
| if (ownership.manifest.paths.includes(rel)) return true; | |
| // Re-read so concurrently recorded paths from other processes are preserved. | |
| const current = loadOwnership(configDir)?.manifest ?? ownership.manifest; | |
| if (current.paths.includes(rel)) { | |
| ownershipCache.set(cacheKey, { owner: ownership.owner, manifest: current }); | |
| return true; | |
| } | |
| const manifest = { | |
| ...current, | |
| paths: [...current.paths, rel].sort(), | |
| }; | |
| writeManifest(configDir, manifest); | |
| ownershipCache.set(cacheKey, { owner: ownership.owner, manifest }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/config-ownership.ts` around lines 244 - 249, Update the manifest
write flow around ownershipCache.set to re-read the current on-disk manifest
immediately before merging and writing, rather than merging from the potentially
stale ownershipCache snapshot. Preserve existing paths recorded by other
processes while adding rel, then update the cache with the merged manifest; use
the repository’s established locking mechanism if needed to serialize concurrent
read-modify-write operations.
| test("redirect responses log a credential-safe final-URL hint and fall back", async () => { | ||
| const warning = spyOn(console, "warn").mockImplementation(() => {}); | ||
| const redirectTarget = new URL("https://final.example/v1/models?token=secret#fragment"); | ||
| redirectTarget.username = "user"; | ||
| redirectTarget.password = "password"; | ||
| globalThis.fetch = (async () => new Response(null, { | ||
| status: 302, | ||
| headers: { | ||
| location: redirectTarget.toString(), | ||
| }, | ||
| })) as typeof fetch; | ||
|
|
||
| try { | ||
| const models = await gatherRoutedModels(config()); | ||
| const ids = models.filter(model => model.provider === PROVIDER).map(model => model.id); | ||
| const warningText = warning.mock.calls.flat().join(" "); | ||
|
|
||
| expect(ids.sort()).toEqual(["grok-4.3", "grok-4.5"]); | ||
| expect(warningText).toContain("returned 302 redirect"); | ||
| expect(warningText).toContain("https://final.example/v1/models"); | ||
| expect(warningText).not.toContain("user:password"); | ||
| expect(warningText).not.toContain("token=secret"); | ||
| } finally { | ||
| warning.mockRestore(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Asymmetric cache cleanup: this test leaves discovery state for PROVIDER behind while its sibling clears it.
The link-local test at Lines 209-227 wraps its assertions in try { … } finally { clearModelCache(providerName) }, but this redirect test's finally (Lines 196-198) only restores the console.warn spy. gatherRoutedModels → fetchProviderModels records failure/discovery state per provider (failureAt, discoveryStatus in src/codex/model-cache.ts), so the 302 failure recorded for xai-live-test can persist into later tests in this file that reuse PROVIDER and silently mask a real live-discovery call (order-dependent flake). Unless a suite-level beforeEach/afterEach already resets it, add clearModelCache(PROVIDER) to the finally.
♻️ Suggested cleanup
} finally {
warning.mockRestore();
+ clearModelCache(PROVIDER);
}#!/bin/bash
# Does this suite already reset model-cache/fetch state between tests?
rg -nP -C4 'beforeEach|afterEach|clearModelCache|originalFetch' tests/provider-live-models.test.ts🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/provider-live-models.test.ts` around lines 174 - 199, Add model-cache
cleanup to the redirect test’s finally block by calling
clearModelCache(PROVIDER) alongside warning.mockRestore(). Preserve the existing
assertions and console warning cleanup, ensuring the failure/discovery state
recorded by gatherRoutedModels is removed after the test.
| test("proxy mode reaches one real proxy across outbound, connection-test, and model-discovery paths", async () => { | ||
| const childHome = mkdtempSync(join(tmpdir(), "ocx-provider-proxy-e2e-")); | ||
| const child = Bun.spawn([ | ||
| process.execPath, | ||
| "tests/fixtures/provider-outbound-e2e.ts", | ||
| ], { | ||
| cwd: process.cwd(), | ||
| env: { | ||
| ...process.env, | ||
| OPENCODEX_HOME: childHome, | ||
| }, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The spawned fixture inherits the developer's/CI's ambient proxy env, which can invalidate the routing assertions below.
Lines 152-155 spread ...process.env into the child. Unlike the in-process tests (Lines 48, 94, 121), nothing strips HTTP_PROXY/HTTPS_PROXY/ALL_PROXY/NO_PROXY here, so a machine or CI runner that exports a corporate proxy (or a NO_PROXY=*) changes which requests the fixture routes through its own stub proxy — breaking the exact-order assertions at Lines 198-204 with a confusing "proxyRequests mismatch" rather than a clear failure. Make the child env deterministic by clearing the proxy keys the fixture is expected to set itself.
🛡️ Proposed hardening
const childHome = mkdtempSync(join(tmpdir(), "ocx-provider-proxy-e2e-"));
+ const cleanEnv = { ...process.env };
+ for (const key of proxyKeys) delete cleanEnv[key];
const child = Bun.spawn([
process.execPath,
"tests/fixtures/provider-outbound-e2e.ts",
], {
cwd: process.cwd(),
env: {
- ...process.env,
+ ...cleanEnv,
OPENCODEX_HOME: childHome,
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("proxy mode reaches one real proxy across outbound, connection-test, and model-discovery paths", async () => { | |
| const childHome = mkdtempSync(join(tmpdir(), "ocx-provider-proxy-e2e-")); | |
| const child = Bun.spawn([ | |
| process.execPath, | |
| "tests/fixtures/provider-outbound-e2e.ts", | |
| ], { | |
| cwd: process.cwd(), | |
| env: { | |
| ...process.env, | |
| OPENCODEX_HOME: childHome, | |
| }, | |
| stdout: "pipe", | |
| stderr: "pipe", | |
| }); | |
| test("proxy mode reaches one real proxy across outbound, connection-test, and model-discovery paths", async () => { | |
| const childHome = mkdtempSync(join(tmpdir(), "ocx-provider-proxy-e2e-")); | |
| const cleanEnv = { ...process.env }; | |
| for (const key of proxyKeys) delete cleanEnv[key]; | |
| const child = Bun.spawn([ | |
| process.execPath, | |
| "tests/fixtures/provider-outbound-e2e.ts", | |
| ], { | |
| cwd: process.cwd(), | |
| env: { | |
| ...cleanEnv, | |
| OPENCODEX_HOME: childHome, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/provider-outbound.test.ts` around lines 145 - 158, Make the spawned
child environment in the “proxy mode reaches one real proxy across outbound,
connection-test, and model-discovery paths” test deterministic by clearing
HTTP_PROXY, HTTPS_PROXY, ALL_PROXY, and NO_PROXY after spreading process.env.
Preserve the fixture’s own proxy configuration while preventing ambient proxy
settings from affecting its routing assertions.
| function managementHeaders(initial?: HeadersInit): Headers { | ||
| const token = configuredAdminToken(); | ||
| if (!token) throw new Error("management token was not initialized"); | ||
| const headers = new Headers(initial); | ||
| headers.set("x-opencodex-api-key", token); | ||
| return headers; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
This local managementHeaders duplicates tests/helpers/management-auth.ts:27-32 with divergent semantics.
The shared helper added in this same PR is lenient (if (token && !headers.has(...))), while this copy hard-throws on a missing token and unconditionally overwrites an existing x-opencodex-api-key. Two same-named helpers with different behavior across tests/ will drift the moment one is updated — and the overwrite semantics silently defeat any future test here that wants to send a deliberately wrong key. Import the shared helper and keep the fail-fast assertion explicit at the point where it matters.
♻️ Suggested consolidation
-function managementHeaders(initial?: HeadersInit): Headers {
- const token = configuredAdminToken();
- if (!token) throw new Error("management token was not initialized");
- const headers = new Headers(initial);
- headers.set("x-opencodex-api-key", token);
- return headers;
-}
+// tests/helpers/management-auth.ts owns the header shape; assert the token exists
+// so a missing admin token fails loudly instead of turning 200-expectations into 401s.
+function managementHeaders(initial?: HeadersInit): Headers {
+ if (!configuredAdminToken()) throw new Error("management token was not initialized");
+ return sharedManagementHeaders(initial);
+}plus the import:
+import { managementHeaders as sharedManagementHeaders } from "./helpers/management-auth";As per path instructions for tests/** ("Tests are flat Bun tests under tests/"), the shared tests/helpers/management-auth.ts module is the right home for this shape.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function managementHeaders(initial?: HeadersInit): Headers { | |
| const token = configuredAdminToken(); | |
| if (!token) throw new Error("management token was not initialized"); | |
| const headers = new Headers(initial); | |
| headers.set("x-opencodex-api-key", token); | |
| return headers; | |
| } | |
| import { managementHeaders as sharedManagementHeaders } from "./helpers/management-auth"; | |
| function managementHeaders(initial?: HeadersInit): Headers { | |
| if (!configuredAdminToken()) throw new Error("management token was not initialized"); | |
| return sharedManagementHeaders(initial); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/server-auth.test.ts` around lines 59 - 65, Remove the local
managementHeaders helper from server-auth.test.ts and import the shared helper
from tests/helpers/management-auth.ts. Preserve fail-fast behavior by adding an
explicit configuredAdminToken assertion at the specific test setup point that
requires initialization, while allowing managementHeaders to retain its shared
lenient and non-overwriting semantics.
Source: Path instructions
| headers: { | ||
| host: `attacker.test:${server.port}`, | ||
| origin: attackerOrigin, | ||
| "x-opencodex-api-key": configuredAdminToken() ?? "missing-admin-token", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The ?? "missing-admin-token" sentinel can make this rejection test pass for the wrong reason.
This test asserts that a management request with a rebound Host header is rejected. If configuredAdminToken() ever returns null (admin token file not yet generated, env var cleared by a preceding test), the request is sent with the bogus sentinel and gets rejected by the credential check instead of the host-rebinding guard — the assertion still goes green while the security control under test is never exercised. Lines 59-61 already chose fail-fast for exactly this reason; use the same path here.
🛡️ Proposed fix
- "x-opencodex-api-key": configuredAdminToken() ?? "missing-admin-token",
+ ...Object.fromEntries(managementHeaders()),(or pass headers: managementHeaders({ host: … }) as done at Lines 671-674)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "x-opencodex-api-key": configuredAdminToken() ?? "missing-admin-token", | |
| ...Object.fromEntries(managementHeaders()), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/server-auth.test.ts` at line 613, Update the management request in the
host-rebinding rejection test to obtain the admin token through the existing
fail-fast path used around lines 59-61, or reuse managementHeaders with the
overridden host as shown near lines 671-674. Remove the configuredAdminToken()
nullish sentinel so the test cannot pass due to credential rejection instead of
the host-rebinding guard.
| function websocketHandshakeOpens(url: URL, token: string): Promise<boolean> { | ||
| return new Promise(resolve => { | ||
| const target = new URL("/v1/responses", url); | ||
| target.protocol = target.protocol === "https:" ? "wss:" : "ws:"; | ||
| const socket = new WebSocket(target, { | ||
| headers: { "X-OpenCodex-API-Key": token }, | ||
| } as unknown as string[]); | ||
| let settled = false; | ||
| const finish = (opened: boolean) => { | ||
| if (settled) return; | ||
| settled = true; | ||
| clearTimeout(timer); | ||
| try { socket.close(); } catch { /* already closed */ } | ||
| resolve(opened); | ||
| }; | ||
| socket.addEventListener("open", () => finish(true)); | ||
| socket.addEventListener("error", () => finish(false)); | ||
| socket.addEventListener("close", () => finish(false)); | ||
| const timer = setTimeout(() => finish(false), 5_000); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
The handshake fallback timer (5 000 ms) equals Bun's default per-test timeout, so a slow handshake aborts the test instead of yielding a clean false.
websocketHandshakeOpens arms setTimeout(() => finish(false), 5_000) at Line 60, and the test at Lines 374-396 performs five handshakes (two rejected creds × handshake, plus the accepted data-secret one) with no explicit test(..., timeout) argument. Normally each rejection settles immediately on error/close, but if any single handshake stalls, the helper's own 5 s guard can never fire first — Bun's default 5 s test timeout wins and the failure surfaces as an opaque timeout rather than expect(...).toBe(false). Compare tests/provider-outbound.test.ts Line 209, which sets an explicit 15_000. Shorten the helper's guard and give this test headroom.
🛡️ Proposed fix
- const timer = setTimeout(() => finish(false), 5_000);
+ // Must stay well below the per-test timeout so a stalled handshake reports
+ // `false` instead of aborting the whole test.
+ const timer = setTimeout(() => finish(false), 2_000);- });
+ }, 20_000);(applied to the test closing at Line 396)
Also applies to: 374-396
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/server-management-auth.test.ts` around lines 42 - 62, Shorten the
fallback timer in websocketHandshakeOpens so it expires before Bun’s default
per-test timeout, then configure the affected authentication test containing the
five handshakes with an explicit timeout long enough to cover all attempts.
Preserve the helper’s false result on stalled handshakes and the existing
assertions for rejected and accepted credentials.
#697 moved provider model discovery onto the pinned outbound transport, which resolves and pins the peer address instead of reading `globalThis.fetch`. Catalog tests that stub the global therefore escaped their stub and tried real DNS, failing with DestinationDnsResolutionError — 25 failures across four files after the merge. `providerOutboundGet` already supports a caller-owned executor via `provider.fetch`; #697 used it in two spots. Extend that to the rest: a shared `withStubbedProviderFetch` helper attaches an executor that defers to whatever `globalThis.fetch` is installed at call time, so a test may still swap its stub after building the config. This changes test wiring only. Production discovery keeps the pinned transport with no injected executor, which is the security property #697 added. The suite also gets faster — codex-catalog.test.ts drops from ~33s to ~2.5s now that four cases no longer wait on real DNS timeouts.
The same #697 transport change that broke the root catalog tests also breaks four cases in gui/tests/models-empty-provider.test.tsx: they stub `globalThis.fetch`, discovery no longer reads it, and the calls hang on real DNS until the 5s test timeout. Reuse the `withStubbedProviderFetch` helper so these configs carry the caller-owned executor too. Test wiring only. This one hid from the root suite: `bun run test` covers `tests/`, while CI runs `cd gui && bun test tests` as a separate step, so the failure only appeared on the runner.
Promotes the current dev line to main. Highlights since v2.7.42: - Security hardening batch 1 (#697): management/data credential split, blank-hostname rejection, pinned provider discovery transports, clickjacking and websocket-origin defenses, and safer update/uninstall paths. - 31 bug fixes and 18 features across catalog, Windows service, Cursor bridge, Codex account routing, and the GUI. - Account-pool settings card no longer prints its title twice. BREAKING CHANGES: - A blank "hostname" is rejected on write and degraded to 127.0.0.1 on read with a warning. - Data-plane credentials (OPENCODEX_API_AUTH_TOKEN, service-api-token, config.apiKeys) no longer reach /api/*. Operational scripts must move to OPENCODEX_ADMIN_AUTH_TOKEN. Model clients are unaffected. - A non-loopback dashboard no longer mints session tokens.
2 participants
Summary
Hardens local proxy auth, config uninstall, service cleanup, and provider
outbound requests.
Closes multiple items from the private 2026-07-28 security review in full, and
two in part. Full finding IDs, severities, descriptions, and the list of
unfixed items stay in the private report and are not listed here.
All 14 security commits are scoped to one finding each; none touch unrelated
code. One additional merge-only commit,
7d995ef7, synchronizes the branch withthe current
devhead without rewriting those commits. Supporting test-onlycommits:
43b4f651,42bdb1f3,8e221b5d.Two of the items this PR touches are partially addressed and must not be
treated as closed. Remaining work is tracked privately.
Breaking changes
These describe the behavior after this PR. Operators upgrading will notice:
"hostname": ""is rejected on write and degraded to127.0.0.1on readwith a warning.
OPENCODEX_API_AUTH_TOKEN,service-api-token,config.apiKeys) no longer reach/api/*. Operational scripts must move toOPENCODEX_ADMIN_AUTH_TOKEN. Model clients are unaffected.prompts for the management token. The loopback dashboard keeps a seamless
same-origin session.
manifest refuse to delete, retain their files, and exit non-zero.
npmcannot be resolved from an absolutePATH entry outside the launch directory.
NO_PROXY; otherwise the request is refused with an actionable message.localhost,127.0.0.1,::1, and[::1]are already excludedautomatically.
the sanitized target and asks the operator to configure the final URL.
Outbound provider connection test and model discovery now use a two-path
transport: without a proxy, connections are pinned to a pre-validated address
while SNI,
Host, and certificate validation are preserved; with a proxy, Bun'snative fetch keeps
HTTP_PROXY/HTTPS_PROXY/ALL_PROXY/NO_PROXYsemantics rather than silently connecting direct.
Verification
All of the following ran on Windows only.
bun run test— 5955 passed, 4 skipped, 0 failed, 429 files, 834.37s, exit 0bun run typecheck— exit 0bun run privacy:scan— exit 0HTTP_PROXY,HTTPS_PROXY, andALL_PROXYindividually; no silent direct connectThe first full-suite attempt after the
devmerge ended with a Bun 1.3.14internal assertion (exit 3), not a test assertion. A later complete run on the
same code tree produced the successful result above.
Never executed on a real host. CI will be the first three-platform run for:
launchctlcleanup (execFileSync("/bin/launchctl", …),5eafd362)node:httpspinned transport on non-WindowsAlso not verified: enterprise proxies, authenticated proxies, PAC, and
HTTPS CONNECTtunnels.NO_PROXYCIDR entries are not supported and this isdocumented rather than implemented.
Checklist
Published user documentation still needs to cover every breaking change from
the earlier batches, especially blank-hostname degradation and the operational
migration to the independent management credential.