Skip to content

fix(cli): redact proxy credentials in config output - #5414

Closed
luvs01 wants to merge 3 commits into
lidge-jun:devfrom
luvs01:codex/fix-proxy-credential-leakage-in-cli
Closed

luvs01 wants to merge 3 commits into
lidge-jun:devfrom
luvs01:codex/fix-proxy-credential-leakage-in-cli

Conversation

@luvs01

@luvs01 luvs01 commented Sep 21, 2026 •

Copy link
Copy Markdown
Collaborator

Motivation

  • Provider-level proxy URLs may contain userinfo (user:password@host) and were not covered by the CLI's exact-key redaction, exposing proxy credentials in ocx config show|get|set output.
  • The management/dashboard DTO correctly redacts proxy, but the CLI used a separate key-list and therefore omitted it.

Description

  • Handle proxy as a URL-aware case in the CLI redactor (src/cli/config-command.ts): a URL carrying userinfo is masked via redactUrlForLog (userinfo/query/hash stripped, host and port kept for diagnostics); "direct", credential-less URLs, and empty values print unchanged; a non-URL non-"direct" value fails closed to ********.
  • config export intentionally keeps the raw file — it is a restore path, and masking it would lose the proxy password. The SECRET_KEYS comment now states this explicitly.
  • Extend tests/cli/cli-config-command.test.ts to cover credentialed, "direct", and credential-less proxy values across config show --json, config get, and JSON config set responses.
  • Document the masking contract in structure/config.md.

Testing

  • bun test tests/cli/cli-config-command.test.ts: 3 tests pass (19 assertions) on head acdbb1c.
  • bun run structure:check passed.
  • The full test run has unrelated environment-sensitive failures on this machine; the focused CLI test for this change passes under the repository Bun runtime.

Codex Task


Devin Review

Summary by CodeRabbit

  • Bug Fixes
    • Proxy URLs containing usernames or passwords are now masked in configuration display and update output.
    • Proxy hosts and ports remain visible, while invalid proxy values are fully masked.
    • Credential-free proxy URLs and the direct setting remain readable.
  • Documentation
    • Clarified that configuration exports retain raw values and that URL credentials require explicit handling.

@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 21, 2026
@coderabbitai

coderabbitai Bot commented Sep 21, 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: 95d0125f-8350-45ec-b392-b4c5255b2eec

📥 Commits

Reviewing files that changed from the base of the PR and between 1a4fee0 and 19f9271.

📒 Files selected for processing (3)
  • src/cli/config-command.ts
  • structure/config.md
  • tests/cli/cli-config-command.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

The config CLI now applies dedicated redaction to proxy values. Credential-bearing URLs lose userinfo while retaining host and port. direct and credential-less URLs remain unchanged. Tests cover show, get, and set output.

Changes

Proxy Credential Redaction

Layer / File(s) Summary
Proxy redaction contract
src/cli/config-command.ts, structure/config.md
redact now handles proxy values explicitly. It preserves empty and direct values, masks URL userinfo with redactUrlForLog, preserves host and port, and fully masks invalid non-URL values. The documentation states that config export retains raw configuration content.
Proxy redaction validation
tests/cli/cli-config-command.test.ts
The tests cover credentialed, direct, and credential-less proxies. They verify redaction in config show --json, config get, and JSON config set output.

Priority: ➖ Normal

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

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 19f92

CLI output now hides proxy credentials while preserving usable host and port information; direct and credential-less proxies remain unchanged. No actionable merge-blocking risk remains.

🚥 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 3 functions across 2 files. (1 skipped: 1 … 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: CLI proxy credentials are redacted in configuration output.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
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 3 functions across 2 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • 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.

@github-actions

github-actions Bot commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

✅ READY

  • all PR quality gates passed.

Hygiene

✅ Deterministic PR hygiene checks passed.

@github-actions
github-actions Bot marked this pull request as draft September 21, 2026 05:34
@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 66 / 80

ocx config show, get, set은 비밀번호처럼 보이는 키만 가렸습니다. 제공자 프록시 주소는 http://아이디:비밀번호@호스트:포트처럼 주소 안에 비밀번호가 들어가는데, 키 이름이 proxy라서 그 목록에 없었습니다. 그래서 화면에 비밀번호가 그대로 나왔습니다.

이 PR은 proxy를 비밀 키 목록에 넣습니다. 값이 빈 문자열이 아니면 ********로 바꿉니다. 테스트는 제공자 프록시에 비밀번호가 있을 때 show, get, set 출력에 그 비밀번호가 없는지 확인합니다. base는 dev입니다. types.ts와 config.ts를 나누는 작업이 아니고, 같은 수정의 다른 열린 PR은 없습니다.

라인 - src/cli/config-command.ts 25, 98: 키가 proxy이고 값이 비어 있지 않은 문자열이면 무조건 가립니다. 제공자 프록시의 "direct"는 비밀번호가 아닙니다. src/types/provider.ts 424행은 "direct"를 "이 제공자는 전역 프록시를 쓰지 않는다"는 뜻으로 적습니다. 지금은 ocx config show가 그 설정도 ********로 보여 줍니다. 비밀번호 없는 http://127.0.0.1:7890도 호스트와 포트를 잃습니다. 로그용 redactUrlForLog는 아이디와 비밀번호만 지우고 호스트는 남깁니다.

라인 - src/cli/config-command.ts 275-279: ocx config export는 redact를 호출하지 않고 설정 원문을 파일이나 표준 출력에 씁니다. PR 설명은 내보내기에서도 가려진다고 하는데, 이 PR은 그 경로를 고치지 않습니다. 테스트도 export를 실행하지 않습니다.

라인 - tests/cli/cli-config-command.test.ts 52: "direct"와 비밀번호 없는 주소를 보지 않습니다. 비밀번호가 있는 주소만 통과하면, "direct"가 가려지는 회귀는 잡히지 않습니다.

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

프록시 값 전체를 비밀로 볼지, 주소 안의 아이디와 비밀번호만 가릴지 정해 주세요. "direct"는 비밀이 아닙니다. export는 복원용이라 원문을 남기는 것이 맞는지, 화면과 같이 가려야 하는지도 정해 주세요. 가리면 그 파일로는 프록시 비밀번호를 되돌릴 수 없습니다. PR은 아직 초안입니다.

너의 추천

방향은 유지하면 됩니다. 주소 안에 있는 아이디와 비밀번호는 화면에 나오면 안 됩니다. 머지 전에 "direct"와 비밀번호 없는 주소는 그대로 보이게 하고, 비밀번호가 있는 주소만 아이디와 비밀번호를 지우세요. export를 일부러 원문으로 둔다면 PR 설명에서 export를 빼세요. 정말 내보내기에서도 가리려면 export 경로와 테스트를 추가하세요. 닫을 중복 PR은 없습니다.

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

@github-actions
github-actions Bot marked this pull request as ready for review September 21, 2026 06:24
@github-actions
github-actions Bot marked this pull request as draft September 21, 2026 13:23
@luvs01
luvs01 force-pushed the codex/fix-proxy-credential-leakage-in-cli branch from 1a4fee0 to 73946f3 Compare September 21, 2026 13:34
Mask only credential-bearing proxy URLs (userinfo stripped via redactUrlForLog, host/port kept); direct and credential-less values print unchanged. Clarify that config export stays raw for restore.
@luvs01

luvs01 commented Sep 21, 2026

Copy link
Copy Markdown
Collaborator Author

Applied on acdbb1c — took the recommended shape:

  • proxy is no longer blanket-masked. "direct", empty values, and credential-less URLs (e.g. http://127.0.0.1:7890) print unchanged; only a URL with userinfo is masked, via redactUrlForLog — userinfo/query/hash stripped, host and port kept so config show still says where traffic goes. A non-URL value that is not "direct" cannot be proven credential-free, so it fails closed to ******** (the provider schema already rejects malformed proxy values, so this is a defensive fallback).
  • config export stays raw: it is the restore path and redacting it would lose the proxy password. The SECRET_KEYS comment and structure/config.md now state that contract explicitly instead of claiming export is redacted.
  • tests/cli/cli-config-command.test.ts now covers credentialed, "direct", and credential-less values across show --json, get, and set --json (3 tests, 19 assertions, passing on this head).

@github-actions
github-actions Bot marked this pull request as ready for review September 22, 2026 04:43
@luvs01

luvs01 commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator Author

Consolidated into #5538 as a single related-function aggregate.

Source head: 19f92710ec2e0ab502e9c913c103733359336d30. Replacement head: 62c677c05d880a5c231bb4efd35b7f8bb637d16f.

Both unique source commits, including the review follow-up, match carried commits by stable Git patch ID. All affected runtime and test files match the source head exactly at the replacement head; merge-only source commits have no additional combined diff. The combined changed-file run passed 85 tests / 376 assertions; typecheck, structure, privacy and standalone size-ratchet checks passed. Full suite, hosted cross-platform CI and independent review remain pending.

Closing this duplicate standalone review entry at the author's request after verifying migration. This is not a merge or release claim; remaining integration checks and reviews are tracked on the draft replacement. Original branches are retained.

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