Skip to content

fix(diagnostics): bound project config reads - #5367

Closed
luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:fix/config-read-bounds
Closed

luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:fix/config-read-bounds

Conversation

@luvs01

@luvs01 luvs01 commented Sep 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Close a local denial-of-service window in project-config diagnostics: discovery accepted any .codex/config.toml via existsSync and the collector synchronously readFileSync-read project configs with no type or size checks, so a symlinked special device or a huge file could hang or exhaust the process.
  • Add MAX_PROJECT_CONFIG_BYTES (1 MiB) and a readBoundedRegularFile helper that opens with O_NOFOLLOW, verifies a regular file within the size cap via fstat, and reads through a bounded descriptor, returning null on failure or oversize.
  • Harden discoverProjectCodexConfigPaths with an lstat check that rejects non-regular or oversized candidates before they enter discovery results.
  • Replace the unbounded readFileSync in collectProjectCodexConfigWarnings with the bounded reader so diagnostics never block or allocate unbounded memory.
  • Add regression tests asserting symlinked and oversized project configs are excluded from discovery.

Verification

  • bun test tests/codex-integration/project-config-warnings.test.ts — 28 pass, 0 fail (includes the two new regression tests).
  • bun x tsc --noEmit — clean.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

  • All CI tests are green on my local testing.

  • I pushed my PR to the latest dev commit.

  • I resolved all correct Codex and CodeRabbit findings.

  • My PR is ready for review.

@coderabbitai

coderabbitai Bot commented Sep 20, 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 the bug Something isn't working label Sep 20, 2026
@github-actions

github-actions Bot commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

✅ READY

  • all PR quality gates passed; the review readiness checklist is complete.

Review readiness checklist

  • ✅ All CI tests are green on my local testing.
  • ✅ I pushed my PR to the latest dev commit.
  • ✅ I resolved all correct Codex and CodeRabbit findings.
  • ✅ My PR is ready for review.

4/4 boxes ticked.

Automatic ready-for-review conversion failed; please mark the pull request ready manually if it is still a draft.
The review-ready label marks this PR as ready; review automation runs independently.
Maintainers notified: @lidge-jun @Ingwannu

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 67 / 80

이 PR은 프로젝트 .codex/config.toml을 진단할 때, 심볼릭 링크나 아주 큰 파일 때문에 프로세스가 멈추거나 메모리를 많이 쓰는 구멍을 줄입니다. 예전에는 파일이 “있다”만 보고 readFileSync로 끝까지 읽었어요. 지금은 발견 단계에서 lstat으로 일반 파일이고 1 MiB 이하인지 확인하고, 읽을 때는 O_NOFOLLOW로 연 뒤 fstat으로 다시 검사하고, 최대 1 MiB+1바이트까지만 읽습니다. 심볼릭 링크와 큰 파일을 발견 목록에서 빼는 테스트 두 개도 들어 있습니다. base는 dev이고, 작성자 기준으로 관련 bun 테스트 28개와 tsc는 통과했다고 합니다. 아직 draft이고 본문 체크리스트는 0/4입니다. types/config 분할이나 미리보기 배포 이야기는 해당 없습니다. 같은 주제로 겹치는 열린 PR은 없습니다.

라인 src/codex/project-config-warnings.ts readBoundedRegularFile — 열 때 O_NOFOLLOW만 켭니다. 같은 저장소의 src/codex/orca-auth-source.ts readBoundedLocalFile, src/codex/quota.ts, src/remote-control/workspace-executor.tsO_NONBLOCK도 같이 켭니다. 발견과 읽기 사이에 파일이 FIFO(파이프)로 바뀌면, 블록 없이 열지 않는 한 openSync가 끝없이 기다릴 수 있습니다. 이 PR이 막겠다고 한 “멈추기”가 그 경쟁에서는 남을 수 있습니다.

라인 src/codex/project-config-warnings.ts readBoundedRegularFile — 버퍼를 항상 MAX_PROJECT_CONFIG_BYTES + 1(약 1 MiB)로 allocUnsafe합니다. orca-auth-sourceshim-state-filefstat 크기(+1)만큼만 잡습니다. 진단은 캐시 TTL 안에서 여러 경로를 돌 수 있어서, 작은 설정 파일마다 1 MiB를 잡는 비용이 큽니다. Windows에서는 다른 codex 코드처럼 O_NOFOLLOW ?? 0 또는 win32에서 0으로 두는 가드가 없습니다.

라인 tests/codex-integration/project-config-warnings.test.ts — 새 테스트는 discoverProjectCodexConfigPaths만 봅니다. collectProjectCodexConfigWarningsreadBoundedRegularFile 경로, FIFO/O_NONBLOCK, 크기 경계에서 읽기가 null인 경우는 없습니다. 발견만 막아도 수집은 그 목록을 쓰므로 심볼릭·대형 제외는 대체로 잠기지만, 리더 자체의 멈추기/상한은 회귀로 안 잠깁니다.

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

전역 CODEX_HOME/config.toml은 여전히 크기·타입 검사 없는 readFileSync입니다. 프로젝트만 막을지, 전역도 같은 상한을 쓸지 범위를 정해 주세요. 상한을 넘는 프로젝트 설정을 조용히 건너뛸지, 진단 경고로 남길지도 정책입니다. 이름만 같은 readBoundedRegularFile이 shim·remote-control에도 있으니, 공용 헬퍼로 모을지 이 파일 전용으로 둘지도 정하면 좋습니다.

너의 추천

방향과 base dev는 맞습니다. 머지 전에 (1) O_NONBLOCK(플랫폼에서 없으면 0)을 열고, (2) 버퍼는 fstat 크기+1로 잡으며, (3) Windows O_NOFOLLOW 가드를 맞추세요. 가능하면 FIFO 또는 읽기 상한을 readBoundedRegularFile/수집 경로로 하나 더 잠그세요. 그 구멍이 닫히기 전에는 draft를 유지하고, 체크리스트는 그 다음에 채우면 됩니다. 무효·중복으로 닫을 다른 열린 PR은 없습니다.

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

Discovery accepted any .codex/config.toml via existsSync and the collector synchronously readFileSync-read project configs with no type or size checks, so a symlinked special device or a huge file could hang or exhaust the process. Open candidates with O_NOFOLLOW, verify a regular file under a 1 MiB cap with fstat, and read through a bounded descriptor; lstat rejects non-regular or oversized candidates before discovery.
@luvs01

luvs01 commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator Author

Consolidated into #5540 in native Stack #5505.

Source head: 585e5308eb02ec1631d5353acf79f0a507b0cb83. Replacement head: e352aee0343548a606a8d071b3504848c7503a41.

All 1 unique source contribution commit(s) match their carried commits by stable Git patch ID. The follow-up additionally prevents blocking FIFO opens, uses the observed file size, and rejects detected growth/truncation. Prepared aggregate checks passed 200 runtime/configuration tests and 20 GUI/locale tests. Combined route/probe rerun passed 119 tests with one POSIX skip; combined GUI tests, typecheck, structure, privacy and ratchet checks passed. Full cross-platform CI, POSIX execution, docs build 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 review-ready

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants