feat(hardening): phase 6 — key rotation, adversarial gates, seven-locale docs - #2789
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
|
![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: 207254fe01
ℹ️ 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".
| const restored = restoreTokenBackup(connection.pendingOperation.oldKeyBackupPath); | ||
| await abortClientKeyRotation(connection.managementUrl, authority, connection.apiKeyId, started.rotationId, { fetchImpl: deps.fetchImpl }); |
There was a problem hiding this comment.
If a failure occurs after the new token is installed and the subsequent abort request fails transiently, these lines restore the old token before leaving the pending marker in place. On the next recovery, both the current file and .prev therefore contain the same accepted old token; recoverRotationWithAuthority interprets both successful probes as distinct old/new candidates, commits the pending new key, and then records the old fingerprint locally, locking the client out. Abort before restoring, or persist enough identity to distinguish duplicate old-token files during recovery.
AGENTS.md reference: src/AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
| if (backup.kind === "present" && current.kind === "present") { | ||
| try { | ||
| removeOrphanTokenBackup(); | ||
| return { kind: "orphan-cleaned" }; |
There was a problem hiding this comment.
When ocx connect status runs while rotateConnectedClientKey is awaiting /api/keys/rotate—after writeTokenBackup but before the pending marker is persisted—this inspection sees an apparent orphan and deletes it. Rotation can then install the new token and marker, but any later probe or commit failure becomes unrecoverable because .prev is gone. Serialize cleanup with rotation or persist a pre-request intent marker before making the backup visible.
AGENTS.md reference: src/AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
| const responseBody = method === "HEAD" || !upstream.body | ||
| ? null | ||
| : boundedRelayResponseStream(upstream.body, HUB_RELAY_RESPONSE_BODY_MAX_BYTES, signal); |
There was a problem hiding this comment.
For a chunked upstream response without Content-Length, this now returns the upstream status immediately and discovers the 16 MiB overflow only while the caller consumes the stream. The caller consequently observes (for example) a 200 followed by a partial-body stream error instead of the relay's established 502 JSON failure, which breaks management clients expecting a complete JSON response and stable relay error mapping. Buffer the bounded body before constructing the response, or otherwise avoid exposing the upstream status until the size check succeeds.
AGENTS.md reference: src/AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
리뷰 · 우선순위 63 / 80이 PR은 remote-hub 스택 phase 6(마지막 hardening)입니다. 베이스는 클라이언트 키 회전은 보안 방향은 맞습니다. 회전 비밀을 argv/로그에 안 남기려는 테스트가 있고, orphan backup 정리가 있습니다. 다만 지금 CI는 빨강입니다. 라인 src/server/management/api-key-rotation.ts - start/commit/abort, TTL, timingSafeEqual, pendingRotation. 라인 src/lib/service-secrets.ts - writeTokenBackup/restoreTokenBackup/removeOrphanTokenBackup. 라인 src/cli/connect.ts / src/client/connect.ts - ocx connect rotate, pendingOperation 복구. 라인 gui/.../ApiKeys*.tsx / api-keys-utils.ts - start/commit/abort UI, 일회 비밀 표시. 라인 src/client/hub-relay.ts / tests/client-hub-relay.test.ts - SSRF·헤더 밀수·취소 전파. 라인 src/types/config.ts / src/config.ts - pendingRotation 등 스키마. 분할 캠페인 면적 증가. 라인 docs-site/.../guides/remote-hub.md (+7 locales) - remote-hub 가이드 번역·nav. 경로 base - #2786(p5) 위. p4 draft·p5 CI/ready 전이면 이 PR도 대기입니다. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
The hardening scope is valuable, but this cannot receive a final security approval as an intermediate Draft stack head. Exact head 207254fe012406aca66c90a1370f773ec0ceace0 targets codex/remote-hub-p5; the repository integration policy requires dev (or scoped dev2-go), and Phases 1-5 are not all landed. The current 95-file diff therefore includes inherited stack state rather than only the Phase 6 change. Resolve the predecessor chain, retarget/rebase the final Phase 6 delta onto current dev, complete the readiness gates, and rerun exact-head cross-platform/runtime/GUI CI. Then re-request non-author security review of rotation crash recovery, token backup ownership and cleanup, one-time secret display, session invalidation, pairing rate limits, relay SSRF/header stripping, and cancellation. No merge or maintainer-sponsored label should be applied to this intermediate head.
a62c8eb to
8bcfcaa
Compare
207254f to
ff29132
Compare
8bcfcaa to
9557186
Compare
ba65041 to
08f0bf1
Compare
9557186 to
ff3ce26
Compare
08f0bf1 to
b6aa976
Compare
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Requesting changes on exact head 4ab13b4635d336a48a5d5fac9aee29d85b289f99.
The abort-before-restore ordering fixes the earlier token-identity failure, but two current-head recovery/relay blockers remain:
-
inspectClientRotationRecoveryGate()re-readspendingOperation, but rotation writes.prevbefore that marker. A concurrentocx connect statusin the pre-marker window still sees current+backup with no pending operation and deletes the live backup as an orphan. The reread cannot observe a marker that has not been persisted yet. Serialize across processes with the same coordinator/rotation lock, or persist a durable pre-request intent before exposing the backup; add a deterministic concurrent status/rotate regression. -
For chunked hub responses, the relay returns the upstream status and a bounded stream immediately. If the stream later exceeds 16 MiB, the caller has already observed a 200 and partial body instead of the relay contract's stable 502 JSON error. Buffer through the bounded reader before exposing the response status, or otherwise withhold the upstream response until the limit is proven.
This phase remains Draft, enforce-target is red for the required GUI screenshot, and every parent is still blocked. Keep the exact head unapproved until these findings and the inherited chain are resolved.
ff3ce26 to
072cc29
Compare
4ab13b4 to
842bfc9
Compare
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Requesting changes on exact head 2349d39e8afde25043a14c69b1da91f5a11be3dd.
The abort-before-restore ordering is preserved, but the two remaining recovery/relay blockers are not fixed:
rotateConnectedClientKey()still writes.prevatsrc/client/connect.ts:276, starts the remote rotation, and only persistspendingOperationat lines 279-290.inspectClientRotationRecoveryGate()re-reads state atsrc/client/state.ts:117, but if status lands before that marker exists it still callsremoveOrphanTokenBackup()at line 121. The comments acknowledge the remaining window while the code still deletes inside it. Serialize status cleanup with rotation, or persist durable intent before exposing.prev; add a deterministic cross-process status/rotate regression.src/client/hub-relay.ts:280-287still returns the upstream status withboundedRelayResponseStream()immediately. A chunked response that crosses 16 MiB therefore exposes 200 plus a partial body before the limit failure, rather than the relay contract stable 502 JSON response. Buffer through the bounded reader before constructing the response, or otherwise withhold status until the limit is proven; cover chunked overflow at the public response boundary.
This head is still Draft, with macos, enforce-target, and aggregate ci red, and it inherits blocked parents. Keep it unapproved until the code paths and exact-head gates are resolved.
…an in-flight backup Two rotation defects, plus the client-side reconciliation this phase needs after the earlier contract changes. Abort before restore. The rollback path restored the local token file and then asked the hub to abort the pending rotation. When that abort failed transiently, the process held the old key locally while the hub still had a pending rotation for the new one — two sides disagreeing about which generation is current, surfaced only as "rollback was incomplete". The hub is the authority on which generation is live, so it is asked first and the local file is rewound only after it agrees. On failure both candidates and the pending marker stay on disk, because recovery genuinely cannot tell which generation wins without asking. A concurrent status no longer deletes a live backup. The orphan-cleanup branch fires on "backup present, token present, no pending marker", which is exactly what `ocx connect status` sees if it lands in the window where rotateConnectedClientKey has written .prev but has not yet persisted pendingOperation. It deleted the rollback target the in-flight rotation was relying on. The gate now re-reads persisted state — the caller's snapshot may predate the marker — and declines to clean while a rotation is recorded. Client catalog reconciliation. This phase adds schema validation and the x-opencodex-key-id echo on top of a conditional-fetch path that Phase 1 (D2) removed, so the validator handling is dropped and the additions are kept: no If-None-Match, no ETag in the result, and any 304 is catalog_unexpected_304. That refusal is strictly wider than the two cases it replaces (catalog_304_without_lkg, catalog_etag_mismatch), both of which required a conditional request the client no longer makes. The rotate path also still carried --allow-insecure-http; removed for the same reason as the connect path. tests/loopback-listener-admission.test.ts takes this phase's fix for the ingress role assertion — supplying a complete client connection so the ingress rule is what refuses — over the narrower split made while rebasing p5.
tests/management-route-registry.test.ts reconciles the declared registry against source and found four routes this phase serves but never declared: POST, POST /commit and DELETE on /api/keys/rotate, plus POST /api/session/logout. The rotate trio are ordinary management mutations and are declared as such. /api/session/logout carries a session-only exemption with its reason. It ends the CURRENT gui-session and requires that session's own Origin and CSRF, so there is nothing for a CLI verb to act on: the CLI holds an admin token, and this route refuses the admin token precisely so it cannot end a consent session it never established. Giving it a CLI surface would mean inventing one.
Declaring the three rotate routes in the registry surfaced the other half of that contract: tests/cli-capabilities.test.ts requires every management route to be capability-covered, exempt, or in the dated ratchet. These were none of the three, and unlike POST /api/session/logout they have a real CLI driver, so an exemption would have been a false claim. One capability covers all three routes because they are one operation: start returns the new secret once, commit promotes it, abort unwinds a rotation that could not be confirmed. Splitting them into separate verbs would imply an operator can stop halfway, which is the state the recovery gate exists to prevent. skills/ocx/references/01_management_surface.md is regenerated, since it is derived from this table.
Two surfaces this stack added still rendered for a user who never enabled remote hub. Key rotation was offered on every API key. It is a connected-client operation: it swaps the data key this machine uses against its hub, through a commit/abort handshake the hub arbitrates. A standalone install has no hub to rotate against, so the control both advertised remote hub to someone who never turned it on and would have driven a handshake with nothing on the other end. The handlers are passed only for a connected runtime; without them the section does not render. The usage source row read "Source: local usage.jsonl" on standalone. Naming the source answers "which store served these numbers", and that question only exists once there are two. On a standalone install there is exactly one, so the line said nothing the page did not already imply while still making the user read past a sentence about topology. It renders only when connected, alongside the machine/hub scope toggle it belongs with.
072cc29 to
76257cc
Compare
2349d39 to
efefe36
Compare
2 participants
Summary
devlog/_plan/260827_remote_hub/080_phase6_hardening.md).ocx connect rotatewith a persistedpendingOperationrecovery marker,<tokenfile>.prevbackup owned by the service-secrets module (writeTokenBackup/restoreTokenBackup/removeOrphanTokenBackup), commit-on-doubly-accepted crash recovery, and orphan-backup cleanup for the crash window before marker persistence.ocx access key rotate, GUI ApiKeys start/commit/abort with one-time secret display), session logout/invalidations, pairing rate limits.UI change
The management-side key rotation is the visible surface of this phase. Screenshot from the phase-6 head, taken against a live proxy with a key in the pending-rotation state:
What the shot documents: the
키 교체section appears only while a rotation is pending, states that the new key must be applied to clients and verified before commit, shows the cutover deadline, and offers exactly two exits —새 키로 확정(commit) and키 교체 취소(abort). The one-time secret is displayed at issuance and is not re-rendered here, which is the intended behavior. Locale shown is Korean; the same strings ship for all supported locales.Verification
Run on lidge-ai (remote CI host) at 207254f, per the no-local-suite policy:
bun run typecheck— clean.bun run test— full suite, exit 0, zero failing tests.cd gui && bun run test— 1098 pass, 0 fail;bun run build— clean;bun run lint:gui— 0 warnings, 0 errors.bun run privacy:scan— the only finding is the pre-existingdevlog/_plan/260827_release_train/020_preview_release.md:36hit already on dev; zero findings from this stack.Checklist