Skip to content

fix(usage): bound the usage-log permission hardening cache - #4976

Merged
lidge-jun merged 3 commits into
devfrom
codex/carry-4795-usage-log-perms
Sep 18, 2026
Merged

lidge-jun merged 3 commits into
devfrom
codex/carry-4795-usage-log-perms

Conversation

@lidge-jun

Copy link
Copy Markdown
Owner

Carries #4795 by @chilung-cgu, rebased onto current dev with the review finding fixed. Original commit authorship is preserved.

Summary

appendUsageEntry called mkdirSync and two chmodSync on every single append. The original PR removed that syscall storm by remembering the path it had already hardened.

The defect this carry fixes. That cache was keyed on the path alone and never expired, so after the first append the process would never narrow permissions again. An externally widened mode — 0755 on a directory that must be 0700, or a world-readable usage.jsonl — stayed widened for the life of the process. The perf win was real and the correctness loss was real, and they are separable: the cache now expires after one second. Consecutive appends still skip the syscalls; the first append at or after the boundary reapplies directory mode 0700 before the write and file mode 0600 after it. A path change, a clock rollback, or an ENOENT on append also invalidates it.

Security boundary. The usage log is an append-only ledger of request metadata and bounded account attribution, kept owner-only by POSIX mode on both the directory and the file. That mode is the whole boundary.

What this does and does not guarantee. It guarantees bounded, write-triggered repair: an external widening is corrected on the next append at or after the one-second boundary, and chmod remains best-effort on platforms that ignore it. It does not monitor the filesystem, does not stop another local process from widening the mode again immediately after a check, and does not narrow the window between appendFileSync creating the file and the following chmod. It adds no symlink or path-replacement defense beyond what was already there; those boundaries are unchanged.

Verification

Local verification was not run: this lane forbids running any local suite, typecheck, build, or install. Hosted CI on this PR head is the executable verification.

Static checks performed in place of local execution:

  • The original cache was traced against every exit path to confirm it suppressed both chmod calls permanently, not merely for a burst — the only invalidations were a path change and an ENOENT retry.
  • recordOwnedConfigPath was read to confirm it is idempotent and separately cached, so calling it once per second rather than once per append changes no ownership bookkeeping. Its own mkdirSync is guarded by an existence check, which is what makes the syscall-count assertion in the new test exact rather than approximate.
  • The new syscall-count test pins Date.now so it measures the cache rather than the runner's scheduling: five appends that straddled the one-second boundary would legitimately harden twice, and a saturated runner can take that long.
  • The permission re-narrowing test is skipped on Windows because POSIX mode bits do not carry there, not to avoid a failure; the repository already uses that guard for the same reason elsewhere.

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.

Co-authored-by: chilung b0423031@gmail.com

chilung-cgu and others added 3 commits September 18, 2026 08:19
…eanup

Co-authored-by: chilung <b0423031@gmail.com>
Keep consecutive appends off redundant filesystem hardening while reapplying owner-only modes after a one-second cache window.

Co-authored-by: chilung <b0423031@gmail.com>
@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner September 17, 2026 23:26
@coderabbitai

coderabbitai Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

Next included review available in 1 minute.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: c5e4ba1e-a86c-4f75-a42b-f93be214206a

📥 Commits

Reviewing files that changed from the base of the PR and between 61ee647 and 29ef1b7.

📒 Files selected for processing (4)
  • src/usage/log.ts
  • structure/gui-and-management-api.md
  • structure/runtime.md
  • tests/usage/usage-log.test.ts

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.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 17, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-17T23:29:04.072911Z 29ef1b7 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@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 17, 2026
@lidge-jun

Copy link
Copy Markdown
Owner Author

리뷰 · 우선순위 77 / 80

이 PR(#4976)은 기여자 chilung-cgu의 #4795를 현재 dev 위에 다시 올린 캐리(carry) 입니다. 작성자는 lidge-jun, 브랜치는 codex/carry-4795-usage-log-perms이고, 원본 커밋 저자 정보는 유지합니다. 바꾸는 곳은 src/usage/log.ts, tests/usage/usage-log.test.ts, 그리고 사용량 계약 문서 structure/gui-and-management-api.md / structure/runtime.md입니다. types.ts/config.ts 분할 캠페인과 무관하므로 close-don't-rebase 대상이 아닙니다.

지금 dev HEAD는 61ee64747 (패키지 2.59.0, tip #4948 cold status setup)입니다. 그 HEAD의 appendUsageEntry는 요청이 끝날 때마다 ensureUsageLogDir()mkdirSync + chmodSync(dir, 0o700)을 돌리고, appendFileSync 뒤에 chmodSync(path, 0o600)을 또 합니다. 동시 서브에이전트가 많으면 이 동기 파일시스템 호출이 Bun 이벤트 루프를 막아 디스패치가 늦어집니다. #4795가 그걸 고치려 경로만 기억해 두는 캐시를 넣었는데, 경로만 같고 만료가 없으면 프로세스 평생 권한을 다시 좁히지 않습니다. 밖에서 디렉터리를 0755로, usage.jsonl을 세계 읽기로 풀어 둬도 첫 append 이후에는 그대로 남습니다. 성능 이득과 보안 손실이 한 줄에 묶여 있었고, 이번 캐리는 그걸 분리합니다.

고치는 방법은 모듈 캐시 ensuredUsageLogDir / ensuredUsageLogFilepathcheckedAt을 같이 넣고, USAGE_LOG_PERMISSION_RECHECK_MS = 1000으로 묶는 것입니다. usageLogPermissionCheckIsCurrent는 경로가 같고, 시계가 뒤로 가지 않았고(now >= checkedAt), 아직 1초 안일 때만 “이미 확인함”으로 봅니다. 연속 append는 시스템 콜을 건너뛰고, 경계에 닿거나 넘은 첫 append는 디렉터리 0700을 쓰기 전에, 파일 0600을 쓰기 후에 다시 맞춥니다. 경로가 바뀌거나, 시계가 롤백되거나, append 중 ENOENT가 나면 캐시를 비우고 바로 다시 만듭니다. resetUsageReadCacheForTests()에도 두 캐시를 null로 돌려 테스트끼리 상태가 새지 않게 했습니다.

이 로그는 ~/.opencodex/usage.jsonl에 요청 메타와 계정 귀속만 쌓는 추가 전용(append-only) 장부입니다. POSIX 모드(디렉터리 소유자만 0700, 파일 소유자만 0600)가 그 경계의 전부입니다. PR 본문이 솔직히 적은 한계도 맞습니다. 쓰기 때 1초마다 고쳐 주는 것이지, 파일시스템을 계속 감시하지 않고, 다른 로컬 프로세스가 직후에 다시 풀어 놓는 것도 막지 않으며, appendFileSync가 파일을 만든 뒤 chmod하기 전 짧은 창도 그대로입니다. 심볼릭 링크·경로 교체 방어도 예전과 같습니다. 그래도 “영원히 안 고친다”에서 “최대 약 1초 뒤에 다시 좁힌다”로 바뀌는 것은 실사용에서 의미가 큽니다.

테스트는 세 갈래입니다. (1) Date.now를 고정한 채 5번 append하면 mkdirSync 1회·chmodSync 2회만 나오는지 잠급니다. 러너가 느려 1초를 넘기면 두 번 harden해도 정상이라, 시계를 고정하지 않으면 카운트가 흔들립니다. (2) Windows가 아닌 곳에서 디렉터리·파일을 풀어 둔 뒤 시계를 +1000ms 밀고 append하면 다시 0700/0600인지 확인합니다. Windows는 POSIX 모드가 안 먹어서 skipIf인데, 저장소 다른 곳과 같은 이유입니다. (3) 디렉터리를 지운 뒤 append해도 ENOENT로 캐시를 비우고 복구되는지 확인합니다. 문서도 “1초 캐시 + ENOENT 무효화 + 감시가 아님”을 계약 문장으로 적어 두어, 나중에 읽는 사람이 과대 해석하기 어렵게 했습니다. 로컬 스위트는 이 레인 규칙상 안 돌렸고, 호스티드 CI가 검증입니다. 지금 CI는 일부 성공·다수 진행 중이라 exact-head 초록을 기다려야 합니다.

라인 src/usage/log.ts usageLogPermissionCheckIsCurrent - 1초 창 안에는 외부에서 풀어 둔 모드가 그대로 남을 수 있다. 본문이 명시한 한계이지만, 장시간 idle 뒤 첫 append 전에도 풀어져 있으면 그 한 번까지는 남는다(다음 경계에서야 고쳐짐).
라인 src/usage/log.ts doAppend filePermissionsCurrent - 파일 chmod는 append 뒤에만 돈다. 새로 만들어지는 순간과 chmod 사이 창은 그대로다. 의도된 한계이나, “항상 0600이 보장된다”고 읽으면 과장이다.
라인 src/usage/log.ts ensureUsageLogDir - 1초에 한 번만 recordOwnedConfigPath를 부른다. 본문대로 멱등·별도 캐시라 소유권 장부는 깨지지 않아야 하지만, ownership 목록이 프로세스 중간에 비워지는 경로가 생기면 캐시와 목록이 어긋날 여지는 #4795 리뷰와 같다.
경로/심볼 tests Windows skip - POSIX 모드 재좁히기 테스트만 건너뛴다. Windows에서 best-effort chmod가 no-op인 건 기존과 같고, 회귀를 숨기려는 skip은 아니다.
경로/심볼 #4795 원본 - 원본 PR은 여전히 open이다. 이 캐리가 머지되면 Landed via #4976 at <commit> + landed-via-maintainer로 닫아야 open PR 수가 부풀지 않는다.

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

  • 1초 TTL이 보안·성능 균형으로 충분한지, 더 짧게(예: 250ms) 둘지
  • #4795를 이 PR 머지 직후 landed-via로 닫을지, 기여자에게 한 줄 감사 멘션을 남길지
  • CI exact-head가 초록이 되기 전에 tip-rebase가 필요한지(지금 base는 dev, HEAD 61ee64747와 맞춤)

너의 추천
호스티드 CI가 exact-head로 초록이면 머지한다. 머지 직후 #4795에 Landed via #4976 at <commit>를 남기고 landed-via-maintainer 라벨을 붙인 뒤 completed/superseded로 닫는다. 1초 창·append-후-chmod 한계는 본문·문서에 이미 적혀 있으니 추가 커밋은 필요 없다. types/config 분할과 무관하고, #4974 같은 정책 위반도 아니다.

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

@lidge-jun

Copy link
Copy Markdown
Owner Author

Merging with macOS legs outstanding, and recording why rather than leaving it implicit.

At this exact head the full Linux suite (test 1/4 through 4/4), gates, storage policy, enforce-target, the docs build, and the keyring and npm-global smokes are green. The macOS legs are queued behind a saturated hosted-runner pool shared by several concurrent lanes, and the sharded macOS legs are separately known to go silent mid-suite and be cancelled at their job budget — a long-standing defect recorded with six occurrences in #4956, including two from the 2.58.0 round that were previously written off as capacity.

This change is platform-neutral, so waiting on a queue that is both saturated and known-unreliable would delay the work without adding information. The evidence that governs the release is not per-PR macOS legs; it is the full-platform lane=all dispatch at the frozen release candidate, which is held until #4956 has a named cause. Nothing is promoted on the strength of this merge.

Stating the boundary plainly: this is merged on Linux, gates and cross-platform smoke evidence at its exact head, with macOS coverage deferred to the candidate run rather than claimed here.

@lidge-jun
lidge-jun merged commit fe0dfce into dev Sep 18, 2026
27 checks passed
@lidge-jun
lidge-jun deleted the codex/carry-4795-usage-log-perms branch September 18, 2026 00:02
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