feat(remote-gui): phase 2 — remote GUI sessions, pairing, and management CORS - #2776
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
UI screenshot waived by the |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7099760a5c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!Number.isFinite(declaredLength) || declaredLength > GUI_PAIRING_EXCHANGE_BODY_LIMIT) { | ||
| return withManagementCors(Response.json({ error: "pairing exchange body too large" }, { status: 413, headers: { "Cache-Control": "no-store" } }), req, config); | ||
| } | ||
| const text = await req.text(); |
There was a problem hiding this comment.
When an unauthenticated caller omits Content-Length or uses chunked transfer encoding, declaredLength defaults to zero and req.text() buffers the entire request before enforcing the 4 KiB limit. The server-wide maxRequestBodySize is 256 MiB, so concurrent requests to this public bootstrap endpoint can cause very large allocations or process exhaustion; reject unsupported framing or read the stream through a 4 KiB bounded reader before materializing it.
Useful? React with 👍 / 👎.
|
|
||
| export interface OcxHubConfig { | ||
| /** Canonical browser-reachable management origin advertised by a hub. */ | ||
| managementPublicOrigin?: string; |
There was a problem hiding this comment.
This adds the user-facing hub.managementPublicOrigin, remoteGui.allowedTailscaleUsers, remoteGui.allowInsecureHttp, and ocx gui pair surfaces, but a repo-wide check of docs-site/ finds no instructions explaining how to configure or safely use them. Add operator documentation for the supported deployment, pairing exchange, origin allowlist, and insecure-HTTP implications so users are not left to infer a security-sensitive workflow from TypeScript declarations and CLI help alone.
AGENTS.md reference: src/AGENTS.md:L28-L28
Useful? React with 👍 / 👎.
리뷰 · 우선순위 63 / 80이 PR은 원격 허브 스택 2단계입니다. 베이스는 #2772( 실제 원격 발급 경로는 페어링입니다. 페어링 쪽은 조심히 짜여 있습니다. grant는 sha-256 digest만 저장하고, 소비는 겹치는 PR이 있습니다. #2414는 라인 src/server/index.ts trustedTailscaleIngress: false - 프로덕션 호출 두 곳 모두 false입니다. Tailscale 자동 세션 코드는 테스트에서만 true가 됩니다. 이 PR의 원격 경로는 pairing뿐입니다. 라인 src/server/gui-session.ts isRemoteGuiBrowserOriginAllowed - 라인 src/server/gui-session.ts REMOTE_GUI_SESSION_TTL_MS - 12시간이고, 인증 성공할 때마다 슬라이딩으로 연장됩니다. 루프백 5분과 다릅니다. 만료를 고정할지, 이 길이를 줄일지는 운영 판단입니다. 라인 src/server/gui-session.ts mintSession expiresAt getter/setter - 반환 객체의 setter가 맵 안 세션을 바꿉니다. 슬라이딩 TTL을 위한 연결이지만, 호출부가 만료를 직접 만지면 한 세션이 두 곳에서 고쳐집니다. 라인 src/server/index.ts POST /opencodex-session - 예전에는 GET 루프백 부트스트랩만 있었습니다. 이제는 POST가 항상 이 경로를 먹고, 실패면 401입니다. GUI fallback으로 떨어지지 않습니다. 의도에 맞습니다. 경로 src/config.ts hub/remoteGui - 키가 없어도 기본 동작은 바이트 동일해야 합니다. 경로 #2414 - 같은 문제를 쿠키 세션으로 풀려는 draft입니다. 허브 스택을 가기로 하면 저 PR은 superseded입니다. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
This is a security-boundary change and remains worth reviewing, but I cannot approve it in its current integration state. Exact head 7099760a5c236c921a8852578d8003a172fa593e is Draft and targets codex/remote-hub-p1, while the repository policy accepts integration PRs only against dev or scoped dev2-go. Its parent Phase 1 PR #2772 and stack base #2771 are still open, so this diff is not yet the exact code that would land. Land or otherwise resolve the predecessors, retarget/rebase the resulting Phase 2 delta onto current dev, mark the readiness checklist complete, and rerun exact-head cross-platform CI. Then re-request the non-author security review for the remote-session issuance, origin binding, CSRF/CORS, pairing TTL/replay, and admin-token consent boundaries. No merge or sponsorship should happen before that.
c10ef21 to
07d7f10
Compare
7099760 to
b728285
Compare
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Re-reviewing exact head b7282858b8f51fa1e79d5fee07605f17298d3da7 on the current stacked Phase-1 base.
The implementation has removed the intended allowInsecureHttp authorization effect and the positive HTTP test now refuses, but one transport-proof bypass remains and blocks this auth surface:
-
consumeGuiPairingGrant()checksisPairingTransportPermitted(managementRequestOrigin(req, config))atsrc/server/gui-session.ts:243-280. For a non-loopback Host,managementRequestOrigin()returns configuredhub.managementPublicOriginbefore looking at the request URL (src/server/auth-cors.ts:120-150). Therefore a hub configured withhttps://hub.examplewill classify a direct plaintexthttp://hub.example/opencodex-sessionrequest as HTTPS. The reusable grant can cross the public HTTP listener and mint a reusable session even though the new comments claim plaintext is impossible. A configured public origin is routing metadata, not proof that this request traversed a trusted TLS terminator.Thread an explicit trusted-ingress/observed-transport fact to the exchange, sourced only from an HTTPS origin or the dedicated loopback management/Tailscale ingress that actually terminates authenticated TLS. The ordinary public listener must pass false. Add a live/request-level regression using an
http:Request plus non-loopback Host and configured HTTPS public origin; it must refuse before looking up/consuming the grant, while the trusted loopback-terminator case remains constructible. -
The retired option is still part of the strict persisted schema/type (
src/config.ts:898-912,src/types/config.ts:254-268) but no implementation emits the promised one-time warning or removes it on normalization. More importantly, the PR body still advertisesremoteGui.allowInsecureHttpas a working opt-in, while the exact code and corrected parent decision say it grants nothing. Update the description, config diagnostics/tests, and every decade-doc occurrence so no user or later phase reintroduces it. Accepting an old field solely to avoid whole-config failure is fine; presenting it as current configuration is not.
This PR also remains Draft, targets the stacked codex/remote-hub-p1 branch, and depends on #2772 plus #2771, both currently under CHANGES_REQUESTED. Do not sponsor or mark this phase Ready until the parent contract and Phase 1 are corrected, then rebase/retarget the actual landing delta onto current dev and rerun exact-head cross-platform CI. The existing digest storage, one-use deletion, origin equality, CSRF, and admin-token non-exchange direction remains valuable; this review is not rejecting that design.
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Re-reviewing exact head 2b36ad4961bd050010e4af7f6651fed8dc086982.
The new bounded reader fixes the unauthenticated chunked-body allocation finding, and the old allowInsecureHttp setting is now retired rather than granting plaintext pairing. The original transport-proof blocker is still present.
For a non-loopback Host, managementRequestOrigin() returns configured hub.managementPublicOrigin before consulting the request URL. A hub configured with https://hub.example therefore classifies a direct plaintext http://hub.example/opencodex-session request as HTTPS. consumeGuiPairingGrant() feeds that configured value into isPairingTransportPermitted, so an on-path/direct HTTP request can still spend the reusable grant and receive a reusable session. Configuration is intended destination metadata, not evidence of observed TLS.
Bind pairing admission to transport evidence from the actual trusted ingress/listener. A direct HTTP socket must remain HTTP even when the advertised public origin is HTTPS; if a TLS terminator is supported, accept forwarded scheme only from an explicitly trusted ingress boundary. Add the negative direct-HTTP/configured-HTTPS regression and a positive trusted-proxy HTTPS case.
This phase is also Draft, enforce-target is red for the required GUI screenshot, the current operator-doc thread is unresolved, and it inherits unchanged Phase-1 #2772 where /readyz can still throw on an invalid Host. Keep this phase blocked until its own transport proof, screenshot/docs, and the parent blockers are resolved.
… exchange body Two defects the review raised on this phase, both on the unauthenticated pairing path. Plaintext pairing is removed rather than gated. consumeGuiPairingGrant issued an "insecure-http-pairing" session over non-loopback HTTP whenever remoteGui.allowInsecureHttp was true. A reusable grant on plaintext is readable by anything on the path and the session it mints is reusable, so the flag recorded a risk the operator could not bound rather than controlling one. A grant now crosses loopback or authenticated HTTPS only, and no configuration re-opens it. The scheme check also moved ahead of the grant lookup. It previously ran after the grant was found and validated, so a refused exchange still consumed a single-use code — an attacker who strips TLS termination could spend every code the operator prints without ever authenticating. Refusing first leaves the grant intact, which the regression test asserts by replaying the same unspent grant successfully over HTTPS. allowInsecureHttp stays in the schema, marked retired. The config schema is strict, so deleting the key would make an existing config file fail to load entirely; accepting and ignoring it is the smaller harm. The exchange body bound now holds against caller-chosen framing. The endpoint is reachable without a credential, and the pre-check read Content-Length, which the caller controls: omit the header and Number(null ?? "0") is 0, or send chunked and there is no header at all. Both passed the check and reached req.text(), which buffers to completion — an unauthenticated caller decided how much memory the process spent, and the post-check only measured a string it had already been forced to hold. The read now stops at limit+1 bytes and cancels the body rather than draining it. The regression test streams 512 KiB against the 4 KiB limit with no Content-Length and asserts the server pulled fewer chunks than were offered; it fails against the previous implementation. Also repairs the rebase of tests/cli-dispatch.test.ts and tests/cli-registry.test.ts, where dev and this phase appended different tests at the same place. Both sides are kept.
Tests that clear a rejected session reach the admin-token fallback, which calls window.prompt. happy-dom does not implement it, so those tests died on a TypeError instead of asserting the behavior they were written for. Most tests in the file never reach the fallback, which is why it stayed hidden. A null-returning stub is the honest stand-in: it means "the operator dismissed the prompt", which is the path these tests want. Installed only when prompt is genuinely missing, so a real implementation is never shadowed.
07d7f10 to
4fdd54d
Compare
2b36ad4 to
dad1712
Compare
lidge-jun
added
the
gui-screenshot-waived
|
Labeled #2789 is a different case and keeps the requirement: it touches |
2 participants
Summary
devlog/_plan/260827_remote_hub/040_phase2_remote_session.md, security-review-required per MAINTAINERS policy).serverOrigin/browserOriginand threads the full consumer chain (session bootstrap meta tags,gui/src/api.tsreaders, shared validation predicate inrequireManagementAuth/managementPrincipal).hub.managementPublicOrigin(operator-configured canonical origin;/readyzmanagementUrlprefers it behind TLS terminators).remoteGui.allowedTailscaleUsers, and pairing viaocx gui pair --origin <browser-origin>— single-use, 5-min-TTL, origin-bound, digest-stored grants exchanged atPOST /opencodex-session, rate-capped 8/min.remoteGui.allowInsecureHttpis the loudly-warned plain-HTTP tailnet opt-in.REMOTE_GUI_SESSION_TTL_MS); loopback keeps the existing 5-minute behavior.x-opencodex-gui-originandx-opencodex-csrf-tokenfor admitted origins.gui-session-only; the forged-loopback-Host refusal keeps its default-deny meaning.Verification
Run on lidge-ai (remote CI host) at 7099760, per the no-local-suite policy:
bun run typecheck— clean.bun test tests/server-management-auth.test.ts tests/sidebar-routes.test.ts tests/native-profile-route-security.test.ts tests/gui-management-session.test.ts tests/management-origin-tls.test.ts tests/server-auth.test.ts tests/core-lab-boundary.test.ts tests/startup-prompt.test.ts— 183 pass, 0 fail (P2-A01..A20: wrong server/browser origin, missing CSRF, expired/replayed grants, identity spoof on the public listener, admin-token exchange refusal, insecure-HTTP refusal without opt-in).Checklist