From b65c470ced458fab028b5e2ff29bcbd49ad82679 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 22:25:02 +0900 Subject: [PATCH 1/6] docs(devlog): plan the #1102 loopback listener after five audit rounds The design changed twice under audit. The first proposal was an opt-in switch that would have admitted loopback socket peers on a remote bind; it rode resolveApiAuth into eight endpoints unrelated to #1102, and a public listener's peer address only proves the last transport hop, which Docker Desktop, host-network containers, WSL mirrored networking and tunnels all terminate locally. The shipped design leaves public admission untouched and opens a separate 127.0.0.1-bound listener, so the kernel refuses remote connections instead of us judging addresses. Later rounds closed the implementation contracts: a fixed port (an ephemeral one would manufacture the restart breakage the issue claimed and we disproved), a per-request auth/CORS policy view narrow enough that it cannot masquerade as business config, one startup transaction across both binds, composite stop that completes cleanup AND propagates failure, and GET /v1/models on the allowlist because Codex falls back to it when no catalog is installed. The last blocker was the sharpest: the /v1/models ablation would not have gone red, because the models-manager catches refresh failures and returns its bundled list. The acceptance test now turns on a runtime-generated unique routed model that no bundled catalog can synthesize. --- .../100_loopback_peer_admission.md | 378 ++++++++++++++++++ 1 file changed, 378 insertions(+) create mode 100644 devlog/_plan/260807_untouched_bug_stack/100_loopback_peer_admission.md diff --git a/devlog/_plan/260807_untouched_bug_stack/100_loopback_peer_admission.md b/devlog/_plan/260807_untouched_bug_stack/100_loopback_peer_admission.md new file mode 100644 index 000000000..08c71baf1 --- /dev/null +++ b/devlog/_plan/260807_untouched_bug_stack/100_loopback_peer_admission.md @@ -0,0 +1,378 @@ +# 100 — #1102: `0.0.0.0` 바인드에서 로컬 Codex 가 401 로 막힌다 + +> **개정 이력.** 첫 판은 "opt-in 으로 loopback 소켓 피어를 무인증 admit" 을 +> 제안했다. 독립 감사가 P1 다섯 건으로 되돌렸고, 그중 둘이 설계를 바꿨다: +> (a) 그 스위치는 `resolveApiAuth` 를 타고 #1102 와 무관한 8개 엔드포인트까지 +> 열고, (b) 공용 리스너의 피어 주소는 최종 사용자 신원이 아니다. 아래는 +> 재설계된 판이다. +> +> **2차 개정.** 재설계본도 감사에서 P1 세 건을 받았다. 설계 방향은 유지됐지만 +> 구현 계약이 비어 있었다: ephemeral 포트가 재시작마다 바뀌면 우리가 부정했던 +> "재시작 후 app-server 가 깨진다" 를 우리 손으로 만들고, 로컬 리스너의 +> auth/origin/WS 처리 경계가 미정이며, 두 bind 가 하나의 트랜잭션이 아니었다. +> 아래 §고정 포트 / §리스너 정책 / §바인드 트랜잭션 이 그 답이다. +> +> **3차 개정.** 세 번째 감사가 P1 둘을 더 찾았고 둘 다 검증된 사실이다: +> 카탈로그가 없을 때 app-server 가 `GET /v1/models` 로 폴백하는데 우리 +> allowlist 가 그걸 404 로 막고, `allSettled` 만으로는 stop 실패가 삼켜져 +> 재시작이 아직 포트를 쥔 리스너 위에 바인드를 시도한다. + +## 이슈가 말한 것과 실제 + +리포터는 두 개의 트리거를 보고했다. 하나는 정확했고, 하나는 원인이 다르다. + +**맞음 — direct-spawn 갭.** `app-server` 는 shim 의 `CODEX_INTERNAL_COMMANDS` +(`src/codex/shim.ts:42`) 에 있고 shim 은 디스패치 전에 토큰을 export 한다 +(`:384-389`). 그러니 shim 을 거친 `codex app-server` 는 인증된다. 문제는 서드파티 +호스트가 `require.resolve('@openai/codex/bin/codex.js')` 로 엔트리포인트를 직접 +resolve 해서 spawn 할 때다. 그 경로는 shim 을 통째로 우회하고, 대안이 없다: +`/v1/responses` admission 은 `x-opencodex-api-key` 만 받고 +(`src/server/auth-cors.ts:369-376`), 토큰 파일은 admission 시점에 읽히지 않는다. + +**틀림 — "재시작하면 토큰이 회전된다".** `writeServiceApiTokenFile()` 은 이미 +`process.env.OPENCODEX_API_AUTH_TOKEN` 에 있는 값을 쓰고, 없으면 아무것도 쓰지 +않는다 (`src/service.ts:347`). 호출자는 service install/repair 뿐이고 +(`:1707`, `:1799`, `:1937`, `:2116`), `ocx service start` 는 파일을 다시 쓰지 +않는다 (`:2660`). 토큰은 애초에 사용자가 공급하는 값이고 OpenCodex 가 생성하지 +않는다. 그러니 평범한 재시작이 살아 있는 app-server 를 무효화하지 않는다. + +이 정정은 이미 이슈에 코멘트로 게시되어 있고, 리포터에게 두 가지를 물었다. +답은 아직 없다. + +## 왜 파일 기반 대안이 전부 막히는가 + +토큰을 shim 밖 프로세스에 "실제로 전달" 하려면 그 프로세스의 환경을 바꿔야 +하는데, OS 프로세스 환경은 spawn 시 복사되고 우리는 남의 프로세스 환경을 사후에 +못 바꾼다. 남는 후보를 전부 확인했다: + +| 후보 | 왜 안 되는가 | +|---|---| +| Codex `env_http_headers` 를 파일 기반으로 | 값이 **환경변수 이름**이다. 업스트림 설계이고 우리 쪽 변경 범위 밖 | +| static `http_headers` | 시크릿을 `~/.codex/config.toml` 에 평문으로 직렬화한다. 백업·저널·동기화 경로로 퍼진다 | +| `auth.command` | bearer credential 을 공급하는데, `/v1/responses` 는 전용 헤더만 받는다. Codex Direct 와 충돌 방지를 위한 의도적 거부 (`auth-cors.ts:369-372`) | +| OS 전역 환경 주입 | 무관한 GUI/터미널 자식까지 credential 을 상속한다. 이미 떠 있는 호스트에는 적용도 안 된다 | + +전부 막힌다. 그래서 이건 credential **전달** 문제가 아니라 admission **정책** +문제다. + +## 첫 설계가 왜 틀렸나 + +처음에는 `isApiAuthRequired()` 를 우회하는 opt-in 스위치 +(`trustLoopbackPeersOnRemoteBind`) 를 제안했다. 감사가 두 가지를 지적했고 둘 다 +코드로 확인된다. + +**하나 — 폭발 반경.** `resolveApiAuth` 는 8곳에서 호출된다 +(`src/server/index.ts:692, 882, 903, 937, 1009, 1024, 1087, 1121`): `/v1/models`, +Images generations/edits, artifacts, alpha search, Messages, Live/Realtime, +sideband WebSocket. `resolveResponsesApiAuth` 도 `/v1/responses` 만이 아니라 +compact 와 Chat Completions 경로에서 쓰인다. resolver 안에 피어 예외를 넣으면 +#1102 가 요청하지 않은 표면 전부가 같이 열린다. 수용 기준이 +`/v1/responses` 만 검사했으므로 이 확대를 탐지하지도 못했을 것이다. + +**둘 — 피어 주소가 증명하는 것.** `requestIP()` 는 **마지막 transport hop** 만 +알려준다. Docker Desktop 의 포트 포워딩, `--network host` 컨테이너, WSL2 의 +mirrored networking 과 `netsh portproxy`, Kubernetes sidecar, VPN/터널 종단 — +전부 원격 연결을 로컬 TCP 연결로 다시 연다. 그 배포에서는 원격 호출자가 +loopback 피어로 보인다. 흔한 구성이고, 첫 판은 리버스 프록시와 SSH 터널만 +예시로 들어 이 계열을 과소평가했다. + +"opt-in 이니까 괜찮다" 로는 부족하다. 켜는 사람이 자기 배포가 저 목록에 +해당하는지 모를 수 있다. + +## 재설계 — 인증을 우회하지 않고, 별도 리스너를 연다 + +감사가 제시한 대안이 더 낫다. 공용 리스너의 admission 정책은 **한 줄도** 바꾸지 +않는다. 대신 `127.0.0.1` 에만 바인드된 **두 번째 리스너**를 옵션으로 연다. + +``` +0.0.0.0:10100 ← 기존 리스너. 인증 정책 불변. 모든 원격 호출자는 키가 필요하다. +127.0.0.1:PORT ← 새 리스너. 커널이 원격 연결을 아예 받지 않는다. +``` + +차이가 핵심이다. 첫 설계는 "원격에서 온 연결인데 로컬처럼 보이면 통과" 였다. +이 설계는 **커널이 원격 연결을 애초에 accept 하지 않는다.** 판정할 주소가 없고, +속일 피어 필드도 없다. Docker 포트 포워딩도 `127.0.0.1` 바인드는 기본적으로 +호스트 밖으로 내보내지 못한다. + +주입되는 Codex provider block 은 이미 wildcard 바인드에서 `base_url` 을 +`127.0.0.1` 로 쓴다 (`tests/codex-inject.test.ts:47-54`). 그 URL 의 포트만 로컬 +리스너로 바꾸면 shim 을 우회해 직접 spawn 된 app-server 도 인증 없이 붙는다 — +**공용 리스너의 경계는 한 줄도 건드리지 않고.** (넓히는 것이 없다는 뜻은 +아니다 — 명시적인 로컬 신뢰 표면이 하나 추가된다. 아래 §여전히 opt-in 인 +이유 참조.) + +### 여전히 opt-in 인 이유 + +`127.0.0.1` 바인드라도 그 머신의 **모든 로컬 프로세스**가 접근할 수 있다. +단일 사용자 워크스테이션에서는 받아들일 만하고, 멀티테넌트 호스트에서는 아니다. +그래서 기본값은 꺼짐이고, 이름은 결과가 드러나게 짓는다: +`unauthenticatedLoopbackListener`. + +더 정확히 말하면, 이 설계는 **보안 경계를 넓히지 않는** 것이 아니라 +**공용 리스너의 경계를 그대로 두고 명시적인 로컬 신뢰 표면을 하나 추가하는** +것이다. 그 표면에서 무인증 로컬 프로세스는 active-turn capacity, 계정 풀 쿼터, +유료 provider credential 을 소비할 수 있다 — 즉 인증된 원격 클라이언트를 굶길 +수 있다. 문서 경고는 "모든 로컬 프로세스가 접근 가능" 에서 멈추지 않고 이 +비용·DoS 측면까지 적는다. + +## 고정 포트 — ephemeral 은 우리가 부정한 버그를 우리가 만든다 + +첫 재설계본은 포트 미지정 시 OS 할당을 허용했다. 그건 틀렸다. + +`ocx sync` 와 startup sync 는 공용 `port` 만 `injectCodexConfig()` 에 넘긴다 +(`src/codex/sync.ts:100`, `src/cli/index.ts:353`). 로컬 리스너의 실제 포트를 +발견할 경로가 없다. 그리고 ephemeral 포트는 재시작마다 바뀔 수 있는데, +`config.toml` 이 새 포트로 다시 쓰여도 **이미 실행 중인 app-server 는 시작 시 +읽은 옛 `base_url` 을 계속 쓴다.** + +그 실패 모드를 그대로 읽어보면 — "재시작하면 이미 떠 있는 app-server 가 깨진다" +— 이 이슈가 신고했고 우리가 코드로 부정한 바로 그 증상이다. 원인이 토큰 회전이 +아니었을 뿐이고, ephemeral 포트로는 진짜로 만들어낸다. + +**포트는 설정에 필수로 둔다.** 오프라인 `ocx sync`, 재시작, 이미 실행 중인 +app-server 가 전부 같은 값을 본다. 활성화 시 포트를 안 주면 config 검증이 +거부한다. + +## 리스너 정책 — 무엇을 어떻게 다르게 취급하는가 + +로컬 리스너는 같은 프로세스, 같은 라우팅, 같은 계정 풀을 쓴다. 다른 것은 두 +가지뿐이다. + +**1. auth/origin 판정용 config view.** 같은 `config` 객체를 그대로 넘기면 +`hostname` 이 `"0.0.0.0"` 이라 `resolveResponsesApiAuth()` 가 여전히 인증을 +요구한다. 그렇다고 config 전체를 `{...config, hostname:"127.0.0.1"}` 로 복제해 +오래 들고 있으면 management 로 설정을 바꿨을 때 로컬 리스너가 낡은 값을 쓴다. + +그래서 **비즈니스/라우팅은 canonical config 를 공유하고, auth 와 origin 판정에만 +매 요청 만든 view 를 넘긴다.** + +**resolver 시그니처는 바꾸지 않는다.** `resolveResponsesApiAuth(req, config)` 에 +`allowUnauthenticated` 같은 파라미터를 추가하면 공용 리스너에서도 호출 가능한 +admission 우회 스위치가 생긴다. 정책 선택은 resolver 밖, 리스너 클로저에서 +한다. + +view 를 받는 함수는 이것들 전부다 — 하나라도 빠뜨리면 그 지점만 공용 정책으로 +판정한다: + +- `resolveResponsesApiAuth` +- `isAllowedRequestOrigin` +- `withCors`, `corsHeaders` +- `jsonResponse` — `/v1/models` 의 성공 응답이 이걸 통과하며 내부에서 CORS + 헤더를 만든다 (`src/server/auth-cors.ts:187-191`). 빠뜨리면 그 경로만 공용 + 정책으로 헤더를 붙인다 +- 에러 응답 헬퍼 (CORS 헤더를 붙이는 것들) + +모델 수집과 응답 내용 구성에는 계속 canonical config 를 넘긴다 — view 는 오직 +auth/CORS 판정용이다. + +view 타입은 `Pick` 수준으로 +좁힌다. 완전한 비즈니스 config 로 위장할 수 없어야 실수로 라우팅 경로에 흘러도 +타입에서 걸린다. + +**2. origin 게이트는 반드시 적용한다.** 인증만 우회하고 origin 검사에 공용 +config 를 넘기면 `isAllowedRequestOrigin` 의 remote 분기를 타서 +`isSameOriginAsRequest()` 로 허용될 수 있다 (`src/server/auth-cors.ts:76-82`). +공격자 서버가 피해자 브라우저로 `127.0.0.1` 에 붙는 DNS rebinding 이 정확히 그 +모양이다 — 커널 관점에서는 정상 로컬 연결이다. 로컬 리스너는 loopback 분기를 +타야 하고, 그 분기는 `Host` 헤더까지 검사한다. + +커널 바인드와 Host/Origin 게이트가 **함께** 경계다. 바인드만으로는 브라우저를 +경유한 접근을 막지 못한다. + +**3. WebSocket upgrade 는 그 요청을 받은 서버로.** 현재 Responses WS 는 클로저 +바깥의 primary `server.upgrade()` 를 부른다 (`src/server/index.ts:621`). 그대로 +공유하면 로컬 리스너가 받은 Request 를 primary 서버에서 upgrade 하려 든다. +반드시 해당 fetch 호출의 `requestServer.upgrade()` 를 쓴다. + +### 라우트 allowlist + +"data-plane 만" 은 너무 넓었다. 정확히 고정한다: + +- `POST /v1/responses` +- `/v1/responses` WebSocket upgrade +- `POST /v1/responses/compact` + +- `GET /v1/models` + +`/v1/models` 를 넣는 이유는 증거가 나왔기 때문이다. `syncCodex` 는 카탈로그 +생성이 실패하거나 소스가 없으면 경고만 남기고 `catalogPath: null` 로 +`injectCodexConfig()` 를 부른다 (`src/codex/sync.ts:129-156`). 그러면 Codex 는 +static catalog 매니저 대신 online 매니저를 고르고, app-server 의 `model/list` 가 +`GET {base_url}/models` 로 나간다. 우리가 404 를 주면 모델 목록이 낡은 채로 +남거나 번들 캐시로 떨어진다. 정확히 direct-spawn 호스트를 고치겠다면서 그 +호스트의 모델 목록을 깨뜨리는 셈이다. + +대안은 카탈로그 설치 실패 시 활성화를 fail-closed 로 막는 것인데, 카탈로그 +없음은 이미 경고로 관용되는 상태다. 그걸 이 옵션 때문에 에러로 승격시키는 건 +범위를 넘는다. + +나머지 — Chat Completions, Messages, Images, search, artifacts, Live/Realtime, +`/api/*`, GUI, health/readiness — 는 404. + +## 바인드 트랜잭션 + +config 검증으로 두 포트가 다른지 보는 것만으로는 부족하다. 로컬 포트를 다른 +프로세스가 이미 잡고 있을 수 있다. + +두 bind 를 **하나의 startup 트랜잭션**으로 다룬다. 어느 쪽이 실패하든 이미 열린 +리스너를 `await stop(true)` 로 닫고 원래 오류를 다시 던진다. 그렇지 않으면 +primary 만 살아남고, CLI 의 기존 포트 재시도가 이걸 공용 포트 충돌로 오인해 +다른 포트를 고르면서 리스너를 누적한다 (`src/cli/index.ts:234`). + +로컬 포트 충돌과 공용 포트 충돌은 구분한다. 로컬 충돌 때문에 공용 포트를 바꾸지 +않는다. + +합성 `stop()` 은 두 가지를 **동시에** 만족해야 한다. 한쪽만 하면 다른 쪽이 +깨진다. + +1. **정리는 끝까지 시도한다.** 한쪽 stop 이 실패해도 나머지 stop 과 native + lifecycle release 를 건너뛰지 않는다. +2. **실패는 호출자에게 전파한다.** `allSettled` 로 삼키면 안 된다. + +2번이 중요한 이유: 기존 `stopServerListener` 는 stop 실패를 의도적으로 +전파하고, 모든 호출자가 같은 결과를 본 뒤에야 교체 프로세스가 포트를 잡게 +되어 있다 (`src/server/lifecycle.ts:290-305`). 삼키면 `drainAndShutdown` 이 +종료 완료로 오인하고, 아직 포트를 쥔 리스너 위에 교체가 바인드를 시도한다. +정리는 다 했는데 실패는 보고되는 상태여야 하므로, 결과를 모아 하나라도 +실패했으면 `AggregateError` 로 reject 한다. + +### 이 설계가 P1 다섯 건에 어떻게 답하는가 + +| 감사 P1 | 재설계에서 | +|---|---| +| 피어 주소는 최종 신원이 아니다 | 피어 주소를 아예 판정하지 않는다. 커널 바인드 + Host/Origin 게이트가 경계다 | +| 8개 무관 엔드포인트가 같이 열린다 | 공용 리스너 정책 불변. 로컬 리스너는 4개 라우트만 노출하고 나머지는 404 | +| 새 admission kind 의 로그 파급 | `{ kind: "loopback" }` 재사용 — 이미 존재하는 kind 이고 의미도 정확하다 (인증 없는 로컬 바인드). 새 kind 없음 | +| 문자열 모양 주소 판정 | 판정 함수 자체가 없다 | +| 수용 기준이 실제 경로를 증명 못 함 | 실제 리스너를 띄우고 원격 인터페이스에서 연결 거부를 확인한다 | + +`admissionKind` 를 새로 늘리지 않는 것이 특히 크다. 감사가 지적한 대로 +`RequestLogContext`, `RequestLogEntry`, `PersistedUsageEntry` 가 전부 세 kind 로 +고정돼 있고 (`src/server/request-log.ts:52,119`, `src/usage/log.ts:56`), +`KNOWN_ADMISSION_KINDS` 가 모르는 값을 조용히 버린다 (`src/usage/log.ts:115`). +새 kind 는 타입체크를 깨거나 감사 로그에서 사라진다. + +## 변경 파일 + +- `src/types.ts` — `unauthenticatedLoopbackListener?: { enabled: false } | { enabled: true; port: number }` + (판별 유니온: 꺼져 있을 때 포트를 요구하지 않는다) +- `src/config.ts` — 스키마 + 검증 (포트 필수, 공용 포트와 동일 거부) +- `src/server/index.ts` — 두 번째 `Bun.serve`, 바인드 트랜잭션, 합성 stop, + 라우트 allowlist, 요청별 auth/origin view, `requestServer.upgrade()` +- `src/codex/inject.ts` — 켜져 있으면 `base_url` 이 로컬 리스너 포트를 가리킴 +- `src/codex/sync.ts`, `src/cli/index.ts` — 로컬 포트를 주입 경로로 전달 +- `src/cli/index.ts` — 실효 공용 포트 검증, 폴백 선택에서 로컬 포트 제외 +- `docs-site/` — 설정 문서 + 로컬 접근·비용·DoS 경고 +- `tests/` — 아래 기준 + +## 수용 기준 + +1. 설정 없음 → 리스너가 하나뿐. `0.0.0.0` 동작은 오늘과 동일 (401 유지). +2. 설정 켬 → `127.0.0.1:PORT` 로 키 없이 `POST /v1/responses` 가 admit 되고 + `{ kind: "loopback" }` 로 기록된다. 실제 WS upgrade 와 + `POST /v1/responses/compact` 도 같다. +3. 설정 켬 → 공용 리스너는 **여전히** 키를 요구한다. +4. 설정 켬 → 로컬 리스너가 비-loopback 인터페이스에 바인드되지 않는다. 머신의 + non-loopback 주소로 실제 연결을 시도해 거부를 확인한다. non-loopback + 인터페이스가 없어 skip 되면 기준 14 의 첫 ablation 이 green 이 되므로, 지원 + OS 에서는 skip 없이 돌거나 별도 결정적 보조 검사를 둔다. +5. allowlist 밖 라우트는 로컬 리스너에서 404: Chat Completions, Messages, + Images, search, artifacts, Live/Realtime, `/api/*`, GUI, health/readiness 각 + 대표 하나씩. +6. 적대적 `Host`/`Origin` (DNS rebinding 형태) 은 로컬 리스너에서도 거부된다. + 거부만이 아니라 **반환되는 CORS 헤더도** 로컬 정책 view 로 만들어졌는지 + 확인한다 — 라우팅 전 origin 판정만 보면 응답 헤더 경로의 누락을 놓친다. + 성공 응답도 확인한다: 로컬 `/v1/models` 200 응답의 CORS 헤더가 로컬 view 로 + 만들어졌는지. +7. 주입되는 `base_url` 이 설정된 로컬 포트를 가리키고, 재시작 후에도, 독립 + `ocx sync` 실행 후에도 같은 값이다. +8. 포트 필수: 활성화하면서 포트를 생략하거나 공용 포트와 같게 주면 config + 검증이 거부한다. +9. 로컬 포트를 다른 소켓이 이미 점유한 상태로 기동하면 startup 이 실패하고 + **두 포트 모두** 다시 바인드 가능한 상태로 남는다 (rollback). +10. `server.stop(true)` 와 `drainAndShutdown()` 양쪽에서 두 리스너가 모두 + 닫힌다. 한쪽 stop 이 실패해도 다른 쪽 stop 과 lifecycle release 가 실행된다. + **그리고 호출자는 reject 를 관측한다** — 정리 완주와 실패 전파 둘 다. +11. 실제 direct-spawn 수용 테스트: 격리된 `CODEX_HOME` 으로 app-server 를 + 띄우고 `model/list` 를 부르고 턴을 하나 돌려서, 요청이 로컬 리스너에 + `loopback` 으로 도달하는지 확인한다. **카탈로그 있음과 없음 두 경로 모두.** + 라우트에 POST 를 날려보는 것만으로는 리포터가 신고한 통합이 동작한다는 + 증명이 되지 않는다. + + **오라클이 없으면 이 기준은 공허하다.** Codex 의 models-manager 는 refresh + 실패를 catch 하고 기존 번들/캐시 목록을 반환한다. 그러니 `/v1/models` 를 + allowlist 에서 빼도 `model/list` 는 여전히 성공하고, 번들 모델로 턴을 + 돌리면 그것도 성공한다 — 기준이 green 인 채로 호환 경로가 깨진다. 이 + 저장소가 반복해서 데인 "통과만 하는 테스트" 의 교과서적 형태다. + + 카탈로그 없음 경로는 **오직 우리 라우트를 통해서만 알 수 있는 모델**로 + 판정한다: + + - Codex 의 번들 카탈로그와 캐시에 존재할 수 없는 고유 이름의 routed 모델을 + 구성한다. 이름은 **런타임에 생성**한다 — + `ocx-direct-spawn-${crypto.randomUUID()}` 형태. 하드코딩한 이름은 언젠가 + 누군가의 카탈로그와 충돌할 수 있고, 그 순간 오라클이 조용히 죽는다. + - `model/list` 응답에 **그 이름이 정확히** 들어 있는지 단언한다. + - 턴도 **그 모델로** 돌리고, 의도한 가짜 업스트림에 도달하는지 확인한다. + - 격리된 `CODEX_HOME` 은 `models_cache.json` 없이 시작한다. 기동 **전에** + `models_cache.json` 부재와 활성 `model_catalog_json` 부재를 단언한다 — + 전제가 깨진 채로 도는 테스트는 오라클이 아니다. + + 실행 경로도 고정한다. PATH 의 `codex` 가 아니라 resolve 된 + `@openai/codex/bin/codex.js` 를 직접 띄우고, 자식 환경에서 + `OPENCODEX_API_AUTH_TOKEN` 을 제거한다. 그러지 않으면 shim 인증 경로를 + 실수로 타면서 아무것도 증명하지 못한다. + + 증명은 둘로 나눈다. 이 저장소는 `@openai/codex` 를 테스트 의존성으로 설치하지 + 않으므로, CI 에서 결정적으로 도는 부분과 실기동 증거를 구분한다: + + - **CI 결정적:** 로컬 리스너의 `/v1/models?client_version=...` 라우트가 + 고유 모델을 반환하는지, allowlist 에서 빼면 404 가 되는지. + - **활성화 증거 (skip 금지):** 실제 지원 버전의 Codex app-server 로 위 + 시퀀스를 돌린 기록. 스킵된 채로는 이 기준을 충족한 것으로 치지 않는다. +12. 실효 공용 포트 충돌: `ocx start --port <로컬포트>`, `config.port = 0` 이 + 로컬 포트로 해석되는 경우, 그리고 선호 포트가 막혀 `findAvailablePort()` 의 + ephemeral 폴백이 로컬 포트를 고르는 경우 — 전부 startup 이 실패하는 대신 + 로컬 포트를 후보에서 제외해야 한다 (`src/cli/index.ts:146-180`). +13. 활성화 시 시작 로그에 눈에 띄는 경고가 나온다: `127.0.0.1:PORT`, 무인증 + 로컬 접근, 유료 credential 소비, 로컬 DoS 위험. +14. ablation: + - 로컬 리스너 hostname 을 `0.0.0.0` 으로 바꾸면 기준 4 가 red. + - `inject.ts` 포트 배선을 되돌리면 기준 7 이 red. + - origin 판정에 공용 config 를 넘기면 기준 6 이 red. + - rollback 을 제거하면 기준 9 가 red. + - 합성 stop 이 한쪽 reject 를 삼키게 하면 기준 10 이 red. + - `/v1/models` 를 allowlist 에서 빼면 기준 11 의 카탈로그 없음 경로가 red. + +## 상태 — 완결이 아니라 완화책 + +감사의 마지막 P1 을 그대로 받는다. 기본값이 꺼짐이므로, 리포터가 이 옵션을 +수용하기 전까지 원래 재현은 여전히 401 이다. 그래서 이 유닛은 **#1102 를 close +하지 않는다.** PR 은 `Closes` 대신 이슈를 참조하고, 리포터에게 이 옵션이 +배포에 맞는지 묻는 코멘트를 남긴다. + +--- + +## 부록 — 첫 설계의 원 분석 (기록용) + +`isApiAuthRequired()` 는 오직 바인드 hostname 만 본다: + +```ts +export function isApiAuthRequired(config: OcxConfig): boolean { + return !isLoopbackHostname(config.hostname); +} +``` + +`hostname: "0.0.0.0"` 이면 요청 피어가 `127.0.0.1` 이어도 인증을 요구한다. +그런데 우리가 Codex 에 주입하는 provider block 은 wildcard 바인드에서 +`base_url` 을 `127.0.0.1` 로 쓴다 (`tests/codex-inject.test.ts:47-54`). 즉 +우리가 만들어낸 구성이 정확히 이 상황을 만든다. + +이 진단 자체는 유효하고 재설계도 같은 사실 위에 서 있다. 다만 해법이 +"admission 을 우회" 에서 "별도 리스너" 로 바뀌었다. + +## 범위 밖 + +토큰 grace window 와 `service rotate-api-token` 은 별개 유닛이다. 리포터가 보고한 +회전 트리거는 원인이 다르다고 확인됐고 (자동 회전이 없음), operator 가 직접 값을 +바꾸고 install/repair 한 경우만 남는데 그건 이 이슈가 신고한 것이 아니다. From 7e3f5b9c968bf231f3e6e21b1923cdaa4795f4eb Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 22:49:53 +0900 Subject: [PATCH 2/6] feat(server): optional unauthenticated loopback listener for direct-spawn Codex (#1102) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `codex app-server` launched by a host that resolves the entrypoint directly never passes through the generated shim, so it never inherits OPENCODEX_API_AUTH_TOKEN. On a wildcard bind every model call then 401s at admission, before any SSE frame. The tempting fix — exempt callers whose socket peer looks like loopback — is unsound. `requestIP()` proves only the last transport hop, and Docker Desktop port forwarding, host-network containers, WSL mirrored networking and tunnel terminators all re-open remote connections locally. It would also have ridden `resolveApiAuth` into eight endpoints unrelated to this issue. Instead a second listener binds 127.0.0.1. The kernel refuses remote connections outright, so there is no address to judge, and the public listener's admission policy is byte-for-byte unchanged. The parts that are load-bearing: Auth and CORS read a `RequestPolicyView` — a Pick of hostname, corsAllowOrigins and apiKeys — chosen per request from the receiving listener. Rewriting the whole config and holding it would go stale on the next management change; adding an `allowUnauthenticated` parameter to the resolvers would put an admission bypass on the wrong side of the boundary. The narrow type also means a policy view that leaks into a routing path fails to typecheck. The bind is loopback but the boundary is not the bind alone: an attacker page can make a victim's browser connect to 127.0.0.1. The listener therefore takes the same Host/Origin branch a plain loopback bind always has. A test asserts the same hostile Origin that the loopback policy rejects would be accepted under the public policy. The port is required in config, never OS-assigned. An ephemeral port would change across restarts while running app-servers kept the old base_url — the exact symptom this issue reported for token rotation, which does not actually happen. `GET /v1/models` is on the four-route allowlist because when catalog materialization fails, `syncCodex` injects with `catalogPath: null` and Codex falls back to an online model manager that refreshes through it. Both binds are one startup transaction, and composite stop completes cleanup on both listeners while still propagating failure — swallowing it would let drainAndShutdown report success while a socket is held. Off by default. When on, every local process can spend account quota and paid provider credentials, which the startup warning and the docs say plainly. Ablation: reverting the policy view to the shared config makes 3 tests red including the hostile-Host case; dropping the auth-header rule makes 1 red; removing the port validator makes 2 red; rewriting the public bind to a literal 127.0.0.1 makes the F4 symmetry guard red. Refs #1102 --- .../docs/reference/configuration/server.md | 38 +++ src/codex/inject.ts | 18 +- src/config.ts | 42 ++- src/server/auth-cors.ts | 57 +++- src/server/index.ts | 288 +++++++++++++----- src/types.ts | 24 ++ tests/loopback-listener-admission.test.ts | 168 ++++++++++ .../windows-deploy-close-regressions.test.ts | 14 +- 8 files changed, 550 insertions(+), 99 deletions(-) create mode 100644 tests/loopback-listener-admission.test.ts diff --git a/docs-site/src/content/docs/reference/configuration/server.md b/docs-site/src/content/docs/reference/configuration/server.md index bc72f17b3..942af88cc 100644 --- a/docs-site/src/content/docs/reference/configuration/server.md +++ b/docs-site/src/content/docs/reference/configuration/server.md @@ -65,6 +65,44 @@ A `0.0.0.0` bind exposes the proxy and configured provider access to the LAN. Us networks with a strong token. ::: +### Local clients that cannot receive the token + +A remote bind requires a credential from every caller, including local ones. That breaks a specific +case: a `codex app-server` launched by a host process that resolves the Codex entrypoint directly +(`require.resolve('@openai/codex/bin/codex.js')`) never passes through the generated `codex` shim, +so it never inherits `OPENCODEX_API_AUTH_TOKEN` and every model call fails with `401` before a +stream opens. + +`unauthenticatedLoopbackListener` opens a second listener bound to `127.0.0.1` that admits without a +credential. The main listener is untouched — remote callers still need the token. + +```json +{ + "hostname": "0.0.0.0", + "port": 10100, + "unauthenticatedLoopbackListener": { "enabled": true, "port": 10200 } +} +``` + +`ocx sync` then writes `base_url = "http://127.0.0.1:10200/v1"` into the managed Codex provider block +and omits the auth header, so a directly spawned app-server works without any credential plumbing. + +The port is required and must differ from the proxy port. It is never OS-assigned: an ephemeral port +would change across restarts while already-running app-servers kept the previous `base_url`. + +The listener serves only `POST /v1/responses`, its WebSocket upgrade, `POST /v1/responses/compact`, +and `GET /v1/models`. Everything else, including `/api/*` and the dashboard, returns `404`. + +:::danger[This is an unauthenticated surface] +Every process on the machine can use this listener. It spends account quota and paid provider +credentials, and it can exhaust the shared turn capacity that authenticated remote clients depend +on. Do not enable it on a shared or multi-tenant host. + +Binding to `127.0.0.1` means the kernel refuses remote connections, but it does not stop a browser: +a page you visit can make your browser connect to `127.0.0.1`. The listener therefore applies the +same `Host` and `Origin` checks as an ordinary loopback bind. Off by default. +::: + ### SSH port forwarding Remote use does not require a remote bind. Keep loopback and forward it: diff --git a/src/codex/inject.ts b/src/codex/inject.ts index 62ff9f220..20d570870 100644 --- a/src/codex/inject.ts +++ b/src/codex/inject.ts @@ -189,8 +189,13 @@ export function providerBaseHost(hostname: string | undefined): string { } export function shouldInjectApiAuthHeader( - config: Pick | undefined, + config: Pick | undefined, ): boolean { + // The unauthenticated loopback listener is a loopback bind, so it admits without a + // credential (#1102). Emitting the env header anyway would be worse than useless: the + // directly-spawned app-server this exists for has no OPENCODEX_API_AUTH_TOKEN in its + // environment, and Codex would send an empty header value. + if (config?.unauthenticatedLoopbackListener?.enabled) return false; return !isLoopbackHostname(config?.hostname); } @@ -630,6 +635,17 @@ export async function injectCodexConfig( config?: OcxConfig, options: InjectCodexOptions = {}, ): Promise { + // Point Codex at the unauthenticated loopback listener when it is enabled (#1102). + // + // Resolved here rather than at the call sites because every caller already passes the proxy + // port and the config together: startup sync, `ocx sync`, and the ensure path would each + // need the same two-line change, and a caller that missed it would silently emit a base_url + // requiring a credential the directly-spawned app-server does not have. + // + // The listener port is fixed in config, never OS-assigned, so this value survives restarts + // and matches what an already-running app-server read at startup. + const loopback = config?.unauthenticatedLoopbackListener; + if (loopback?.enabled) port = loopback.port; if (!existsSync(CODEX_CONFIG_PATH)) { return { success: false, diff --git a/src/config.ts b/src/config.ts index 717b11aea..b929f074c 100644 --- a/src/config.ts +++ b/src/config.ts @@ -1035,6 +1035,14 @@ const configSchema = z.object({ // is safe: startServer() already falls back to 127.0.0.1 for a missing hostname. Write-time // rejection lives in validateConfigCandidate() so bad values still surface to the caller. hostname: z.string().trim().min(1).optional().catch(undefined), + // Discriminated on `enabled` so a disabled entry cannot be forced to carry a port, and an + // enabled one cannot omit it (#1102). A malformed value degrades to undefined rather than + // failing the whole parse: this is an opt-in convenience surface, and a hand-edit typo here + // must never reset providers/apiKeys through the backup-and-defaults repair path. + unauthenticatedLoopbackListener: z.union([ + z.object({ enabled: z.literal(false) }), + z.object({ enabled: z.literal(true), port: z.number().int().min(1).max(65535) }), + ]).optional().catch(undefined), providers: z.record(z.string(), providerConfigSchema), defaultProvider: z.string().min(1).default("openai"), openaiProviderTierVersion: z.union([z.literal(1), z.literal(2)]).optional(), @@ -1974,13 +1982,45 @@ function codexAccountPickerEnabledError(value: unknown): string | null { } /** Validate an in-memory config candidate without touching disk. Used by headless CLI import/set. */ +/** + * Reject a loopback-listener port that collides with the proxy port (#1102). + * + * The schema can only check the shape of each field on its own; the two ports being distinct + * is a relationship between them. Letting the pair through would surface as a startup failure + * after the public listener already bound, which reads like an unrelated port conflict. + * + * This is write-time only, matching `blankHostnameError`: a live caller can be told the value + * is wrong, whereas a hand-edited config on the read path degrades to undefined rather than + * resetting the whole file. + */ +function loopbackListenerPortError(value: unknown): string | null { + if (!value || typeof value !== "object" || Array.isArray(value)) return null; + const listener = (value as Record).unauthenticatedLoopbackListener; + if (listener === undefined) return null; + if (!listener || typeof listener !== "object" || Array.isArray(listener)) { + return "schema_invalid: unauthenticatedLoopbackListener: must be an object or omitted"; + } + const entry = listener as Record; + if (entry.enabled !== true) return null; + const listenerPort = entry.port; + if (typeof listenerPort !== "number" || !Number.isInteger(listenerPort) || listenerPort < 1 || listenerPort > 65535) { + return "schema_invalid: unauthenticatedLoopbackListener.port: must be an integer port when enabled"; + } + const proxyPort = (value as Record).port; + if (typeof proxyPort === "number" && proxyPort === listenerPort) { + return "schema_invalid: unauthenticatedLoopbackListener.port: must differ from the proxy port"; + } + return null; +} + export function validateConfigCandidate(value: unknown): { ok: true; config: OcxConfig } | { ok: false; error: string } { const boundaryError = blankHostnameError(value) ?? claudeSubagentEffortError(value) ?? appOwnedMemoryBudgetError(value) ?? googleAntigravityStaticCatalogVersionError(value) ?? codexAccountPrioritiesError(value) - ?? codexAccountPickerEnabledError(value); + ?? codexAccountPickerEnabledError(value) + ?? loopbackListenerPortError(value); if (boundaryError) return { ok: false, error: boundaryError }; const result = configSchema.safeParse(value); if (result.success) return { ok: true, config: normalizeApiKeyIds(result.data as OcxConfig) }; diff --git a/src/server/auth-cors.ts b/src/server/auth-cors.ts index e439ceecb..536526f3e 100644 --- a/src/server/auth-cors.ts +++ b/src/server/auth-cors.ts @@ -73,7 +73,7 @@ export function isSameOriginAsRequest(req: Request, origin: string): boolean { } } -export function isAllowedRequestOrigin(req: Request, config: OcxConfig): boolean { +export function isAllowedRequestOrigin(req: Request, config: RequestPolicyView): boolean { const origin = req.headers.get("Origin"); if (!isApiAuthRequired(config)) { if (!isLoopbackRequestHost(req.headers.get("Host"))) return false; @@ -82,7 +82,7 @@ export function isAllowedRequestOrigin(req: Request, config: OcxConfig): boolean return !origin || isLoopbackOriginValue(origin) || isSameOriginAsRequest(req, origin) || isExtraAllowedOrigin(origin, config); } -function isExtraAllowedOrigin(origin: string, cfg: OcxConfig): boolean { +function isExtraAllowedOrigin(origin: string, cfg: RequestPolicyView): boolean { if (!cfg.corsAllowOrigins?.length) return false; const parsedOrigin = comparableOrigin(origin); return cfg.corsAllowOrigins.some(allowed => { @@ -136,7 +136,7 @@ export function browserSecurityHeaders(): Record { }; } -export function corsHeaders(req?: Request, config?: OcxConfig): Record { +export function corsHeaders(req?: Request, config?: RequestPolicyView): Record { const origin = req?.headers.get("Origin"); const allowOrigin = origin && req && config && isAllowedRequestOrigin(req, config) ? origin : _corsOrigin; return { @@ -160,7 +160,7 @@ export function managementCorsHeaders(req?: Request, config?: OcxConfig): Record return headers; } -export function withCors(response: Response, req: Request, config: OcxConfig): Response { +export function withCors(response: Response, req: Request, config: RequestPolicyView): Response { const headers = new Headers(response.headers); for (const [name, value] of Object.entries(corsHeaders(req, config))) { headers.set(name, value); @@ -184,14 +184,18 @@ export function withManagementCors(response: Response, req: Request, config: Ocx }); } -export function jsonResponse(data: unknown, status = 200, req?: Request, config?: OcxConfig): Response { +export function jsonResponse(data: unknown, status = 200, req?: Request, config?: RequestPolicyView): Response { return new Response(JSON.stringify(data), { status, headers: { "Content-Type": "application/json", ...corsHeaders(req, config) }, }); } -export function configuredApiAuthToken(_config: OcxConfig): string | undefined { +// The parameter is vestigial — the token has always come from the environment — but callers +// pass a config, so keep accepting one. Typed as `unknown` rather than `OcxConfig` so a narrow +// policy view can reach it too (#1102); widening to OcxConfig here would force every caller in +// the admission path back to the full config. +export function configuredApiAuthToken(_config?: unknown): string | undefined { const token = process.env.OPENCODEX_API_AUTH_TOKEN?.trim(); return token || undefined; } @@ -208,10 +212,37 @@ export function isLoopbackHostname(hostname: string | undefined): boolean { return normalized === "" || normalized === "localhost" || normalized === "127.0.0.1" || normalized === "::1" || normalized === "[::1]"; } -export function isApiAuthRequired(config: OcxConfig): boolean { +export function isApiAuthRequired(config: Pick): boolean { return !isLoopbackHostname(config.hostname); } +/** + * The slice of config that decides admission and CORS, and nothing else (#1102). + * + * The unauthenticated loopback listener shares this process with the public one: same routing, + * same account pool, same drain. The only thing it must see differently is its own bind + * address, because `isApiAuthRequired` reads `hostname` and the shared config says "0.0.0.0". + * + * Two ways to express that were rejected. Passing the whole config with `hostname` rewritten + * and holding it for the listener's lifetime would go stale the moment the management API + * changes a setting. Adding an `allowUnauthenticated` parameter to the resolvers would create a + * callable admission bypass that the PUBLIC listener could also reach — the switch would exist + * on the wrong side of the boundary. + * + * So this type is deliberately narrow: it cannot masquerade as a business config, and a policy + * view that leaks into a routing path fails to typecheck rather than silently taking effect. + */ +export type RequestPolicyView = Pick; + +/** Derive the per-request policy view for a listener. Cheap enough to build per request. */ +export function requestPolicyView(config: OcxConfig, bindHostname: string): RequestPolicyView { + return { + hostname: bindHostname, + ...(config.corsAllowOrigins ? { corsAllowOrigins: config.corsAllowOrigins } : {}), + ...(config.apiKeys ? { apiKeys: config.apiKeys } : {}), + }; +} + export function assertServerAuthConfig(config: OcxConfig): void { const hasConfiguredDataCredential = !!configuredApiAuthToken(config) || (config.apiKeys ?? []).some(entry => !!entry.key.trim()); @@ -253,7 +284,7 @@ export type DataPlaneAdmission = * discarded, which is what makes per-key attribution possible without touching * the admission decision itself. */ -export function resolveDataPlaneAdmissionSecret(token: string, config: OcxConfig): DataPlaneAdmission | null { +export function resolveDataPlaneAdmissionSecret(token: string, config: Pick): DataPlaneAdmission | null { const actual = token.trim(); if (!actual) return null; if (secretEquals(actual, configuredApiAuthToken(config))) return { kind: "environment" }; @@ -341,7 +372,7 @@ export function validateForwardAdmissionCredential(headers: Headers, config: Ocx * Resolving form of `hasValidApiAuth`: identical header precedence, identical * decision, but it names the admission instead of collapsing it to a boolean. */ -export function resolveApiAuth(req: Request, config: OcxConfig): DataPlaneAdmission | null { +export function resolveApiAuth(req: Request, config: RequestPolicyView): DataPlaneAdmission | null { // A loopback bind never reads a token at all, so there is no key to name. if (!isApiAuthRequired(config)) return { kind: "loopback" }; const actual = req.headers.get("x-opencodex-api-key")?.trim() @@ -352,11 +383,11 @@ export function resolveApiAuth(req: Request, config: OcxConfig): DataPlaneAdmiss return resolveDataPlaneAdmissionSecret(actual, config); } -export function hasValidApiAuth(req: Request, config: OcxConfig): boolean { +export function hasValidApiAuth(req: Request, config: RequestPolicyView): boolean { return resolveApiAuth(req, config) !== null; } -export function requireApiAuth(req: Request, config: OcxConfig, _kind: "data-plane"): Response | null { +export function requireApiAuth(req: Request, config: RequestPolicyView, _kind: "data-plane"): Response | null { if (hasValidApiAuth(req, config)) return null; return formatErrorResponse(401, "authentication_error", "opencodex API key required"); } @@ -366,7 +397,7 @@ export function requireApiAuth(req: Request, config: OcxConfig, _kind: "data-pla * Codex Direct. Remote binds must use the dedicated proxy header so the two bearer * domains can never be confused. */ -export function resolveResponsesApiAuth(req: Request, config: OcxConfig): DataPlaneAdmission | null { +export function resolveResponsesApiAuth(req: Request, config: RequestPolicyView): DataPlaneAdmission | null { if (!isApiAuthRequired(config)) return { kind: "loopback" }; // Dedicated header ONLY. `Authorization` on these transports may belong to // Codex Direct passthrough, and the two bearer domains must stay unconfusable. @@ -375,7 +406,7 @@ export function resolveResponsesApiAuth(req: Request, config: OcxConfig): DataPl return resolveDataPlaneAdmissionSecret(actual, config); } -export function requireResponsesApiAuth(req: Request, config: OcxConfig): Response | null { +export function requireResponsesApiAuth(req: Request, config: RequestPolicyView): Response | null { if (resolveResponsesApiAuth(req, config)) return null; return formatErrorResponse(401, "authentication_error", "opencodex API key required"); } diff --git a/src/server/index.ts b/src/server/index.ts index b25f4ce67..83793c560 100644 --- a/src/server/index.ts +++ b/src/server/index.ts @@ -138,6 +138,8 @@ import { admissionFields, resolveApiAuth, resolveResponsesApiAuth, + requestPolicyView, + type RequestPolicyView, safeConfigDTO, setCorsOrigin, withCors, @@ -492,6 +494,49 @@ export function startServer(port?: number, deps: StartServerDeps = {}): Server config; + const loopbackPolicy = (): RequestPolicyView => requestPolicyView(config, "127.0.0.1"); + void publicPolicy; + + /** + * Routes the unauthenticated loopback listener will serve. Everything else 404s. + * + * This is an allowlist rather than a filter applied to the public handler, because a filter + * inverts the failure mode: a route added later would be reachable here by default. The four + * entries are exactly what a directly-spawned `codex app-server` needs. + * + * `GET /v1/models` is on the list for a reason that is easy to miss. When catalog + * materialization fails or finds no source, `syncCodex` warns and injects with + * `catalogPath: null`; Codex then builds an ONLINE model manager and `model/list` refreshes + * through `GET {base_url}/models`. Returning 404 there would leave the picker on its bundled + * fallback — fixing the direct-spawn host while breaking its model list. + */ + function loopbackRouteAllowed(url: URL, req: Request): boolean { + const path = url.pathname; + if (path === "/v1/responses") { + return req.method === "POST" || req.headers.get("upgrade")?.toLowerCase() === "websocket"; + } + if (path === "/v1/responses/compact") return req.method === "POST"; + if (path === "/v1/models") return req.method === "GET"; + return false; + } + // Codex treats empty / non-JSON 503 bodies as "Unknown error" (#452). Keep Retry-After and // the server_is_overloaded code so clients can back off, but always return a JSON envelope. function drainingResponse(req: Request): Response { @@ -554,12 +599,27 @@ export function startServer(port?: number, deps: StartServerDeps = {}): Server {}, }; let server: Server; + let loopbackServer: Server | null = null; try { - server = Bun.serve({ - port: listenPort, - hostname: bindHost, + const serveOptions = { idleTimeout: 255, - async fetch(req, requestServer): Promise { + async fetch(req: Request, requestServer: Server): Promise { + // The unauthenticated loopback listener (#1102) serves a fixed allowlist and nothing + // else. Rejecting here, before any handler runs, is what keeps the surface from growing + // silently when a route is added below. + if (requestServer === loopbackServer && !loopbackRouteAllowed(new URL(req.url), req)) { + return withCors( + formatErrorResponse(404, "not_found", `Unknown endpoint: ${req.method} ${new URL(req.url).pathname}`), + req, + loopbackPolicy(), + ); + } + // Auth and CORS decisions below read `policy`, not `config`. For the public listener the + // two are the same object, so its behaviour is unchanged; for the loopback listener the + // view substitutes 127.0.0.1 as the bind address, which is what routes it through the + // same code path a plain loopback bind has always taken — Host-header check included. + // Routing, provider selection and response bodies keep using `config`. + const policy: RequestPolicyView = requestServer === loopbackServer ? loopbackPolicy() : config; const url = new URL(req.url); markActivity(`${req.method} ${url.pathname}`); @@ -580,18 +640,18 @@ export function startServer(port?: number, deps: StartServerDeps = {}): Server 0, ), - }, 200, req, config); + }, 200, req, policy); } // OpenAI list shape: native gpt bare + routed models namespaced "/" // (pure availability list — disabled natives are omitted entirely). @@ -835,7 +899,7 @@ export function startServer(port?: number, deps: StartServerDeps = {}): Server { const response = await handleImages(req, config, endpoint, logCtx, turnAdmissionLease); addFinalRequestLog(requestId, start, logCtx, response.status, response.status === 499 ? { closeReason: "client_cancel" } : undefined); - return withCors(response, req, config); + return withCors(response, req, policy); }); } if (req.method === "GET" && url.pathname.startsWith("/v1/opencodex/artifacts/")) { - const admission = resolveApiAuth(req, config); - if (!admission) return withCors(formatErrorResponse(401, "authentication_error", "opencodex API key required"), req, config); - if (!isAllowedRequestOrigin(req, config)) { - return withCors(formatErrorResponse(403, "origin_rejected", "cross-origin data-plane request blocked"), req, config); + const admission = resolveApiAuth(req, policy); + if (!admission) return withCors(formatErrorResponse(401, "authentication_error", "opencodex API key required"), req, policy); + if (!isAllowedRequestOrigin(req, policy)) { + return withCors(formatErrorResponse(403, "origin_rejected", "cross-origin data-plane request blocked"), req, policy); } const id = decodeURIComponent(url.pathname.slice("/v1/opencodex/artifacts/".length)); const { resolveArtifactPath } = await import("../images/artifacts"); const artifactPath = resolveArtifactPath(id); if (!artifactPath) { - return withCors(formatErrorResponse(404, "not_found", "artifact not found"), req, config); + return withCors(formatErrorResponse(404, "not_found", "artifact not found"), req, policy); } const file = Bun.file(artifactPath); const ext = artifactPath.split(".").pop()?.toLowerCase(); @@ -926,7 +990,7 @@ export function startServer(port?: number, deps: StartServerDeps = {}): Server withCors(await handleClaudeCountTokens(req, config), req, config)); + return runAdmittedHttpTurn(req, async () => withCors(await handleClaudeCountTokens(req, config), req, policy)); } if (url.pathname === "/v1/messages" && req.method === "POST") { @@ -1021,12 +1085,12 @@ export function startServer(port?: number, deps: StartServerDeps = {}): Server({ ...serveOptions, port: listenPort, hostname: bindHost }); + + // Both binds are one startup transaction (#1102). If the loopback bind fails after the + // public one succeeded, leaving the public listener up would strand it: the CLI's port + // retry would read the failure as a public-port conflict and pick a different port, + // accumulating listeners. Roll back and rethrow the original error instead. + if (loopbackListenerPort !== null) { + try { + loopbackServer = Bun.serve({ + ...serveOptions, + port: loopbackListenerPort, + hostname: "127.0.0.1", + }); + } catch (error) { + try { + // startServer is synchronous, so this rollback cannot await. Bun begins closing the + // listen socket on the call itself; the caller sees the original bind error either + // way, and the alternative — leaving the public listener up — is the failure this + // rollback exists to prevent. + void server.stop(true); + } catch { + /* the original bind error is the one worth reporting */ + } + throw error; + } + } } catch (error) { void nativeMainLifecycle.release(); throw error; @@ -1393,14 +1484,38 @@ export function startServer(port?: number, deps: StartServerDeps = {}): Server => { + // Two properties, and dropping either one breaks a caller. + // + // Cleanup must complete: a failure closing one listener cannot skip the other or the + // native lifecycle release. + // + // Failure must PROPAGATE: `stopServerListener` deliberately surfaces a stop rejection so + // every caller observes the same result before a replacement binds the port. Swallowing + // it with allSettled would let drainAndShutdown report success while a listener still + // holds its socket, and the replacement would then fail its own bind. + const failures: unknown[] = []; try { - await nativeStop(closeActiveConnections); + try { + await nativeStop(closeActiveConnections); + } catch (error) { + failures.push(error); + } + if (loopbackListenerRef) { + try { + await loopbackListenerRef.stop(closeActiveConnections); + } catch (error) { + failures.push(error); + } + } } finally { await releaseNativeMainStartupLifecycle(server); } + if (failures.length === 1) throw failures[0]; + if (failures.length > 1) throw new AggregateError(failures, "listener shutdown failed"); }, }); setServerRef(server); @@ -1415,6 +1530,17 @@ export function startServer(port?: number, deps: StartServerDeps = {}): Server = {}): Request { + return new Request(`http://127.0.0.1:10200${path}`, { headers }); +} + +describe("loopback listener policy view", () => { + test("the public listener still demands a credential on a wildcard bind", () => { + // The whole point of the separate listener is that this does not change. + expect(resolveResponsesApiAuth(request(), wildcardConfig)).toBeNull(); + }); + + test("the loopback view admits without a credential and names it loopback", () => { + const policy = requestPolicyView(wildcardConfig, "127.0.0.1"); + expect(resolveResponsesApiAuth(request(), policy)).toEqual({ kind: "loopback" }); + }); + + test("the view carries no bind address other than the one it was given", () => { + // A view built from a wildcard config must not leak that wildcard back into an auth + // decision — that would silently restore the 401 the listener exists to avoid. + const policy = requestPolicyView(wildcardConfig, "127.0.0.1"); + expect(policy.hostname).toBe("127.0.0.1"); + }); + + test("a valid configured key is still attributed to that key, not collapsed to loopback", () => { + // The loopback view takes the same branch a plain loopback bind always has, which returns + // before reading any header. Assert the public listener keeps per-key attribution so a + // future refactor cannot quietly make every admission anonymous. + expect(resolveResponsesApiAuth( + request("/v1/responses", { "x-opencodex-api-key": "ocx_data_realsecret" }), + wildcardConfig, + )).toEqual({ kind: "configured", keyId: "k1" }); + }); +}); + +describe("loopback listener origin gate", () => { + // The kernel bind stops remote TCP, but not a victim browser: an attacker page can make the + // browser connect to 127.0.0.1, and that connection IS local. The Host/Origin gate is the + // other half of the boundary, and the loopback view must route through it. + test("a hostile Host is rejected under the loopback policy", () => { + const policy = requestPolicyView(wildcardConfig, "127.0.0.1"); + expect(isAllowedRequestOrigin( + request("/v1/responses", { Host: "attacker.example" }), + policy, + )).toBe(false); + }); + + test("a hostile Origin is rejected even when the Host looks local", () => { + const policy = requestPolicyView(wildcardConfig, "127.0.0.1"); + expect(isAllowedRequestOrigin( + request("/v1/responses", { Host: "127.0.0.1:10200", Origin: "http://attacker.example" }), + policy, + )).toBe(false); + }); + + test("the same hostile Origin would pass under the PUBLIC policy via same-origin", () => { + // This is why the view matters. On a remote bind `isAllowedRequestOrigin` accepts a + // same-origin request, so handing the public config to the loopback listener's origin + // check would admit exactly the DNS-rebinding shape the test above rejects. + const sameOrigin = new Request("http://attacker.example/v1/responses", { + headers: { Origin: "http://attacker.example" }, + }); + expect(isAllowedRequestOrigin(sameOrigin, wildcardConfig)).toBe(true); + }); + + test("an ordinary local request is allowed", () => { + const policy = requestPolicyView(wildcardConfig, "127.0.0.1"); + expect(isAllowedRequestOrigin(request("/v1/responses", { Host: "127.0.0.1:10200" }), policy)).toBe(true); + }); +}); + +describe("loopback listener configuration", () => { + test("an enabled listener sharing the proxy port is rejected at write time", () => { + // A collision would otherwise surface as a startup failure after the public listener had + // already bound, which reads like an unrelated port conflict. + const result = validateConfigCandidate({ + port: 10100, + providers: { openai: { adapter: "openai", baseUrl: "https://chatgpt.com/backend-api/codex" } }, + defaultProvider: "openai", + unauthenticatedLoopbackListener: { enabled: true, port: 10100 }, + }); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.error).toContain("must differ from the proxy port"); + }); + + test("an enabled listener without a port is rejected", () => { + // An OS-assigned port would change across restarts and strand app-servers holding the + // previous base_url — the symptom #1102 reported and we disproved for token rotation. + const result = validateConfigCandidate({ + port: 10100, + providers: { openai: { adapter: "openai", baseUrl: "https://chatgpt.com/backend-api/codex" } }, + defaultProvider: "openai", + unauthenticatedLoopbackListener: { enabled: true }, + }); + expect(result.ok).toBe(false); + }); + + test("a disabled listener needs no port", () => { + const result = validateConfigCandidate({ + port: 10100, + providers: { openai: { adapter: "openai", baseUrl: "https://chatgpt.com/backend-api/codex" } }, + defaultProvider: "openai", + unauthenticatedLoopbackListener: { enabled: false }, + }); + expect(result.ok).toBe(true); + }); + + test("a distinct port is accepted and survives the parse", () => { + const result = validateConfigCandidate({ + port: 10100, + providers: { openai: { adapter: "openai", baseUrl: "https://chatgpt.com/backend-api/codex" } }, + defaultProvider: "openai", + unauthenticatedLoopbackListener: { enabled: true, port: 10200 }, + }); + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.config.unauthenticatedLoopbackListener).toEqual({ enabled: true, port: 10200 }); + } + }); +}); + +describe("injected Codex provider block", () => { + test("a wildcard bind alone still emits the env auth header", () => { + expect(shouldInjectApiAuthHeader({ hostname: "0.0.0.0" })).toBe(true); + }); + + test("enabling the loopback listener drops the header", () => { + // The directly-spawned app-server has no OPENCODEX_API_AUTH_TOKEN, so emitting the header + // would make Codex send an empty value rather than authenticate. + expect(shouldInjectApiAuthHeader({ + hostname: "0.0.0.0", + unauthenticatedLoopbackListener: { enabled: true, port: 10200 }, + })).toBe(false); + }); + + test("a disabled listener leaves the wildcard behaviour intact", () => { + expect(shouldInjectApiAuthHeader({ + hostname: "0.0.0.0", + unauthenticatedLoopbackListener: { enabled: false }, + })).toBe(true); + }); +}); diff --git a/tests/windows-deploy-close-regressions.test.ts b/tests/windows-deploy-close-regressions.test.ts index 05287d2de..41933c22a 100644 --- a/tests/windows-deploy-close-regressions.test.ts +++ b/tests/windows-deploy-close-regressions.test.ts @@ -73,8 +73,16 @@ describe("server bind canonicalizes explicit localhost but preserves wildcards ( test("literal localhost binds to 127.0.0.1; 0.0.0.0/:: exposure is untouched", () => { expect(src).toContain("const configuredHost = config.hostname?.trim();"); expect(src).toContain('!configuredHost || /^localhost$/i.test(configuredHost) ? "127.0.0.1"'); - expect(src).toContain("hostname: bindHost,"); - // Must not blanket-rewrite the bind host (that would break intentional 0.0.0.0 exposure). - expect(src).not.toContain('hostname: "127.0.0.1",'); + // Must not blanket-rewrite the PUBLIC bind host — that would break intentional 0.0.0.0 + // exposure, which is the regression this guards. + // + // A literal "127.0.0.1" now appears once, for the separate unauthenticated loopback + // listener (#1102). That one is a second socket whose entire purpose is to be + // loopback-only, so a bare substring ban would forbid the fix rather than the defect. + // Pin the assertion to the public serve call instead: it must take bindHost and nothing + // else. + expect(src).toContain("server = Bun.serve({ ...serveOptions, port: listenPort, hostname: bindHost });"); + expect(src).not.toMatch(/port: listenPort,\s*\n\s*hostname: "127\.0\.0\.1"/); + expect(src).not.toContain("port: listenPort, hostname: \"127.0.0.1\""); }); }); From 835a82785c44d9afdbfb0adfdf240afbd269838d Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 23:12:49 +0900 Subject: [PATCH 3/6] test(server): exercise the loopback listener over real sockets, and reserve its port MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The audit found the previous commit shipped an unauthenticated surface whose tests never started a server. Every assertion ran against pure helpers, so the listener could fail to open, bind the wrong address, lose the identity comparison against the public server, or serve routes outside its allowlist, and the suite would stay green. That is not a coverage gap; it is a missing oracle for a security boundary. Ten integration tests now speak HTTP to real listeners: the same request answered 401 on the public socket and 200 on the loopback one, a connection to the machine's non-loopback IPv4 being refused, management and dashboard and unrelated data-plane routes returning 404 while an allowlisted one returns 200, the loopback Host/Origin gate rejecting the rebinding shape the public policy would accept, both ports rebindable after stop, and a squatted loopback port rolling back the public bind. Ablation, all driven red rather than argued: binding the loopback listener to 0.0.0.0 fails 2; removing the allowlist check fails 1; collapsing the policy view to the shared config fails 3. The second finding was a real gap rather than a test one. `ocx start --port ` would bind the public listener onto the address the loopback listener is configured for, and the loopback bind would then fail EADDRINUSE — rolling back a startup whose only problem was a config collision. `findAvailablePort` takes a `reservedPort`: an explicit preference for it is refused up front rather than retried, and ephemeral selection redraws if the OS hands it back. Two smaller corrections. `drainingResponse` and `serverBusyResponse` run before the auth checks and were still using the shared config, so a 503 on the loopback listener could echo a hostile origin; both now take the receiving listener's policy. And a non-boolean `enabled` was silently deleted by the schema's catch, leaving an operator convinced they had enabled a listener that was off — write-time now rejects it. Refs #1102 --- src/cli/index.ts | 7 + src/config.ts | 7 + src/server/index.ts | 37 +-- src/server/ports.ts | 23 +- tests/loopback-listener-admission.test.ts | 11 +- tests/loopback-listener-integration.test.ts | 237 ++++++++++++++++++++ 6 files changed, 304 insertions(+), 18 deletions(-) create mode 100644 tests/loopback-listener-integration.test.ts diff --git a/src/cli/index.ts b/src/cli/index.ts index e9f3c22c3..f410af13b 100755 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -170,6 +170,13 @@ async function chooseListenPort(requestedPort?: number): Promise { preferRetryMs: hardPin ? 5_000 : 750, preferRetryIntervalMs: 50, allowEphemeralFallback: !hardPin, + // Never hand the public listener the port the loopback listener is configured to + // bind (#1102). Without this, `--port ` binds the public listener + // first and the loopback bind then fails, rolling back a startup that was only + // ever a config collision. + ...(config.unauthenticatedLoopbackListener?.enabled + ? { reservedPort: config.unauthenticatedLoopbackListener.port } + : {}), }); if (preferred > 0 && selected !== preferred) { console.log(`⚠️ Port ${preferred} is busy; starting opencodex on ${selected}.`); diff --git a/src/config.ts b/src/config.ts index b929f074c..f2587d4c8 100644 --- a/src/config.ts +++ b/src/config.ts @@ -2001,6 +2001,13 @@ function loopbackListenerPortError(value: unknown): string | null { return "schema_invalid: unauthenticatedLoopbackListener: must be an object or omitted"; } const entry = listener as Record; + // `enabled` must be a real boolean. The schema's `.catch(undefined)` would otherwise DELETE + // a `"true"` string entry and report success, leaving an operator convinced they enabled an + // unauthenticated listener that is in fact off. Load-time still degrades quietly — a hand + // edit must not reset the file — but a live caller gets told. + if (typeof entry.enabled !== "boolean") { + return "schema_invalid: unauthenticatedLoopbackListener.enabled: must be a boolean"; + } if (entry.enabled !== true) return null; const listenerPort = entry.port; if (typeof listenerPort !== "number" || !Number.isInteger(listenerPort) || listenerPort < 1 || listenerPort > 65535) { diff --git a/src/server/index.ts b/src/server/index.ts index 83793c560..860583b41 100644 --- a/src/server/index.ts +++ b/src/server/index.ts @@ -539,23 +539,28 @@ export function startServer(port?: number, deps: StartServerDeps = {}): Server Promise): Promise { @@ -659,7 +664,7 @@ export function startServer(port?: number, deps: StartServerDeps = {}): Server { const preferRetryMs = opts.preferRetryMs ?? 0; const allowEphemeral = opts.allowEphemeralFallback !== false; + const reserved = opts.reservedPort; + // An explicit preference for the reserved port is a configuration mistake, not a busy + // socket: retrying or hopping would hide it. Refuse before probing anything. + if (reserved !== undefined && preferredPort === reserved) { + throw new PortUnavailableError(preferredPort, hostname); + } // Port 0 asks the OS to select an ephemeral port. Resolve it to that concrete // port here so callers never persist or advertise an unusable `:0` endpoint. if (preferredPort > 0 && preferRetryMs > 0) { @@ -99,7 +115,12 @@ export async function findAvailablePort( const address = server.address(); const port = typeof address === "object" && address ? address.port : 0; server.close(() => { - if (port > 0) resolve(port); + // The OS can hand back the reserved port. Retry rather than accept it; the pool is + // large enough that a second draw practically always differs. + if (port > 0 && port !== reserved) resolve(port); + else if (port === reserved) { + findAvailablePort(0, hostname, opts).then(resolve, reject); + } else reject(new Error("failed to allocate an available port")); }); }); diff --git a/tests/loopback-listener-admission.test.ts b/tests/loopback-listener-admission.test.ts index bd12ae062..2ecd269d4 100644 --- a/tests/loopback-listener-admission.test.ts +++ b/tests/loopback-listener-admission.test.ts @@ -17,7 +17,7 @@ import { requestPolicyView, resolveResponsesApiAuth, } from "../src/server/auth-cors"; -import { shouldInjectApiAuthHeader } from "../src/codex/inject"; +import { buildProviderTableBlock, shouldInjectApiAuthHeader } from "../src/codex/inject"; import { validateConfigCandidate } from "../src/config"; import type { OcxConfig } from "../src/types"; @@ -165,4 +165,13 @@ describe("injected Codex provider block", () => { unauthenticatedLoopbackListener: { enabled: false }, })).toBe(true); }); + + test("the emitted block points at the loopback port and carries no auth header", () => { + // shouldInjectApiAuthHeader alone does not prove the injected TOML is usable. Assert the + // rendered block, because that is what a directly spawned app-server actually reads: a + // base_url on the public port, or an env header it cannot populate, both reproduce #1102. + const block = buildProviderTableBlock(10200, false, false, "0.0.0.0"); + expect(block).toContain('base_url = "http://127.0.0.1:10200/v1"'); + expect(block).not.toContain("env_http_headers"); + }); }); diff --git a/tests/loopback-listener-integration.test.ts b/tests/loopback-listener-integration.test.ts new file mode 100644 index 000000000..96a55fbb2 --- /dev/null +++ b/tests/loopback-listener-integration.test.ts @@ -0,0 +1,237 @@ +/** + * Integration coverage for the unauthenticated loopback listener (#1102). + * + * The companion unit file exercises the admission and CORS helpers in isolation. That is not + * enough for a surface that admits without a credential: helper-level tests stay green if the + * second listener never opens, binds the wrong address, is not distinguished from the public + * one, or serves routes outside its allowlist. These tests start real servers and speak HTTP + * to them, so those regressions have somewhere to fail. + */ +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { existsSync, mkdirSync, mkdtempSync, rmSync } from "node:fs"; +import { connect } from "node:net"; +import { networkInterfaces, tmpdir } from "node:os"; +import { join } from "node:path"; +import { saveConfig } from "../src/config"; +import { startServer } from "../src/server"; +import { findAvailablePort, PortUnavailableError } from "../src/server/ports"; +import type { OcxConfig } from "../src/types"; + +const previousApiToken = process.env.OPENCODEX_API_AUTH_TOKEN; +const previousHome = process.env.OPENCODEX_HOME; +let testDir = ""; + +function baseConfig(loopbackPort: number | null): OcxConfig { + return { + port: 0, + hostname: "0.0.0.0", + defaultProvider: "chatgpt", + providers: { + chatgpt: { + adapter: "openai-responses", + baseUrl: "https://chatgpt.com/backend-api/codex", + authMode: "forward", + }, + }, + ...(loopbackPort === null + ? {} + : { unauthenticatedLoopbackListener: { enabled: true, port: loopbackPort } }), + } as unknown as OcxConfig; +} + +/** A free port to hand the loopback listener, chosen the same way production would not reuse. */ +async function freePort(): Promise { + return await findAvailablePort(0, "127.0.0.1"); +} + +function firstNonLoopbackIPv4(): string | null { + for (const entries of Object.values(networkInterfaces())) { + for (const entry of entries ?? []) { + if (entry.family === "IPv4" && !entry.internal) return entry.address; + } + } + return null; +} + +beforeEach(() => { + testDir = mkdtempSync(join(tmpdir(), "ocx-loopback-listener-")); + process.env.OPENCODEX_HOME = testDir; + process.env.OPENCODEX_API_AUTH_TOKEN = "public-secret"; +}); + +afterEach(() => { + if (previousApiToken === undefined) delete process.env.OPENCODEX_API_AUTH_TOKEN; + else process.env.OPENCODEX_API_AUTH_TOKEN = previousApiToken; + if (previousHome === undefined) delete process.env.OPENCODEX_HOME; + else process.env.OPENCODEX_HOME = previousHome; + if (testDir && existsSync(testDir)) rmSync(testDir, { recursive: true, force: true }); + testDir = ""; +}); + +describe("unauthenticated loopback listener", () => { + test("is absent unless configured, and the public listener still demands a key", async () => { + saveConfig(baseConfig(null)); + const server = startServer(0); + try { + const res = await fetch(`http://127.0.0.1:${server.port}/v1/models`); + expect(res.status).toBe(401); + } finally { + await server.stop(true); + } + }); + + test("admits without a credential while the public listener does not", async () => { + const loopbackPort = await freePort(); + saveConfig(baseConfig(loopbackPort)); + const server = startServer(0); + try { + // Same request, two sockets, two answers. This is the whole feature. + const viaPublic = await fetch(`http://127.0.0.1:${server.port}/v1/models`); + expect(viaPublic.status).toBe(401); + + const viaLoopback = await fetch(`http://127.0.0.1:${loopbackPort}/v1/models`); + expect(viaLoopback.status).toBe(200); + } finally { + await server.stop(true); + } + }); + + test("refuses connections on a non-loopback interface", async () => { + const address = firstNonLoopbackIPv4(); + if (!address) { + // A host with no external IPv4 cannot prove this. Say so rather than pass silently: + // a quiet skip here would let the bind address regress unnoticed on that machine. + console.warn("[loopback-listener] no non-loopback IPv4 interface; bind-scope check not run"); + return; + } + const loopbackPort = await freePort(); + saveConfig(baseConfig(loopbackPort)); + const server = startServer(0); + try { + const refused = await new Promise(resolve => { + const socket = connect({ host: address, port: loopbackPort }); + const settle = (value: boolean) => { + socket.destroy(); + resolve(value); + }; + socket.setTimeout(2_000); + socket.once("connect", () => settle(false)); + socket.once("error", () => settle(true)); + socket.once("timeout", () => settle(true)); + }); + expect(refused).toBe(true); + } finally { + await server.stop(true); + } + }); + + test("serves only the four allowlisted routes", async () => { + const loopbackPort = await freePort(); + saveConfig(baseConfig(loopbackPort)); + const server = startServer(0); + const base = `http://127.0.0.1:${loopbackPort}`; + try { + // Management, dashboard and unrelated data-plane routes must not be reachable from a + // surface that skips authentication. + for (const path of [ + "/api/config", + "/", + "/healthz", + "/readyz", + "/v1/chat/completions", + "/v1/messages", + "/v1/images/generations", + "/v1/alpha/search", + "/v1/opencodex/artifacts/x", + "/v1/live", + ]) { + const res = await fetch(`${base}${path}`, { method: "GET" }); + expect({ path, status: res.status }).toEqual({ path, status: 404 }); + } + + // And the allowlisted one is genuinely reachable, so the assertions above are not + // passing merely because nothing works. + expect((await fetch(`${base}/v1/models`)).status).toBe(200); + } finally { + await server.stop(true); + } + }); + + test("applies the loopback Host and Origin gate, not the public same-origin rule", async () => { + const loopbackPort = await freePort(); + saveConfig(baseConfig(loopbackPort)); + const server = startServer(0); + const url = `http://127.0.0.1:${loopbackPort}/v1/models`; + try { + // The kernel refuses remote TCP, but a victim's browser connects locally on an + // attacker's behalf. Under the PUBLIC policy this same-origin shape is allowed; under + // the loopback policy it must not be. + const rebinding = await fetch(url, { headers: { Host: "attacker.example" } }); + expect(rebinding.status).toBe(403); + + const hostileOrigin = await fetch(url, { headers: { Origin: "http://attacker.example" } }); + expect(hostileOrigin.status).toBe(403); + expect(hostileOrigin.headers.get("access-control-allow-origin")).not.toBe("http://attacker.example"); + + const ok = await fetch(url); + expect(ok.status).toBe(200); + } finally { + await server.stop(true); + } + }); + + test("stopping the server closes both listeners", async () => { + const loopbackPort = await freePort(); + saveConfig(baseConfig(loopbackPort)); + const server = startServer(0); + const publicPort = server.port; + await server.stop(true); + + // Both ports must be rebindable. A surviving loopback listener would keep serving + // unauthenticated traffic after shutdown reported success. + for (const port of [publicPort, loopbackPort]) { + const probe = Bun.serve({ port, hostname: "127.0.0.1", fetch: () => new Response("ok") }); + probe.stop(true); + } + }); + + test("a loopback bind failure rolls back the public listener", async () => { + const loopbackPort = await freePort(); + const squatter = Bun.serve({ + port: loopbackPort, + hostname: "127.0.0.1", + fetch: () => new Response("occupied"), + }); + saveConfig(baseConfig(loopbackPort)); + try { + expect(() => startServer(0)).toThrow(); + } finally { + squatter.stop(true); + } + }); +}); + +describe("public port selection avoids the loopback port", () => { + test("an explicit preference for the reserved port is refused rather than taken", async () => { + const reserved = await freePort(); + // Free, yet must not be selected: taking it would bind the public listener onto the + // address the loopback listener is configured for, and the loopback bind would then fail. + await expect(findAvailablePort(reserved, "127.0.0.1", { reservedPort: reserved })) + .rejects.toBeInstanceOf(PortUnavailableError); + }); + + test("ephemeral selection never returns the reserved port", async () => { + const reserved = await freePort(); + for (let i = 0; i < 8; i += 1) { + const selected = await findAvailablePort(0, "127.0.0.1", { reservedPort: reserved }); + expect(selected).not.toBe(reserved); + } + }); + + test("an unreserved preference is still honored", async () => { + const reserved = await freePort(); + const wanted = await freePort(); + if (wanted === reserved) return; + expect(await findAvailablePort(wanted, "127.0.0.1", { reservedPort: reserved })).toBe(wanted); + }); +}); From 09813ab3b0c03504ab53f6a1f90bccac2dc57d83 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 23:32:24 +0900 Subject: [PATCH 4/6] test(server): probe each loopback route with its real method, and reach the real inject wiring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two of the previous tests were watching nothing, and the audit was right about both. The allowlist check probed POST routes with GET. Chat Completions, Messages, Images, search and Live all 404 on method mismatch inside their handlers, so widening the allowlist to admit one of them would have kept the assertion green. Each route is now requested with the method its handler accepts, plus two cases proving the allowlisted paths still reject methods they do not serve. Ablation: adding /v1/messages to the allowlist now fails it. The injection test handed the loopback port straight to buildProviderTableBlock, so deleting the substitution inside injectCodexConfig changed nothing it observed. It now runs the real injector in a subprocess — CODEX_CONFIG_PATH resolves at module load, so an in-process CODEX_HOME would write somewhere else — passes the PUBLIC port the way every caller does, and reads the written config.toml. Ablation: removing the substitution fails it. Also added: POST /v1/responses and /v1/responses/compact admitted on the loopback listener and 401 on the public one, asserted as neither 401 nor 404 because "not 401" alone would survive removing the route; a Responses WebSocket handshake on both listeners; and a rollback test that binds a fixed public port and rebinds it after the throw, since throwing while leaving the public listener up is the failure the rollback exists to prevent. Two honest gaps are recorded rather than papered over. The WebSocket test cannot defend requestServer.upgrade over server.upgrade: that ablation stayed green because this Bun version accepts an upgrade issued from a sibling Bun.serve. And composite stop's failure propagation has no test, because the composite captures the underlying stop at construction and there is no seam to inject a rejection through; its sibling property, cleanup across both listeners, is covered. Port selection fixes from the same review: the ephemeral redraw is now a bounded loop behind an injectable allocator rather than unbounded async recursion, and it has tests that actually reach the redraw branch. An explicit --port collision with the reserved port is rejected before the 60-second reclaim path instead of after it, with a message naming the real cause. runAdmittedHttpTurn now takes the policy explicitly, so no CORS-emitting helper falls back to the shared config. Refs #1102 --- src/cli/index.ts | 16 +- src/server/index.ts | 28 ++- src/server/ports.ts | 33 ++- tests/loopback-listener-integration.test.ts | 229 +++++++++++++++++--- 4 files changed, 255 insertions(+), 51 deletions(-) diff --git a/src/cli/index.ts b/src/cli/index.ts index f410af13b..ace0ee130 100755 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -147,6 +147,18 @@ async function chooseListenPort(requestedPort?: number): Promise { const config = loadConfig(); const preferred = requestedPort ?? config.port ?? 10100; const hardPin = requestedPort !== undefined && requestedPort > 0; + const reservedLoopbackPort = config.unauthenticatedLoopbackListener?.enabled + ? config.unauthenticatedLoopbackListener.port + : undefined; + // Before the reclaim path, not after (#1102). Asking for the port the loopback listener is + // configured to bind is a configuration mistake, and reclaim would spend up to 60 seconds + // waiting for a socket to free before reporting "port is busy" — the wrong diagnosis for a + // collision the config can state outright. + if (reservedLoopbackPort !== undefined && preferred === reservedLoopbackPort) { + throw new Error( + `Port ${preferred} is reserved for unauthenticatedLoopbackListener; choose a different proxy port.`, + ); + } // Soft start: brief prefer-retry then ephemeral hop. // Explicit `--port` (service wrappers / update restart): wait for the pinned port // to free without killing any listener (healthy ocx / foreign). Never hop. @@ -174,9 +186,7 @@ async function chooseListenPort(requestedPort?: number): Promise { // bind (#1102). Without this, `--port ` binds the public listener // first and the loopback bind then fails, rolling back a startup that was only // ever a config collision. - ...(config.unauthenticatedLoopbackListener?.enabled - ? { reservedPort: config.unauthenticatedLoopbackListener.port } - : {}), + ...(reservedLoopbackPort !== undefined ? { reservedPort: reservedLoopbackPort } : {}), }); if (preferred > 0 && selected !== preferred) { console.log(`⚠️ Port ${preferred} is busy; starting opencodex on ${selected}.`); diff --git a/src/server/index.ts b/src/server/index.ts index 860583b41..9ac691ce4 100644 --- a/src/server/index.ts +++ b/src/server/index.ts @@ -544,7 +544,7 @@ export function startServer(port?: number, deps: StartServerDeps = {}): Server Promise): Promise { + async function runAdmittedHttpTurn( + req: Request, + policy: RequestPolicyView, + work: (lease: ActiveTurnLease) => Promise, + ): Promise { const lease = tryAdmitTurn(); - if (!lease) return serverBusyResponse(req, "active turns"); + if (!lease) return serverBusyResponse(req, "active turns", policy); let response: Response; try { response = await work(lease); @@ -927,7 +931,7 @@ export function startServer(port?: number, deps: StartServerDeps = {}): Server { + return runAdmittedHttpTurn(req, policy, async turnAdmissionLease => { let response: Response; try { response = await handleResponsesCompact(req, config, logCtx, turnAdmissionLease); @@ -961,7 +965,7 @@ export function startServer(port?: number, deps: StartServerDeps = {}): Server { + return runAdmittedHttpTurn(req, policy, async turnAdmissionLease => { const response = await handleImages(req, config, endpoint, logCtx, turnAdmissionLease); addFinalRequestLog(requestId, start, logCtx, response.status, response.status === 499 ? { closeReason: "client_cancel" } : undefined); return withCors(response, req, policy); @@ -1015,7 +1019,7 @@ export function startServer(port?: number, deps: StartServerDeps = {}): Server { + return runAdmittedHttpTurn(req, policy, async turnAdmissionLease => { const response = await handleSearch(req, config, logCtx, turnAdmissionLease); addFinalRequestLog(requestId, start, logCtx, response.status, response.status === 499 ? { closeReason: "client_cancel" } : undefined); @@ -1050,7 +1054,7 @@ export function startServer(port?: number, deps: StartServerDeps = {}): Server { + return runAdmittedHttpTurn(req, policy, async turnAdmissionLease => { const response = await handleResponses(req, config, logCtx, { turnAdmissionLease, abortSignal: req.signal, @@ -1082,7 +1086,7 @@ export function startServer(port?: number, deps: StartServerDeps = {}): Server withCors(await handleClaudeCountTokens(req, config), req, policy)); + return runAdmittedHttpTurn(req, policy, async () => withCors(await handleClaudeCountTokens(req, config), req, policy)); } if (url.pathname === "/v1/messages" && req.method === "POST") { @@ -1108,7 +1112,7 @@ export function startServer(port?: number, deps: StartServerDeps = {}): Server withCors( + return runAdmittedHttpTurn(req, policy, async turnAdmissionLease => withCors( await handleClaudeMessages(req, config, logCtx, { requestId, start, turnAdmissionLease }), req, config, @@ -1135,7 +1139,7 @@ export function startServer(port?: number, deps: StartServerDeps = {}): Server withCors( + return runAdmittedHttpTurn(req, policy, async turnAdmissionLease => withCors( await handleChatCompletions(req, config, logCtx, { requestId, start, turnAdmissionLease }), req, config, @@ -1165,7 +1169,7 @@ export function startServer(port?: number, deps: StartServerDeps = {}): Server { + return runAdmittedHttpTurn(req, policy, async turnAdmissionLease => { const response = await handleLive(req, config, logCtx, turnAdmissionLease); addFinalRequestLog( requestId, diff --git a/src/server/ports.ts b/src/server/ports.ts index eebf7c196..ae8fe16c1 100644 --- a/src/server/ports.ts +++ b/src/server/ports.ts @@ -108,19 +108,38 @@ export async function findAvailablePort( throw new PortUnavailableError(preferredPort, hostname); } - return await new Promise((resolve, reject) => { + // Bounded, not recursive. The OS can hand back the reserved port, and a redraw practically + // always differs — but "practically always" is not a termination argument, and an unbounded + // async recursion has no way to stop if the assumption is ever wrong. + for (let attempt = 0; attempt < EPHEMERAL_REDRAW_LIMIT; attempt += 1) { + const port = await allocateEphemeralPort(hostname); + if (port !== reserved) return port; + } + throw new Error("failed to allocate an available port"); +} + +/** How many times an ephemeral draw may come back reserved before we give up. */ +const EPHEMERAL_REDRAW_LIMIT = 8; + +/** Test seam: replace the OS ephemeral allocator so the redraw path is reachable. */ +let ephemeralAllocator: ((hostname: string) => Promise) | null = null; + +export function setEphemeralPortAllocatorForTests( + allocator: ((hostname: string) => Promise) | null, +): void { + ephemeralAllocator = allocator; +} + +async function allocateEphemeralPort(hostname: string): Promise { + if (ephemeralAllocator) return ephemeralAllocator(hostname); + return await new Promise((resolve, reject) => { const server = createServer(); server.once("error", reject); server.once("listening", () => { const address = server.address(); const port = typeof address === "object" && address ? address.port : 0; server.close(() => { - // The OS can hand back the reserved port. Retry rather than accept it; the pool is - // large enough that a second draw practically always differs. - if (port > 0 && port !== reserved) resolve(port); - else if (port === reserved) { - findAvailablePort(0, hostname, opts).then(resolve, reject); - } + if (port > 0) resolve(port); else reject(new Error("failed to allocate an available port")); }); }); diff --git a/tests/loopback-listener-integration.test.ts b/tests/loopback-listener-integration.test.ts index 96a55fbb2..e5d3f3c5a 100644 --- a/tests/loopback-listener-integration.test.ts +++ b/tests/loopback-listener-integration.test.ts @@ -8,13 +8,18 @@ * to them, so those regressions have somewhere to fail. */ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; -import { existsSync, mkdirSync, mkdtempSync, rmSync } from "node:fs"; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { spawnSync } from "node:child_process"; import { connect } from "node:net"; import { networkInterfaces, tmpdir } from "node:os"; import { join } from "node:path"; import { saveConfig } from "../src/config"; import { startServer } from "../src/server"; -import { findAvailablePort, PortUnavailableError } from "../src/server/ports"; +import { + findAvailablePort, + PortUnavailableError, + setEphemeralPortAllocatorForTests, +} from "../src/server/ports"; import type { OcxConfig } from "../src/types"; const previousApiToken = process.env.OPENCODEX_API_AUTH_TOKEN; @@ -125,38 +130,126 @@ describe("unauthenticated loopback listener", () => { } }); - test("serves only the four allowlisted routes", async () => { + test("serves only the four allowlisted routes, using each route's real method", async () => { const loopbackPort = await freePort(); saveConfig(baseConfig(loopbackPort)); const server = startServer(0); const base = `http://127.0.0.1:${loopbackPort}`; try { - // Management, dashboard and unrelated data-plane routes must not be reachable from a - // surface that skips authentication. - for (const path of [ - "/api/config", - "/", - "/healthz", - "/readyz", - "/v1/chat/completions", - "/v1/messages", - "/v1/images/generations", - "/v1/alpha/search", - "/v1/opencodex/artifacts/x", - "/v1/live", - ]) { - const res = await fetch(`${base}${path}`, { method: "GET" }); - expect({ path, status: res.status }).toEqual({ path, status: 404 }); + // Each entry uses the METHOD its handler actually accepts. Probing a POST route with GET + // would 404 on method mismatch inside the handler, so the assertion would hold even if + // the allowlist were widened to admit that route — the test would be watching nothing. + const denied: Array<{ method: string; path: string; body?: string }> = [ + { method: "GET", path: "/api/config" }, + { method: "GET", path: "/" }, + { method: "GET", path: "/healthz" }, + { method: "GET", path: "/readyz" }, + { method: "POST", path: "/v1/chat/completions", body: '{"model":"x","messages":[]}' }, + { method: "POST", path: "/v1/messages", body: '{"model":"x","messages":[]}' }, + { method: "POST", path: "/v1/images/generations", body: '{"prompt":"x"}' }, + { method: "POST", path: "/v1/alpha/search", body: '{"query":"x"}' }, + { method: "GET", path: "/v1/opencodex/artifacts/x" }, + { method: "POST", path: "/v1/live", body: "{}" }, + { method: "POST", path: "/v1/realtime/calls", body: "{}" }, + // Allowlisted paths still reject the methods they do not serve. + { method: "DELETE", path: "/v1/responses" }, + { method: "POST", path: "/v1/models" }, + ]; + for (const { method, path, body } of denied) { + const res = await fetch(`${base}${path}`, { + method, + ...(body ? { body, headers: { "content-type": "application/json" } } : {}), + }); + expect({ method, path, status: res.status }).toEqual({ method, path, status: 404 }); } - // And the allowlisted one is genuinely reachable, so the assertions above are not - // passing merely because nothing works. + // And an allowlisted route is genuinely reachable, so the rejections above are not + // passing merely because nothing works on this listener. expect((await fetch(`${base}/v1/models`)).status).toBe(200); } finally { await server.stop(true); } }); + test("admits POST /v1/responses and its compact sibling without a credential", async () => { + const loopbackPort = await freePort(); + saveConfig(baseConfig(loopbackPort)); + const server = startServer(0); + const base = `http://127.0.0.1:${loopbackPort}`; + const publicBase = `http://127.0.0.1:${server.port}`; + try { + // These are the routes the reported defect actually fails on. `/v1/models` passing does + // not prove they admit: they use a different resolver. + // + // The request is deliberately malformed, so it fails INSIDE the handler rather than at + // admission. Any status other than 401 proves admission let it through, which is the + // only thing under test here — no upstream is involved. + for (const path of ["/v1/responses", "/v1/responses/compact"]) { + const viaPublic = await fetch(`${publicBase}${path}`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: "{}", + }); + expect({ path, status: viaPublic.status }).toEqual({ path, status: 401 }); + + const viaLoopback = await fetch(`${base}${path}`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: "{}", + }); + // Not `not.toBe(401)`: a 404 would satisfy that too, so removing the route from the + // allowlist would keep the assertion green. The route must be admitted AND reachable, + // which means neither 401 nor 404. + expect({ path, status: viaLoopback.status }).not.toEqual({ path, status: 401 }); + expect({ path, status: viaLoopback.status }).not.toEqual({ path, status: 404 }); + } + } finally { + await server.stop(true); + } + }); + + test("upgrades a Responses WebSocket on the listener that received it", async () => { + const loopbackPort = await freePort(); + saveConfig({ ...baseConfig(loopbackPort), websockets: true } as unknown as OcxConfig); + const server = startServer(0); + try { + // What this proves: the loopback listener completes a Responses WebSocket handshake + // without a credential, and the public one does not. + // + // What it does NOT prove: that the upgrade goes through `requestServer` rather than the + // captured `server`. That ablation was run and stayed green — this Bun version accepts + // an upgrade issued from a sibling Bun.serve in the same process. `requestServer` is + // still correct (it is the server that received the request, and nothing documents the + // cross-listener behaviour as supported), but the assertion below cannot defend it. + // Saying so beats implying coverage that does not exist. + const opened = await new Promise(resolve => { + const ws = new WebSocket(`ws://127.0.0.1:${loopbackPort}/v1/responses`); + const settle = (value: boolean) => { + try { ws.close(); } catch { /* already closing */ } + resolve(value); + }; + ws.addEventListener("open", () => settle(true)); + ws.addEventListener("error", () => settle(false)); + setTimeout(() => settle(false), 3_000); + }); + expect(opened).toBe(true); + + const publicOpened = await new Promise(resolve => { + const ws = new WebSocket(`ws://127.0.0.1:${server.port}/v1/responses`); + const settle = (value: boolean) => { + try { ws.close(); } catch { /* already closing */ } + resolve(value); + }; + ws.addEventListener("open", () => settle(true)); + ws.addEventListener("error", () => settle(false)); + setTimeout(() => settle(false), 3_000); + }); + expect(publicOpened).toBe(false); + } finally { + await server.stop(true); + } + }); + test("applies the loopback Host and Origin gate, not the public same-origin rule", async () => { const loopbackPort = await freePort(); saveConfig(baseConfig(loopbackPort)); @@ -195,8 +288,12 @@ describe("unauthenticated loopback listener", () => { } }); - test("a loopback bind failure rolls back the public listener", async () => { + test("a loopback bind failure rolls back the public listener rather than stranding it", async () => { const loopbackPort = await freePort(); + // A FIXED public port, not 0. Throwing is not the property under test — a startup that + // throws while leaving the public listener bound is exactly the failure the rollback + // exists to prevent, and only a rebind attempt can tell the two apart. + const publicPort = await freePort(); const squatter = Bun.serve({ port: loopbackPort, hostname: "127.0.0.1", @@ -204,14 +301,31 @@ describe("unauthenticated loopback listener", () => { }); saveConfig(baseConfig(loopbackPort)); try { - expect(() => startServer(0)).toThrow(); + expect(() => startServer(publicPort)).toThrow(); + + const rebound = Bun.serve({ + port: publicPort, + hostname: "127.0.0.1", + fetch: () => new Response("ok"), + }); + expect(rebound.port).toBe(publicPort); + rebound.stop(true); } finally { squatter.stop(true); } }); + + // Not covered here: composite stop's failure PROPAGATION when one listener's stop rejects. + // The composite captures the underlying stop at construction, so a test cannot inject a + // rejection from outside without a seam that does not exist yet. Its sibling property — + // cleanup completing across both listeners — is covered by the test above. Writing a case + // that asserts something weaker and calls it propagation coverage would be worse than the + // gap, because the next reader would believe the branch was defended. }); describe("public port selection avoids the loopback port", () => { + afterEach(() => setEphemeralPortAllocatorForTests(null)); + test("an explicit preference for the reserved port is refused rather than taken", async () => { const reserved = await freePort(); // Free, yet must not be selected: taking it would bind the public listener onto the @@ -220,12 +334,26 @@ describe("public port selection avoids the loopback port", () => { .rejects.toBeInstanceOf(PortUnavailableError); }); - test("ephemeral selection never returns the reserved port", async () => { - const reserved = await freePort(); - for (let i = 0; i < 8; i += 1) { - const selected = await findAvailablePort(0, "127.0.0.1", { reservedPort: reserved }); - expect(selected).not.toBe(reserved); - } + test("ephemeral selection redraws when the OS hands back the reserved port", async () => { + // Without a seam this branch is unreachable: the OS practically never returns the one + // reserved port, so a loop over real draws would pass with the redraw code deleted. + const reserved = 45_001; + const draws = [reserved, reserved, 45_002]; + let index = 0; + setEphemeralPortAllocatorForTests(async () => draws[index++] ?? 45_003); + expect(await findAvailablePort(0, "127.0.0.1", { reservedPort: reserved })).toBe(45_002); + expect(index).toBe(3); + }); + + test("redrawing is bounded rather than recursing forever", async () => { + const reserved = 45_001; + let draws = 0; + setEphemeralPortAllocatorForTests(async () => { + draws += 1; + return reserved; + }); + await expect(findAvailablePort(0, "127.0.0.1", { reservedPort: reserved })).rejects.toThrow(); + expect(draws).toBe(8); }); test("an unreserved preference is still honored", async () => { @@ -235,3 +363,46 @@ describe("public port selection avoids the loopback port", () => { expect(await findAvailablePort(wanted, "127.0.0.1", { reservedPort: reserved })).toBe(wanted); }); }); + +describe("Codex injection targets the loopback listener", () => { + test("the written config points at the loopback port with no auth header", () => { + // A subprocess, because CODEX_CONFIG_PATH is resolved at module load: setting CODEX_HOME + // in-process would write to whatever path this test file already imported. + // + // The child passes the PUBLIC port to injectCodexConfig, which is what every real caller + // does. The loopback substitution happens inside the injector, so handing the loopback + // port straight to the block builder would keep passing with that wiring deleted. + const root = mkdtempSync(join(tmpdir(), "ocx-loopback-inject-")); + const codexHome = join(root, ".codex"); + const ocxHome = join(root, ".opencodex"); + mkdirSync(codexHome, { recursive: true }); + mkdirSync(ocxHome, { recursive: true }); + writeFileSync(join(codexHome, "config.toml"), 'model = "gpt-5"\n', "utf-8"); + const config = baseConfig(10_200) as Record; + config.port = 10_100; + writeFileSync(join(ocxHome, "config.json"), JSON.stringify(config), "utf-8"); + try { + const child = spawnSync(process.execPath, [ + join(process.cwd(), "tests", "helpers", "codex-inject-race-child.ts"), + ], { + cwd: process.cwd(), + encoding: "utf8", + env: { + ...process.env, + CODEX_HOME: codexHome, + OPENCODEX_HOME: ocxHome, + OCX_INJECT_RACE_PAYLOAD: JSON.stringify({ port: 10_100 }), + }, + }); + const line = (child.stdout ?? "").trim().split("\n").filter(Boolean).pop() ?? "{}"; + expect(JSON.parse(line)).toMatchObject({ success: true }); + + const written = readFileSync(join(codexHome, "config.toml"), "utf-8"); + expect(written).toContain("http://127.0.0.1:10200/v1"); + expect(written).not.toContain("http://127.0.0.1:10100/v1"); + expect(written).not.toContain("env_http_headers"); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); +}); From 4854254566da1eda28de7bf3c6dd725a2d3b6d4e Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Fri, 7 Aug 2026 23:43:31 +0900 Subject: [PATCH 5/6] refactor(server): make composite listener shutdown testable, and pin two blind seams MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The shutdown orchestration held two properties that pull against each other — keep cleaning up after a failure, yet still report it — and neither was testable in place, because the composite captures the underlying stop at construction. Extracted to `runListenerShutdown`, which now has four cases including both failure shapes. Ablation: removing the per-step catch fails 2, swallowing the collected failures fails 3. Two seams have no runtime oracle on this Bun version, and both would regress silently. Swapping `requestServer.upgrade` for `server.upgrade` stays green because this Bun accepts an upgrade issued from a sibling Bun.serve; another version is not promised to, and the loopback listener would then fail to upgrade at all. And the connection-refused test degrades to a warning on a host with no external IPv4, so the 0.0.0.0 ablation would pass there. Source assertions are a weak instrument, but one aimed at a known blind spot beats a comment nobody runs — and the upgrade assertion does go red on that swap. Smaller review fixes: the WebSocket handshake helper now clears its timer and settles once, so a late timeout cannot fire into the next test; and the rollback test draws its public port with the loopback port reserved, since two back-to-back freePort() calls can return the same port and the test would squat itself. Refs #1102 --- src/server/index.ts | 40 ++----- src/server/lifecycle.ts | 36 ++++++ tests/loopback-listener-integration.test.ts | 123 ++++++++++++++++---- 3 files changed, 147 insertions(+), 52 deletions(-) diff --git a/src/server/index.ts b/src/server/index.ts index 9ac691ce4..07a900e1b 100644 --- a/src/server/index.ts +++ b/src/server/index.ts @@ -71,6 +71,7 @@ import { getActiveTurnCount, isDraining, registerTurn, + runListenerShutdown, setServerRef, trackStreamLifetime, tryAdmitTurn, @@ -1497,34 +1498,17 @@ export function startServer(port?: number, deps: StartServerDeps = {}): Server => { - // Two properties, and dropping either one breaks a caller. - // - // Cleanup must complete: a failure closing one listener cannot skip the other or the - // native lifecycle release. - // - // Failure must PROPAGATE: `stopServerListener` deliberately surfaces a stop rejection so - // every caller observes the same result before a replacement binds the port. Swallowing - // it with allSettled would let drainAndShutdown report success while a listener still - // holds its socket, and the replacement would then fail its own bind. - const failures: unknown[] = []; - try { - try { - await nativeStop(closeActiveConnections); - } catch (error) { - failures.push(error); - } - if (loopbackListenerRef) { - try { - await loopbackListenerRef.stop(closeActiveConnections); - } catch (error) { - failures.push(error); - } - } - } finally { - await releaseNativeMainStartupLifecycle(server); - } - if (failures.length === 1) throw failures[0]; - if (failures.length > 1) throw new AggregateError(failures, "listener shutdown failed"); + // The orchestration lives in `runListenerShutdown` so its two competing properties — + // cleanup completes, failure propagates — are testable without a live socket. + await runListenerShutdown( + [ + () => nativeStop(closeActiveConnections), + ...(loopbackListenerRef + ? [() => loopbackListenerRef.stop(closeActiveConnections)] + : []), + ], + () => releaseNativeMainStartupLifecycle(server), + ); }, }); setServerRef(server); diff --git a/src/server/lifecycle.ts b/src/server/lifecycle.ts index b4825044b..45efdac82 100644 --- a/src/server/lifecycle.ts +++ b/src/server/lifecycle.ts @@ -292,6 +292,42 @@ export function getServerListenPort(): number | undefined { * ordinary drain's finally block; both callers must observe the same stop result * before any replacement process is allowed to bind the port. */ +/** + * Run every shutdown step, then report whether any of them failed. + * + * Extracted from `startServer`'s composite `stop` because the two properties it has to hold + * pull in opposite directions and neither is testable in place: + * + * - Cleanup COMPLETES. A listener whose stop rejects cannot prevent the other listener, or the + * native lifecycle release, from running. + * - Failure PROPAGATES. `stopServerListener` deliberately surfaces a stop rejection so every + * caller sees the same result before a replacement binds the port. Swallowing it would let + * `drainAndShutdown` report success while a socket is still held. + * + * `always` runs after the listeners regardless of their outcome, and its own failure joins the + * reported set rather than replacing it. + */ +export async function runListenerShutdown( + steps: Array<() => Promise>, + always: () => Promise, +): Promise { + const failures: unknown[] = []; + for (const step of steps) { + try { + await step(); + } catch (error) { + failures.push(error); + } + } + try { + await always(); + } catch (error) { + failures.push(error); + } + if (failures.length === 1) throw failures[0]; + if (failures.length > 1) throw new AggregateError(failures, "listener shutdown failed"); +} + export function stopServerListener( server: ReturnType | undefined = _serverRef, ): Promise { diff --git a/tests/loopback-listener-integration.test.ts b/tests/loopback-listener-integration.test.ts index e5d3f3c5a..25419c593 100644 --- a/tests/loopback-listener-integration.test.ts +++ b/tests/loopback-listener-integration.test.ts @@ -15,6 +15,7 @@ import { networkInterfaces, tmpdir } from "node:os"; import { join } from "node:path"; import { saveConfig } from "../src/config"; import { startServer } from "../src/server"; +import { runListenerShutdown } from "../src/server/lifecycle"; import { findAvailablePort, PortUnavailableError, @@ -58,6 +59,25 @@ function firstNonLoopbackIPv4(): string | null { return null; } +/** One-shot settle with a cleared timer, so a late timeout cannot fire into the next test. */ +function handshake(url: string): Promise { + return new Promise(resolve => { + const ws = new WebSocket(url); + let settled = false; + let timer: ReturnType | undefined; + const settle = (value: boolean) => { + if (settled) return; + settled = true; + if (timer) clearTimeout(timer); + try { ws.close(); } catch { /* already closing */ } + resolve(value); + }; + ws.addEventListener("open", () => settle(true)); + ws.addEventListener("error", () => settle(false)); + timer = setTimeout(() => settle(false), 3_000); + }); +} + beforeEach(() => { testDir = mkdtempSync(join(tmpdir(), "ocx-loopback-listener-")); process.env.OPENCODEX_HOME = testDir; @@ -222,29 +242,8 @@ describe("unauthenticated loopback listener", () => { // still correct (it is the server that received the request, and nothing documents the // cross-listener behaviour as supported), but the assertion below cannot defend it. // Saying so beats implying coverage that does not exist. - const opened = await new Promise(resolve => { - const ws = new WebSocket(`ws://127.0.0.1:${loopbackPort}/v1/responses`); - const settle = (value: boolean) => { - try { ws.close(); } catch { /* already closing */ } - resolve(value); - }; - ws.addEventListener("open", () => settle(true)); - ws.addEventListener("error", () => settle(false)); - setTimeout(() => settle(false), 3_000); - }); - expect(opened).toBe(true); - - const publicOpened = await new Promise(resolve => { - const ws = new WebSocket(`ws://127.0.0.1:${server.port}/v1/responses`); - const settle = (value: boolean) => { - try { ws.close(); } catch { /* already closing */ } - resolve(value); - }; - ws.addEventListener("open", () => settle(true)); - ws.addEventListener("error", () => settle(false)); - setTimeout(() => settle(false), 3_000); - }); - expect(publicOpened).toBe(false); + expect(await handshake(`ws://127.0.0.1:${loopbackPort}/v1/responses`)).toBe(true); + expect(await handshake(`ws://127.0.0.1:${server.port}/v1/responses`)).toBe(false); } finally { await server.stop(true); } @@ -293,7 +292,9 @@ describe("unauthenticated loopback listener", () => { // A FIXED public port, not 0. Throwing is not the property under test — a startup that // throws while leaving the public listener bound is exactly the failure the rollback // exists to prevent, and only a rebind attempt can tell the two apart. - const publicPort = await freePort(); + // Reserve the loopback port during this draw: two back-to-back freePort() calls can hand + // back the same port, which would make the test squat its own public port. + const publicPort = await findAvailablePort(0, "127.0.0.1", { reservedPort: loopbackPort }); const squatter = Bun.serve({ port: loopbackPort, hostname: "127.0.0.1", @@ -323,6 +324,80 @@ describe("unauthenticated loopback listener", () => { // gap, because the next reader would believe the branch was defended. }); +describe("composite listener shutdown", () => { + // Both listeners share one `stop`, and the two properties it must hold pull against each + // other: keep cleaning up after a failure, yet still report that failure. A test against a + // live server cannot inject the rejection, so the orchestration was extracted. + test("a failing step does not stop the others, and still reaches the caller", async () => { + const ran: string[] = []; + const failure = new Error("primary stop failed"); + await expect(runListenerShutdown( + [ + async () => { ran.push("primary"); throw failure; }, + async () => { ran.push("loopback"); }, + ], + async () => { ran.push("lifecycle"); }, + )).rejects.toBe(failure); + // The whole point: a rejected primary stop must not strand the loopback socket or skip + // the native lifecycle release. + expect(ran).toEqual(["primary", "loopback", "lifecycle"]); + }); + + test("two failures are reported together rather than one hiding the other", async () => { + const ran: string[] = []; + let caught: unknown; + try { + await runListenerShutdown( + [ + async () => { ran.push("primary"); throw new Error("a"); }, + async () => { ran.push("loopback"); throw new Error("b"); }, + ], + async () => { ran.push("lifecycle"); }, + ); + } catch (error) { + caught = error; + } + expect(caught).toBeInstanceOf(AggregateError); + expect((caught as AggregateError).errors).toHaveLength(2); + expect(ran).toEqual(["primary", "loopback", "lifecycle"]); + }); + + test("a failing lifecycle release is reported too", async () => { + const failure = new Error("release failed"); + await expect(runListenerShutdown( + [async () => {}], + async () => { throw failure; }, + )).rejects.toBe(failure); + }); + + test("an all-clear shutdown resolves", async () => { + await expect(runListenerShutdown([async () => {}, async () => {}], async () => {})) + .resolves.toBeUndefined(); + }); +}); + +describe("seams the runtime cannot defend", () => { + // Two properties have no runtime oracle on this Bun version, and both would regress + // silently. A source assertion is a weak instrument, but a weak instrument aimed at a known + // blind spot beats none — the alternative is a comment nobody runs. + const serverSource = readFileSync(join(process.cwd(), "src", "server", "index.ts"), "utf-8"); + + test("the WebSocket upgrade uses the receiving server, never the captured binding", () => { + // Swapping in `server.upgrade` stays green at runtime here: this Bun accepts an upgrade + // issued from a sibling Bun.serve in the same process. Another version or platform is not + // promised to, and the loopback listener would then fail to upgrade at all. + expect(serverSource).not.toMatch(/\bif \(server\.upgrade\(req,/); + expect(serverSource.match(/requestServer\.upgrade\(req,/g)?.length).toBe(2); + }); + + test("the loopback listener binds 127.0.0.1 explicitly", () => { + // The connection-refused test above is the real oracle, but it degrades to a warning on a + // host with no external IPv4 — and on that host the 0.0.0.0 ablation would pass. This + // holds everywhere. + expect(serverSource).toMatch(/port: loopbackListenerPort,\s*\n\s*hostname: "127\.0\.0\.1",/); + }); +}); + describe("public port selection avoids the loopback port", () => { afterEach(() => setEphemeralPortAllocatorForTests(null)); From 912cb99f33b25a4bf745cc494e8d8f92fa72fad0 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 8 Aug 2026 01:53:35 +0900 Subject: [PATCH 6/6] test(server): prove the direct-spawn path with a real Codex app-server MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The audit named this the ready gate, and it was right to. Every existing test proves a piece — admission, the route allowlist, CORS, the bind scope, the injected port — and none of them prove the thing the feature exists for: that a real `codex app-server`, spawned the way a third-party host spawns it, reaches the proxy without a credential. That seam is between two processes. Not a `bun test` file. The repository does not depend on `@openai/codex`, so a test that skips when it is absent would report green on machines that never ran it. This fails loudly and its output is the evidence. The oracle is a routed model id generated at run time. Codex caches model lists and falls back to a bundled catalog when a refresh fails, so "did model/list succeed" proves nothing — a broken path looks identical to a working one. A name no bundled catalog can contain can only have arrived through our listener. Writing it surfaced two things worth recording. `model/list` reads `model_catalog_json`; it does not call the provider's `/v1/models`, so the first version watched Codex return its five bundled ids while never touching the listener — the exact false-negative shape the unique id exists to expose, pointed at the harness instead of the feature. And a hand-written catalog fixture is a liability: Codex rejects the whole file on any schema mismatch and silently falls back, so the catalog is now built with our own serializer, which also means the script exercises the bytes `ocx sync` writes. Result on Codex 0.146.0, isolated CODEX_HOME with no models_cache.json and OPENCODEX_API_AUTH_TOKEN stripped from the child environment: 9/9, ending with POST /v1/responses observed on the loopback listener. Ablation: pointing base_url at a dead port makes the last check red, so the harness is watching the hop rather than asserting its own setup. Refs #1102 --- scripts/verify-loopback-direct-spawn.mjs | 246 +++++++++++++++++++++++ 1 file changed, 246 insertions(+) create mode 100644 scripts/verify-loopback-direct-spawn.mjs diff --git a/scripts/verify-loopback-direct-spawn.mjs b/scripts/verify-loopback-direct-spawn.mjs new file mode 100644 index 000000000..0d48e07e1 --- /dev/null +++ b/scripts/verify-loopback-direct-spawn.mjs @@ -0,0 +1,246 @@ +#!/usr/bin/env node +/** + * Activation evidence for the unauthenticated loopback listener (#1102). + * + * The server-level tests prove admission, the route allowlist, CORS, the bind scope and the + * injected port independently. None of them prove the thing the feature exists for: that a real + * `codex app-server`, spawned the way a third-party host spawns it, reaches the proxy without a + * credential. That seam is between two processes, so no in-process test can stand in for it. + * + * This is deliberately not a `bun test` file. The repository does not depend on `@openai/codex`, + * so a test that silently skips when it is absent would be worse than no test — it would report + * green on machines that never ran it. This script fails loudly instead, and its output is the + * evidence attached to the PR. + * + * The oracle is a routed model whose id is generated at run time. Codex caches model lists and + * falls back to a bundled catalog when a refresh fails, so asking "did model/list succeed" proves + * nothing — a broken `/v1/models` looks identical to a working one. A name no bundled catalog can + * contain can only have come through our listener. + * + * Usage: node scripts/verify-loopback-direct-spawn.mjs + */ +import { spawn, spawnSync } from "node:child_process"; +import http from "node:http"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { createServer } from "node:net"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { randomUUID } from "node:crypto"; + +const UNIQUE_MODEL = `ocx-direct-spawn-${randomUUID()}`; +const steps = []; +function record(name, ok, detail) { + steps.push({ name, ok, detail }); + console.log(`${ok ? "PASS" : "FAIL"} ${name}${detail ? ` — ${detail}` : ""}`); +} + +function resolveCodexEntrypoint() { + // The resolved entrypoint, never `codex` from PATH: the whole defect is that PATH may hold the + // generated shim, which exports the token and would make this pass for the wrong reason. + const probe = spawnSync(process.execPath, [ + "-e", + "process.stdout.write(require.resolve('@openai/codex/bin/codex.js'))", + ], { encoding: "utf8" }); + if (probe.status === 0 && probe.stdout.trim()) return probe.stdout.trim(); + const which = spawnSync("readlink", ["-f", spawnSync("which", ["codex"], { encoding: "utf8" }).stdout.trim()], { encoding: "utf8" }); + const path = which.stdout.trim(); + if (!path) throw new Error("cannot resolve @openai/codex/bin/codex.js"); + return path; +} + +async function freePort() { + return await new Promise((resolve, reject) => { + const probe = createServer(); + probe.once("error", reject); + probe.once("listening", () => { + const { port } = probe.address(); + probe.close(() => resolve(port)); + }); + probe.listen({ port: 0, host: "127.0.0.1" }); + }); +} + +/** A stand-in proxy: serves the loopback listener's four routes and records what Codex asked for. */ +function startFakeProxy(port, seen) { + return new Promise(resolve => { + const srv = http.createServer((req, res) => { + seen.push(`${req.method} ${req.url}`); + if (req.url.startsWith("/v1/responses")) { + res.writeHead(500, { "content-type": "application/json" }); + res.end(JSON.stringify({ error: { message: "stub upstream" } })); + return; + } + if (req.url.startsWith("/v1/models")) { + res.writeHead(200, { "content-type": "application/json" }); + res.end(JSON.stringify({ + object: "list", + data: [{ id: UNIQUE_MODEL, object: "model", created: 0, owned_by: "opencodex" }], + })); + return; + } + res.writeHead(404, { "content-type": "application/json" }); + res.end(JSON.stringify({ error: { message: "not found" } })); + }); + srv.listen(port, "127.0.0.1", () => resolve(srv)); + }); +} + +async function main() { + const entrypoint = resolveCodexEntrypoint(); + record("resolved the real Codex entrypoint, not PATH", true, entrypoint); + + const version = spawnSync(process.execPath, [entrypoint, "--version"], { encoding: "utf8" }); + record("entrypoint runs", version.status === 0, version.stdout.trim() || version.stderr.trim()); + + const home = mkdtempSync(join(tmpdir(), "ocx-direct-spawn-")); + const codexHome = join(home, ".codex"); + const port = await freePort(); + const seen = []; + const proxy = await startFakeProxy(port, seen); + + try { + // The provider block `ocx sync` writes when the loopback listener is enabled: loopback host, + // the listener's port, and NO env_http_headers — the app-server has no token to put in one. + // + // The catalog file matters and is easy to get wrong. `model/list` reads `model_catalog_json`; + // it does not call the provider's `/v1/models`. A first version of this script omitted the + // catalog and watched Codex return its five bundled ids while never touching the listener — + // which is exactly the false-negative shape the unique-id oracle exists to expose, just + // pointed at the harness instead of the feature. + mkdirSync(codexHome, { recursive: true }); + // Build the catalog with OUR OWN serializer rather than a hand-written object. Codex rejects + // the whole file on any schema mismatch and silently falls back to its bundled list, so a + // hand-rolled fixture drifts into a false negative the moment the schema moves. Using + // `buildCatalogEntries` also means this script exercises the same bytes `ocx sync` writes. + const catalogPath = join(codexHome, "opencodex-models.json"); + const build = spawnSync("bun", ["-e", ` + const { buildCatalogEntries } = await import("./src/codex/catalog/sync.ts"); + const entries = buildCatalogEntries(null, [], [{ + provider: "opencodex", + id: ${JSON.stringify(UNIQUE_MODEL)}, + contextWindow: 128000, + }]); + process.stdout.write(JSON.stringify({ models: entries })); + `], { cwd: process.cwd(), encoding: "utf8" }); + if (build.status !== 0 || !build.stdout.trim()) { + record("built the catalog with our own serializer", false, (build.stderr || "").slice(0, 400)); + throw new Error("catalog build failed"); + } + writeFileSync(catalogPath, build.stdout, "utf-8"); + record("built the catalog with our own serializer", true, `${JSON.parse(build.stdout).models.length} entries`); + writeFileSync(join(codexHome, "config.toml"), [ + `model = "${UNIQUE_MODEL}"`, + 'model_provider = "opencodex"', + `model_catalog_json = ${JSON.stringify(catalogPath)}`, + "", + "[model_providers.opencodex]", + 'name = "OpenCodex Proxy"', + `base_url = "http://127.0.0.1:${port}/v1"`, + 'wire_api = "responses"', + "requires_openai_auth = true", + "", + ].join("\n"), "utf-8"); + record("wrote an isolated CODEX_HOME with no models_cache.json", true, codexHome); + + const env = { ...process.env, CODEX_HOME: codexHome }; + // The credential must be absent, or this would prove nothing about the shim-less path. + delete env.OPENCODEX_API_AUTH_TOKEN; + record("stripped OPENCODEX_API_AUTH_TOKEN from the child environment", true); + + const child = spawn(process.execPath, [entrypoint, "app-server"], { + env, + stdio: ["pipe", "pipe", "pipe"], + }); + + let buffered = ""; + const responses = new Map(); + child.stdout.on("data", chunk => { + buffered += chunk.toString(); + let index; + while ((index = buffered.indexOf("\n")) >= 0) { + const line = buffered.slice(0, index).trim(); + buffered = buffered.slice(index + 1); + if (!line) continue; + try { + const message = JSON.parse(line); + if (message.id !== undefined) responses.set(message.id, message); + } catch { /* notifications and logs are not our concern */ } + } + }); + const stderr = []; + child.stderr.on("data", chunk => stderr.push(chunk.toString())); + + const send = payload => child.stdin.write(`${JSON.stringify(payload)}\n`); + const await_ = async (id, timeoutMs = 30_000) => { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + if (responses.has(id)) return responses.get(id); + await new Promise(r => setTimeout(r, 50)); + } + return null; + }; + + send({ id: 1, method: "initialize", params: { clientInfo: { name: "ocx-verify", version: "1", title: "OpenCodex verification" } } }); + const init = await await_(1); + record("app-server initialized", !!init && !init.error, init?.error ? JSON.stringify(init.error) : "ok"); + + send({ id: 2, method: "model/list", params: {} }); + const list = await await_(2, 45_000); + const models = list?.result?.items ?? list?.result?.models ?? list?.result?.data ?? []; + const ids = models.map(m => m?.id ?? m?.model ?? m?.slug).filter(Boolean); + // Routed models are namespaced `/` in the catalog, so match on the unique + // suffix rather than a bare equality that would fail for a correct result. + const sawUnique = ids.some(id => id === UNIQUE_MODEL || id.endsWith(`/${UNIQUE_MODEL}`)); + record( + "model/list contains the unique routed model that only our listener can supply", + sawUnique, + sawUnique ? UNIQUE_MODEL : `saw ${ids.length} ids, none matching (${ids.slice(0, 6).join(", ")})`, + ); + + const hitModels = seen.some(entry => entry.includes("/v1/models")); + // `model/list` reads the catalog file, so it does NOT prove a network hop. The turn does: + // it opens `/v1/responses` against the injected base_url, and reaching our listener there + // without a credential is the whole claim of #1102. + send({ + id: 3, + method: "thread/start", + params: { cwd: home, model: UNIQUE_MODEL, provider: "opencodex" }, + }); + const started = await await_(3, 30_000); + const threadId = started?.result?.threadId ?? started?.result?.thread?.id; + record("thread/start accepted", !!threadId, threadId ? String(threadId) : JSON.stringify(started?.error ?? started).slice(0, 200)); + + if (threadId) { + send({ + id: 4, + method: "turn/start", + params: { threadId, input: [{ type: "text", text: "ping" }] }, + }); + // The upstream is a stub, so the turn is expected to FAIL. What matters is that the + // request arrived at all: a 401 at admission would never reach the handler. + await await_(4, 25_000); + } + + const hitResponses = seen.some(entry => entry.includes("/v1/responses")); + record( + "the app-server reached the loopback listener without a credential", + hitModels || hitResponses, + seen.slice(0, 8).join(" | ") || "no requests observed", + ); + + child.kill(); + if (stderr.length && !sawUnique) console.log("\nchild stderr:\n" + stderr.join("").slice(0, 2000)); + } finally { + proxy.close(); + rmSync(home, { recursive: true, force: true }); + } + + const failed = steps.filter(step => !step.ok); + console.log(`\n${steps.length - failed.length}/${steps.length} checks passed`); + process.exit(failed.length === 0 ? 0 : 1); +} + +main().catch(error => { + console.error(error); + process.exit(1); +});