Skip to content

fix(config): avoid logging operator-supplied credential/group identifiers in credentialGroups diagnostics - #5427

Closed
luvs01 wants to merge 3 commits into
lidge-jun:devfrom
luvs01:fix/592-credential-group-log
Closed

luvs01 wants to merge 3 commits into
lidge-jun:devfrom
luvs01:fix/592-credential-group-log

Conversation

@luvs01

@luvs01 luvs01 commented Sep 21, 2026 •

Copy link
Copy Markdown
Collaborator

Motivation

  • credentialGroups diagnostics could echo operator-supplied credential or group identifiers into logs, leaking sensitive names into shared diagnostics.

Description

  • In src/routing/identity-domains.ts the credentialGroups warning path no longer interpolates operator-supplied credential/group identifiers; it reports the entry shape instead.
  • structure/catalog.md updated to match the new diagnostic surface.
  • Regression coverage in tests/config/config-load-degrade.test.ts asserts the malformed-credentialGroups warning never includes operator-supplied identifiers.

Testing

  • bun test tests/config/config-load-degrade.test.ts: 22 tests pass.

@coderabbitai

coderabbitai Bot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added bug Something isn't working review-ready labels Sep 21, 2026
@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.

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 63 / 80

설정에 pool.credentialGroups를 잘못 쓰면, 불러올 때 경고로 그 목록을 버립니다. 예전에는 경고 문장 안에 그룹 이름과 자격증명 문자열이 그대로 들어갔습니다. 설정 파일에 실수로 붙인 비밀이나, 이름만으로도 민감한 그룹 표기가 공유 로그에 남을 수 있었습니다.

이 PR은 credentialGroupIssues가 만드는 문장을 바꿉니다. 이제 그룹과 멤버의 순서 번호만 말하고, 운영자가 넣은 문자열은 넣지 않습니다. 같은 함수를 쓰는 쓰기 검사도 같은 문장을 씁니다. structure/catalog.md도 그에 맞게 고쳤습니다. 불러오기 테스트는 가짜 자격증명·그룹 이름이 경고에 안 나오는지 확인합니다. base는 dev입니다. 지금 tip은 403b6b2ac20d이고, 패키지는 @bitkyc08/opencodex 2.61.0입니다. 이 커밋은 tip보다 1개 앞이고, tip보다 1개 뒤처져 있습니다. 뒤처진 커밋은 #5425 테스트 수정입니다. GitHub에서는 아직 초안입니다. 본문 준비 체크는 네 칸 모두 표시되어 있고, review-ready 라벨이 붙어 있습니다. GitHub에 나온 검사는 hygiene, label, enforce-target, resolve-pr뿐입니다. CodeRabbit은 초안이라 건너뛰었습니다. types.ts와 config.ts를 나누는 변경은 아닙니다.

src/routing/identity-domains.ts:261 - 위 주석은 아직 "운영자가 읽을 수 있는 문장"이라고 적혀 있습니다. 문장은 이제 인덱스만 말합니다. 읽어 고칠 때는 맞지만, 주석과 실제가 어긋납니다.

src/config/load-degrade.ts:423 - 여기 주석은 예전처럼 문장에 JSON.stringify로 멤버가 들어간다고 적혀 있습니다. 이제는 인덱스로 바뀌었고, 그 위에 redactSecretString도 그대로 있습니다. 주석만 예전 이야기입니다.

tests/config/config-load-degrade.test.ts - 회귀는 "provider-qualified"가 깨진 경우만 봅니다. 같은 함수가 만드는 중복 id, 빈 목록, 두 그룹에 같은 멤버 문장도 식별자를 빼도록 바뀌었는데, 그 길은 경고에 안 나오는지 직접 확인하지 않습니다.

메인테이너의 판단이 필요한 지점
쓰기 검사(credentialGroupsSchema, poolCredentialGroupsError)도 같은 인덱스 문장을 씁니다. 로그 유출을 막으려면 맞습니다. 다만 설정을 고치는 사람에게는 그룹 이름이 없어 어디를 고칠지 덜 분명해집니다. 로드 경고만 인덱스로 두고, 쓰기 거절은 이름을 남길지입니다.
classifyCredential의 declaredGroupConflict는 여전히 그룹 id를 담습니다. 그 값이 다른 로그·진단으로 나가면, 이번 경고 경로만 막아도 이름은 남을 수 있습니다. 그 표면까지 같은 규칙으로 맞출지입니다.
아직 초안이고 tip보다 1커밋 뒤입니다. 로컬에서 bun test tests/config/config-load-degrade.test.ts 22개가 통과했다고 적혀 있습니다. 이 리뷰는 그 테스트를 다시 돌리지 않았습니다.

너의 추천
방향은 맞습니다. 로드 경고에서 운영자 문자열을 빼는 위치가 맞습니다. tip에 맞춘 뒤 초안을 풀고, GitHub 테스트가 보이면 머지를 판단하세요. 쓰기 거절 문장에 이름을 남길지는 그 전에 짧게 정하면 됩니다. 주석 두 줄과 나머지 이슈 형태 회귀는 머지를 막지 않습니다.

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

@luvs01

luvs01 commented Sep 21, 2026

Copy link
Copy Markdown
Collaborator Author

Review feedback applied on f8a67b3dc1:nn- Stale comments corrected: identity-domains.ts now says messages name positions only (not "operator-readable"), and load-degrade.ts describes the indexed messages with redaction as the second layer.n- New regression test covers the remaining issue shapes (duplicate group id, empty group, member listed twice) and asserts operator strings stay out of the warning.nnOn the two maintainer questions: the write-rejection keeps the same indexed messages (a config editor sees file positions, which is enough to locate the entry); declaredGroupConflict group ids are internal diagnostics, not the load-warning path this PR fixes - left as-is per scope.nnTests: config-load-degrade` 23 pass.

@luvs01

luvs01 commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator Author

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

Source head: 3716a8a3206f8857d40071c9acd4b1a38e40abce. 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