fix(server): treat forwarded loopback ports as loopback - #573
Conversation
The first interactive `ocx start` asked with a typed `[y/N]` line. Replace it with an inline selector that shows both choices, moves with the arrow keys, and accepts `y`/`n` directly. No stays highlighted, so Enter alone still declines, and escape or Ctrl-C decline as well. The prompt now also requires `gh auth status` to succeed: a logged-out `gh` cannot fulfil a Yes, so asking would only produce a failure message. Terminals without raw mode fall back to the typed question. Declining remains terminal — no persisted decline state and nothing injected into any model prompt to keep nudging afterwards.
Three changes to the one-time GitHub-star question: - The highlighted choice is now Yes, so Enter accepts. The selector shows which side is highlighted, so Enter never does something unannounced. - The question names its mechanism: starring goes through the user's own `gh` login, so the prompt says so. - When an agent is driving `ocx start`, the prompt is not auto-answered. The agent is told to ask the user and to run the star command only on a yes. The one-time marker stays unwritten, so the real selector still appears on the user's own hand-typed run. Agent detection lives in src/cli/agent-driven.ts and is env-var based: a false positive only postpones the prompt, while a false negative would let an agent spend the user's GitHub identity.
WP1 docs-only cycle. Five decade docs at diff-level precision, ordered by dependency (auth gate -> response layer -> adapter -> PR cleanup) rather than effort: - 010 SSH remote proxy: isLoopbackRequestHost couples loopback identity to port equality, so ssh -L on a different local port 403s the whole /v1/* data plane. The sibling isLoopbackOriginValue already dropped its port check in e4e0612 for the same reason. - 020 issue #553: the same three-line 'Provider unreachable' shape repeats at core.ts 1197/1746/1788; fold into one helper and give ERR_TLS_CERT_ALTNAME_INVALID its own wording plus a verification command. - 030 issue #545: the anthropic adapter prepends the Claude Code identity unconditionally on OAuth, so a classifier that already carries it loses output budget it capped at 64 tokens and retries. - 040 PR #527: its first commit already landed on dev as 9dd3c42, which is what makes the branch dirty; rebase keeping only a64aa58. - 050 PR #557: no code work remains, only a security-boundary decision. No production code changed in this cycle.
Independent read-only review returned GO-WITH-FIXES with 4 blockers. Retires WP4 outright. Its premise was inverted: skipSystemPromptPrefix means Claude Code does NOT prepend its prefix, so the proposed dedup guard could never fire — while the unit tests would have passed by feeding the constant to itself. The compared strings differ anyway, and a maintainer had already ruled on issue #545, from an outbound capture, that removing the OAuth identity block is not a safe fix. Also folds back: - the ssh -g / devcontainer residual risk the research doc carried but the plan had dropped, now stated as accepted risk - the CODEOWNERS security-review boundary on src/server/auth-cors.ts, so WP2 is NEEDS_HUMAN-eligible rather than self-merging - OcxConfig imports from src/types, not src/config (tsc TS2459) - a tsconfig-independent error code cast - a range-diff precondition before skipping 1ba588e on PR #527 - server-auth.test.ts:582-602 named as WP2's must-not-break oracle
The plan assumed 1ba588e could simply be skipped because dev already carries 9dd3c42. Measured: same six files, but dev's version is +122 lines larger (208 vs 86 insertions, 150 differing range-diff lines) — it is a rewrite that absorbed review feedback, not a rename. Skipping the commit blindly and resolving conflicts toward the PR side would have silently reverted those 122 lines. The rebase procedure now pins dev as the canonical side and requires a post-cherry-pick diff against origin/dev to prove the delta is confined to the stale app-server warning.
isLoopbackRequestHost() required the Host header's port to equal the proxy's own port, so a proxy reached through `ssh -L 20100:localhost:10100` arrived as `Host: localhost:20100` and was refused. That gate sits in front of the whole data plane — /v1/models, /v1/responses, /v1/messages, /v1/chat/completions, /v1/live and the Responses WebSocket upgrade — not just CORS, and it fires with no Origin header at all, so Codex CLI, Claude Code and curl all saw a proxy that looked completely dead rather than one with a CORS problem. Loopback is a trust boundary by hostname, not by port. The sibling isLoopbackOriginValue() dropped its own port check for exactly this reason in e4e0612. Port equality was never the rebinding defense either: a rebinding browser connects to the real port and sends it verbatim, so the hostname check is what rejected it before and still does. Verified against 40 Host forms — localhost.attacker.com, 127.0.0.1.nip.io, localtest.me, 0.0.0.0, 127.0.0.2, [::ffff:127.0.0.1] and a Cyrillic homograph all stay refused. Also accepts the FQDN form `localhost.`, which curl sends verbatim and which was refused for the same class of reason. The predicate had no test coverage at all. The new file pins both directions, including a characterization test for the pre-existing fail-open on an unparseable Host so tightening it later needs a deliberate failing test. Docs: forwarding is now documented, with the caveats that a forwarded loopback is unauthenticated (ssh -g / container publishing expose it), that the client base URL must be set by hand, and that provider OAuth login needs its own forward.
📝 WalkthroughWalkthroughThe PR adds owner-ledger and bug-resolution planning documents, fixes SSH loopback admission for forwarded ports with regression coverage and documentation, and updates the CLI’s GitHub star prompt with agent detection, interactive confirmation, and authentication checks. ChangesOwner decision ledger
Bug-bundle resolution plans
Agent-aware CLI prompting
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SSHClient
participant Proxy
participant AuthCors
SSHClient->>Proxy: Forward request through remapped local port
Proxy->>AuthCors: Evaluate Host and Origin
AuthCors-->>Proxy: Accept loopback hostname independent of port
sequenceDiagram
participant StartCommand
participant AgentDetection
participant GitHubCLI
participant InteractiveConfirm
StartCommand->>AgentDetection: Check environment
AgentDetection-->>StartCommand: Defer or continue
StartCommand->>GitHubCLI: Verify authentication
GitHubCLI-->>StartCommand: Report availability
StartCommand->>InteractiveConfirm: Show Yes/No prompt
InteractiveConfirm-->>StartCommand: Return answer
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2da6f6df3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| resolve(answer); | ||
| // A Ctrl-C during the prompt is still a Ctrl-C: hand it back to the | ||
| // process so the normal shutdown path runs instead of being eaten here. | ||
| if (interrupted) process.kill(process.pid, "SIGINT"); |
There was a problem hiding this comment.
When an authenticated Windows user presses Ctrl-C at this first-run prompt, process.kill(pid, "SIGINT") terminates the process rather than delivering a graceful signal (src/lib/process-control.ts explicitly documents this Windows behavior). Because the prompt runs after the server writes its PID/runtime state and injects system configuration, this bypasses the registered shutdown cleanup and can leave stale PID files or injected client configuration behind. Propagate cancellation to handleStart or invoke its graceful shutdown path without self-killing.
AGENTS.md reference: AGENTS.md:L36-L37
Useful? React with 👍 / 👎.
| const onData = (chunk: Buffer | string) => { | ||
| const key = typeof chunk === "string" ? chunk : chunk.toString("utf8"); | ||
| if (key === KEY_INTERRUPT) return finish(false, true); | ||
| if (key === KEY_ESCAPE) return finish(false, false); | ||
| if (KEY_ENTER.has(key)) return finish(yes, false); |
There was a problem hiding this comment.
Raw stream chunks do not correspond one-to-one with keypresses, but every branch compares the entire chunk to one key sequence. If the terminal coalesces inputs such as arrow-right plus Enter into "\x1b[C\r", neither action is recognized and the prompt appears stuck; if it splits an arrow sequence after "\x1b", the first byte is misread as a standalone Escape and declines immediately. Buffer and tokenize the input rather than treating each data event as exactly one key.
Useful? React with 👍 / 👎.
| const normalized = (hostname ?? "127.0.0.1").trim().toLowerCase(); | ||
| // A fully-qualified "localhost." is the same host as "localhost": curl and some clients | ||
| // send the trailing dot verbatim, and refusing it 403s a legitimate loopback caller. | ||
| const normalized = (hostname ?? "127.0.0.1").trim().toLowerCase().replace(/\.$/, ""); |
There was a problem hiding this comment.
When config.hostname is localhost. or 127.0.0.1., this shared predicate now makes isApiAuthRequired treat the bind as unauthenticated loopback, while the unchanged classifiers in src/codex/inject.ts and src/service.ts still treat it as non-loopback. As a result, Codex injection enters legacy remote mode and writes an env_http_headers reference to an otherwise-unrequired OPENCODEX_API_AUTH_TOKEN, and ocx service install refuses the same configuration without that token. Apply trailing-dot normalization only to parsed request hosts, or update every configuration-side classifier consistently.
Useful? React with 👍 / 👎.
| forward that port too: | ||
|
|
||
| ```bash | ||
| ssh -L 20100:localhost:10100 -L 1455:localhost:1455 you@remote |
There was a problem hiding this comment.
The suggested second forward only works for ChatGPT, whose callback uses port 1455. Other supported OAuth providers listen on different fixed ports—for example Anthropic uses 54545, Google Antigravity uses 51121, and xAI uses 56121—so users following this generic ocx login <provider> guidance for those providers still receive an unreachable callback. Label 1455 as ChatGPT-specific and list or link to each provider's required callback port.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devlog/_plan/260727_owner_decision_ledger/008_wsl_ssh_report_intake.md`:
- Around line 53-60: Update candidate C2 and the related next-action text to
distinguish already-supported general SSH remote access from the missing `ssh -L
local-port:localhost:server-port` different-port forwarding case. Remove the
claim that remote access is unmodeled or requires a new issue, and align the
follow-up status with `009_ssh_remote_proxy_rootcause.md`.
In `@devlog/_plan/260727_owner_decision_ledger/010_bug_bundle_fixability.md`:
- Line 76: Update the affected Markdown lines in this document so issue
identifiers beginning with #, including `#557` and the referenced `#533`, `#424`,
`#447`, and `#527` occurrences, cannot be interpreted as headings; prefix each
identifier with PR or wrap it in inline code, while preserving the surrounding
Korean text and meaning.
In `@devlog/_plan/260728_bug_bundle_resolution/000_plan.md`:
- Around line 3-6: Update the audit-trail dates in the plan document’s session
metadata and the independently reviewed entries to use the actual review date,
July 27, 2026. If those entries describe future work rather than completed
events, label them explicitly as planned/future instead of claiming completion.
In `@devlog/_plan/260728_bug_bundle_resolution/040_pr527_rebase.md`:
- Around line 97-105: Update the post-rebase validation step around the
cherry-pick procedure to compare the complete tree, rather than limiting git
diff to src/codex/. Ensure the check includes all files changed between
1ba588eff and 9dd3c42da, including tests, and verify no assertions or review
changes were dropped before declaring the rebase safe.
- Around line 127-143: Update the PR rebase plan so it changes PR `#527`’s head as
well as its base: either force-push the rebased commits to the branch currently
backing `#527`, or explicitly close `#527` and create a replacement PR from
codex/pr527-rebase. If creating a replacement, revise all later gh pr
view/checks references and related instructions to use the new PR number.
In `@docs-site/src/content/docs/reference/configuration.md`:
- Around line 179-181: Update the documentation sentence describing loopback
handling to say that the Host has a hostname of localhost, 127.0.0.1, or ::1,
rather than saying it resolves to those values. Preserve the existing statement
that any port is accepted and the listed clients work.
In `@src/cli/interactive-confirm.ts`:
- Around line 104-122: Update the onData handler in interactive-confirm.ts to
buffer raw input and distinguish a bare Escape from split arrow-key sequences
such as \x1b[D and \x1b[C before calling finish(false, false); preserve the
existing selection updates for left/right input. Add or update coverage in
tests/interactive-confirm.test.ts lines 28-34 to verify split escape sequences
move the selection and bare Escape still cancels.
In `@src/cli/star-prompt.ts`:
- Around line 25-36: Avoid calling gh auth status for agent-driven runs by
moving the isAgentDriven() short-circuit ahead of the authentication probe in
the star prompt flow. Update ghAvailable() or its caller so gh --version may
still determine installation, but the network-backed auth check is skipped when
the prompt is deferred for agents.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3f4ec1fb-a272-4c79-90fb-4952661ac40b
📒 Files selected for processing (21)
devlog/_plan/260727_owner_decision_ledger/000_scope.mddevlog/_plan/260727_owner_decision_ledger/007_delta_260728.mddevlog/_plan/260727_owner_decision_ledger/008_wsl_ssh_report_intake.mddevlog/_plan/260727_owner_decision_ledger/009_ssh_remote_proxy_rootcause.mddevlog/_plan/260727_owner_decision_ledger/010_bug_bundle_fixability.mddevlog/_plan/260728_bug_bundle_resolution/000_plan.mddevlog/_plan/260728_bug_bundle_resolution/010_ssh_loopback_gate.mddevlog/_plan/260728_bug_bundle_resolution/020_tls_altname_diagnosis.mddevlog/_plan/260728_bug_bundle_resolution/030_claude_system_dedup.mddevlog/_plan/260728_bug_bundle_resolution/040_pr527_rebase.mddevlog/_plan/260728_bug_bundle_resolution/050_pr557_boundary.mddocs-site/src/content/docs/reference/configuration.mdsrc/cli/agent-driven.tssrc/cli/index.tssrc/cli/interactive-confirm.tssrc/cli/star-prompt.tssrc/server/auth-cors.tstests/agent-driven.test.tstests/interactive-confirm.test.tstests/server-loopback-host-gate.test.tstests/startup-prompt.test.ts
| ## 후보 재순위 (정정 후) | ||
|
|
||
| | # | 후보 | 근거 | 실제 강도 | | ||
| | --- | --- | --- | --- | | ||
| | C1 | **#131 회귀** — GUI/CLI 로그인 수동 폴백이 다시 깨짐 | #131이 정확히 이 증상. **PR #491이 07-27 18:59에 `src/oauth/login-cli.ts`를 포함해 승인 0건으로 머지됨** — 제보 12시간 전 | **가장 유력**. 회귀 용의자가 시간·파일 양쪽으로 맞는다 | | ||
| | C2 | SSH 포트포워딩으로 원격 프록시 사용 | 저장소가 이 구성을 모델링하지 않음 | 중. 진짜 빈 땅이지만 제보 표현과의 연결이 약함 | | ||
| | C3 | WSL2 localhost 단방향 | `doctor.ts:845`가 힌트 제공 | **낮음**. 힌트 안에 `networkingMode=mirrored` 해법이 명시돼 있고, `hostname` 설정 레버도 문서화돼 있다(`docs-site/.../configuration.md:33`) — 발견성 문제지 기능 부재가 아님 | | ||
| | C4 | shim interop 거부 | `src/codex/shim.ts:311-314` | **낮음**. 거부 메시지가 복구 명령까지 명시한다. "꼬였다"와 가장 안 맞는다 | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Narrow C2 to different-port SSH forwarding.
This “corrected” intake still says the repository does not model SSH remote access and that C2 requires a new issue. That contradicts 009_ssh_remote_proxy_rootcause.md, which records that remote access is already modeled and only port-remapped forwarding was missing. Update the candidate and next-action text to distinguish general remote access from ssh -L local-port:localhost:server-port; otherwise the ledger misstates the root cause and follow-up status.
Proposed wording change
- C2 | SSH 포트포워딩으로 원격 프록시 사용 | 저장소가 이 구성을 모델링하지 않음
+ C2 | 다른 로컬 포트의 SSH 포트포워딩 | 일반 원격 접근은 모델링돼 있지만 포트 변경 포워딩은 미지원
-3. C2면 새 이슈 — 저장소가 모델링하지 않은 유일한 시나리오다.
+3. C2가 다른 로컬 포트 포워딩이면 `009_ssh_remote_proxy_rootcause.md`의 확정 원인과 수정 상태를 반영한다.Also applies to: 73-86
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260727_owner_decision_ledger/008_wsl_ssh_report_intake.md`
around lines 53 - 60, Update candidate C2 and the related next-action text to
distinguish already-supported general SSH remote access from the missing `ssh -L
local-port:localhost:server-port` different-port forwarding case. Remove the
claim that remote access is unmodeled or requires a new issue, and align the
follow-up status with `009_ssh_remote_proxy_rootcause.md`.
| **#533** `BLOCKED-BY-DEPENDENCY` · #557이 대체 | ||
| 파일 목록 23개가 #557과 동일하다. 조상 관계가 아니라 **더 새 dev 위에 리베이스된 | ||
| 별도 작업**이고, #533에 없는 두 수정(preflight 권한 게이트, job 상태 살균)을 | ||
| #557이 갖고 있다. #533은 Windows 테스트 2건이 실제로 실패했고 크로스플랫폼 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Prevent issue numbers from being parsed as headings.
These lines start with #533, #424, #447, or #527, which triggers MD018 and can produce inconsistent Markdown rendering. Prefix them with PR or wrap the identifiers in code formatting.
Proposed fix
-#533에 없는 두 수정
+PR `#533에` 없는 두 수정
-#424(Grok 이미지 브리지)
+PR `#424`(Grok 이미지 브리지)
-#447·#429·#528은
+PR `#447`·#429·#528은
-#527의 base가
+PR `#527의` base가Also applies to: 82-82, 162-162, 169-169
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 76-76: No space after hash on atx style heading
(MD018, no-missing-space-atx)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260727_owner_decision_ledger/010_bug_bundle_fixability.md` at
line 76, Update the affected Markdown lines in this document so issue
identifiers beginning with #, including `#557` and the referenced `#533`, `#424`,
`#447`, and `#527` occurrences, cannot be interpreted as headings; prefix each
identifier with PR or wrap it in inline code, while preserving the surrounding
Korean text and meaning.
Source: Linters/SAST tools
| 세션: `019fa53a-5c95-76d1-b616-faab73d044e2` | ||
| goalplan: `opencodex-bug-pr-6-7-pabcd-work-phase-wp1-docs-f` | ||
| 기준: `origin/dev` = `f195e90bc`, 로컬 `dev` = `c17f51659`(미푸시 2건) | ||
| 작성: 2026-07-28 (WP1 docs-only 사이클) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Use an accurate date for the audit trail.
This document says it was written and independently reviewed on July 28, 2026, but the current review date is July 27, 2026. Because later terminal criteria rely on these evidence claims, either use the actual date or label the entries as planned/future events.
Also applies to: 110-123
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260728_bug_bundle_resolution/000_plan.md` around lines 3 - 6,
Update the audit-trail dates in the plan document’s session metadata and the
independently reviewed entries to use the actual review date, July 27, 2026. If
those entries describe future work rather than completed events, label them
explicitly as planned/future instead of claiming completion.
| **따라서 계획의 "그냥 건너뛴다"는 그대로 실행하면 안 된다.** 수정된 절차: | ||
|
|
||
| 1. `a64aa5856`만 체리픽하되, **충돌이 나는 것이 정상**이다 — `a64aa5856`는 | ||
| `1ba588eff` 위에 쓰였고 dev에는 그 상위집합이 있다. | ||
| 2. 충돌 해소 시 **dev(`9dd3c42da`) 쪽을 무조건 정본**으로 삼는다. `1ba588eff` | ||
| 버전의 코드가 남으면 리뷰 반영분이 되돌려진다. | ||
| 3. `a64aa5856`가 **새로 추가하는 것만** 얹는다 — stale app-server 경고. | ||
| 4. 체리픽 후 `git diff origin/dev -- src/codex/` 로 실제 델타가 stale 경고에만 | ||
| 국한되는지 확인한다. `9dd3c42da`의 라인이 사라졌으면 잘못된 해소다. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate the complete post-rebase diff.
1ba588eff and 9dd3c42da differ in six files, including test files, but the proposed check only runs git diff origin/dev -- src/codex/. That cannot detect dropped or reverted assertions under tests/; passing tests does not prove the intended coverage survived. Compare the full tree, or explicitly include all six affected files, before declaring the rebase safe.
- git diff origin/dev -- src/codex/
+ git diff --stat origin/dev --
+ git diff --name-status origin/dev --🧰 Tools
🪛 LanguageTool
[grammar] ~102-~102: Ensure spelling is correct
Context: ...삼는다. 1ba588eff 버전의 코드가 남으면 리뷰 반영분이 되돌려진다. 3. a64aa5856가 새로 추가하는 것만 얹는다 — s...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260728_bug_bundle_resolution/040_pr527_rebase.md` around lines
97 - 105, Update the post-rebase validation step around the cherry-pick
procedure to compare the complete tree, rather than limiting git diff to
src/codex/. Ensure the check includes all files changed between 1ba588eff and
9dd3c42da, including tests, and verify no assertions or review changes were
dropped before declaring the rebase safe.
| ### 5. 푸시 + 리타깃 | ||
|
|
||
| ```bash | ||
| git push -u origin codex/pr527-rebase | ||
| gh pr edit 527 --base dev | ||
| ``` | ||
|
|
||
| head 브랜치가 바뀌므로 실제로는 **#527을 닫고 새 PR을 여는 편이 깔끔할 수 | ||
| 있다.** B 단계에서 결정한다: | ||
|
|
||
| | 선택 | 장점 | 단점 | | ||
| | --- | --- | --- | | ||
| | 기존 #527에 force-push + 리타깃 | 이슈 링크 보존 | force-push 필요 | | ||
| | 새 PR + #527 클로즈 | 이력이 깨끗 | 링크가 새 번호로 이동 | | ||
|
|
||
| 오너 본인 PR이고 리뷰 코멘트가 없으므로(reviewDecision 공란) **어느 쪽이든 | ||
| 정보 손실이 없다.** 기존 PR 유지를 우선한다. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='devlog/_plan/260728_bug_bundle_resolution/040_pr527_rebase.md'
echo '## File size'
wc -l "$file"
echo
echo '## Relevant excerpt (lines 90-160)'
cat -n "$file" | sed -n '90,160p'
echo
echo '## Search for PR 527 / branch references in the plan'
rg -n 'PR 527|`#527`|codex/pr527-rebase|gh pr edit|base dev|head branch|reviewDecision|force-push|새 PR' devlog/_plan/260728_bug_bundle_resolution/040_pr527_rebase.md
echo
echo '## Nearby related plan files'
git ls-files 'devlog/_plan/260728_bug_bundle_resolution/*' | sed -n '1,120p'Repository: lidge-jun/opencodex
Length of output: 3224
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='devlog/_plan/260728_bug_bundle_resolution/040_pr527_rebase.md'
echo '## Early plan context (lines 1-90)'
cat -n "$file" | sed -n '1,90p'
echo
echo '## Other mentions of branch/PR state in this file'
python3 - <<'PY'
from pathlib import Path
text = Path('devlog/_plan/260728_bug_bundle_resolution/040_pr527_rebase.md').read_text()
for needle in ['codex/catalog-written-signal', 'codex/pr527-rebase', '`#527`', 'head', 'base', 'force-push', 'reviewDecision']:
print(f'--- {needle} ---')
for i, line in enumerate(text.splitlines(), 1):
if needle in line:
print(f'{i}: {line}')
PY
echo
echo '## Related files that mention PR 527 or branch names'
python3 - <<'PY'
from pathlib import Path
root = Path('devlog/_plan/260728_bug_bundle_resolution')
for p in sorted(root.glob('*.md')):
txt = p.read_text()
if any(s in txt for s in ['`#527`', 'codex/pr527-rebase', 'codex/catalog-written-signal']):
print(p)
PYRepository: lidge-jun/opencodex
Length of output: 4691
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='devlog/_plan/260728_bug_bundle_resolution/040_pr527_rebase.md'
cat -n "$file" | sed -n '160,176p'Repository: lidge-jun/opencodex
Length of output: 718
Update the PR head, not just the base (devlog/_plan/260728_bug_bundle_resolution/040_pr527_rebase.md:129-131, 171-175). git push -u origin codex/pr527-rebase creates a new branch, but gh pr edit 527 --base dev only changes the target branch; PR #527 still points at the old head, so the rebased commits won’t be the ones reviewers and CI are attached to. Either push the rewritten commits to the branch that backs #527, or close #527 and open a new PR from codex/pr527-rebase, then update the later gh pr view/checks 527 steps to the new PR number.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260728_bug_bundle_resolution/040_pr527_rebase.md` around lines
127 - 143, Update the PR rebase plan so it changes PR `#527`’s head as well as its
base: either force-push the rebased commits to the branch currently backing
`#527`, or explicitly close `#527` and create a replacement PR from
codex/pr527-rebase. If creating a replacement, revise all later gh pr
view/checks references and related instructions to use the new PR number.
| The local port does not have to match the remote one. opencodex treats any request whose | ||
| `Host` resolves to `localhost`, `127.0.0.1`, or `::1` as loopback regardless of port, so | ||
| `http://localhost:20100/v1` works for Codex CLI, Claude Code, the dashboard, and `curl`. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Describe hostname matching as textual validation, not DNS resolution.
isLoopbackHostname normalizes and compares exact hostname forms; it does not resolve DNS. Saying the Host “resolves to” localhost could imply that an attacker-controlled name resolving to 127.0.0.1 is trusted, which contradicts the rebinding tests. Use wording such as “has a hostname of localhost, 127.0.0.1, or ::1.”
As per path instructions, user-facing documentation must describe the exact hostname-based behavior implemented by src/server/auth-cors.ts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs-site/src/content/docs/reference/configuration.md` around lines 179 -
181, Update the documentation sentence describing loopback handling to say that
the Host has a hostname of localhost, 127.0.0.1, or ::1, rather than saying it
resolves to those values. Preserve the existing statement that any port is
accepted and the listed clients work.
Source: Path instructions
| const onData = (chunk: Buffer | string) => { | ||
| const key = typeof chunk === "string" ? chunk : chunk.toString("utf8"); | ||
| if (key === KEY_INTERRUPT) return finish(false, true); | ||
| if (key === KEY_ESCAPE) return finish(false, false); | ||
| if (KEY_ENTER.has(key)) return finish(yes, false); | ||
| const lower = key.toLowerCase(); | ||
| if (lower === "y") return finish(true, false); | ||
| if (lower === "n") return finish(false, false); | ||
| if (KEY_YES_SIDE.has(key)) { | ||
| yes = true; | ||
| paint(); | ||
| return; | ||
| } | ||
| if (KEY_NO_SIDE.has(key) || key === KEY_TAB) { | ||
| yes = key === KEY_TAB ? !yes : false; | ||
| paint(); | ||
| return; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files first.
git ls-files src/cli/interactive-confirm.ts tests/interactive-confirm.test.ts
# Show a compact outline of the implementation file.
ast-grep outline src/cli/interactive-confirm.ts --view expanded
# Show a compact outline of the test file.
ast-grep outline tests/interactive-confirm.test.ts --view expanded
# Read the relevant sections with line numbers.
sed -n '1,220p' src/cli/interactive-confirm.ts | cat -n
printf '\n--- TESTS ---\n'
sed -n '1,220p' tests/interactive-confirm.test.ts | cat -nRepository: lidge-jun/opencodex
Length of output: 10816
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node <<'JS'
const { PassThrough } = require('node:stream');
function probeWrites(label, writes) {
const pt = new PassThrough();
const chunks = [];
pt.on('data', c => chunks.push(c.toString('utf8')));
for (const w of writes) pt.write(w);
pt.end();
console.log(label, JSON.stringify(chunks));
}
probeWrites('single-arrow', ['\x1b[D']);
probeWrites('split-arrow', ['\x1b', '[D']);
probeWrites('two-writes', ['a', 'b']);
probeWrites('three-writes', ['\x1b', '[', 'D']);
JSRepository: lidge-jun/opencodex
Length of output: 264
🌐 Web query:
Node.js raw mode escape sequence chunking readline keypress timeout ESC split bytes
💡 Result:
In Node.js, managing input from a terminal in raw mode involves handling ANSI escape sequences that may be fragmented across multiple data chunks [1][2]. When a terminal sends an escape sequence—such as an arrow key (e.g., \x1b[D)—it may arrive in separate packets (e.g., \x1b followed by [D), potentially causing incorrect processing if the receiver assumes the sequence is complete within a single read [1]. Node.js addresses this through the readline module's emitKeypressEvents function, which implements a streaming parser to reconstruct these sequences [3][4]. Key technical considerations include: 1. Escape Code Timeout: When the parser receives an ambiguous sequence—such as a lone ESC (\x1b) character—it cannot immediately determine if it is a standalone Escape key press or the beginning of a longer ANSI sequence [4]. It waits for a specific duration, defined by the escapeCodeTimeout (default 500ms), for additional bytes to arrive before emitting a keypress event [5][4]. This delay ensures that valid multi-byte sequences are correctly identified rather than being split into multiple, separate events [2][6]. 2. Configuration: You can control this behavior using the escapeCodeTimeout property when creating a readline interface [4]. For scenarios requiring high-responsiveness where the standard 500ms delay is undesirable, users have requested more granular control, such as setting the timeout to zero, though current implementations generally rely on the interface-level setting [5][4]. 3. Limitations: While the built-in parser handles standard terminal navigation sequences, it does not support all possible ANSI escape codes [3][7]. For instance, complex sequences like Terminal Cursor Position Reports (\x1b[6n) may be partially misinterpreted or flow into the input stream as regular strings if the response exceeds the parser's expected buffer length [7]. For applications requiring robust, arbitrary ANSI sequence parsing beyond what readline provides, external streaming parser libraries are recommended to manage stateful buffering across chunks [8]. Top results: [3][2][6][5][4][7]
Citations:
- 1: readline: splitted escape sequences are processed incorrectly nodejs/node#1403
- 2: nodejs/http2@aed6bce
- 3: readline: turn emitKeys into a streaming parser nodejs/node#1601
- 4: https://nodejs.org/dist/latest-v22.x/docs/api/readline.html
- 5: Add a way to set the escape timeout directly to stdin nodejs/node#63990
- 6: nodejs/node@3881189
- 7: Module "node:readline" doesn't address escape sequence of cursor position report from terminal, causing part of it to flow into input stream as regular string nodejs/node#57602
- 8: https://github.com/danneu/ansi-stream-parser
Buffer escape sequences before treating \x1b as cancel.
src/cli/interactive-confirm.ts:104-122 assumes each 'data' chunk is a complete key. In raw mode, \x1b can arrive before the rest of an arrow-key sequence (\x1b[D / \x1b[C), so this path can resolve the prompt as Escape instead of moving the selection. Keep a small buffer or use readline’s keypress parser so bare Escape is distinguished from split escape sequences.
📍 Affects 2 files
src/cli/interactive-confirm.ts#L104-L122(this comment)tests/interactive-confirm.test.ts#L28-L34
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli/interactive-confirm.ts` around lines 104 - 122, Update the onData
handler in interactive-confirm.ts to buffer raw input and distinguish a bare
Escape from split arrow-key sequences such as \x1b[D and \x1b[C before calling
finish(false, false); preserve the existing selection updates for left/right
input. Add or update coverage in tests/interactive-confirm.test.ts lines 28-34
to verify split escape sequences move the selection and bare Escape still
cancels.
| /** | ||
| * Whether `gh` is both installed and logged in. Starring goes through the | ||
| * user's own `gh` auth, so an unauthenticated CLI cannot fulfil a "Yes" — in | ||
| * that case the prompt stays silent instead of asking for something it would | ||
| * then fail to do. | ||
| */ | ||
| function ghAvailable(): boolean { | ||
| const r = spawnSync("gh", ["--version"], { stdio: "ignore", timeout: 3000, windowsHide: true }); | ||
| return !r.error && r.status === 0; | ||
| const version = spawnSync("gh", ["--version"], { stdio: "ignore", timeout: 3000, windowsHide: true }); | ||
| if (version.error || version.status !== 0) return false; | ||
| const auth = spawnSync("gh", ["auth", "status"], { stdio: "ignore", timeout: 5000, windowsHide: true }); | ||
| return !auth.error && auth.status === 0; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File map ==\n'
git ls-files src/cli/star-prompt.ts tests/startup-prompt.test.ts
printf '\n== Outline of src/cli/star-prompt.ts ==\n'
ast-grep outline src/cli/star-prompt.ts --view expanded || true
printf '\n== Relevant lines in src/cli/star-prompt.ts ==\n'
sed -n '1,180p' src/cli/star-prompt.ts | cat -n
printf '\n== Relevant lines in tests/startup-prompt.test.ts ==\n'
sed -n '1,240p' tests/startup-prompt.test.ts | cat -n
printf '\n== Search for ghAvailable / isAgentDriven usage ==\n'
rg -n "ghAvailable|isAgentDriven|printAgentDeferral|auth status|marker" src/cli/star-prompt.ts tests/startup-prompt.test.tsRepository: lidge-jun/opencodex
Length of output: 11290
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,180p' src/cli/star-prompt.ts | nl -baRepository: lidge-jun/opencodex
Length of output: 197
🌐 Web query:
gh auth status generates api call graphql endpoint query current user
💡 Result:
Yes, the gh auth status command performs an API call to the GitHub GraphQL endpoint to verify authentication and retrieve information about the current user [1][2]. When you run gh auth status, the GitHub CLI iterates over your stored authentication tokens for each configured host [3][4][5]. To confirm these tokens are valid and to retrieve associated details, the command executes a query against the GitHub GraphQL API [1][2]. Technical details regarding this behavior include: 1. GraphQL Usage: The command uses a GraphQL query to identify the "viewer" (the current user) [1]. This allows the CLI to verify that the token is active and authorized [3][5]. 2. Scope Retrieval: While the GraphQL query identifies the user, token scopes are often extracted from the HTTP response headers (such as X-OAuth-Scopes) returned during this API interaction [1][2]. 3. Error Handling: If the GraphQL request returns a non-200 status code (e.g., 401 Unauthorized), the command identifies the token as invalid and reports an error [1][5][2]. This is part of the command's primary function to display the authentication state of each known account [3][4]. 4. Limitations with Fine-Grained Tokens: Because fine-grained personal access tokens have different support levels for the GraphQL API compared to classic PATs or OAuth tokens, the CLI may not always be able to determine or display scopes for these specific token types, sometimes resulting in warnings [2]. For more advanced or manual interaction with GitHub's GraphQL API, the gh api graphql command is the standard interface provided by the CLI [6][7].
Citations:
- 1: Show token scopes in
gh auth statuscli/cli#6520 - 2: show token scopes on auth status cli/cli#6546
- 3: https://cli.github.com/manual/gh_auth_status
- 4: https://github.com/cli/cli/blob/trunk/pkg/cmd/auth/status/status.go
- 5:
gh auth statusshould have a JSON output option cli/cli#8637 - 6: https://cli.github.com/manual/gh_api
- 7: https://github.blog/developer-skills/github/exploring-github-cli-how-to-interact-with-githubs-graphql-api-endpoint/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,200p' src/cli/agent-driven.ts | cat -nRepository: lidge-jun/opencodex
Length of output: 1682
Move the agent short-circuit before gh auth status in src/cli/star-prompt.ts:75-84. ghAvailable() calls gh auth status, which hits GitHub’s API; because isAgentDriven() runs later, every agent/CI ocx start still pays that network round trip even when the prompt is only being deferred. Split the installed check from the auth check, or skip the auth probe on the agent path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli/star-prompt.ts` around lines 25 - 36, Avoid calling gh auth status
for agent-driven runs by moving the isAgentDriven() short-circuit ahead of the
authentication probe in the star prompt flow. Update ghAvailable() or its caller
so gh --version may still determine installation, but the network-backed auth
check is skipped when the prompt is deferred for agents.
Summary
A proxy reached through
ssh -L 20100:localhost:10100was refused entirely, not partially.isLoopbackRequestHost()required theHostheader's port to equal the proxy's own port, so a forwarded request arriving asHost: localhost:20100failed the check.That gate is not CORS-only.
isAllowedRequestOriginis the admission check in front of/v1/models,/v1/responses,/v1/responses/compact,/v1/messages,/v1/chat/completions,/v1/liveand the Responses WebSocket upgrade, and on a loopback bind it rejects on theHostheader before looking atOrigin. Codex CLI, Claude Code andcurlsend noOriginat all, so the whole proxy looked dead rather than misconfigured.Loopback is a trust boundary by hostname, not by port. The sibling
isLoopbackOriginValue()dropped its own port check for exactly this reason in e4e0612 ("any http/https origin on localhost/127.0.0.1/::1 is treated as same-trust-boundary"); only theHostside was left on the old rule.Why the port check was not a defense
A DNS-rebinding browser connects to the proxy's real port and sends that port verbatim in
Host. The port comparison therefore always succeeded for the attack it looked like it was stopping —isLoopbackHostnameis what rejected it, and still does.Verified against 40
Hostforms. All still refused:WHATWG normalization folds
127.1,2130706433,0x7f000001and[0:0:0:0:0:0:0:1]to genuine loopback, which is correct rather than a bypass.Non-loopback binds are untouched: they take the
isApiAuthRequiredbranch, which enforces token auth and refuses startup withoutOPENCODEX_API_AUTH_TOKEN.Also fixed
isLoopbackHostnamenow strips a single trailing dot, so the FQDN formcurl http://localhost.:20100/is accepted. Same class of bug — a legitimate loopback caller refused on spelling.Tests
This predicate had no test coverage at all before this PR. The new file pins both directions, including a characterization test for the pre-existing fail-open on an unparseable
Host(if (!parsed) return true), so tightening that later requires a deliberate failing test rather than silent drift. That fail-open is unchanged here and is not browser-reachable — a browser composesHostfrom its own connection.Activation evidence, not just a green suite:
Full run:
tests/server-auth.test.ts:582("loopback management API rejects host-header same-origin rebinding") was treated as the must-not-break oracle and passes.Docs
configuration.mdgains an SSH port forwarding subsection under Remote access, with three caveats that were easy to get wrong:ssh -g -L, container publishing and some devcontainer/Codespaces modes bind the client side to0.0.0.0and expose both the management API and the data plane with no credentialocxonly ever writes127.0.0.1with the local default port)Review
src/server/auth-cors.tsis under.github/CODEOWNERSas an authentication boundary, so this needs explicit security review perMAINTAINERS.mdand should not self-merge. An independent adversarial audit was run against the change (40-vector Host probe, every call site of the predicate, WS upgrade path,corsHeadersbehavior) and returned GO-WITH-FIXES; all of its findings are folded into this commit. That audit is a technical check, not a substitute for maintainer review.Planning and evidence:
devlog/_plan/260728_bug_bundle_resolution/010_ssh_loopback_gate.md.Summary by CodeRabbit
New Features
Bug Fixes
Tests