Skip to content

fix(link): strip provider credentials at relay - #6034

Closed
Ingwannu wants to merge 1 commit into
devfrom
fix/6032-link-relay-credentials
Closed

Ingwannu wants to merge 1 commit into
devfrom
fix/6032-link-relay-credentials

Conversation

@Ingwannu

@Ingwannu Ingwannu commented Sep 27, 2026 •

Copy link
Copy Markdown
Owner

Summary

  • strip Azure api-key and Google x-goog-api-key alongside the existing caller credential headers before Child → Home forwarding
  • preserve link admission, ordinary metadata such as idempotency-key, framing rules, and /v1/usage dedicated-key behavior
  • cover the pure helper, actual fetch, usage route, mixed-case names, and real machine-listener socket
  • document the explicit named-header boundary and rationale

Closes #6032.

Security boundary

The Home is a distinct credential owner. A linked Child replaces caller credentials with the link key and uses the Home's accounts. Before this patch, built-in Azure/Google credential forms were not in the explicit denylist and survived forwarding. This is a conditional paired-machine disclosure, not an unauthenticated Internet path.

The fix stays explicit rather than using a broad *key* heuristic, which could incorrectly remove protocol metadata such as idempotency-key. Custom authentication header names still require deliberate review when introduced.

Validation

All local commands ran one at a time in transient user units with CPUQuota=75%, MemoryHigh=1G, MemoryMax=1536M, swap disabled, and disposable HOME/CODEX_HOME/OPENCODEX_HOME/TMPDIR; lightweight gates used 50% CPU and 512 MiB max.

  • bun test tests/clients/client-link-relay.test.ts — 23 pass, 0 fail
  • bun run structure:check — pass
  • bun run privacy:scan — pass
  • bun scripts/file-size-ratchet.ts — pass
  • git diff --check — pass
  • independent pre-patch source→sink investigation and fresh post-patch bypass review — no remaining blocker

No full suite/build, live service restart, production-home command, or credential was used.

Summary by CodeRabbit

  • Security
    • Caller-supplied Azure, Anthropic-compatible, and Google API-key credentials are no longer forwarded through the remote link to Home. The relay continues to use the link key for authorization, and /v1/usage forwards only its dedicated OpenCodeX key.
  • Documentation
    • Updated remote-link security guidance to clarify which credentials are not forwarded and that Home serves requests using its own accounts.

@Ingwannu
Ingwannu requested a review from lidge-jun as a code owner September 27, 2026 02:11
@Ingwannu

Copy link
Copy Markdown
Owner Author

@lidge-jun security-boundary review requested on exact head bb5cb63453. The merged #5998 leak is covered through helper, outbound fetch, /v1/usage, case normalization, and real machine-listener tests; focused local validation and an independent post-patch bypass review are green. Awaiting hosted exact-head CI before merge consideration.

@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Sep 27, 2026
@coderabbitai

coderabbitai Bot commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: db2b0045-ab96-445d-b26b-0a0d6eacccc0

📥 Commits

Reviewing files that changed from the base of the PR and between d25f972 and bb5cb63.

📒 Files selected for processing (5)
  • docs-site/src/content/docs/guides/remote-link.md
  • src/client/link-relay.ts
  • structure/decisions/ADR-6032-link-relay-credential-boundary.md
  • structure/remote-link.md
  • tests/clients/client-link-relay.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.


📝 Walkthrough

Walkthrough

The Child link relay now removes caller-supplied Azure api-key and Google x-goog-api-key headers before forwarding requests to the Home. Tests cover header construction and the actual Home listener. Documentation and ADR-6032 describe the credential boundary.

Changes

Link relay credential boundary

Layer / File(s) Summary
Filter credentials and verify relay forwarding
src/client/link-relay.ts, tests/clients/client-link-relay.test.ts, structure/decisions/ADR-6032-link-relay-credential-boundary.md, structure/remote-link.md, docs-site/src/content/docs/guides/remote-link.md
The relay denylist now omits caller api-key and x-goog-api-key headers. Tests check that these headers do not reach the Home for regular requests or /v1/usage, while the link key remains in use and Idempotency-Key is forwarded. The ADR and documentation describe the filtering and credential ownership.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to bb5cb

The relay keeps the named provider credentials on the Child while preserving the link key and ordinary request metadata. No identified issue prevents merging, subject to normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to bb5cb

The change reduces credential exposure for linked machines without expanding relay access. The remaining uncertainty is whether every deployment path uses the inspected relay and whether future provider credential names will receive the same protection.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The affected exposure is a caller able to supply headers to a linked Child request that reaches its paired Home, not a new unauthenticated Internet entrypoint. The Home is a separate credential owner.

Trust Boundaries and Controls

  • observed — Caller credential headers are removed before the network fetch and replaced with the link admission credential. The inspected production route and exported helper share this header policy.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (3 skipped: 3 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: the link relay now strips provider credentials before forwarding requests.
Linked Issues check ✅ Passed Issue #6032 coding requirements are met. src/client/link-relay.ts adds api-key and x-goog-api-key to the explicit CALLER_CREDENTIAL_HEADERS denylist. linkRequestHeaders combines that denylis…
Out of Scope Changes check ✅ Passed The reviewed changes stay within issue #6032. The source change blocks the two provider credential headers. The relay tests verify the helper, outbound fetch, /v1/usage, mixed-case names, and listen…
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (3 skipped: 3 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@Ingwannu

Copy link
Copy Markdown
Owner Author

@lidge-jun exact security-fix head bb5cb63453b5105e58bb0876d6e00c26646fe995 is fully green in Cross-platform CI 36287811505, including all four test shards, gates, docs/structure, packaging, desktop shell, and aggregate ci. Pre-patch boundary investigation, focused real-listener tests, and fresh post-patch bypass review are all complete with no unresolved thread. Maintainer review/approval can proceed.

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 42 / 80

이 PR은 Child가 Home으로 요청을 넘기기 전에, 호출자가 붙인 Azure 키와 Google 키를 빼 버립니다.

Child와 Home은 서로 다른 컴퓨터입니다. Child는 링크 열쇠만 보여 주고, Home은 자기 계정으로 답을 합니다. 예전 목록은 authorization, x-api-key, x-opencodex-api-key, chatgpt-account-id, cookie였습니다. Azure 어댑터가 쓰는 api-key와 Google 어댑터가 쓰는 x-goog-api-key는 그 목록에 없어서 Home까지 갔습니다. #6032가 그 구멍을 적었습니다.

지금은 그 두 이름이 목록에 있습니다. 비교 전에 이름을 소문자로 바꾸므로 Api-Key와 X-Goog-Api-Key도 빠집니다. 링크 열쇠는 그대로 붙습니다. 보통 요청은 Authorization: Bearer이고, /v1/usage만 x-opencodex-api-key입니다. Idempotency-Key는 남습니다. 테스트는 헤더를 만드는 함수, 실제로 보내는 fetch, /v1/usage, 진짜 소켓 리스너를 봅니다. 영어 안내와 ADR-6032도 같이 고쳤습니다. 베이스는 dev입니다. 같은 구멍을 고치는 다른 열린 PR은 없습니다. 헤드 bb5cb63453에서 test 1/4부터 4/4, gates, structure gate는 통과했습니다.

라인 - docs-site/src/content/docs/ko/guides/remote-link.md 63행. 영어 안내 docs-site/src/content/docs/guides/remote-link.md는 Bearer, Azure api-key, Anthropic x-api-key, Google x-goog-api-key를 이름까지 적습니다. 한국어 63행은 "인증 정보는 Home으로 전달되지 않으며"만 말합니다. 그 문장은 이번 수정 뒤에도 맞습니다. 영어처럼 헤더 이름은 없습니다.

메인테이너의 판단이 필요한 지점

다음 제공자가 다른 헤더 이름으로 키를 보내면, 그 이름은 아직 통과합니다. ADR-6032는 이름에 key가 있으면 전부 지우는 방식을 거절했습니다. Idempotency-Key까지 빠지기 때문입니다. 다음 키는 목록에 이름을 더하는 쪽으로 둘지입니다.

주소의 물음표 뒤도 터널로 넘어갑니다. src/client/link-relay.ts의 linkRelayDestination이 url.search를 그대로 붙입니다. 지금 Azure와 Google 어댑터는 키를 헤더에 넣습니다. 물음표 뒤를 이번 목록에 넣을지는 이번 이슈 밖입니다.

너의 추천

머지하세요. #6032를 닫으면 됩니다. 베이스는 dev로 두세요. 닫을 중복 PR은 없습니다. types.ts와 config.ts 분할과 겹치지 않습니다. 한국어 63행에 헤더 이름을 보태는 일은 머지 다음이어도 됩니다.

이 댓글은 grok-bot이 작성했습니다

@lidge-jun

Copy link
Copy Markdown
Owner

Landed on dev in #6059 (merge 8923ad9835) as one squashed commit that keeps your authorship. Thank you. Closing because this repository merges into dev, so GitHub does not close carried PRs automatically.

@lidge-jun lidge-jun closed this Sep 27, 2026
mdwsk88 pushed a commit to mdwsk88/opencodex that referenced this pull request Sep 27, 2026
Carried from lidge-jun#6034 into merge train round 3.

Co-authored-by: Ingwannu <ingwannu@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants